From e3bbdb313f7507139d7ec91995200b9bc543d7ae Mon Sep 17 00:00:00 2001 From: Noah Dietz Date: Thu, 31 Aug 2023 13:42:25 -0700 Subject: [PATCH] feat(AIP-122): disallow embedded resource (#1245) --- docs/rules/0122/embedded-resource.md | 107 ++++++++++++++++ rules/aip0122/aip0122.go | 1 + rules/aip0122/embedded_resource.go | 54 ++++++++ rules/aip0122/embedded_resource_test.go | 159 ++++++++++++++++++++++++ rules/internal/utils/resource.go | 28 +++++ rules/internal/utils/resource_test.go | 79 ++++++++++++ 6 files changed, 428 insertions(+) create mode 100644 docs/rules/0122/embedded-resource.md create mode 100644 rules/aip0122/embedded_resource.go create mode 100644 rules/aip0122/embedded_resource_test.go diff --git a/docs/rules/0122/embedded-resource.md b/docs/rules/0122/embedded-resource.md new file mode 100644 index 000000000..4ec1d7417 --- /dev/null +++ b/docs/rules/0122/embedded-resource.md @@ -0,0 +1,107 @@ +--- +rule: + aip: 122 + name: [core, '0122', embedded-resource] + summary: Resource references should not be embedded resources. +permalink: /122/embedded-resource +redirect_from: + - /0122/embedded-resource +--- + +# Resource reference type + +This rule enforces that references to resource are via +`google.api.resource_reference`, not by embedding the resource message, as +mandated in [AIP-122][]. + +## Details + +This rule complains if it sees a resource field of type `message` that is also +annotated as a `google.api.resource`. + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect. +message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "books/{book}" + }; + string name = 1; + + // Incorrect. Resource references should not be embedded resource messages. + Author author = 2; +} + +message Author { + option (google.api.resource) = { + type: "library.googleapis.com/Author" + pattern: "authors/{author}" + }; + + string name = 1; +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "books/{book}" + }; + string name = 1; + + string author = 2 [(google.api.resource_reference) = { + type: "library.googleapis.com/Author" + }]; +} + +message Author { + option (google.api.resource) = { + type: "library.googleapis.com/Author" + pattern: "authors/{author}" + }; + + string name = 1; +} +``` + +## Disabling + +If you need to violate this rule, use a leading comment above the method. +Remember to also include an [aip.dev/not-precedent][] comment explaining why. + +```proto +message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "books/{book}" + }; + string name = 1; + + // (-- api-linter: core::0122::embedded-resource=disabled + // aip.dev/not-precedent: We need to do this because reasons. --) + Author author = 2; +} + +message Author { + option (google.api.resource) = { + type: "library.googleapis.com/Author" + pattern: "authors/{author}" + }; + + string name = 1; +} +``` + +If you need to violate this rule for an entire file, place the comment at the +top of the file. + +[aip-122]: https://aip.dev/122 +[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/rules/aip0122/aip0122.go b/rules/aip0122/aip0122.go index e5075ba32..e9a17fab1 100644 --- a/rules/aip0122/aip0122.go +++ b/rules/aip0122/aip0122.go @@ -29,5 +29,6 @@ func AddRules(r lint.RuleRegistry) error { nameSuffix, resourceReferenceType, resourceIdOutputOnly, + embeddedResource, ) } diff --git a/rules/aip0122/embedded_resource.go b/rules/aip0122/embedded_resource.go new file mode 100644 index 000000000..01d8a6946 --- /dev/null +++ b/rules/aip0122/embedded_resource.go @@ -0,0 +1,54 @@ +// 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 aip0122 + +import ( + "fmt" + + "github.com/googleapis/api-linter/lint" + "github.com/googleapis/api-linter/rules/internal/utils" + "github.com/jhump/protoreflect/desc" +) + +var embeddedResource = &lint.MessageRule{ + Name: lint.NewRuleName(122, "embedded-resource"), + OnlyIf: utils.IsResource, + LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { + var problems []lint.Problem + for _, f := range m.GetFields() { + mt := f.GetMessageType() + if mt == nil || !utils.IsResource(mt) { + continue + } + + r := utils.GetResource(mt) + if utils.IsResourceRevision(m) && utils.IsRevisionRelationship(r, utils.GetResource(m)) { + continue + } + + suggestion := fmt.Sprintf(`string %s = %d [(google.api.resource_reference).type = %q];`, f.GetName(), f.GetNumber(), r.GetType()) + if f.IsRepeated() { + suggestion = fmt.Sprintf("repeated %s", suggestion) + } + problems = append(problems, lint.Problem{ + Message: "refer to a resource by name, not by embedding the resource message", + Descriptor: f, + Suggestion: suggestion, + }) + } + + return problems + }, +} diff --git a/rules/aip0122/embedded_resource_test.go b/rules/aip0122/embedded_resource_test.go new file mode 100644 index 000000000..d7b0e6e66 --- /dev/null +++ b/rules/aip0122/embedded_resource_test.go @@ -0,0 +1,159 @@ +// Copyright 2022 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 aip0122 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +func TestEmbeddedResource(t *testing.T) { + for _, test := range []struct { + name string + FieldA, FieldB string + problems testutils.Problems + }{ + {"Valid", "", "", nil}, + { + "ValidReferences", + `string library = 2 [(google.api.resource_reference).type = "library.googleapis.com/Library"];`, + `string librarian = 3 [(google.api.resource_reference).type = "library.googleapis.com/Librarian"];`, + nil, + }, + { + "InvalidEmbeddedResources", + "Library library = 2;", + "Librarian librarian = 3;", + testutils.Problems{ + { + Message: "not by embedding", + Suggestion: `string library = 2 [(google.api.resource_reference).type = "library.googleapis.com/Library"];`, + }, + { + Message: "not by embedding", + Suggestion: `string librarian = 3 [(google.api.resource_reference).type = "library.googleapis.com/Librarian"];`, + }, + }, + }, + { + "InvalidRepeatedEmbeddedResource", + "repeated Library library = 2;", + "", + testutils.Problems{{ + Message: "not by embedding", + Suggestion: `repeated string library = 2 [(google.api.resource_reference).type = "library.googleapis.com/Library"];`, + }}, + }, + } { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/api/resource.proto"; + message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "publishers/{publisher}/books/{book}" + }; + string name = 1; + + {{.FieldA}} + + {{.FieldB}} + } + + message Library { + option (google.api.resource) = { + type: "library.googleapis.com/Library" + pattern: "libraries/{library}" + }; + string name = 1; + } + + message Librarian { + option (google.api.resource) = { + type: "library.googleapis.com/Librarian" + pattern: "libraries/{library}/librarians/{librarian}" + }; + string name = 1; + } + `, test) + m := f.FindMessage("Book") + + want := test.problems + if len(want) > 0 { + want[0].Descriptor = m.FindFieldByName("library") + } + if len(want) > 1 { + want[1].Descriptor = m.FindFieldByName("librarian") + } + if diff := want.Diff(embeddedResource.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} + +func TestEmbeddedResource_Revisions(t *testing.T) { + for _, test := range []struct { + name string + SnapshotType string + problems testutils.Problems + }{ + {"Valid", "Book", nil}, + { + "InvalidEmbeddedResource", + "Library", + testutils.Problems{{ + Message: "not by embedding", + Suggestion: `string snapshot = 2 [(google.api.resource_reference).type = "library.googleapis.com/Library"];`, + }}, + }, + } { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/api/resource.proto"; + message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "publishers/{publisher}/books/{book}" + }; + string name = 1; + } + + message BookRevision { + option (google.api.resource) = { + type: "library.googleapis.com/BookRevision" + pattern: "publishers/{publisher}/books/{book}/revisions/{revision}" + }; + string name = 1; + + {{.SnapshotType}} snapshot = 2; + } + + message Library { + option (google.api.resource) = { + type: "library.googleapis.com/Library" + pattern: "libraries/{library}" + }; + string name = 1; + } + `, test) + field := f.FindMessage("BookRevision").FindFieldByName("snapshot") + if diff := test.problems.SetDescriptor(field).Diff(embeddedResource.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} diff --git a/rules/internal/utils/resource.go b/rules/internal/utils/resource.go index 6e6e0a204..a4408f016 100644 --- a/rules/internal/utils/resource.go +++ b/rules/internal/utils/resource.go @@ -15,6 +15,9 @@ package utils import ( + "strings" + + "github.com/jhump/protoreflect/desc" apb "google.golang.org/genproto/googleapis/api/annotations" ) @@ -62,3 +65,28 @@ func GetResourceNameField(r *apb.ResourceDescriptor) string { } return "name" } + +// IsResourceRevision determines if the given message represents a resource +// revision as described in AIP-162. +func IsResourceRevision(m *desc.MessageDescriptor) bool { + return IsResource(m) && strings.HasSuffix(m.GetName(), "Revision") +} + +// IsRevisionRelationship determines if the "revision" resource is actually +// a revision of the "parent" resource. +func IsRevisionRelationship(parent, revision *apb.ResourceDescriptor) bool { + _, pType, ok := SplitResourceTypeName(parent.GetType()) + if !ok { + return false + } + _, rType, ok := SplitResourceTypeName(revision.GetType()) + if !ok { + return false + } + + if !strings.HasSuffix(rType, "Revision") { + return false + } + rType = strings.TrimSuffix(rType, "Revision") + return pType == rType +} diff --git a/rules/internal/utils/resource_test.go b/rules/internal/utils/resource_test.go index cc38b2e59..26b994934 100644 --- a/rules/internal/utils/resource_test.go +++ b/rules/internal/utils/resource_test.go @@ -17,6 +17,7 @@ package utils import ( "testing" + "github.com/googleapis/api-linter/rules/internal/testutils" apb "google.golang.org/genproto/googleapis/api/annotations" ) @@ -102,3 +103,81 @@ func TestGetResourcePlural(t *testing.T) { }) } } + +func TestIsResourceRevision(t *testing.T) { + for _, test := range []struct { + name, Message, Resource string + want bool + }{ + { + name: "valid_revision", + Message: "BookRevision", + Resource: `option (google.api.resource) = {type: "library.googleapis.com/BookRevision"};`, + want: true, + }, + { + name: "not_revision_no_resource", + Message: "BookRevision", + want: false, + }, + { + name: "not_revision_bad_name", + Message: "Book", + Resource: `option (google.api.resource) = {type: "library.googleapis.com/Book"};`, + want: false, + }, + } { + f := testutils.ParseProto3Tmpl(t, ` + import "google/api/resource.proto"; + message {{.Message}} { + {{.Resource}} + string name = 1; + } + `, test) + m := f.FindMessage(test.Message) + if got := IsResourceRevision(m); got != test.want { + t.Errorf("IsResourceRevision(%+v): got %v, want %v", m, got, test.want) + } + } +} + +func TestIsRevisionRelationship(t *testing.T) { + for _, test := range []struct { + name string + typeA, typeB string + want bool + }{ + { + name: "revision_relationship", + typeA: "library.googleapis.com/Book", + typeB: "library.googleapis.com/BookRevision", + want: true, + }, + { + name: "non_revision_relationship", + typeA: "library.googleapis.com/Book", + typeB: "library.googleapis.com/Library", + want: false, + }, + { + name: "invalid_type_a", + typeA: "library.googleapis.com", + typeB: "library.googleapis.com/Library", + want: false, + }, + { + name: "invalid_type_b", + typeA: "library.googleapis.com/Book", + typeB: "library.googleapis.com", + want: false, + }, + } { + t.Run(test.name, func(t *testing.T) { + a := &apb.ResourceDescriptor{Type: test.typeA} + b := &apb.ResourceDescriptor{Type: test.typeB} + if got := IsRevisionRelationship(a, b); got != test.want { + t.Errorf("IsRevisionRelationship(%s, %s): got %v, want %v", test.typeA, test.typeB, got, test.want) + } + }) + } +}