Skip to content

Commit

Permalink
fix: handle unresolvable lro response types AIP-133 and AIP-233 (#980)
Browse files Browse the repository at this point in the history 
* fix: handle unresolvable lro response types AIP-133 and AIP-233

* address feedback

* address feedback

* address feedback
  • Loading branch information
@noahdietz
noahdietz authored Jun 16, 2022
1 parent cbd521d commit bbb743a
Show file tree
Hide file tree
Showing 7 changed files with 171 additions and 35 deletions.
20 changes: 14 additions & 6 deletions rules/aip0133/resource_reference_type.go
View file
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
View file
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
View file
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
View file
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
View file
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
View file
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
View file
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

0 comments on commit bbb743a

Please sign in to comment.