Repository: googleapis/api-linter
Pull request #650 (2020-10-07 19:34:13)

refactor: Make separate IsDeclarativeFriendly methods for message and method.
Otherwise they can not be sent to `OnlyIf`.
Additionally, support request messages (e.g. `CreateBookRequest`).


Patch:
diff --git a/rules/internal/utils/declarative_friendly.go b/rules/internal/utils/declarative_friendly.go
index 04f98af..8239817 100644
--- a/rules/internal/utils/declarative_friendly.go
+++ b/rules/internal/utils/declarative_friendly.go
@@ -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
 }
 
@@ -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
+}


Test patch:
diff --git a/rules/internal/utils/declarative_friendly_test.go b/rules/internal/utils/declarative_friendly_test.go
index 3bd3efd..4e58cb8 100644
--- a/rules/internal/utils/declarative_friendly_test.go
+++ b/rules/internal/utils/declarative_friendly_test.go
@@ -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 {
@@ -42,9 +42,21 @@ 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)
+					}
+				})
 			}
 		})
 	}
@@ -52,7 +64,7 @@ func TestDeclarativeFriendlyMessage(t *testing.T) {
 	// 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.")
 		}
 	})
@@ -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)
 					}
 				})
@@ -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)
 		}
 	})