Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Permit top-level resources to have batch methods. #619

Merged
merged 2 commits into from
Sep 15, 2020
Merged

fix: Permit top-level resources to have batch methods. #619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2c3f53
fix: Permit top-level resources to have batch methods.
Sep 2, 2020
b3cfa27
fix: Also do the right thing on batch methods in AIP-136.
Sep 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions rules/aip0136/http_uri_suffix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,12 @@ var uriSuffix = &lint.MethodRule{
// There are some known exceptions, particularly around "Batch".
// ----------------------------------------------------------------------
// If the URI contains `{name=` or `{parent=`, expect `:verb`.
if strings.Contains(httpRule.URI, "{name=") || strings.Contains(httpRule.URI, "{parent=") {
if strings.Contains(httpRule.URI, ":batch") {
rpcSlice := strings.Split(strcase.SnakeCase(m.GetName()), "_")
if rpcSlice[0] == "batch" {
want = ":" + strcase.LowerCamelCase(rpcSlice[0]+"_"+rpcSlice[1])
} else {
want = ":" + rpcSlice[0]
}
want = ":" + strcase.LowerCamelCase(rpcSlice[0]+"_"+rpcSlice[1])
} else if strings.Contains(httpRule.URI, "{name=") || strings.Contains(httpRule.URI, "{parent=") {
rpcSlice := strings.Split(strcase.SnakeCase(m.GetName()), "_")
want = ":" + rpcSlice[0]
}

// AIP-162 introduces some special cases around revisions, where
Expand Down
2 changes: 1 addition & 1 deletion rules/aip0231/aip0231.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ func AddRules(r lint.RuleRegistry) error {
outputName,
httpBody,
httpVerb,
parentField,
namesField,
requestParentField,
resourceField,
uriSuffix,
)
Expand Down
30 changes: 27 additions & 3 deletions rules/aip0231/request_parent_field.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,41 @@ package aip0231

import (
"fmt"
"strings"

"github.com/googleapis/api-linter/lint"
"github.com/googleapis/api-linter/locations"
"github.com/googleapis/api-linter/rules/internal/utils"
"github.com/jhump/protoreflect/desc"
"github.com/jhump/protoreflect/desc/builder"
"github.com/stoewer/go-strcase"
)

// The Batch Get request message should have parent field.
var parentField = &lint.MessageRule{
Name: lint.NewRuleName(231, "request-parent-field"),
OnlyIf: isBatchGetRequestMessage,
var requestParentField = &lint.MessageRule{
Name: lint.NewRuleName(231, "request-parent-field"),
OnlyIf: func(m *desc.MessageDescriptor) bool {
// Sanity check: If the resource has a pattern, and that pattern
// contains only one variable, then a parent field is not expected.
//
// In order to parse out the pattern, we get the resource message
// from the response, then get the resource annotation from that,
// and then inspect the pattern there (oy!).
plural := strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Request"), "BatchGet")
if resp := m.GetFile().FindMessage(fmt.Sprintf("BatchGet%sResponse", plural)); resp != nil {
if paged := resp.FindFieldByName(strcase.SnakeCase(plural)); paged != nil {
if resource := utils.GetResource(paged.GetMessageType()); resource != nil {
for _, pattern := range resource.GetPattern() {
if strings.Count(pattern, "{") == 1 {
return false
}
}
}
}
}
Comment on lines +39 to +50
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this logic (which is used for BatchGet, BatchCreate, and BatchUpdate) be pulled out into a function that takes the "batching method type" as param i.e. "BatchGet", as well as the necessary proto input i.e. m *desc.MessageDescriptor? This code is non-trivial and used in three places, feels ripe for a func. WDYT? If there is a nuance in the code for each that prevents this that I missed, lmk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be, although the code is slightly different each time, and I am not sure there is a good place to put it. I do not know that it is common enough to justify going in utils. This is right in the sour spot where the function can not be in the AIP modules themselves, but are not generic enough for me to be convinced that they belong in utils.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I understand. Bummer!


return isBatchGetRequestMessage(m)
},
LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
// Rule check: Establish that a `parent` field is present.
parentField := m.FindFieldByName("parent")
Expand Down
103 changes: 37 additions & 66 deletions rules/aip0231/request_parent_field_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2019 Google LLC
// Copyright 2020 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -19,80 +19,51 @@ import (

"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
for _, test := range []struct {
name string
RPC string
Field string
Pattern string
problems testutils.Problems
}{
{
"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{{Suggestion: "string"}},
func(m *desc.MessageDescriptor) desc.Descriptor {
return m.FindFieldByName("parent")
},
},
{
"Irrelevant",
"EnumerateBooksRequest",
&field{"id", builder.FieldTypeString()},
testutils.Problems{},
nil,
},
}
{"Valid", "BatchGetBooks", "string parent = 1;", "publishers/{p}/books/{b}", nil},
{"Missing", "BatchGetBooks", "", "publishers/{p}/books/{b}", testutils.Problems{{Message: "no `parent`"}}},
{"InvalidType", "BatchGetBooks", "int32 parent = 1;", "publishers/{p}/books/{b}", testutils.Problems{{Suggestion: "string"}}},
{"IrrelevantRPCName", "EnumerateBooks", "", "publishers/{p}/books/{b}", nil},
{"IrrelevantNoParent", "BatchGetBooks", "", "books/{b}", nil},
} {
t.Run(test.name, func(t *testing.T) {
f := testutils.ParseProto3Tmpl(t, `
import "google/api/resource.proto";

// 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)
service Library {
rpc {{.RPC}}({{.RPC}}Request) returns ({{.RPC}}Response);
}

if test.messageField != nil {
messageBuilder.AddField(
builder.NewField(test.messageField.fieldName, test.messageField.fieldType),
)
}
message {{.RPC}}Request {
{{.Field}}
repeated string names = 2;
}

message, err := messageBuilder.Build()
if err != nil {
t.Fatalf("Could not build %s message.", test.messageName)
}
message {{.RPC}}Response {
repeated Book books = 1;
}

// Determine the descriptor that a failing test will attach to.
var problemDesc desc.Descriptor = message
if test.problemDesc != nil {
problemDesc = test.problemDesc(message)
message Book {
option (google.api.resource) = {
pattern: "{{.Pattern}}";
};
string name = 1;
}
`, test)
var d desc.Descriptor = f.GetMessageTypes()[0]
if test.name == "InvalidType" {
d = f.GetMessageTypes()[0].GetFields()[0]
}

// 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 != "" {
if diff := test.problems.SetDescriptor(d).Diff(requestParentField.Lint(f)); diff != "" {
t.Errorf(diff)
}
})
Expand Down
32 changes: 28 additions & 4 deletions rules/aip0233/request_parent_field.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,55 @@ package aip0233

import (
"fmt"
"strings"

"github.com/googleapis/api-linter/lint"
"github.com/googleapis/api-linter/locations"
"github.com/googleapis/api-linter/rules/internal/utils"
"github.com/jhump/protoreflect/desc"
"github.com/jhump/protoreflect/desc/builder"
"github.com/stoewer/go-strcase"
)

// The Batch Create request message should have parent field.
var requestParentField = &lint.MessageRule{
Name: lint.NewRuleName(233, "request-parent-field"),
OnlyIf: isBatchCreateRequestMessage,
Name: lint.NewRuleName(233, "request-parent-field"),
OnlyIf: func(m *desc.MessageDescriptor) bool {
// Sanity check: If the resource has a pattern, and that pattern
// contains only one variable, then a parent field is not expected.
//
// In order to parse out the pattern, we get the resource message
// from the response, then get the resource annotation from that,
// and then inspect the pattern there (oy!).
plural := strings.TrimPrefix(strings.TrimSuffix(m.GetName(), "Request"), "BatchCreate")
if resp := m.GetFile().FindMessage(fmt.Sprintf("BatchCreate%sResponse", plural)); resp != nil {
if paged := resp.FindFieldByName(strcase.SnakeCase(plural)); paged != nil {
if resource := utils.GetResource(paged.GetMessageType()); resource != nil {
for _, pattern := range resource.GetPattern() {
if strings.Count(pattern, "{") == 1 {
return false
}
}
}
}
}

return isBatchCreateRequestMessage(m)
},
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()),
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`,
Message: "`parent` field on create request message must be a string.",
Descriptor: parentField,
Location: locations.FieldType(parentField),
Suggestion: "string",
Expand Down
97 changes: 39 additions & 58 deletions rules/aip0233/request_parent_field_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2019 Google LLC
// Copyright 2020 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -21,68 +21,49 @@ import (
"github.com/jhump/protoreflect/desc"
)

func TestRequestParentField(t *testing.T) {
// Set up the testing permutations.
tests := []struct {
testName string
MessageName string
Field string
problems testutils.Problems
problemDesc func(m *desc.MessageDescriptor) desc.Descriptor
func TestParentField(t *testing.T) {
for _, test := range []struct {
name string
RPC string
Field string
Pattern string
problems testutils.Problems
}{
{
"Valid",
"BatchCreateBooksRequest",
"string parent",
testutils.Problems{},
nil},
{
"MissingField",
"BatchCreateBooksRequest",
"string name",
testutils.Problems{{Message: "parent"}},
nil,
},
{
"InvalidType",
"BatchCreateBooksRequest",
"int32 parent",
testutils.Problems{{
Message: "string",
Suggestion: "string",
}},
func(m *desc.MessageDescriptor) desc.Descriptor {
return m.FindFieldByName("parent")
},
},
{
"Irrelevant",
"EnumerateBooksRequest",
"string name",
testutils.Problems{},
nil,
},
}
{"Valid", "BatchCreateBooks", "string parent = 1;", "publishers/{p}/books/{b}", nil},
{"Missing", "BatchCreateBooks", "", "publishers/{p}/books/{b}", testutils.Problems{{Message: "no `parent`"}}},
{"InvalidType", "BatchCreateBooks", "int32 parent = 1;", "publishers/{p}/books/{b}", testutils.Problems{{Suggestion: "string"}}},
{"IrrelevantRPCName", "EnumerateBooks", "", "publishers/{p}/books/{b}", nil},
{"IrrelevantNoParent", "BatchCreateBooks", "", "books/{b}", nil},
} {
t.Run(test.name, func(t *testing.T) {
f := testutils.ParseProto3Tmpl(t, `
import "google/api/resource.proto";

// Run each test individually.
for _, test := range tests {
t.Run(test.testName, func(t *testing.T) {
file := testutils.ParseProto3Tmpl(t, `
message {{.MessageName}} {
{{.Field}} = 1;
}`, test)
service Library {
rpc {{.RPC}}({{.RPC}}Request) returns ({{.RPC}}Response);
}

m := file.GetMessageTypes()[0]
message {{.RPC}}Request {
{{.Field}}
repeated string names = 2;
}

// Determine the descriptor that a failing test will attach to.
var problemDesc desc.Descriptor = m
if test.problemDesc != nil {
problemDesc = test.problemDesc(m)
}
message {{.RPC}}Response {
repeated Book books = 1;
}

// Run the lint rule, and establish that it returns the correct problems.
problems := requestParentField.Lint(file)
if diff := test.problems.SetDescriptor(problemDesc).Diff(problems); diff != "" {
message Book {
option (google.api.resource) = {
pattern: "{{.Pattern}}";
};
string name = 1;
}
`, test)
var d desc.Descriptor = f.GetMessageTypes()[0]
if test.name == "InvalidType" {
d = f.GetMessageTypes()[0].GetFields()[0]
}
if diff := test.problems.SetDescriptor(d).Diff(requestParentField.Lint(f)); diff != "" {
t.Errorf(diff)
}
})
Expand Down
Loading