fix(AIP-133/AIP-134): handle qualified lro response type name compari…

…son (#1475)
googleapis · Feb 19, 2025 · 5e8fe24 · 5e8fe24
1 parent b9ecfd1
commit 5e8fe24
Show file tree

Hide file tree

Showing 6 changed files with 150 additions and 14 deletions.
diff --git a/rules/aip0133/response_message_name.go b/rules/aip0133/response_message_name.go
@@ -30,12 +30,14 @@ var outputName = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		want := utils.GetResourceMessageName(m, "Create")
 
-		// If this is an LRO, then use the annotated response type instead of
-		// the actual RPC return type.
-		got := m.GetOutputType().GetName()
-		if utils.IsOperation(m.GetOutputType()) {
-			got = utils.GetOperationInfo(m).GetResponseType()
+		// Load the response type, resolving the
+		// `google.longrunning.OperationInfo.response_type` if necessary.
+		resp := utils.GetResponseType(m)
+		if resp == nil {
+			// If we can't resolve it, let the AIP-151 rule warn about this.
+			return nil
 		}
+		got := resp.GetName()
 
 		// Rule check: Establish that for methods such as `CreateFoo`, the response
 		// message should be named `Foo`

diff --git a/rules/aip0133/response_message_name_test.go b/rules/aip0133/response_message_name_test.go
@@ -40,7 +40,7 @@ func TestOutputMessageName(t *testing.T) {
 	// Run each test individually.
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
-			// Create a minimal service with a AIP-134 Update method
+			// Create a minimal service with a AIP-133 Create method
 			file := testutils.ParseProto3Tmpl(t, `
 				import "google/longrunning/operations.proto";
 				service Library {
@@ -68,3 +68,61 @@ func TestOutputMessageName(t *testing.T) {
 		})
 	}
 }
+
+func TestResponseMessageName_FullyQualified(t *testing.T) {
+	for _, test := range []struct {
+		name              string
+		TypePkg           string
+		ServicePkg        string
+		TypeName          string
+		ResponseTypeValue string
+		problems          testutils.Problems
+	}{
+		{
+			name:       "ValidLocalImport",
+			TypePkg:    "library",
+			ServicePkg: "library",
+			TypeName:   "Book",
+			problems:   nil,
+		},
+		{
+			name:       "ValidXPkgImport",
+			TypePkg:    "other",
+			ServicePkg: "library",
+			TypeName:   "Book",
+			problems:   nil,
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			files := testutils.ParseProto3Tmpls(t, map[string]string{
+				"type.proto": `
+			package {{.TypePkg}};
+	
+			message {{.TypeName}} {}
+			`,
+				"service.proto": `
+			package {{.ServicePkg}};
+	
+			import "google/longrunning/operations.proto";
+			import "type.proto";
+	
+			service Foo {
+				rpc Create{{.TypeName}} (Create{{.TypeName}}Request) returns (google.longrunning.Operation) {
+					option (google.longrunning.operation_info) = {
+					response_type: "{{.TypePkg}}.{{.TypeName}}"
+					metadata_type: "Create{{.TypeName}}Metadata"
+					};
+				}
+			}
+			message Create{{.TypeName}}Request {}
+			message Create{{.TypeName}}Metadata {}
+			`,
+			}, test)
+			file := files["service.proto"]
+			got := outputName.Lint(file)
+			if diff := test.problems.SetDescriptor(file.GetServices()[0].GetMethods()[0]).Diff(got); diff != "" {
+				t.Error(diff)
+			}
+		})
+	}
+}
diff --git a/rules/aip0134/response_message_name.go b/rules/aip0134/response_message_name.go
@@ -32,12 +32,15 @@ var responseMessageName = &lint.MethodRule{
 		// Rule check: Establish that for methods such as `UpdateFoo`, the response
 		// message is `Foo` or `google.longrunning.Operation`.
 		want := strings.Replace(m.GetName(), "Update", "", 1)
-		got := m.GetOutputType().GetName()
 
-		// If the return type is an LRO, use the annotated response type instead.
-		if utils.IsOperation(m.GetOutputType()) {
-			got = utils.GetOperationInfo(m).GetResponseType()
+		// Load the response type, resolving the
+		// `google.longrunning.OperationInfo.response_type` if necessary.
+		resp := utils.GetResponseType(m)
+		if resp == nil {
+			// If we can't resolve it, let the AIP-151 rule warn about this.
+			return nil
 		}
+		got := resp.GetName()
 
 		// Return a problem if we did not get the expected return name.
 		//

diff --git a/rules/aip0134/response_message_name_test.go b/rules/aip0134/response_message_name_test.go
@@ -68,3 +68,61 @@ func TestResponseMessageName(t *testing.T) {
 		})
 	}
 }
+
+func TestResponseMessageName_FullyQualified(t *testing.T) {
+	for _, test := range []struct {
+		name              string
+		TypePkg           string
+		ServicePkg        string
+		TypeName          string
+		ResponseTypeValue string
+		problems          testutils.Problems
+	}{
+		{
+			name:       "ValidLocalImport",
+			TypePkg:    "library",
+			ServicePkg: "library",
+			TypeName:   "Book",
+			problems:   nil,
+		},
+		{
+			name:       "ValidXPkgImport",
+			TypePkg:    "other",
+			ServicePkg: "library",
+			TypeName:   "Book",
+			problems:   nil,
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			files := testutils.ParseProto3Tmpls(t, map[string]string{
+				"type.proto": `
+			package {{.TypePkg}};
+	
+			message {{.TypeName}} {}
+			`,
+				"service.proto": `
+			package {{.ServicePkg}};
+	
+			import "google/longrunning/operations.proto";
+			import "type.proto";
+	
+			service Foo {
+				rpc Update{{.TypeName}} (Update{{.TypeName}}Request) returns (google.longrunning.Operation) {
+					option (google.longrunning.operation_info) = {
+					response_type: "{{.TypePkg}}.{{.TypeName}}"
+					metadata_type: "Update{{.TypeName}}Metadata"
+					};
+				}
+			}
+			message Update{{.TypeName}}Request {}
+			message Update{{.TypeName}}Metadata {}
+			`,
+			}, test)
+			file := files["service.proto"]
+			got := responseMessageName.Lint(file)
+			if diff := test.problems.SetDescriptor(file.GetServices()[0].GetMethods()[0]).Diff(got); diff != "" {
+				t.Error(diff)
+			}
+		})
+	}
+}
diff --git a/rules/internal/utils/find.go b/rules/internal/utils/find.go
@@ -25,10 +25,19 @@ import (
 // FindMessage looks for a message in a file and all imports within the
 // same package.
 func FindMessage(f *desc.FileDescriptor, name string) *desc.MessageDescriptor {
+	// Default to using the current file's package.
+	pkg := f.GetPackage()
+
 	// FileDescriptor.FindMessage requires fully-qualified message names;
 	// attempt to infer that.
-	if !strings.Contains(name, ".") && f.GetPackage() != "" {
-		name = f.GetPackage() + "." + name
+	if !strings.Contains(name, ".") {
+		if pkg != "" {
+			name = pkg + "." + name
+		}
+	} else if !strings.HasPrefix(name, pkg+".") {
+		// If value is fully qualified, but from a different package,
+		// accommodate that.
+		pkg = name[:strings.LastIndex(name, ".")]
 	}
 
 	// Attempt to find the message in the file provided.
@@ -39,7 +48,7 @@ func FindMessage(f *desc.FileDescriptor, name string) *desc.MessageDescriptor {
 	// Attempt to find the message in any dependency files if they are in the
 	// same package.
 	for _, dep := range f.GetDependencies() {
-		if f.GetPackage() == dep.GetPackage() {
+		if pkg == dep.GetPackage() {
 			if m := FindMessage(dep, name); m != nil {
 				return m
 			}

diff --git a/rules/internal/utils/find_test.go b/rules/internal/utils/find_test.go
@@ -41,7 +41,13 @@ func TestFindMessage(t *testing.T) {
 		t.Errorf("Got nil, expected Book message.")
 	}
 	if scroll := FindMessage(files["c.proto"], "Scroll"); scroll != nil {
-		t.Errorf("Got Sctoll message, expected nil.")
+		t.Errorf("Got Scroll message, expected nil.")
+	}
+	if book := FindMessage(files["c.proto"], "test.Book"); book == nil {
+		t.Errorf("Got nil, expected Book message from qualified name.")
+	}
+	if scroll := FindMessage(files["c.proto"], "other.Scroll"); scroll == nil {
+		t.Errorf("Got nil message, expected Scroll message from qualified name.")
 	}
 }