Repository: googleapis/api-linter (go)

Issue:
Parent is required even for top-level resources
The following linters:

- https://linter.aip.dev/133/method-signature
- https://linter.aip.dev/133/http-uri-parent

expect Create methods to always have a `parent`.

Should the linters not warn on missing `parent` when the resource is a top level resource/collection item?

> A parent field must be included **unless the resource being listed is a top-level resource**. 
(https://aip.dev/133)

Pull request description:
[AIP-132/133] Permit no parent on top level fields.
Fixes #483.

This permits top level fields on List or Create requests
where the corresponding resource is annotated with
google.api.resource and has only one variable.

This implementation is intentionally cautious, preferring
to still complain unless it is sure that a top-level resource
is in use.

Patch:
diff --git a/rules/aip0132/request_parent_required.go b/rules/aip0132/request_parent_required.go
index 2a52c32..5256641 100644
--- a/rules/aip0132/request_parent_required.go
+++ b/rules/aip0132/request_parent_required.go
@@ -2,9 +2,12 @@ package aip0132
 
 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 List standard method should contain a parent field.
@@ -14,6 +17,26 @@ 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 := m.GetFile().FindMessage(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
+							}
+						}
+					}
+				}
+			}
+
+			// Nope, there should be a parent field and is not. Complain.
 			return []lint.Problem{{
 				Message:    fmt.Sprintf("Message %q has no `parent` field", m.GetName()),
 				Descriptor: m,
diff --git a/rules/aip0133/request_parent_required.go b/rules/aip0133/request_parent_required.go
index e1b18f7..03a9947 100644
--- a/rules/aip0133/request_parent_required.go
+++ b/rules/aip0133/request_parent_required.go
@@ -2,9 +2,12 @@ package aip0133
 
 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"
 )
 
 var requestParentRequired = &lint.MessageRule{
@@ -12,6 +15,24 @@ var requestParentRequired = &lint.MessageRule{
 	OnlyIf: isCreateRequestMessage,
 	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
 		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 request, then get the resource annotation from that,
+			// and then inspect the pattern there (oy!).
+			singular := getResourceMsgNameFromReq(m)
+			if field := m.FindFieldByName(strcase.SnakeCase(singular)); field != nil {
+				if resource := utils.GetResource(field.GetMessageType()); resource != nil {
+					for _, pattern := range resource.GetPattern() {
+						if strings.Count(pattern, "{") == 1 {
+							return nil
+						}
+					}
+				}
+			}
+
+			// Nope, this is not the unusual case, and a parent field is expected.
 			return []lint.Problem{{
 				Message:    fmt.Sprintf("Message %q has no `parent` field", m.GetName()),
 				Descriptor: m,
diff --git a/rules/internal/utils/extension.go b/rules/internal/utils/extension.go
index 020edac..ed18a74 100644
--- a/rules/internal/utils/extension.go
+++ b/rules/internal/utils/extension.go
@@ -61,6 +61,9 @@ func GetMethodSignatures(m *desc.MethodDescriptor) [][]string {
 
 // GetResource returns the google.api.resource annotation.
 func GetResource(m *desc.MessageDescriptor) *apb.ResourceDescriptor {
+	if m == nil {
+		return nil
+	}
 	opts := m.GetMessageOptions()
 	if x, err := proto.GetExtension(opts, apb.E_Resource); err == nil {
 		return x.(*apb.ResourceDescriptor)
diff --git a/rules/internal/utils/type_name.go b/rules/internal/utils/type_name.go
index fcc3b11..747206a 100644
--- a/rules/internal/utils/type_name.go
+++ b/rules/internal/utils/type_name.go
@@ -21,7 +21,7 @@ import (
 )
 
 // GetTypeName returns the name of the type of the field, as a string,
-// regardless
+// regardless of primitive, message, etc.
 //
 // TODO: Add support for map types.
 func GetTypeName(f *desc.FieldDescriptor) string {


Test patch:
diff --git a/rules/aip0132/request_parent_required_test.go b/rules/aip0132/request_parent_required_test.go
index c21cdc1..98b35b1 100644
--- a/rules/aip0132/request_parent_required_test.go
+++ b/rules/aip0132/request_parent_required_test.go
@@ -31,4 +31,26 @@ func TestRequestParentRequired(t *testing.T) {
 			}
 		})
 	}
+
+	// Test the "top-level exception", which is more involved
+	// than the other tests and therefore handled separately.
+	t.Run("ValidTopLevel", func(t *testing.T) {
+		f := testutils.ParseProto3String(t, `
+			import "google/api/resource.proto";
+			message ListBooksRequest {}
+			message ListBooksResponse {
+				repeated Book books = 1;
+			}
+			message Book {
+				option (google.api.resource) = {
+					pattern: "books/{book}"
+				};
+				string name = 1;
+			}
+		`)
+		problems := requestParentRequired.Lint(f)
+		if diff := (testutils.Problems{}).Diff(problems); diff != "" {
+			t.Errorf(diff)
+		}
+	})
 }
diff --git a/rules/aip0133/request_parent_required_test.go b/rules/aip0133/request_parent_required_test.go
index d638e8c..df31b45 100644
--- a/rules/aip0133/request_parent_required_test.go
+++ b/rules/aip0133/request_parent_required_test.go
@@ -35,4 +35,25 @@ func TestRequestParentFieldRequired(t *testing.T) {
 			}
 		})
 	}
+
+	// Test the "top-level exception", which is more involved
+	// than the other tests and therefore handled separately.
+	t.Run("ValidTopLevel", func(t *testing.T) {
+		f := testutils.ParseProto3String(t, `
+			import "google/api/resource.proto";
+			message CreateBookRequest {
+				Book book = 2;
+			}
+			message Book {
+				option (google.api.resource) = {
+					pattern: "books/{book}"
+				};
+				string name = 1;
+			}
+		`)
+		problems := requestParentRequired.Lint(f)
+		if diff := (testutils.Problems{}).Diff(problems); diff != "" {
+			t.Errorf(diff)
+		}
+	})
 }
diff --git a/rules/internal/utils/extension_test.go b/rules/internal/utils/extension_test.go
index fb1dc07..cea2390 100644
--- a/rules/internal/utils/extension_test.go
+++ b/rules/internal/utils/extension_test.go
@@ -146,7 +146,12 @@ func TestGetResource(t *testing.T) {
 	t.Run("Absent", func(t *testing.T) {
 		f := testutils.ParseProto3String(t, "message Book {}")
 		if got := GetResource(f.GetMessageTypes()[0]); got != nil {
-			t.Errorf(`Got "%v", expected nil`, got)
+			t.Errorf(`Got "%v", expected nil.`, got)
+		}
+	})
+	t.Run("Nil", func(t *testing.T) {
+		if got := GetResource(nil); got != nil {
+			t.Errorf(`Got "%v", expected nil.`, got)
 		}
 	})
 }