From 7ad11d0add4228236bae63f8b3e87812428758e8 Mon Sep 17 00:00:00 2001 From: Noah Dietz Date: Fri, 4 Aug 2023 10:19:11 -0700 Subject: [PATCH] feat(AIP-121): enforce list requirement (#1225) --- docs/rules/0121/resource-must-support-get.md | 2 +- docs/rules/0121/resource-must-support-list.md | 82 ++++++++++ rules/aip0121/aip0121.go | 1 + rules/aip0121/resource_must_support_get.go | 13 +- rules/aip0121/resource_must_support_list.go | 65 ++++++++ .../resource_must_support_list_test.go | 150 ++++++++++++++++++ 6 files changed, 305 insertions(+), 8 deletions(-) create mode 100644 docs/rules/0121/resource-must-support-list.md create mode 100644 rules/aip0121/resource_must_support_list.go create mode 100644 rules/aip0121/resource_must_support_list_test.go diff --git a/docs/rules/0121/resource-must-support-get.md b/docs/rules/0121/resource-must-support-get.md index 4a3f276eb..908c63214 100644 --- a/docs/rules/0121/resource-must-support-get.md +++ b/docs/rules/0121/resource-must-support-get.md @@ -2,7 +2,7 @@ rule: aip: 121 name: [core, '0121', resource-must-support-get] - summary: All resource names must use camel case in collection identifiers. + summary: All resources must have a Standard Get method. permalink: /121/resource-must-support-get redirect_from: - /0121/resource-must-support-get diff --git a/docs/rules/0121/resource-must-support-list.md b/docs/rules/0121/resource-must-support-list.md new file mode 100644 index 000000000..b394ff452 --- /dev/null +++ b/docs/rules/0121/resource-must-support-list.md @@ -0,0 +1,82 @@ +--- +rule: + aip: 121 + name: [core, '0121', resource-must-support-list] + summary: All resources must have a Standard List method. +permalink: /121/resource-must-support-list +redirect_from: + - /0121/resource-must-support-list +--- + +# Resource must support list + +This rule enforces that all, non-Singleton resources support the List operation +as mandated in [AIP-121][]. + +## Details + +This rule scans a service for Create, Update, and Get methods for resources +(that are not Singletons), and ensures each one has a List method. + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect. +service Foo { + // Book has a create, but no List method. + rpc CreateBook(CreateBookRequest) returns (Book) {}; +} + +message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "books/{book}" + }; +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +service Foo { + rpc CreateBook(CreateBookRequest) returns (Book) {}; + rpc ListBooks(ListBookRequest) returns (ListBooksResponse) {}; +} + +message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "books/{book}" + }; +} +``` + +## Disabling + +If you need to violate this rule, use a leading comment above the service. +Remember to also include an [aip.dev/not-precedent][] comment explaining why. + +```proto +// (-- api-linter: core::0121::resource-must-support-list=disabled +// aip.dev/not-precedent: We need to do this because reasons. --) +service Foo { + // Book has a create, but no List method. + rpc CreateBook(CreateBookRequest) returns (Book) {}; +} + +message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "books/{book}" + }; +} +``` + +If you need to violate this rule for an entire file, place the comment at the +top of the file. + +[aip-121]: https://aip.dev/121 +[aip.dev/not-precedent]: https://aip.dev/not-precedent \ No newline at end of file diff --git a/rules/aip0121/aip0121.go b/rules/aip0121/aip0121.go index 1dcc61406..66dc10fad 100644 --- a/rules/aip0121/aip0121.go +++ b/rules/aip0121/aip0121.go @@ -25,5 +25,6 @@ func AddRules(r lint.RuleRegistry) error { return r.Register( 121, resourceMustSupportGet, + resourceMustSupportList, ) } diff --git a/rules/aip0121/resource_must_support_get.go b/rules/aip0121/resource_must_support_get.go index 01ac6b650..ff414fa27 100644 --- a/rules/aip0121/resource_must_support_get.go +++ b/rules/aip0121/resource_must_support_get.go @@ -26,11 +26,10 @@ import ( var resourceMustSupportGet = &lint.ServiceRule{ Name: lint.NewRuleName(121, "resource-must-support-get"), LintService: func(s *desc.ServiceDescriptor) []lint.Problem { - problems := []lint.Problem{} - // Add the empty string to avoid adding multiple nil checks. - // Just say it's valid instead. - resourcesWithGet := stringset.New("") - resourcesWithOtherMethods := map[string]*desc.MessageDescriptor{} + var problems []lint.Problem + var resourcesWithGet stringset.Set + var resourcesWithOtherMethods stringset.Set + // Iterate all RPCs and try to find resources. Mark the // resources which have a Get method, and which ones do not. for _, m := range s.GetMethods() { @@ -40,12 +39,12 @@ var resourceMustSupportGet = &lint.ServiceRule{ } else if utils.IsCreateMethod(m) || utils.IsUpdateMethod(m) { if msg := utils.GetResponseType(m); msg != nil { t := utils.GetResource(msg).GetType() - resourcesWithOtherMethods[t] = msg + resourcesWithOtherMethods.Add(t) } } else if utils.IsListMethod(m) { if msg := utils.GetListResourceMessage(m); msg != nil { t := utils.GetResource(msg).GetType() - resourcesWithOtherMethods[t] = msg + resourcesWithOtherMethods.Add(t) } } } diff --git a/rules/aip0121/resource_must_support_list.go b/rules/aip0121/resource_must_support_list.go new file mode 100644 index 000000000..036816cc5 --- /dev/null +++ b/rules/aip0121/resource_must_support_list.go @@ -0,0 +1,65 @@ +// Copyright 2023 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 aip0121 + +import ( + "fmt" + + "bitbucket.org/creachadair/stringset" + "github.com/googleapis/api-linter/lint" + "github.com/googleapis/api-linter/rules/internal/utils" + "github.com/jhump/protoreflect/desc" +) + +var resourceMustSupportList = &lint.ServiceRule{ + Name: lint.NewRuleName(121, "resource-must-support-list"), + LintService: func(s *desc.ServiceDescriptor) []lint.Problem { + var problems []lint.Problem + var resourcesWithList stringset.Set + var resourcesWithOtherMethods stringset.Set + + // Iterate all RPCs and try to find resources. Mark the + // resources which have a List method, and which ones do not. + for _, m := range s.GetMethods() { + if utils.IsListMethod(m) { + if msg := utils.GetListResourceMessage(m); msg != nil { + t := utils.GetResource(msg).GetType() + resourcesWithList.Add(t) + } + } else if utils.IsCreateMethod(m) || utils.IsUpdateMethod(m) || utils.IsGetMethod(m) { + if msg := utils.GetResponseType(m); msg != nil { + // Skip tracking Singleton resources, they do not need List. + if utils.IsSingletonResource(msg) { + continue + } + t := utils.GetResource(msg).GetType() + resourcesWithOtherMethods.Add(t) + } + } + } + + for t := range resourcesWithOtherMethods { + if !resourcesWithList.Contains(t) { + problems = append(problems, lint.Problem{ + Message: fmt.Sprintf( + "Missing Standard List method for resource %q", t, + ), + Descriptor: s, + }) + } + } + return problems + }, +} diff --git a/rules/aip0121/resource_must_support_list_test.go b/rules/aip0121/resource_must_support_list_test.go new file mode 100644 index 000000000..5f9f4354d --- /dev/null +++ b/rules/aip0121/resource_must_support_list_test.go @@ -0,0 +1,150 @@ +// Copyright 2023 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 aip0121 + +import ( + "testing" + + "github.com/googleapis/api-linter/lint" + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +// TestResourceMustSupportList tests the resourceMustSupportList +// lint rule by declaring a service proto, then declaring a +// google.api.resource message, then declaring non-List +// methods. +func TestResourceMustSupportList(t *testing.T) { + for _, test := range []struct { + name string + RPCs string + problems testutils.Problems + }{ + {"ValidCreateList", ` + rpc ListBooks(ListBooksRequest) returns (ListBooksResponse) {}; + rpc CreateBook(CreateBookRequest) returns (Book) {}; + `, nil}, + {"ValidCreateListLRO", ` + rpc ListBooks(ListBooksRequest) returns (ListBooksResponse) {}; + rpc CreateBook(CreateBookRequest) returns (google.longrunning.Operation) { + option (google.longrunning.operation_info) = { + response_type: "Book" + }; + }; + `, nil}, + {"ValidListGet", ` + rpc GetBook(GetBookRequest) returns (Book) {}; + rpc ListBooks(ListBooksRequest) returns (ListBooksResponse) {}; + `, nil}, + {"ValidUpdateList", ` + rpc ListBooks(ListBooksRequest) returns (ListBooksResponse) {}; + rpc UpdateBook(UpdateBookRequest) returns (Book) {}; + `, nil}, + {"InvalidCreateOnly", ` + rpc CreateBook(CreateBookRequest) returns (Book) {}; + `, []lint.Problem{ + {Message: `resource "library.googleapis.com/Book"`}, + }}, + {"InvalidCreateOnlyLRO", ` + rpc CreateBook(CreateBookRequest) returns (google.longrunning.Operation) { + option (google.longrunning.operation_info) = { + response_type: "Book" + }; + }; + `, []lint.Problem{ + {Message: `resource "library.googleapis.com/Book"`}, + }}, + {"InvalidUpdateOnly", ` + rpc UpdateBook(UpdateBookRequest) returns (Book) {}; + `, []lint.Problem{ + {Message: `resource "library.googleapis.com/Book"`}, + }}, + {"InvalidGetOnly", ` + rpc GetBook(GetBookRequest) returns (Book) {}; + `, []lint.Problem{ + {Message: `resource "library.googleapis.com/Book"`}, + }}, + {"ValidIgnoreSingleton", ` + rpc GetBookCover(GetBookCoverRequest) returns (BookCover) {}; + `, nil}, + } { + t.Run(test.name, func(t *testing.T) { + file := testutils.ParseProto3Tmpl(t, ` + import "google/api/resource.proto"; + import "google/longrunning/operations.proto"; + import "google/protobuf/field_mask.proto"; + service Foo { + {{.RPCs}} + } + + // This is at the top to make it retrievable + // by the test code. + message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "books/{book}" + singular: "book" + plural: "books" + }; + } + + message BookCover { + option (google.api.resource) = { + type: "library.googleapis.com/BookCover" + pattern: "books/{book}/bookCover" + singular: "bookCover" + }; + } + + message GetBookCoverRequest { + string name = 1; + } + + message CreateBookRequest { + // The parent resource where this book will be created. + // Format: publishers/{publisher} + string parent = 1; + + // The book to create. + Book book = 2; + } + + message GetBookRequest { + string name = 1; + } + + message UpdateBookRequest { + Book book = 1; + google.protobuf.FieldMask update_mask = 2; + } + + message ListBooksRequest { + string parent = 1; + int32 page_size = 2; + string page_token = 3; + } + + message ListBooksResponse { + repeated Book books = 1; + string next_page_token = 2; + } + `, test) + s := file.GetServices()[0] + got := resourceMustSupportList.Lint(file) + if diff := test.problems.SetDescriptor(s).Diff(got); diff != "" { + t.Error(diff) + } + }) + } +}