Repository: googleapis/api-linter
Pull request #440 (2019-12-10 23:56:13)

[Refactor] AIP134: split "request-resource-field" and "request-mask-field"
.


Patch:
diff --git a/docs/rules/0134/request-mask-field.md b/docs/rules/0134/request-mask-field.md
index 97a2d80..c2ace5e 100644
--- a/docs/rules/0134/request-mask-field.md
+++ b/docs/rules/0134/request-mask-field.md
@@ -15,9 +15,8 @@ request message for the field mask, as mandated in [AIP-134][].
 
 ## Details
 
-This rule looks at any message matching `Update*Request` and complains if it
-can not find a field named `update_mask`, or if that field has any type other
-than `google.protobuf.FieldMask`.
+This rule looks at any message matching `Update*Request` and complains if
+the `update_mask` field has any type other than `google.protobuf.FieldMask`.
 
 ## Examples
 
@@ -25,9 +24,10 @@ than `google.protobuf.FieldMask`.
 
 ```proto
 // Incorrect.
-// `google.protobuf.FieldMask update_mask` is missing.
 message UpdateBookRequest {
   Book book = 1;
+  // Field type should be `google.protobuf.FieldMask`.
+  string update_mask = 2;
 }
 ```
 
@@ -48,10 +48,11 @@ 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::0134::request-mask-field=disabled
-//     aip.dev/not-precedent: We need to do this because reasons. --)
 message UpdateBookRequest {
   Book book = 1;
+  // (-- api-linter: core::0134::request-mask-field=disabled
+  //     aip.dev/not-precedent: We need to do this because reasons. --)
+  string update_mask = 2;
 }
 ```
 
diff --git a/docs/rules/0134/request-mask-required.md b/docs/rules/0134/request-mask-required.md
new file mode 100644
index 0000000..130afe4
--- /dev/null
+++ b/docs/rules/0134/request-mask-required.md
@@ -0,0 +1,61 @@
+---
+rule:
+  aip: 134
+  name: [core, '0134', request-mask-required]
+  summary: Update RPCs must have a field mask in the request.
+permalink: /134/request-mask-required
+redirect_from:
+  - /0134/request-mask-required
+---
+
+# Update methods: Mask field
+
+This rule enforces that all `Update` standard methods have a field in the
+request message for the field mask, as mandated in [AIP-134][].
+
+## Details
+
+This rule looks at any message matching `Update*Request` and complains if it
+can not find a field named `update_mask`.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+// `google.protobuf.FieldMask update_mask` is missing.
+message UpdateBookRequest {
+  Book book = 1;
+}
+```
+
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+message UpdateBookRequest {
+  Book book = 1;
+  google.protobuf.FieldMask update_mask = 2;
+}
+```
+
+## Disabling
+
+If you need to violate this rule, use a leading comment above the message.
+Remember to also include an [aip.dev/not-precedent][] comment explaining why.
+
+```proto
+// (-- api-linter: core::0134::request-mask-required=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+message UpdateBookRequest {
+  Book book = 1;
+}
+```
+
+If you need to violate this rule for an entire file, place the comment at the
+top of the file.
+
+[aip-134]: https://aip.dev/134
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
diff --git a/docs/rules/0134/request-resource-field.md b/docs/rules/0134/request-resource-field.md
index e640af1..38f09ad 100644
--- a/docs/rules/0134/request-resource-field.md
+++ b/docs/rules/0134/request-resource-field.md
@@ -15,8 +15,8 @@ request message for the resource itself, as mandated in [AIP-134][].
 
 ## Details
 
-This rule looks at any message matching `Update*Request` and complains if there
-is no field of the resource's type with the expected field name.
+This rule looks at any message matching `Update*Request` and complains if 
+the field of the resource's type is not named properly.
 
 ## Examples
 
@@ -24,16 +24,9 @@ is no field of the resource's type with the expected field name.
 
 ```proto
 // Incorrect.
-// `Book book` is missing.
 message UpdateBookRequest {
-  google.protobuf.FieldMask update_mask = 2;
-}
-```
-
-```proto
-// Incorrect.
-message UpdateBookRequest {
-  Book payload = 1;  // Field name should be `book`.
+  // Field name should be `book`.
+  Book payload = 1;
   google.protobuf.FieldMask update_mask = 2;
 }
 ```
@@ -50,14 +43,13 @@ message UpdateBookRequest {
 
 ## 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).
+If you need to violate this rule, use a leading comment above the field.
 Remember to also include an [aip.dev/not-precedent][] comment explaining why.
 
 ```proto
