Repository: googleapis/api-linter
Pull request #245 (2019-10-18 21:17:16)

[AIP-133/134/135] Test LRO responses better.
This adds response type checking when the protobuf response
type is an LRO.

(It also fixes a couple of bugs I found in the process. ;-))


Patch:
diff --git a/rules/aip0133/method_rules.go b/rules/aip0133/method_rules.go
index e46ed93..6033808 100644
--- a/rules/aip0133/method_rules.go
+++ b/rules/aip0133/method_rules.go
@@ -71,12 +71,6 @@ var httpBody = &lint.MethodRule{
 	URI:    "https://aip.dev/133#guidance",
 	OnlyIf: isCreateMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		// We only care about Create methods for the purpose of this rule;
-		// ignore everything else.
-		if !isCreateMethod(m) {
-			return nil
-		}
-
 		resourceMsgName := getResourceMsgName(m)
 		resourceFieldName := strings.ToLower(resourceMsgName)
 		for _, fieldDesc := range m.GetInputType().GetFields() {
@@ -143,22 +137,26 @@ var outputName = &lint.MethodRule{
 	URI:    "https://aip.dev/133#guidance",
 	OnlyIf: isCreateMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
-		output := m.GetOutputType()
+		want := getResourceMsgName(m)
 
-		// AIP-0151
-		if output.GetFullyQualifiedName() == "google.longrunning.Operation" {
-			return nil
+		// If this is an LRO, then use the annotated response type instead of
+		// the actual RPC return type.
+		got := m.GetOutputType().GetName()
+		if m.GetOutputType().GetFullyQualifiedName() == "google.longrunning.Operation" {
+			got = utils.GetOperationInfo(m).GetResponseType()
 		}
 
-		resourceMsgName := getResourceMsgName(m)
-
 		// Rule check: Establish that for methods such as `CreateFoo`, the response
 		// message should be named `Foo`
-		if output.GetName() != resourceMsgName {
+		//
+		// Note: If `got` is empty string, this is an unannotated LRO.
+		// The AIP-151 rule will whine about that, and this rule should not as it
+		// would be confusing.
+		if got != want && got != "" {
 			return []lint.Problem{{
 				Message: fmt.Sprintf(
 					"Create RPCs should have the corresponding resource as the response message, such as %q.",
-					resourceMsgName,
+					want,
 				),
 				// TODO: suggestion will be set after the location is set properly
 				// Suggestion: want,
diff --git a/rules/aip0134/method_rules.go b/rules/aip0134/method_rules.go
index 2148b97..6106533 100644
--- a/rules/aip0134/method_rules.go
+++ b/rules/aip0134/method_rules.go
@@ -55,25 +55,31 @@ var responseMessageName = &lint.MethodRule{
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Rule check: Establish that for methods such as `UpdateFoo`, the response
 		// message is `Foo` or `google.longrunning.Operation`.
-		got := m.GetOutputType().GetFullyQualifiedName()
-		want := []string{
-			m.GetName()[6:],
-			"google.longrunning.Operation",
+		want := strings.Replace(m.GetName(), "Update", "", 1)
+		got := m.GetOutputType().GetName()
+
+		// If the return type is an LRO, use the annotated response type instead.
+		if m.GetOutputType().GetFullyQualifiedName() == "google.longrunning.Operation" {
+			got = utils.GetOperationInfo(m).GetResponseType()
 		}
-		for _, v := range want {
-			if got == v {
-				return nil
-			}
+
+		// Return a problem if we did not get the expected return name.
+		//
+		// Note: If `got` is empty string, this is an unannotated LRO.
+		// The AIP-151 rule will whine about that, and this rule should not as it
+		// would be confusing.
+		if got != want && got != "" {
+			return []lint.Problem{{
+				Message: fmt.Sprintf(
+					"Update RPCs should have response message type %q, not %q.",
+					want,
+					got,
+				),
+				Suggestion: want,
+				Descriptor: m,
+			}}
 		}
-		return []lint.Problem{{
-			Message: fmt.Sprintf(
-				"Update RPCs should have response message type in %q, not %q.",
-				want,
-				got,
-			),
-			Suggestion: want[0],
-			Descriptor: m,
-		}}
+		return nil
 	},
 }
 
diff --git a/rules/aip0135/method_rules.go b/rules/aip0135/method_rules.go
index 931775a..bd0d04b 100644
--- a/rules/aip0135/method_rules.go
+++ b/rules/aip0135/method_rules.go
@@ -16,9 +16,11 @@ package aip0135
 
 import (
 	"fmt"
+	"strings"
 
-	"github.com/googleapis/api-linter/rules/internal/utils"
+	"bitbucket.org/creachadair/stringset"
 	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/rules/internal/utils"
 	"github.com/jhump/protoreflect/desc"
 )
 
@@ -52,27 +54,38 @@ var responseMessageName = &lint.MethodRule{
 	OnlyIf: isDeleteMethod,
 	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
 		// Rule check: Establish that for methods such as `DeleteFoo`, the response
-		// message is `google.protobuf.Empty`, `google.longrunning.Operation`, or `Foo`.
-		got := m.GetOutputType().GetFullyQualifiedName()
-		want := []string{
+		// message is `google.protobuf.Empty` or `Foo`.
+		got := m.GetOutputType().GetName()
+		if stringset.New("Empty", "Operation").Contains(got) {
+			got = m.GetOutputType().GetFullyQualifiedName()
+		}
+		want := stringset.New(
 			"google.protobuf.Empty",
-			"google.longrunning.Operation",
-			m.GetName()[6:],
+			strings.Replace(m.GetName(), "Delete", "", 1),
+		)
+
+		// If the return type is an Operation, use the annotated response type.
+		if got == "google.longrunning.Operation" {
+			got = utils.GetOperationInfo(m).GetResponseType()
 		}
-		for _, v := range want {
-			if got == v {
-				return nil
-			}
+
+		// If we did not get a permitted value, return a problem.
+		//
+		// Note: If `got` is empty string, this is an unannotated LRO.
+		// The AIP-151 rule will whine about that, and this rule should not as it
+		// would be confusing.
+		if !want.Contains(got) && got != "" {
+			return []lint.Problem{{
+				Message: fmt.Sprintf(
+					"Delete RPCs should have response message type of Empty or the resource, not %q.",
+					got,
+				),
+				Suggestion: "google.protobuf.Empty",
+				Descriptor: m,
+			}}
 		}
-		return []lint.Problem{{
-			Message: fmt.Sprintf(
-				"Delete RPCs should have response message type in %q, not %q.",
-				want,
-				got,
-			),
-			Suggestion: want[0],
-			Descriptor: m,
-		}}
+
+		return nil
 	},
 }
 


Test patch:
diff --git a/rules/aip0133/method_rules_test.go b/rules/aip0133/method_rules_test.go
index 39f0bc4..cbb1684 100644
--- a/rules/aip0133/method_rules_test.go
+++ b/rules/aip0133/method_rules_test.go
@@ -226,40 +226,49 @@ func TestInputName(t *testing.T) {
 	}
 }
 
-// Test does not cover the case when Output is "google.longrunning.Operation"
 func TestOutputMessageName(t *testing.T) {
 	// Set up the testing permutations.
 	tests := []struct {
-		testName   string
-		methodName string
-		outputName string
-		msg        string
+		testName     string
+		MethodName   string
+		RespTypeName string
+		LRO          bool
+		problems     testutils.Problems
 	}{
-		{"Valid", "CreateBook", "Book", ""},
-		{"Invalid", "CreateBook", "CreateBookResponse",
-			"Create RPCs should have the corresponding resource as the response message, such as \"Book\"."},
-		{"Irrelevant", "BuildBook", "BuildBookResponse", ""},
+		{"ValidResource", "CreateBook", "Book", false, testutils.Problems{}},
+		{"ValidLRO", "CreateBook", "Book", true, testutils.Problems{}},
+		{"Invalid", "CreateBook", "CreateBookResponse", false, testutils.Problems{{Message: "Book"}}},
+		{"InvalidLRO", "CreateBook", "CreateBookResponse", true, testutils.Problems{{Message: "Book"}}},
+		{"Irrelevant", "BuildBook", "BuildBookResponse", false, testutils.Problems{}},
 	}
 
 	// Run each test individually.
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
-			// Create a minimal service with a AIP-133 Get method
-			// (or with a different method, in the "Irrelevant" case).
-			service, err := builder.NewService("Library").AddMethod(builder.NewMethod(test.methodName,
-				builder.RpcTypeMessage(builder.NewMessage("GetBookRequest"), false),
-				builder.RpcTypeMessage(builder.NewMessage(test.outputName), false),
-			)).Build()
-			if err != nil {
-				t.Fatalf("Could not build %s method.", test.methodName)
-			}
+			// Create a minimal service with a AIP-134 Update method
+			file := testutils.ParseProto3Tmpl(t, `
+				import "google/longrunning/operations.proto";
+				service Library {
+					rpc {{.MethodName}}({{.MethodName}}Request)
+							returns ({{ if .LRO }}google.longrunning.Operation{{ else }}{{ .RespTypeName }}{{ end }}) {
+						{{ if .LRO -}}
+						option (google.longrunning.operation_info) = {
+							response_type: "{{.RespTypeName}}"
+							metadata_type: "{{.MethodName}}Metadata"
+						};
+						{{ end -}}
+					}
+				}
+				message {{.MethodName}}Request {}
+				message {{.RespTypeName}} {}
+			`, test)
 
-			// Run the lint rule, and establish that it returns the expected problems.
-			problems := outputName.Lint(service.GetFile())
-			if test.msg == "" && len(problems) > 0 {
-				t.Errorf("Got %v, expected no problems.", problems)
-			} else if test.msg != "" && !strings.Contains(problems[0].Message, test.msg) {
-				t.Errorf("Got %q, expected message containing %q", problems[0].Message, test.msg)
+			// Run the lint rule, and establish that it returns the correct
+			// number of problems.
+			problems := outputName.Lint(file)
+			method := file.GetServices()[0].GetMethods()[0]
+			if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" {
+				t.Errorf(diff)
 			}
 		})
 	}
diff --git a/rules/aip0134/method_rules_test.go b/rules/aip0134/method_rules_test.go
index aaa2a48..9203227 100644
--- a/rules/aip0134/method_rules_test.go
+++ b/rules/aip0134/method_rules_test.go
@@ -21,10 +21,8 @@ import (
 	"github.com/golang/protobuf/proto"
 	dpb "github.com/golang/protobuf/protoc-gen-go/descriptor"
 	"github.com/googleapis/api-linter/rules/internal/testutils"
-	"github.com/jhump/protoreflect/desc"
 	"github.com/jhump/protoreflect/desc/builder"
 	"google.golang.org/genproto/googleapis/api/annotations"
-	lro "google.golang.org/genproto/googleapis/longrunning"
 )
 
 func TestRequestMessageName(t *testing.T) {
@@ -63,45 +61,47 @@ func TestRequestMessageName(t *testing.T) {
 }
 
 func TestResponseMessageName(t *testing.T) {
-	op, err := desc.LoadMessageDescriptorForMessage(&lro.Operation{})
-	if err != nil {
-		t.Fatalf("Unable to load the Operation message.")
-	}
-	opbldr, err := builder.FromMessage(op)
-	if err != nil {
-		t.Fatalf("Unable to construct builder from op desc.")
-	}
-
 	// Set up the testing permutations.
 	tests := []struct {
-		testName    string
-		methodName  string
-		respMessage *builder.MessageBuilder
-		problems    testutils.Problems
+		testName     string
+		MethodName   string
+		RespTypeName string
+		LRO          bool
+		problems     testutils.Problems
 	}{
-		{"ValidResource", "UpdateBook", builder.NewMessage("Book"), testutils.Problems{}},
-		{"ValidLRO", "UpdateBook", opbldr, testutils.Problems{}},
-		{"Invalid", "UpdateBook", builder.NewMessage("UpdateBookResponse"), testutils.Problems{{Suggestion: "Book"}}},
-		{"Irrelevant", "AcquireBook", builder.NewMessage("AcquireBookResponse"), testutils.Problems{}},
+		{"ValidResource", "UpdateBook", "Book", false, testutils.Problems{}},
+		{"ValidLRO", "UpdateBook", "Book", true, testutils.Problems{}},
+		{"Invalid", "UpdateBook", "UpdateBookResponse", false, testutils.Problems{{Suggestion: "Book"}}},
+		{"InvalidLRO", "UpdateBook", "UpdateBookResponse", true, testutils.Problems{{Suggestion: "Book"}}},
+		{"Irrelevant", "MutateBook", "MutateBookResponse", false, testutils.Problems{}},
 	}
 
 	// Run each test individually.
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
 			// Create a minimal service with a AIP-134 Update method
-			// (or with a different method, in the "Irrelevant" case).
-			service, err := builder.NewService("Library").AddMethod(builder.NewMethod(test.methodName,
-				builder.RpcTypeMessage(builder.NewMessage("UpdateBookRequest"), false),
-				builder.RpcTypeMessage(test.respMessage, false),
-			)).Build()
-			if err != nil {
-				t.Fatalf("Could not build %s method.", test.methodName)
-			}
+			file := testutils.ParseProto3Tmpl(t, `
+				import "google/longrunning/operations.proto";
+				service Library {
+					rpc {{.MethodName}}({{.MethodName}}Request)
+							returns ({{ if .LRO }}google.longrunning.Operation{{ else }}{{ .RespTypeName }}{{ end }}) {
+						{{ if .LRO -}}
+						option (google.longrunning.operation_info) = {
+							response_type: "{{.RespTypeName}}"
+							metadata_type: "{{.MethodName}}Metadata"
+						};
+						{{ end -}}
+					}
+				}
+				message {{.MethodName}}Request {}
+				message {{.RespTypeName}} {}
+			`, test)
 
 			// Run the lint rule, and establish that it returns the correct
 			// number of problems.
-			problems := responseMessageName.Lint(service.GetFile())
-			if diff := test.problems.SetDescriptor(service.GetMethods()[0]).Diff(problems); diff != "" {
+			problems := responseMessageName.Lint(file)
+			method := file.GetServices()[0].GetMethods()[0]
+			if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" {
 				t.Errorf(diff)
 			}
 		})
diff --git a/rules/aip0135/method_rules_test.go b/rules/aip0135/method_rules_test.go
index 04deb9d..808c720 100644
--- a/rules/aip0135/method_rules_test.go
+++ b/rules/aip0135/method_rules_test.go
@@ -21,11 +21,8 @@ import (
 	"github.com/golang/protobuf/proto"
 	dpb "github.com/golang/protobuf/protoc-gen-go/descriptor"
 	"github.com/googleapis/api-linter/rules/internal/testutils"
-	epb "github.com/golang/protobuf/ptypes/empty"
-	"github.com/jhump/protoreflect/desc"
 	"github.com/jhump/protoreflect/desc/builder"
 	"google.golang.org/genproto/googleapis/api/annotations"
-	lro "google.golang.org/genproto/googleapis/longrunning"
 )
 
 func TestRequestMessageName(t *testing.T) {
@@ -64,54 +61,59 @@ func TestRequestMessageName(t *testing.T) {
 }
 
 func TestResponseMessageName(t *testing.T) {
-	// Get the correct message type for google.protobuf.Empty
-	empty, err := desc.LoadMessageDescriptorForMessage(&epb.Empty{})
-	if err != nil {
-		t.Fatalf("Unable to load the empty message.")
-	}
-	emptybldr, err := builder.FromMessage(empty)
-	if err != nil {
-		t.Fatalf("Unable to construct builder from empty desc.")
-	}
-	op, err := desc.LoadMessageDescriptorForMessage(&lro.Operation{})
-	if err != nil {
-		t.Fatalf("Unable to load the Operation message.")
-	}
-	opbldr, err := builder.FromMessage(op)
-	if err != nil {
-		t.Fatalf("Unable to construct builder from op desc.")
+	tmpl := map[string]string{
+		"sync": `
+			package test;
+			import "google/protobuf/empty.proto";
+			service Library {
+				rpc {{.MethodName}}({{.MethodName}}Request) returns ({{.RespTypeName}});
+			}
+			message {{.MethodName}}Request {}
+			{{ if (ne .RespTypeName "google.protobuf.Empty") }}message {{.RespTypeName}} {}{{ end }}
+		`,
+		"lro": `
+			package test;
+			import "google/longrunning/operations.proto";
+			service Library {
+				rpc {{.MethodName}}({{.MethodName}}Request)
+				    returns (google.longrunning.Operation) {
+					option (google.longrunning.operation_info) = {
+						response_type: "{{.RespTypeName}}"
+						metadata_type: "{{.MethodName}}Metadata"
+					};
+				}
+			}
+			message {{.MethodName}}Request {}
+		`,
 	}
 
 	// Set up the testing permutations.
 	tests := []struct {
 		testName     string
-		methodName   string
-		respMessage  *builder.MessageBuilder
+		tmpl         string
+		MethodName   string
+		RespTypeName string
 		problems     testutils.Problems
 	}{
-		{"ValidEmpty", "DeleteBook", emptybldr, testutils.Problems{}},
-		{"ValidLRO", "DeleteBook", opbldr, testutils.Problems{}},
-		{"ValidResource", "DeleteBook", builder.NewMessage("Book"), testutils.Problems{}},
-		{"Invalid", "DeleteBook", builder.NewMessage("DeleteBookResponse"), testutils.Problems{{Suggestion: "google.protobuf.Empty"}}},
-		{"Irrelevant", "AcquireBook", builder.NewMessage("AcquireBookResponse"), testutils.Problems{}},
+		{"ValidEmpty", tmpl["sync"], "DeleteBook", "google.protobuf.Empty", testutils.Problems{}},
+		{"ValidResource", tmpl["sync"], "DeleteBook", "Book", testutils.Problems{}},
+		{"ValidLROEmpty", tmpl["lro"], "DeleteBook", "google.protobuf.Empty", testutils.Problems{}},
+		{"ValidLROResource", tmpl["lro"], "DeleteBook", "Book", testutils.Problems{}},
+		{"Invalid", tmpl["sync"], "DeleteBook", "DeleteBookResponse", testutils.Problems{{Suggestion: "google.protobuf.Empty"}}},
+		{"InvalidLRO", tmpl["lro"], "DeleteBook", "DeleteBookResponse", testutils.Problems{{Suggestion: "google.protobuf.Empty"}}},
+		{"Irrelevant", tmpl["sync"], "DestroyBook", "DestroyBookResponse", testutils.Problems{}},
 	}
 
 	// Run each test individually.
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
 			// Create a minimal service with a AIP-135 Delete method
-			// (or with a different method, in the "Irrelevant" case).
-			service, err := builder.NewService("Library").AddMethod(builder.NewMethod(test.methodName,
-				builder.RpcTypeMessage(builder.NewMessage("DeleteBookRequest"), false),
-				builder.RpcTypeMessage(test.respMessage, false),
-			)).Build()
-			if err != nil {
-				t.Fatalf("Could not build %s method.", test.methodName)
-			}
+			file := testutils.ParseProto3Tmpl(t, test.tmpl, test)
 
 			// Run the lint rule, and establish that it returns the expected problems.
-			problems := responseMessageName.Lint(service.GetFile())
-			if diff := test.problems.SetDescriptor(service.GetMethods()[0]).Diff(problems); diff != "" {
+			method := file.GetServices()[0].GetMethods()[0]
+			problems := responseMessageName.Lint(file)
+			if diff := test.problems.SetDescriptor(method).Diff(problems); diff != "" {
 				t.Errorf(diff)
 			}
 		})
@@ -138,7 +140,7 @@ func TestHttpVerb(t *testing.T) {
 		methodName string
 		msg        string
 	}{
-		{"Valid", httpDelete, "GeDeleteBook", ""},
+		{"Valid", httpDelete, "DeleteBook", ""},
 		{"Invalid", httpGet, "DeleteBook", "HTTP DELETE"},
 		{"Irrelevant", httpGet, "AcquireBook", ""},
 	}