fix(AIP-203): lint non-maps and same package (#1175)

Although all request messages **must** be annotated, this linter only verifies messages that are in the same package as some upstream protos (e.g. common protos) may be difficult to modify.
googleapis · Jun 7, 2023 · 4e9eb29 · 4e9eb29
1 parent ca24295
commit 4e9eb29
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 10 deletions.
diff --git a/docs/rules/0203/field-behavior-required.md b/docs/rules/0203/field-behavior-required.md
@@ -23,6 +23,10 @@ This rule looks at all fields and ensures they have a
 one of the options `OUTPUT_ONLY`, `REQUIRED`, or `OPTIONAL`: all fields must
 fall into one of these categories.
 
+Although all request messages **must** be annotated, this linter only verifies
+messages that are in the same package as some upstream protos (e.g. common
+protos) may be difficult to modify.
+
 ## Examples
 
 **Incorrect** code for this rule:

diff --git a/rules/aip0203/field_behavior_required.go b/rules/aip0203/field_behavior_required.go
@@ -31,7 +31,8 @@ var fieldBehaviorRequired = &lint.MethodRule{
 	Name: lint.NewRuleName(203, "field-behavior-required"),
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		req := m.GetInputType()
-		ps := problems(req)
+		p := m.GetFile().GetPackage()
+		ps := problems(req, p)
 		if len(ps) == 0 {
 			return nil
 		}
@@ -40,7 +41,7 @@ var fieldBehaviorRequired = &lint.MethodRule{
 	},
 }
 
-func problems(m *desc.MessageDescriptor) []lint.Problem {
+func problems(m *desc.MessageDescriptor, pkg string) []lint.Problem {
 	var ps []lint.Problem
 
 	for _, f := range m.GetFields() {
@@ -53,8 +54,8 @@ func problems(m *desc.MessageDescriptor) []lint.Problem {
 			ps = append(ps, *p)
 		}
 
-		if mt := f.GetMessageType(); mt != nil {
-			ps = append(ps, problems(mt)...)
+		if mt := f.GetMessageType(); mt != nil && !mt.IsMapEntry() && mt.GetFile().GetPackage() == pkg {
+			ps = append(ps, problems(mt, pkg)...)
 		}
 	}
 

diff --git a/rules/aip0203/field_behavior_required_test.go b/rules/aip0203/field_behavior_required_test.go
@@ -42,6 +42,13 @@ func TestFieldBehaviorRequired_SingleFile_SingleMessage(t *testing.T) {
 			"int32 page_count = 1 [(google.api.field_behavior) = OPTIONAL];",
 			nil,
 		},
+		// Maps should not be recursed to MapEntries, as they have no field
+		// behavior.
+		{
+			"ValidMap",
+			"map<string, string> page_count = 1 [(google.api.field_behavior) = OPTIONAL];",
+			nil,
+		},
 		{
 			"ValidOutputOnly",
 			"int32 page_count = 1 [(google.api.field_behavior) = OUTPUT_ONLY];",
@@ -64,6 +71,8 @@ func TestFieldBehaviorRequired_SingleFile_SingleMessage(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			f := testutils.ParseProto3Tmpl(t, `
+				package apilinter.test.field_behavior_required;
+
 				import "google/api/field_behavior.proto";
 				import "google/api/resource.proto";
 
@@ -93,7 +102,6 @@ func TestFieldBehaviorRequired_SingleFile_SingleMessage(t *testing.T) {
 					string name = 1;
 				}
 			`, tc)
-
 			field := f.GetMessageTypes()[0].GetFields()[0]
 
 			if diff := tc.problems.SetDescriptor(field).Diff(fieldBehaviorRequired.Lint(f)); diff != "" {
@@ -226,6 +234,12 @@ func TestFieldBehaviorRequired_NestedMessages_MultipleFile(t *testing.T) {
 			"annotated",
 			nil,
 		},
+		{
+			"ValidAnnotatedAndChildInOtherPackageUnannotated",
+			"unannotated.NonAnnotated",
+			"non_annotated",
+			nil,
+		},
 		{
 			"InvalidChildNotAnnotated",
 			"NonAnnotated",
@@ -236,8 +250,11 @@ func TestFieldBehaviorRequired_NestedMessages_MultipleFile(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			f1 := `
+				package apilinter.test.field_behavior_required;
+
 				import "google/api/field_behavior.proto";
 				import "resource.proto";
+				import "unannotated.proto";
 
 				service Library {
 					rpc UpdateBook(UpdateBookRequest) returns (UpdateBookResponse) {
@@ -256,6 +273,8 @@ func TestFieldBehaviorRequired_NestedMessages_MultipleFile(t *testing.T) {
 			`
 
 			f2 := `
+				package apilinter.test.field_behavior_required;
+
 				import "google/api/field_behavior.proto";
 
 				message NonAnnotated {
@@ -267,9 +286,18 @@ func TestFieldBehaviorRequired_NestedMessages_MultipleFile(t *testing.T) {
 				}
 			`
 
+			f3 := `
+				package apilinter.test.unannotated;
+
+				message NonAnnotated {
+					string nested = 1;
+				}
+			`
+
 			srcs := map[string]string{
-				"service.proto":  f1,
-				"resource.proto": f2,
+				"service.proto":     f1,
+				"resource.proto":    f2,
+				"unannotated.proto": f3,
 			}
 
 			ds := testutils.ParseProto3Tmpls(t, srcs, tc)
@@ -281,9 +309,11 @@ func TestFieldBehaviorRequired_NestedMessages_MultipleFile(t *testing.T) {
 				t.Errorf(diff)
 			}
 
-			want := "resource.proto"
-			if got := fd.GetFile().GetName(); got != want {
-				t.Fatalf("got file name %q for location of field but wanted %q", got, want)
+			if tc.problems != nil {
+				want := "resource.proto"
+				if got := fd.GetFile().GetName(); got != want {
+					t.Fatalf("got file name %q for location of field but wanted %q", got, want)
+				}
 			}
 		})
 	}