diff --git a/rules/aip0136/http_uri_suffix.go b/rules/aip0136/http_uri_suffix.go index 99477941f..fb28bd21f 100644 --- a/rules/aip0136/http_uri_suffix.go +++ b/rules/aip0136/http_uri_suffix.go @@ -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 diff --git a/rules/aip0231/aip0231.go b/rules/aip0231/aip0231.go index 2811f0312..fa6dc8b8b 100644 --- a/rules/aip0231/aip0231.go +++ b/rules/aip0231/aip0231.go @@ -32,8 +32,8 @@ func AddRules(r lint.RuleRegistry) error { outputName, httpBody, httpVerb, - parentField, namesField, + requestParentField, resourceField, uriSuffix, ) diff --git a/rules/aip0231/request_parent_field.go b/rules/aip0231/request_parent_field.go index dc09bd175..c8fa8dbb4 100644 --- a/rules/aip0231/request_parent_field.go +++ b/rules/aip0231/request_parent_field.go @@ -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 + } + } + } + } + } + + return isBatchGetRequestMessage(m) + }, LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { // Rule check: Establish that a `parent` field is present. parentField := m.FindFieldByName("parent") diff --git a/rules/aip0231/request_parent_field_test.go b/rules/aip0231/request_parent_field_test.go index 4009ab112..03e644173 100644 --- a/rules/aip0231/request_parent_field_test.go +++ b/rules/aip0231/request_parent_field_test.go @@ -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. @@ -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) } }) diff --git a/rules/aip0233/request_parent_field.go b/rules/aip0233/request_parent_field.go index d1b33b330..8d61f4274 100644 --- a/rules/aip0233/request_parent_field.go +++ b/rules/aip0233/request_parent_field.go @@ -16,23 +16,47 @@ 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, }} } @@ -40,7 +64,7 @@ var requestParentField = &lint.MessageRule{ // 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", diff --git a/rules/aip0233/request_parent_field_test.go b/rules/aip0233/request_parent_field_test.go index 0d9c5e6fc..b18b8b8f0 100644 --- a/rules/aip0233/request_parent_field_test.go +++ b/rules/aip0233/request_parent_field_test.go @@ -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. @@ -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) } }) diff --git a/rules/aip0234/request_parent_field.go b/rules/aip0234/request_parent_field.go index 817cec858..3db8f5fdd 100644 --- a/rules/aip0234/request_parent_field.go +++ b/rules/aip0234/request_parent_field.go @@ -16,23 +16,47 @@ package aip0234 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 Update request message should have parent field. var requestParentField = &lint.MessageRule{ - Name: lint.NewRuleName(234, "request-parent-field"), - OnlyIf: isBatchUpdateRequestMessage, + Name: lint.NewRuleName(234, "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"), "BatchUpdate") + if resp := m.GetFile().FindMessage(fmt.Sprintf("BatchUpdate%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 isBatchUpdateRequestMessage(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, }} } @@ -40,7 +64,7 @@ var requestParentField = &lint.MessageRule{ // Rule check: Establish that the parent field is a string. if parentField.GetType() != builder.FieldTypeString().GetType() { return []lint.Problem{{ - Message: `"parent" field on Update request message must be a string`, + Message: "`parent` field on Update request message must be a string.", Descriptor: parentField, Location: locations.FieldType(parentField), Suggestion: "string", diff --git a/rules/aip0234/request_parent_field_test.go b/rules/aip0234/request_parent_field_test.go index 284c1d73b..360e1c4a4 100644 --- a/rules/aip0234/request_parent_field_test.go +++ b/rules/aip0234/request_parent_field_test.go @@ -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. @@ -21,65 +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 +func TestParentField(t *testing.T) { + for _, test := range []struct { + name string + RPC string + Field string + Pattern string + problems testutils.Problems }{ - { - "Valid", - "BatchUpdateBooksRequest", - "string parent", - testutils.Problems{}, - }, - { - "MissingField", - "BatchUpdateBooksRequest", - "string id", - testutils.Problems{{Message: "parent"}}, - }, - { - "InvalidType", - "BatchUpdateBooksRequest", - "int32 parent", - testutils.Problems{{ - Message: "string", - Suggestion: "string", - }}, - }, - { - "Irrelevant", - "EnumerateBooksRequest", - "string id", - testutils.Problems{}, - }, - } + {"Valid", "BatchUpdateBooks", "string parent = 1;", "publishers/{p}/books/{b}", nil}, + {"Missing", "BatchUpdateBooks", "", "publishers/{p}/books/{b}", testutils.Problems{{Message: "no `parent`"}}}, + {"InvalidType", "BatchUpdateBooks", "int32 parent = 1;", "publishers/{p}/books/{b}", testutils.Problems{{Suggestion: "string"}}}, + {"IrrelevantRPCName", "EnumerateBooks", "", "publishers/{p}/books/{b}", nil}, + {"IrrelevantNoParent", "BatchUpdateBooks", "", "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; - repeated UpdateBookRequest requests = 2; + service Library { + rpc {{.RPC}}({{.RPC}}Request) returns ({{.RPC}}Response); } - message UpdateBookRequest{} - `, test) - // Determine the descriptor that a failing test will attach to. - var problemDesc desc.Descriptor - if parent := file.GetMessageTypes()[0].FindFieldByName("parent"); parent != nil { - problemDesc = parent - } else { - problemDesc = file.GetMessageTypes()[0] - } + message {{.RPC}}Request { + {{.Field}} + repeated string names = 2; + } - // 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 {{.RPC}}Response { + repeated Book books = 1; + } + + 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) } })