fix(rules): fix HasParent check and utilify it (#1468)

rules/aip0132/request_parent_required.go

@@ -17,21 +17,14 @@ var requestParentRequired = &lint.MessageRule{
 	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
 		// Rule check: Establish that a `parent` field is present.
 		if m.FindFieldByName("parent") == nil {
-			// Sanity check: If the resource has a pattern, and that pattern
-			// 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,
 			// and then inspect the pattern there (oy!).
 			plural := strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Request"), "List")
 			if resp := utils.FindMessage(m.GetFile(), fmt.Sprintf("List%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, "{") == 1 {
-								return nil
-							}
-						}
+				if resField := resp.FindFieldByName(strcase.SnakeCase(plural)); resField != nil {
+					if !utils.HasParent(utils.GetResource(resField.GetMessageType())) {
+						return nil
 					}
 				}
 			}

rules/aip0133/http_uri_parent.go

@@ -40,7 +40,7 @@ var httpURIParent = &lint.MethodRule{
 			}
 		}
 
-		return utils.IsCreateMethod(m) && !hasNoParent(res)
+		return utils.IsCreateMethod(m) && utils.HasParent(utils.GetResource(res))
 	},
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		if problems := utils.LintHTTPURIHasParentVariable(m); problems != nil {

rules/aip0133/method_signature.go

@@ -34,7 +34,7 @@ var methodSignature = &lint.MethodRule{
 
 		// Determine what signature we want.
 		want := []string{}
-		if !hasNoParent(utils.GetResponseType(m)) {
+		if utils.HasParent(utils.GetResource(utils.GetResponseType(m))) {
 			want = append(want, "parent")
 		}
 		for _, f := range m.GetInputType().GetFields() {

rules/aip0133/request_parent_required.go

@@ -2,7 +2,6 @@ package aip0133
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/googleapis/api-linter/lint"
 	"github.com/googleapis/api-linter/rules/internal/utils"
@@ -23,7 +22,7 @@ var requestParentRequired = &lint.MessageRule{
 			// and then inspect the pattern there (oy!).
 			singular := getResourceMsgNameFromReq(m)
 			if field := m.FindFieldByName(strcase.SnakeCase(singular)); field != nil {
-				if hasNoParent(field.GetMessageType()) {
+				if !utils.HasParent(utils.GetResource(field.GetMessageType())) {
 					return nil
 				}
 			}
@@ -38,14 +37,3 @@ var requestParentRequired = &lint.MessageRule{
 		return nil
 	},
 }
-
-func hasNoParent(m *desc.MessageDescriptor) bool {
-	if resource := utils.GetResource(m); resource != nil {
-		for _, pattern := range resource.GetPattern() {
-			if strings.Count(pattern, "{") == 1 {
-				return true
-			}
-		}
-	}
-	return false
-}

rules/aip0231/request_parent_field.go

@@ -28,21 +28,14 @@ import (
 var requestParentField = &lint.MessageRule{
 	Name: lint.NewRuleName(231, "request-parent-field"),
 	OnlyIf: func(m *desc.MessageDescriptor) bool {
-		// Sanity check: If the resource has a pattern, and that pattern
-		// 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,
 		// and then inspect the pattern there (oy!).
 		plural := strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Request"), "BatchGet")
 		if resp := utils.FindMessage(m.GetFile(), fmt.Sprintf("BatchGet%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, "{") == 1 {
-							return false
-						}
-					}
+			if resField := resp.FindFieldByName(strcase.SnakeCase(plural)); resField != nil {
+				if !utils.HasParent(utils.GetResource(resField.GetMessageType())) {
+					return false
 				}
 			}
 		}

rules/aip0233/request_parent_field.go

@@ -28,21 +28,14 @@ import (
 var requestParentField = &lint.MessageRule{
 	Name: lint.NewRuleName(233, "request-parent-field"),
 	OnlyIf: func(m *desc.MessageDescriptor) bool {
-		// Sanity check: If the resource has a pattern, and that pattern
-		// 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,
 		// and then inspect the pattern there (oy!).
 		plural := strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Request"), "BatchCreate")
 		if resp := utils.FindMessage(m.GetFile(), fmt.Sprintf("BatchCreate%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, "{") == 1 {
-							return false
-						}
-					}
+			if resField := resp.FindFieldByName(strcase.SnakeCase(plural)); resField != nil {
+				if !utils.HasParent(utils.GetResource(resField.GetMessageType())) {
+					return false
 				}
 			}
 		}

rules/aip0234/request_parent_field.go

@@ -28,21 +28,14 @@ import (
 var requestParentField = &lint.MessageRule{
 	Name: lint.NewRuleName(234, "request-parent-field"),
 	OnlyIf: func(m *desc.MessageDescriptor) bool {
-		// Sanity check: If the resource has a pattern, and that pattern
-		// 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,
 		// and then inspect the pattern there (oy!).
 		plural := strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Request"), "BatchUpdate")
 		if resp := utils.FindMessage(m.GetFile(), fmt.Sprintf("BatchUpdate%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, "{") == 1 {
-							return false
-						}
-					}
+			if resField := resp.FindFieldByName(strcase.SnakeCase(plural)); resField != nil {
+				if !utils.HasParent(utils.GetResource(resField.GetMessageType())) {
+					return false
 				}
 			}
 		}

rules/aip0235/request_parent_field.go

@@ -26,21 +26,14 @@ import (
 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 only one variable, then a parent field is not expected.
-		//
 		// In order to parse out the pattern, we get the resource message
 		// 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")
 		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
-					}
-				}
+			if !utils.HasParent(utils.GetResource(msg)) {
+				return false
 			}
 		}
 

rules/internal/utils/resource.go

@@ -90,3 +90,21 @@ func IsRevisionRelationship(parent, revision *apb.ResourceDescriptor) bool {
 	rType = strings.TrimSuffix(rType, "Revision")
 	return pType == rType
 }
+
+// HasParent determines if the given resource has a parent resource or not
+// based on the pattern(s) it defines having multiple resource ID segments
+// or not. Incomplete or nil input returns false.
+func HasParent(resource *apb.ResourceDescriptor) bool {
+	if resource == nil || len(resource.GetPattern()) == 0 {
+		return false
+	}
+
+	for _, pattern := range resource.GetPattern() {
+		// multiple ID variable segments indicates presence of parent
+		if strings.Count(pattern, "{") > 1 {
+			return true
+		}
+	}
+
+	return false
+}

rules/internal/utils/resource_test.go

@@ -181,3 +181,42 @@ func TestIsRevisionRelationship(t *testing.T) {
 		})
 	}
 }
+
+func TestHasParent(t *testing.T) {
+	for _, test := range []struct {
+		name    string
+		pattern string
+		want    bool
+	}{
+		{
+			name:    "child resource",
+			pattern: "foos/{foo}/bars/{bar}",
+			want:    true,
+		},
+		{
+			name:    "top level resource",
+			pattern: "foos/{foo}",
+			want:    false,
+		},
+		{
+			name:    "top level singleton",
+			pattern: "foo",
+			want:    false,
+		},
+		{
+			name:    "empty",
+			pattern: "",
+			want:    false,
+		},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			var in *apb.ResourceDescriptor
+			if test.pattern != "" {
+				in = &apb.ResourceDescriptor{Pattern: []string{test.pattern}}
+			}
+			if got := HasParent(in); got != test.want {
+				t.Errorf("HasParent(%s): got %v, want %v", test.pattern, got, test.want)
+			}
+		})
+	}
+}