Skip to content

Commit

Permalink
refactor: Make separate IsDeclarativeFriendly methods for message and…
Browse files Browse the repository at this point in the history 
… method. (#650)

* refactor: Make separate IsDeclarativeFriendly methods for message and method.

Otherwise they can not be sent to OnlyIf.
Additionally, support request messages.

* Incorporate golint feedback.
  • Loading branch information
Luke Sneeringer authored Oct 7, 2020
1 parent c7abce4 commit ddc47f2
Show file tree
Hide file tree
Showing 2 changed files with 126 additions and 68 deletions.
168 changes: 107 additions & 61 deletions rules/internal/utils/declarative_friendly.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,82 +22,104 @@ import (
apb "google.golang.org/genproto/googleapis/api/annotations"
)

// IsDeclarativeFriendly returns true if the descriptor is declarative-friendly.
// IsDeclarativeFriendlyMessage returns true if the descriptor is
// declarative-friendly.
//
// For messages, this means the message is annotated with google.api.resource
// and style: DECLARATIVE_FRIENDLY is set.
//
// For methods, the output message is checked and the same logic applied.
// Additionally, methods beginning with "List" (and "Delete" if returning Empty)
// have special logic applied to try to find the correct message and run the
// check.
func IsDeclarativeFriendly(d desc.Descriptor) bool {
switch m := d.(type) {
case *desc.MessageDescriptor:
// We have a message; get the google.api.resource annotation and
// see if it is styled declarative-friendly.
if resource := GetResource(m); resource != nil {
for _, style := range resource.GetStyle() {
if style == apb.ResourceDescriptor_DECLARATIVE_FRIENDLY {
return true
}
}
}
case *desc.MethodDescriptor:
// Get the return type for the method. If the method is an LRO, then
// get the response type from the operation_info annotation.
response := m.GetOutputType()
if response.GetFullyQualifiedName() == "google.longrunning.Operation" {
if opInfo := GetOperationInfo(m); opInfo != nil {
response = findMessage(m.GetFile(), opInfo.GetResponseType())

// Sanity check: We may not have found the message.
// If that is the case, give up and assume the method is not
// declarative-friendly.
if response == nil {
return false
}
// This is true if:
// - The message is annotated with google.api.resource and
// style: DECLARATIVE_FRIENDLY is set.
// - The message is a standard method request message for a resource with
// google.api.resource and style:DECLARATIVE_FRIENDLY set.
func IsDeclarativeFriendlyMessage(m *desc.MessageDescriptor) bool {
// Get the google.api.resource annotation and see if it is styled
// declarative-friendly.
if resource := GetResource(m); resource != nil {
for _, style := range resource.GetStyle() {
if style == apb.ResourceDescriptor_DECLARATIVE_FRIENDLY {
return true
}
}
}

// If the return value has a google.api.resource annotation, we can
// assume it is the resource and check it.
if IsResource(response) {
return IsDeclarativeFriendly(response)
// If this is a standard method request message, find the corresponding
// resource message. The easiest way to do this is to farm it out to the
// corresponding method.
if n := m.GetName(); strings.HasSuffix(n, "Request") {
if method := findMethod(m.GetFile(), strings.TrimSuffix(n, "Request")); method != nil {
return IsDeclarativeFriendlyMethod(method)
}
}

// We found no evidence that this descriptor is supposed to be
// declarative-friendly. Return false.
return false
}

// IsDeclarativeFriendlyMethod returns true if the method is for a
// declarative-friendly resource.
//
// This is true if:
// - The output message is a declarative-friendly message
// (according to IsDeclarativeFriendlyMessage).
// - The method begins with "List" and the first repeated field is a
// declarative-friendly resource.
// - The method begins with "Delete", the return type is Empty, and an
// appropriate resource message is found and is declarative-friendly.
// - The method is a custom method where a matching resource is found (by
// subset checks on the name) and is declarative-friendly.
func IsDeclarativeFriendlyMethod(m *desc.MethodDescriptor) bool {
// Get the return type for the method. If the method is an LRO, then
// get the response type from the operation_info annotation.
response := m.GetOutputType()
if response.GetFullyQualifiedName() == "google.longrunning.Operation" {
if opInfo := GetOperationInfo(m); opInfo != nil {
response = findMessage(m.GetFile(), opInfo.GetResponseType())

// If the return value is a List response (AIP-132), we should be able
// to find the resource as a field in the response.
if n := response.GetName(); strings.HasPrefix(n, "List") && strings.HasSuffix(n, "Response") {
for _, field := range response.GetFields() {
if field.IsRepeated() && field.GetMessageType() != nil {
return IsDeclarativeFriendly(field.GetMessageType())
}
// Sanity check: We may not have found the message.
// If that is the case, give up and assume the method is not
// declarative-friendly.
if response == nil {
return false
}
}
}

// If this is a Delete method (AIP-135) with a return value of Empty,
// try to find the resource.
if strings.HasPrefix(m.GetName(), "Delete") && m.GetOutputType().GetName() == "Empty" {
if resource := findMessage(m.GetFile(), strings.TrimPrefix(m.GetName(), "Delete")); resource != nil {
return IsDeclarativeFriendly(resource)
// If the return value has a google.api.resource annotation, we can
// assume it is the resource and check it.
if IsResource(response) {
return IsDeclarativeFriendlyMessage(response)
}

// If the return value is a List response (AIP-132), we should be able
// to find the resource as a field in the response.
if n := response.GetName(); strings.HasPrefix(n, "List") && strings.HasSuffix(n, "Response") {
for _, field := range response.GetFields() {
if field.IsRepeated() && field.GetMessageType() != nil {
return IsDeclarativeFriendlyMessage(field.GetMessageType())
}
}
}

// At this point, we probably have a custom method.
// Try to identify a resource by whittling away at the method name and
// seeing if there is a match.
snakeName := strings.Split(strcase.SnakeCase(m.GetName()), "_")
for i := 1; i < len(snakeName); i++ {
name := strcase.UpperCamelCase(strings.Join(snakeName[i:], "_"))
if resource := findMessage(m.GetFile(), name); resource != nil {
return IsDeclarativeFriendly(resource)
}
// If this is a Delete method (AIP-135) with a return value of Empty,
// try to find the resource.
if strings.HasPrefix(m.GetName(), "Delete") && m.GetOutputType().GetName() == "Empty" {
if resource := findMessage(m.GetFile(), strings.TrimPrefix(m.GetName(), "Delete")); resource != nil {
return IsDeclarativeFriendlyMessage(resource)
}
}

// We found no evidence that this descriptor is supposed to be
// declarative-friendly. Return false.
// At this point, we probably have a custom method.
// Try to identify a resource by whittling away at the method name and
// seeing if there is a match.
snakeName := strings.Split(strcase.SnakeCase(m.GetName()), "_")
for i := 1; i < len(snakeName); i++ {
name := strcase.UpperCamelCase(strings.Join(snakeName[i:], "_"))
if resource := findMessage(m.GetFile(), name); resource != nil {
return IsDeclarativeFriendlyMessage(resource)
}
}

// We found no evidence that this method is declarative-friendly.
return false
}

Expand Down Expand Up @@ -128,3 +150,27 @@ func findMessage(f *desc.FileDescriptor, name string) *desc.MessageDescriptor {
// Whelp, no luck. Too bad.
return nil
}

// findMethod returns a method with a given name, or nil if none is found.
func findMethod(f *desc.FileDescriptor, name string) *desc.MethodDescriptor {
for _, s := range getServices(f) {
for _, m := range s.GetMethods() {
if m.GetName() == name {
return m
}
}
}
return nil
}

// getServices finds all services in a file and all imports within the
// same package.
func getServices(f *desc.FileDescriptor) []*desc.ServiceDescriptor {
answer := f.GetServices()
for _, dep := range f.GetDependencies() {
if f.GetPackage() == dep.GetPackage() {
answer = append(answer, getServices(dep)...)
}
}
return answer
}
26 changes: 19 additions & 7 deletions rules/internal/utils/declarative_friendly_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func TestDeclarativeFriendlyMessage(t *testing.T) {
{"FalseOtherStyle", "style: STYLE_UNSPECIFIED", false},
} {
t.Run(test.name, func(t *testing.T) {
m := testutils.ParseProto3Tmpl(t, `
f := testutils.ParseProto3Tmpl(t, `
import "google/api/resource.proto";
message Book {
Expand All @@ -42,17 +42,29 @@ func TestDeclarativeFriendlyMessage(t *testing.T) {
{{.Style}}
};
}
`, test).GetMessageTypes()[0]
if got := IsDeclarativeFriendly(m); got != test.want {
t.Errorf("Got %v, expected %v.", got, test.want)
message CreateBookRequest {
Book book = 1;
}
service Library {
rpc CreateBook(CreateBookRequest) returns (Book);
}
`, test)
for _, m := range f.GetMessageTypes() {
t.Run(m.GetName(), func(t *testing.T) {
if got := IsDeclarativeFriendlyMessage(m); got != test.want {
t.Errorf("Got %v, expected %v.", got, test.want)
}
})
}
})
}

// Test the case where the google.api.resource annotation is not present.
t.Run("NotResource", func(t *testing.T) {
m := testutils.ParseProto3String(t, "message Book {}").GetMessageTypes()[0]
if IsDeclarativeFriendly(m) {
if IsDeclarativeFriendlyMessage(m) {
t.Errorf("Got true, expected false.")
}
})
Expand Down Expand Up @@ -154,7 +166,7 @@ func TestDeclarativeFriendlyMethod(t *testing.T) {
}
`, tmpl), s)
m := f.GetServices()[0].GetMethods()[0]
if got := IsDeclarativeFriendly(m); got != test.want {
if got := IsDeclarativeFriendlyMethod(m); got != test.want {
t.Errorf("Got %v, expected %v.", got, test.want)
}
})
Expand All @@ -177,7 +189,7 @@ func TestDeclarativeFriendlyMethod(t *testing.T) {
`)
m := f.GetServices()[0].GetMethods()[0]
want := false
if got := IsDeclarativeFriendly(m); got != want {
if got := IsDeclarativeFriendlyMethod(m); got != want {
t.Errorf("Got %v, expected %v.", got, want)
}
})
Expand Down

0 comments on commit ddc47f2

Please sign in to comment.