Repository: googleapis/api-linter
Pull request #475 (2020-01-30 21:08:24)

[AIP-136] Enforce parent variable consistency.
The AIP-136 [http-uri-suffix](https://linter.aip.dev/136/http-uri-suffix) rule continues to be the single most confusing for people. The false positive rate is basically zero, but the problems it finds are varied and subtle and so the error message is useless a lot of the time.

This adds a new rule, http-parent-variable, for catching a common case that _looks_ like a false positive (but is not) and quiets the http-uri-suffix rule if the other rule is noisy.


Patch:
diff --git a/docs/rules/0136/http-name-variable.md b/docs/rules/0136/http-name-variable.md
index 71da9d6..34b8025 100644
--- a/docs/rules/0136/http-name-variable.md
+++ b/docs/rules/0136/http-name-variable.md
@@ -39,6 +39,8 @@ rpc WritePage(WritePageRequest) return (WritePageResponse) {
 
 ```proto
 // Correct.
+// If Page is not a first-class resource, use `book` as the variable name
+// and a verb-noun suffix.
 rpc WritePage(WritePageRequest) return (WritePageResponse) {
   option (google.api.http) = {
     post: "/v1/{book=publishers/*/books/*}:writePage"
@@ -49,14 +51,19 @@ rpc WritePage(WritePageRequest) return (WritePageResponse) {
 
 ```proto
 // Correct.
-rpc WriteBook(WriteBookRequest) return (WriteBookResponse) {
+// If Page is a first-class, already-created resource, use `name` as the
+// variable name and a verb-only suffix.
+rpc WritePage(WritePageRequest) return (WritePageResponse) {
   option (google.api.http) = {
-    post: "/v1/{name=publishers/*/books/*}:write"
+    post: "/v1/{name=publishers/*/books/*/pages/*}:write"
     body: "*"
   };
 }
 ```
 
+See also the [http-parent-variable][] rule (for first-class resources that are
+created by the custom RPC).
+
 ## Disabling
 
 If you need to violate this rule, use a leading comment above the method.
@@ -78,3 +85,4 @@ top of the file.
 
 [aip-136]: https://aip.dev/136
 [aip.dev/not-precedent]: https://aip.dev/not-precedent
+[http-parent-variable]: ./http-parent-variable.md
diff --git a/docs/rules/0136/http-parent-variable.md b/docs/rules/0136/http-parent-variable.md
new file mode 100644
index 0000000..25d7e65
--- /dev/null
+++ b/docs/rules/0136/http-parent-variable.md
@@ -0,0 +1,89 @@
+---
+rule:
+  aip: 136
+  name: [core, '0136', http-parent-variable]
+  summary: |
+    Custom methods should only use `parent` if the RPC noun matches the
+    resource.
+permalink: /136/http-parent-variable
+redirect_from:
+  - /0136/http-parent-variable
+---
+
+# Custom methods: HTTP parent variable
+
+This rule enforces that custom methods only use the `parent` variable if the
+RPC noun matches the resource, as mandated in [AIP-136][].
+
+## Details
+
+This rule looks at custom methods and, if the URI contains a `parent` variable,
+it ensures that the RPC name ends with the same text as the final component in
+the URI (after adjusting for case).
+
+## Examples
+
+**Incorrect** code for this rule:
+
+```proto
+// Incorrect.
+// The variable should be "book", or the RPC name should change.
+rpc WritePage(WritePageRequest) return (WritePageResponse) {
+  option (google.api.http) = {
+    post: "/v1/{parent=publishers/*/books/*}:writePage"
+    body: "*"
+  };
+}
+```
+
+**Correct** code for this rule:
+
+```proto
+// Correct.
+// If Page is not a first-class resource, use `book` as the variable name
+// and a verb-noun suffix.
+rpc WritePage(WritePageRequest) return (WritePageResponse) {
+  option (google.api.http) = {
+    post: "/v1/{book=publishers/*/books/*}:writePage"
+    body: "*"
+  };
+}
+```
+
+```proto
+// Correct.
+// If Page is a first-class, not-yet-created resource, use `parent` as the
+// variable name and a verb-only suffix.
+rpc WritePage(WritePageRequest) return (WritePageResponse) {
+  option (google.api.http) = {
+    post: "/v1/{parent=publishers/*/books/*}/pages:write"
+    body: "*"
+  };
+}
+```
+
+See also the [http-name-variable][] rule (for first-class resources that are
+already created).
+
+## Disabling
+
+If you need to violate this rule, use a leading comment above the method.
+Remember to also include an [aip.dev/not-precedent][] comment explaining why.
+
+```proto
+// (-- api-linter: core::0136::http-parent-variable=disabled
+//     aip.dev/not-precedent: We need to do this because reasons. --)
+rpc WritePage(WritePageRequest) return (WritePageResponse) {
+  option (google.api.http) = {
+    post: "/v1/{parent=publishers/*/books/*}:writePage"
+    body: "*"
+  };
+}
+```
+
+If you need to violate this rule for an entire file, place the comment at the
+top of the file.
+
+[aip-136]: https://aip.dev/136
+[aip.dev/not-precedent]: https://aip.dev/not-precedent
+[http-name-variable]: ./http-name-variable.md
diff --git a/docs/rules/0136/http-uri-suffix.md b/docs/rules/0136/http-uri-suffix.md
index 5107614..bfa3622 100644
--- a/docs/rules/0136/http-uri-suffix.md
+++ b/docs/rules/0136/http-uri-suffix.md
@@ -22,6 +22,10 @@ the appropriate suffix at the end of the URI. More specifically:
   the end of the URI.
 - Otherwise, it expects `:verbNoun` at the end of the URI.
 
+**Note:** This rule will not run if the [http-name-variable][] or
+[http-parent-variable][] rules raise an issue, as a significant number of
+issues raised by this rule are _actually_ violations of one of those.
+
 ## Examples
 
 ### Verb only
@@ -99,3 +103,5 @@ top of the file.
 
 [aip-136]: https://aip.dev/136
 [aip.dev/not-precedent]: https://aip.dev/not-precedent
+[http-name-variable]: ./http-name-variable.md
+[http-parent-variable]: ./http-parent-variable.md
diff --git a/rules/aip0136/aip0136.go b/rules/aip0136/aip0136.go
index 19e0a26..60b0c2a 100644
--- a/rules/aip0136/aip0136.go
+++ b/rules/aip0136/aip0136.go
@@ -30,6 +30,7 @@ func AddRules(r lint.RuleRegistry) error {
 		httpBody,
 		httpMethod,
 		httpNameVariable,
+		httpParentVariable,
 		noPrepositions,
 		uriSuffix,
 		verbNoun,
diff --git a/rules/aip0136/http_parent_variable.go b/rules/aip0136/http_parent_variable.go
new file mode 100644
index 0000000..d5b8633
--- /dev/null
+++ b/rules/aip0136/http_parent_variable.go
@@ -0,0 +1,56 @@
+// 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 aip0136
+
+import (
+	"strings"
+
+	pluralize "github.com/gertd/go-pluralize"
+	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/rules/internal/utils"
+	"github.com/jhump/protoreflect/desc"
+	"github.com/stoewer/go-strcase"
+)
+
+var httpParentVariable = &lint.MethodRule{
+	Name:   lint.NewRuleName(136, "http-parent-variable"),
+	OnlyIf: isCustomMethod,
+	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
+		p := pluralize.NewClient()
+		for _, http := range utils.GetHTTPRules(m) {
+			vars := http.GetVariables()
+
+			// If there is a "parent" variable, the noun should be present
+			// in the RPC name.
+			if _, ok := vars["parent"]; ok {
+				// Determine the resource.
+				segs := strings.Split(strings.Split(http.GetPlainURI(), ":")[0], "/")
+				plural := segs[len(segs)-1]
+				singular := p.Singular(plural)
+
+				// Does the RPC name end in the singular or plural name of the resource?
+				// If not, complain.
+				snakeName := strcase.SnakeCase(m.GetName())
+				if !strings.HasSuffix(snakeName, plural) && !strings.HasSuffix(snakeName, singular) {
+					return []lint.Problem{{
+						Message:    "The parent variable should only be used if the RPC noun matches the URI.",
+						Descriptor: m,
+					}}
+				}
+			}
+		}
+		return nil
+	},
+}
diff --git a/rules/aip0136/http_uri_suffix.go b/rules/aip0136/http_uri_suffix.go
index ed3cfbf..33b9f2d 100644
--- a/rules/aip0136/http_uri_suffix.go
+++ b/rules/aip0136/http_uri_suffix.go
@@ -29,7 +29,7 @@ import (
 var uriSuffix = &lint.MethodRule{
 	Name: lint.NewRuleName(136, "http-uri-suffix"),
 	OnlyIf: func(m *desc.MethodDescriptor) bool {
-		return isCustomMethod(m) && httpNameVariable.LintMethod(m) == nil
+		return isCustomMethod(m) && httpNameVariable.LintMethod(m) == nil && httpParentVariable.LintMethod(m) == nil
 	},
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		for _, httpRule := range utils.GetHTTPRules(m) {


Test patch:
diff --git a/rules/aip0136/http_parent_variable_test.go b/rules/aip0136/http_parent_variable_test.go
new file mode 100644
index 0000000..ba2ccb6
--- /dev/null
+++ b/rules/aip0136/http_parent_variable_test.go
@@ -0,0 +1,56 @@
+// 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 aip0136
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestHTTPParentVariable(t *testing.T) {
+	for _, test := range []struct {
+		name       string
+		MethodName string
+		URI        string
+		problems   testutils.Problems
+	}{
+		{"Valid", "WriteBook", "/v1/{parent=publishers/*}/books:write", testutils.Problems{}},
+		{"ValidPlural", "WriteBook", "/v1/{parent=publishers/*}/books:write", testutils.Problems{}},
+		{"Invalid", "WritePage", "/v1/{parent=publishers/*/books/*}:writePage", testutils.Problems{{Message: "parent variable"}}},
+		{"ValidBookVar", "WritePage", "/v1/{book=publishers/*/books/*}:writePage", testutils.Problems{}},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				import "google/api/annotations.proto";
+				service Library {
+					rpc {{.MethodName}}({{.MethodName}}Request)
+							returns ({{.MethodName}}Response) {
+						option (google.api.http) = {
+							post: "{{.URI}}"
+							body: "*"
+						};
+					}
+				}
+				message {{.MethodName}}Request {}
+				message {{.MethodName}}Response {}
+			`, test)
+			m := f.GetServices()[0].GetMethods()[0]
+			if diff := test.problems.SetDescriptor(m).Diff(httpParentVariable.Lint(f)); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}