Repository: googleapis/api-linter
Pull request #638 (2020-09-29 21:00:36)

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


Patch:
diff --git a/docs/rules/0231/request-names-field.md b/docs/rules/0231/request-names-field.md
index 54912a3..50652da 100644
--- a/docs/rules/0231/request-names-field.md
+++ b/docs/rules/0231/request-names-field.md
@@ -38,7 +38,7 @@ message BatchGetBooksRequest {
 // Incorrect.
 message BatchGetBooksRequest {
   string parent = 1;
-  string books = 2;  // Field should be repeated.
+  string names = 2;  // Field should be repeated.
 }
 ```
 
diff --git a/docs/rules/0235/request-names-field.md b/docs/rules/0235/request-names-field.md
new file mode 100644
index 0000000..c98f02a
--- /dev/null
+++ b/docs/rules/0235/request-names-field.md
@@ -0,0 +1,74 @@
+---
+rule:
+  aip: 235
+  name: [core, '0235', request-names-field]
+  summary: Batch Delete RPCs should have a `names` field in the request.
+permalink: /235/request-names-field
+redirect_from:
+  - /0235/request-names-field
+---
+
+# Batch Delete methods: Names field
+
+This rule enforces that all `BatchDelete` methods have a `repeated string names`
+field in the request message, as mandated in [AIP-235][].
+
+## Details
+
+This rule looks at any message matching `BatchDelete*Request` and complains if
+the `names` field is missing, if it has any type other than `string`, or
+if it is not `repeated`.
+
+Alternatively, if there is a `repeated DeleteBookRequest requests` field, this is
+accepted in its place.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+message BatchDeleteBooksRequest {
+  string parent = 1;
+  repeated string books = 2;  // Field name should be `names`.
+}
+```
+
+```proto
+// Incorrect.
+message BatchDeleteBooksRequest {
+  string parent = 1;
+  string names = 2;  // Field should be repeated.
+}
+```
+
+**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 `names` 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-names-field=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+message BatchDeleteBooksRequest {
+  string parent = 1;
+  repeated string books = 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/aip0231/request_names_field.go b/rules/aip0231/request_names_field.go
index d071131..1b23939 100644
--- a/rules/aip0231/request_names_field.go
+++ b/rules/aip0231/request_names_field.go
@@ -34,7 +34,7 @@ var namesField = &lint.MessageRule{
 		names := m.FindFieldByName("names")
 		getReqMsg := m.FindFieldByName("requests")
 
-		// Rule check: Ensure that the names field is existed.
+		// Rule check: Ensure that the names field is present.
 		if names == nil && getReqMsg == nil {
 			problems = append(problems, lint.Problem{
 				Message:    fmt.Sprintf(`Message %q has no "names" field`, m.GetName()),
@@ -42,7 +42,7 @@ var namesField = &lint.MessageRule{
 			})
 		}
 
-		// Rule check: Ensure that only the suggested names field is existed.
+		// Rule check: Ensure that only the suggested names field is present.
 		if names != nil && getReqMsg != nil {
 			problems = append(problems, lint.Problem{
 				Message:    fmt.Sprintf(`Message %q should delete "requests" field, only keep the "names" field`, m.GetName()),
@@ -80,7 +80,7 @@ var namesField = &lint.MessageRule{
 
 		// Rule check: Ensure that the standard get request message field is the
 		// correct type. Note: Use m.GetName()[8:len(m.GetName())-7]) to retrieve
-		// the resource name from the the batch get request, for example:
+		// the resource name from the batch get request, for example:
 		// "BatchGetBooksRequest" -> "Books"
 		rightTypeName := fmt.Sprintf("Get%sRequest", pluralize.NewClient().Singular(m.GetName()[8:len(m.GetName())-7]))
 		if getReqMsg != nil && (getReqMsg.GetMessageType() == nil || getReqMsg.GetMessageType().GetName() != rightTypeName) {
diff --git a/rules/aip0235/aip0235.go b/rules/aip0235/aip0235.go
index cd0e0da..0d268a0 100644
--- a/rules/aip0235/aip0235.go
+++ b/rules/aip0235/aip0235.go
@@ -33,6 +33,7 @@ func AddRules(r lint.RuleRegistry) error {
 		pluralMethodName,
 		requestMessageName,
 		requestNamesBehavior,
+		requestNamesField,
 		requestNamesReference,
 		requestParentField,
 	)
diff --git a/rules/aip0235/request_names_field.go b/rules/aip0235/request_names_field.go
new file mode 100644
index 0000000..e0ab29c
--- /dev/null
+++ b/rules/aip0235/request_names_field.go
@@ -0,0 +1,94 @@
+// 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"
+
+	"github.com/gertd/go-pluralize"
+	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/locations"
+	"github.com/jhump/protoreflect/desc"
+	"github.com/jhump/protoreflect/desc/builder"
+)
+
+// The Batch Delete standard method should have repeated name field or repeated
+// standard delete request message field, but the latter one is not suggested.
+var requestNamesField = &lint.MessageRule{
+	Name:   lint.NewRuleName(235, "request-names-field"),
+	OnlyIf: isBatchDeleteRequestMessage,
+	LintMessage: func(m *desc.MessageDescriptor) (problems []lint.Problem) {
+		// Rule check: Establish that a name field is present.
+		names := m.FindFieldByName("names")
+		deleteReqMsg := m.FindFieldByName("requests")
+
+		// Rule check: Ensure that the names field is present.
+		if names == nil && deleteReqMsg == nil {
+			problems = append(problems, lint.Problem{
+				Message:    fmt.Sprintf(`Message %q has no "names" field`, m.GetName()),
+				Descriptor: m,
+			})
+		}
+
+		// Rule check: Ensure that only the suggested names field is present.
+		if names != nil && deleteReqMsg != nil {
+			problems = append(problems, lint.Problem{
+				Message:    fmt.Sprintf(`Message %q should delete "requests" field, only keep the "names" field`, m.GetName()),
+				Descriptor: deleteReqMsg,
+			})
+		}
+
+		// Rule check: Ensure that the names field is repeated.
+		if names != nil && !names.IsRepeated() {
+			problems = append(problems, lint.Problem{
+				Message:    `The "names" field should be repeated`,
+				Suggestion: "repeated string",
+				Descriptor: names,
+				Location:   locations.FieldType(names),
+			})
+		}
+
+		// Rule check: Ensure that the names field is the correct type.
+		if names != nil && names.GetType() != builder.FieldTypeString().GetType() {
+			problems = append(problems, lint.Problem{
+				Message:    `"names" field on Batch Delete Request should be a "string" type`,
+				Suggestion: "string",
+				Descriptor: names,
+				Location:   locations.FieldType(names),
+			})
+		}
+
+		// Rule check: Ensure that the standard delete request message field is repeated.
+		if deleteReqMsg != nil && !deleteReqMsg.IsRepeated() {
+			problems = append(problems, lint.Problem{
+				Message:    `The "requests" field should be repeated`,
+				Descriptor: deleteReqMsg,
+			})
+		}
+
+		// Rule check: Ensure that the standard delete request message field is the
+		// correct type. Note: Use m.GetName()[11:len(m.GetName())-7]) to retrieve
+		// the resource name from the batch delete request, for example:
+		// "BatchDeleteBooksRequest" -> "Books"
+		rightTypeName := fmt.Sprintf("Delete%sRequest", pluralize.NewClient().Singular(m.GetName()[11:len(m.GetName())-7]))
+		if deleteReqMsg != nil && (deleteReqMsg.GetMessageType() == nil || deleteReqMsg.GetMessageType().GetName() != rightTypeName) {
+			problems = append(problems, lint.Problem{
+				Message:    fmt.Sprintf(`The "requests" field on Batch Delete Request should be a %q type`, rightTypeName),
+				Descriptor: deleteReqMsg,
+			})
+		}
+		return
+	},
+}


Test patch:
diff --git a/rules/aip0231/request_names_field_test.go b/rules/aip0231/request_names_field_test.go
index 41d039f..883fc5c 100644
--- a/rules/aip0231/request_names_field_test.go
+++ b/rules/aip0231/request_names_field_test.go
@@ -111,7 +111,7 @@ func TestNamesField(t *testing.T) {
 			},
 		},
 		{
-			testName: "Invalid-NamesFieldWrongType",
+			testName: "Invalid-GetReqFieldWrongType",
 			src: `
 				message BatchGetBooksRequest {
 					repeated string requests = 1;
@@ -122,6 +122,11 @@ func TestNamesField(t *testing.T) {
 				return m.FindFieldByName("requests")
 			},
 		},
+		{
+			testName: "Irrelevant-UnmatchedMessageName",
+			src:      `message GetBooksRequest {}`,
+			problems: testutils.Problems{},
+		},
 	}
 
 	// Run each test individually.
diff --git a/rules/aip0235/request_names_field_test.go b/rules/aip0235/request_names_field_test.go
new file mode 100644
index 0000000..64def32
--- /dev/null
+++ b/rules/aip0235/request_names_field_test.go
@@ -0,0 +1,151 @@
+// 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 TestNamesField(t *testing.T) {
+	// Set up the testing permutations.
+	tests := []struct {
+		testName    string
+		src         string
+		problems    testutils.Problems
+		problemDesc func(m *desc.MessageDescriptor) desc.Descriptor
+	}{
+		{
+			testName: "Valid-Names",
+			src: `
+				message BatchDeleteBooksRequest {
+					repeated string names = 1;
+				}
+			`,
+			problems: testutils.Problems{},
+		},
+		{
+			testName: "Valid-StandardDeleteReq",
+			src: `
+				message BatchDeleteBooksRequest {
+					repeated DeleteBookRequest requests = 1;
+				}
+
+				message DeleteBookRequest {}
+			`,
+			problems: testutils.Problems{},
+		},
+		{
+			testName: "Invalid-MissingNamesField",
+			src: `
+				message BatchDeleteBooksRequest {
+					string parent = 1;
+				}
+			`,
+			problems: testutils.Problems{{Message: `Message "BatchDeleteBooksRequest" has no "names" field`}},
+		},
+		{
+			testName: "Invalid-KeepingNamesFieldOnly",
+			src: `
+				message BatchDeleteBooksRequest {
+					repeated string names = 1;
+					repeated DeleteBookRequest requests = 2;
+				}
+
+				message DeleteBookRequest {}
+			`,
+			problems: testutils.Problems{{Message: `Message "BatchDeleteBooksRequest" should delete "requests" field, only keep the "names" field`}},
+			problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor {
+				return m.FindFieldByName("requests")
+			},
+		},
+		{
+			testName: "Invalid-NamesFieldIsNotRepeated",
+			src: `
+				message BatchDeleteBooksRequest {
+					string names = 1;
+				}`,
+			problems: testutils.Problems{{Suggestion: "repeated string"}},
+			problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor {
+				return m.FindFieldByName("names")
+			},
+		},
+		{
+			testName: "Invalid-NamesFieldWrongType",
+			src: `
+				message BatchDeleteBooksRequest {
+					repeated int32 names = 1;
+				}
+			`,
+			problems: testutils.Problems{{Suggestion: "string"}},
+			problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor {
+				return m.FindFieldByName("names")
+			},
+		},
+		{
+			testName: "Invalid-DeleteReqFieldIsNotRepeated",
+			src: `
+				message BatchDeleteBooksRequest {
+					DeleteBookRequest requests = 1;
+				}
+
+				message DeleteBookRequest {}
+			`,
+			problems: testutils.Problems{{Message: `The "requests" field should be repeated`}},
+			problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor {
+				return m.FindFieldByName("requests")
+			},
+		},
+		{
+			testName: "Invalid-DeleteReqFieldWrongType",
+			src: `
+				message BatchDeleteBooksRequest {
+					repeated string requests = 1;
+				}
+			`,
+			problems: testutils.Problems{{Message: `The "requests" field on Batch Delete Request should be a "DeleteBookRequest" type`}},
+			problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor {
+				return m.FindFieldByName("requests")
+			},
+		},
+		{
+			testName: "Irrelevant-UnmatchedMessageName",
+			src:      `message DeleteBooksRequest {}`,
+			problems: testutils.Problems{},
+		},
+	}
+
+	// Run each test individually.
+	for _, test := range tests {
+		t.Run(test.testName, func(t *testing.T) {
+			file := testutils.ParseProto3String(t, test.src)
+
+			m := file.GetMessageTypes()[0]
+
+			// Determine the descriptor that a failing test will attach to.
+			var problemDesc desc.Descriptor = m
+			if test.problemDesc != nil {
+				problemDesc = test.problemDesc(m)
+			}
+
+			problems := requestNamesField.Lint(file)
+			if diff := test.problems.SetDescriptor(problemDesc).Diff(problems); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}