From 088ec196da93a9338b2abf60469cb55ecec5c34d Mon Sep 17 00:00:00 2001 From: Noah Dietz Date: Wed, 24 Jul 2024 14:29:34 -0700 Subject: [PATCH] feat(AIP-123): enforce singular as ID segment (#1403) * feat(AIP-123): enforce singular as ID segment * address typos * change helper name top level to root level * fix typo Co-authored-by: Sam Levenick * add plural in example and remove extraneous comma * handle top level singleton in root level check --- docs/rules/0123/resource-pattern-singular.md | 88 ++++++++ docs/rules/0123/resource-singular.md | 4 +- rules/aip0123/aip0123.go | 87 ++++++++ rules/aip0123/aip0123_test.go | 205 ++++++++++++++++++ rules/aip0123/resource_pattern_singular.go | 75 +++++++ .../aip0123/resource_pattern_singular_test.go | 127 +++++++++++ rules/internal/utils/extension.go | 18 +- 7 files changed, 596 insertions(+), 8 deletions(-) create mode 100644 docs/rules/0123/resource-pattern-singular.md create mode 100644 rules/aip0123/resource_pattern_singular.go create mode 100644 rules/aip0123/resource_pattern_singular_test.go diff --git a/docs/rules/0123/resource-pattern-singular.md b/docs/rules/0123/resource-pattern-singular.md new file mode 100644 index 000000000..3bcb48fc7 --- /dev/null +++ b/docs/rules/0123/resource-pattern-singular.md @@ -0,0 +1,88 @@ +--- +rule: + aip: 123 + name: [core, '0123', resource-pattern-singular] + summary: Resource patterns must use the singular as the resource ID segment +permalink: /123/resource-pattern-singular +redirect_from: + - /0123/resource-pattern-singular +--- + +# Resource `pattern` use of `singular` + +This rule enforces that messages that have a `google.api.resource` annotation +use the `singular` form as the resource ID segment, as described in [AIP-123][]. + +## Details + +This rule scans messages with a `google.api.resource` annotation, and validates +the `singular` form of the resource type name is used as the resource ID 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" + // resource ID segment doesn't match the singular. + pattern: "publishers/{publisher}/bookShelves/{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-singular=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}/bookShelves/{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/docs/rules/0123/resource-singular.md b/docs/rules/0123/resource-singular.md index a036fa687..22fae7fde 100644 --- a/docs/rules/0123/resource-singular.md +++ b/docs/rules/0123/resource-singular.md @@ -8,7 +8,7 @@ redirect_from: - /0123/resource-singular --- -# Resource type name +# Resource `singular` This rule enforces that messages that have a `google.api.resource` annotation, have a properly formatted `singular`, as described in [AIP-123][]. @@ -60,7 +60,7 @@ If you need to violate this rule, use a leading comment above the message. // aip.dev/not-precedent: We need to do this because reasons. --) message Book { option (google.api.resource) = { - type: "library.googleapis.com/Genre/Mystery/Book" + type: "library.googleapis.com/Book" pattern: "publishers/{publisher}/books/{book}" singular: "shelf", }; diff --git a/rules/aip0123/aip0123.go b/rules/aip0123/aip0123.go index f586e90e7..ce484756c 100644 --- a/rules/aip0123/aip0123.go +++ b/rules/aip0123/aip0123.go @@ -24,6 +24,7 @@ import ( "github.com/googleapis/api-linter/rules/internal/utils" "github.com/jhump/protoreflect/desc" "github.com/stoewer/go-strcase" + apb "google.golang.org/genproto/googleapis/api/annotations" ) // AddRules accepts a register function and registers each of @@ -44,6 +45,7 @@ func AddRules(r lint.RuleRegistry) error { resourceDefinitionVariables, resourceDefinitionPatterns, resourceDefinitionTypeName, + resourcePatternSingular, nameNeverOptional, ) } @@ -76,6 +78,91 @@ func getVariables(pattern string) []string { return answer } +// 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. +func isRootLevelResourcePattern(pattern string) bool { + return strings.Count(pattern, "/") <= 1 +} + +// getParentIDVariable is a helper that returns the parent resource ID segment +// for a given pattern. Returns empty string if the pattern has no variables or +// is a top-level resource. +func getParentIDVariable(pattern string) string { + variables := getVariables(pattern) + // no variables shouldn't happen but should be safe + if len(variables) == 0 || isRootLevelResourcePattern(pattern) { + return "" + } + + // TODO: handle if singleton is a *parent*. + if utils.IsSingletonResourcePattern(pattern) { + // Last variable is the parent's for a singleton child. + return variables[len(variables)-1] + } + + return variables[len(variables)-2] +} + +// nestedSingular returns the would be reduced singular form of a nested +// resource. Use isNestedName to check eligibility before using nestedSingular. +// This will return empty if the resource is not eligible for nested name +// reduction. +func nestedSingular(resource *apb.ResourceDescriptor) string { + if !isNestedName(resource) { + return "" + } + parentIDVar := getParentIDVariable(resource.GetPattern()[0]) + + singular := utils.GetResourceSingular(resource) + singularSnake := strcase.SnakeCase(singular) + + return strings.TrimPrefix(singularSnake, 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 +// singular to the resource ID segment of the parent portion of the pattern. If +// that is a prefix of resource singular (in snake_case form as well), then the +// resource name could be reduced. For example, given `singular: "userEvent"` +// and `pattern: "users/{user}/userEvents/{user_event}"`, isNestedName would +// return `true`, because the `pattern` could be reduced to +// `"users/{user}/events/{event}"`. +func isNestedName(resource *apb.ResourceDescriptor) bool { + if len(resource.GetPattern()) == 0 { + return false + } + // only evaluate the first pattern b.c patterns cannot be reordered + // and nested names must be used consistently, thus from the beginning + pattern := resource.GetPattern()[0] + + // Can't be a nested collection if it is a top level resource. + if isRootLevelResourcePattern(pattern) { + return false + } + + singular := utils.GetResourceSingular(resource) + + // If the resource type's singular is not camelCase then it is not a + // multi-word type and we do not need to check for nestedness in naming. + singularSnake := strcase.SnakeCase(singular) + if strings.Count(singularSnake, "_") < 1 { + return false + } + + // Second to last resource ID variable will be the parent's to compare the + // child resource's singular form against. This prevents us from needing to + // deal with pluralizations due to the child resource typically being + // pluralized on its own noun, not the parent noun. + parentIDVar := getParentIDVariable(pattern) + + // For example: + // publisher_credit starts with publisher --> nested + // book_shelf does not start with library --> not nested re:naming + return strings.HasPrefix(singularSnake, parentIDVar) +} + // getPlainPattern returns the pattern with all variables replaced with "*". // // For example, a pattern of "publishers/{publisher}/books/{book}" would diff --git a/rules/aip0123/aip0123_test.go b/rules/aip0123/aip0123_test.go index d0d127b28..1701c3468 100644 --- a/rules/aip0123/aip0123_test.go +++ b/rules/aip0123/aip0123_test.go @@ -18,6 +18,7 @@ import ( "testing" "github.com/googleapis/api-linter/lint" + apb "google.golang.org/genproto/googleapis/api/annotations" ) func TestAddRules(t *testing.T) { @@ -25,3 +26,207 @@ func TestAddRules(t *testing.T) { t.Errorf("AddRules got an error: %v", err) } } + +func TestIsNestedName(t *testing.T) { + for _, test := range []struct { + name string + resource *apb.ResourceDescriptor + want bool + }{ + { + name: "top level", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/Project", + Pattern: []string{"projects/{project}"}, + Singular: "project", + Plural: "projects", + }, + want: false, + }, + { + name: "non-nested child collection", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/Location", + Pattern: []string{"projects/{project}/locations/{location}"}, + Singular: "location", + Plural: "locations", + }, + want: false, + }, + { + name: "non-nested child collection multi-word", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/BillingAccount", + Pattern: []string{"projects/{project}/billingAccounts/{billing_account}"}, + Singular: "billingAccount", + Plural: "billingAccounts", + }, + want: false, + }, + { + name: "nested child collection full", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/UserEvent", + Pattern: []string{"projects/{project}/users/{user}/userEvents/{user_event}"}, + Singular: "userEvent", + Plural: "userEvents", + }, + want: true, + }, + { + name: "nested child collection reduced", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/UserEvent", + Pattern: []string{"projects/{project}/users/{user}/events/{event}"}, + Singular: "userEvent", + Plural: "userEvents", + }, + want: true, + }, + { + name: "nested singleton full", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/UserConfig", + Pattern: []string{"projects/{project}/users/{user}/userConfig"}, + Singular: "userConfig", + Plural: "userConfigs", + }, + want: true, + }, + { + name: "nested singleton reduced", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/UserConfig", + Pattern: []string{"projects/{project}/users/{user}/config"}, + Singular: "userConfig", + Plural: "userConfigs", + }, + want: true, + }, + } { + t.Run(test.name, func(t *testing.T) { + if got := isNestedName(test.resource); got != test.want { + t.Errorf("got %v, expected %v for pattern %q", got, test.want, test.resource.GetPattern()[0]) + } + }) + } +} + +func TestNestedSingular(t *testing.T) { + for _, test := range []struct { + name string + resource *apb.ResourceDescriptor + want string + }{ + { + name: "top level", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/Project", + Pattern: []string{"projects/{project}"}, + Singular: "project", + Plural: "projects", + }, + }, + { + name: "non-nested child collection", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/Location", + Pattern: []string{"projects/{project}/locations/{location}"}, + Singular: "location", + Plural: "locations", + }, + }, + { + name: "non-nested child collection multi-word", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/BillingAccount", + Pattern: []string{"projects/{project}/billingAccounts/{billing_account}"}, + Singular: "billingAccount", + Plural: "billingAccounts", + }, + }, + { + name: "nested child collection full", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/UserEvent", + Pattern: []string{"projects/{project}/users/{user}/userEvents/{user_event}"}, + Singular: "userEvent", + Plural: "userEvents", + }, + want: "event", + }, + { + name: "nested singleton full", + resource: &apb.ResourceDescriptor{ + Type: "example.googleapis.com/UserConfig", + Pattern: []string{"projects/{project}/users/{user}/userConfig"}, + Singular: "userConfig", + Plural: "userConfigs", + }, + want: "config", + }, + } { + t.Run(test.name, func(t *testing.T) { + if got := nestedSingular(test.resource); got != test.want { + t.Errorf("got %v, expected %v for pattern %q", got, test.want, test.resource.GetPattern()[0]) + } + }) + } +} + +func TestIsTopLevelResourcePattern(t *testing.T) { + for _, test := range []struct { + name string + pattern string + want bool + }{ + { + name: "top level", + pattern: "projects/{project}", + want: true, + }, + { + name: "not top level", + pattern: "projects/{project}/locations/{location}", + }, + } { + t.Run(test.name, func(t *testing.T) { + if got := isRootLevelResourcePattern(test.pattern); got != test.want { + t.Errorf("got %v, expected %v for pattern %q", got, test.want, test.pattern) + } + }) + } +} + +func TestGetParentIDVariable(t *testing.T) { + for _, test := range []struct { + name string + pattern string + want string + }{ + { + name: "top level", + pattern: "projects/{project}", + }, + { + name: "no variables", + pattern: "foos", + }, + { + name: "standard child collection", + pattern: "projects/{project}/locations/{location}", + want: "project", + }, + { + name: "singleton", + pattern: "projects/{project}/config", + want: "project", + }, + } { + t.Run(test.name, func(t *testing.T) { + if got := getParentIDVariable(test.pattern); got != test.want { + t.Errorf("got %v, expected %v for pattern %q", got, test.want, test.pattern) + } + }) + } +} diff --git a/rules/aip0123/resource_pattern_singular.go b/rules/aip0123/resource_pattern_singular.go new file mode 100644 index 000000000..f95b2d1c4 --- /dev/null +++ b/rules/aip0123/resource_pattern_singular.go @@ -0,0 +1,75 @@ +// 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" + "github.com/stoewer/go-strcase" +) + +var resourcePatternSingular = &lint.MessageRule{ + Name: lint.NewRuleName(123, "resource-pattern-singular"), + OnlyIf: func(m *desc.MessageDescriptor) bool { + return utils.IsResource(m) && len(utils.GetResource(m).GetPattern()) > 0 + }, + LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { + var problems []lint.Problem + res := utils.GetResource(m) + nested := isNestedName(res) + var nn string + if nested { + nn = nestedSingular(res) + } + + patterns := res.GetPattern() + singular := utils.GetResourceSingular(res) + + if !utils.IsSingletonResource(m) { + singular = fmt.Sprintf("{%s}", strcase.SnakeCase(singular)) + 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.HasSuffix(patterns[0], nn) || !strings.HasSuffix(patterns[0], singular)) + + for _, pattern := range patterns { + if !strings.HasSuffix(pattern, singular) { + // allow the reduced, nested name instead if present + if nested && strings.HasSuffix(pattern, nn) { + continue + } + want := singular + if nestedFirstPattern { + want = nn + } + + problems = append(problems, lint.Problem{ + Message: fmt.Sprintf("resource pattern %q final segment must include the resource singular %q", pattern, want), + Descriptor: m, + Location: locations.MessageResource(m), + }) + } + } + + return problems + }, +} diff --git a/rules/aip0123/resource_pattern_singular_test.go b/rules/aip0123/resource_pattern_singular_test.go new file mode 100644 index 000000000..31cc63442 --- /dev/null +++ b/rules/aip0123/resource_pattern_singular_test.go @@ -0,0 +1,127 @@ +// 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 TestResourcePatternSingularSimple(t *testing.T) { + for _, test := range []struct { + name string + Pattern string + problems testutils.Problems + }{ + {"Valid", "publishers/{publisher}/bookShelves/{book_shelf}", testutils.Problems{}}, + {"ValidSingleton", "publishers/{publisher}/bookShelf", 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"}}}, + } { + 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(resourcePatternSingular.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} + +func TestResourcePatternSingularNested(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: "ValidSingleton", + FirstPattern: "publishers/{publisher}/credit", + SecondPattern: "authors/{author}/credit", + problems: testutils.Problems{}, + }, + { + name: "ValidSingletonFull", + FirstPattern: "publishers/{publisher}/publisherCredit", + SecondPattern: "authors/{author}/publisherCredit", + problems: testutils.Problems{}, + }, + { + name: "InvalidSecondWithFirstNestedName", + FirstPattern: "publishers/{publisher}/credits/{credit}", + SecondPattern: "authors/{author}/credits/{published}", + problems: testutils.Problems{{Message: `final segment must include the resource singular "{credit}"`}}, + }, + { + name: "InvalidFirstWithReducedSecond", + FirstPattern: "publishers/{publisher}/credits/{published}", + SecondPattern: "authors/{author}/credits/{credit}", + problems: testutils.Problems{{Message: `final segment must include the resource singular "{credit}"`}}, + }, + { + name: "InvalidSingletonFirstPattern", + FirstPattern: "publishers/{publisher}/published", + SecondPattern: "authors/{author}/credit", + problems: testutils.Problems{{Message: `final segment must include the resource singular "credit"`}}, + }, + } { + 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(resourcePatternSingular.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} diff --git a/rules/internal/utils/extension.go b/rules/internal/utils/extension.go index be1994823..20a159d1c 100644 --- a/rules/internal/utils/extension.go +++ b/rules/internal/utils/extension.go @@ -135,19 +135,25 @@ func IsResource(m *desc.MessageDescriptor) bool { // IsSingletonResource returns true if the given message is a singleton // resource according to its pattern. func IsSingletonResource(m *desc.MessageDescriptor) bool { - // If the pattern ends in something other than "}", that indicates that this is a singleton. - // - // For example: - // publishers/{publisher}/books/{book} -- not a singleton, many books - // publishers/*/settings -- a singleton; one settings object per publisher for _, pattern := range GetResource(m).GetPattern() { - if !strings.HasSuffix(pattern, "}") { + if IsSingletonResourcePattern(pattern) { return true } } return false } +// IsSingletonResource returns true if the given message is a singleton +// resource according to its pattern. +func IsSingletonResourcePattern(pattern string) bool { + // If the pattern ends in something other than "}", that indicates that this is a singleton. + // + // For example: + // publishers/{publisher}/books/{book} -- not a singleton, many books + // publishers/*/settings -- a singleton; one settings object per publisher + return !strings.HasSuffix(pattern, "}") +} + // GetResourceDefinitions returns the google.api.resource_definition annotations // for a file. func GetResourceDefinitions(f *desc.FileDescriptor) []*apb.ResourceDescriptor {