Repository: googleapis/api-linter
Pull request #232 (2019-10-14 18:14:12)

[AIP-151] Add rules for checking LRO annotations.


Patch:
diff --git a/rules/aip0151/aip0151.go b/rules/aip0151/aip0151.go
new file mode 100644
index 0000000..41c56c6
--- /dev/null
+++ b/rules/aip0151/aip0151.go
@@ -0,0 +1,40 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package aip0151 contains rules defined in https://aip.dev/151.
+package aip0151
+
+import (
+	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/rules/internal/utils"
+	"github.com/jhump/protoreflect/desc"
+)
+
+// AddRules adds all of the AIP-151 rules to the provided registry.
+func AddRules(r lint.RuleRegistry) {
+	r.Register(
+		lroAnnotationExists,
+		lroDefinedInFile,
+		lroMetadata,
+		lroResponse,
+	)
+}
+
+func isLRO(m *desc.MethodDescriptor) bool {
+	return m.GetOutputType().GetFullyQualifiedName() == "google.longrunning.Operation"
+}
+
+func isAnnotatedLRO(m *desc.MethodDescriptor) bool {
+	return isLRO(m) && utils.GetOperationInfo(m) != nil
+}
diff --git a/rules/aip0151/annotation.go b/rules/aip0151/annotation.go
new file mode 100644
index 0000000..7fb75d7
--- /dev/null
+++ b/rules/aip0151/annotation.go
@@ -0,0 +1,128 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package aip0151
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/googleapis/api-linter/lint"
+	"github.com/googleapis/api-linter/rules/internal/utils"
+	"github.com/jhump/protoreflect/desc"
+)
+
+var lroAnnotationExists = &lint.MethodRule{
+	Name:   lint.NewRuleName("core", "0151", "lro-annotation"),
+	URI:    "https://aip.dev/151#guidance",
+	OnlyIf: isLRO,
+	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
+		if utils.GetOperationInfo(m) == nil {
+			return []lint.Problem{{
+				Message:    "Methods returning an LRO must include the operation_info annotation.",
+				Descriptor: m,
+			}}
+		}
+		return nil
+	},
+}
+
+var lroResponse = &lint.MethodRule{
+	Name:   lint.NewRuleName("core", "0151", "lro-annotation", "response"),
+	URI:    "https://aip.dev/151#guidance",
+	OnlyIf: isAnnotatedLRO,
+	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
+		lro := utils.GetOperationInfo(m)
+
+		// Ensure the response type is set.
+		if lro.GetResponseType() == "" {
+			return []lint.Problem{{
+				Message:    "Methods returning an LRO must set the response type.",
+				Descriptor: m,
+			}}
+		}
+
+		// Unless this is a Delete method, the response type should not be Empty.
+		if strings.HasPrefix(m.GetName(), "Delete") {
+			return nil
+		}
+		if t := lro.GetResponseType(); t == "Empty" || t == "google.protobuf.Empty" {
+			return []lint.Problem{{
+				Message:    "Methods returning an LRO should create a blank message rather than using Empty.",
+				Descriptor: m,
+			}}
+		}
+
+		return nil
+	},
+}
+
+var lroMetadata = &lint.MethodRule{
+	Name:   lint.NewRuleName("core", "0151", "lro-annotation", "metadata"),
+	URI:    "https://aip.dev/151#guidance",
+	OnlyIf: isAnnotatedLRO,
+	LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
+		lro := utils.GetOperationInfo(m)
+
+		// Ensure the response type is set.
+		if lro.GetMetadataType() == "" {
+			return []lint.Problem{{
+				Message:    "Methods returning an LRO must set the metadata type.",
+				Descriptor: m,
+			}}
+		}
+
+		// The netadata type should not be Empty.
+		if t := lro.GetMetadataType(); t == "Empty" || t == "google.protobuf.Empty" {
+			return []lint.Problem{{
+				Message:    "Methods returning an LRO should create a blank message rather than using Empty.",
+				Descriptor: m,
+			}}
+		}
+
+		return nil
+	},
+}
+
+var lroDefinedInFile = &lint.MethodRule{
+	Name:   lint.NewRuleName("core", "0151", "lro-annotation", "defined-in-file"),
+	URI:    "https://aip.dev/151#guidance",
+	OnlyIf: isAnnotatedLRO,
+	LintMethod: func(m *desc.MethodDescriptor) (problems []lint.Problem) {
+		lro := utils.GetOperationInfo(m)
+		for k, t := range map[string]string{"Response": lro.GetResponseType(), "Metadata": lro.GetMetadataType()} {
+			// Ignore types defined in other packages.
+			if t == "" || strings.Contains(t, ".") {
+				continue
+			}
+
+			// Complain if the message is not defined in this file.
+			file := m.GetFile()
+			if pkg := file.GetPackage(); pkg != "" {
+				t = pkg + "." + t
+			}
+			if file.FindMessage(t) == nil {
+				problems = append(problems, lint.Problem{
+					Message: fmt.Sprintf(
+						"%s messages should be defined in the same file as the RPC.",
+						k,
+					),
+					Descriptor: m,
+				})
+			}
+		}
+
+		return
+	},
+}
diff --git a/rules/internal/utils/extension.go b/rules/internal/utils/extension.go
index a6aa23f..ce606b2 100644
--- a/rules/internal/utils/extension.go
+++ b/rules/internal/utils/extension.go
@@ -18,6 +18,7 @@ import (
 	"github.com/golang/protobuf/proto"
 	"github.com/jhump/protoreflect/desc"
 	apb "google.golang.org/genproto/googleapis/api/annotations"
+	lrpb "google.golang.org/genproto/googleapis/longrunning"
 )
 
 // GetFieldBehavior returns a slice of FieldBehavior annotations for
