Skip to content

Commit

Permalink
protoparse: enforce unique JSON names (#523)
Browse files Browse the repository at this point in the history 
- this is (strangely?) a case-insensitive check, just like protoc enforces
- this enforces *default* JSON names don't conflict for proto3 files (even for fields that define a custom JSON name)
- this also adds checks that custom JSON names don't conflict (unlike protoc)
- this will only warn for proto2 files unless the issue is between conflicting *custom* names (JSON mapping was introduced in proto3, so not backwards compatible to fail due to default JSON names in proto2 files)
  • Loading branch information
@jhump
jhump committed Aug 17, 2022
1 parent 7614117 commit 3057e78
Show file tree
Hide file tree
Showing 4 changed files with 281 additions and 4 deletions.
2 changes: 1 addition & 1 deletion desc/protoparse/descriptor_protos.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func (r *parseResult) createFileDescriptor(filename string, file *ast.FileNode)
fd.Syntax = proto.String(file.Syntax.Syntax.AsString())
}
} else {
r.errs.warn(file.Start(), ErrNoSyntax)
r.errs.warnWithPos(file.Start(), ErrNoSyntax)
}

for _, decl := range file.Decls {
Expand Down
10 changes: 8 additions & 2 deletions desc/protoparse/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,18 @@ func (h *errorHandler) handleError(err error) error {
return err
}

func (h *errorHandler) warn(pos *SourcePos, err error) {
func (h *errorHandler) warnWithPos(pos *SourcePos, err error) {
if h.warnReporter != nil {
h.warnReporter(ErrorWithSourcePos{Pos: pos, Underlying: err})
}
}

func (h *errorHandler) warn(err ErrorWithSourcePos) {
if h.warnReporter != nil {
h.warnReporter(err)
}
}

func (h *errorHandler) getError() error {
if h.errsReported > 0 && h.err == nil {
return ErrInvalidSource
Expand Down Expand Up @@ -138,7 +144,7 @@ func (e ErrorWithSourcePos) Unwrap() error {

var _ ErrorWithPos = ErrorWithSourcePos{}

func errorWithPos(pos *SourcePos, format string, args ...interface{}) ErrorWithPos {
func errorWithPos(pos *SourcePos, format string, args ...interface{}) ErrorWithSourcePos {
return ErrorWithSourcePos{Pos: pos, Underlying: fmt.Errorf(format, args...)}
}

Expand Down
102 changes: 101 additions & 1 deletion desc/protoparse/linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ func (l *linker) linkFiles() (map[string]*desc.FileDescriptor, error) {
if err := l.checkExtensionsInFile(fd, r); err != nil {
return nil, err
}
// and final check for json name configuration
if err := l.checkJsonNamesInFile(fd, r); err != nil {
return nil, err
}
}

// When Parser calls linkFiles, it does not check errs again, and it expects that linkFiles
Expand Down Expand Up @@ -1051,7 +1055,7 @@ func (l *linker) checkForUnusedImports(filename string) {
if pos == nil {
pos = ast.UnknownPos(r.fd.GetName())
}
l.errs.warn(pos, errUnusedImport(dep))
l.errs.warnWithPos(pos, errUnusedImport(dep))
}
}
}
Expand Down Expand Up @@ -1111,3 +1115,99 @@ func (l *linker) checkExtension(fld *desc.FieldDescriptor, res *parseResult) err

return nil
}

func (l *linker) checkJsonNamesInFile(fd *desc.FileDescriptor, res *parseResult) error {
for _, md := range fd.GetMessageTypes() {
if err := l.checkJsonNamesInMessage(md, res); err != nil {
return err
}
}
return nil
}

func (l *linker) checkJsonNamesInMessage(md *desc.MessageDescriptor, res *parseResult) error {
if err := checkJsonNames(md, res, false); err != nil {
return err
}
if err := checkJsonNames(md, res, true); err != nil {
return err
}
return nil
}

func checkJsonNames(md *desc.MessageDescriptor, res *parseResult, useCustom bool) error {
type seenName struct {
source *dpb.FieldDescriptorProto
// field's original JSON nane (which can differ in case from map key)
orig string
// true if orig is a custom JSON name (vs. the field's default JSON name)
custom bool
}
seen := map[string]seenName{}

for _, fd := range md.GetFields() {
scope := "field " + md.GetName() + "." + fd.GetName()
defaultName := internal.JsonName(fd.GetName())
name := defaultName
custom := false
if useCustom {
n := fd.GetJSONName()
if n != defaultName || hasCustomJsonName(res, fd) {
name = n
custom = true
}
}
lcaseName := strings.ToLower(name)
if existing, ok := seen[lcaseName]; ok {
// When useCustom is true, we'll only report an issue when a conflict is
// due to a custom name. That way, we don't double report conflicts on
// non-custom names.
if !useCustom || custom || existing.custom {
fldNode := res.getFieldNode(fd.AsFieldDescriptorProto())
customStr, srcCustomStr := "custom", "custom"
if !custom {
customStr = "default"
}
if !existing.custom {
srcCustomStr = "default"
}
otherName := ""
if name != existing.orig {
otherName = fmt.Sprintf(" %q", existing.orig)
}
conflictErr := errorWithPos(fldNode.Start(), "%s: %s JSON name %q conflicts with %s JSON name%s of field %s, defined at %v",
scope, customStr, name, srcCustomStr, otherName, existing.source.GetName(), res.getFieldNode(existing.source).Start())

// Since proto2 did not originally have default JSON names, we report conflicts involving
// default names as just warnings.
if !md.IsProto3() && (!custom || !existing.custom) {
res.errs.warn(conflictErr)
} else if err := res.errs.handleError(conflictErr); err != nil {
return err
}
}
} else {
seen[lcaseName] = seenName{orig: name, source: fd.AsFieldDescriptorProto(), custom: custom}
}
}
return nil
}

func hasCustomJsonName(res *parseResult, fd *desc.FieldDescriptor) bool {
// if we have the AST, we can more precisely determine if there was a custom
// JSON named defined, even if it is explicitly configured to tbe the same
// as the default JSON name for the field.
fdProto := fd.AsFieldDescriptorProto()
opts := res.getFieldNode(fdProto).GetOptions()
if opts == nil {
return false
}
for _, opt := range opts.Options {
if len(opt.Name.Parts) == 1 &&
opt.Name.Parts[0].Name.AsIdentifier() == "json_name" &&
!opt.Name.Parts[0].IsExtension() {
return true
}
}
return false
}
171 changes: 171 additions & 0 deletions desc/protoparse/linker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1006,6 +1006,115 @@ func TestLinkerValidation(t *testing.T) {
},
"foo.proto:4:3: field Foo.e: google.protobuf.Struct.FieldsEntry is a synthetic map entry and may not be referenced explicitly",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string foo = 1;\n" +
" string bar = 2 [json_name=\"foo\"];\n" +
"}\n",
},
"foo.proto:4:3: field Foo.bar: custom JSON name \"foo\" conflicts with default JSON name of field foo, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string foo = 1 [json_name=\"foo_bar\"];\n" +
" string bar = 2 [json_name=\"Foo_Bar\"];\n" +
"}\n",
},
"foo.proto:4:3: field Foo.bar: custom JSON name \"Foo_Bar\" conflicts with custom JSON name \"foo_bar\" of field foo, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string fooBar = 1;\n" +
" string foo_bar = 2;\n" +
"}\n",
},
"foo.proto:4:3: field Foo.foo_bar: default JSON name \"fooBar\" conflicts with default JSON name of field fooBar, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string fooBar = 1;\n" +
" string foo_bar = 2 [json_name=\"fuber\"];\n" +
"}\n",
},
"foo.proto:4:3: field Foo.foo_bar: default JSON name \"fooBar\" conflicts with default JSON name of field fooBar, defined at foo.proto:3:3",
}, {
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string fooBar = 1;\n" +
" string FOO_BAR = 2;\n" +
"}\n",
},
"foo.proto:4:3: field Foo.FOO_BAR: default JSON name \"FOOBAR\" conflicts with default JSON name \"fooBar\" of field fooBar, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto3\";\n" +
"message Foo {\n" +
" string fooBar = 1;\n" +
" string __foo_bar = 2;\n" +
"}\n",
},
"foo.proto:4:3: field Foo.__foo_bar: default JSON name \"FooBar\" conflicts with default JSON name \"fooBar\" of field fooBar, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string foo = 1 [json_name=\"foo_bar\"];\n" +
" optional string bar = 2 [json_name=\"Foo_Bar\"];\n" +
"}\n",
},
"foo.proto:4:3: field Foo.bar: custom JSON name \"Foo_Bar\" conflicts with custom JSON name \"foo_bar\" of field foo, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string fooBar = 1;\n" +
" optional string foo_bar = 2;\n" +
"}\n",
},
"", // should succeed: only check default JSON names in proto3
},
{
map[string]string{
"foo.proto": "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string fooBar = 1 [json_name=\"fooBar\"];\n" +
" optional string foo_bar = 2 [json_name=\"fooBar\"];\n" +
"}\n",
},
"foo.proto:4:3: field Foo.foo_bar: custom JSON name \"fooBar\" conflicts with custom JSON name of field fooBar, defined at foo.proto:3:3",
},
{
map[string]string{
"foo.proto": "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string fooBar = 1;\n" +
" optional string FOO_BAR = 2;\n" +
"}\n",
},
"", // should succeed: only check default JSON names in proto3
},
{
map[string]string{
"foo.proto": "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string fooBar = 1;\n" +
" optional string __foo_bar = 2;\n" +
"}\n",
},
"", // should succeed: only check default JSON names in proto3
},
}
for i, tc := range testCases {
acc := func(filename string) (io.ReadCloser, error) {
Expand Down Expand Up @@ -1180,3 +1289,65 @@ func TestSyntheticOneOfCollisions(t *testing.T) {
}
testutil.Eq(t, actual, expected)
}

func TestCustomJSONNameWarnings(t *testing.T) {
testCases := []struct {
source string
warning string
}{
{
source: "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string foo = 1;\n" +
" optional string bar = 2 [json_name=\"foo\"];\n" +
"}\n",
warning: "test.proto:4:3: field Foo.bar: custom JSON name \"foo\" conflicts with default JSON name of field foo, defined at test.proto:3:3",
},
{
source: "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string foo_bar = 1;\n" +
" optional string fooBar = 2;\n" +
"}\n",
warning: "test.proto:4:3: field Foo.fooBar: default JSON name \"fooBar\" conflicts with default JSON name of field foo_bar, defined at test.proto:3:3",
},
{
source: "syntax = \"proto2\";\n" +
"message Foo {\n" +
" optional string foo_bar = 1;\n" +
" optional string fooBar = 2;\n" +
"}\n",
warning: "test.proto:4:3: field Foo.fooBar: default JSON name \"fooBar\" conflicts with default JSON name of field foo_bar, defined at test.proto:3:3",
},
}
for i, tc := range testCases {
acc := func(filename string) (io.ReadCloser, error) {
if filename == "test.proto" {
return ioutil.NopCloser(strings.NewReader(tc.source)), nil
}
return nil, fmt.Errorf("file not found: %s", filename)
}
var warnings []string
warnFunc := func(err ErrorWithPos) {
warnings = append(warnings, err.Error())
}
_, err := Parser{Accessor: acc, WarningReporter: warnFunc}.ParseFiles("test.proto")
if err != nil {
t.Errorf("case %d: expecting no error; instead got error %q", i, err)
}
if tc.warning == "" && len(warnings) > 0 {
t.Errorf("case %d: expecting no warnings; instead got: %v", i, warnings)
} else {
found := false
for _, w := range warnings {
if w == tc.warning {
found = true
break
}
}
if !found {
t.Errorf("case %d: expecting warning %q; instead got: %v", i, tc.warning, warnings)
}
}
}
}

0 comments on commit 3057e78

Please sign in to comment.