Repository: googleapis/api-linter
Pull request #383 (2019-11-24 00:57:19)

[Refactor] Split 0131::request-name-field into two rules.
It separates 0131::request-name-field to two rules:
1. Check if a Get request message has a name field -- users disable it at the message level;
2. Check if the name field type is string -- users disable it at the field level.

There are some code and documentation duplication, but it is much clearer about how to disable the rules. And each rule is smaller and easier to understand and test.


Patch:
diff --git a/docs/rules/0131/request-name-field.md b/docs/rules/0131/request-name-field.md
index 062c842..1f14e23 100644
--- a/docs/rules/0131/request-name-field.md
+++ b/docs/rules/0131/request-name-field.md
@@ -2,7 +2,7 @@
 rule:
   aip: 131
   name: [core, '0131', request-name-field]
-  summary: Get RPCs must have a `name` field in the request.
+  summary: Get RPCs must have a `string name` field in the request.
 permalink: /131/request-name-field
 redirect_from:
   - /0131/request-name-field
@@ -15,8 +15,8 @@ in the request message, as mandated in [AIP-131][].
 
 ## Details
 
-This rule looks at any message matching `Get*Request` and complains if either
-the `name` field is missing, or if it has any type other than `string`.
+This rule looks at any message matching `Get*Request` and complains if
+the `name` field type is not `string`.
 
 ## Examples
 
@@ -25,20 +25,7 @@ the `name` field is missing, or if it has any type other than `string`.
 ```proto
 // Incorrect.
 message GetBookRequest {
-  string book = 1 [  // Field name should be `name`.
-    (google.api.field_behavior) = REQUIRED,
-    (google.api.resource_reference).type = "library.googleapis.com/Book"
-  ];
-}
-```
-
-```proto
-// Incorrect.
-message GetBookRequest {
-  bytes name = 1 [  // Field type should be `string`.
-    (google.api.field_behavior) = REQUIRED,
-    (google.api.resource_reference).type = "library.googleapis.com/Book"
-  ];
+  bytes name = 1;  // Field type should be `string`.
 }
 ```
 
