diff --git a/rules/aip0136/response_message_name.go b/rules/aip0136/response_message_name.go index b6d43270..eaab9160 100644 --- 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 index 88013a1b..fe2cb1a0 100644 --- 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 index 4c71e3a4..69a8ddac 100644 --- 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 index 8e8c477b..9e07ee1d 100644 --- 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)