feat(AIP-121): lint for mutable reference cycles (googleapis#1238)

Introduces rules to lint for mutable resource reference cycles.

Fixes googleapis#1109

docs/rules/0121/no-mutable-cycles.md

@@ -0,0 +1,129 @@
+---
+rule:
+ aip: 121
+ name: [core, '0121', no-mutable-cycles]
+ summary: Resources must not form a resource reference cycle.
+permalink: /121/no-mutable-cycles
+redirect_from:
+ - /0121/no-mutable-cycles
+---
+
+# Resource must support get
+
+This rule enforces that resources do not create reference cycles of mutable 
+references as mandated in [AIP-121][].
+
+## Details
+
+This rule scans the fields of every resource and ensures that any references to
+other resources do not create a mutable cycle between them.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+message Book {
+ option (google.api.resource) = {
+ type: "library.googleapis.com/Book"
+ pattern: "books/{book}"
+ };
+
+ string name = 1;
+
+ // Incorrect. Creates potential reference cycle.
+ 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;
+
+ // Incorrect. Creates potential reference cycle.
+ string book = 2 [
+ (google.api.resource_reference).type = "library.googleapis.com/Book"
+ ];
+}
+```
+
+**Correct** code for this rule:
+
+```proto
+message Book {
+ option (google.api.resource) = {
+ type: "library.googleapis.com/Book"
+ pattern: "books/{book}"
+ };
+
+ string name = 1;
+
+ // Correct because the other reference is OUTPUT_ONLY.
+ 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;
+
+ // Correct because an OUTPUT_ONLY reference breaks the mutation cycle.
+ string book = 2 [
+ (google.api.resource_reference).type = "library.googleapis.com/Book",
+ (google.api.field_behavior) = OUTPUT_ONLY
+ ];
+}
+```
+
+## 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
+message Book {
+ option (google.api.resource) = {
+ type: "library.googleapis.com/Book"
+ pattern: "books/{book}"
+ };
+
+ string name = 1;
+
+ // (-- api-linter: core::0121::no-mutable-cycles=disabled
+ // aip.dev/not-precedent: We need to do this because reasons. --)
+ 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;
+
+ // (-- api-linter: core::0121::no-mutable-cycles=disabled
+ // aip.dev/not-precedent: We need to do this because reasons. --)
+ string book = 2 [
+ (google.api.resource_reference).type = "library.googleapis.com/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

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

rules/aip0121/no_mutable_cycles.go

@@ -0,0 +1,90 @@
+// 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 (
+ "strings"
+
+ "bitbucket.org/creachadair/stringset"
+ "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 noMutableCycles = &lint.MessageRule{
+ Name: lint.NewRuleName(121, "no-mutable-cycles"),
+ OnlyIf: utils.IsResource,
+ LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
+ res := utils.GetResource(m)
+
+ return findCycles(res.GetType(), m, stringset.New(), nil)
+ },
+}
+
+func findCycles(start string, node *desc.MessageDescriptor, seen stringset.Set, chain []string) []lint.Problem {
+ var problems []lint.Problem
+ nodeRes := utils.GetResource(node)
+
+ chain = append(chain, nodeRes.GetType())
+ seen.Add(nodeRes.GetType())
+
+ for _, f := range node.GetFields() {
+ if !isMutableReference(f) {
+ continue
+ }
+ ref := utils.GetResourceReference(f)
+ // Skip indirect references for now.
+ if ref.GetChildType() != "" {
+ continue
+ }
+ if ref.GetType() == start {
+ cycle := strings.Join(append(chain, start), " > ")
+ problems = append(problems, lint.Problem{
+ Message: "mutable resource reference introduces a reference cycle:\n" + cycle,
+ Descriptor: f,
+ Location: locations.FieldResourceReference(f),
+ })
+ } else if !seen.Contains(ref.GetType()) {
+ next := utils.FindResourceMessage(ref.GetType(), node.GetFile())
+ // Skip unresolvable references.
+ if next == nil {
+ continue
+ }
+ if probs := findCycles(start, next, seen.Clone(), chain); len(probs) == 1 {
+ // A recursive call will only have one finding as it returns
+ // immediately.
+ p := probs[0]
+ p.Descriptor = f
+ p.Location = locations.FieldResourceReference(f)
+
+ problems = append(problems, p)
+ }
+ }
+ // Recursive calls should return immediately upon finding a cycle.
+ // The initial scan of the starting resource message should scan
+ // all of its own fields and not return immediately.
+ if len(problems) > 0 && len(chain) > 1 {
+ return problems
+ }
+ }
+
+ return problems
+}
+
+func isMutableReference(f *desc.FieldDescriptor) bool {
+ behaviors := utils.GetFieldBehavior(f)
+ return utils.HasResourceReference(f) && !behaviors.Contains("OUTPUT_ONLY")
+}

rules/aip0121/no_mutable_cycles_test.go

@@ -0,0 +1,159 @@
+// 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/rules/internal/testutils"
+)
+
+func TestNoMutableCycles(t *testing.T) {
+
+ for _, test := range []struct {
+ name string
+ BookExtensions, PublisherExtensions, LibraryExtensions, OtherPublisherExtensions string
+ problems testutils.Problems
+ }{
+ {
+ "ValidNoCycle",
+ `[(google.api.resource_reference).type = "library.googleapis.com/Library"]`,
+ `[(google.api.resource_reference).type = "library.googleapis.com/Library"]`,
+ "",
+ "",
+ nil,
+ },
+ {
+ "InvalidCycle",
+ `[(google.api.resource_reference).type = "library.googleapis.com/Publisher"]`,
+ `[(google.api.resource_reference).type = "library.googleapis.com/Book"]`,
+ "",
+ "",
+ testutils.Problems{{
+ Message: "cycle",
+ }},
+ },
+ {
+ "InvalidSelfReferenceCycle",
+ "",
+ `[(google.api.resource_reference).type = "library.googleapis.com/Publisher"]`,
+ "",
+ "",
+ testutils.Problems{{
+ Message: "cycle",
+ }},
+ },
+ {
+ "InvalidDeepCycle",
+ `[(google.api.resource_reference).type = "library.googleapis.com/Publisher"]`,
+ `[(google.api.resource_reference).type = "library.googleapis.com/Library"]`,
+ `[(google.api.resource_reference).type = "library.googleapis.com/Book"]`,
+ "",
+ testutils.Problems{{
+ Message: "cycle",
+ }},
+ },
+ {
+ "InvalidDeepAndShallowCycles",
+ `[(google.api.resource_reference).type = "library.googleapis.com/Publisher"]`,
+ `[(google.api.resource_reference).type = "library.googleapis.com/Library"]`,
+ `[(google.api.resource_reference).type = "library.googleapis.com/Book"]`,
+ `[(google.api.resource_reference).type = "library.googleapis.com/Book"]`,
+ testutils.Problems{
+ {
+ Message: "cycle",
+ },
+ {
+ Message: "cycle",
+ },
+ },
+ },
+ {
+ "ValidOutputOnlyCyclicReference",
+ `[(google.api.resource_reference).type = "library.googleapis.com/Publisher"]`,
+ `[
+ (google.api.resource_reference).type = "library.googleapis.com/Book",
+ (google.api.field_behavior) = OUTPUT_ONLY
+ ]`,
+ "",
+ "",
+ nil,
+ },
+ {
+ "ValidOutputOnlyDeepCyclicReference",
+ `[(google.api.resource_reference).type = "library.googleapis.com/Publisher"]`,
+ `[(google.api.resource_reference).type = "library.googleapis.com/Library"]`,
+ `[
+ (google.api.resource_reference).type = "library.googleapis.com/Book",
+ (google.api.field_behavior) = OUTPUT_ONLY
+ ]`,
+ "",
+ nil,
+ },
+ } {
+ t.Run(test.name, func(t *testing.T) {
+ f := testutils.ParseProto3Tmpl(t, `
+ import "google/api/resource.proto";
+ import "google/api/field_behavior.proto";
+ message Book {
+ option (google.api.resource) = {
+ type: "library.googleapis.com/Book"
+ pattern: "publishers/{publisher}/books/{book}"
+ };
+ string name = 1;
+
+ string resource = 2 {{.BookExtensions}};
+ }
+
+ message Publisher {
+ option (google.api.resource) = {
+ type: "library.googleapis.com/Publisher"
+ pattern: "publishers/{publisher}"
+ };
+ string name = 1;
+
+ string resource = 2 {{.PublisherExtensions}};
+
+ string other_resource = 3 {{.OtherPublisherExtensions}};
+ }
+
+ message Library {
+ option (google.api.resource) = {
+ type: "library.googleapis.com/Library"
+ pattern: "libraries/{library}"
+ };
+ string name = 1;
+
+ string resource = 3 {{.LibraryExtensions}};
+ }
+ `, test)
+
+ msg := f.FindMessage("Publisher")
+ want := test.problems
+ if len(want) >= 1 {
+ want[0].Descriptor = msg.FindFieldByName("resource")
+ }
+ if len(want) == 2 {
+ want[1].Descriptor = msg.FindFieldByName("other_resource")
+ }
+ // If this rule was run on the entire test file, there would be two
+ // findings, one for each resource in the cycle. To simplify that,
+ // we just lint one of the offending messages.
+ if diff := want.Diff(noMutableCycles.LintMessage(msg)); diff != "" {
+ t.Error(diff)
+ }
+ })
+ }
+}