@@ -47,24 +34,21 @@ message GetBookRequest {
 ```proto
 // Correct.
 message GetBookRequest {
-  string name = 1 [
-    (google.api.field_behavior) = REQUIRED,
-    (google.api.resource_reference).type = "library.googleapis.com/Book"
-  ];
+  string name = 1;
 }
 ```
 
 ## Disabling
 
-If you need to violate this rule, use a leading comment above the message (if
-the `name` field is missing) or above the field (if it is the wrong type).
+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::0131::request-name-field=disabled
-//     aip.dev/not-precedent: This is named "book" for historical reasons. --)
+
 message GetBookRequest {
-  string book = 1;
+  // (-- api-linter: core::0131::request-name-field=disabled
+  //     aip.dev/not-precedent: This uses `bytes` for historical reasons. --)
+  bytes name = 1;
 }
 ```
 
diff --git a/docs/rules/0131/request-name-required.md b/docs/rules/0131/request-name-required.md
new file mode 100644
index 0000000..43b8e27
--- /dev/null
+++ b/docs/rules/0131/request-name-required.md
@@ -0,0 +1,67 @@
+---
+rule:
+  aip: 131
+  name: [core, '0131', request-name-required]
+  summary: Get RPCs must have a `name` field in the request.
+permalink: /131/request-name-required
+redirect_from:
+  - /0131/request-name-required
+---
+
+# Get methods: Name field
+
+This rule enforces that all `Get` standard methods have a `string name` field
+in the request message, as mandated in [AIP-131][].
+
+## Details
+
+This rule looks at any message matching `Get*Request` and complains if
+the `name` field is missing.
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+message GetBookRequest {
+  string book = 1 [  // Field name should be `name`.
+    (google.api.field_behavior) = REQUIRED,
+    (google.api.resource_reference).type = "library.googleapis.com/Book"
+  ];
+}
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+message GetBookRequest {
+  string name = 1 [
+    (google.api.field_behavior) = REQUIRED,
+    (google.api.resource_reference).type = "library.googleapis.com/Book"
+  ];
+}
+```
+
+## 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::0131::request-name-required=disabled
+//     aip.dev/not-precedent: This is named "book" for historical reasons. --)
+message GetBookRequest {
+  string book = 1 [
+    (google.api.field_behavior) = REQUIRED,
+    (google.api.resource_reference).type = "library.googleapis.com/Book"
+  ];
+}
+```
+
+If you need to violate this rule for an entire file, place the comment at the
+top of the file.
+
+[aip-131]: https://aip.dev/131
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
diff --git a/rules/aip0131/aip0131.go b/rules/aip0131/aip0131.go
index 29c70eb..e0245f4 100644
--- a/rules/aip0131/aip0131.go
+++ b/rules/aip0131/aip0131.go
@@ -34,8 +34,9 @@ func AddRules(r lint.RuleRegistry) error {
 		responseMessageName,
 		requestMessageName,
 		requestNameBehavior,
+		requestNameField,
 		requestNameReference,
-		standardFields,
+		requestNameRequired,
 		synonyms,
 		unknownFields,
 	)
diff --git a/rules/aip0131/request_name_field.go b/rules/aip0131/request_name_field.go
index ec556f9..fb647dd 100644
--- a/rules/aip0131/request_name_field.go
+++ b/rules/aip0131/request_name_field.go
@@ -15,34 +15,24 @@
 package aip0131
 
 import (
-	"fmt"
-
 	"github.com/googleapis/api-linter/lint"
 	"github.com/googleapis/api-linter/locations"
 	"github.com/jhump/protoreflect/desc"
 	"github.com/jhump/protoreflect/desc/builder"
 )
 
-// The Get standard method should only have expected fields.
-var standardFields = &lint.MessageRule{
-	Name:   lint.NewRuleName(131, "request-name-field"),
-	OnlyIf: isGetRequestMessage,
-	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
-		// Rule check: Establish that a name field is present.
-		name := m.FindFieldByName("name")
-		if name == nil {
-			return []lint.Problem{{
-				Message:    fmt.Sprintf("Method %q has no `name` field", m.GetName()),
-				Descriptor: m,
-			}}
-		}
-
-		// Rule check: Ensure that the name field is the correct type.
-		if name.GetType() != builder.FieldTypeString().GetType() {
+// Get request should have a string name field.
+var requestNameField = &lint.FieldRule{
+	Name: lint.NewRuleName(131, "request-name-field"),
+	OnlyIf: func(f *desc.FieldDescriptor) bool {
+		return isGetRequestMessage(f.GetOwner()) && f.GetName() == "name"
+	},
+	LintField: func(f *desc.FieldDescriptor) []lint.Problem {
+		if f.GetType() != builder.FieldTypeString().GetType() {
 			return []lint.Problem{{
 				Message:    "`name` field on Get RPCs should be a string",
-				Descriptor: name,
-				Location:   locations.FieldType(name),
+				Descriptor: f,
+				Location:   locations.FieldType(f),
 				Suggestion: "string",
 			}}
 		}
diff --git a/rules/aip0131/request_name_required.go b/rules/aip0131/request_name_required.go
new file mode 100644
index 0000000..591636a
--- /dev/null
+++ b/rules/aip0131/request_name_required.go
@@ -0,0 +1,38 @@
+// 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 aip0131
+
+import (
+	"fmt"
+
+	"github.com/googleapis/api-linter/lint"
+	"github.com/jhump/protoreflect/desc"
+)
+
+// The Get standard method should have some required fields.
+var requestNameRequired = &lint.MessageRule{
+	Name:   lint.NewRuleName(131, "request-name-required"),
+	OnlyIf: isGetRequestMessage,
+	LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
+		if m.FindFieldByName("name") == nil {
+			return []lint.Problem{{
+				Message:    fmt.Sprintf("Method %q has no `name` field", m.GetName()),
+				Descriptor: m,
+			}}
+		}
+
+		return nil
+	},
+}


Test patch:
diff --git a/rules/aip0131/request_name_field_test.go b/rules/aip0131/request_name_field_test.go
index 638cfad..99f3ebb 100644
--- a/rules/aip0131/request_name_field_test.go
+++ b/rules/aip0131/request_name_field_test.go
@@ -18,59 +18,31 @@ import (
 	"testing"
 
 	"github.com/googleapis/api-linter/rules/internal/testutils"
-	"github.com/jhump/protoreflect/desc/builder"
 )
 
-func TestStandardFields(t *testing.T) {
-	// Set up the testing permutations.
+func TestRequestNameFieldType(t *testing.T) {
 	tests := []struct {
-		testName      string
-		messageName   string
-		nameFieldName string
+		name          string
+		MessageName   string
+		NameFieldType string
 		problems      testutils.Problems
 	}{
-		{"Valid", "GetBookRequest", "name", testutils.Problems{}},
-		{"InvalidName", "GetBookRequest", "id", testutils.Problems{{Message: "name"}}},
-		{"Irrelevant", "AcquireBookRequest", "id", testutils.Problems{}},
+		{"StringNameFieldType_Valid", "GetBookRequest", "string", nil},
+		{"BytesNameFieldType_Invalid", "GetBookRequest", "bytes", testutils.Problems{{Suggestion: "string"}}},
+		{"NotGetRequest_BytesNameFieldType_Valid", "SomeMessage", "bytes", nil},
 	}
 
-	// Run each test individually.
 	for _, test := range tests {
-		t.Run(test.testName, func(t *testing.T) {
-			// Create an appropriate message descriptor.
-			message, err := builder.NewMessage(test.messageName).AddField(
-				builder.NewField(test.nameFieldName, builder.FieldTypeString()),
-			).Build()
-			if err != nil {
-				t.Fatalf("Could not build %s message.", test.messageName)
-			}
-
-			// Run the lint rule, and establish that it returns the correct problems.
-			problems := standardFields.Lint(message.GetFile())
-			if diff := test.problems.SetDescriptor(message).Diff(problems); diff != "" {
+		t.Run(test.name, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+			message {{.MessageName}} {
+				{{.NameFieldType}} name = 1;
+			}`, test)
+
+			problems := requestNameField.Lint(f)
+			if diff := test.problems.SetDescriptor(f.GetMessageTypes()[0].GetFields()[0]).Diff(problems); diff != "" {
 				t.Errorf("Problems did not match: %v", diff)
 			}
 		})
 	}
 }
