feat(AIP-121): enforce list requirement (#1225)

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

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

rules/aip0121/aip0121.go

@@ -25,5 +25,6 @@ func AddRules(r lint.RuleRegistry) error {
 	return r.Register(
 		121,
 		resourceMustSupportGet,
+		resourceMustSupportList,
 	)
 }

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)
 				}
 			}
 		}

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
+	},
+}

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)
+			}
+		})
+	}
+}