diff --git a/go.sum b/go.sum index 8cdbf8685..4d0cce0ab 100644 --- a/go.sum +++ b/go.sum @@ -22,10 +22,10 @@ github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/google/go-cmp v0.3.1 h1:Xye71clBPdm5HgqGwUkwhbynsUJZhDbS20FvLhQ2izg= github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU= -github.com/jhump/protoreflect v1.5.0 h1:NgpVT+dX71c8hZnxHof2M7QDK7QtohIJ7DYycjnkyfc= -github.com/jhump/protoreflect v1.5.0/go.mod h1:eaTn3RZAmMBcV0fifFvlm6VHNz3wSkYyXYWUh7ymB74= github.com/jhump/protoreflect v1.5.1-0.20191024142022-1d0024204e58 h1:RHSRszt3DIDwIwV1fPXtZI74OL36BkDIzsp2IOdp+Xc= github.com/jhump/protoreflect v1.5.1-0.20191024142022-1d0024204e58/go.mod h1:eaTn3RZAmMBcV0fifFvlm6VHNz3wSkYyXYWUh7ymB74= +github.com/kjk/inflect v0.0.0-20190213232030-bfc9dcd6393b h1:pSKZGMcdvyNmiHxmVni0zd28IL+xf+kKAixrsMdS3Mg= +github.com/kjk/inflect v0.0.0-20190213232030-bfc9dcd6393b/go.mod h1:dI+lsL14MLXYhREARMljP6+dati5FyM0eg9QEMUdUUU= github.com/lithammer/dedent v1.1.0 h1:VNzHMVCBNG1j0fh3OrsFRkVUwStdDArbgBWoPAffktY= github.com/lithammer/dedent v1.1.0/go.mod h1:jrXYCQtgg0nJiN+StA2KgR7w6CiQNv9Fd/Z9BP0jIOc= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= diff --git a/rules/aip0231/aip0231.go b/rules/aip0231/aip0231.go new file mode 100644 index 000000000..f8cc40092 --- /dev/null +++ b/rules/aip0231/aip0231.go @@ -0,0 +1,60 @@ +// 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 aip0231 contains rules defined in https://aip.dev/231. +package aip0231 + +import ( + "regexp" + + "github.com/googleapis/api-linter/lint" + "github.com/jhump/protoreflect/desc" +) + +// AddRules accepts a register function and registers each of +// this AIP's rules to it. +func AddRules(r lint.RuleRegistry) { + r.Register( + pluralMethodResourceName, + inputName, + outputName, + httpBody, + httpUrl, + httpVerb, + parentField, + namesField, + resourceField, + ) +} + +var batchGetMethodRegexp = regexp.MustCompile("^BatchGet(?:[A-Za-z0-9]|$)") +var batchGetReqMessageRegexp = regexp.MustCompile("^BatchGet[A-Za-z0-9]*Request$") +var batchGetResMessageRegexp = regexp.MustCompile("^BatchGet[A-Za-z0-9]*Response$") +var batchGetURINameRegexp = regexp.MustCompile("\\{[a-zA-Z=/*]+\\}\\/[A-Za-z0-9-=/*]+:batchGet$") +var getReqMessageRegexp = regexp.MustCompile("^Get[A-Za-z0-9]*Request$") + +// Returns true if this is a AIP-231 Get method, false otherwise. +func isBatchGetMethod(m *desc.MethodDescriptor) bool { + return batchGetMethodRegexp.MatchString(m.GetName()) +} + +// Returns true if this is an AIP-231 Get request message, false otherwise. +func isBatchGetRequestMessage(m *desc.MessageDescriptor) bool { + return batchGetReqMessageRegexp.MatchString(m.GetName()) +} + +// Returns true if this is an AIP-231 Get request message, false otherwise. +func isBatchGetResponseMessage(m *desc.MessageDescriptor) bool { + return batchGetResMessageRegexp.MatchString(m.GetName()) +} diff --git a/rules/aip0231/aip0231_test.go b/rules/aip0231/aip0231_test.go new file mode 100644 index 000000000..e2b37b9ff --- /dev/null +++ b/rules/aip0231/aip0231_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 aip0231 + +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::0231") { + t.Errorf("Rule %s is not namespaced to core::0231.", ruleName) + } + } +} diff --git a/rules/aip0231/message_rules.go b/rules/aip0231/message_rules.go new file mode 100644 index 000000000..0d8ecff8d --- /dev/null +++ b/rules/aip0231/message_rules.go @@ -0,0 +1,150 @@ +// 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 aip0231 + +import ( + "fmt" + + "github.com/gertd/go-pluralize" + "github.com/googleapis/api-linter/lint" + "github.com/jhump/protoreflect/desc" + "github.com/jhump/protoreflect/desc/builder" +) + +// The Batch Get request message should have parent field. +var parentField = &lint.MessageRule{ + Name: lint.NewRuleName("core", "0231", "request-message", "parent-field"), + URI: "https://aip.dev/231#request-message", + OnlyIf: isBatchGetRequestMessage, + LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { + // Rule check: Establish that a `parent` field is present. + parentField := m.FindFieldByName("parent") + if parentField == nil { + return []lint.Problem{{ + Message: fmt.Sprintf("Message %q has no `parent` field", m.GetName()), + Descriptor: m, + }} + } + + // Rule check: Establish that the parent field is a string. + if parentField.GetType() != builder.FieldTypeString().GetType() { + return []lint.Problem{{ + Message: "`parent` field on create request message must be a string", + Descriptor: parentField, + }} + } + + return nil + }, +} + +// The Batch Get standard method should have repeated name field or repeated +// standard get request message field, but the latter one is not suggested. +var namesField = &lint.MessageRule{ + Name: lint.NewRuleName("core", "0231", "request-message", "name-field"), + URI: "https://aip.dev/231#request-message", + OnlyIf: isBatchGetRequestMessage, + LintMessage: func(m *desc.MessageDescriptor) (problems []lint.Problem) { + // Rule check: Establish that a name field is present. + names := m.FindFieldByName("names") + getReqMsg := m.FindFieldByName("requests") + + // Rule check: Ensure that the names field is existed. + if names == nil && getReqMsg == nil { + problems = append(problems, lint.Problem{ + Message: fmt.Sprintf(`Message %q has no "names" field`, m.GetName()), + Descriptor: m, + }) + } + + // Rule check: Ensure that only the suggested names field is existed. + if names != nil && getReqMsg != nil { + problems = append(problems, lint.Problem{ + Message: fmt.Sprintf(`Message %q should delete "requests" field, only keep the "names" field`, m.GetName()), + Descriptor: getReqMsg, + }) + } + + // Rule check: Ensure that the names field is repeated. + if names != nil && !names.IsRepeated() { + problems = append(problems, lint.Problem{ + Message: `The "names"" field should be repeated`, + Descriptor: names, + }) + } + + // Rule check: Ensure that the names field is the correct type. + if names != nil && names.GetType() != builder.FieldTypeString().GetType() { + problems = append(problems, lint.Problem{ + Message: `"names" field on Batch Get Request should be a "string" type`, + Descriptor: names, + }) + } + + // Rule check: Ensure that the standard get request message field is repeated. + if getReqMsg != nil && !getReqMsg.IsRepeated() { + problems = append(problems, lint.Problem{ + Message: `The "requests" field should be repeated`, + Descriptor: getReqMsg, + }) + } + + // Rule check: Ensure that the standard get request message field is the + // correct type. Note: Use m.GetName()[8:len(m.GetName())-7]) to retrieve + // the resource name from the the batch get request, for example: + // "BatchGetBooksRequest" -> "Books" + rightTypeName := fmt.Sprintf("Get%sRequest", pluralize.NewClient().Singular(m.GetName()[8:len(m.GetName())-7])) + if getReqMsg != nil && (getReqMsg.GetMessageType() == nil || getReqMsg.GetMessageType().GetName() != rightTypeName) { + problems = append(problems, lint.Problem{ + Message: fmt.Sprintf(`The "requests" field on Batch Get Request should be a %q type`, rightTypeName), + Descriptor: getReqMsg, + }) + } + return + }, +} + +// The Batch Get response message should have resource field. +var resourceField = &lint.MessageRule{ + Name: lint.NewRuleName("core", "0231", "response-message", "resource-field"), + URI: "https://aip.dev/231#response-message", + OnlyIf: isBatchGetResponseMessage, + LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { + // the singular form the resource name, the first letter is Capitalized. + // Note: Use m.GetName()[8 : len(m.GetName())-9] to retrieve the resource + // name from the the batch get response, for example: + // "BatchGetBooksResponse" -> "Books" + resourceMsgName := pluralize.NewClient().Singular(m.GetName()[8 : len(m.GetName())-9]) + + for _, fieldDesc := range m.GetFields() { + if msgDesc := fieldDesc.GetMessageType(); msgDesc != nil && msgDesc.GetName() == resourceMsgName { + if !fieldDesc.IsRepeated() { + return []lint.Problem{{ + Message: fmt.Sprintf("The %q type field on Batch Get Response message should be repeated", msgDesc.GetName()), + Descriptor: fieldDesc, + }} + } + + return nil + } + } + + // Rule check: Establish that a resource field must be included. + return []lint.Problem{{ + Message: fmt.Sprintf("Message %q has no %q type field", m.GetName(), resourceMsgName), + Descriptor: m, + }} + }, +} diff --git a/rules/aip0231/message_rules_test.go b/rules/aip0231/message_rules_test.go new file mode 100644 index 000000000..ce8a1d7ab --- /dev/null +++ b/rules/aip0231/message_rules_test.go @@ -0,0 +1,282 @@ +// 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 aip0231 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" + "github.com/jhump/protoreflect/desc" + "github.com/jhump/protoreflect/desc/builder" +) + +type field struct { + fieldName string + fieldType *builder.FieldType +} + +func TestParentField(t *testing.T) { + // Set up the testing permutations. + tests := []struct { + testName string + messageName string + messageField *field + problems testutils.Problems + problemDesc func(m *desc.MessageDescriptor) desc.Descriptor + }{ + { + "Valid", + "BatchGetBooksRequest", + &field{"parent", builder.FieldTypeString()}, + testutils.Problems{}, + nil}, + { + "MissingField", + "BatchGetBooksRequest", + nil, + testutils.Problems{{Message: "parent"}}, + nil, + }, + { + "InvalidType", + "BatchGetBooksRequest", + &field{"parent", builder.FieldTypeDouble()}, + testutils.Problems{{Message: "string"}}, + func(m *desc.MessageDescriptor) desc.Descriptor { + return m.FindFieldByName("parent") + }, + }, + { + "Irrelevant", + "EnumerateBooksRequest", + &field{"id", builder.FieldTypeString()}, + testutils.Problems{}, + nil, + }, + } + + // Run each test individually. + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + // Create an appropriate message descriptor. + messageBuilder := builder.NewMessage(test.messageName) + + if test.messageField != nil { + messageBuilder.AddField( + builder.NewField(test.messageField.fieldName, test.messageField.fieldType), + ) + } + + message, err := messageBuilder.Build() + if err != nil { + t.Fatalf("Could not build %s message.", test.messageName) + } + + // Determine the descriptor that a failing test will attach to. + var problemDesc desc.Descriptor = message + if test.problemDesc != nil { + problemDesc = test.problemDesc(message) + } + + // Run the lint rule, and establish that it returns the correct problems. + problems := parentField.Lint(message.GetFile()) + if diff := test.problems.SetDescriptor(problemDesc).Diff(problems); diff != "" { + t.Errorf(diff) + } + }) + } +} + +func TestResourceField(t *testing.T) { + // Set up the testing permutations. + tests := []struct { + testName string + src string + problems testutils.Problems + problemDesc func(m *desc.MessageDescriptor) desc.Descriptor + }{ + { + testName: "Valid", + src: ` +message BatchGetBooksResponse { + // Books requested. + repeated Book books = 1; +}`, + problems: testutils.Problems{}, + }, + { + testName: "FieldIsNotRepeated", + src: ` +message BatchGetBooksResponse { + // Book requested. + Book book = 1; +}`, + problems: testutils.Problems{{Message: "The \"Book\" type field on Batch Get Response message should be repeated"}}, + }, + { + testName: "MissingField", + src: ` +message BatchGetBooksResponse { + string response = 1; +}`, + problems: testutils.Problems{{Message: "Message \"BatchGetBooksResponse\" has no \"Book\" type field"}}, + problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor { + return m + }, + }, + } + + // Run each test individually. + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + template := ` +{{.Src}} +message Book { +}` + file := testutils.ParseProto3Tmpl(t, template, struct{ Src string }{test.src}) + + m := file.GetMessageTypes()[0] + + // Determine the descriptor that a failing test will attach to. + var problemDesc desc.Descriptor = m.GetFields()[0] + if test.problemDesc != nil { + problemDesc = test.problemDesc(m) + } + + problems := resourceField.Lint(file) + if diff := test.problems.SetDescriptor(problemDesc).Diff(problems); diff != "" { + t.Errorf(diff) + } + }) + } +} + +func TestNamesField(t *testing.T) { + // Set up the testing permutations. + tests := []struct { + testName string + src string + problems testutils.Problems + problemDesc func(m *desc.MessageDescriptor) desc.Descriptor + }{ + { + testName: "Valid-Names", + src: ` +message BatchGetBooksRequest { +repeated string names = 1; +}`, + problems: testutils.Problems{}, + }, + { + testName: "Valid-StandardGetReq", + src: ` +message BatchGetBooksRequest { +repeated GetBookRequest requests = 1; +} + +message GetBookRequest {} +`, + problems: testutils.Problems{}, + }, + { + testName: "Invalid-MissingNamesField", + src: ` +message BatchGetBooksRequest { + string parent = 1; +}`, + problems: testutils.Problems{{Message: `Message "BatchGetBooksRequest" has no "names" field`}}, + }, + { + testName: "Invalid-KeepingNamesFieldOnly", + src: ` +message BatchGetBooksRequest { +repeated string names = 1; +repeated GetBookRequest requests = 2; +} + +message GetBookRequest {}`, + problems: testutils.Problems{{Message: `Message "BatchGetBooksRequest" should delete "requests" field, only keep the "names" field`}}, + problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor { + return m.FindFieldByName("requests") + }, + }, + { + testName: "Invalid-NamesFieldIsNotRepeated", + src: ` +message BatchGetBooksRequest { +string names = 1; +}`, + problems: testutils.Problems{{Message: `The "names"" field should be repeated`}}, + problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor { + return m.FindFieldByName("names") + }, + }, + { + testName: "Invalid-NamesFieldWrongType", + src: ` +message BatchGetBooksRequest { +repeated int32 names = 1; +}`, + problems: testutils.Problems{{Message: `"names" field on Batch Get Request should be a "string" type`}}, + problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor { + return m.FindFieldByName("names") + }, + }, + { + testName: "Invalid-GetReqFieldIsNotRepeated", + src: ` +message BatchGetBooksRequest { +GetBookRequest requests = 1; +} + +message GetBookRequest {}`, + problems: testutils.Problems{{Message: `The "requests" field should be repeated`}}, + problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor { + return m.FindFieldByName("requests") + }, + }, + { + testName: "Invalid-NamesFieldWrongType", + src: ` +message BatchGetBooksRequest { + repeated string requests = 1; +}`, + problems: testutils.Problems{{Message: `The "requests" field on Batch Get Request should be a "GetBookRequest" type`}}, + problemDesc: func(m *desc.MessageDescriptor) desc.Descriptor { + return m.FindFieldByName("requests") + }, + }, + } + + // Run each test individually. + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + file := testutils.ParseProto3String(t, test.src) + + m := file.GetMessageTypes()[0] + + // Determine the descriptor that a failing test will attach to. + var problemDesc desc.Descriptor = m + if test.problemDesc != nil { + problemDesc = test.problemDesc(m) + } + + problems := namesField.Lint(file) + if diff := test.problems.SetDescriptor(problemDesc).Diff(problems); diff != "" { + t.Errorf(diff) + } + }) + } +} diff --git a/rules/aip0231/method_rules.go b/rules/aip0231/method_rules.go new file mode 100644 index 000000000..8f0b93ea9 --- /dev/null +++ b/rules/aip0231/method_rules.go @@ -0,0 +1,158 @@ +// 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 aip0231 + +import ( + "fmt" + + "github.com/gertd/go-pluralize" + "github.com/googleapis/api-linter/lint" + "github.com/googleapis/api-linter/rules/internal/utils" + "github.com/jhump/protoreflect/desc" +) + +var pluralMethodResourceName = &lint.MethodRule{ + Name: lint.NewRuleName("core", "0231", "method-name", "name"), + URI: "https://aip.dev/231#guidance", + OnlyIf: isBatchGetMethod, + LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { + // Note: m.GetName()[8:] is used to retrieve the resource name from the + // method name. For example, "BatchGetFoos" -> "Foos" + pluralMethodResourceName := m.GetName()[8:] + + pluralize := pluralize.NewClient() + + // Rule check: Establish that for methods such as `BatchGetFoos` + if !pluralize.IsPlural(pluralMethodResourceName) { + return []lint.Problem{{ + Message: fmt.Sprintf( + `The resource part in method name %q shouldn't %q, but should be its plural form %q`, + m.GetName(), pluralMethodResourceName, pluralize.Plural(pluralMethodResourceName), + ), + Descriptor: m, + }} + } + + return nil + }, +} + +// Batch Get method should have a properly named Request message. +var inputName = &lint.MethodRule{ + Name: lint.NewRuleName("core", "0231", "input-message", "name"), + URI: "https://aip.dev/231#guidance", + OnlyIf: isBatchGetMethod, + LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { + pluralInputResourceName := pluralize.NewClient().Plural(m.GetName()[8:]) + + // Rule check: Establish that for methods such as `BatchGetFoos`, the request + // message is named `BatchGetFoosRequest`. + if got, want := m.GetInputType().GetName(), fmt.Sprintf("BatchGet%sRequest", pluralInputResourceName); got != want { + return []lint.Problem{{ + Message: fmt.Sprintf( + "Batch Get RPCs should have a properly named request message %q, but not %q", + want, got, + ), + Descriptor: m, + }} + } + + return nil + }, +} + +// Batch Get method should have a properly named Response message. +var outputName = &lint.MethodRule{ + Name: lint.NewRuleName("core", "0231", "output-message", "name"), + URI: "https://aip.dev/231#guidance", + OnlyIf: isBatchGetMethod, + LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { + pluralInputResourceName := pluralize.NewClient().Plural(m.GetName()[8:]) + + // Rule check: Establish that for methods such as `BatchGetFoos`, the request + // message is named `BatchGetFoosResponse`. + if got, want := m.GetOutputType().GetName(), fmt.Sprintf("BatchGet%sResponse", pluralInputResourceName); got != want { + return []lint.Problem{{ + Message: fmt.Sprintf( + "Batch Get RPCs should have a properly named response message %q, but not %q", + want, got, + ), + Descriptor: m, + }} + } + + return nil + }, +} + +// Batch Get methods should use the HTTP GET verb. +var httpVerb = &lint.MethodRule{ + Name: lint.NewRuleName("core", "0231", "http-verb"), + URI: "https://aip.dev/231#guidance", + OnlyIf: isBatchGetMethod, + LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { + // Rule check: Establish that the RPC uses HTTP GET. + for _, httpRule := range utils.GetHTTPRules(m) { + if httpRule.Method != "GET" { + return []lint.Problem{{ + Message: "Batch Get methods must use the HTTP GET verb.", + Descriptor: m, + }} + } + } + + return nil + }, +} + +// Batch Get methods should have a proper HTTP pattern. +var httpUrl = &lint.MethodRule{ + Name: lint.NewRuleName("core", "0231", "http-name"), + URI: "https://aip.dev/231#guidance", + OnlyIf: isBatchGetMethod, + LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { + // Establish that the RPC has no HTTP body. + for _, httpRule := range utils.GetHTTPRules(m) { + if !batchGetURINameRegexp.MatchString(httpRule.URI) { + return []lint.Problem{{ + Message: `Get methods URI should be end with ":batchGet".`, + Descriptor: m, + }} + } + } + + return nil + }, +} + +// Get methods should not have an HTTP body. +var httpBody = &lint.MethodRule{ + Name: lint.NewRuleName("core", "0231", "http-body"), + URI: "https://aip.dev/231#guidance", + OnlyIf: isBatchGetMethod, + LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { + // Establish that the RPC has no HTTP body. + for _, httpRule := range utils.GetHTTPRules(m) { + if httpRule.Body != "" { + return []lint.Problem{{ + Message: "Batch Get methods should not have an HTTP body.", + Descriptor: m, + }} + } + } + + return nil + }, +} diff --git a/rules/aip0231/method_rules_test.go b/rules/aip0231/method_rules_test.go new file mode 100644 index 000000000..8f397a59c --- /dev/null +++ b/rules/aip0231/method_rules_test.go @@ -0,0 +1,509 @@ +// 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 aip0231 + +import ( + "testing" + + "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/builder" + "google.golang.org/genproto/googleapis/api/annotations" +) + +func TestPluralMethodResourceName(t *testing.T) { + // Set up the testing permutations. + tests := []struct { + testName string + src string + problems testutils.Problems + }{ + { + testName: "Valid-BatchGetBooks", + src: `import "google/api/annotations.proto"; + +service BookService { + rpc BatchGetBooks(BatchGetBooksRequest) returns (BatchGetBooksResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/books:batchGet" + }; + } +} + +message BatchGetBooksRequest {} + +message BatchGetBooksResponse{} +`, + problems: testutils.Problems{}, + }, + { + testName: "Valid-BatchGetMen", + src: `import "google/api/annotations.proto"; + +service ManService { + rpc BatchGetMen(BatchGetMenRequest) returns (BatchGetMenResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/men:batchGet" + }; + } +} + +message BatchGetMenRequest {} + +message BatchGetMenResponse{} +`, + problems: testutils.Problems{}, + }, + { + testName: "Invalid-SingularBus", + src: `import "google/api/annotations.proto"; + +service BusService { + rpc BatchGetBus(BatchGetBusRequest) returns (BatchGetBusResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/buses:batchGet" + }; + } +} + +message BatchGetBusRequest {} + +message BatchGetBusResponse{} +`, + problems: testutils.Problems{{Message: `The resource part in method name "BatchGetBus" shouldn't "Bus", but should be its plural form "Buses"`}}, + }, + { + testName: "Invalid-SingularCorpPerson", + src: `import "google/api/annotations.proto"; + +service CorpPersonService { + rpc BatchGetCorpPerson(BatchGetCorpPersonRequest) returns (BatchGetCorpPersonResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/corpPerson:batchGet" + }; + } +} + +message BatchGetCorpPersonRequest {} + +message BatchGetCorpPersonResponse{} +`, + problems: testutils.Problems{{Message: `The resource part in method name "BatchGetCorpPerson" shouldn't "CorpPerson", but should be its plural form "CorpPeople"`}}, + }, + { + testName: "Invalid-Irrelevant", + src: `import "google/api/annotations.proto"; + +service BookService { + rpc GetBook(GetBookRequest) returns (Book) { + option (google.api.http) = { + get: "/v1/{name=publishers/*/books/*}" + }; + } +} + +message GetBookRequest {} + +message Book{} +`, + problems: testutils.Problems{}, + }, + } + + // Run each test individually. + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + file := testutils.ParseProto3String(t, test.src) + + m := file.GetServices()[0].GetMethods()[0] + + problems := pluralMethodResourceName.Lint(file) + if diff := test.problems.SetDescriptor(m).Diff(problems); diff != "" { + t.Errorf(diff) + } + }) + } +} + +func TestInputName(t *testing.T) { + // Set up the testing permutations. + tests := []struct { + testName string + src string + problems testutils.Problems + }{ + { + testName: "Valid-BatchGetBooksRequest", + src: `import "google/api/annotations.proto"; + +service BookService { + rpc BatchGetBooks(BatchGetBooksRequest) returns (BatchGetBooksResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/books:batchGet" + }; + } +} + +message BatchGetBooksRequest {} + +message BatchGetBooksResponse{} +`, + problems: testutils.Problems{}, + }, + { + testName: "Valid-BatchGetMenRequest", + src: `import "google/api/annotations.proto"; + +service ManService { + rpc BatchGetMen(BatchGetMenRequest) returns (BatchGetMenResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/men:batchGet" + }; + } +} + +message BatchGetMenRequest {} + +message BatchGetMenResponse{} +`, + problems: testutils.Problems{}, + }, + { + testName: "Invalid-SingularBus", + src: `import "google/api/annotations.proto"; + +service BusService { + rpc BatchGetBuses(BatchGetBusRequest) returns (BatchGetBusResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/buses:batchGet" + }; + } +} + +message BatchGetBusRequest {} + +message BatchGetBusResponse{} +`, + problems: testutils.Problems{{Message: `Batch Get RPCs should have a properly named request message "BatchGetBusesRequest", but not "BatchGetBusRequest"`}}, + }, + { + testName: "Invalid-SingularCorpPerson", + src: `import "google/api/annotations.proto"; + +service CorpPersonService { + rpc BatchGetCorpPerson(BatchGetCorpPersonRequest) returns (BatchGetCorpPersonResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/corpPerson:batchGet" + }; + } +} + +message BatchGetCorpPersonRequest {} + +message BatchGetCorpPersonResponse{} +`, + problems: testutils.Problems{{Message: `Batch Get RPCs should have a properly named request message "BatchGetCorpPeopleRequest", but not "BatchGetCorpPersonRequest"`}}, + }, + { + testName: "Irrelevant", + src: `import "google/api/annotations.proto"; + +service BookService { + rpc GetBook(GetBookRequest) returns (Book) { + option (google.api.http) = { + get: "/v1/{name=publishers/*/books/*}" + }; + } +} + +message GetBookRequest {} + +message Book{} +`, + problems: testutils.Problems{}, + }, + } + + // Run each test individually. + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + file := testutils.ParseProto3String(t, test.src) + + m := file.GetServices()[0].GetMethods()[0] + + problems := inputName.Lint(file) + if diff := test.problems.SetDescriptor(m).Diff(problems); diff != "" { + t.Errorf(diff) + } + }) + } +} + +func TestOutputName(t *testing.T) { + // Set up the testing permutations. + tests := []struct { + testName string + src string + problems testutils.Problems + }{ + { + testName: "Valid-BatchGetBooksResponse", + src: `import "google/api/annotations.proto"; + +service BookService { + rpc BatchGetBooks(BatchGetBooksRequest) returns (BatchGetBooksResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/books:batchGet" + }; + } +} + +message BatchGetBooksRequest {} + +message BatchGetBooksResponse{} +`, + problems: testutils.Problems{}, + }, + { + testName: "Valid-BatchGetBooksResponse", + src: `import "google/api/annotations.proto"; + +service ManService { + rpc BatchGetMen(BatchGetMenRequest) returns (BatchGetMenResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/men:batchGet" + }; + } +} + +message BatchGetMenRequest {} + +message BatchGetMenResponse{} +`, + problems: testutils.Problems{}, + }, + { + testName: "Invalid-SingularBus", + src: `import "google/api/annotations.proto"; + +service BusService { + rpc BatchGetBuses(BatchGetBusRequest) returns (BatchGetBusResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/buses:batchGet" + }; + } +} + +message BatchGetBusRequest {} + +message BatchGetBusResponse{} +`, + problems: testutils.Problems{{Message: `Batch Get RPCs should have a properly named response message "BatchGetBusesResponse", but not "BatchGetBusResponse"`}}, + }, + { + testName: "Invalid-SingularCorpPerson", + src: `import "google/api/annotations.proto"; + +service CorpPersonService { + rpc BatchGetCorpPerson(BatchGetCorpPersonRequest) returns (BatchGetCorpPersonResponse) { + option (google.api.http) = { + get: "/v1/{parent=publishers/*}/corpPerson:batchGet" + }; + } +} + +message BatchGetCorpPersonRequest {} + +message BatchGetCorpPersonResponse{} +`, + problems: testutils.Problems{{Message: `Batch Get RPCs should have a properly named response message "BatchGetCorpPeopleResponse", but not "BatchGetCorpPersonResponse"`}}, + }, + { + testName: "Irrelevant", + src: `import "google/api/annotations.proto"; + +service BookService { + rpc GetBook(GetBookRequest) returns (Book) { + option (google.api.http) = { + get: "/v1/{name=publishers/*/books/*}" + }; + } +} + +message GetBookRequest {} + +message Book{} +`, + problems: testutils.Problems{}, + }, + } + + // Run each test individually. + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + file := testutils.ParseProto3String(t, test.src) + + m := file.GetServices()[0].GetMethods()[0] + + problems := outputName.Lint(file) + if diff := test.problems.SetDescriptor(m).Diff(problems); diff != "" { + t.Errorf(diff) + } + }) + } +} + +func TestHttpVerb(t *testing.T) { + // Set up GET and POST HTTP annotations. + httpGet := &annotations.HttpRule{ + Pattern: &annotations.HttpRule_Get{ + Get: "/v1/{parent=publishers/*}/books:batchGet", + }, + } + httpPost := &annotations.HttpRule{ + Pattern: &annotations.HttpRule_Post{ + Post: "/v1/{name=publishers/*/books/*}", + }, + } + + // Set up testing permutations. + tests := []struct { + testName string + httpRule *annotations.HttpRule + methodName string + problems testutils.Problems + }{ + {"Valid", httpGet, "BatchGetBooks", nil}, + {"Invalid", httpPost, "BatchGetBooks", testutils.Problems{{Message: "Batch Get methods must use the HTTP GET verb."}}}, + {"Irrelevant", httpPost, "AcquireBook", nil}, + } + + // Run each test. + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + // Create a MethodOptions with the annotation set. + opts := &dpb.MethodOptions{} + if err := proto.SetExtension(opts, annotations.E_Http, test.httpRule); err != nil { + t.Fatalf("Failed to set google.api.http annotation.") + } + + // Create a minimal service with a AIP-231 Get method + // (or with a different method, in the "Irrelevant" case). + service, err := builder.NewService("BookService").AddMethod(builder.NewMethod(test.methodName, + builder.RpcTypeMessage(builder.NewMessage("BatchGetBooksRequest"), false), + builder.RpcTypeMessage(builder.NewMessage("BatchGetBooksResponse"), false), + ).SetOptions(opts)).Build() + if err != nil { + t.Fatalf("Could not build %s method.", test.methodName) + } + + // Run the method, ensure we get what we expect. + problems := httpVerb.Lint(service.GetFile()) + if diff := test.problems.SetDescriptor(service.GetMethods()[0]).Diff(problems); diff != "" { + t.Errorf(diff) + } + }) + } +} + +func TestHttpUrl(t *testing.T) { + tests := []struct { + testName string + uri string + methodName string + problems testutils.Problems + }{ + {"Valid", "/v1/{parent=publishers/*}/books:batchGet", "BatchGetBooks", nil}, + {"InvalidVarName", "/v1/{parent=publishers/*}/books", "BatchGetBooks", testutils.Problems{{Message: `Get methods URI should be end with ":batchGet".`}}}, + {"Irrelevant", "/v1/{book=publishers/*/books/*}", "AcquireBook", nil}, + } + + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + // Create a MethodOptions with the annotation set. + opts := &dpb.MethodOptions{} + httpRule := &annotations.HttpRule{ + Pattern: &annotations.HttpRule_Get{ + Get: test.uri, + }, + } + if err := proto.SetExtension(opts, annotations.E_Http, httpRule); err != nil { + t.Fatalf("Failed to set google.api.http annotation.") + } + + // Create a minimal service with a AIP-231 Get method + // (or with a different method, in the "Irrelevant" case). + service, err := builder.NewService("BookService").AddMethod(builder.NewMethod(test.methodName, + builder.RpcTypeMessage(builder.NewMessage("BatchGetBooksRequest"), false), + builder.RpcTypeMessage(builder.NewMessage("BatchGetBooksResponse"), false), + ).SetOptions(opts)).Build() + if err != nil { + t.Fatalf("Could not build %s method.", test.methodName) + } + + // Run the method, ensure we get what we expect. + problems := httpUrl.Lint(service.GetFile()) + if diff := test.problems.SetDescriptor(service.GetMethods()[0]).Diff(problems); diff != "" { + t.Errorf(diff) + } + }) + } +} + +func TestHttpBody(t *testing.T) { + tests := []struct { + testName string + body string + methodName string + problems testutils.Problems + }{ + {"Valid", "", "BatchGetBooks", nil}, + {"Invalid", "*", "BatchGetBooks", testutils.Problems{{Message: "Batch Get methods should not have an HTTP body."}}}, + {"Irrelevant", "*", "AcquireBook", nil}, + } + + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + // Create a MethodOptions with the annotation set. + opts := &dpb.MethodOptions{} + httpRule := &annotations.HttpRule{ + Pattern: &annotations.HttpRule_Get{ + Get: "/v1/{name=publishers/*/books/*}", + }, + Body: test.body, + } + if err := proto.SetExtension(opts, annotations.E_Http, httpRule); err != nil { + t.Fatalf("Failed to set google.api.http annotation.") + } + + // Create a minimal service with a AIP-231 Get method + // (or with a different method, in the "Irrelevant" case). + service, err := builder.NewService("BookService").AddMethod(builder.NewMethod(test.methodName, + builder.RpcTypeMessage(builder.NewMessage("BatchGetBooksRequest"), false), + builder.RpcTypeMessage(builder.NewMessage("BatchGetBooksResponse"), false), + ).SetOptions(opts)).Build() + if err != nil { + t.Fatalf("Could not build %s method.", test.methodName) + } + + // Run the method, ensure we get what we expect. + problems := httpBody.Lint(service.GetFile()) + if diff := test.problems.SetDescriptor(service.GetMethods()[0]).Diff(problems); diff != "" { + t.Errorf(diff) + } + }) + } +}