fix(AIP-136): handle LRO response names (#1391)

* fix(AIP-136): handle LRO response names * split LRO tests out for easier reading
googleapis · May 23, 2024 · ec79f53 · ec79f53
1 parent 4d25ba2
commit ec79f53
Show file tree

Hide file tree

Showing 4 changed files with 99 additions and 6 deletions.
diff --git a/rules/aip0136/response_message_name.go b/rules/aip0136/response_message_name.go
@@ -55,10 +55,15 @@ var responseMessageName = &lint.MethodRule{
 			return nil
 		}
 
+		loc := locations.MethodResponseType(m)
+		if utils.IsOperation(m.GetOutputType()) {
+			loc = locations.MethodOperationInfo(m)
+		}
+
 		return []lint.Problem{{
 			Message:    fmt.Sprintf(responseMessageNameErrorMessage, response.GetName()),
 			Descriptor: m,
-			Location:   locations.MethodResponseType(m),
+			Location:   loc,
 		}}
 
 	},

diff --git a/rules/aip0136/response_message_name_test.go b/rules/aip0136/response_message_name_test.go
@@ -38,9 +38,11 @@ func TestResponseMessageName(t *testing.T) {
 				file := testutils.ParseProto3Tmpl(t, `
 				package test;
 				import "google/api/resource.proto";
+
 				service Library {
 					rpc {{.MethodName}}({{.MethodName}}Request) returns ({{.RespMessageName}});
 				}
+
 				message {{.MethodName}}Request {}
 				message {{.RespMessageName}} {}
 				`, test)
@@ -53,6 +55,46 @@ func TestResponseMessageName(t *testing.T) {
 		}
 	})
 
+	t.Run("Response Suffix - LRO", func(t *testing.T) {
+		// Set up the testing permutations.
+		tests := []struct {
+			testName    string
+			MethodName  string
+			MessageName string
+			problems    testutils.Problems
+		}{
+			{"Valid", "ArchiveBook", "ArchiveBookResponse", testutils.Problems{}},
+			{"Invalid", "ArchiveBook", "ArchiveBookResp", testutils.Problems{{Message: "not \"ArchiveBookResp\"."}}},
+		}
+
+		for _, test := range tests {
+			t.Run(test.testName, func(t *testing.T) {
+				file := testutils.ParseProto3Tmpl(t, `
+				package test;
+				import "google/api/resource.proto";
+				import "google/longrunning/operations.proto";
+
+				service Library {
+					rpc {{.MethodName}}({{.MethodName}}Request) returns (google.longrunning.Operation) {
+						option (google.longrunning.operation_info) = {
+							response_type: "{{.MessageName}}"
+							metadata_type: "OperationMetadata"
+						};
+					};
+				}
+				message {{.MethodName}}Request {}
+				message {{.MessageName}} {}
+				message OperationMetadata {}
+				`, test)
+				method := file.GetServices()[0].GetMethods()[0]
+				problems := responseMessageName.Lint(file)
+				if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" {
+					t.Error(diff)
+				}
+			})
+		}
+	})
+
 	t.Run("Resource", func(t *testing.T) {
 		// Set up the testing permutations.
 		tests := []struct {

diff --git a/rules/internal/utils/common_lints.go b/rules/internal/utils/common_lints.go
@@ -181,12 +181,24 @@ func LintMethodHasMatchingRequestName(m *desc.MethodDescriptor) []lint.Problem {
 // LintMethodHasMatchingResponseName returns a problem if the given method's response type does not
 // have a name matching the method's, with a "Response" suffix.
 func LintMethodHasMatchingResponseName(m *desc.MethodDescriptor) []lint.Problem {
-	if got, want := m.GetOutputType().GetName(), m.GetName()+"Response"; got != want {
+	// GetResponseType handles the LRO case.
+	if got, want := GetResponseType(m).GetName(), m.GetName()+"Response"; got != want {
+		loc := locations.MethodResponseType(m)
+		suggestion := want
+
+		// If the RPC is an LRO, we need to tweak the finding.
+		if isLongRunningOperation(m.GetOutputType()) {
+			loc = locations.MethodOperationInfo(m)
+			// Clear the suggestion b.c we cannot easily pin point the
+			// response_type field.
+			suggestion = ""
+		}
+
 		return []lint.Problem{{
 			Message:    fmt.Sprintf("Response message should be named after the RPC, i.e. %q.", want),
-			Suggestion: want,
+			Suggestion: suggestion,
 			Descriptor: m,
-			Location:   locations.MethodResponseType(m),
+			Location:   loc,
 		}}
 	}
 	return nil

diff --git a/rules/internal/utils/common_lints_test.go b/rules/internal/utils/common_lints_test.go
@@ -204,21 +204,55 @@ func TestLintMethodHasMatchingRequestName(t *testing.T) {
 }
 
 func TestLintMethodHasMatchingResponseName(t *testing.T) {
+	for _, test := range []struct {
+		testName     string
+		ResponseName string
+		problems     testutils.Problems
+	}{
+		{"Valid", "GetBookResponse", nil},
+		{"Invalid", "AcquireBookResponse", testutils.Problems{{Suggestion: "GetBookResponse"}}},
+	} {
+		t.Run(test.testName, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				service Library {
+					rpc GetBook(GetBookRequest) returns ({{.ResponseName}});
+				}
+				message GetBookRequest {}
+				message {{.ResponseName}} {}
+			`, test)
+			method := f.GetServices()[0].GetMethods()[0]
+			problems := LintMethodHasMatchingResponseName(method)
+			if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" {
+				t.Error(diff)
+			}
+		})
+	}
+}
+
+func TestLintMethodHasMatchingResponseNameLRO(t *testing.T) {
 	for _, test := range []struct {
 		testName    string
 		MessageName string
 		problems    testutils.Problems
 	}{
 		{"Valid", "GetBookResponse", nil},
-		{"Invalid", "AcquireBookResponse", testutils.Problems{{Suggestion: "GetBookResponse"}}},
+		{"Invalid", "AcquireBookResponse", testutils.Problems{{Message: "GetBookResponse"}}},
 	} {
 		t.Run(test.testName, func(t *testing.T) {
 			f := testutils.ParseProto3Tmpl(t, `
+				import "google/longrunning/operations.proto";
+
 				service Library {
-					rpc GetBook(GetBookRequest) returns ({{.MessageName}});
+					rpc GetBook(GetBookRequest) returns (google.longrunning.Operation) {
+						option (google.longrunning.operation_info) = {
+							response_type: "{{.MessageName}}"
+							metadata_type: "OperationMetadata"
+						};
+					}
 				}
 				message GetBookRequest {}
 				message {{.MessageName}} {}
+				message OperationMetadata {}
 			`, test)
 			method := f.GetServices()[0].GetMethods()[0]
 			problems := LintMethodHasMatchingResponseName(method)