Repository: googleapis/api-linter
Pull request #636 (2020-09-26 12:59:56)

Add request-parent-field rule for AIP 235
Part of #612.


Patch:
diff --git a/docs/rules/0235/request-parent-field.md b/docs/rules/0235/request-parent-field.md
new file mode 100644
index 0000000..4410835
--- /dev/null
+++ b/docs/rules/0235/request-parent-field.md
@@ -0,0 +1,71 @@
+---
+rule:
+  aip: 235
+  name: [core, '0235', request-parent-field]
+  summary: Batch Delete RPCs must have a `parent` field in the request.
+permalink: /235/request-parent-field
+redirect_from:
+  - /0235/request-parent-field
+---
+
+# Batch Delete methods: Parent field
+
+This rule enforces that all `BatchDelete` methods have a `string parent` field in
+the request message, as mandated in [AIP-235][].
+
+## Details
+
+This rule looks at any message matching `BatchDelete*Request` and complains if
+either the `parent` field is missing, or if it has any type other than
+`string`.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+message BatchDeleteBooksRequest {
+  string publisher = 1;  // Field name should be `parent`.
+  repeated string names = 2;
+}
+```
+
+```proto
+// Incorrect.
+message BatchDeleteBooksRequest {
+  bytes parent = 1;  // Field type should be `string`.
+  repeated string names = 2;
+}
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+message BatchDeleteBooksRequest {
+  string parent = 1;
+  repeated string names = 2;
+}
+```
+
+## Disabling
+
+If you need to violate this rule, use a leading comment above the message (if
+the `parent` 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::0235::request-parent-field=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+message BatchDeleteBooksRequest {
+  string publisher = 1;  // Field name should be `parent`.
+  repeated string names = 2;
+}
+```
+
+If you need to violate this rule for an entire file, place the comment at the
+top of the file.
+
+[aip-235]: https://aip.dev/235
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
diff --git a/rules/aip0235/aip0235.go b/rules/aip0235/aip0235.go
index 9758007..4aa67b5 100644
--- a/rules/aip0235/aip0235.go
+++ b/rules/aip0235/aip0235.go
@@ -32,13 +32,20 @@ func AddRules(r lint.RuleRegistry) error {
 		httpUriSuffix,
 		pluralMethodName,
 		requestMessageName,
+		requestParentField,
 	)
 }
 
 var batchDeleteMethodRegexp = regexp.MustCompile("^BatchDelete(?:[A-Z]|$)")
+var batchDeleteReqMessageRegexp = regexp.MustCompile("^BatchDelete[A-Za-z0-9]*Request$")
 var batchDeleteURIRegexp = regexp.MustCompile(`:batchDelete$`)
 
 // Returns true if this is a AIP-235 Batch Delete method, false otherwise.
 func isBatchDeleteMethod(m *desc.MethodDescriptor) bool {
 	return batchDeleteMethodRegexp.MatchString(m.GetName())
 }
+
+// Returns true if this is an AIP-235 Batch Delete request message, false otherwise.
+func isBatchDeleteRequestMessage(m *desc.MessageDescriptor) bool {
+	return batchDeleteReqMessageRegexp.MatchString(m.GetName())
+}
diff --git a/rules/aip0235/request_parent_field.go b/rules/aip0235/request_parent_field.go
new file mode 100644
index 0000000..e7ee3b1
--- /dev/null
+++ b/rules/aip0235/request_parent_field.go
@@ -0,0 +1,76 @@
+// Copyright 2020 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 aip0235
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/locations"
+	"github.com/googleapis/api-linter/rules/internal/utils"
+	"github.com/jhump/protoreflect/desc"
+	"github.com/jhump/protoreflect/desc/builder"
+	"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.
+		//
+		// 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"), "BatchDelete")
+		if resp := m.GetFile().FindMessage(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
+						}
+					}
+				}
+			}
+		}
+
+		return isBatchDeleteRequestMessage(m)
+	},
+	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
+		// Rule check: Establish that a `parent` field is present.
+		parentField := m.FindFieldByName("parent")
+		if parentField == nil {
+			return []lint.Problem{{
+				Message:    fmt.Sprintf("Message %q has no `parent` field.", m.GetName()),
+				Descriptor: m,
+			}}
+		}
+
+		// Rule check: Establish that the parent field is a string.
+		if parentField.GetType() != builder.FieldTypeString().GetType() {
+			return []lint.Problem{{
+				Message:    "`parent` field on Batch Delete request message must be a string.",
+				Descriptor: parentField,
+				Location:   locations.FieldType(parentField),
+				Suggestion: "string",
+			}}
+		}
+
+		return nil
+	},
+}


Test patch:
diff --git a/rules/aip0235/request_parent_field_test.go b/rules/aip0235/request_parent_field_test.go
new file mode 100644
index 0000000..de1b5b2
--- /dev/null
+++ b/rules/aip0235/request_parent_field_test.go
@@ -0,0 +1,71 @@
+// Copyright 2020 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 aip0235
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+	"github.com/jhump/protoreflect/desc"
+)
+
+func TestParentField(t *testing.T) {
+	for _, test := range []struct {
+		name     string
+		RPC      string
+		Field    string
+		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},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				import "google/api/resource.proto";
+
+				service Library {
+					rpc {{.RPC}}({{.RPC}}Request) returns ({{.RPC}}Response);
+				}
+
+				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]
+			if test.name == "InvalidType" {
+				d = f.GetMessageTypes()[0].GetFields()[0]
+			}
+			if diff := test.problems.SetDescriptor(d).Diff(requestParentField.Lint(f)); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}