Repository: googleapis/api-linter
Pull request #649 (2020-10-07 15:05:02)

Add rule for AIP-235 soft-delete response resource field
Part of #612.


Patch:
diff --git a/docs/rules/0235/response-resource-field.md b/docs/rules/0235/response-resource-field.md
new file mode 100644
index 0000000..7386b4e
--- /dev/null
+++ b/docs/rules/0235/response-resource-field.md
@@ -0,0 +1,68 @@
+---
+rule:
+  aip: 235
+  name: [core, '0235', response-resource-field]
+  summary:
+    Batch Delete RPCs must have a repeated field for the resource in the response.
+permalink: /235/response-resource-field
+redirect_from:
+  - /0235/response-resource-field
+---
+
+# Batch Delete methods: Resource field
+
+This rule enforces that all `BatchDelete` methods have a repeated field in the
+soft-delete response message for the resource itself, as mandated in
+[AIP-235][].
+
+## Details
+
+This rule looks at any message matching `BatchDelete*Response` and complains if
+there is no repeated field of the resource's type with the expected field name.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+message BatchDeleteBooksResponse {
+  // `repeated Book books` is missing.
+}
+```
+
+```proto
+// Incorrect.
+message BatchDeleteBooksResponse {
+  Book books = 1;  // Field should be repeated.
+}
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+message BatchDeleteBooksResponse {
+  repeated Book books = 1;
+}
+```
+
+## Disabling
+
+If you need to violate this rule, use a leading comment above the message (if
+the resource field is missing) or above the field (if it is improperly named).
+Remember to also include an [aip.dev/not-precedent][] comment explaining why.
+
+```proto
+// (-- api-linter: core::0235::response-resource-field=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+message BatchDeleteBooksResponse {
+  // `repeated Book books` is missing.
+}
+```
+
+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 de0b64b..84a0b6f 100644
--- a/rules/aip0235/aip0235.go
+++ b/rules/aip0235/aip0235.go
@@ -39,11 +39,13 @@ func AddRules(r lint.RuleRegistry) error {
 		requestParentReference,
 		requestRequestsBehavior,
 		requestUnknownFields,
+		responseResourceField,
 	)
 }
 
 var batchDeleteMethodRegexp = regexp.MustCompile("^BatchDelete(?:[A-Z]|$)")
 var batchDeleteReqMessageRegexp = regexp.MustCompile("^BatchDelete[A-Za-z0-9]*Request$")
+var batchDeleteResponseMessageRegexp = regexp.MustCompile("^BatchDelete[A-Za-z0-9]*Response$")
 var batchDeleteURIRegexp = regexp.MustCompile(`:batchDelete$`)
 
 // Returns true if this is a AIP-235 Batch Delete method, false otherwise.
@@ -55,3 +57,8 @@ func isBatchDeleteMethod(m *desc.MethodDescriptor) bool {
 func isBatchDeleteRequestMessage(m *desc.MessageDescriptor) bool {
 	return batchDeleteReqMessageRegexp.MatchString(m.GetName())
 }
+
+// Returns true if this is an AIP-235 Batch Delete response message, false otherwise.
+func isBatchDeleteResponseMessage(m *desc.MessageDescriptor) bool {
+	return batchDeleteResponseMessageRegexp.MatchString(m.GetName())
+}
diff --git a/rules/aip0235/response_resource_field.go b/rules/aip0235/response_resource_field.go
new file mode 100644
index 0000000..452b9c7
--- /dev/null
+++ b/rules/aip0235/response_resource_field.go
@@ -0,0 +1,55 @@
+// 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/gertd/go-pluralize"
+	"github.com/googleapis/api-linter/lint"
+	"github.com/jhump/protoreflect/desc"
+)
+
+// The Batch Delete response message should have resource field.
+var responseResourceField = &lint.MessageRule{
+	Name:   lint.NewRuleName(235, "response-resource-field"),
+	OnlyIf: isBatchDeleteResponseMessage,
+	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
+		// the singular form the resource name, the first letter is Capitalized.
+		// Note: Retrieve the resource name from the the batch update response,
+		// for example: "BatchDeleteBooksResponse" -> "Books"
+		resourceMsgName := pluralize.NewClient().Singular(strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Response"), "BatchDelete"))
+
+		for _, fieldDesc := range m.GetFields() {
+			if msgDesc := fieldDesc.GetMessageType(); msgDesc != nil && msgDesc.GetName() == resourceMsgName {
+				if !fieldDesc.IsRepeated() {
+					return []lint.Problem{{
+						Message:    fmt.Sprintf("The %q type field on Batch Delete Response message should be repeated", msgDesc.GetName()),
+						Descriptor: fieldDesc,
+					}}
+				}
+
+				return nil
+			}
+		}
+
+		// Rule check: Establish that a resource field must be included.
+		return []lint.Problem{{
+			Message:    fmt.Sprintf("Message %q has no %q type field", m.GetName(), resourceMsgName),
+			Descriptor: m,
+		}}
+	},
+}


Test patch:
diff --git a/rules/aip0235/response_resource_field_test.go b/rules/aip0235/response_resource_field_test.go
new file mode 100644
index 0000000..5635245
--- /dev/null
+++ b/rules/aip0235/response_resource_field_test.go
@@ -0,0 +1,86 @@
+// 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 TestResponseResourceField(t *testing.T) {
+	// Set up the testing permutations.
+	tests := []struct {
+		testName    string
+		Message     string
+		Field       string
+		problems    testutils.Problems
+		problemDesc func(m *desc.MessageDescriptor) desc.Descriptor
+	}{
+		{
+			testName: "Valid",
+			Message:  "BatchDeleteBooksResponse",
+			Field:    "repeated Book books",
+			problems: testutils.Problems{},
+		},
+		{
+			testName: "FieldIsNotRepeated",
+			Message:  "BatchDeleteBooksResponse",
+			Field:    "Book book",
+			problems: testutils.Problems{{Message: "repeated"}},
+		},
+		{
+			testName: "MissingField",
+			Message:  "BatchDeleteBooksResponse",
+			Field:    "string response",
+			problems: testutils.Problems{{Message: `no "Book" type field`}},
+			problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor {
+				return m
+			},
+		},
+		{
+			testName: "IrrelevantMessage",
+			Message:  "DeleteBookResponse",
+			Field:    "Book books",
+			problems: testutils.Problems{},
+		},
+	}
+
+	// Run each test individually.
+	for _, test := range tests {
+		t.Run(test.testName, func(t *testing.T) {
+			file := testutils.ParseProto3Tmpl(t, `
+				message {{.Message}} {
+					{{.Field}} = 1;
+				}
+				message Book {
+				}`, test)
+
+			m := file.GetMessageTypes()[0]
+
+			// Determine the descriptor that a failing test will attach to.
+			var problemDesc desc.Descriptor = m.GetFields()[0]
+			if test.problemDesc != nil {
+				problemDesc = test.problemDesc(m)
+			}
+
+			problems := responseResourceField.Lint(file)
+			if diff := test.problems.SetDescriptor(problemDesc).Diff(problems); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}