diff --git a/docs/rules/0124/index.md b/docs/rules/0124/index.md new file mode 100644 index 000000000..e3eecbfa1 --- /dev/null +++ b/docs/rules/0124/index.md @@ -0,0 +1,10 @@ +--- +aip_listing: 124 +permalink: /124/ +redirect_from: + - /0124/ +--- + +# Resource association + +{% include linter-aip-listing.md aip=124 %} diff --git a/docs/rules/0124/valid-reference.md b/docs/rules/0124/valid-reference.md new file mode 100644 index 000000000..5bf2c8ceb --- /dev/null +++ b/docs/rules/0124/valid-reference.md @@ -0,0 +1,96 @@ +--- +rule: + aip: 124 + name: [core, '0124', valid-reference] + summary: Resource patterns should use consistent variable naming. +permalink: /124/valid-reference +redirect_from: + - /0124/valid-reference +--- + +# Valid resource references + +This rule enforces that resource reference annotations refer to valid and +reachable resource types, 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 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. + +Certain common resource types are exempt from this rule. + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect. +message Book { + // Needs a resource annotation; without one, resource references are invalid. + string name = 1; + + // ... +} + +message GetBookRequest { + string name = 1 [(google.api.resource_reference) = { + type: "library.googleapis.com/Book" // Lint warning; referennce not found. + }] +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +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 +message Book { + string name = 1; +} + +message GetBookRequest { + // (-- api-linter: core::0124::valid-reference=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/rules/aip0124/aip0124.go b/rules/aip0124/aip0124.go new file mode 100644 index 000000000..b41ad73e3 --- /dev/null +++ b/rules/aip0124/aip0124.go @@ -0,0 +1,29 @@ +// 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 contains rules defined in https://aip.dev/124. +package aip0124 + +import ( + "github.com/googleapis/api-linter/lint" +) + +// AddRules accepts a register function and registers each of +// this AIP's rules to it. +func AddRules(r lint.RuleRegistry) error { + return r.Register( + 124, + validReference, + ) +} diff --git a/rules/aip0124/aip0124_test.go b/rules/aip0124/aip0124_test.go new file mode 100644 index 000000000..b313c675d --- /dev/null +++ b/rules/aip0124/aip0124_test.go @@ -0,0 +1,27 @@ +// 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/lint" +) + +func TestAddRules(t *testing.T) { + if err := AddRules(lint.NewRuleRegistry()); err != nil { + t.Errorf("AddRules got an error: %v", err) + } +} diff --git a/rules/aip0124/valid_reference.go b/rules/aip0124/valid_reference.go new file mode 100644 index 000000000..67cec8298 --- /dev/null +++ b/rules/aip0124/valid_reference.go @@ -0,0 +1,99 @@ +// 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" + + "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 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 + }, + 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 getDependencies(f.GetFile(), f.GetFile().GetPackage()) { + // 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() { + if res := utils.GetResource(message); res != nil { + if res.GetType() == urt { + return nil + } + } + } + + // Some resources are defined as file annotations. Check for these too. + for _, rd := range utils.GetResourceDefinitions(file) { + if rd.GetType() == urt { + return nil + } + } + } + + // We could not find a resource with that type. Return a problem. + return []lint.Problem{{ + Message: fmt.Sprintf("Could not find resource of type %q", urt), + Descriptor: f, + Location: locations.FieldResourceReference(f), + }} + }, +} + +// getDependencies returns all dependencies in the same package. +func getDependencies(file *desc.FileDescriptor, pkg string) 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 { + answer[f.GetName()] = f + for name, f2 := range getDependencies(f, pkg) { + answer[name] = f2 + } + } + } + return answer +} diff --git a/rules/aip0124/valid_reference_test.go b/rules/aip0124/valid_reference_test.go new file mode 100644 index 000000000..7419a2b4a --- /dev/null +++ b/rules/aip0124/valid_reference_test.go @@ -0,0 +1,124 @@ +// 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 TestValidReference(t *testing.T) { + for _, test := range []struct { + name string + Field string + Ref string + problems testutils.Problems + }{ + {"Found", "type", "library.googleapis.com/Book", nil}, + {"FoundChild", "child_type", "library.googleapis.com/Book", nil}, + {"NotFound", "type", "library.googleapis.com/Bok", testutils.Problems{{Message: "Could not find"}}}, + {"NotFoundChild", "child_type", "library.googleapis.com/Bok", testutils.Problems{{Message: "Could not find"}}}, + {"Ignored", "type", "cloudresourcemanager.googleapis.com/Project", nil}, + } { + t.Run(test.name, func(t *testing.T) { + t.Run("SameFileResourceDefinition", func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/api/resource.proto"; + option (google.api.resource_definition) = { + type: "library.googleapis.com/Book" + }; + message Foo { + string book = 1 [(google.api.resource_reference).{{.Field}} = "{{.Ref}}"]; + string irrelevant = 2; + } + `, test) + field := f.GetMessageTypes()[0].GetFields()[0] + if diff := test.problems.SetDescriptor(field).Diff(validReference.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + + t.Run("SameFileResourceMessage", func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/api/resource.proto"; + message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + }; + } + message Foo { + string book = 1 [(google.api.resource_reference).{{.Field}} = "{{.Ref}}"]; + } + `, test) + field := f.GetMessageTypes()[1].GetFields()[0] + if diff := test.problems.SetDescriptor(field).Diff(validReference.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + }) + + t.Run("DirectDependency", func(t *testing.T) { + files := testutils.ParseProto3Tmpls(t, map[string]string{ + "dep.proto": ` + import "google/api/resource.proto"; + message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + }; + } + `, + "leaf.proto": ` + 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(validReference.Lint(file)); diff != "" { + t.Errorf(diff) + } + }) + + t.Run("RemoteDependency", func(t *testing.T) { + files := testutils.ParseProto3Tmpls(t, map[string]string{ + "dep.proto": ` + import "google/api/resource.proto"; + message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + }; + } + `, + "intermediate.proto": `import "dep.proto";`, + "leaf.proto": ` + 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(validReference.Lint(file)); diff != "" { + t.Errorf(diff) + } + }) + } +} diff --git a/rules/internal/testutils/parse.go b/rules/internal/testutils/parse.go index fe91818c8..78362ae97 100644 --- a/rules/internal/testutils/parse.go +++ b/rules/internal/testutils/parse.go @@ -82,20 +82,37 @@ func ParseProto3String(t *testing.T, src string) *desc.FileDescriptor { // It parses the template using Go's text/template Parse function, and then // calls ParseProto3String. func ParseProto3Tmpl(t *testing.T, src string, data interface{}) *desc.FileDescriptor { - // Create a new template object. - tmpl, err := template.New("test").Parse(src) - if err != nil { - t.Fatalf("Unable to parse Go template: %v", err) - } + return ParseProto3Tmpls(t, map[string]string{ + "test.proto": src, + }, data)["test.proto"] +} - // Execute the template and write the results to a bytes representing - // the desired proto. - var protoBytes bytes.Buffer - err = tmpl.Execute(&protoBytes, data) - if err != nil { - t.Fatalf("Unable to execute Go template: %v", err) +// ParseProto3Tmpls parses template strings representing a proto file, +// and returns FileDescriptors. +// +// It parses the template using Go's text/template Parse function, and then +// calls ParseProto3Strings. +func ParseProto3Tmpls(t *testing.T, srcs map[string]string, data interface{}) map[string]*desc.FileDescriptor { + strs := map[string]string{} + for fn, src := range srcs { + // Create a new template object. + tmpl, err := template.New("test").Parse(src) + if err != nil { + t.Fatalf("Unable to parse Go template: %v", err) + } + + // Execute the template and write the results to a bytes representing + // the desired proto. + var protoBytes bytes.Buffer + err = tmpl.Execute(&protoBytes, data) + if err != nil { + t.Fatalf("Unable to execute Go template: %v", err) + } + + // Add the proto to the map to send to parse strings. + strs[fn] = fmt.Sprintf("syntax = %q;\n\n%s", "proto3", protoBytes.String()) } // Parse the proto as a string. - return ParseProto3String(t, protoBytes.String()) + return ParseProtoStrings(t, strs) } diff --git a/rules/internal/utils/extension.go b/rules/internal/utils/extension.go index 5ea832c93..3c899d6b9 100644 --- a/rules/internal/utils/extension.go +++ b/rules/internal/utils/extension.go @@ -71,6 +71,16 @@ func GetResource(m *desc.MessageDescriptor) *apb.ResourceDescriptor { return nil } +// GetResourceDefinitions returns the google.api.resource_definition annotations +// for a file. +func GetResourceDefinitions(f *desc.FileDescriptor) []*apb.ResourceDescriptor { + opts := f.GetFileOptions() + if x := proto.GetExtension(opts, apb.E_ResourceDefinition); x != nil { + return x.([]*apb.ResourceDescriptor) + } + return nil +} + // GetResourceReference returns the google.api.resource_reference annotation. func GetResourceReference(f *desc.FieldDescriptor) *apb.ResourceReference { opts := f.GetFieldOptions() diff --git a/rules/internal/utils/extension_test.go b/rules/internal/utils/extension_test.go index 1c47116df..8bae2b0bd 100644 --- a/rules/internal/utils/extension_test.go +++ b/rules/internal/utils/extension_test.go @@ -156,6 +156,53 @@ func TestGetResource(t *testing.T) { }) } +func TestGetResourceDefinition(t *testing.T) { + t.Run("Zero", func(t *testing.T) { + f := testutils.ParseProto3String(t, ` + import "google/api/resource.proto"; + `) + if got := GetResourceDefinitions(f); got != nil { + t.Errorf("Got %v, expected nil.", got) + } + }) + t.Run("One", func(t *testing.T) { + f := testutils.ParseProto3String(t, ` + import "google/api/resource.proto"; + option (google.api.resource_definition) = { + type: "library.googleapis.com/Book" + }; + `) + defs := GetResourceDefinitions(f) + if got, want := len(defs), 1; got != want { + t.Errorf("Got %d definitions, expected %d.", got, want) + } + if got, want := defs[0].GetType(), "library.googleapis.com/Book"; got != want { + t.Errorf("Got %s for type, expected %s.", got, want) + } + }) + t.Run("Two", func(t *testing.T) { + f := testutils.ParseProto3String(t, ` + import "google/api/resource.proto"; + option (google.api.resource_definition) = { + type: "library.googleapis.com/Book" + }; + option (google.api.resource_definition) = { + type: "library.googleapis.com/Author" + }; + `) + defs := GetResourceDefinitions(f) + if got, want := len(defs), 2; got != want { + t.Errorf("Got %d definitions, expected %d.", got, want) + } + if got, want := defs[0].GetType(), "library.googleapis.com/Book"; got != want { + t.Errorf("Got %s for type, expected %s.", got, want) + } + if got, want := defs[1].GetType(), "library.googleapis.com/Author"; got != want { + t.Errorf("Got %s for type, expected %s.", got, want) + } + }) +} + func TestGetResourceReference(t *testing.T) { t.Run("Present", func(t *testing.T) { f := testutils.ParseProto3String(t, ` diff --git a/rules/internal/utils/string_pluralize.go b/rules/internal/utils/string_pluralize.go index 93beef6af..d447e5b37 100644 --- a/rules/internal/utils/string_pluralize.go +++ b/rules/internal/utils/string_pluralize.go @@ -20,6 +20,7 @@ import ( var pluralizeClient = pluralize.NewClient() +// ToPlural converts a string to its plural form. func ToPlural(s string) string { // Need to convert name to singular first to support none standard case such as persons, cactuses. // persons -> person -> people diff --git a/rules/rules.go b/rules/rules.go index be485c5f7..8c85cb56f 100644 --- a/rules/rules.go +++ b/rules/rules.go @@ -53,6 +53,7 @@ import ( "github.com/googleapis/api-linter/lint" "github.com/googleapis/api-linter/rules/aip0122" "github.com/googleapis/api-linter/rules/aip0123" + "github.com/googleapis/api-linter/rules/aip0124" "github.com/googleapis/api-linter/rules/aip0126" "github.com/googleapis/api-linter/rules/aip0127" "github.com/googleapis/api-linter/rules/aip0131" @@ -88,6 +89,7 @@ type addRulesFuncType func(lint.RuleRegistry) error var aipAddRulesFuncs = []addRulesFuncType{ aip0122.AddRules, aip0123.AddRules, + aip0124.AddRules, aip0126.AddRules, aip0127.AddRules, aip0131.AddRules,