From 9ca20337246712bbf25bab7f99f733c99955407c Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Tue, 16 Aug 2022 12:26:18 -0400 Subject: [PATCH 1/2] enforce unique JSON names, case-insensitive --- desc/protoparse/descriptor_protos.go | 2 +- desc/protoparse/errors.go | 10 +- desc/protoparse/linker.go | 103 +++++++++++++++- desc/protoparse/linker_test.go | 171 +++++++++++++++++++++++++++ 4 files changed, 282 insertions(+), 4 deletions(-) diff --git a/desc/protoparse/descriptor_protos.go b/desc/protoparse/descriptor_protos.go index 740677da..aef22787 100644 --- a/desc/protoparse/descriptor_protos.go +++ b/desc/protoparse/descriptor_protos.go @@ -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 { diff --git a/desc/protoparse/errors.go b/desc/protoparse/errors.go index 00ff6209..7c15b7d4 100644 --- a/desc/protoparse/errors.go +++ b/desc/protoparse/errors.go @@ -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 @@ -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...)} } diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go index d80ebdee..646bd097 100644 --- a/desc/protoparse/linker.go +++ b/desc/protoparse/linker.go @@ -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 @@ -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)) } } } @@ -1111,3 +1115,100 @@ 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() { + // found it + return true + } + } + return false +} diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go index 00241195..2d44d3a8 100644 --- a/desc/protoparse/linker_test.go +++ b/desc/protoparse/linker_test.go @@ -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) { @@ -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) + } + } + } +} From 69c0288e5536bd116d57c9e4ee0697ad31a1daae Mon Sep 17 00:00:00 2001 From: Joshua Humphries Date: Tue, 16 Aug 2022 21:03:36 -0400 Subject: [PATCH 2/2] remove superfluous comment --- desc/protoparse/linker.go | 1 - 1 file changed, 1 deletion(-) diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go index 646bd097..5cf2554e 100644 --- a/desc/protoparse/linker.go +++ b/desc/protoparse/linker.go @@ -1206,7 +1206,6 @@ func hasCustomJsonName(res *parseResult, fd *desc.FieldDescriptor) bool { if len(opt.Name.Parts) == 1 && opt.Name.Parts[0].Name.AsIdentifier() == "json_name" && !opt.Name.Parts[0].IsExtension() { - // found it return true } }