Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle unresolvable lro response types AIP-133 and AIP-233 #980

Merged
merged 7 commits into from
Jun 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions rules/aip0133/resource_reference_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,24 @@ import (
var resourceReferenceType = &lint.MethodRule{
Name: lint.NewRuleName(133, "resource-reference-type"),
OnlyIf: func(m *desc.MethodDescriptor) bool {
// Return type of the RPC.
ot := m.GetOutputType()
if ot.GetName() == "Operation" {
ot = utils.GetResponseType(m)
}

// Unresolvable response_type for an Operation results in nil here.
resource := utils.GetResource(ot)
p := m.GetInputType().FindFieldByName("parent")
return isCreateMethod(m) && p != nil && utils.GetResourceReference(p) != nil
return isCreateMethod(m) && p != nil && utils.GetResourceReference(p) != nil && resource != nil
},
LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
out := m.GetOutputType()
if out.GetName() == "Operation" {
info := utils.GetOperationInfo(m)
out = utils.FindMessage(m.GetFile(), info.GetResponseType())
// Return type of the RPC.
ot := m.GetOutputType()
if ot.GetName() == "Operation" {
ot = utils.GetResponseType(m)
}
resource := utils.GetResource(out)
resource := utils.GetResource(ot)
parent := m.GetInputType().FindFieldByName("parent")
ref := utils.GetResourceReference(parent)

Expand Down
22 changes: 12 additions & 10 deletions rules/aip0133/resource_reference_type_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,16 +66,18 @@ func TestResourceReferenceType(t *testing.T) {
func TestResourceReferenceTypeLRO(t *testing.T) {
// Set up testing permutations.
tests := []struct {
testName string
TypeName string
RefType string
problems testutils.Problems
testName string
TypeName string
RefType string
ResponseType string
problems testutils.Problems
}{
{"ValidChildType", "library.googleapis.com/Book", "child_type", nil},
{"ValidChildTypeLRO", "library.googleapis.com/Book", "child_type", nil},
{"ValidType", "library.googleapis.com/Shelf", "type", nil},
{"InvalidType", "library.googleapis.com/Book", "type", testutils.Problems{{Message: "not a `type`"}}},
{"InvalidChildType", "library.googleapis.com/Shelf", "child_type", testutils.Problems{{Message: "`child_type`"}}},
{"ValidChildType", "library.googleapis.com/Book", "child_type", "Book", nil},
{"ValidChildTypeLRO", "library.googleapis.com/Book", "child_type", "Book", nil},
{"ValidType", "library.googleapis.com/Shelf", "type", "Book", nil},
{"InvalidType", "library.googleapis.com/Book", "type", "Book", testutils.Problems{{Message: "not a `type`"}}},
{"InvalidChildType", "library.googleapis.com/Shelf", "child_type", "Book", testutils.Problems{{Message: "`child_type`"}}},
{"SkipInvalidUnresolvableResponseType", "library.googleapis.com/Shelf", "child_type", "Foo", nil},
}

// Run each test.
Expand All @@ -87,7 +89,7 @@ func TestResourceReferenceTypeLRO(t *testing.T) {
service Library {
rpc CreateBook(CreateBookRequest) returns (google.longrunning.Operation) {
option (google.longrunning.operation_info) = {
response_type: "Book"
response_type: "{{ .ResponseType }}"
metadata_type: "Book"
};
}
Expand Down
20 changes: 10 additions & 10 deletions rules/aip0233/resource_reference_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@ import (
var resourceReferenceType = &lint.MethodRule{
Name: lint.NewRuleName(233, "resource-reference-type"),
OnlyIf: func(m *desc.MethodDescriptor) bool {
out := m.GetOutputType()
if out.GetName() == "Operation" {
info := utils.GetOperationInfo(m)
out = utils.FindMessage(m.GetFile(), info.GetResponseType())
// Return type of the RPC.
ot := m.GetOutputType()
if ot.GetName() == "Operation" {
ot = utils.GetResponseType(m)
}

// First repeated message field must be annotated with google.api.resource.
repeated := utils.GetRepeatedMessageFields(out)
repeated := utils.GetRepeatedMessageFields(ot)
if len(repeated) == 0 {
return false
}
Expand All @@ -44,12 +44,12 @@ var resourceReferenceType = &lint.MethodRule{
return isBatchCreateMethod(m) && parent != nil && utils.GetResourceReference(parent) != nil && resource != nil
},
LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
msg := m.GetOutputType()
if msg.GetName() == "Operation" {
info := utils.GetOperationInfo(m)
msg = utils.FindMessage(m.GetFile(), info.GetResponseType())
// Return type of the RPC.
ot := m.GetOutputType()
if ot.GetName() == "Operation" {
ot = utils.GetResponseType(m)
}
repeated := utils.GetRepeatedMessageFields(msg)
repeated := utils.GetRepeatedMessageFields(ot)
resMsg := repeated[0].GetMessageType()

resource := utils.GetResource(resMsg)
Expand Down
20 changes: 11 additions & 9 deletions rules/aip0233/resource_reference_type_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,17 @@ func TestResourceReferenceType(t *testing.T) {
func TestResourceReferenceTypeLRO(t *testing.T) {
// Set up testing permutations.
tests := []struct {
testName string
TypeName string
RefType string
problems testutils.Problems
testName string
TypeName string
RefType string
ResponseType string
problems testutils.Problems
}{
{"ValidChildType", "library.googleapis.com/Book", "child_type", nil},
{"ValidType", "library.googleapis.com/Shelf", "type", nil},
{"InvalidType", "library.googleapis.com/Book", "type", testutils.Problems{{Message: "not a `type`"}}},
{"InvalidChildType", "library.googleapis.com/Shelf", "child_type", testutils.Problems{{Message: "child_type"}}},
{"ValidChildType", "library.googleapis.com/Book", "child_type", "BatchCreateBooksResponse", nil},
{"ValidType", "library.googleapis.com/Shelf", "type", "BatchCreateBooksResponse", nil},
{"InvalidType", "library.googleapis.com/Book", "type", "BatchCreateBooksResponse", testutils.Problems{{Message: "not a `type`"}}},
{"InvalidChildType", "library.googleapis.com/Shelf", "child_type", "BatchCreateBooksResponse", testutils.Problems{{Message: "child_type"}}},
{"SkipInvalidUnresolvableResponseType", "library.googleapis.com/Shelf", "child_type", "Foo", nil},
}

// Run each test.
Expand All @@ -94,7 +96,7 @@ func TestResourceReferenceTypeLRO(t *testing.T) {
service Library {
rpc BatchCreateBooks(BatchCreateBooksRequest) returns (google.longrunning.Operation) {
option (google.longrunning.operation_info) = {
response_type: "BatchCreateBooksResponse"
response_type: "{{ .ResponseType }}"
metadata_type: "BatchCreateBooksResponse"
};
}
Expand Down
33 changes: 33 additions & 0 deletions rules/internal/utils/extension.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,46 @@ func GetFieldBehavior(f *desc.FieldDescriptor) stringset.Set {

// GetOperationInfo returns the google.longrunning.operation_info annotation.
func GetOperationInfo(m *desc.MethodDescriptor) *lrpb.OperationInfo {
if m == nil {
return nil
}
opts := m.GetMethodOptions()
if x := proto.GetExtension(opts, lrpb.E_OperationInfo); x != nil {
return x.(*lrpb.OperationInfo)
}
return nil
}

// GetResponseType returns the message referred to by the
// (google.longrunning.operation_info).response_type annotation.
func GetResponseType(m *desc.MethodDescriptor) *desc.MessageDescriptor {
if m == nil {
return nil
}
info := GetOperationInfo(m)
if info == nil {
return nil
}
typ := FindMessage(m.GetFile(), info.GetResponseType())

return typ
}

// GetMetadataType returns the message referred to by the
// (google.longrunning.operation_info).metadata_type annotation.
func GetMetadataType(m *desc.MethodDescriptor) *desc.MessageDescriptor {
if m == nil {
return nil
}
info := GetOperationInfo(m)
if info == nil {
return nil
}
typ := FindMessage(m.GetFile(), info.GetMetadataType())

return typ
}

// GetMethodSignatures returns the `google.api.method_signature` annotations.
func GetMethodSignatures(m *desc.MethodDescriptor) [][]string {
answer := [][]string{}
Expand Down
86 changes: 86 additions & 0 deletions rules/internal/utils/extension_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,92 @@ func TestGetOperationInfoNone(t *testing.T) {
}
}

func TestGetOperationInfoResponseType(t *testing.T) {
// Set up testing permutations.
tests := []struct {
testName string
ResponseType string
valid bool
}{
{"Valid", "WriteBookResponse", true},
{"Invalid", "Foo", false},
}
for _, test := range tests {
t.Run(test.testName, func(t *testing.T) {
fd := testutils.ParseProto3Tmpl(t, `
import "google/longrunning/operations.proto";
service Library {
rpc WriteBook(WriteBookRequest) returns (google.longrunning.Operation) {
option (google.longrunning.operation_info) = {
response_type: "{{ .ResponseType }}"
metadata_type: "WriteBookMetadata"
};
}
}
message WriteBookRequest {}
message WriteBookResponse {}
`, test)

typ := GetResponseType(fd.GetServices()[0].GetMethods()[0])

if validType := typ != nil; validType != test.valid {
t.Fatalf("Expected valid(%v) response_type message", test.valid)
}

if !test.valid {
return
}

if got, want := typ.GetName(), test.ResponseType; got != want {
t.Errorf("Response type - got %q, want %q.", got, want)
}
})
}
}

func TestGetOperationInfoMetadataType(t *testing.T) {
// Set up testing permutations.
tests := []struct {
testName string
MetadataType string
valid bool
}{
{"Valid", "WriteBookMetadata", true},
{"Invalid", "Foo", false},
}
for _, test := range tests {
t.Run(test.testName, func(t *testing.T) {
fd := testutils.ParseProto3Tmpl(t, `
import "google/longrunning/operations.proto";
service Library {
rpc WriteBook(WriteBookRequest) returns (google.longrunning.Operation) {
option (google.longrunning.operation_info) = {
response_type: "WriteBookResponse"
metadata_type: "{{ .MetadataType }}"
};
}
}
message WriteBookRequest {}
message WriteBookMetadata {}
`, test)

typ := GetMetadataType(fd.GetServices()[0].GetMethods()[0])

if validType := typ != nil; validType != test.valid {
t.Fatalf("Expected valid(%v) metadata_type message", test.valid)
}

if !test.valid {
return
}

if got, want := typ.GetName(), test.MetadataType; got != want {
t.Errorf("Metadata type - got %q, want %q.", got, want)
}
})
}
}

func TestGetResource(t *testing.T) {
t.Run("Present", func(t *testing.T) {
f := testutils.ParseProto3String(t, `
Expand Down
5 changes: 5 additions & 0 deletions rules/internal/utils/find.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@ func (f fieldSorter) Less(i, j int) bool {
func GetRepeatedMessageFields(m *desc.MessageDescriptor) []*desc.FieldDescriptor {
var fields fieldSorter

// If an unresolable message is fed into this helper, return empty slice.
if m == nil {
return fields
}

for _, f := range m.GetFields() {
if f.IsRepeated() && f.GetType() == descriptorpb.FieldDescriptorProto_TYPE_MESSAGE {
fields = append(fields, f)
Expand Down