fix(aip-130): identify standard and custom methods (#1396)

rules/aip0136/aip0136.go

@@ -16,11 +16,7 @@
 package aip0136
 
 import (
-	"strings"
-
 	"github.com/googleapis/api-linter/lint"
-	"github.com/googleapis/api-linter/rules/internal/utils"
-	"github.com/jhump/protoreflect/desc"
 )
 
 // AddRules accepts a register function and registers each of
@@ -42,22 +38,3 @@ func AddRules(r lint.RuleRegistry) error {
 		// httpParentVariable,
 	)
 }
-
-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
-		}
-	}
-	return true
-}

rules/aip0136/http_body.go

@@ -27,7 +27,7 @@ import (
 
 var httpBody = &lint.MethodRule{
 	Name:   lint.NewRuleName(136, "http-body"),
-	OnlyIf: isCustomMethod,
+	OnlyIf: utils.IsCustomMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		for _, httpRule := range utils.GetHTTPRules(m) {
 			noBody := stringset.New("GET", "DELETE")

rules/aip0136/http_method.go

@@ -25,7 +25,7 @@ import (
 
 var httpMethod = &lint.MethodRule{
 	Name:   lint.NewRuleName(136, "http-method"),
-	OnlyIf: isCustomMethod,
+	OnlyIf: utils.IsCustomMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// DeleteFooRevision is still a custom method, but delete is expected
 		// (enforced in AIP-162 rules).

rules/aip0136/http_method_test.go

@@ -36,7 +36,6 @@ func TestHttpMethod(t *testing.T) {
 		{"InvalidDelete", "ArchiveBook", "delete", "", testutils.Problems{{Message: "POST or GET"}}},
 		{"IrrelevantPatch", "UpdateBook", "patch", "", nil},
 		{"IrrelevantDelete", "DeleteBook", "delete", "", nil},
-		{"IrrelevantPut", "ReplaceBook", "put", "", nil},
 	}
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {

rules/aip0136/http_name_variable.go

@@ -27,7 +27,7 @@ import (
 
 var httpNameVariable = &lint.MethodRule{
 	Name:   lint.NewRuleName(136, "http-name-variable"),
-	OnlyIf: isCustomMethod,
+	OnlyIf: utils.IsCustomMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		p := pluralize.NewClient()
 		for _, http := range utils.GetHTTPRules(m) {

rules/aip0136/http_parent_variable.go

@@ -27,7 +27,7 @@ import (
 
 var httpParentVariable = &lint.MethodRule{
 	Name:   lint.NewRuleName(136, "http-parent-variable"),
-	OnlyIf: isCustomMethod,
+	OnlyIf: utils.IsCustomMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		p := pluralize.NewClient()
 		for _, http := range utils.GetHTTPRules(m) {

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 && httpParentVariable.LintMethod(m) == nil
+		return utils.IsCustomMethod(m) && httpNameVariable.LintMethod(m) == nil && httpParentVariable.LintMethod(m) == nil
 	},
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		for _, httpRule := range utils.GetHTTPRules(m) {

rules/aip0136/http_uri_suffix_test.go

@@ -39,14 +39,8 @@ func TestURISuffix(t *testing.T) {
 		{"ValidOneWord", "Translate", "/v3:translate", testutils.Problems{}},
 		{"ValidStdMethod", "GetBook", "/v1/{name=publishers/*/books/*}", testutils.Problems{}},
 		{"ValidTwoWordNoun", "WriteAudioBook", "/v1/{name=publishers/*/audioBooks/*}:write", 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{}},
 		{"ValidCollection", "SortBooks", "/v1/{publisher=publishers/*}/books:sort", testutils.Problems{}},
 		{"ValidNoParent", "SearchBooks", "/v1/books:search", 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 {

rules/aip0136/request_message_name.go

@@ -22,6 +22,6 @@ import (
 // Custom methods should have a properly named Request message.
 var requestMessageName = &lint.MethodRule{
 	Name:       lint.NewRuleName(136, "request-message-name"),
-	OnlyIf:     isCustomMethod,
+	OnlyIf:     utils.IsCustomMethod,
 	LintMethod: utils.LintMethodHasMatchingRequestName,
 }

rules/aip0136/response_message_name.go

@@ -31,7 +31,7 @@ const responseMessageNameErrorMessage = "" +
 // with a Response suffix, or the resource being operated on.
 var responseMessageName = &lint.MethodRule{
 	Name:   lint.NewRuleName(136, "response-message-name"),
-	OnlyIf: isCustomMethod,
+	OnlyIf: utils.IsCustomMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// A response is considered valid if
 		// - The response name matches the RPC name with a `Response` suffix

rules/aip0136/response_message_name_test.go

@@ -162,4 +162,41 @@ func TestResponseMessageName(t *testing.T) {
 			})
 		}
 	})
+
+	t.Run("Batch methods", func(t *testing.T) {
+		// Set up the testing permutations.
+		tests := []struct {
+			testName   string
+			MethodName string
+			problems   testutils.Problems
+		}{
+			{"BatchGet", "BatchGetBooks", testutils.Problems{}},
+			{"BatchUpdate", "BatchUpdateBooks", testutils.Problems{}},
+			{"BatchCreate", "BatchCreateBooks", testutils.Problems{}},
+			{"BatchDelete", "BatchDeleteBooks", testutils.Problems{}},
+		}
+
+		for _, test := range tests {
+			t.Run(test.testName, func(t *testing.T) {
+				// Batch methods are standard methods according to AIP-130, as such they should not
+				// be considered for this lint rule.  These tests are setting up request and response
+				// models that should fail this linter if it were to run
+				file := testutils.ParseProto3Tmpl(t, `
+				package test;
+
+				service Library {
+					rpc {{.MethodName}}(DummyRequest) returns (DummyResponse) {};
+				}
+
+				message DummyRequest {}
+				message DummyResponse {}
+				`, test)
+				method := file.GetServices()[0].GetMethods()[0]
+				problems := responseMessageName.Lint(file)
+				if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" {
+					t.Error(diff)
+				}
+			})
+		}
+	})
 }

rules/internal/utils/method.go

@@ -29,6 +29,7 @@ var (
 	deleteMethodRegexp               = regexp.MustCompile("^Delete(?:[A-Z]|$)")
 	deleteRevisionMethodRegexp       = regexp.MustCompile("^Delete[A-Za-z0-9]*Revision$")
 	legacyListRevisionsURINameRegexp = regexp.MustCompile(`:listRevisions$`)
+	standardMethodRegexp             = regexp.MustCompile("^(Batch(Get|Create|Update|Delete))|(Get|Create|Update|Delete|List)(?:[A-Z]|$)")
 )
 
 // IsCreateMethod returns true if this is a AIP-133 Create method.
@@ -114,3 +115,13 @@ func GetListResourceMessage(m *desc.MethodDescriptor) *desc.MessageDescriptor {
 func IsStreaming(m *desc.MethodDescriptor) bool {
 	return m.IsClientStreaming() || m.IsServerStreaming()
 }
+
+// IsStandardMethod returns true if this is a AIP-130 Standard Method
+func IsStandardMethod(m *desc.MethodDescriptor) bool {
+	return standardMethodRegexp.MatchString(m.GetName())
+}
+
+// IsCustomMethod returns true if this is a AIP-130 Custom Method
+func IsCustomMethod(m *desc.MethodDescriptor) bool {
+	return !IsStandardMethod(m)
+}

rules/internal/utils/method_test.go

@@ -318,3 +318,70 @@ func TestGetListResourceMessage(t *testing.T) {
 		})
 	}
 }
+
+func TestIsStandardMethod(t *testing.T) {
+	for _, test := range []struct {
+		name           string
+		RPCs           string
+		wantIsStandard bool
+	}{
+		{"ValidCreate", `
+			rpc CreateBook(Book) returns (Book) {};
+		`, true},
+		{"ValidUpdate", `
+			rpc UpdateBook(Book) returns (Book) {};
+		`, true},
+		{"ValidGet", `
+			rpc GetBook(Book) returns (Book) {};
+		`, true},
+		{"ValidDelete", `
+			rpc DeleteBook(Book) returns (Book) {};
+		`, true},
+		{"ValidList", `
+			rpc ListBooks(Book) returns (Book) {};
+		`, true},
+		{"ValidBatchCreate", `
+			rpc BatchCreateBooks(Book) returns (Book) {};
+		`, true},
+		{"ValidBatchUpdate", `
+			rpc BatchUpdateBooks(Book) returns (Book) {};
+		`, true},
+		{"ValidBatchGet", `
+			rpc BatchGetBooks(Book) returns (Book) {};
+		`, true},
+		{"ValidBatchDelete", `
+			rpc BatchDeleteBooks(Book) returns (Book) {};
+		`, true},
+		{"InvalidArchive", `
+			rpc ArchiveBook(Book) returns (Book) {};
+		`, false},
+		{"InvalidSort", `
+			rpc SortBooks(Book) returns (Book) {};
+		`, false},
+		{"InvalidTranslate", `
+			rpc TranslateText(Book) returns (Book) {};
+		`, false},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			file := testutils.ParseProto3Tmpl(t, `
+				service Foo {
+					{{.RPCs}}
+				}
+
+				// This is the request and response, which is irrelevant for
+				// asserting if the rpc is a standard method or not. We just
+				// check the naming of the rpc against a regex
+				message Book {}
+			`, test)
+			method := file.GetServices()[0].GetMethods()[0]
+			gotIsStandard := IsStandardMethod(method)
+			gotIsCustom := IsCustomMethod(method)
+			if gotIsStandard != test.wantIsStandard {
+				t.Errorf("IsStandardMethod got %v, want %v", gotIsStandard, test.wantIsStandard)
+			}
+			if gotIsCustom != !test.wantIsStandard {
+				t.Errorf("IsCustomMethod got %v, want %v", gotIsCustom, !test.wantIsStandard)
+			}
+		})
+	}
+}