-// (-- api-linter: core::0134::request-resource-field=disabled
-//     aip.dev/not-precedent: We need to do this because reasons. --)
 message UpdateBookRequest {
+  // (-- api-linter: core::0134::request-resource-field=disabled
+  //     aip.dev/not-precedent: We need to do this because reasons. --)
   Book payload = 1;
   google.protobuf.FieldMask update_mask = 2;
 }
diff --git a/docs/rules/0134/request-resource-required.md b/docs/rules/0134/request-resource-required.md
new file mode 100644
index 0000000..c54e4e2
--- /dev/null
+++ b/docs/rules/0134/request-resource-required.md
@@ -0,0 +1,60 @@
+---
+rule:
+  aip: 134
+  name: [core, '0134', request-resource-required]
+  summary: Update RPCs must have a field for the resource in the request.
+permalink: /134/request-resource-required
+redirect_from:
+  - /0134/request-resource-required
+---
+
+# Update methods: Resource field
+
+This rule enforces that all `Update` standard methods have a field in the
+request message for the resource itself, as mandated in [AIP-134][].
+
+## Details
+
+This rule looks at any message matching `Update*Request` and complains if there
+is no field of the resource's type with the expected field name.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+// `Book book` is missing.
+message UpdateBookRequest {
+  google.protobuf.FieldMask update_mask = 2;
+}
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+message UpdateBookRequest {
+  Book book = 1;
+  google.protobuf.FieldMask update_mask = 2;
+}
+```
+
+## Disabling
+
+If you need to violate this rule, use a leading comment above the message.
+Remember to also include an [aip.dev/not-precedent][] comment explaining why.
+
+```proto
+// (-- api-linter: core::0134::request-resource-required=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+message UpdateBookRequest {
+  google.protobuf.FieldMask update_mask = 2;
+}
+```
+
+If you need to violate this rule for an entire file, place the comment at the
+top of the file.
+
+[aip-134]: https://aip.dev/134
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
diff --git a/rules/aip0134/aip0134.go b/rules/aip0134/aip0134.go
index a4e797d..2ed62e7 100644
--- a/rules/aip0134/aip0134.go
+++ b/rules/aip0134/aip0134.go
@@ -35,8 +35,10 @@ func AddRules(r lint.RuleRegistry) error {
 		methodSignature,
 		responseMessageName,
 		requestMaskField,
+		requestMaskRequired,
 		requestMessageName,
-		resourceField,
+		requestResourceField,
+		requestResourceRequired,
 		synonyms,
 		unknownFields,
 	)
diff --git a/rules/aip0134/request_mask_field.go b/rules/aip0134/request_mask_field.go
index c0bd932..2000ae9 100644
--- a/rules/aip0134/request_mask_field.go
+++ b/rules/aip0134/request_mask_field.go
@@ -20,23 +20,18 @@ import (
 	"github.com/jhump/protoreflect/desc"
 )
 
-var requestMaskField = &lint.MessageRule{
-	Name:   lint.NewRuleName(134, "request-mask-field"),
-	OnlyIf: isUpdateRequestMessage,
-	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
-		updateMask := m.FindFieldByName("update_mask")
-		if updateMask == nil {
-			return []lint.Problem{{
-				Message:    "Update methods should have an `update_mask` field.",
-				Descriptor: m,
-			}}
-		}
-		if t := updateMask.GetMessageType(); t == nil || t.GetFullyQualifiedName() != "google.protobuf.FieldMask" {
+var requestMaskField = &lint.FieldRule{
+	Name: lint.NewRuleName(134, "request-mask-field"),
+	OnlyIf: func(f *desc.FieldDescriptor) bool {
+		return isUpdateRequestMessage(f.GetOwner()) && f.GetName() == "update_mask"
+	},
+	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
+		if t := f.GetMessageType(); t == nil || t.GetFullyQualifiedName() != "google.protobuf.FieldMask" {
 			return []lint.Problem{{
 				Message:    "The `update_mask` field should be a google.protobuf.FieldMask.",
 				Suggestion: "google.protobuf.FieldMask",
-				Descriptor: updateMask,
-				Location:   locations.FieldType(updateMask),
+				Descriptor: f,
+				Location:   locations.FieldType(f),
 			}}
 		}
 		return nil
diff --git a/rules/aip0134/request_mask_required.go b/rules/aip0134/request_mask_required.go
new file mode 100644
index 0000000..1ca2853
--- /dev/null
+++ b/rules/aip0134/request_mask_required.go
@@ -0,0 +1,35 @@
+// Copyright 2019 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 aip0134
+
+import (
+	"github.com/googleapis/api-linter/lint"
+	"github.com/jhump/protoreflect/desc"
+)
+
+var requestMaskRequired = &lint.MessageRule{
+	Name:   lint.NewRuleName(134, "request-mask-required"),
+	OnlyIf: isUpdateRequestMessage,
+	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
+		updateMask := m.FindFieldByName("update_mask")
+		if updateMask == nil {
+			return []lint.Problem{{
+				Message:    "Update methods should have an `update_mask` field.",
+				Descriptor: m,
+			}}
+		}
+		return nil
+	},
+}
diff --git a/rules/aip0134/request_resource_field.go b/rules/aip0134/request_resource_field.go
index bd18b8c..2919588 100644
--- a/rules/aip0134/request_resource_field.go
+++ b/rules/aip0134/request_resource_field.go
@@ -23,34 +23,26 @@ import (
 	"github.com/stoewer/go-strcase"
 )
 
-// The create request message should have resource field.
-var resourceField = &lint.MessageRule{
-	Name:   lint.NewRuleName(134, "request-resource-field"),
-	OnlyIf: isUpdateRequestMessage,
-	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
-		resourceMsgName := extractResource(m.GetName())
-
-		// The rule (resource field name must map to the POST body) is
-		// checked by AIP-0134 ("core::0134::http-body")
-		for _, fieldDesc := range m.GetFields() {
-			if msgDesc := fieldDesc.GetMessageType(); msgDesc != nil && msgDesc.GetName() == resourceMsgName {
-				// Rule check: Is the field named properly?
-				if want := strcase.SnakeCase(resourceMsgName); fieldDesc.GetName() != want {
-					return []lint.Problem{{
-						Message:    fmt.Sprintf("Resource field should be named %q.", want),
-						Descriptor: fieldDesc,
-						Suggestion: want,
-						Location:   locations.DescriptorName(fieldDesc),
-					}}
-				}
-				return nil
-			}
+// The resource field in a update method should named properly.
+var requestResourceField = &lint.FieldRule{
+	Name: lint.NewRuleName(134, "request-resource-field"),
+	OnlyIf: func(f *desc.FieldDescriptor) bool {
+		message := f.GetOwner()
+		return isUpdateRequestMessage(message) &&
+			f.GetMessageType() != nil &&
+			f.GetMessageType().GetName() == extractResource(message.GetName())
+	},
+	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
+		resourceName := extractResource(f.GetOwner().GetName())
+		wantFieldName := strcase.SnakeCase(resourceName)
+		if f.GetName() != wantFieldName {
+			return []lint.Problem{{
+				Message:    fmt.Sprintf("Resource field should be named %q.", wantFieldName),
+				Descriptor: f,
+				Suggestion: wantFieldName,
+				Location:   locations.DescriptorName(f),
+			}}
 		}
-
-		// 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,
-		}}
+		return nil
 	},
 }
diff --git a/rules/aip0134/request_resource_required.go b/rules/aip0134/request_resource_required.go
new file mode 100644
index 0000000..681aef5
--- /dev/null
+++ b/rules/aip0134/request_resource_required.go
@@ -0,0 +1,30 @@
+package aip0134
+
+import (
+	"fmt"
+
+	"github.com/googleapis/api-linter/lint"
+	"github.com/jhump/protoreflect/desc"
+)
+
+// The create request message should have resource field.
+var requestResourceRequired = &lint.MessageRule{
+	Name:   lint.NewRuleName(134, "request-resource-required"),
+	OnlyIf: isUpdateRequestMessage,
+	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
+		resourceMsgName := extractResource(m.GetName())
+		for _, fieldDesc := range m.GetFields() {
+			msgDesc := fieldDesc.GetMessageType()
+			if msgDesc != nil && msgDesc.GetName() == resourceMsgName {
+				// found the resource field.
+				return nil
+			}
+		}
+
+		// No resource field.
+		return []lint.Problem{{
+			Message:    fmt.Sprintf("Message %q has no %q type field", m.GetName(), resourceMsgName),
+			Descriptor: m,
+		}}
+	},
+}


Test patch:
diff --git a/rules/aip0134/request_mask_field_test.go b/rules/aip0134/request_mask_field_test.go
index 7b2950e..6473575 100644
--- a/rules/aip0134/request_mask_field_test.go
+++ b/rules/aip0134/request_mask_field_test.go
@@ -18,43 +18,33 @@ import (
 	"testing"
 
 	"github.com/googleapis/api-linter/rules/internal/testutils"
-	"github.com/jhump/protoreflect/desc"
 )
 
 func TestRequestMaskField(t *testing.T) {
 	tests := []struct {
-		testName   string
-		Field      string
-		descriptor func(*desc.MessageDescriptor) desc.Descriptor
-		problems   testutils.Problems
+		name        string
+		MessageName string
+		FieldType   string
+		FieldName   string
+		problems    testutils.Problems
 	}{
-		{"Valid", "google.protobuf.FieldMask update_mask = 2;", nil, testutils.Problems{}},
-		{"InvalidMissing", "", nil, testutils.Problems{{Message: "`update_mask` field"}}},
-		{
-			"InvalidType",
-			"string update_mask = 2;",
-			func(m *desc.MessageDescriptor) desc.Descriptor {
-				return m.GetFields()[1]
-			},
-			testutils.Problems{{Suggestion: "google.protobuf.FieldMask"}},
-		},
+		{"Valid", "UpdateBookRequest", "google.protobuf.FieldMask", "update_mask", nil},
+		{"InvalidType", "UpdateBookRequest", "string", "update_mask", testutils.Problems{{Suggestion: "google.protobuf.FieldMask"}}},
+		{"IrrelevantMessage", "ModifyBookRequest", "string", "update_mask", nil},
+		{"IrrelevantField", "UpdateBookRequest", "string", "modify_mask", nil},
 	}
 	for _, test := range tests {
-		t.Run(test.testName, func(t *testing.T) {
+		t.Run(test.name, func(t *testing.T) {
 			file := testutils.ParseProto3Tmpl(t, `
 				import "google/protobuf/field_mask.proto";
