fix: Fix handling of top-level resources in AIP-235 request-parent-fi…

…eld rule (#796)
googleapis · Mar 16, 2021 · 6be37c7 · 6be37c7
1 parent 1c94b29
commit 6be37c7
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 28 deletions.
diff --git a/rules/aip0235/request_parent_field.go b/rules/aip0235/request_parent_field.go
@@ -15,33 +15,30 @@
 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.
 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
@@ -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
@@ -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)
+}