From 26a1c823efba9d31b04a86f4071397df87d2ae4a Mon Sep 17 00:00:00 2001 From: Andrew Paseltiner Date: Wed, 21 Apr 2021 15:01:07 -0400 Subject: [PATCH] feat: Require a name field in messages with google.api.resource annotations (#839) --- docs/rules/0123/resource-name-field.md | 83 +++++++++++++++++++++++ rules/aip0123/aip0123.go | 1 + rules/aip0123/resource_name_field.go | 26 +++++++ rules/aip0123/resource_name_field_test.go | 55 +++++++++++++++ 4 files changed, 165 insertions(+) create mode 100644 docs/rules/0123/resource-name-field.md create mode 100644 rules/aip0123/resource_name_field.go create mode 100644 rules/aip0123/resource_name_field_test.go diff --git a/docs/rules/0123/resource-name-field.md b/docs/rules/0123/resource-name-field.md new file mode 100644 index 000000000..e5c90faab --- /dev/null +++ b/docs/rules/0123/resource-name-field.md @@ -0,0 +1,83 @@ +--- +rule: + aip: 123 + name: [core, '0123', resource-name-field] + summary: Resource messages should have a `string name` field. +permalink: /123/resource-name-field +redirect_from: + - /0123/resource-name-field +--- + +# Resource `name` field + +This rule enforces that messages that appear to represent resources have a +`string name` field, as described in [AIP-123][]. + +## Details + +This rule scans all messages that have a `google.api.resource` annotation, and +complains if the `name` field is missing or if it is any type other than +singular `string`. + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect: missing `string name` field. +message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "publishers/{publisher}/books/{book}" + }; +} +``` + +```proto +// Incorrect. +message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "publishers/{publisher}/books/{book}" + }; + + // Should be `string`, not `bytes`. + bytes name = 1; +} +``` + +**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; +} +``` + +## Disabling + +If you need to violate this rule, use a leading comment above the message, or +above the field if it is the wrong type. + +```proto +// (-- api-linter: core::0123::resource-name-field=disabled +// aip.dev/not-precedent: We need to do this because reasons. --) +message Book { + option (google.api.resource) = { + type: "library.googleapis.com/Book" + pattern: "publishers/{publisher}/books/{book}" + }; +} +``` + +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 diff --git a/rules/aip0123/aip0123.go b/rules/aip0123/aip0123.go index e3916d6ca..0f0af1933 100644 --- a/rules/aip0123/aip0123.go +++ b/rules/aip0123/aip0123.go @@ -33,6 +33,7 @@ func AddRules(r lint.RuleRegistry) error { 123, duplicateResource, resourceAnnotation, + resourceNameField, resourcePattern, resourceReferenceType, resourceVariables, diff --git a/rules/aip0123/resource_name_field.go b/rules/aip0123/resource_name_field.go new file mode 100644 index 000000000..c25874c29 --- /dev/null +++ b/rules/aip0123/resource_name_field.go @@ -0,0 +1,26 @@ +// Copyright 2021 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 ( + "github.com/googleapis/api-linter/lint" + "github.com/googleapis/api-linter/rules/internal/utils" +) + +var resourceNameField = &lint.MessageRule{ + Name: lint.NewRuleName(123, "resource-name-field"), + OnlyIf: utils.IsResource, + LintMessage: utils.LintFieldPresentAndSingularString("name"), +} diff --git a/rules/aip0123/resource_name_field_test.go b/rules/aip0123/resource_name_field_test.go new file mode 100644 index 000000000..5cacd9ac7 --- /dev/null +++ b/rules/aip0123/resource_name_field_test.go @@ -0,0 +1,55 @@ +// Copyright 2021 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 ( + "strings" + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" + "github.com/jhump/protoreflect/desc" +) + +func TestResourceNameField(t *testing.T) { + for _, test := range []struct { + name string + Options string + Field string + problems testutils.Problems + }{ + {"ValidBothPresent", `option (google.api.resource) = { type: "foo" };`, `string name = 1;`, nil}, + {"InvalidNoField", `option (google.api.resource) = { type: "foo" };`, ``, testutils.Problems{{Message: "`name`"}}}, + {"InvalidTypeNotString", `option (google.api.resource) = { type: "foo" };`, `int32 name = 1;`, testutils.Problems{{Suggestion: "string"}}}, + {"InvalidTypeRepeated", `option (google.api.resource) = { type: "foo" };`, `repeated string name = 1;`, testutils.Problems{{Suggestion: "string"}}}, + {"IrrelevantNoAnnotation", ``, ``, nil}, + } { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/api/resource.proto"; + message Book { + {{.Options}} + {{.Field}} + } + `, test) + var d desc.Descriptor = f.GetMessageTypes()[0] + if strings.HasPrefix(test.name, "InvalidType") { + d = f.GetMessageTypes()[0].GetFields()[0] + } + if diff := test.problems.SetDescriptor(d).Diff(resourceNameField.Lint(f)); diff != "" { + t.Error(diff) + } + }) + } +}