diff --git a/docs/rules/0135/force-field.md b/docs/rules/0135/force-field.md new file mode 100644 index 000000000..5a7b3cbaf --- /dev/null +++ b/docs/rules/0135/force-field.md @@ -0,0 +1,73 @@ +--- +rule: + aip: 135 + name: [core, '0135', force-field] + summary: Delete RPCs for resources with child collections should have a `force` field in the request. +permalink: /135/force-field +redirect_from: + - /0135/force-field +--- + +# Delete methods: Name field + +This rule enforces that the standard `Delete` method for a resource that parents +other resources in the service have a `bool force` field in the request message, +as mandated in [AIP-135][]. + +## Details + +This rule looks at any message matching `Delete*Request` for a resource with +child resources in the same service and complains if the `force` field is +missing. + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect. +message DeletePublisherRequest { + // Where Publisher parents the Book resource. + string name = 1 [ + (google.api.resource_reference).type = "library.googleapis.com/Publisher"]; + + // Missing `bool force` field. +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +message DeletePublisherRequest { + // Where Publisher parents the Book resource. + string name = 1 [ + (google.api.resource_reference).type = "library.googleapis.com/Publisher"]; + + // If set to true, any books from this publisher will also be deleted. + // (Otherwise, the request will only work if the publisher has no books.) + bool force = 2; +} +``` + +## Disabling + +If you need to violate this rule, use a leading comment above the message (if +the `name` field is missing) or above the field (if it is the wrong type). +Remember to also include an [aip.dev/not-precedent][] comment explaining why. + +```proto +// (-- api-linter: core::0135::force-field=disabled +// aip.dev/not-precedent: We need to do this because reasons. --) +message DeletePublisherRequest { + // Where Publisher parents the Book resource. + string name = 1 [ + (google.api.resource_reference).type = "library.googleapis.com/Publisher"]; +} +``` + +If you need to violate this rule for an entire file, place the comment at the +top of the file. + +[aip-135]: https://aip.dev/135#cascading-delete +[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/rules/aip0135/aip0135.go b/rules/aip0135/aip0135.go index 4b501474f..61d1a0694 100644 --- a/rules/aip0135/aip0135.go +++ b/rules/aip0135/aip0135.go @@ -27,6 +27,7 @@ import ( func AddRules(r lint.RuleRegistry) error { return r.Register( 135, + forceField, httpBody, httpMethod, httpNameField, diff --git a/rules/aip0135/force_field.go b/rules/aip0135/force_field.go new file mode 100644 index 000000000..1c11fac99 --- /dev/null +++ b/rules/aip0135/force_field.go @@ -0,0 +1,51 @@ +// 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 aip0135 + +import ( + "github.com/googleapis/api-linter/lint" + "github.com/googleapis/api-linter/rules/internal/utils" + "github.com/jhump/protoreflect/desc" +) + +// Delete methods for resources that are parents should have a bool force field. +var forceField = &lint.MessageRule{ + Name: lint.NewRuleName(135, "force-field"), + OnlyIf: func(m *desc.MessageDescriptor) bool { + name := m.FindFieldByName("name") + ref := utils.GetResourceReference(name) + validRef := ref != nil && ref.GetType() != "" && utils.FindResource(ref.GetType(), m.GetFile()) != nil + + return isDeleteRequestMessage(m) && validRef + }, + LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { + force := m.FindFieldByName("force") + name := m.FindFieldByName("name") + ref := utils.GetResourceReference(name) + res := utils.FindResource(ref.GetType(), m.GetFile()) + + children := utils.FindResourceChildren(res, m.GetFile()) + if len(children) > 0 && force == nil { + return []lint.Problem{ + { + Message: "Delete requests for resources with children should have a `bool force` field", + Descriptor: m, + }, + } + } + + return nil + }, +} diff --git a/rules/aip0135/force_field_test.go b/rules/aip0135/force_field_test.go new file mode 100644 index 000000000..2c92a208f --- /dev/null +++ b/rules/aip0135/force_field_test.go @@ -0,0 +1,72 @@ +// 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 aip0135 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +func TestForceField(t *testing.T) { + tests := []struct { + name string + Reference string + BoolField string + problems testutils.Problems + }{ + {"ValidWithChildren", `.type = "library.googleapis.com/Publisher"`, "force", nil}, + {"ValidWithoutChildren", `.type = "library.googleapis.com/Book"`, "other", nil}, + {"SkipIncorrectChildTypeReference", `.child_type = "library.googleapis.com/Publisher"`, "other", nil}, + {"InvalidMissingForce", `.type = "library.googleapis.com/Publisher"`, "other", testutils.Problems{{Message: "bool force"}}}, + } + + for _, test := range tests { + 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 Publisher { + option (google.api.resource) = { + type: "library.googleapis.com/Publisher" + pattern: "publishers/{publisher}" + }; + + string name = 1; + } + + message DeleteResourceRequest { + string name = 1 [(google.api.resource_reference){{.Reference}}]; + + bool {{.BoolField}} = 2; + } + `, test) + msg := f.FindMessage("DeleteResourceRequest") + problems := forceField.Lint(f) + if diff := test.problems.SetDescriptor(msg).Diff(problems); diff != "" { + t.Errorf("Problems did not match: %v", diff) + } + }) + } +} diff --git a/rules/internal/utils/extension.go b/rules/internal/utils/extension.go index 16bdbfac3..788cd8a90 100644 --- a/rules/internal/utils/extension.go +++ b/rules/internal/utils/extension.go @@ -160,6 +160,9 @@ func GetResourceDefinitions(f *desc.FileDescriptor) []*apb.ResourceDescriptor { // GetResourceReference returns the google.api.resource_reference annotation. func GetResourceReference(f *desc.FieldDescriptor) *apb.ResourceReference { + if f == nil { + return nil + } opts := f.GetFieldOptions() if x := proto.GetExtension(opts, apb.E_ResourceReference); x != nil { return x.(*apb.ResourceReference) @@ -200,3 +203,33 @@ func SplitResourceTypeName(typ string) (service string, typeName string, ok bool return } + +// FindResourceChildren attempts to search for other resources defined in the +// package that are parented by the given resource. +func FindResourceChildren(parent *apb.ResourceDescriptor, file *desc.FileDescriptor) []*apb.ResourceDescriptor { + pats := parent.GetPattern() + if len(pats) == 0 { + return nil + } + // Use the first pattern in the resource because: + // 1. Patterns cannot be rearranged, so this is the true first pattern + // 2. The true first pattern is the one most likely to be used as a parent. + first := pats[0] + + var children []*apb.ResourceDescriptor + files := append(file.GetDependencies(), file) + for _, f := range files { + for _, m := range f.GetMessageTypes() { + if r := GetResource(m); r != nil && r.GetType() != parent.GetType() { + for _, p := range r.GetPattern() { + if strings.HasPrefix(p, first) { + children = append(children, r) + break + } + } + } + } + } + + return children +} diff --git a/rules/internal/utils/extension_test.go b/rules/internal/utils/extension_test.go index 80f5237ea..382a30367 100644 --- a/rules/internal/utils/extension_test.go +++ b/rules/internal/utils/extension_test.go @@ -20,6 +20,8 @@ import ( "bitbucket.org/creachadair/stringset" "github.com/google/go-cmp/cmp" "github.com/googleapis/api-linter/rules/internal/testutils" + apb "google.golang.org/genproto/googleapis/api/annotations" + "google.golang.org/protobuf/proto" ) func TestGetFieldBehavior(t *testing.T) { @@ -471,3 +473,91 @@ func TestGetOutputOrLROResponseMessage(t *testing.T) { }) } } + +func TestFindResourceChildren(t *testing.T) { + publisher := &apb.ResourceDescriptor{ + Type: "library.googleapis.com/Publisher", + Pattern: []string{ + "publishers/{publisher}", + }, + } + shelf := &apb.ResourceDescriptor{ + Type: "library.googleapis.com/Shelf", + Pattern: []string{ + "shelves/{shelf}", + }, + } + book := &apb.ResourceDescriptor{ + Type: "library.googleapis.com/Book", + Pattern: []string{ + "publishers/{publisher}/books/{book}", + }, + } + edition := &apb.ResourceDescriptor{ + Type: "library.googleapis.com/Edition", + Pattern: []string{ + "publishers/{publisher}/books/{book}/editions/{edition}", + }, + } + files := testutils.ParseProtoStrings(t, map[string]string{ + "book.proto": ` + syntax = "proto3"; + package test; + + 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 Edition { + option (google.api.resource) = { + type: "library.googleapis.com/Edition" + pattern: "publishers/{publisher}/books/{book}/editions/{edition}" + }; + + string name = 1; + } + `, + "shelf.proto": ` + syntax = "proto3"; + package test; + + import "book.proto"; + import "google/api/resource.proto"; + + message Shelf { + option (google.api.resource) = { + type: "library.googleapis.com/Shelf" + pattern: "shelves/{shelf}" + }; + + string name = 1; + + repeated Book books = 2; + } + `, + }) + + for _, tst := range []struct { + name string + parent *apb.ResourceDescriptor + want []*apb.ResourceDescriptor + }{ + {"has_child_same_file", book, []*apb.ResourceDescriptor{edition}}, + {"has_child_other_file", publisher, []*apb.ResourceDescriptor{book, edition}}, + {"no_children", shelf, nil}, + } { + t.Run(tst.name, func(t *testing.T) { + got := FindResourceChildren(tst.parent, files["shelf.proto"]) + if diff := cmp.Diff(tst.want, got, cmp.Comparer(proto.Equal)); diff != "" { + t.Errorf("got(-),want(+):\n%s", diff) + } + }) + } +}