Repository: jhump/protoreflect
Pull request #424 (2021-10-01 01:21:18)

protoparse: support for groups in options is lacking
There were numerous issues in protoparse when source used groups in custom options.

These fixes should address https://github.com/bufbuild/buf/issues/620.

In addition to the issue in that bug report (about how protoc expects the group name, not the lower-cased field name, in the text format for message literals in options), there was also an issue when an option that used the selector operator (to set a field _inside_ a custom option field that is a message) would be rejected the left-hand side was a group -- it was incorrectly treating group fields as if they were _not_ message types.

The new test cases added verify that in all of these situations, protoparse now matches the behavior of protoc.


Patch:
diff --git a/desc/protoparse/options.go b/desc/protoparse/options.go
index 1d7ac00..81352f4 100644
--- a/desc/protoparse/options.go
+++ b/desc/protoparse/options.go
@@ -1069,7 +1069,7 @@ func interpretField(res *parseResult, mc *messageContext, element descriptorish,
 	if len(opt.GetName()) > nameIndex+1 {
 		nextnm := opt.GetName()[nameIndex+1]
 		nextnode := res.getOptionNamePartNode(nextnm)
-		if fld.GetType() != dpb.FieldDescriptorProto_TYPE_MESSAGE {
+		if fld.GetType() != dpb.FieldDescriptorProto_TYPE_MESSAGE && fld.GetType() != dpb.FieldDescriptorProto_TYPE_GROUP {
 			return nil, res.errs.handleErrorWithPos(nextnode.Start(),
 				"%vcannot set field %s because %s is not a message",
 				mc, nextnm.GetNamePart(), nm.GetNamePart())
@@ -1339,6 +1339,23 @@ func fieldValue(res *parseResult, mc *messageContext, fld fldDescriptorish, val
 					}
 				} else {
 					ffld = fmd.FindFieldByName(a.Name.Value())
+					// Groups are indicated in the text format by the group name (which is
+					// camel-case), NOT the field name (which is lower-case).
+					// ...but only regular fields, not extensions that are groups...
+					if ffld != nil && ffld.GetType() == dpb.FieldDescriptorProto_TYPE_GROUP && ffld.GetMessageType().GetName() != a.Name.Value() {
+						// this is kind of silly to fail here, but this mimics protoc behavior
+						return nil, errorWithPos(val.Start(), "%vfield %s not found (did you mean the group named %s?)", mc, a.Name.Value(), ffld.GetMessageType().GetName())
+					}
+					if ffld == nil {
+						// could be a group name
+						for _, fd := range fmd.GetFields() {
+							if fd.GetType() == dpb.FieldDescriptorProto_TYPE_GROUP && fd.GetMessageType().GetName() == a.Name.Value() {
+								// found it!
+								ffld = fd
+								break
+							}
+						}
+					}
 				}
 				if ffld == nil {
 					return nil, errorWithPos(val.Start(), "%vfield %s not found", mc, string(a.Name.Name.AsIdentifier()))


Test patch:
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index ae3add9..68d0b88 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -420,6 +420,98 @@ func TestLinkerValidation(t *testing.T) {
 			},
 			"foo.proto:1:95: field com.google.Foo.str: unknown type google.protobuf.StringValue; resolved to com.google.protobuf.StringValue which is not defined; consider using a leading dot",
 		},
+		{
+			map[string]string{
+				"foo.proto": "syntax = \"proto2\";\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message Foo {\n" +
+					"  optional group Bar = 1 { optional string name = 1; }\n" +
+					"}\n" +
+					"extend google.protobuf.MessageOptions { optional Foo foo = 10001; }\n" +
+					"message Baz { option (foo).bar.name = \"abc\"; }\n",
+			},
+			"", // should succeed
+		},
+		{
+			map[string]string{
+				"foo.proto": "syntax = \"proto2\";\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message Foo {\n" +
+					"  optional group Bar = 1 { optional string name = 1; }\n" +
+					"}\n" +
+					"extend google.protobuf.MessageOptions { optional Foo foo = 10001; }\n" +
+					"message Baz { option (foo).Bar.name = \"abc\"; }\n",
+			},
+			"foo.proto:7:28: message Baz: option (foo).Bar.name: field Bar of Foo does not exist",
+		},
+		{
+			map[string]string{
+				"foo.proto": "syntax = \"proto2\";\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"extend google.protobuf.MessageOptions {\n" +
+					"  optional group Foo = 10001 { optional string name = 1; }\n" +
+					"}\n" +
+					"message Bar { option (foo).name = \"abc\"; }\n",
+			},
+			"", // should succeed
+		},
+		{
+			map[string]string{
+				"foo.proto": "syntax = \"proto2\";\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"extend google.protobuf.MessageOptions {\n" +
+					"  optional group Foo = 10001 { optional string name = 1; }\n" +
+					"}\n" +
+					"message Bar { option (Foo).name = \"abc\"; }\n",
+			},
+			"foo.proto:6:22: message Bar: invalid extension: Foo is a message, not an extension",
+		},
+		{
+			map[string]string{
+				"foo.proto": "syntax = \"proto2\";\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message Foo {\n" +
+					"  optional group Bar = 1 { optional string name = 1; }\n" +
+					"}\n" +
+					"extend google.protobuf.MessageOptions { optional Foo foo = 10001; }\n" +
+					"message Baz { option (foo) = { Bar< name: \"abc\" > }; }\n",
+			},
+			"", // should succeed
+		},
+		{
+			map[string]string{
+				"foo.proto": "syntax = \"proto2\";\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message Foo {\n" +
+					"  optional group Bar = 1 { optional string name = 1; }\n" +
+					"}\n" +
+					"extend google.protobuf.MessageOptions { optional Foo foo = 10001; }\n" +
+					"message Baz { option (foo) = { bar< name: \"abc\" > }; }\n",
+			},
+			"foo.proto:7:30: message Baz: option (foo): field bar not found (did you mean the group named Bar?)",
+		},
+		{
+			map[string]string{
+				"foo.proto": "syntax = \"proto2\";\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message Foo { extensions 1 to 10; }\n" +
+					"extend Foo { optional group Bar = 10 { optional string name = 1; } }\n" +
+					"extend google.protobuf.MessageOptions { optional Foo foo = 10001; }\n" +
+					"message Baz { option (foo) = { [bar]< name: \"abc\" > }; }\n",
+			},
+			"", // should succeed
+		},
+		{
+			map[string]string{
+				"foo.proto": "syntax = \"proto2\";\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message Foo { extensions 1 to 10; }\n" +
+					"extend Foo { optional group Bar = 10 { optional string name = 1; } }\n" +
+					"extend google.protobuf.MessageOptions { optional Foo foo = 10001; }\n" +
+					"message Baz { option (foo) = { [Bar]< name: \"abc\" > }; }\n",
+			},
+			"foo.proto:6:30: message Baz: option (foo): field Bar not found",
+		},
 	}
 	for i, tc := range testCases {
 		acc := func(filename string) (io.ReadCloser, error) {