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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cec8a94
fix: handle unresolvable lro response types AIP-133 and AIP-233
noahdietz Jun 8, 2022
97c01c4
Merge branch 'main' into fix-unresolvable
noahdietz Jun 8, 2022
8f5f82f
Merge branch 'main' into fix-unresolvable
noahdietz Jun 8, 2022
59ee2e9
address feedback
noahdietz Jun 13, 2022
8875c0a
Merge branch 'fix-unresolvable' of https://github.com/noahdietz/api-l…
noahdietz Jun 13, 2022
7e1c957
address feedback
noahdietz Jun 16, 2022
4110513
address feedback
noahdietz Jun 16, 2022
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
12 changes: 9 additions & 3 deletions rules/aip0133/resource_reference_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,20 @@ import (
var resourceReferenceType = &lint.MethodRule{
Name: lint.NewRuleName(133, "resource-reference-type"),
OnlyIf: func(m *desc.MethodDescriptor) bool {
out := m.GetOutputType()
noahdietz marked this conversation as resolved.
Show resolved Hide resolved
if out.GetName() == "Operation" {
out = utils.GetResponseType(m)
}

// Unresolvable response_type for an Operation results in nil here.
resource := utils.GetResource(out)
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())
out = utils.GetResponseType(m)
}
resource := utils.GetResource(out)
parent := m.GetInputType().FindFieldByName("parent")
Expand Down
22 changes: 12 additions & 10 deletions rules/aip0133/resource_reference_type_test.go
View file
Open in desktop
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
12 changes: 5 additions & 7 deletions rules/aip0233/resource_reference_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ var resourceReferenceType = &lint.MethodRule{
OnlyIf: func(m *desc.MethodDescriptor) bool {
out := m.GetOutputType()
if out.GetName() == "Operation" {
info := utils.GetOperationInfo(m)
out = utils.FindMessage(m.GetFile(), info.GetResponseType())
out = utils.GetResponseType(m)
}

// First repeated message field must be annotated with google.api.resource.
Expand All @@ -44,12 +43,11 @@ 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())
out := m.GetOutputType()
if out.GetName() == "Operation" {
out = utils.GetResponseType(m)
}
repeated := utils.GetRepeatedMessageFields(msg)
repeated := utils.GetRepeatedMessageFields(out)
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
Open in desktop
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
Open in desktop
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
78 changes: 78 additions & 0 deletions rules/internal/utils/extension_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,84 @@ func TestGetOperationInfoNone(t *testing.T) {
}
}

func TestGetOperationInfoResponseType(t *testing.T) {
// Set up testing permutations.
tests := []struct {
testName string
ResponseType string
invalid bool
noahdietz marked this conversation as resolved.
Show resolved Hide resolved
}{
{"Valid", "WriteBookResponse", false},
{"Invalid", "Foo", true},
}
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 (typ == nil) != test.invalid {
noahdietz marked this conversation as resolved.
Show resolved Hide resolved
t.Fatalf("Expected invalid(%v) response_type message", test.invalid)
}
if test.invalid {
return
}
if got, want := typ.GetName(), test.ResponseType; got != want {
t.Errorf("Response type - got %q, want %q.", got, want)
}
})
noahdietz marked this conversation as resolved.
Show resolved Hide resolved
}
}

func TestGetOperationInfoMetadataType(t *testing.T) {
// Set up testing permutations.
tests := []struct {
testName string
MetadataType string
invalid bool
noahdietz marked this conversation as resolved.
Show resolved Hide resolved
}{
{"Valid", "WriteBookMetadata", false},
{"Invalid", "Foo", true},
}
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 (typ == nil) != test.invalid {
noahdietz marked this conversation as resolved.
Show resolved Hide resolved
t.Fatalf("Expected invalid(%v) metadata_type message", test.invalid)
}
if test.invalid {
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
Open in desktop
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