diff --git a/docs/rules/0136/response-message-name.md b/docs/rules/0136/response-message-name.md new file mode 100644 index 00000000..c57007ce --- /dev/null +++ b/docs/rules/0136/response-message-name.md @@ -0,0 +1,158 @@ +--- +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 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 + +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 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 + +### Response Suffix + +**Incorrect** code for this rule: + +```proto +// Incorrect. +// Should be `TranslateTextResponse`. +rpc TranslateText(TranslateTextRequest) returns (Text) { + option (google.api.http) = { + post: "/v1/{project=projects/*}:translateText" + body: "*" + }; +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +rpc TranslateText(TranslateTextRequest) returns (TranslateTextResponse) { + option (google.api.http) = { + post: "/v1/{project=projects/*}:translateText" + 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 +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"]; +} +``` + +### 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"]; +} +``` + +## 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 c3d60026..61e8289e 100644 --- a/rules/aip0136/aip0136.go +++ b/rules/aip0136/aip0136.go @@ -32,6 +32,7 @@ func AddRules(r lint.RuleRegistry) error { httpMethod, noPrepositions, requestMessageName, + 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 00000000..b6d43270 --- /dev/null +++ b/rules/aip0136/response_message_name.go @@ -0,0 +1,65 @@ +// 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" + + "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" +) + +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{ + Name: lint.NewRuleName(136, "response-message-name"), + OnlyIf: isCustomMethod, + LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { + // 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 + + // Short-circuit: Output type has `Response` suffix + suffixFindings := utils.LintMethodHasMatchingResponseName(m) + if len(suffixFindings) == 0 { + return nil + } + + response := utils.GetResponseType(m) + requestResourceType := utils.GetResourceReference(m.GetInputType().FindFieldByName("name")).GetType() + responseResourceType := utils.GetResource(response).GetType() + + // Short-circuit: Output type is the resource being operated on + if utils.IsResource(response) && responseResourceType == requestResourceType { + return nil + } + + return []lint.Problem{{ + 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 new file mode 100644 index 00000000..88013a1b --- /dev/null +++ b/rules/aip0136/response_message_name_test.go @@ -0,0 +1,123 @@ +// 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) { + 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{{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"; + 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{{Message: "not \"Author\"."}}}, + {"Invalid LRO", "ArchiveBook", "Author", true, testutils.Problems{{Message: "not \"Author\"."}}}, + } + + 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 }}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"]; + } + `, test) + method := file.GetServices()[0].GetMethods()[0] + problems := responseMessageName.Lint(file) + if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" { + t.Error(diff) + } + }) + } + }) +}