-				message UpdateBookRequest {
-					Book book = 1;
-					{{.Field}}
+				message {{.MessageName}} {
+					{{.FieldType}} {{.FieldName}} = 1;
 				}
 				message Book {}
 			`, test)
-			message := file.GetMessageTypes()[0]
-			var d desc.Descriptor = message
-			if test.descriptor != nil {
-				d = test.descriptor(message)
-			}
-			if diff := test.problems.SetDescriptor(d).Diff(requestMaskField.Lint(file)); diff != "" {
+			field := file.GetMessageTypes()[0].GetFields()[0]
+			problems := requestMaskField.Lint(file)
+			if diff := test.problems.SetDescriptor(field).Diff(problems); diff != "" {
 				t.Errorf(diff)
 			}
 		})
diff --git a/rules/aip0134/request_mask_required_test.go b/rules/aip0134/request_mask_required_test.go
new file mode 100644
index 0000000..e4e5995
--- /dev/null
+++ b/rules/aip0134/request_mask_required_test.go
@@ -0,0 +1,49 @@
+// Copyright 2019 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 aip0134
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestRequestMaskFieldRequired(t *testing.T) {
+	tests := []struct {
+		name     string
+		Field    string
+		problems testutils.Problems
+	}{
+		{"Valid", "google.protobuf.FieldMask update_mask = 2;", nil},
+		{"InvalidMissing", "", testutils.Problems{{Message: "`update_mask` field"}}},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			file := testutils.ParseProto3Tmpl(t, `
+				import "google/protobuf/field_mask.proto";
+				message UpdateBookRequest {
+					Book book = 1;
+					{{.Field}}
+				}
+				message Book {}
+			`, test)
+			message := file.GetMessageTypes()[0]
+			problems := requestMaskRequired.Lint(file)
+			if diff := test.problems.SetDescriptor(message).Diff(problems); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}
diff --git a/rules/aip0134/request_resource_field_test.go b/rules/aip0134/request_resource_field_test.go
index 6f95ca6..00c68e1 100644
--- a/rules/aip0134/request_resource_field_test.go
+++ b/rules/aip0134/request_resource_field_test.go
@@ -18,59 +18,39 @@ import (
 	"testing"
 
 	"github.com/googleapis/api-linter/rules/internal/testutils"
-	"github.com/jhump/protoreflect/desc"
 )
 
 func TestResourceField(t *testing.T) {
 	// Set up the testing permutations.
 	tests := []struct {
-		testName          string
+		name              string
 		MessageName       string
 		ResourceName      string
 		ResourceFieldName string
-		descriptor        func(*desc.MessageDescriptor) desc.Descriptor
 		problems          testutils.Problems
 	}{
-		{"Valid", "UpdateBookRequest", "Book", "book", nil, testutils.Problems{}},
-		{"ValidTwoWords", "UpdateBigBookRequest", "BigBook", "big_book", nil, testutils.Problems{}},
-		{
-			"InvalidMismatch",
-			"UpdateBookRequest",
-			"Foo",
-			"foo",
-			nil,
-			testutils.Problems{{Message: "has no \"Book\""}}},
-		{
-			"InvalidFieldName",
-			"UpdateBookRequest",
-			"Book",
-			"big_book",
-			func(m *desc.MessageDescriptor) desc.Descriptor {
-				return m.GetFields()[0]
-			},
-			testutils.Problems{{Suggestion: "book"}},
-		},
+		{"Valid", "UpdateBookRequest", "Book", "book", nil},
+		{"InvalidFieldName", "UpdateBookRequest", "Book", "big_book", testutils.Problems{{Suggestion: "book"}}},
+		{"IrrelevantMessage", "ModifyBookRequest", "Book", "big_book", nil},
+		{"IrrelevantFieldType", "UpdateBookRequest", "string", "big_book", nil},
+		{"IrrelevantFieldMessage", "UpdateBookRequest", "Foo", "big_book", nil},
 	}
 
 	// Run each test individually.
 	for _, test := range tests {
-		t.Run(test.testName, func(t *testing.T) {
+		t.Run(test.name, func(t *testing.T) {
 			file := testutils.ParseProto3Tmpl(t, `
 				import "google/protobuf/field_mask.proto";
 				message {{.MessageName}} {
 					{{.ResourceName}} {{.ResourceFieldName}} = 1;
-					google.protobuf.FieldMask update_mask = 2;
 				}
 				message {{.ResourceName}} {}
 			`, test)
-			var d desc.Descriptor = file.GetMessageTypes()[0]
-			if test.descriptor != nil {
-				d = test.descriptor(d.(*desc.MessageDescriptor))
-			}
+			field := file.GetMessageTypes()[0].GetFields()[0]
 
 			// Run the lint rule, and establish that it returns the correct problems.
-			problems := resourceField.Lint(file)
-			if diff := test.problems.SetDescriptor(d).Diff(problems); diff != "" {
+			problems := requestResourceField.Lint(file)
+			if diff := test.problems.SetDescriptor(field).Diff(problems); diff != "" {
 				t.Errorf(diff)
 			}
 		})
diff --git a/rules/aip0134/request_resource_required_test.go b/rules/aip0134/request_resource_required_test.go
new file mode 100644
index 0000000..c932924
--- /dev/null
+++ b/rules/aip0134/request_resource_required_test.go
@@ -0,0 +1,40 @@
+package aip0134
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestRequestResourceFieldRequired(t *testing.T) {
+	// Set up the testing permutations.
+	tests := []struct {
+		name              string
+		MessageName       string
+		ResourceName      string
+		ResourceFieldName string
+		problems          testutils.Problems
+	}{
+		{"Valid", "UpdateBookRequest", "Book", "book", nil},
+		{"ValidTwoWords", "UpdateBigBookRequest", "BigBook", "big_book", nil},
+		{"InvalidMismatch", "UpdateBookRequest", "Foo", "foo", testutils.Problems{{Message: "has no \"Book\""}}},
+	}
+
+	// Run each test individually.
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			file := testutils.ParseProto3Tmpl(t, `
+				import "google/protobuf/field_mask.proto";
+				message {{.MessageName}} {
+					{{.ResourceName}} {{.ResourceFieldName}} = 1;
+				}
+				message {{.ResourceName}} {}
+			`, test)
+			message := file.GetMessageTypes()[0]
+			problems := requestResourceRequired.Lint(file)
+			if diff := test.problems.SetDescriptor(message).Diff(problems); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}