fix(AIP-203): field-behavior-required ignore imported request types (#1504)

fixes a bug where the method input type used as the starting point for the `field-behavior-required` recursion did not check if the input type itself was from the same package. It is not a super common practice in general, but appears a lot for APIs that mix in the google.iam.v1 IAM Policy service RPCs and reuse their request messages. Same thing could happen with `google.api.HttpBody` which is sometimes used as a RPC input type.

Updates #1503

Author: noahdietz

rules/aip0203/field_behavior_required.go

@@ -49,6 +49,12 @@ var fieldBehaviorRequired = &lint.MethodRule{
 func problems(m *desc.MessageDescriptor, pkg string, visited map[desc.Descriptor]bool) []lint.Problem {
 	var ps []lint.Problem
 
+	// Ensure the input type, or recursively visited message, is part of the
+	// same package before linting.
+	if m.GetFile().GetPackage() != pkg {
+		return nil
+	}
+
 	for _, f := range m.GetFields() {
 		// ignore the field if it was already visited
 		if ok := visited[f]; ok {
@@ -68,7 +74,7 @@ func problems(m *desc.MessageDescriptor, pkg string, visited map[desc.Descriptor
 			}
 		}
 
-		if mt := f.GetMessageType(); mt != nil && !mt.IsMapEntry() && mt.GetFile().GetPackage() == pkg {
+		if mt := f.GetMessageType(); mt != nil && !mt.IsMapEntry() {
 			ps = append(ps, problems(mt, pkg, visited)...)
 		}
 	}

rules/aip0203/field_behavior_required_test.go

@@ -253,26 +253,37 @@ func TestFieldBehaviorRequired_NestedMessages_MultipleFile(t *testing.T) {
 		name             string
 		MessageType      string
 		MessageFieldName string
+		RequestMessage   string
 		problems         testutils.Problems
 	}{
 		{
 			"ValidAnnotatedAndChildAnnotated",
 			"Annotated",
 			"annotated",
+			"UpdateBookRequest",
 			nil,
 		},
 		{
 			"ValidAnnotatedAndChildInOtherPackageUnannotated",
 			"unannotated.NonAnnotated",
 			"non_annotated",
+			"UpdateBookRequest",
 			nil,
 		},
 		{
 			"InvalidChildNotAnnotated",
 			"NonAnnotated",
 			"non_annotated",
+			"UpdateBookRequest",
 			testutils.Problems{{Message: "must be set"}},
 		},
+		{
+			"SkipRequestInOtherPackageUnannotated",
+			"Annotated",                // set this so that the template compiles
+			"annotated",                // set this so that the template compiles
+			"unannotated.NonAnnotated", // unannotated message as request
+			nil,
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -284,7 +295,7 @@ func TestFieldBehaviorRequired_NestedMessages_MultipleFile(t *testing.T) {
 				import "unannotated.proto";
 
 				service Library {
-					rpc UpdateBook(UpdateBookRequest) returns (UpdateBookResponse) {
+					rpc UpdateBook({{.RequestMessage}}) returns (UpdateBookResponse) {
 					}
 				}
 
@@ -317,7 +328,11 @@ func TestFieldBehaviorRequired_NestedMessages_MultipleFile(t *testing.T) {
 				package apilinter.test.unannotated;
 
 				message NonAnnotated {
-					string nested = 1;
+					OtherNonAnnotated nested = 1;
+				}
+
+				message OtherNonAnnotated {
+					string foo = 1;
 				}
 			`