fix(AIP-136): ignore revision methods (#1432)

rules/aip0136/response_message_name_test.go

@@ -31,6 +31,7 @@ func TestResponseMessageName(t *testing.T) {
 		}{
 			{"Valid", "ArchiveBook", "ArchiveBookResponse", testutils.Problems{}},
 			{"Invalid", "ArchiveBook", "ArchiveBookResp", testutils.Problems{{Message: "not \"ArchiveBookResp\"."}}},
+			{"SkipRevisionMethod", "CommitBook", "Book", testutils.Problems{}},
 		}
 
 		for _, test := range tests {

rules/aip0162/aip0162.go

@@ -68,64 +68,40 @@ func AddRules(r lint.RuleRegistry) error {
 }
 
 var (
-	tagRevisionMethodRegexp     = regexp.MustCompile(`^Tag([A-Za-z0-9]+)Revision$`)
 	tagRevisionReqMessageRegexp = regexp.MustCompile(`^Tag(?:[A-Za-z0-9]+)RevisionRequest$`)
 	tagRevisionURINameRegexp    = regexp.MustCompile(`:tagRevision$`)
 )
 
-// Returns true if this is an AIP-162 Tag Revision method, false otherwise.
-func isTagRevisionMethod(m *desc.MethodDescriptor) bool {
-	return tagRevisionMethodRegexp.MatchString(m.GetName())
-}
-
 // Returns true if this is an AIP-162 Tag Revision request message, false otherwise.
 func isTagRevisionRequestMessage(m *desc.MessageDescriptor) bool {
 	return tagRevisionReqMessageRegexp.MatchString(m.GetName())
 }
 
 var (
-	commitMethodRegexp     = regexp.MustCompile(`^Commit([A-Za-z0-9]+)$`)
 	commitReqMessageRegexp = regexp.MustCompile(`^Commit(?:[A-Za-z0-9]+)Request$`)
 	commitURINameRegexp    = regexp.MustCompile(`:commit$`)
 )
 
-// Returns true if this is an AIP-162 Commit method, false otherwise.
-func isCommitMethod(m *desc.MethodDescriptor) bool {
-	return commitMethodRegexp.MatchString(m.GetName())
-}
-
 // Returns true if this is an AIP-162 Commit request message, false otherwise.
 func isCommitRequestMessage(m *desc.MessageDescriptor) bool {
 	return commitReqMessageRegexp.MatchString(m.GetName())
 }
 
 var (
-	rollbackMethodRegexp     = regexp.MustCompile(`^Rollback([A-Za-z0-9]+)$`)
 	rollbackReqMessageRegexp = regexp.MustCompile(`^Rollback(?:[A-Za-z0-9]+)Request$`)
 	rollbackURINameRegexp    = regexp.MustCompile(`:rollback$`)
 )
 
-// Returns true if this is an AIP-162 Rollback method, false otherwise.
-func isRollbackMethod(m *desc.MethodDescriptor) bool {
-	return rollbackMethodRegexp.MatchString(m.GetName())
-}
-
 // Returns true if this is an AIP-162 Rollback request message, false otherwise.
 func isRollbackRequestMessage(m *desc.MessageDescriptor) bool {
 	return rollbackReqMessageRegexp.MatchString(m.GetName())
 }
 
 var (
-	deleteRevisionMethodRegexp     = regexp.MustCompile(`^Delete(?:[A-Za-z0-9]+)Revision$`)
 	deleteRevisionReqMessageRegexp = regexp.MustCompile(`^Delete(?:[A-Za-z0-9]+)RevisionRequest$`)
 	deleteRevisionURINameRegexp    = regexp.MustCompile(`:deleteRevision$`)
 )
 
-// Returns true if this is an AIP-162 Delete Revision method, false otherwise.
-func isDeleteRevisionMethod(m *desc.MethodDescriptor) bool {
-	return deleteRevisionMethodRegexp.MatchString(m.GetName())
-}
-
 // Returns true if this is an AIP-162 Delete Revision request message, false otherwise.
 func isDeleteRevisionRequestMessage(m *desc.MessageDescriptor) bool {
 	return deleteRevisionReqMessageRegexp.MatchString(m.GetName())

rules/aip0162/commit_http_body.go

@@ -22,6 +22,6 @@ import (
 // Commit methods should have "*" as the HTTP body.
 var commitHTTPBody = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "commit-http-body"),
-	OnlyIf:     isCommitMethod,
+	OnlyIf:     utils.IsCommitRevisionMethod,
 	LintMethod: utils.LintWildcardHTTPBody,
 }

rules/aip0162/commit_http_method.go

@@ -22,6 +22,6 @@ import (
 // Commit methods should use the HTTP POST method.
 var commitHTTPMethod = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "commit-http-method"),
-	OnlyIf:     isCommitMethod,
+	OnlyIf:     utils.IsCommitRevisionMethod,
 	LintMethod: utils.LintHTTPMethod("POST"),
 }

rules/aip0162/commit_http_uri_suffix.go

@@ -24,7 +24,7 @@ import (
 // Commit methods should have a proper HTTP pattern.
 var commitHTTPURISuffix = &lint.MethodRule{
 	Name:   lint.NewRuleName(162, "commit-http-uri-suffix"),
-	OnlyIf: isCommitMethod,
+	OnlyIf: utils.IsCommitRevisionMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		for _, httpRule := range utils.GetHTTPRules(m) {
 			if !commitURINameRegexp.MatchString(httpRule.URI) {

rules/aip0162/commit_request_message_name.go

@@ -22,6 +22,6 @@ import (
 // Commit messages should have a properly named request message.
 var commitRequestMessageName = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "commit-request-message-name"),
-	OnlyIf:     isCommitMethod,
+	OnlyIf:     utils.IsCommitRevisionMethod,
 	LintMethod: utils.LintMethodHasMatchingRequestName,
 }

rules/aip0162/commit_response_message_name.go

@@ -25,16 +25,19 @@ import (
 
 var commitResponseMessageName = &lint.MethodRule{
 	Name:   lint.NewRuleName(162, "commit-response-message-name"),
-	OnlyIf: isCommitMethod,
+	OnlyIf: utils.IsCommitRevisionMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Rule check: Establish that for methods such as `CommitBook`, the response
 		// message is `Book`.
+		want, ok := utils.ExtractRevisionResource(m)
+		if !ok {
+			return nil
+		}
 		response := utils.GetResponseType(m)
 		if response == nil {
 			return nil
 		}
 		got := response.GetName()
-		want := commitMethodRegexp.FindStringSubmatch(m.GetName())[1]
 		loc := locations.MethodResponseType(m)
 		suggestion := want
 

rules/aip0162/delete_revision_http_body.go

@@ -22,6 +22,6 @@ import (
 // Delete Revision methods should have no HTTP body.
 var deleteRevisionHTTPBody = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "delete-revision-http-body"),
-	OnlyIf:     isDeleteRevisionMethod,
+	OnlyIf:     utils.IsDeleteRevisionMethod,
 	LintMethod: utils.LintNoHTTPBody,
 }

rules/aip0162/delete_revision_http_method.go

@@ -22,6 +22,6 @@ import (
 // Delete Revision methods should use the HTTP DELETE method.
 var deleteRevisionHTTPMethod = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "delete-revision-http-method"),
-	OnlyIf:     isDeleteRevisionMethod,
+	OnlyIf:     utils.IsDeleteRevisionMethod,
 	LintMethod: utils.LintHTTPMethod("DELETE"),
 }

rules/aip0162/delete_revision_http_uri_suffix.go

@@ -24,7 +24,7 @@ import (
 // Delete Revision methods should have a proper HTTP pattern.
 var deleteRevisionHTTPURISuffix = &lint.MethodRule{
 	Name:   lint.NewRuleName(162, "delete-revision-http-uri-suffix"),
-	OnlyIf: isDeleteRevisionMethod,
+	OnlyIf: utils.IsDeleteRevisionMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		for _, httpRule := range utils.GetHTTPRules(m) {
 			if !deleteRevisionURINameRegexp.MatchString(httpRule.URI) {

rules/aip0162/delete_revision_request_message_name.go

@@ -22,6 +22,6 @@ import (
 // Delete Revision messages should have a properly named request message.
 var deleteRevisionRequestMessageName = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "delete-revision-request-message-name"),
-	OnlyIf:     isDeleteRevisionMethod,
+	OnlyIf:     utils.IsDeleteRevisionMethod,
 	LintMethod: utils.LintMethodHasMatchingRequestName,
 }

rules/aip0162/delete_revision_response_message_name.go

@@ -16,7 +16,6 @@ package aip0162
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/googleapis/api-linter/lint"
 	"github.com/googleapis/api-linter/locations"
@@ -27,14 +26,17 @@ import (
 // Delete Revision methods should return the resource itself.
 var deleteRevisionResponseMessageName = &lint.MethodRule{
 	Name:   lint.NewRuleName(162, "delete-revision-response-message-name"),
-	OnlyIf: isDeleteRevisionMethod,
+	OnlyIf: utils.IsDeleteRevisionMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
+		want, ok := utils.ExtractRevisionResource(m)
+		if !ok {
+			return nil
+		}
 		response := utils.GetResponseType(m)
 		if response == nil {
 			return nil
 		}
 		got := response.GetName()
-		want := strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Revision"), "Delete")
 
 		loc := locations.MethodResponseType(m)
 		suggestion := want

rules/aip0162/rollback_http_body.go

@@ -22,6 +22,6 @@ import (
 // Rollback methods should have "*" as the HTTP body.
 var rollbackHTTPBody = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "rollback-http-body"),
-	OnlyIf:     isRollbackMethod,
+	OnlyIf:     utils.IsRollbackRevisionMethod,
 	LintMethod: utils.LintWildcardHTTPBody,
 }

rules/aip0162/rollback_http_method.go

@@ -22,6 +22,6 @@ import (
 // Rollback methods should use the HTTP POST method.
 var rollbackHTTPMethod = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "rollback-http-method"),
-	OnlyIf:     isRollbackMethod,
+	OnlyIf:     utils.IsRollbackRevisionMethod,
 	LintMethod: utils.LintHTTPMethod("POST"),
 }

rules/aip0162/rollback_http_uri_suffix.go

@@ -24,7 +24,7 @@ import (
 // Rollback methods should have a proper HTTP pattern.
 var rollbackHTTPURISuffix = &lint.MethodRule{
 	Name:   lint.NewRuleName(162, "rollback-http-uri-suffix"),
-	OnlyIf: isRollbackMethod,
+	OnlyIf: utils.IsRollbackRevisionMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		for _, httpRule := range utils.GetHTTPRules(m) {
 			if !rollbackURINameRegexp.MatchString(httpRule.URI) {

rules/aip0162/rollback_request_message_name.go

@@ -22,6 +22,6 @@ import (
 // Rollback messages should have a properly named request message.
 var rollbackRequestMessageName = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "rollback-request-message-name"),
-	OnlyIf:     isRollbackMethod,
+	OnlyIf:     utils.IsRollbackRevisionMethod,
 	LintMethod: utils.LintMethodHasMatchingRequestName,
 }

rules/aip0162/rollback_response_message_name.go

@@ -25,11 +25,14 @@ import (
 
 var rollbackResponseMessageName = &lint.MethodRule{
 	Name:   lint.NewRuleName(162, "rollback-response-message-name"),
-	OnlyIf: isRollbackMethod,
+	OnlyIf: utils.IsRollbackRevisionMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Rule check: Establish that for methods such as `RollbackBook`, the response
 		// message is `Book`.
-		want := rollbackMethodRegexp.FindStringSubmatch(m.GetName())[1]
+		want, ok := utils.ExtractRevisionResource(m)
+		if !ok {
+			return nil
+		}
 		response := utils.GetResponseType(m)
 		if response == nil {
 			return nil

rules/aip0162/tag_revision_http_body.go

@@ -22,6 +22,6 @@ import (
 // Tag Revision methods should have "*" as the HTTP body.
 var tagRevisionHTTPBody = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "tag-revision-http-body"),
-	OnlyIf:     isTagRevisionMethod,
+	OnlyIf:     utils.IsTagRevisionMethod,
 	LintMethod: utils.LintWildcardHTTPBody,
 }

rules/aip0162/tag_revision_http_method.go

@@ -22,6 +22,6 @@ import (
 // Tag Revision methods should use the HTTP POST method.
 var tagRevisionHTTPMethod = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "tag-revision-http-method"),
-	OnlyIf:     isTagRevisionMethod,
+	OnlyIf:     utils.IsTagRevisionMethod,
 	LintMethod: utils.LintHTTPMethod("POST"),
 }

rules/aip0162/tag_revision_http_uri_suffix.go

@@ -24,7 +24,7 @@ import (
 // Tag Revision methods should have a proper HTTP pattern.
 var tagRevisionHTTPURISuffix = &lint.MethodRule{
 	Name:   lint.NewRuleName(162, "tag-revision-http-uri-suffix"),
-	OnlyIf: isTagRevisionMethod,
+	OnlyIf: utils.IsTagRevisionMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		for _, httpRule := range utils.GetHTTPRules(m) {
 			if !tagRevisionURINameRegexp.MatchString(httpRule.URI) {

rules/aip0162/tag_revision_request_message_name.go

@@ -22,6 +22,6 @@ import (
 // Tag Revision messages should have a properly named request message.
 var tagRevisionRequestMessageName = &lint.MethodRule{
 	Name:       lint.NewRuleName(162, "tag-revision-request-message-name"),
-	OnlyIf:     isTagRevisionMethod,
+	OnlyIf:     utils.IsTagRevisionMethod,
 	LintMethod: utils.LintMethodHasMatchingRequestName,
 }

rules/aip0162/tag_revision_response_message_name.go

@@ -25,11 +25,14 @@ import (
 
 var tagRevisionResponseMessageName = &lint.MethodRule{
 	Name:   lint.NewRuleName(162, "tag-revision-response-message-name"),
-	OnlyIf: isTagRevisionMethod,
+	OnlyIf: utils.IsTagRevisionMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Rule check: Establish that for methods such as `TagBookRevision`, the response
 		// message is `Book`.
-		want := tagRevisionMethodRegexp.FindStringSubmatch(m.GetName())[1]
+		want, ok := utils.ExtractRevisionResource(m)
+		if !ok {
+			return nil
+		}
 		response := utils.GetResponseType(m)
 		if response == nil {
 			return nil

rules/internal/utils/method.go

@@ -21,15 +21,20 @@ import (
 )
 
 var (
-	createMethodRegexp               = regexp.MustCompile("^Create(?:[A-Z]|$)")
-	getMethodRegexp                  = regexp.MustCompile("^Get(?:[A-Z]|$)")
-	listMethodRegexp                 = regexp.MustCompile("^List(?:[A-Z]|$)")
+	createMethodRegexp   = regexp.MustCompile("^Create(?:[A-Z]|$)")
+	getMethodRegexp      = regexp.MustCompile("^Get(?:[A-Z]|$)")
+	listMethodRegexp     = regexp.MustCompile("^List(?:[A-Z]|$)")
+	updateMethodRegexp   = regexp.MustCompile("^Update(?:[A-Z]|$)")
+	deleteMethodRegexp   = regexp.MustCompile("^Delete(?:[A-Z]|$)")
+	standardMethodRegexp = regexp.MustCompile("^(Batch(Get|Create|Update|Delete))|(Get|Create|Update|Delete|List)(?:[A-Z]|$)")
+
+	// AIP-162 Resource revision methods
 	listRevisionsMethodRegexp        = regexp.MustCompile(`^List(?:[A-Za-z0-9]+)Revisions$`)
-	updateMethodRegexp               = regexp.MustCompile("^Update(?:[A-Z]|$)")
-	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]|$)")
+	commitRevisionMethodRegexp       = regexp.MustCompile(`^Commit([A-Za-z0-9]+)$`)
+	deleteRevisionMethodRegexp       = regexp.MustCompile(`^Delete([A-Za-z0-9]+)Revision$`)
+	rollbackRevisionMethodRegexp     = regexp.MustCompile(`^Rollback([A-Za-z0-9]+)$`)
+	tagRevisionMethodRegexp          = regexp.MustCompile(`^Tag([A-Za-z0-9]+)Revision$`)
 )
 
 // IsCreateMethod returns true if this is a AIP-133 Create method.
@@ -121,7 +126,65 @@ func IsStandardMethod(m *desc.MethodDescriptor) bool {
 	return standardMethodRegexp.MatchString(m.GetName())
 }
 
-// IsCustomMethod returns true if this is a AIP-130 Custom Method
+// IsCustomMethod returns true if this is a AIP-136 Custom Method
 func IsCustomMethod(m *desc.MethodDescriptor) bool {
-	return !IsStandardMethod(m)
+	return !IsStandardMethod(m) && !isRevisionMethod(m)
+}
+
+// isRevisionMethod returns true if the given method is any of the documented
+// Revision methods. At the moment, this is only relevant for excluding revision
+// methods from other method type checks.
+func isRevisionMethod(m *desc.MethodDescriptor) bool {
+	return IsDeleteRevisionMethod(m) ||
+		IsTagRevisionMethod(m) ||
+		IsCommitRevisionMethod(m) ||
+		IsRollbackRevisionMethod(m)
+}
+
+// IsDeleteRevisionMethod returns true if this is an AIP-162 Delete Revision
+// method, false otherwise.
+func IsDeleteRevisionMethod(m *desc.MethodDescriptor) bool {
+	return deleteRevisionMethodRegexp.MatchString(m.GetName())
+}
+
+// IsTagRevisionMethod returns true if this is an AIP-162 Tag Revision method,
+// false otherwise.
+func IsTagRevisionMethod(m *desc.MethodDescriptor) bool {
+	return tagRevisionMethodRegexp.MatchString(m.GetName())
+}
+
+// IsCommitRevisionMethod returns true if this is an AIP-162 Commit method,
+// false otherwise.
+func IsCommitRevisionMethod(m *desc.MethodDescriptor) bool {
+	return commitRevisionMethodRegexp.MatchString(m.GetName())
+}
+
+// IsRollbackRevisionMethod returns true if this is an AIP-162 Rollback method,
+// false otherwise.
+func IsRollbackRevisionMethod(m *desc.MethodDescriptor) bool {
+	return rollbackRevisionMethodRegexp.MatchString(m.GetName())
+}
+
+// ExtractRevisionResource uses the appropriate revision method regular
+// expression to capture and extract the resource noun in the method name.
+// If the given method is not one of the standard revision RPCs, it returns
+// empty string and false.
+func ExtractRevisionResource(m *desc.MethodDescriptor) (string, bool) {
+	if !isRevisionMethod(m) {
+		return "", false
+	}
+
+	n := m.GetName()
+
+	if IsCommitRevisionMethod(m) {
+		return commitRevisionMethodRegexp.FindStringSubmatch(n)[1], true
+	} else if IsTagRevisionMethod(m) {
+		return tagRevisionMethodRegexp.FindStringSubmatch(n)[1], true
+	} else if IsRollbackRevisionMethod(m) {
+		return rollbackRevisionMethodRegexp.FindStringSubmatch(n)[1], true
+	} else if IsDeleteRevisionMethod(m) {
+		return deleteRevisionMethodRegexp.FindStringSubmatch(n)[1], true
+	}
+
+	return "", false
 }