Repository: googleapis/api-linter
Pull request #626 (2020-09-16 18:16:29)

feat: Split the reference package mismatch into a new rule.
This splits out the package mismatch logic (complaining if a
string in package A references a resource defined in package B)
into its own rule.

This is useful because it allows this rule to be enabled/disabled
independently in situations where a package mismatch should be
permitted.


Patch:
diff --git a/docs/rules/0124/reference-same-package.md b/docs/rules/0124/reference-same-package.md
new file mode 100644
index 0000000..a93d401
--- /dev/null
+++ b/docs/rules/0124/reference-same-package.md
@@ -0,0 +1,111 @@
+---
+rule:
+  aip: 124
+  name: [core, '0124', reference-same-package]
+  summary: Resource references should refer to resources in the same package.
+permalink: /124/reference-same-package
+redirect_from:
+  - /0124/reference-same-package
+---
+
+# Resource reference package
+
+This rule enforces that resource reference annotations refer resources defined
+in the same package, as described in [AIP-124][].
+
+## Details
+
+This rule scans all fields with `google.api.resource_reference` annotations,
+and complains if the `type` on them refers to a resource that is defined in a
+different protobuf package.
+
+Certain common resource types are exempt from this rule.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+package google.example.library.v1;
+
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/Book"
+    pattern: "publishers/{publisher}/books/{book}"
+  };
+
+  string name = 1;
+
+  // ...
+}
+```
+
+```proto
+// Incorrect.
+package google.example.libray.v1;  // Typo: Different package.
+
+message GetBookRequest {
+  string name = 1 [(google.api.resource_reference) = {
+    type: "library.googleapis.com/Book"  // Lint warning: package mismatch.
+  }]
+}
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+package google.example.library;
+
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/Book"
+    pattern: "publishers/{publisher}/books/{book}"
+  };
+
+  string name = 1;
+
+  // ...
+}
+
+message GetBookRequest {
+  string name = 1 [(google.api.resource_reference) = {
+    type: "library.googleapis.com/Book"
+  }]
+}
+```
+
+## Disabling
+
+If you need to violate this rule, use a leading comment above the field.
+Remember to also include an [aip.dev/not-precedent][] comment explaining why.
+
+```proto
+package google.example.library.common;
+
+message Book {
+  option (google.api.resource) = {
+    type: "library.googleapis.com/Book"
+    pattern: "publishers/{publisher}/books/{book}"
+  };
+}
+```
+
+```proto
+package google.example.library.v1;
+
+message GetBookRequest {
+  // (-- api-linter: core::0124::reference-same-package=disabled
+  //     aip.dev/not-precedent: We need to do this because reasons. --)
+  string name = 1 [(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-124]: http://aip.dev/124
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
diff --git a/docs/rules/0124/valid-reference.md b/docs/rules/0124/valid-reference.md
index 5697d7f..1b1066b 100644
--- a/docs/rules/0124/valid-reference.md
+++ b/docs/rules/0124/valid-reference.md
@@ -2,7 +2,7 @@
 rule:
   aip: 124
   name: [core, '0124', valid-reference]
-  summary: Resource patterns should use consistent variable naming.
+  summary: Resource references should have corresponding resource defintions.
 permalink: /124/valid-reference
 redirect_from:
   - /0124/valid-reference
@@ -20,7 +20,7 @@ and complains if the `type` on them refers to a resource with no corresponding
 `google.api.resource` or `google.api.resource_definition`.
 
 The rule scans the file where the field is found and all files imported by that
-file (recursively) as long as they are in the same package.
+file (recursively).
 
 Certain common resource types are exempt from this rule.
 
@@ -90,7 +90,3 @@ top of the file.
 
 [aip-124]: http://aip.dev/124
 [aip.dev/not-precedent]: https://aip.dev/not-precedent
-
-```
-
-```
diff --git a/rules/aip0124/aip0124.go b/rules/aip0124/aip0124.go
index b41ad73..8a7cbd1 100644
--- a/rules/aip0124/aip0124.go
+++ b/rules/aip0124/aip0124.go
@@ -16,7 +16,10 @@
 package aip0124
 
 import (
+	"bitbucket.org/creachadair/stringset"
 	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/rules/internal/utils"
+	"github.com/jhump/protoreflect/desc"
 )
 
 // AddRules accepts a register function and registers each of
@@ -24,6 +27,34 @@ import (
 func AddRules(r lint.RuleRegistry) error {
 	return r.Register(
 		124,
+		referenceSamePackage,
 		validReference,
 	)
 }
+
+// isUnknownType returns true if and only if the type is not a common,
+// well known type.
+func isUnknownType(f *desc.FieldDescriptor) bool {
+	if ref := utils.GetResourceReference(f); ref != nil {
+		urt := ref.GetType()
+		if urt == "" {
+			urt = ref.GetChildType()
+		}
+		return !stringset.New(
+			// Allow the common resource types in GCP.
+			// FIXME: Modularize this.
+			"cloudresourcemanager.googleapis.com/Project",
+			"cloudresourcemanager.googleapis.com/Organization",
+			"cloudresourcemanager.googleapis.com/Folder",
+			"billing.googleapis.com/BillingAccount",
+			"locations.googleapis.com/Location",
+
+			// Allow *.
+			"*",
+
+			// If no type is declared, ignore this.
+			"",
+		).Contains(urt)
+	}
+	return false
+}
diff --git a/rules/aip0124/reference_same_package.go b/rules/aip0124/reference_same_package.go
new file mode 100644
index 0000000..a381963
--- /dev/null
+++ b/rules/aip0124/reference_same_package.go
@@ -0,0 +1,76 @@
+// Copyright 2020 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 aip0124
+
+import (
+	"fmt"
+
+	"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 referenceSamePackage = &lint.FieldRule{
+	Name:   lint.NewRuleName(124, "reference-same-package"),
+	OnlyIf: isUnknownType,
+	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
+		// Get the type we are checking for.
+		ref := utils.GetResourceReference(f)
+		urt := ref.GetType()
+		if urt == "" {
+			urt = ref.GetChildType()
+		}
+
+		// Iterate over each dependency file and check for a matching resource.
+		for _, file := range getNonPkgDependencies(f.GetFile(), f.GetFile().GetPackage()) {
+			// If we find a message with a resource annotation matching our universal
+			// resource type, then it is in the wrong package.
+			for _, message := range file.GetMessageTypes() {
+				if res := utils.GetResource(message); res != nil && res.GetType() == urt {
+					return []lint.Problem{{
+						Message:    fmt.Sprintf("Resource type %q should be declared in the same package as it is referenced.", urt),
+						Descriptor: f,
+						Location:   locations.FieldResourceReference(f),
+					}}
+				}
+			}
+
+			// Some resources are defined as file annotations. Check for these too.
+			for _, rd := range utils.GetResourceDefinitions(file) {
+				if rd.GetType() == urt {
+					return []lint.Problem{{
+						Message:    fmt.Sprintf("Resource type %q should be declared in the same package as it is referenced.", urt),
+						Descriptor: f,
+						Location:   locations.FieldResourceReference(f),
+					}}
+				}
+			}
+		}
+
+		return nil
+	},
+}
+
+// getNonPkgDependencies returns dependencies in other packages.
+func getNonPkgDependencies(file *desc.FileDescriptor, pkg string) map[string]*desc.FileDescriptor {
+	answer := map[string]*desc.FileDescriptor{}
+	for name, dep := range getAllDependencies(file) {
+		if dep.GetPackage() != pkg {
+			answer[name] = dep
+		}
+	}
+	return answer
+}
diff --git a/rules/aip0124/valid_reference.go b/rules/aip0124/valid_reference.go
index 67cec82..c2f0ef5 100644
--- a/rules/aip0124/valid_reference.go
+++ b/rules/aip0124/valid_reference.go
@@ -17,7 +17,6 @@ package aip0124
 import (
 	"fmt"
 
-	"bitbucket.org/creachadair/stringset"
 	"github.com/googleapis/api-linter/lint"
 	"github.com/googleapis/api-linter/locations"
 	"github.com/googleapis/api-linter/rules/internal/utils"
@@ -25,28 +24,8 @@ import (
 )
 
 var validReference = &lint.FieldRule{
-	Name: lint.NewRuleName(124, "valid-reference"),
-	OnlyIf: func(f *desc.FieldDescriptor) bool {
-		if ref := utils.GetResourceReference(f); ref != nil {
-			urt := ref.GetType()
-			if urt == "" {
-				urt = ref.GetChildType()
-			}
-			return !stringset.New(
-				// Whitelist the common resource types in GCP.
-				// FIXME: Modularize this.
-				"cloudresourcemanager.googleapis.com/Project",
-				"cloudresourcemanager.googleapis.com/Organization",
-				"cloudresourcemanager.googleapis.com/Folder",
-				"billing.googleapis.com/BillingAccount",
-				"locations.googleapis.com/Location",
-
-				// If no type is declared, ignore this.
-				"",
-			).Contains(urt)
-		}
-		return false
-	},
+	Name:   lint.NewRuleName(124, "valid-reference"),
+	OnlyIf: isUnknownType,
 	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
 		// Get the type we are checking for.
 		ref := utils.GetResourceReference(f)
@@ -56,7 +35,7 @@ var validReference = &lint.FieldRule{
 		}
 
 		// Iterate over each dependency file and check for a matching resource.
-		for _, file := range getDependencies(f.GetFile(), f.GetFile().GetPackage()) {
+		for _, file := range getAllDependencies(f.GetFile()) {
 			// Most resources will be messages. If we find a message with a
 			// resource annotation matching our universal resource type, we are done.
 			for _, message := range file.GetMessageTypes() {
@@ -84,13 +63,13 @@ var validReference = &lint.FieldRule{
 	},
 }
 
-// getDependencies returns all dependencies in the same package.
-func getDependencies(file *desc.FileDescriptor, pkg string) map[string]*desc.FileDescriptor {
+// getAllDependencies returns all dependencies.
+func getAllDependencies(file *desc.FileDescriptor) map[string]*desc.FileDescriptor {
 	answer := map[string]*desc.FileDescriptor{file.GetName(): file}
 	for _, f := range file.GetDependencies() {
-		if _, found := answer[f.GetName()]; !found && f.GetPackage() == pkg {
+		if _, found := answer[f.GetName()]; !found {
 			answer[f.GetName()] = f
-			for name, f2 := range getDependencies(f, pkg) {
+			for name, f2 := range getAllDependencies(f) {
 				answer[name] = f2
 			}
 		}


Test patch:
diff --git a/rules/aip0124/reference_same_package_test.go b/rules/aip0124/reference_same_package_test.go
new file mode 100644
index 0000000..7f35754
--- /dev/null
+++ b/rules/aip0124/reference_same_package_test.go
@@ -0,0 +1,119 @@
+// Copyright 2020 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 aip0124
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestReferenceSamePackage(t *testing.T) {
+	for _, test := range []struct {
+		name         string
+		Field        string
+		Ref          string
+		OtherPackage string
+		problems     testutils.Problems
+	}{
+		{"SamePkg", "type", "library.googleapis.com/Book", "same", nil},
+		{"SamePkgChild", "child_type", "library.googleapis.com/Book", "same", nil},
+		{"NotSamePkg", "type", "library.googleapis.com/Book", "other", testutils.Problems{{Message: "same package"}}},
+		{"NotSamePkgChild", "child_type", "library.googleapis.com/Book", "other", testutils.Problems{{Message: "same package"}}},
+		{"Ignored", "type", "cloudresourcemanager.googleapis.com/Project", "other", nil},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			t.Run("DirectDependency", func(t *testing.T) {
+				files := testutils.ParseProto3Tmpls(t, map[string]string{
+					"dep.proto": `
+						package {{.OtherPackage}};
+						import "google/api/resource.proto";
+						message Book {
+							option (google.api.resource) = {
+								type: "{{.Ref}}"
+							};
+						}
+					`,
+					"leaf.proto": `
+						package same;
+						import "google/api/resource.proto";
+						import "dep.proto";
+						message Foo {
+							string book = 1 [(google.api.resource_reference).{{.Field}} = "{{.Ref}}"];
+						}
+					`,
+				}, test)
+				file := files["leaf.proto"]
+				field := file.GetMessageTypes()[0].GetFields()[0]
+				if diff := test.problems.SetDescriptor(field).Diff(referenceSamePackage.Lint(file)); diff != "" {
+					t.Errorf(diff)
+				}
+			})
+
+			t.Run("DirectDependencyResourceDefinition", func(t *testing.T) {
+				files := testutils.ParseProto3Tmpls(t, map[string]string{
+					"dep.proto": `
+						package {{.OtherPackage}};
+						import "google/api/resource.proto";
+						option (google.api.resource_definition) = {
+							type: "{{.Ref}}"
+						};
+					`,
+					"leaf.proto": `
+						package same;
+						import "google/api/resource.proto";
+						import "dep.proto";
+						message Foo {
+							string book = 1 [(google.api.resource_reference).{{.Field}} = "{{.Ref}}"];
+						}
+					`,
+				}, test)
+				file := files["leaf.proto"]
+				field := file.GetMessageTypes()[0].GetFields()[0]
+				if diff := test.problems.SetDescriptor(field).Diff(referenceSamePackage.Lint(file)); diff != "" {
+					t.Errorf(diff)
+				}
+			})
+
+			t.Run("RemoteDependency", func(t *testing.T) {
+				files := testutils.ParseProto3Tmpls(t, map[string]string{
+					"dep.proto": `
+						package {{.OtherPackage}};
+						import "google/api/resource.proto";
+						message Book {
+							option (google.api.resource) = {
+								type: "library.googleapis.com/Book"
+							};
+						}
+					`,
+					"intermediate.proto": `import "dep.proto";`,
+					"leaf.proto": `
+						package same;
+						import "google/api/resource.proto";
+						import "intermediate.proto";
+						message Foo {
+							string book = 1 [(google.api.resource_reference).{{.Field}} = "{{.Ref}}"];
+						}
+					`,
+				}, test)
+				file := files["leaf.proto"]
+				field := file.GetMessageTypes()[0].GetFields()[0]
+				if diff := test.problems.SetDescriptor(field).Diff(referenceSamePackage.Lint(file)); diff != "" {
+					t.Errorf(diff)
+				}
+			})
+		})
+	}
+}