From 62a90e55f9e291f934e3e13597302e5423ef17c4 Mon Sep 17 00:00:00 2001 From: Matt Birchler Date: Wed, 15 May 2024 11:09:00 -0400 Subject: [PATCH 1/9] feat(aip-0136): response message name This patch implements the guidelines around the response message name for custom methods Ref: https://google.aip.dev/136 --- docs/rules/0136/response-message-name.md | 113 ++++++++++++++++++++ rules/aip0136/aip0136.go | 1 + rules/aip0136/response_message_name.go | 64 +++++++++++ rules/aip0136/response_message_name_test.go | 83 ++++++++++++++ 4 files changed, 261 insertions(+) create mode 100644 docs/rules/0136/response-message-name.md create mode 100644 rules/aip0136/response_message_name.go create mode 100644 rules/aip0136/response_message_name_test.go diff --git a/docs/rules/0136/response-message-name.md b/docs/rules/0136/response-message-name.md new file mode 100644 index 000000000..85790de48 --- /dev/null +++ b/docs/rules/0136/response-message-name.md @@ -0,0 +1,113 @@ +--- +rule: + aip: 136 + name: [core, '0136', response-message-name] + summary: Custom methods must have standardized response message names. +permalink: /136/response-message-name +redirect_from: + - /0136/response-message-name +--- + +# Custom methods: Response message + +This rule enforces that all custom methods should take a response message +matching the RPC name, with a `Response` suffix, or the resource being operated +on [AIP-136][]. + +## Details + +This rule looks at any method that is not a standard method, and complains if +the name of the corresponding input message does not match the name of the RPC +with the suffix `Response` appended, or the resource itself. It will use the +`(google.api.resource_reference)` on the first field of the request message to +determine the resource name that should be used. + +## Examples + +### Response Suffix + +**Incorrect** code for this rule: + +```proto +// Incorrect. +// Should be `ArchiveBookResponse`. +rpc ArchiveBook(ArchiveBookRequest) returns (ArchiveBookResp) { + option (google.api.http) = { + post: "/v1/{name=publishers/*/books/*}:archive" + body: "*" + }; +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +rpc ArchiveBook(ArchiveBookRequest) returns (ArchiveBookResponse) { + option (google.api.http) = { + post: "/v1/{name=publishers/*/books/*}:archive" + body: "*" + }; + +``` + +### Resource + +**Incorrect** code for this rule: + +```proto +// Incorrect. +// Should be `Book`. +rpc ArchiveBook(ArchiveBookRequest) returns (Author) { + option (google.api.http) = { + post: "/v1/{name=publishers/*/books/*}:archive" + body: "*" + }; +} + +message ArchiveBookRequest { + // The book to archive. + // Format: publishers/{publisher}/books/{book} + string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"]; +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +rpc ArchiveBook(ArchiveBookRequest) returns (Book) { + option (google.api.http) = { + post: "/v1/{name=publishers/*/books/*}:archive" + body: "*" + }; +} + +message ArchiveBookRequest { + // The book to archive. + // Format: publishers/{publisher}/books/{book} + string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"]; +} +``` + +## Disabling + +If you need to violate this rule, use a leading comment above the method. +Remember to also include an [aip.dev/not-precedent][] comment explaining why. + +```proto +// (-- api-linter: core::0136::response-message-name=disabled +// aip.dev/not-precedent: We need to do this because reasons. --) +rpc ArchiveBook(ArchiveBookRequest) returns (ArchiveBookResp) { + option (google.api.http) = { + post: "/v1/{name=publishers/*/books/*}:archive" + body: "*" + }; +} +``` + +If you need to violate this rule for an entire file, place the comment at the +top of the file. + +[aip-136]: https://aip.dev/136 +[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/rules/aip0136/aip0136.go b/rules/aip0136/aip0136.go index f6cd025e4..e5e4a2834 100644 --- a/rules/aip0136/aip0136.go +++ b/rules/aip0136/aip0136.go @@ -31,6 +31,7 @@ func AddRules(r lint.RuleRegistry) error { httpBody, httpMethod, noPrepositions, + responseMessageName, standardMethodsOnly, uriSuffix, verbNoun, diff --git a/rules/aip0136/response_message_name.go b/rules/aip0136/response_message_name.go new file mode 100644 index 000000000..149098d02 --- /dev/null +++ b/rules/aip0136/response_message_name.go @@ -0,0 +1,64 @@ +// Copyright 2019 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package aip0136 + +import ( + "fmt" + + "bitbucket.org/creachadair/stringset" + "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" +) + +// Custom methods should return a response message matching the RPC name, +// with a Response suffix. +var responseMessageName = &lint.MethodRule{ + Name: lint.NewRuleName(135, "response-message-name"), + OnlyIf: isCustomMethod, + LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { + matchingResponseName := m.GetName() + "Response" + suggestion := matchingResponseName + + got := m.GetOutputType().GetName() + want := stringset.New(matchingResponseName) + + // Get the resource name from the first request message field, if it + // is a resource reference + if len(m.GetInputType().GetFields()) > 0 { + rr := utils.GetResourceReference(m.GetInputType().GetFields()[0]) + if _, resourceMsgNameSingular, ok := utils.SplitResourceTypeName(rr.GetType()); ok { + want.Add(resourceMsgNameSingular) + suggestion += " or " + resourceMsgNameSingular + } + } + + if !want.Contains(got) { + msg := "Custom methods should return a response message matching the RPC name, with a Response suffix, or the resource, not %q." + + problem := lint.Problem{ + Message: fmt.Sprintf(msg, got), + Suggestion: suggestion, + Descriptor: m, + Location: locations.MethodResponseType(m), + } + + return []lint.Problem{problem} + } + + return nil + }, +} diff --git a/rules/aip0136/response_message_name_test.go b/rules/aip0136/response_message_name_test.go new file mode 100644 index 000000000..bd0221fa5 --- /dev/null +++ b/rules/aip0136/response_message_name_test.go @@ -0,0 +1,83 @@ +// Copyright 2019 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package aip0136 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +func TestResponseMessageName(t *testing.T) { + // Set up the testing permutations. + tests := []struct { + testName string + MethodName string + RespMessageName string + OperatingOnResource bool + problems testutils.Problems + }{ + {"Valid Resource", "ArchiveBook", "Book", true, testutils.Problems{}}, + {"Invalid Resource", "ArchiveBook", "Author", true, testutils.Problems{{Suggestion: "ArchiveBookResponse or Book"}}}, + {"Valid Response Suffix Stateful", "ArchiveBook", "ArchiveBookResponse", true, testutils.Problems{}}, + {"Valid Response Suffix Stateless", "ArchiveBook", "ArchiveBookResponse", false, testutils.Problems{}}, + {"Invalid Response Suffix", "ArchiveBook", "ArchiveBookResp", true, testutils.Problems{{Suggestion: "ArchiveBookResponse or Book"}}}, + {"Unable To Find Resource", "ArchiveBook", "ArchiveBookResp", false, testutils.Problems{{Suggestion: "ArchiveBookResponse"}}}, + {"Irrelevant", "DeleteBook", "DeleteBookResp", true, testutils.Problems{}}, + } + + // Run each test individually. + for _, test := range tests { + t.Run(test.testName, func(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 { + {{ if (.OperatingOnResource) }} + // The book to archive. + // Format: publishers/{publisher}/books/{book} + string name = 1 [ + (google.api.resource_reference) = { + type: "library.googleapis.com/Book" + }]; + {{ end }} + } + message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "publishers/{publisher}/books/{book}" + }; + } + message Author { + option (google.api.resource) = { + type: "library.googleapis.com/Author" + pattern: "authors/{author}" + }; + } + {{ if and (ne .RespMessageName "Book") (ne .RespMessageName "Author") }} + message {{.RespMessageName}} {} + {{ end }} + `, test) + method := file.GetServices()[0].GetMethods()[0] + problems := responseMessageName.Lint(file) + if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" { + t.Error(diff) + } + }) + } +} From ba25dab627eb35bb30ccb45e115aa395569c089a Mon Sep 17 00:00:00 2001 From: Matt Birchler Date: Fri, 17 May 2024 13:05:22 -0400 Subject: [PATCH 2/9] fix(AIP-136): improve logic --- docs/rules/0136/response-message-name.md | 80 +++++++--- rules/aip0136/response_message_name.go | 55 ++++--- rules/aip0136/response_message_name_test.go | 161 +++++++++++++------- 3 files changed, 197 insertions(+), 99 deletions(-) diff --git a/docs/rules/0136/response-message-name.md b/docs/rules/0136/response-message-name.md index 85790de48..db349de48 100644 --- a/docs/rules/0136/response-message-name.md +++ b/docs/rules/0136/response-message-name.md @@ -17,10 +17,13 @@ on [AIP-136][]. ## Details This rule looks at any method that is not a standard method, and complains if -the name of the corresponding input message does not match the name of the RPC -with the suffix `Response` appended, or the resource itself. It will use the -`(google.api.resource_reference)` on the first field of the request message to -determine the resource name that should be used. +the name of the corresponding output message does not match the name of the RPC +with the suffix `Response` appended, or the resource being operated on. + +**Note:** To identify the resource being operated on, the rule inspects the +name path parameter, which maps to the `name` field on the input type checking +that the resource message derived from `(google.api.resource_reference).type` +matches the response resource. ## Examples @@ -30,10 +33,10 @@ determine the resource name that should be used. ```proto // Incorrect. -// Should be `ArchiveBookResponse`. -rpc ArchiveBook(ArchiveBookRequest) returns (ArchiveBookResp) { +// Should be `TranslateTextResponse`. +rpc TranslateText(TranslateTextRequest) returns (Text) { option (google.api.http) = { - post: "/v1/{name=publishers/*/books/*}:archive" + post: "/v1/{project=projects/*}:translateText" body: "*" }; } @@ -43,12 +46,12 @@ rpc ArchiveBook(ArchiveBookRequest) returns (ArchiveBookResp) { ```proto // Correct. -rpc ArchiveBook(ArchiveBookRequest) returns (ArchiveBookResponse) { +rpc TranslateText(TranslateTextRequest) returns (TranslateTextResponse) { option (google.api.http) = { - post: "/v1/{name=publishers/*/books/*}:archive" + post: "/v1/{project=projects/*}:translateText" body: "*" }; - +} ``` ### Resource @@ -66,16 +69,15 @@ rpc ArchiveBook(ArchiveBookRequest) returns (Author) { } message ArchiveBookRequest { - // The book to archive. - // Format: publishers/{publisher}/books/{book} - string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"]; + // The book to archive. + // Format: publishers/{publisher}/books/{book} + string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"]; } ``` **Correct** code for this rule: ```proto -// Correct. rpc ArchiveBook(ArchiveBookRequest) returns (Book) { option (google.api.http) = { post: "/v1/{name=publishers/*/books/*}:archive" @@ -84,9 +86,53 @@ rpc ArchiveBook(ArchiveBookRequest) returns (Book) { } message ArchiveBookRequest { - // The book to archive. - // Format: publishers/{publisher}/books/{book} - string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"]; + // The book to archive. + // Format: publishers/{publisher}/books/{book} + string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"]; +} +``` + +### Long Running Operation + +**Incorrect** code for this rule: + +```proto +// Incorrect. +// Should be `Book` from Operation. +rpc ArchiveBook(ArchiveBookRequest) returns (google.longrunning.Operation) { + option (google.api.http) = { + post: "/v1/{name=publishers/*/books/*}:archive" + body: "*" + }; + option (google.longrunning.operation_info) = { + response_type: "Author" + } +} + +message ArchiveBookRequest { + // The book to archive. + // Format: publishers/{publisher}/books/{book} + string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"]; +} +``` + +**Correct** code for this rule: + +```proto +rpc ArchiveBook(ArchiveBookRequest) returns (google.longrunning.Operation) { + option (google.api.http) = { + post: "/v1/{name=publishers/*/books/*}:archive" + body: "*" + }; + option (google.longrunning.operation_info) = { + response_type: "Book" + } +} + +message ArchiveBookRequest { + // The book to archive. + // Format: publishers/{publisher}/books/{book} + string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"]; } ``` diff --git a/rules/aip0136/response_message_name.go b/rules/aip0136/response_message_name.go index 149098d02..c790abe2b 100644 --- a/rules/aip0136/response_message_name.go +++ b/rules/aip0136/response_message_name.go @@ -17,7 +17,6 @@ package aip0136 import ( "fmt" - "bitbucket.org/creachadair/stringset" "github.com/googleapis/api-linter/lint" "github.com/googleapis/api-linter/locations" "github.com/googleapis/api-linter/rules/internal/utils" @@ -25,40 +24,46 @@ import ( ) // Custom methods should return a response message matching the RPC name, -// with a Response suffix. +// with a Response suffix, or the resource being operated on. var responseMessageName = &lint.MethodRule{ - Name: lint.NewRuleName(135, "response-message-name"), + Name: lint.NewRuleName(136, "response-message-name"), OnlyIf: isCustomMethod, LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { - matchingResponseName := m.GetName() + "Response" - suggestion := matchingResponseName + // A response is considered valid if + // - The response name matches the RPC name with a `Response` suffix + // - The response is the resource being operated on + // To identify the resource being operated on, we inspect the Resource + // Reference of the input type's `name` field. This guidance is documented + // in https://google.aip.dev/136#resource-based-custom-methods - got := m.GetOutputType().GetName() - want := stringset.New(matchingResponseName) + response := utils.GetResponseType(m) + requestType := utils.GetResourceReference(m.GetInputType().FindFieldByName("name")).GetType() + resourceOperatedOn := utils.FindResourceMessage(requestType, m.GetFile()) - // Get the resource name from the first request message field, if it - // is a resource reference - if len(m.GetInputType().GetFields()) > 0 { - rr := utils.GetResourceReference(m.GetInputType().GetFields()[0]) - if _, resourceMsgNameSingular, ok := utils.SplitResourceTypeName(rr.GetType()); ok { - want.Add(resourceMsgNameSingular) - suggestion += " or " + resourceMsgNameSingular - } + // Output type has `Response` suffix + if len(utils.LintMethodHasMatchingResponseName(m)) == 0 { + return nil } - if !want.Contains(got) { - msg := "Custom methods should return a response message matching the RPC name, with a Response suffix, or the resource, not %q." + // Response is the resource being operated on + if response == resourceOperatedOn { + return nil + } - problem := lint.Problem{ - Message: fmt.Sprintf(msg, got), - Suggestion: suggestion, - Descriptor: m, - Location: locations.MethodResponseType(m), - } + msg := "Custom methods should return a message matching the RPC name, with a `Response` suffix, or the resource being operated on, not %q." + got := m.GetName() - return []lint.Problem{problem} + suggestion := got + "Response" + if utils.IsResource(response) && resourceOperatedOn != nil { + suggestion += " or " + resourceOperatedOn.GetName() } - return nil + return []lint.Problem{{ + Message: fmt.Sprintf(msg, got), + Suggestion: suggestion, + Descriptor: m, + Location: locations.MethodResponseType(m), + }} + }, } diff --git a/rules/aip0136/response_message_name_test.go b/rules/aip0136/response_message_name_test.go index bd0221fa5..c5306ee46 100644 --- a/rules/aip0136/response_message_name_test.go +++ b/rules/aip0136/response_message_name_test.go @@ -21,63 +21,110 @@ import ( ) func TestResponseMessageName(t *testing.T) { - // Set up the testing permutations. - tests := []struct { - testName string - MethodName string - RespMessageName string - OperatingOnResource bool - problems testutils.Problems - }{ - {"Valid Resource", "ArchiveBook", "Book", true, testutils.Problems{}}, - {"Invalid Resource", "ArchiveBook", "Author", true, testutils.Problems{{Suggestion: "ArchiveBookResponse or Book"}}}, - {"Valid Response Suffix Stateful", "ArchiveBook", "ArchiveBookResponse", true, testutils.Problems{}}, - {"Valid Response Suffix Stateless", "ArchiveBook", "ArchiveBookResponse", false, testutils.Problems{}}, - {"Invalid Response Suffix", "ArchiveBook", "ArchiveBookResp", true, testutils.Problems{{Suggestion: "ArchiveBookResponse or Book"}}}, - {"Unable To Find Resource", "ArchiveBook", "ArchiveBookResp", false, testutils.Problems{{Suggestion: "ArchiveBookResponse"}}}, - {"Irrelevant", "DeleteBook", "DeleteBookResp", true, testutils.Problems{}}, - } + t.Run("Response Suffix", func(t *testing.T) { + // Set up the testing permutations. + tests := []struct { + testName string + MethodName string + RespMessageName string + problems testutils.Problems + }{ + {"Valid", "ArchiveBook", "ArchiveBookResponse", testutils.Problems{}}, + {"Invalid", "ArchiveBook", "ArchiveBookResp", testutils.Problems{{Suggestion: "ArchiveBookResponse"}}}, + } - // Run each test individually. - for _, test := range tests { - t.Run(test.testName, func(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 { - {{ if (.OperatingOnResource) }} - // The book to archive. - // Format: publishers/{publisher}/books/{book} - string name = 1 [ - (google.api.resource_reference) = { - type: "library.googleapis.com/Book" - }]; + for _, test := range tests { + t.Run(test.testName, func(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) + 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 { + testName string + MethodName string + RespMessageName string + LRO bool + problems testutils.Problems + }{ + {"Valid", "ArchiveBook", "Book", false, testutils.Problems{}}, + {"Valid LRO", "ArchiveBook", "Book", true, testutils.Problems{}}, + {"Invalid", "ArchiveBook", "Author", false, testutils.Problems{{Suggestion: "ArchiveBookResponse or Book"}}}, + {"Invalid LRO", "ArchiveBook", "Author", true, testutils.Problems{{Suggestion: "ArchiveBookResponse or Book"}}}, + } + + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + file := testutils.ParseProto3Tmpl(t, ` + package test; + + import "google/api/annotations.proto"; + import "google/api/resource.proto"; + import "google/longrunning/operations.proto"; + + service Library { + rpc {{.MethodName}}({{.MethodName}}Request) returns ({{ if .LRO }}google.longrunning.Operation{{ else }}{{.RespMessageName}}{{ end }}) { + option (google.api.http) = { + post: "/v1/{name=publishers/*/books/*}:foo" + body: "*" + }; + {{ if (.LRO) }} + option (google.longrunning.operation_info) = { + response_type: "{{ .RespMessageName }}" + metadata_type: "{{ .RespMessageName | printf "%s%s" "Metadata" }}" + }; + {{ end }} + }; + } + + message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "publishers/{publisher}/books/{book}" + }; + } + + message Author { + option (google.api.resource) = { + type: "library.googleapis.com/Author" + pattern: "authors/{author}" + }; + } + + message {{.MethodName}}Request { + // The book to operate on. + // Format: publishers/{publisher}/books/{book} + string name = 1 [ + (google.api.resource_reference) = { + type: "library.googleapis.com/Book" + }]; + } + + {{ if and (ne .RespMessageName "Book") (ne .RespMessageName "Author") }} + message {{.RespMessageName}} {} {{ end }} - } - message Book { - option (google.api.resource) = { - type: "library.googleapis.com/Book" - pattern: "publishers/{publisher}/books/{book}" - }; - } - message Author { - option (google.api.resource) = { - type: "library.googleapis.com/Author" - pattern: "authors/{author}" - }; - } - {{ if and (ne .RespMessageName "Book") (ne .RespMessageName "Author") }} - message {{.RespMessageName}} {} - {{ end }} - `, test) - method := file.GetServices()[0].GetMethods()[0] - problems := responseMessageName.Lint(file) - if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" { - t.Error(diff) - } - }) - } + `, test) + method := file.GetServices()[0].GetMethods()[0] + problems := responseMessageName.Lint(file) + if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" { + t.Error(diff) + } + }) + } + }) } From bd4a2f914522ab85c6cf92a15cd62454350ad1b7 Mon Sep 17 00:00:00 2001 From: Matt Birchler Date: Fri, 17 May 2024 13:24:20 -0400 Subject: [PATCH 3/9] fix(AIP-136): clean up resource test template --- rules/aip0136/response_message_name_test.go | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/rules/aip0136/response_message_name_test.go b/rules/aip0136/response_message_name_test.go index c5306ee46..108cec7fe 100644 --- a/rules/aip0136/response_message_name_test.go +++ b/rules/aip0136/response_message_name_test.go @@ -83,10 +83,10 @@ func TestResponseMessageName(t *testing.T) { post: "/v1/{name=publishers/*/books/*}:foo" body: "*" }; - {{ if (.LRO) }} + {{ if .LRO }} option (google.longrunning.operation_info) = { response_type: "{{ .RespMessageName }}" - metadata_type: "{{ .RespMessageName | printf "%s%s" "Metadata" }}" + metadata_type: "{{ .RespMessageName }}Metadata" }; {{ end }} }; @@ -109,15 +109,8 @@ func TestResponseMessageName(t *testing.T) { message {{.MethodName}}Request { // The book to operate on. // Format: publishers/{publisher}/books/{book} - string name = 1 [ - (google.api.resource_reference) = { - type: "library.googleapis.com/Book" - }]; + string name = 1 [(google.api.resource_reference).type = "library.googleapis.com/Book"]; } - - {{ if and (ne .RespMessageName "Book") (ne .RespMessageName "Author") }} - message {{.RespMessageName}} {} - {{ end }} `, test) method := file.GetServices()[0].GetMethods()[0] problems := responseMessageName.Lint(file) From 36d1493743eecb0612913b1b7f81ce321ba9aa38 Mon Sep 17 00:00:00 2001 From: Matthew Birchler Date: Fri, 17 May 2024 16:59:59 -0400 Subject: [PATCH 4/9] Update docs/rules/0136/response-message-name.md Co-authored-by: Noah Dietz --- docs/rules/0136/response-message-name.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/rules/0136/response-message-name.md b/docs/rules/0136/response-message-name.md index db349de48..8ee680af6 100644 --- a/docs/rules/0136/response-message-name.md +++ b/docs/rules/0136/response-message-name.md @@ -20,10 +20,9 @@ This rule looks at any method that is not a standard method, and complains if the name of the corresponding output message does not match the name of the RPC with the suffix `Response` appended, or the resource being operated on. -**Note:** To identify the resource being operated on, the rule inspects the -name path parameter, which maps to the `name` field on the input type checking -that the resource message derived from `(google.api.resource_reference).type` -matches the response resource. +**Note:** To identify the resource being operated on, the rule checks for +the request field `name`, and comparing its `(google.api.resource_reference).type`, +if present, to the response message's `(google.api.resource).type`, if present. ## Examples From cfd6056326d7e8844db8ae9029d76a5161ea08b0 Mon Sep 17 00:00:00 2001 From: Matthew Birchler Date: Fri, 17 May 2024 17:00:13 -0400 Subject: [PATCH 5/9] Update docs/rules/0136/response-message-name.md Co-authored-by: Noah Dietz --- docs/rules/0136/response-message-name.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/rules/0136/response-message-name.md b/docs/rules/0136/response-message-name.md index 8ee680af6..c57007ce2 100644 --- a/docs/rules/0136/response-message-name.md +++ b/docs/rules/0136/response-message-name.md @@ -10,9 +10,9 @@ redirect_from: # Custom methods: Response message -This rule enforces that all custom methods should take a response message -matching the RPC name, with a `Response` suffix, or the resource being operated -on [AIP-136][]. +This rule enforces that custom methods have a response message that is named to +match the RPC name with a `Response` suffix, or that is the resource being operated +on, as described in [AIP-136][]. ## Details From b007953dcd47b31c661c4f18e1694236715c3e4b Mon Sep 17 00:00:00 2001 From: Matt Birchler Date: Fri, 17 May 2024 17:11:05 -0400 Subject: [PATCH 6/9] chore(AIP-136): PR feedback --- rules/aip0136/response_message_name.go | 35 +++++++++++---------- rules/aip0136/response_message_name_test.go | 6 ++-- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/rules/aip0136/response_message_name.go b/rules/aip0136/response_message_name.go index c790abe2b..d8f4aae36 100644 --- a/rules/aip0136/response_message_name.go +++ b/rules/aip0136/response_message_name.go @@ -23,6 +23,10 @@ import ( "github.com/jhump/protoreflect/desc" ) +const responseMessageNameErrorMessage = "" + + "Custom methods should return a message matching the RPC name, with a `Response` suffix, " + + "or the resource being operated on, not %q." + // Custom methods should return a response message matching the RPC name, // with a Response suffix, or the resource being operated on. var responseMessageName = &lint.MethodRule{ @@ -36,31 +40,30 @@ var responseMessageName = &lint.MethodRule{ // Reference of the input type's `name` field. This guidance is documented // in https://google.aip.dev/136#resource-based-custom-methods - response := utils.GetResponseType(m) - requestType := utils.GetResourceReference(m.GetInputType().FindFieldByName("name")).GetType() - resourceOperatedOn := utils.FindResourceMessage(requestType, m.GetFile()) - // Output type has `Response` suffix - if len(utils.LintMethodHasMatchingResponseName(m)) == 0 { + suffixFindings := utils.LintMethodHasMatchingResponseName(m) + if len(suffixFindings) == 0 { return nil } - // Response is the resource being operated on - if response == resourceOperatedOn { - return nil - } + response := utils.GetResponseType(m) + requestResourceType := utils.GetResourceReference(m.GetInputType().FindFieldByName("name")).GetType() - msg := "Custom methods should return a message matching the RPC name, with a `Response` suffix, or the resource being operated on, not %q." - got := m.GetName() + // If we can't determine if this is a resource-based custom method return the + // findings from the above matching response name lint + if !utils.IsResource(response) || requestResourceType == "" { + return suffixFindings + } - suggestion := got + "Response" - if utils.IsResource(response) && resourceOperatedOn != nil { - suggestion += " or " + resourceOperatedOn.GetName() + // However, if we can determine that this is a resource-based custom method, + // the ensure that they equal + responseResourceType := utils.GetResource(response).GetType() + if responseResourceType == requestResourceType { + return nil } return []lint.Problem{{ - Message: fmt.Sprintf(msg, got), - Suggestion: suggestion, + Message: fmt.Sprintf(responseMessageNameErrorMessage, response.GetName()), Descriptor: m, Location: locations.MethodResponseType(m), }} diff --git a/rules/aip0136/response_message_name_test.go b/rules/aip0136/response_message_name_test.go index 108cec7fe..f37f8a621 100644 --- a/rules/aip0136/response_message_name_test.go +++ b/rules/aip0136/response_message_name_test.go @@ -64,8 +64,10 @@ func TestResponseMessageName(t *testing.T) { }{ {"Valid", "ArchiveBook", "Book", false, testutils.Problems{}}, {"Valid LRO", "ArchiveBook", "Book", true, testutils.Problems{}}, - {"Invalid", "ArchiveBook", "Author", false, testutils.Problems{{Suggestion: "ArchiveBookResponse or Book"}}}, - {"Invalid LRO", "ArchiveBook", "Author", true, testutils.Problems{{Suggestion: "ArchiveBookResponse or Book"}}}, + {"Invalid", "ArchiveBook", "Author", false, testutils.Problems{{ + Message: "Custom methods should return a message matching the RPC name, with a `Response` suffix, or the resource being operated on, not \"Author\"."}}}, + {"Invalid LRO", "ArchiveBook", "Author", true, testutils.Problems{{ + Message: "Custom methods should return a message matching the RPC name, with a `Response` suffix, or the resource being operated on, not \"Author\"."}}}, } for _, test := range tests { From c28fca731e7798e3d8be357f771ac585d0852f69 Mon Sep 17 00:00:00 2001 From: Matt Birchler Date: Fri, 17 May 2024 17:23:56 -0400 Subject: [PATCH 7/9] fix: always return appropriate error message --- rules/aip0136/response_message_name.go | 10 ++-------- rules/aip0136/response_message_name_test.go | 3 ++- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/rules/aip0136/response_message_name.go b/rules/aip0136/response_message_name.go index d8f4aae36..924d6caaa 100644 --- a/rules/aip0136/response_message_name.go +++ b/rules/aip0136/response_message_name.go @@ -48,17 +48,11 @@ var responseMessageName = &lint.MethodRule{ response := utils.GetResponseType(m) requestResourceType := utils.GetResourceReference(m.GetInputType().FindFieldByName("name")).GetType() - - // If we can't determine if this is a resource-based custom method return the - // findings from the above matching response name lint - if !utils.IsResource(response) || requestResourceType == "" { - return suffixFindings - } + responseResourceType := utils.GetResource(response).GetType() // However, if we can determine that this is a resource-based custom method, // the ensure that they equal - responseResourceType := utils.GetResource(response).GetType() - if responseResourceType == requestResourceType { + if utils.IsResource(response) && responseResourceType == requestResourceType { return nil } diff --git a/rules/aip0136/response_message_name_test.go b/rules/aip0136/response_message_name_test.go index f37f8a621..bae9b8850 100644 --- a/rules/aip0136/response_message_name_test.go +++ b/rules/aip0136/response_message_name_test.go @@ -30,7 +30,8 @@ func TestResponseMessageName(t *testing.T) { problems testutils.Problems }{ {"Valid", "ArchiveBook", "ArchiveBookResponse", testutils.Problems{}}, - {"Invalid", "ArchiveBook", "ArchiveBookResp", testutils.Problems{{Suggestion: "ArchiveBookResponse"}}}, + {"Invalid", "ArchiveBook", "ArchiveBookResp", testutils.Problems{{ + Message: "Custom methods should return a message matching the RPC name, with a `Response` suffix, or the resource being operated on, not \"ArchiveBookResp\"."}}}, } for _, test := range tests { From 8bd648cd0709cdbd1379a0fcce1fcf68714b0bd4 Mon Sep 17 00:00:00 2001 From: Matt Birchler Date: Fri, 17 May 2024 17:26:46 -0400 Subject: [PATCH 8/9] fix: comment candy --- rules/aip0136/response_message_name.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/rules/aip0136/response_message_name.go b/rules/aip0136/response_message_name.go index 924d6caaa..b6d43270f 100644 --- a/rules/aip0136/response_message_name.go +++ b/rules/aip0136/response_message_name.go @@ -40,7 +40,7 @@ var responseMessageName = &lint.MethodRule{ // Reference of the input type's `name` field. This guidance is documented // in https://google.aip.dev/136#resource-based-custom-methods - // Output type has `Response` suffix + // Short-circuit: Output type has `Response` suffix suffixFindings := utils.LintMethodHasMatchingResponseName(m) if len(suffixFindings) == 0 { return nil @@ -50,8 +50,7 @@ var responseMessageName = &lint.MethodRule{ requestResourceType := utils.GetResourceReference(m.GetInputType().FindFieldByName("name")).GetType() responseResourceType := utils.GetResource(response).GetType() - // However, if we can determine that this is a resource-based custom method, - // the ensure that they equal + // Short-circuit: Output type is the resource being operated on if utils.IsResource(response) && responseResourceType == requestResourceType { return nil } From 3eb6784b6c20c71c08ffd216f49c5153b8f97417 Mon Sep 17 00:00:00 2001 From: Matt Birchler Date: Fri, 17 May 2024 17:36:33 -0400 Subject: [PATCH 9/9] chore: pr feedback to cleanup test message diff --- rules/aip0136/response_message_name_test.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/rules/aip0136/response_message_name_test.go b/rules/aip0136/response_message_name_test.go index bae9b8850..88013a1b6 100644 --- a/rules/aip0136/response_message_name_test.go +++ b/rules/aip0136/response_message_name_test.go @@ -30,8 +30,7 @@ func TestResponseMessageName(t *testing.T) { problems testutils.Problems }{ {"Valid", "ArchiveBook", "ArchiveBookResponse", testutils.Problems{}}, - {"Invalid", "ArchiveBook", "ArchiveBookResp", testutils.Problems{{ - Message: "Custom methods should return a message matching the RPC name, with a `Response` suffix, or the resource being operated on, not \"ArchiveBookResp\"."}}}, + {"Invalid", "ArchiveBook", "ArchiveBookResp", testutils.Problems{{Message: "not \"ArchiveBookResp\"."}}}, } for _, test := range tests { @@ -65,10 +64,8 @@ func TestResponseMessageName(t *testing.T) { }{ {"Valid", "ArchiveBook", "Book", false, testutils.Problems{}}, {"Valid LRO", "ArchiveBook", "Book", true, testutils.Problems{}}, - {"Invalid", "ArchiveBook", "Author", false, testutils.Problems{{ - Message: "Custom methods should return a message matching the RPC name, with a `Response` suffix, or the resource being operated on, not \"Author\"."}}}, - {"Invalid LRO", "ArchiveBook", "Author", true, testutils.Problems{{ - Message: "Custom methods should return a message matching the RPC name, with a `Response` suffix, or the resource being operated on, not \"Author\"."}}}, + {"Invalid", "ArchiveBook", "Author", false, testutils.Problems{{Message: "not \"Author\"."}}}, + {"Invalid LRO", "ArchiveBook", "Author", true, testutils.Problems{{Message: "not \"Author\"."}}}, } for _, test := range tests {