From d79af9cc85959ce2f22d2a12f1d2fbfca0fd2e7b Mon Sep 17 00:00:00 2001 From: Andrei Scripniciuc <46186670+andrei-scripniciuc@users.noreply.github.com> Date: Fri, 16 Feb 2024 08:34:39 -0800 Subject: [PATCH] fix(AIP 133-135): fix false positive in 133-135 (#1335) * fix(AIP 133-135): fix false positive in 133-135 * chore: add new line at EoF Co-authored-by: Noah Dietz --------- Co-authored-by: Noah Dietz --- rules/aip0133/aip0133.go | 11 ----------- rules/aip0133/http_body.go | 2 +- rules/aip0133/method_signature.go | 2 +- rules/aip0133/method_signature_test.go | 1 + rules/aip0133/response_lro.go | 2 +- rules/aip0133/response_message_name.go | 2 +- rules/aip0133/response_message_name_test.go | 1 + rules/aip0134/response_lro.go | 2 +- rules/aip0134/response_message_name_test.go | 1 + rules/aip0135/response_lro.go | 2 +- rules/aip0135/response_message_name_test.go | 1 + rules/internal/utils/type_name.go | 16 ++++++++++++++++ 12 files changed, 26 insertions(+), 17 deletions(-) diff --git a/rules/aip0133/aip0133.go b/rules/aip0133/aip0133.go index 0f7956fc2..4a3dbab0e 100644 --- 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 index 5fa0fc284..9907a6dfa 100644 --- 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 index 3e2b0f250..f05ec8669 100644 --- 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 index 5d396dba2..0dd475cea 100644 --- 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 index 39c000c4f..c1ff39325 100644 --- 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 index 6d5a405a1..5dad6e275 100644 --- 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 index 836e82660..c9d39eabc 100644 --- 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 index 5204d27a5..4bc014b35 100644 --- 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 index f28939fe7..141ba8be9 100644 --- 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 index df641eb22..a4bcfac80 100644 --- 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 index 3a38ac29e..0be1840b1 100644 --- 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 index 91064833d..e324edd9f 100644 --- 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):] +}