Skip to content

Commit

Permalink
fix(AIP-136): handle LRO response names (#1391)
Browse files Browse the repository at this point in the history
* fix(AIP-136): handle LRO response names

* split LRO tests out for easier reading
  • Loading branch information
noahdietz authored May 23, 2024
1 parent 4d25ba2 commit ec79f53
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 6 deletions.
7 changes: 6 additions & 1 deletion rules/aip0136/response_message_name.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,15 @@ var responseMessageName = &lint.MethodRule{
return nil
}

loc := locations.MethodResponseType(m)
if utils.IsOperation(m.GetOutputType()) {
loc = locations.MethodOperationInfo(m)
}

return []lint.Problem{{
Message: fmt.Sprintf(responseMessageNameErrorMessage, response.GetName()),
Descriptor: m,
Location: locations.MethodResponseType(m),
Location: loc,
}}

},
Expand Down
42 changes: 42 additions & 0 deletions rules/aip0136/response_message_name_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@ func TestResponseMessageName(t *testing.T) {
file := testutils.ParseProto3Tmpl(t, `
package test;
import "google/api/resource.proto";
service Library {
rpc {{.MethodName}}({{.MethodName}}Request) returns ({{.RespMessageName}});
}
message {{.MethodName}}Request {}
message {{.RespMessageName}} {}
`, test)
Expand All @@ -53,6 +55,46 @@ func TestResponseMessageName(t *testing.T) {
}
})

t.Run("Response Suffix - LRO", func(t *testing.T) {
// Set up the testing permutations.
tests := []struct {
testName string
MethodName string
MessageName string
problems testutils.Problems
}{
{"Valid", "ArchiveBook", "ArchiveBookResponse", testutils.Problems{}},
{"Invalid", "ArchiveBook", "ArchiveBookResp", testutils.Problems{{Message: "not \"ArchiveBookResp\"."}}},
}

for _, test := range tests {
t.Run(test.testName, func(t *testing.T) {
file := testutils.ParseProto3Tmpl(t, `
package test;
import "google/api/resource.proto";
import "google/longrunning/operations.proto";
service Library {
rpc {{.MethodName}}({{.MethodName}}Request) returns (google.longrunning.Operation) {
option (google.longrunning.operation_info) = {
response_type: "{{.MessageName}}"
metadata_type: "OperationMetadata"
};
};
}
message {{.MethodName}}Request {}
message {{.MessageName}} {}
message OperationMetadata {}
`, test)
method := file.GetServices()[0].GetMethods()[0]
problems := responseMessageName.Lint(file)
if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" {
t.Error(diff)
}
})
}
})

t.Run("Resource", func(t *testing.T) {
// Set up the testing permutations.
tests := []struct {
Expand Down
18 changes: 15 additions & 3 deletions rules/internal/utils/common_lints.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,12 +181,24 @@ func LintMethodHasMatchingRequestName(m *desc.MethodDescriptor) []lint.Problem {
// LintMethodHasMatchingResponseName returns a problem if the given method's response type does not
// have a name matching the method's, with a "Response" suffix.
func LintMethodHasMatchingResponseName(m *desc.MethodDescriptor) []lint.Problem {
if got, want := m.GetOutputType().GetName(), m.GetName()+"Response"; got != want {
// GetResponseType handles the LRO case.
if got, want := GetResponseType(m).GetName(), m.GetName()+"Response"; got != want {
loc := locations.MethodResponseType(m)
suggestion := want

// If the RPC is an LRO, we need to tweak the finding.
if isLongRunningOperation(m.GetOutputType()) {
loc = locations.MethodOperationInfo(m)
// Clear the suggestion b.c we cannot easily pin point the
// response_type field.
suggestion = ""
}

return []lint.Problem{{
Message: fmt.Sprintf("Response message should be named after the RPC, i.e. %q.", want),
Suggestion: want,
Suggestion: suggestion,
Descriptor: m,
Location: locations.MethodResponseType(m),
Location: loc,
}}
}
return nil
Expand Down
38 changes: 36 additions & 2 deletions rules/internal/utils/common_lints_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,21 +204,55 @@ func TestLintMethodHasMatchingRequestName(t *testing.T) {
}

func TestLintMethodHasMatchingResponseName(t *testing.T) {
for _, test := range []struct {
testName string
ResponseName string
problems testutils.Problems
}{
{"Valid", "GetBookResponse", nil},
{"Invalid", "AcquireBookResponse", testutils.Problems{{Suggestion: "GetBookResponse"}}},
} {
t.Run(test.testName, func(t *testing.T) {
f := testutils.ParseProto3Tmpl(t, `
service Library {
rpc GetBook(GetBookRequest) returns ({{.ResponseName}});
}
message GetBookRequest {}
message {{.ResponseName}} {}
`, test)
method := f.GetServices()[0].GetMethods()[0]
problems := LintMethodHasMatchingResponseName(method)
if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" {
t.Error(diff)
}
})
}
}

func TestLintMethodHasMatchingResponseNameLRO(t *testing.T) {
for _, test := range []struct {
testName string
MessageName string
problems testutils.Problems
}{
{"Valid", "GetBookResponse", nil},
{"Invalid", "AcquireBookResponse", testutils.Problems{{Suggestion: "GetBookResponse"}}},
{"Invalid", "AcquireBookResponse", testutils.Problems{{Message: "GetBookResponse"}}},
} {
t.Run(test.testName, func(t *testing.T) {
f := testutils.ParseProto3Tmpl(t, `
import "google/longrunning/operations.proto";
service Library {
rpc GetBook(GetBookRequest) returns ({{.MessageName}});
rpc GetBook(GetBookRequest) returns (google.longrunning.Operation) {
option (google.longrunning.operation_info) = {
response_type: "{{.MessageName}}"
metadata_type: "OperationMetadata"
};
}
}
message GetBookRequest {}
message {{.MessageName}} {}
message OperationMetadata {}
`, test)
method := f.GetServices()[0].GetMethods()[0]
problems := LintMethodHasMatchingResponseName(method)
Expand Down

0 comments on commit ec79f53

Please sign in to comment.