feat(AIP-123): resource collection matches plural (#1408)

Lint for the `plural` appearing as the collection segment in resource patterns. Special handling for nested resource collections that potentially stutter with the pattern, affording a reduced collection segment in the pattern. See #1403 for more details. The nested plural form is derived in a round about way where we check to see if the parent resource singular (via the resource ID segment) is a prefix of the child collection segment. We use the parent singular because the child plural wouldn't use the parent plural as the prefix in a "compound"/nested name, it would use the parent singular. This prefix is trimmed if it matches, leaving a reduced child collection name that is already pluralized. For internal bug http://b/343466943
googleapis · Jul 26, 2024 · 9025d3d · 9025d3d
1 parent cf4ba12
commit 9025d3d
Show file tree

Hide file tree

Showing 5 changed files with 304 additions and 0 deletions.
diff --git a/docs/rules/0123/resource-pattern-plural.md b/docs/rules/0123/resource-pattern-plural.md
@@ -0,0 +1,88 @@
+---
+rule:
+  aip: 123
+  name: [core, '0123', resource-pattern-plural]
+  summary: Resource patterns must use the plural as the collection segment
+permalink: /123/resource-pattern-plural
+redirect_from:
+  - /0123/resource-pattern-plural
+---
+
+# Resource `pattern` use of `plural`
+
+This rule enforces that messages that have a `google.api.resource` annotation
+use the `plural` form as the collection segment, as described in [AIP-123][].
+
+## Details
+
+This rule scans messages with a `google.api.resource` annotation, and validates
+the `plural` form of the resource type name is used as the collection segment
+in every pattern.
+
+**Note:** Special consideration is given to type names of child collections that
+stutter next to their parent collection, as described in
+[AIP-122 Nested Collections][nested]. See AIP-122 for more details.
+
+**Important:** Do not accept the suggestion if it would produce a backwards
+incompatible change.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/BookShelf"
+    // collection segment doesn't match the plural.
+    pattern: "publishers/{publisher}/shelves/{book_shelf}"
+    singular: "bookShelf"
+    plural: "bookShelves"
+  };
+
+  string name = 1;
+}
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/BookShelf"
+    pattern: "publishers/{publisher}/bookShelves/{book_shelf}"
+    singular: "bookShelf"
+    plural: "bookShelves"
+  };
+
+  string name = 1;
+}
+```
+
+## Disabling
+
+If you need to violate this rule, use a leading comment above the message.
+
+```proto
+// (-- api-linter: core::0123::resource-pattern-plural=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/BookShelf"
+    pattern: "publishers/{publisher}/shelves/{book_shelf}"
+    singular: "bookShelf"
+    plural: "bookShelves"
+  };
+
+  string name = 1;
+}
+```
+
+If you need to violate this rule for an entire file, place the comment at the
+top of the file.
+
+[aip-123]: http://aip.dev/123
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
+[nested]: https://aip.dev/122#nested-collections
diff --git a/rules/aip0123/aip0123.go b/rules/aip0123/aip0123.go
@@ -46,6 +46,7 @@ func AddRules(r lint.RuleRegistry) error {
 		resourceDefinitionPatterns,
 		resourceDefinitionTypeName,
 		resourcePatternSingular,
+		resourcePatternPlural,
 		nameNeverOptional,
 	)
 }
@@ -78,6 +79,18 @@ func getVariables(pattern string) []string {
 	return answer
 }
 
+// isRootLevelResource determines if the given resource is a root-level
+// resource using the isRootLevelResourcePattern helper.
+func isRootLevelResource(resource *apb.ResourceDescriptor) bool {
+	if len(resource.GetPattern()) == 0 {
+		return false
+	}
+
+	pattern := resource.GetPattern()[0]
+
+	return isRootLevelResourcePattern(pattern)
+}
+
 // isRootLevelResourcePattern determines if the given pattern is that of a
 // root-level resource by checking how many segments it has - root-level
 // resource patterns have only two segments, thus one delimeter.
@@ -120,6 +133,26 @@ func nestedSingular(resource *apb.ResourceDescriptor) string {
 	return strings.TrimPrefix(singularSnake, parentIDVar+"_")
 }
 
+// nestedPlural returns the would be reduced plural form of a nested
+// resource. Use isNestedName to check eligibility before using nestedPlural.
+// This will return empty if the resource is not eligible for nested name
+// reduction.
+func nestedPlural(resource *apb.ResourceDescriptor) string {
+	if !isNestedName(resource) {
+		return ""
+	}
+
+	// use the singular variable to trim the singular prefix of the compound
+	// child colleciton name that is pluralized e.g.
+	// "users/{user}/userEvents/{user_event}" the parent singular "user" is the
+	// prefix of the child collection "userEvents".
+	parentIDVar := getParentIDVariable(resource.GetPattern()[0])
+	parentIDVar = strcase.LowerCamelCase(parentIDVar)
+
+	plural := utils.GetResourcePlural(resource)
+	return strcase.LowerCamelCase(strings.TrimPrefix(plural, parentIDVar))
+}
+
 // isNestedName determines if the resource naming could be reduced as a
 // repetitive, nested collection as per AIP-122 Nested Collections. It does this
 // by analyzing the first resource pattern defined, comparing the resource

diff --git a/rules/aip0123/resource_pattern_plural.go b/rules/aip0123/resource_pattern_plural.go
@@ -0,0 +1,74 @@
+// Copyright 2024 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 aip0123 contains rules defined in https://aip.dev/123.
+package aip0123
+
+import (
+	"fmt"
+	"strings"
+
+	"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"
+)
+
+var resourcePatternPlural = &lint.MessageRule{
+	Name: lint.NewRuleName(123, "resource-pattern-plural"),
+	OnlyIf: func(m *desc.MessageDescriptor) bool {
+		return utils.IsResource(m) && len(utils.GetResource(m).GetPattern()) > 0 && !utils.IsSingletonResource(m)
+	},
+	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
+		var problems []lint.Problem
+		res := utils.GetResource(m)
+		nested := isNestedName(res)
+		var nn string
+		if nested {
+			nn = nestedPlural(res)
+		}
+
+		patterns := res.GetPattern()
+		plural := fmt.Sprintf("/%s/", utils.GetResourcePlural(res))
+		if isRootLevelResource(res) {
+			plural = strings.TrimPrefix(plural, "/")
+		}
+		nn = fmt.Sprintf("/%s/", nn)
+
+		// If the first pattern is reduced or non-compliant, but is nested name eligible, we want to recommend the nested name.
+		nestedFirstPattern := nested && (strings.Contains(patterns[0], nn) || !strings.Contains(patterns[0], plural))
+
+		for ndx, pattern := range patterns {
+			if !strings.Contains(pattern, plural) || ndx > 0 && nestedFirstPattern && !strings.Contains(pattern, nn) {
+				// allow the reduced, nested name instead if present
+				if nested && strings.Contains(pattern, nn) {
+					continue
+				}
+
+				want := plural
+				if nestedFirstPattern {
+					want = nn
+				}
+
+				problems = append(problems, lint.Problem{
+					Message:    fmt.Sprintf("resource pattern %q collection segment must be the resource plural %q", pattern, want),
+					Descriptor: m,
+					Location:   locations.MessageResource(m),
+				})
+			}
+		}
+
+		return problems
+	},
+}
diff --git a/rules/aip0123/resource_pattern_plural_test.go b/rules/aip0123/resource_pattern_plural_test.go
@@ -0,0 +1,108 @@
+// 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 aip0123
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestResourcePatternPluralSimple(t *testing.T) {
+	for _, test := range []struct {
+		name     string
+		Pattern  string
+		problems testutils.Problems
+	}{
+		{"Valid", "publishers/{publisher}/bookShelves/{book_shelf}", testutils.Problems{}},
+		{"ValidRootLevel", "bookShelves/{book_shelf}", testutils.Problems{}},
+		{"Invalid", "publishers/{publisher}/bookShelfs/{book_shelf}", testutils.Problems{{Message: "collection segment must be the resource plural"}}},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				import "google/api/resource.proto";
+
+				message BookShelf {
+					option (google.api.resource) = {
+						type: "library.googleapis.com/BookShelf"
+						singular: "bookShelf"
+						plural: "bookShelves"
+						pattern: "{{.Pattern}}"
+					};
+					string name = 1;
+				}
+			`, test)
+			m := f.GetMessageTypes()[0]
+			if diff := test.problems.SetDescriptor(m).Diff(resourcePatternPlural.Lint(f)); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}
+
+func TestResourcePatternPluralNested(t *testing.T) {
+	for _, test := range []struct {
+		name          string
+		FirstPattern  string
+		SecondPattern string
+		problems      testutils.Problems
+	}{
+		{
+			name:          "Valid",
+			FirstPattern:  "publishers/{publisher}/credits/{credit}",
+			SecondPattern: "authors/{author}/credits/{credit}",
+			problems:      testutils.Problems{},
+		},
+		{
+			name:          "ValidFull",
+			FirstPattern:  "publishers/{publisher}/publisherCredits/{publisher_credit}",
+			SecondPattern: "authors/{author}/publisherCredits/{publisher_credit}",
+			problems:      testutils.Problems{},
+		},
+		{
+			name:          "InvalidSecondWithFirstNestedName",
+			FirstPattern:  "publishers/{publisher}/credits/{credit}",
+			SecondPattern: "authors/{author}/publisherCredits/{credit}",
+			problems:      testutils.Problems{{Message: `collection segment must be the resource plural "/credits/"`}},
+		},
+		{
+			name:          "InvalidFirstWithReducedSecond",
+			FirstPattern:  "publishers/{publisher}/pubCredits/{credit}",
+			SecondPattern: "authors/{author}/credits/{credit}",
+			problems:      testutils.Problems{{Message: `collection segment must be the resource plural "/credits/"`}},
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				import "google/api/resource.proto";
+
+				message PublisherCredit {
+					option (google.api.resource) = {
+						type: "library.googleapis.com/PublisherCredit"
+						singular: "publisherCredit"
+						plural: "publisherCredits"
+						pattern: "{{.FirstPattern}}"
+						pattern: "{{.SecondPattern}}"
+					};
+					string name = 1;
+				}
+			`, test)
+			m := f.GetMessageTypes()[0]
+			if diff := test.problems.SetDescriptor(m).Diff(resourcePatternPlural.Lint(f)); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}
diff --git a/rules/aip0123/resource_pattern_singular_test.go b/rules/aip0123/resource_pattern_singular_test.go
@@ -28,6 +28,7 @@ func TestResourcePatternSingularSimple(t *testing.T) {
 	}{
 		{"Valid", "publishers/{publisher}/bookShelves/{book_shelf}", testutils.Problems{}},
 		{"ValidSingleton", "publishers/{publisher}/bookShelf", testutils.Problems{}},
+		{"ValidRootLevel", "bookShelves/{book_shelf}", testutils.Problems{}},
 		{"Invalid", "publishers/{publisher}/bookShelves/{shelf}", testutils.Problems{{Message: "final segment must include the resource singular"}}},
 		{"InvalidSingleton", "publishers/{publisher}/shelf", testutils.Problems{{Message: "final segment must include the resource singular"}}},
 	} {