Repository: googleapis/api-linter
Pull request #480 (2020-02-03 19:21:41)

[fix] AIP-162 tag/list/delete methods should not false positive.


Patch:
diff --git a/rules/aip0136/aip0136.go b/rules/aip0136/aip0136.go
index 60b0c2a..a606201 100644
--- a/rules/aip0136/aip0136.go
+++ b/rules/aip0136/aip0136.go
@@ -19,6 +19,7 @@ import (
 	"strings"
 
 	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/rules/internal/utils"
 	"github.com/jhump/protoreflect/desc"
 )
 
@@ -38,6 +39,16 @@ func AddRules(r lint.RuleRegistry) error {
 }
 
 func isCustomMethod(m *desc.MethodDescriptor) bool {
+	// Anything with a `:` in the method URI is automatically a custom
+	// method, regardless of the RPC name.
+	for _, httpRule := range utils.GetHTTPRules(m) {
+		if strings.Contains(httpRule.GetPlainURI(), ":") {
+			return true
+		}
+	}
+
+	// Methods with no `:` in the URI are standard methods if they begin with
+	// one of the standard method names.
 	for _, prefix := range []string{"Get", "List", "Create", "Update", "Delete", "Replace"} {
 		if strings.HasPrefix(m.GetName(), prefix) {
 			return false
diff --git a/rules/aip0136/http_name_variable.go b/rules/aip0136/http_name_variable.go
index 51a5f95..b6bc6d9 100644
--- a/rules/aip0136/http_name_variable.go
+++ b/rules/aip0136/http_name_variable.go
@@ -32,6 +32,12 @@ var httpNameVariable = &lint.MethodRule{
 		for _, http := range utils.GetHTTPRules(m) {
 			vars := http.GetVariables()
 
+			// Special case: AIP-162 describes "revision" methods; the `name`
+			// variable is appropriate (and mandated) for those.
+			if strings.HasSuffix(m.GetName(), "Revision") || strings.HasSuffix(m.GetName(), "Revisions") {
+				return nil
+			}
+
 			// If there is a "name" variable, the noun should be present
 			// in the RPC name.
 			if name, ok := vars["name"]; ok {
diff --git a/rules/aip0136/http_uri_suffix.go b/rules/aip0136/http_uri_suffix.go
index 981fbcb..0b0c50b 100644
--- a/rules/aip0136/http_uri_suffix.go
+++ b/rules/aip0136/http_uri_suffix.go
@@ -57,6 +57,22 @@ var uriSuffix = &lint.MethodRule{
 				}
 			}
 
+			// AIP-162 introduces some special cases around revisions, where
+			// `ListFooRevisions` gets a suffix of `:listRevisions` (and the same for
+			// `Delete` and `Tag`).
+			n := m.GetName()
+			if strings.HasPrefix(n, "List") && strings.HasSuffix(m.GetName(), "Revisions") {
+				want = ":listRevisions"
+			}
+			if strings.HasSuffix(m.GetName(), "Revision") {
+				if strings.HasPrefix(m.GetName(), "Tag") {
+					want = ":tagRevision"
+				}
+				if strings.HasPrefix(m.GetName(), "Delete") {
+					want = ":deleteRevision"
+				}
+			}
+
 			// Do we have the suffix we expect?
 			if !strings.HasSuffix(httpRule.URI, want) {
 				// FIXME: We intentionally only return one Problem here.


Test patch:
diff --git a/rules/aip0136/http_method_test.go b/rules/aip0136/http_method_test.go
index 4f3c412..43e9125 100644
--- a/rules/aip0136/http_method_test.go
+++ b/rules/aip0136/http_method_test.go
@@ -43,7 +43,7 @@ func TestHttpMethod(t *testing.T) {
 				service Library {
 					rpc {{.MethodName}}({{.MethodName}}Request) returns ({{.MethodName}}Response) {
 						option (google.api.http) = {
-							{{.HTTPMethod}}: "/v1:{{.MethodName}}"
+							{{.HTTPMethod}}: "/v1/{{.MethodName}}"
 						};
 					}
 				}
diff --git a/rules/aip0136/http_name_variable_test.go b/rules/aip0136/http_name_variable_test.go
index 9fe9ee2..2661743 100644
--- a/rules/aip0136/http_name_variable_test.go
+++ b/rules/aip0136/http_name_variable_test.go
@@ -28,6 +28,8 @@ func TestHTTPVariables(t *testing.T) {
 		problems   testutils.Problems
 	}{
 		{"Valid", "WriteBook", "/v1/{name=publishers/*/books/*}:write", testutils.Problems{}},
+		{"ValidRevision", "TagBookRevision", "/v1/{name=publishers/*/books/*}:tagRevision", testutils.Problems{}},
+		{"ValidRevisions", "ListBookRevisions", "/v1/{name=publishers/*/books/*}:listRevisions", testutils.Problems{}},
 		{"Invalid", "WritePage", "/v1/{name=publishers/*/books/*}:writePage", testutils.Problems{{Message: "name variable"}}},
 		{"ValidBookVar", "WritePage", "/v1/{book=publishers/*/books/*}:writePage", testutils.Problems{}},
 	} {
diff --git a/rules/aip0136/http_uri_suffix_test.go b/rules/aip0136/http_uri_suffix_test.go
index 55c0ac5..a4a5dde 100644
--- a/rules/aip0136/http_uri_suffix_test.go
+++ b/rules/aip0136/http_uri_suffix_test.go
@@ -36,6 +36,12 @@ func TestURISuffix(t *testing.T) {
 		{"InvalidVerbNoun", "TranslateText", "/v3:translate", testutils.Problems{{Message: ":translateText"}}},
 		{"ValidOneWord", "Translate", "/v3:translate", testutils.Problems{}},
 		{"ValidStdMethod", "GetBook", "/v1/{name=publishers/*/books/*}", testutils.Problems{}},
+		{"ValidListRevisions", "ListBookRevisions", "/v1/{name=publishers/*/books/*}:listRevisions", testutils.Problems{}},
+		{"ValidTagRevision", "TagBookRevision", "/v1/{name=publishers/*/books/*}:tagRevision", testutils.Problems{}},
+		{"ValidDeleteRevision", "DeleteBookRevision", "/v1/{name=publishers/*/books/*}:deleteRevision", testutils.Problems{}},
+		{"InvalidListRevisions", "ListBookRevisions", "/v1/{name=publishers/*/books/*}:list", testutils.Problems{{Message: ":listRevisions"}}},
+		{"InvalidTagRevision", "TagBookRevision", "/v1/{name=publishers/*/books/*}:tag", testutils.Problems{{Message: ":tagRevision"}}},
+		{"InvalidDeleteRevision", "DeleteBookRevision", "/v1/{name=publishers/*/books/*}:delete", testutils.Problems{{Message: ":deleteRevision"}}},
 		{"IgnoredFailsVariables", "AddPages", "/v1/{name=publishers/*/books/*}:addPages", testutils.Problems{}},
 	}
 	for _, test := range tests {