From 6be37c7745dbc4f2a369d9e2b970612ac033d02c Mon Sep 17 00:00:00 2001 From: Andrew Paseltiner Date: Tue, 16 Mar 2021 12:01:30 -0400 Subject: [PATCH] fix: Fix handling of top-level resources in AIP-235 request-parent-field rule (#796) --- rules/aip0235/request_parent_field.go | 19 ++++++-------- rules/aip0235/request_parent_field_test.go | 29 +++++++++------------- rules/internal/utils/string_pluralize.go | 5 ++++ 3 files changed, 25 insertions(+), 28 deletions(-) diff --git a/rules/aip0235/request_parent_field.go b/rules/aip0235/request_parent_field.go index 94a45e819..f437f9758 100644 --- a/rules/aip0235/request_parent_field.go +++ b/rules/aip0235/request_parent_field.go @@ -15,13 +15,11 @@ package aip0235 import ( - "fmt" "strings" "github.com/googleapis/api-linter/lint" "github.com/googleapis/api-linter/rules/internal/utils" "github.com/jhump/protoreflect/desc" - "github.com/stoewer/go-strcase" ) // The Batch Delete request message should have parent field. @@ -29,19 +27,18 @@ var requestParentField = &lint.MessageRule{ Name: lint.NewRuleName(235, "request-parent-field"), OnlyIf: func(m *desc.MessageDescriptor) bool { // Sanity check: If the resource has a pattern, and that pattern - // contains no variables, then a parent field is not expected. + // contains only one variable, then a parent field is not expected. // // In order to parse out the pattern, we get the resource message - // from the response, then get the resource annotation from that, + // from the request name, then get the resource annotation from that, // and then inspect the pattern there (oy!). plural := strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Request"), "BatchDelete") - if resp := utils.FindMessage(m.GetFile(), fmt.Sprintf("BatchDelete%sResponse", plural)); resp != nil { - if paged := resp.FindFieldByName(strcase.SnakeCase(plural)); paged != nil { - if resource := utils.GetResource(paged.GetMessageType()); resource != nil { - for _, pattern := range resource.GetPattern() { - if strings.Count(pattern, "{") == 0 { - return false - } + singular := utils.ToSingular(plural) + if msg := utils.FindMessage(m.GetFile(), singular); msg != nil { + if resource := utils.GetResource(msg); resource != nil { + for _, pattern := range resource.GetPattern() { + if strings.Count(pattern, "{") == 1 { + return false } } } diff --git a/rules/aip0235/request_parent_field_test.go b/rules/aip0235/request_parent_field_test.go index 74d1bd238..c52078ab3 100644 --- a/rules/aip0235/request_parent_field_test.go +++ b/rules/aip0235/request_parent_field_test.go @@ -30,41 +30,36 @@ func TestParentField(t *testing.T) { Pattern string problems testutils.Problems }{ - {"Valid", "", "BatchDeleteBooks", "string parent = 1;", "publishers/{p}/books", nil}, - {"Missing", "", "BatchDeleteBooks", "", "publishers/{p}/books", testutils.Problems{{Message: "no `parent`"}}}, - {"InvalidType", "", "BatchDeleteBooks", "int32 parent = 1;", "publishers/{p}/books", testutils.Problems{{Suggestion: "string"}}}, - {"IrrelevantRPCName", "", "EnumerateBooks", "", "publishers/{p}/books", nil}, - {"IrrelevantNoParent", "", "BatchDeleteBooks", "", "books", nil}, + {"Valid", "", "BatchDeleteBooks", "string parent = 1;", "publishers/{p}/books/{b}", nil}, + {"Missing", "", "BatchDeleteBooks", "", "publishers/{p}/books/{b}", testutils.Problems{{Message: "no `parent`"}}}, + {"InvalidType", "", "BatchDeleteBooks", "int32 parent = 1;", "publishers/{p}/books/{b}", testutils.Problems{{Suggestion: "string"}}}, + {"IrrelevantRPCName", "", "EnumerateBooks", "", "publishers/{p}/books/{b}", nil}, + {"IrrelevantNoParent", "", "BatchDeleteBooks", "", "books/{b}", nil}, - {"PackageValid", "package foo;", "BatchDeleteBooks", "string parent = 1;", "publishers/{p}/books", nil}, - {"PackageMissing", "package foo;", "BatchDeleteBooks", "", "publishers/{p}/books", testutils.Problems{{Message: "no `parent`"}}}, - {"PackageInvalidType", "package foo;", "BatchDeleteBooks", "int32 parent = 1;", "publishers/{p}/books", testutils.Problems{{Suggestion: "string"}}}, - {"PackageIrrelevantRPCName", "package foo;", "EnumerateBooks", "", "publishers/{p}/books", nil}, - {"PackageIrrelevantNoParent", "package foo;", "BatchDeleteBooks", "", "books", nil}, + {"PackageValid", "package foo;", "BatchDeleteBooks", "string parent = 1;", "publishers/{p}/books/{b}", nil}, + {"PackageMissing", "package foo;", "BatchDeleteBooks", "", "publishers/{p}/books/{b}", testutils.Problems{{Message: "no `parent`"}}}, + {"PackageInvalidType", "package foo;", "BatchDeleteBooks", "int32 parent = 1;", "publishers/{p}/books/{b}", testutils.Problems{{Suggestion: "string"}}}, + {"PackageIrrelevantRPCName", "package foo;", "EnumerateBooks", "", "publishers/{p}/books/{b}", nil}, + {"PackageIrrelevantNoParent", "package foo;", "BatchDeleteBooks", "", "books/{b}", nil}, } { t.Run(test.name, func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` {{.Package}} import "google/api/resource.proto"; + import "google/protobuf/empty.proto"; service Library { - rpc {{.RPC}}({{.RPC}}Request) returns ({{.RPC}}Response); + rpc {{.RPC}}({{.RPC}}Request) returns (google.protobuf.Empty); } message {{.RPC}}Request { {{.Field}} - repeated string names = 2; - } - - message {{.RPC}}Response { - repeated Book books = 1; } message Book { option (google.api.resource) = { pattern: "{{.Pattern}}"; }; - string name = 1; } `, test) var d desc.Descriptor = f.GetMessageTypes()[0] diff --git a/rules/internal/utils/string_pluralize.go b/rules/internal/utils/string_pluralize.go index d447e5b37..b416086f2 100644 --- a/rules/internal/utils/string_pluralize.go +++ b/rules/internal/utils/string_pluralize.go @@ -27,3 +27,8 @@ func ToPlural(s string) string { return pluralizeClient.Plural(pluralizeClient.Singular(s)) } + +// ToSingular converts a string to its singular form. +func ToSingular(s string) string { + return pluralizeClient.Singular(s) +}