From 8bca1dd68ccf00c39a06da9862ac8c599029297e Mon Sep 17 00:00:00 2001 From: Noah Dietz Date: Fri, 20 Sep 2024 08:11:31 -0700 Subject: [PATCH] fix(AIP-162): handle LRO in response name rules (#1431) --- rules/aip0162/commit_response_message_name.go | 18 ++++++++-- .../commit_response_message_name_test.go | 35 +++++++++++++++++- .../delete_revision_response_message_name.go | 20 +++++++++-- ...ete_revision_response_message_name_test.go | 36 +++++++++++++++++++ .../aip0162/rollback_response_message_name.go | 17 ++++----- .../rollback_response_message_name_test.go | 3 +- .../tag_revision_response_message_name.go | 18 ++++++++-- ...tag_revision_response_message_name_test.go | 35 +++++++++++++++++- 8 files changed, 161 insertions(+), 21 deletions(-) diff --git a/rules/aip0162/commit_response_message_name.go b/rules/aip0162/commit_response_message_name.go index 99876929c..29c69031a 100644 --- a/rules/aip0162/commit_response_message_name.go +++ b/rules/aip0162/commit_response_message_name.go @@ -19,6 +19,7 @@ import ( "github.com/googleapis/api-linter/lint" "github.com/googleapis/api-linter/locations" + "github.com/googleapis/api-linter/rules/internal/utils" "github.com/jhump/protoreflect/desc" ) @@ -28,8 +29,19 @@ var commitResponseMessageName = &lint.MethodRule{ LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { // Rule check: Establish that for methods such as `CommitBook`, the response // message is `Book`. + response := utils.GetResponseType(m) + if response == nil { + return nil + } + got := response.GetName() want := commitMethodRegexp.FindStringSubmatch(m.GetName())[1] - got := m.GetOutputType().GetName() + loc := locations.MethodResponseType(m) + suggestion := want + + if utils.GetOperationInfo(m) != nil { + loc = locations.MethodOperationInfo(m) + suggestion = "" // We cannot offer a precise enough location to make a suggestion. + } // Return a problem if we did not get the expected return name. if got != want { @@ -39,9 +51,9 @@ var commitResponseMessageName = &lint.MethodRule{ want, got, ), - Suggestion: want, + Suggestion: suggestion, Descriptor: m, - Location: locations.MethodResponseType(m), + Location: loc, }} } return nil diff --git a/rules/aip0162/commit_response_message_name_test.go b/rules/aip0162/commit_response_message_name_test.go index 2ec5044ed..acd6437c5 100644 --- a/rules/aip0162/commit_response_message_name_test.go +++ b/rules/aip0162/commit_response_message_name_test.go @@ -33,7 +33,6 @@ func TestCommitResponseMessageName(t *testing.T) { } { t.Run(test.name, func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` - import "google/longrunning/operations.proto"; service Library { rpc {{.Method}}(CommitBookRequest) returns ({{.ResponseType}}); } @@ -47,3 +46,37 @@ func TestCommitResponseMessageName(t *testing.T) { }) } } + +func TestCommitResponseMessageNameLRO(t *testing.T) { + for _, test := range []struct { + name string + Method string + ResponseType string + problems testutils.Problems + }{ + {"Valid", "CommitBook", "Book", nil}, + {"Invalid", "CommitBook", "CommitBookResponse", testutils.Problems{{Message: "Book"}}}, + {"Irrelevant", "AcquireBook", "PurgeBooksResponse", nil}, + } { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/longrunning/operations.proto"; + service Library { + rpc {{.Method}}(CommitBookRequest) returns (google.longrunning.Operation) { + option (google.longrunning.operation_info) = { + response_type: "{{.ResponseType}}" + metadata_type: "OperationMetadata" + }; + }; + } + message CommitBookRequest {} + message {{.ResponseType}} {} + message OperationMetadata{} + `, test) + m := f.GetServices()[0].GetMethods()[0] + if diff := test.problems.SetDescriptor(m).Diff(commitResponseMessageName.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} diff --git a/rules/aip0162/delete_revision_response_message_name.go b/rules/aip0162/delete_revision_response_message_name.go index b574a06ea..8fb342f66 100644 --- a/rules/aip0162/delete_revision_response_message_name.go +++ b/rules/aip0162/delete_revision_response_message_name.go @@ -20,6 +20,7 @@ import ( "github.com/googleapis/api-linter/lint" "github.com/googleapis/api-linter/locations" + "github.com/googleapis/api-linter/rules/internal/utils" "github.com/jhump/protoreflect/desc" ) @@ -28,14 +29,27 @@ var deleteRevisionResponseMessageName = &lint.MethodRule{ Name: lint.NewRuleName(162, "delete-revision-response-message-name"), OnlyIf: isDeleteRevisionMethod, LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { - got := m.GetOutputType().GetName() + response := utils.GetResponseType(m) + if response == nil { + return nil + } + got := response.GetName() want := strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Revision"), "Delete") + + loc := locations.MethodResponseType(m) + suggestion := want + + if utils.GetOperationInfo(m) != nil { + loc = locations.MethodOperationInfo(m) + suggestion = "" // We cannot offer a precise enough location to make a suggestion. + } + if got != want { return []lint.Problem{{ Message: fmt.Sprintf("Delete Revision methods should return the resource itself (`%s`), not `%s`.", want, got), - Suggestion: want, + Suggestion: suggestion, Descriptor: m, - Location: locations.MethodResponseType(m), + Location: loc, }} } return nil diff --git a/rules/aip0162/delete_revision_response_message_name_test.go b/rules/aip0162/delete_revision_response_message_name_test.go index e5fd1faa0..e2de5c87c 100644 --- a/rules/aip0162/delete_revision_response_message_name_test.go +++ b/rules/aip0162/delete_revision_response_message_name_test.go @@ -48,3 +48,39 @@ func TestDeleteRevisionResponseMessageName(t *testing.T) { }) } } + +func TestDeleteRevisionResponseMessageNameLRO(t *testing.T) { + for _, test := range []struct { + name string + MethodName string + ResponseType string + problems testutils.Problems + }{ + {"Valid", "DeleteBookRevision", "Book", nil}, + {"Invalid", "DeleteBookRevision", "DeleteBookRevisionResponse", testutils.Problems{{Message: "Book"}}}, + {"Irrelevant", "DeleteBook", "DeleteBookResponse", nil}, + } { + t.Run(test.name, func(t *testing.T) { + file := testutils.ParseProto3Tmpl(t, ` + import "google/longrunning/operations.proto"; + service Library { + rpc {{.MethodName}}({{.MethodName}}Request) returns (google.longrunning.Operation) { + option (google.longrunning.operation_info) = { + response_type: "{{.ResponseType}}" + metadata_type: "OperationMetadata" + }; + }; + } + message {{.MethodName}}Request {} + message {{.ResponseType}} {} + message OperationMetadata {} + `, test) + + method := file.GetServices()[0].GetMethods()[0] + problems := deleteRevisionResponseMessageName.Lint(file) + if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" { + t.Error(diff) + } + }) + } +} diff --git a/rules/aip0162/rollback_response_message_name.go b/rules/aip0162/rollback_response_message_name.go index c778923f4..5a4fcc738 100644 --- a/rules/aip0162/rollback_response_message_name.go +++ b/rules/aip0162/rollback_response_message_name.go @@ -16,7 +16,6 @@ package aip0162 import ( "fmt" - "strings" "github.com/googleapis/api-linter/lint" "github.com/googleapis/api-linter/locations" @@ -31,15 +30,17 @@ var rollbackResponseMessageName = &lint.MethodRule{ // Rule check: Establish that for methods such as `RollbackBook`, the response // message is `Book`. want := rollbackMethodRegexp.FindStringSubmatch(m.GetName())[1] - got := m.GetOutputType().GetName() + response := utils.GetResponseType(m) + if response == nil { + return nil + } + got := response.GetName() loc := locations.MethodResponseType(m) + suggestion := want - // If LRO, check the response_type short name. - if utils.IsOperation(m.GetOutputType()) { - t := utils.GetOperationInfo(m).GetResponseType() - ndx := strings.LastIndex(t, ".") - got = t[ndx+1:] + if utils.GetOperationInfo(m) != nil { loc = locations.MethodOperationInfo(m) + suggestion = "" // We cannot offer a precise enough location to make a suggestion. } // Return a problem if we did not get the expected return name. @@ -50,7 +51,7 @@ var rollbackResponseMessageName = &lint.MethodRule{ want, got, ), - Suggestion: want, + Suggestion: suggestion, Descriptor: m, Location: loc, }} diff --git a/rules/aip0162/rollback_response_message_name_test.go b/rules/aip0162/rollback_response_message_name_test.go index c0fcd89d1..87e801f2f 100644 --- a/rules/aip0162/rollback_response_message_name_test.go +++ b/rules/aip0162/rollback_response_message_name_test.go @@ -33,7 +33,6 @@ func TestRollbackResponseMessageName(t *testing.T) { } { t.Run(test.name, func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` - import "google/longrunning/operations.proto"; service Library { rpc {{.Method}}(RollbackBookRequest) returns ({{.ResponseType}}); } @@ -56,7 +55,7 @@ func TestRollbackOperationResponse(t *testing.T) { problems testutils.Problems }{ {"Valid", "RollbackBook", "Book", nil}, - {"Invalid", "RollbackBook", "RollbackBookResponse", testutils.Problems{{Suggestion: "Book"}}}, + {"Invalid", "RollbackBook", "RollbackBookResponse", testutils.Problems{{Message: "Book"}}}, {"Irrelevant", "AcquireBook", "PurgeBooksResponse", nil}, } { t.Run(test.name, func(t *testing.T) { diff --git a/rules/aip0162/tag_revision_response_message_name.go b/rules/aip0162/tag_revision_response_message_name.go index 9c01214d3..091bd1030 100644 --- a/rules/aip0162/tag_revision_response_message_name.go +++ b/rules/aip0162/tag_revision_response_message_name.go @@ -19,6 +19,7 @@ import ( "github.com/googleapis/api-linter/lint" "github.com/googleapis/api-linter/locations" + "github.com/googleapis/api-linter/rules/internal/utils" "github.com/jhump/protoreflect/desc" ) @@ -29,7 +30,18 @@ var tagRevisionResponseMessageName = &lint.MethodRule{ // Rule check: Establish that for methods such as `TagBookRevision`, the response // message is `Book`. want := tagRevisionMethodRegexp.FindStringSubmatch(m.GetName())[1] - got := m.GetOutputType().GetName() + response := utils.GetResponseType(m) + if response == nil { + return nil + } + got := response.GetName() + loc := locations.MethodResponseType(m) + suggestion := want + + if utils.GetOperationInfo(m) != nil { + loc = locations.MethodOperationInfo(m) + suggestion = "" // We cannot offer a precise enough location to make a suggestion. + } // Return a problem if we did not get the expected return name. if got != want { @@ -39,9 +51,9 @@ var tagRevisionResponseMessageName = &lint.MethodRule{ want, got, ), - Suggestion: want, + Suggestion: suggestion, Descriptor: m, - Location: locations.MethodResponseType(m), + Location: loc, }} } return nil diff --git a/rules/aip0162/tag_revision_response_message_name_test.go b/rules/aip0162/tag_revision_response_message_name_test.go index 25bbd2fd8..5fe4de036 100644 --- a/rules/aip0162/tag_revision_response_message_name_test.go +++ b/rules/aip0162/tag_revision_response_message_name_test.go @@ -33,7 +33,6 @@ func TestTagRevisionResponseMessageName(t *testing.T) { } { t.Run(test.name, func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` - import "google/longrunning/operations.proto"; service Library { rpc {{.Method}}(TagBookRevisionRequest) returns ({{.ResponseType}}); } @@ -47,3 +46,37 @@ func TestTagRevisionResponseMessageName(t *testing.T) { }) } } + +func TestTagRevisionResponseMessageNameLRO(t *testing.T) { + for _, test := range []struct { + name string + Method string + ResponseType string + problems testutils.Problems + }{ + {"Valid", "TagBookRevision", "Book", nil}, + {"Invalid", "TagBookRevision", "TagBookRevisionResponse", testutils.Problems{{Message: "Book"}}}, + {"Irrelevant", "AcquireBook", "PurgeBooksResponse", nil}, + } { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/longrunning/operations.proto"; + service Library { + rpc {{.Method}}(TagBookRevisionRequest) returns (google.longrunning.Operation) { + option (google.longrunning.operation_info) = { + response_type: "{{.ResponseType}}" + metadata_type: "OperationMetadata" + }; + }; + } + message TagBookRevisionRequest {} + message {{.ResponseType}} {} + message OperationMetadata {} + `, test) + m := f.GetServices()[0].GetMethods()[0] + if diff := test.problems.SetDescriptor(m).Diff(tagRevisionResponseMessageName.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +}