fix(AIP 133-135): fix false positive in 133-135 (#1335)

andrei-scripniciuc · noahdietz · web-flow · commit d79af9cc8595 · 2024-02-16T16:34:39.000Z
* fix(AIP 133-135): fix false positive in 133-135

* chore: add new line at EoF

Co-authored-by: Noah Dietz &lt;noahdietz@users.noreply.github.com&gt;

---------

Co-authored-by: Noah Dietz &lt;noahdietz@users.noreply.github.com&gt;
diff --git a/rules/aip0133/aip0133.go b/rules/aip0133/aip0133.go
@@ -49,17 +49,6 @@ func AddRules(r lint.RuleRegistry) error {
 	)
 }
 
-// get resource message type name from method
-func getResourceMsgName(m *desc.MethodDescriptor) string {
-	// Usually the response message will be the resource message, and its name will
-	// be part of method name (make a double check here to avoid the issue when
-	// method or output naming doesn't follow the right principles)
-	if strings.Contains(m.GetName()[6:], m.GetOutputType().GetName()) {
-		return m.GetOutputType().GetName()
-	}
-	return m.GetName()[6:]
-}
-
 // get resource message type name from request message
 func getResourceMsgNameFromReq(m *desc.MessageDescriptor) string {
 	// retrieve the string between the prefix "Create" and suffix "Request" from
diff --git a/rules/aip0133/http_body.go b/rules/aip0133/http_body.go
@@ -29,7 +29,7 @@ var httpBody = &lint.MethodRule{
 	Name:   lint.NewRuleName(133, "http-body"),
 	OnlyIf: utils.IsCreateMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		resourceMsgName := getResourceMsgName(m)
+		resourceMsgName := utils.GetResourceMessageName(m, "Create")
 		resourceFieldName := strings.ToLower(resourceMsgName)
 		for _, fieldDesc := range m.GetInputType().GetFields() {
 			// when msgDesc is nil, the resource field in the request message is
diff --git a/rules/aip0133/method_signature.go b/rules/aip0133/method_signature.go
@@ -34,7 +34,7 @@ var methodSignature = &lint.MethodRule{
 
 		// Determine what signature we want. The {resource}_id is desired
 		// if and only if the field exists on the request.
-		resourceField := strcase.SnakeCase(getResourceMsgName(m))
+		resourceField := strcase.SnakeCase(utils.GetResourceMessageName(m, "Create"))
 		want := []string{}
 		if !hasNoParent(m.GetOutputType()) {
 			want = append(want, "parent")
diff --git a/rules/aip0133/method_signature_test.go b/rules/aip0133/method_signature_test.go
@@ -30,6 +30,7 @@ func TestMethodSignature(t *testing.T) {
 	}{
 		{"ValidNoID", "CreateBook", `option (google.api.method_signature) = "parent,book";`, "", testutils.Problems{}},
 		{"ValidID", "CreateBook", `option (google.api.method_signature) = "parent,book,book_id";`, "string book_id = 3;", testutils.Problems{}},
+		{"ValidOperation", "CreateUnitOperation", `option (google.api.method_signature) = "parent,unit_operation,unit_operation_id";`, "string unit_operation_id = 3;", testutils.Problems{}},
 		{"MissingNoID", "CreateBook", "", "", testutils.Problems{{Message: `(google.api.method_signature) = "parent,book"`}}},
 		{
 			"MissingID",
diff --git a/rules/aip0133/response_lro.go b/rules/aip0133/response_lro.go
@@ -27,7 +27,7 @@ var responseLRO = &lint.MethodRule{
 		return utils.IsCreateMethod(m) && utils.IsDeclarativeFriendlyMethod(m)
 	},
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		if m.GetOutputType().GetFullyQualifiedName() != "google.longrunning.Operation" {
+		if !utils.IsOperation(m.GetOutputType()) {
 			return []lint.Problem{{
 				Message:    "Declarative-friendly create methods should use an LRO.",
 				Descriptor: m,
diff --git a/rules/aip0133/response_message_name.go b/rules/aip0133/response_message_name.go
@@ -28,7 +28,7 @@ var outputName = &lint.MethodRule{
 	Name:   lint.NewRuleName(133, "response-message-name"),
 	OnlyIf: utils.IsCreateMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		want := getResourceMsgName(m)
+		want := utils.GetResourceMessageName(m, "Create")
 
 		// If this is an LRO, then use the annotated response type instead of
 		// the actual RPC return type.
diff --git a/rules/aip0133/response_message_name_test.go b/rules/aip0133/response_message_name_test.go
@@ -31,6 +31,7 @@ func TestOutputMessageName(t *testing.T) {
 	}{
 		{"ValidResource", "CreateBook", "Book", false, testutils.Problems{}},
 		{"ValidLRO", "CreateBook", "Book", true, testutils.Problems{}},
+		{"ResourceNameContainsOperation", "CreateUnitOperation", "UnitOperation", true, testutils.Problems{}},
 		{"Invalid", "CreateBook", "CreateBookResponse", false, testutils.Problems{{Suggestion: "Book"}}},
 		{"InvalidLRO", "CreateBook", "CreateBookResponse", true, testutils.Problems{{Suggestion: "Book"}}},
 		{"Irrelevant", "BuildBook", "BuildBookResponse", false, testutils.Problems{}},
diff --git a/rules/aip0134/response_lro.go b/rules/aip0134/response_lro.go
@@ -27,7 +27,7 @@ var responseLRO = &lint.MethodRule{
 		return utils.IsUpdateMethod(m) && utils.IsDeclarativeFriendlyMethod(m)
 	},
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		if m.GetOutputType().GetFullyQualifiedName() != "google.longrunning.Operation" {
+		if !utils.IsOperation(m.GetOutputType()) {
 			return []lint.Problem{{
 				Message:    "Declarative-friendly update methods should use an LRO.",
 				Descriptor: m,
diff --git a/rules/aip0134/response_message_name_test.go b/rules/aip0134/response_message_name_test.go
@@ -31,6 +31,7 @@ func TestResponseMessageName(t *testing.T) {
 	}{
 		{"ValidResource", "UpdateBook", "Book", false, testutils.Problems{}},
 		{"ValidLRO", "UpdateBook", "Book", true, testutils.Problems{}},
+		{"ValidLROContainingOperation", "UpdateUnitOperation", "UnitOperation", true, testutils.Problems{}},
 		{"Invalid", "UpdateBook", "UpdateBookResponse", false, testutils.Problems{{Suggestion: "Book"}}},
 		{"InvalidLRO", "UpdateBook", "UpdateBookResponse", true, testutils.Problems{{Suggestion: "Book"}}},
 		{"Irrelevant", "MutateBook", "MutateBookResponse", false, testutils.Problems{}},
diff --git a/rules/aip0135/response_lro.go b/rules/aip0135/response_lro.go
@@ -27,7 +27,7 @@ var responseLRO = &lint.MethodRule{
 		return utils.IsDeleteMethod(m) && utils.IsDeclarativeFriendlyMethod(m)
 	},
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		if m.GetOutputType().GetFullyQualifiedName() != "google.longrunning.Operation" {
+		if !utils.IsOperation(m.GetOutputType()) {
 			return []lint.Problem{{
 				Message:    "Declarative-friendly delete methods should use an LRO.",
 				Descriptor: m,
diff --git a/rules/aip0135/response_message_name_test.go b/rules/aip0135/response_message_name_test.go
@@ -90,6 +90,7 @@ func TestResponseMessageName(t *testing.T) {
 		// the declarative friendly style is no longer deviated for delete.
 		{"ValidEmptyDF", "DeleteBook", "google.protobuf.Empty", "style: DECLARATIVE_FRIENDLY", problems["none"]},
 		{"ValidResource", "DeleteBook", "Book", "", problems["none"]},
+		{"ValidResourceOperation", "DeleteUnitOperation", "UnitOperation", "", problems["none"]},
 		{"Invalid", "DeleteBook", "DeleteBookResponse", "", problems["empty"]},
 		{"Irrelevant", "DestroyBook", "DestroyBookResponse", "", problems["none"]},
 	}
diff --git a/rules/internal/utils/type_name.go b/rules/internal/utils/type_name.go
@@ -40,3 +40,19 @@ func GetTypeName(f *desc.FieldDescriptor) string {
 func IsOperation(m *desc.MessageDescriptor) bool {
 	return m.GetFullyQualifiedName() == "google.longrunning.Operation"
 }
+
+// GetResourceMessageName returns the resource message type name from method
+func GetResourceMessageName(m *desc.MethodDescriptor, expectedVerb string) string {
+	if !strings.HasPrefix(m.GetName(), expectedVerb) {
+		return ""
+	}
+
+	// Usually the response message will be the resource message, and its name will
+	// be part of method name (make a double check here to avoid the issue when
+	// method or output naming doesn't follow the right principles)
+	// Ignore this rule if the return type is an LRO
+	if strings.Contains(m.GetName()[len(expectedVerb):], m.GetOutputType().GetName()) && !IsOperation(m.GetOutputType()) {
+		return m.GetOutputType().GetName()
+	}
+	return m.GetName()[len(expectedVerb):]
+}

Original file line number	Diff line number	Diff line change
`@@ -30,6 +30,7 @@ func TestMethodSignature(t *testing.T) {`
`30`	`30`	`}{`
`31`	`31`	{"ValidNoID", "CreateBook", `option (google.api.method_signature) = "parent,book";`, "", testutils.Problems{}},
`32`	`32`	{"ValidID", "CreateBook", `option (google.api.method_signature) = "parent,book,book_id";`, "string book_id = 3;", testutils.Problems{}},
	`33`	+ {"ValidOperation", "CreateUnitOperation", `option (google.api.method_signature) = "parent,unit_operation,unit_operation_id";`, "string unit_operation_id = 3;", testutils.Problems{}},
`33`	`34`	{"MissingNoID", "CreateBook", "", "", testutils.Problems{{Message: `(google.api.method_signature) = "parent,book"`}}},
`34`	`35`	`{`
`35`	`36`	`"MissingID",`