@@ -29,3 +30,12 @@ func GetFieldBehavior(f *desc.FieldDescriptor) []apb.FieldBehavior {
 	}
 	return nil
 }
+
+// GetOperationInfo returns the LRO annotation.
+func GetOperationInfo(m *desc.MethodDescriptor) *lrpb.OperationInfo {
+	opts := m.GetMethodOptions()
+	if x, err := proto.GetExtension(opts, lrpb.E_OperationInfo); err == nil {
+		return x.(*lrpb.OperationInfo)
+	}
+	return nil
+}
diff --git a/rules/rules.go b/rules/rules.go
index cd51ef2..3d5e145 100644
--- a/rules/rules.go
+++ b/rules/rules.go
@@ -23,6 +23,7 @@ import (
 	"github.com/googleapis/api-linter/rules/aip0134"
 	"github.com/googleapis/api-linter/rules/aip0135"
 	"github.com/googleapis/api-linter/rules/aip0140"
+	"github.com/googleapis/api-linter/rules/aip0151"
 	"github.com/googleapis/api-linter/rules/aip0158"
 	"github.com/googleapis/api-linter/rules/aip0191"
 	"github.com/googleapis/api-linter/rules/aip0192"
@@ -36,6 +37,7 @@ func init() {
 	aip0134.AddRules(coreRules)
 	aip0135.AddRules(coreRules)
 	aip0140.AddRules(coreRules)
+	aip0151.AddRules(coreRules)
 	aip0158.AddRules(coreRules)
 	aip0191.AddRules(coreRules)
 	aip0192.AddRules(coreRules)


Test patch:
diff --git a/rules/aip0151/aip0151_test.go b/rules/aip0151/aip0151_test.go
new file mode 100644
index 0000000..34bbbc8
--- /dev/null
+++ b/rules/aip0151/aip0151_test.go
@@ -0,0 +1,32 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package aip0151
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/googleapis/api-linter/lint"
+)
+
+func TestAddRules(t *testing.T) {
+	rules := make(lint.RuleRegistry)
+	AddRules(rules)
+	for ruleName := range rules {
+		if !strings.HasPrefix(string(ruleName), "core::0151") {
+			t.Errorf("Rule %s is not namespaced to core::0151.", ruleName)
+		}
+	}
+}
diff --git a/rules/aip0151/annotation_test.go b/rules/aip0151/annotation_test.go
new file mode 100644
index 0000000..0afba92
--- /dev/null
+++ b/rules/aip0151/annotation_test.go
@@ -0,0 +1,166 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package aip0151
+
+import (
+	"testing"
+
+	"github.com/googleapis/api-linter/rules/internal/testutils"
+)
+
+func TestAnnotationExistsValid(t *testing.T) {
+	f := testutils.ParseProto3String(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: "WriteBookMetadata"
+				};
+			}
+		}
+		message WriteBookRequest {}
+	`)
+	if diff := (testutils.Problems{}).Diff(lroAnnotationExists.Lint(f)); diff != "" {
+		t.Errorf(diff)
+	}
+}
+
+func TestAnnotationExistsInvalid(t *testing.T) {
+	f := testutils.ParseProto3String(t, `
+		import "google/longrunning/operations.proto";
+		service Library {
+			rpc WriteBook(WriteBookRequest) returns (google.longrunning.Operation);
+		}
+		message WriteBookRequest {}
+	`)
+	want := testutils.Problems{{
+		Descriptor: f.GetServices()[0].GetMethods()[0],
+		Message:    "operation_info annotation",
+	}}
+	if diff := want.Diff(lroAnnotationExists.Lint(f)); diff != "" {
+		t.Errorf(diff)
+	}
+}
+
+func TestLROResponse(t *testing.T) {
+	tests := []struct {
+		testName   string
+		MethodName string
+		Response   string
+		problems   testutils.Problems
+	}{
+		{"Valid", "WriteBook", "WriteBookResponse", testutils.Problems{}},
+		{"InvalidEmptyString", "WriteBook", "", testutils.Problems{{Message: "must set the response type"}}},
+		{"InvalidGPEmpty", "WriteBook", "google.protobuf.Empty", testutils.Problems{{Message: "Empty"}}},
+		{"ValidGPEmptyDelete", "DeleteBook", "google.protobuf.Empty", testutils.Problems{}},
+	}
+	for _, test := range tests {
+		t.Run(test.testName, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				import "google/longrunning/operations.proto";
+				service Library {
+					rpc {{.MethodName}}({{.MethodName}}Request)
+							returns (google.longrunning.Operation) {
+						option (google.longrunning.operation_info) = {
+							response_type: "{{.Response}}"
+							metadata_type: "{{.MethodName}}Metadata"
+						};
+					}
+				}
+				message {{.MethodName}}Request {}
+			`, test)
+			problems := lroResponse.Lint(f)
+			d := f.GetServices()[0].GetMethods()[0]
+			if diff := test.problems.SetDescriptor(d).Diff(problems); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}
+
+func TestLROMetadata(t *testing.T) {
+	tests := []struct {
+		testName   string
+		MethodName string
+		Metadata   string
+		problems   testutils.Problems
+	}{
+		{"Valid", "WriteBook", "WriteBookMetadata", testutils.Problems{}},
+		{"InvalidEmptyString", "WriteBook", "", testutils.Problems{{Message: "must set the metadata type"}}},
+		{"InvalidGPEmpty", "WriteBook", "google.protobuf.Empty", testutils.Problems{{Message: "Empty"}}},
+		{"InvalidGPEmptyDelete", "DeleteBook", "google.protobuf.Empty", testutils.Problems{{Message: "Empty"}}},
+	}
+	for _, test := range tests {
+		t.Run(test.testName, func(t *testing.T) {
+			f := testutils.ParseProto3Tmpl(t, `
+				import "google/longrunning/operations.proto";
+				service Library {
+					rpc {{.MethodName}}({{.MethodName}}Request)
+							returns (google.longrunning.Operation) {
+						option (google.longrunning.operation_info) = {
+							response_type: "{{.MethodName}}Response"
+							metadata_type: "{{.Metadata}}"
+						};
+					}
+				}
+				message {{.MethodName}}Request {}
+			`, test)
+			problems := lroMetadata.Lint(f)
+			d := f.GetServices()[0].GetMethods()[0]
+			if diff := test.problems.SetDescriptor(d).Diff(problems); diff != "" {
+				t.Errorf(diff)
+			}
+		})
+	}
+}
+
+func TestDefinedInFile(t *testing.T) {
+	tests := []struct {
+		testName     string
+		Package      string
+		ResponseType string
+		problems     testutils.Problems
+	}{
+		{"Valid", "", "WriteBookResponse", testutils.Problems{}},
+		{"ValidPkg", "package test;", "WriteBookResponse", testutils.Problems{}},
+		{"InvalidTypo", "", "WriteBookRepsonse", testutils.Problems{{Message: "should be defined"}}},
+		{"InvalidTypoPkg", "package test;", "WriteBookRepsonse", testutils.Problems{{Message: "should be defined"}}},
+		{"ValidExternal", "", "google.protobuf.Empty", testutils.Problems{}},
+		{"ValidExternalPkg", "package test;", "google.protobuf.Empty", testutils.Problems{}},
+	}
+	for _, test := range tests {
+		f := testutils.ParseProto3Tmpl(t, `
+			{{.Package}}
+			import "google/longrunning/operations.proto";
+			service Library {
+				rpc WriteBook(WriteBookRequest) returns (google.longrunning.Operation) {
+					option (google.longrunning.operation_info) = {
+						response_type: "{{.ResponseType}}"
+						metadata_type: "OperationMetadata"
+					};
+				}
+			}
+			message WriteBookRequest {}
+			message WriteBookResponse {}
+			message OperationMetadata {}
+		`, test)
+		problems := lroDefinedInFile.Lint(f)
+		m := f.GetServices()[0].GetMethods()[0]
+		if diff := test.problems.SetDescriptor(m).Diff(problems); diff != "" {
+			t.Errorf(diff)
+		}
+	}
+}
diff --git a/rules/internal/utils/extension_test.go b/rules/internal/utils/extension_test.go
index 5dc69bb..d161aa0 100644
--- a/rules/internal/utils/extension_test.go
+++ b/rules/internal/utils/extension_test.go
@@ -54,3 +54,39 @@ func TestGetFieldBehavior(t *testing.T) {
 		})
 	}
 }
+
+func TestGetOperationInfo(t *testing.T) {
+	fd := testutils.ParseProto3String(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: "WriteBookMetadata"
+				};
+			}
+		}
+		message WriteBookRequest {}
+	`)
+	lro := GetOperationInfo(fd.GetServices()[0].GetMethods()[0])
+	if got, want := lro.ResponseType, "WriteBookResponse"; got != want {
+		t.Errorf("Response type - got %q, want %q.", got, want)
+	}
+	if got, want := lro.MetadataType, "WriteBookMetadata"; got != want {
+		t.Errorf("Metadata type - got %q, want %q.", got, want)
+	}
+}
+
+func TestGetOperationInfoNone(t *testing.T) {
+	fd := testutils.ParseProto3String(t, `
+		service Library {
+			rpc GetBook(GetBookRequest) returns (Book);
+		}
+		message GetBookRequest {}
+		message Book {}
+	`)
+	lro := GetOperationInfo(fd.GetServices()[0].GetMethods()[0])
+	if lro != nil {
+		t.Errorf("Got %v, expected nil LRO annotation.", lro)
+	}
+}