From 325377213c28499aaf07fe7cfae3879127fcaec2 Mon Sep 17 00:00:00 2001 From: Matt Birchler Date: Tue, 14 May 2024 21:44:37 -0400 Subject: [PATCH 1/2] feat: aip 0136 request message names This patch introduces a lint rule where all custom methods should take a request message matching the RPC name, with a Request suffix --- docs/rules/0136/request-message-name.md | 71 ++++++++++++++++++++++ rules/aip0136/aip0136.go | 1 + rules/aip0136/request_message_name.go | 27 ++++++++ rules/aip0136/request_message_name_test.go | 54 ++++++++++++++++ 4 files changed, 153 insertions(+) create mode 100644 docs/rules/0136/request-message-name.md create mode 100644 rules/aip0136/request_message_name.go create mode 100644 rules/aip0136/request_message_name_test.go diff --git a/docs/rules/0136/request-message-name.md b/docs/rules/0136/request-message-name.md new file mode 100644 index 000000000..392007442 --- /dev/null +++ b/docs/rules/0136/request-message-name.md @@ -0,0 +1,71 @@ +--- +rule: + aip: 136 + name: [core, '0136', request-message-name] + summary: Custom methods must have standardized request message names. +permalink: /136/request-message-name +redirect_from: + - /0136/request-message-name +--- + +# Custom methods: Request message + +This rule enforces that all custom methods should take a request message +matching the RPC name, with a `Request` suffix [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 `Request` appended. + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect. +// Should be `ArchiveBookRequest`. +rpc ArchiveBook(Book) returns (ArchiveBookResponse) { + 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: "*" + }; +} + +``` + +## 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::request-message-name=disabled +// aip.dev/not-precedent: We need to do this because reasons. --) +rpc ArchiveBook(Book) returns (ArchiveBookResponse) { + 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..c3d60026a 100644 --- a/rules/aip0136/aip0136.go +++ b/rules/aip0136/aip0136.go @@ -31,6 +31,7 @@ func AddRules(r lint.RuleRegistry) error { httpBody, httpMethod, noPrepositions, + requestMessageName, standardMethodsOnly, uriSuffix, verbNoun, diff --git a/rules/aip0136/request_message_name.go b/rules/aip0136/request_message_name.go new file mode 100644 index 000000000..3884fce2a --- /dev/null +++ b/rules/aip0136/request_message_name.go @@ -0,0 +1,27 @@ +// 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 ( + "github.com/googleapis/api-linter/lint" + "github.com/googleapis/api-linter/rules/internal/utils" +) + +// Custom methods should have a properly named Request message. +var requestMessageName = &lint.MethodRule{ + Name: lint.NewRuleName(136, "request-message-name"), + OnlyIf: isCustomMethod, + LintMethod: utils.LintMethodHasMatchingRequestName, +} diff --git a/rules/aip0136/request_message_name_test.go b/rules/aip0136/request_message_name_test.go new file mode 100644 index 000000000..2dfb8eb4f --- /dev/null +++ b/rules/aip0136/request_message_name_test.go @@ -0,0 +1,54 @@ +// 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 TestRequestMessageName(t *testing.T) { + // Set up the testing permutations. + tests := []struct { + testName string + MethodName string + ReqMessageName string + problems testutils.Problems + }{ + {"Valid", "ArchiveBook", "ArchiveBookRequest", testutils.Problems{}}, + {"Invalid", "ArchiveBook", "ArchiveBookReq", testutils.Problems{{Suggestion: "ArchiveBookRequest"}}}, + {"Irrelevant", "DeleteBook", "DeleteRequest", testutils.Problems{}}, + } + + // Run each test individually. + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + service Library { + rpc {{.MethodName}}({{.ReqMessageName}}) returns (Book) {} + } + message {{.ReqMessageName}} {} + {{if ne .ReqMessageName "Book"}} + message Book {} + {{end}} + `, test) + m := f.GetServices()[0].GetMethods()[0] + if diff := test.problems.SetDescriptor(m).Diff(requestMessageName.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} From 864d29d756fc4b77a1d31d92268d35b82b86f943 Mon Sep 17 00:00:00 2001 From: Matt Birchler Date: Wed, 15 May 2024 09:50:27 -0400 Subject: [PATCH 2/2] chore: simplify test template, always have book message --- rules/aip0136/request_message_name_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/rules/aip0136/request_message_name_test.go b/rules/aip0136/request_message_name_test.go index 2dfb8eb4f..f20de8a92 100644 --- a/rules/aip0136/request_message_name_test.go +++ b/rules/aip0136/request_message_name_test.go @@ -41,9 +41,7 @@ func TestRequestMessageName(t *testing.T) { rpc {{.MethodName}}({{.ReqMessageName}}) returns (Book) {} } message {{.ReqMessageName}} {} - {{if ne .ReqMessageName "Book"}} message Book {} - {{end}} `, test) m := f.GetServices()[0].GetMethods()[0] if diff := test.problems.SetDescriptor(m).Diff(requestMessageName.Lint(f)); diff != "" {