Repository: jhump/protoreflect (go)

Issue:
protoparse: does not check for unique json_name in proto3 files
This is a case where protoparse accepts a proto source file that protoc rejects.

When a file uses `syntax = "proto3"`, protoc _requires_ that all fields have unique JSON names. So if the fields all collide on the same camel-case name (which is how protoc computes the JSON name from the proto field name), it complains:

```proto
// test.proto
syntax = "proto3";
message Foo {
  string bar = 1;
  string _Bar = 2;
}
```

Compiling with protoc yields this:
```
test.proto:5:10: The JSON camel-case name of field "_Bar" conflicts with field "bar". This is not allowed in proto3.
```

But protoparse does not perform this validation.

Note that protoparse should implement the checks _correctly_ and _not_ try to reproduce known bugs in protoc (https://github.com/protocolbuffers/protobuf/issues/5063 and https://github.com/protocolbuffers/protobuf/issues/7192)
(Which means protoparse will intentionally diverge from protoc behavior until these bugs are fixed.)

Pull request:
protoparse: enforce unique JSON names
Fixes #358 

This is done in a case-insensitive way, just as `protoc` does it. However, I wish I knew _why_ the protobuf team chose to make it case-insensitive. Case-sensitivity issues should generally be warnings or linter errors, not compile errors. 🤷 

Patch:
diff --git a/desc/protoparse/descriptor_protos.go b/desc/protoparse/descriptor_protos.go
index 740677d..aef2278 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 00ff620..7c15b7d 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 d80ebde..5cf2554 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,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
+}


Test patch:
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 0024119..2d44d3a 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)
+			}
+		}
+	}
+}