From 120c4c8ae2a4ff456401d1c041e6da6819c2fd54 Mon Sep 17 00:00:00 2001 From: Luke Sneeringer Date: Mon, 24 Aug 2020 10:19:45 -0600 Subject: [PATCH 1/2] feat: Prohibit streaming on long-running operations. --- docs/rules/0151/response-unary.md | 69 ++++++++++++++++++++++++++++ rules/aip0151/aip0151.go | 1 + rules/aip0151/response_unary.go | 36 +++++++++++++++ rules/aip0151/response_unary_test.go | 49 ++++++++++++++++++++ 4 files changed, 155 insertions(+) create mode 100644 docs/rules/0151/response-unary.md create mode 100644 rules/aip0151/response_unary.go create mode 100644 rules/aip0151/response_unary_test.go diff --git a/docs/rules/0151/response-unary.md b/docs/rules/0151/response-unary.md new file mode 100644 index 000000000..6e5f973f3 --- /dev/null +++ b/docs/rules/0151/response-unary.md @@ -0,0 +1,69 @@ +--- +rule: + aip: 151 + name: [core, '0151', response-unary] + summary: Long-running operations must not use streaming. +permalink: /151/response-unary +redirect_from: + - /0151/response-unary +--- + +# Paginated methods: Unary responses + +This rule enforces that all long-running operation methods use unary responses, +as mandated in [AIP-151][]. + +## Details + +This rule looks at any message returning a `google.longrunning.Operation`, and +complains if the method uses gRPC server streaming (the `stream` keyword). + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect. +// Streaming is prohibited on long-running operations. +rpc ReadBook(ReadBookRequest) returns (stream google.longrunning.Operation) { + option (google.api.http) = { + post: "/v1/{parent=publishers/*/books/*}:read" + body: "*" + }; +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +rpc ReadBook(ReadBookRequest) returns (google.longrunning.Operation) { + option (google.api.http) = { + post: "/v1/{parent=publishers/*/books/*}:read" + body: "*" + }; +} +``` + +## Disabling + +If you need to violate this rule, use a leading comment above the message or +above the field. Remember to also include an [aip.dev/not-precedent][] comment +explaining why. + +```proto +// (-- api-linter: core::0151::response-unary +// aip.dev/not-precedent: We need to do this because reasons. --) +rpc ReadBook(ReadBookRequest) returns (stream google.longrunning.Operation) { + option (google.api.http) = { + post: "/v1/{parent=publishers/*/books/*}:read" + body: "*" + }; +} +``` + +If you need to violate this rule for an entire file, place the comment at the +top of the file. + +[aip-151]: https://aip.dev/151 +[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/rules/aip0151/aip0151.go b/rules/aip0151/aip0151.go index 5e004e6dd..999355f5e 100644 --- a/rules/aip0151/aip0151.go +++ b/rules/aip0151/aip0151.go @@ -30,6 +30,7 @@ func AddRules(r lint.RuleRegistry) error { lroMetadataReachable, lroResponse, lroResponseReachable, + responseUnary, ) } diff --git a/rules/aip0151/response_unary.go b/rules/aip0151/response_unary.go new file mode 100644 index 000000000..26d86add0 --- /dev/null +++ b/rules/aip0151/response_unary.go @@ -0,0 +1,36 @@ +// Copyright 2020 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 aip0151 + +import ( + "github.com/googleapis/api-linter/lint" + "github.com/googleapis/api-linter/locations" + "github.com/jhump/protoreflect/desc" +) + +var responseUnary = &lint.MethodRule{ + Name: lint.NewRuleName(151, "response-unary"), + OnlyIf: isLRO, + LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { + if m.IsServerStreaming() { + return []lint.Problem{{ + Message: "Paginated responses must be unary, not streaming.", + Descriptor: m, + Location: locations.MethodResponseType(m), + }} + } + return nil + }, +} diff --git a/rules/aip0151/response_unary_test.go b/rules/aip0151/response_unary_test.go new file mode 100644 index 000000000..a89de7aa2 --- /dev/null +++ b/rules/aip0151/response_unary_test.go @@ -0,0 +1,49 @@ +// Copyright 2020 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 aip0151 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +func TestResponseUnary(t *testing.T) { + for _, test := range []struct { + name string + Stream string + Response string + problems testutils.Problems + }{ + {"Vaild", "", "google.longrunning.Operation", nil}, + {"Invalid", "stream ", "google.longrunning.Operation", testutils.Problems{{Message: "unary, not stream"}}}, + {"Irrelevant", "stream ", "ReadBookResponse", nil}, + } { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/longrunning/operations.proto"; + service Library { + rpc ReadBook(ReadBookRequest) returns ({{.Stream}}{{.Response}}); + } + message ReadBookRequest {} + message ReadBookResponse {} + `, test) + m := f.GetServices()[0].GetMethods()[0] + if diff := test.problems.SetDescriptor(m).Diff(responseUnary.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} From a514aaba85c607d57e4122c6c79692c3bdbcd2a8 Mon Sep 17 00:00:00 2001 From: Luke Sneeringer Date: Mon, 24 Aug 2020 13:31:06 -0700 Subject: [PATCH 2/2] Incorporate @noahdietz feedback. Co-authored-by: Noah Dietz --- rules/aip0151/response_unary.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/aip0151/response_unary.go b/rules/aip0151/response_unary.go index 26d86add0..626738b06 100644 --- a/rules/aip0151/response_unary.go +++ b/rules/aip0151/response_unary.go @@ -26,7 +26,7 @@ var responseUnary = &lint.MethodRule{ LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { if m.IsServerStreaming() { return []lint.Problem{{ - Message: "Paginated responses must be unary, not streaming.", + Message: "LRO responses must be unary, not streaming.", Descriptor: m, Location: locations.MethodResponseType(m), }}