-
-func TestStandardFieldsInvalidType(t *testing.T) {
-	// Create an appropriate message descriptor.
-	message, err := builder.NewMessage("GetBookRequest").AddField(
-		builder.NewField("name", builder.FieldTypeBytes()),
-	).Build()
-	if err != nil {
-		t.Fatalf("Could not build descriptor.")
-	}
-
-	// Run the lint rule, and establish that it returns the correct
-	// number of problems.
-	wantProblems := testutils.Problems{{
-		Descriptor: message.GetFields()[0],
-		Suggestion: "string",
-	}}
-	gotProblems := standardFields.Lint(message.GetFile())
-	if diff := wantProblems.Diff(gotProblems); diff != "" {
-		t.Errorf(diff)
-	}
-}
diff --git a/rules/aip0131/request_name_required_test.go b/rules/aip0131/request_name_required_test.go
new file mode 100644
index 0000000..25c87b1
--- /dev/null
+++ b/rules/aip0131/request_name_required_test.go
@@ -0,0 +1,51 @@
+// 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 aip0131
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestRequestHasNameField(t *testing.T) {
+	// Set up the testing permutations.
+	tests := []struct {
+		testName    string
+		MessageName string
+		FieldName   string
+		problems    testutils.Problems
+	}{
+		{"Valid", "GetBookRequest", "name", testutils.Problems{}},
+		{"InvalidName", "GetBookRequest", "id", testutils.Problems{{Message: "name"}}},
+		{"Irrelevant", "AcquireBookRequest", "id", testutils.Problems{}},
+	}
+
+	// Run each test individually.
+	for _, test := range tests {
+		t.Run(test.testName, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+			message {{.MessageName}} {
+				string {{.FieldName}} = 1;
+			}`, test)
+
+			// Run the lint rule, and establish that it returns the correct problems.
+			problems := requestNameRequired.Lint(f)
+			if diff := test.problems.SetDescriptor(f.GetMessageTypes()[0]).Diff(problems); diff != "" {
+				t.Errorf("Problems did not match: %v", diff)
+			}
+		})
+	}
+}