From ef21d6d36fc8a21e088be52b83d081d6d2510a37 Mon Sep 17 00:00:00 2001 From: noahdietz Date: Fri, 26 May 2023 14:43:18 -0700 Subject: [PATCH 1/2] fix(AIP-158): reference resource plural for field name --- rules/aip0158/response_plural_first_field.go | 12 +++++- .../response_plural_first_field_test.go | 38 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/rules/aip0158/response_plural_first_field.go b/rules/aip0158/response_plural_first_field.go index b35ce3efb..1a43a3d90 100644 --- a/rules/aip0158/response_plural_first_field.go +++ b/rules/aip0158/response_plural_first_field.go @@ -19,6 +19,7 @@ import ( "github.com/googleapis/api-linter/locations" "github.com/googleapis/api-linter/rules/internal/utils" "github.com/jhump/protoreflect/desc" + "github.com/stoewer/go-strcase" ) var responsePluralFirstField = &lint.MessageRule{ @@ -30,7 +31,16 @@ var responsePluralFirstField = &lint.MessageRule{ // Throw a linter warning if, the first field in the message is not named // according to plural(message_name.to_snake().split('_')[1:-1]). firstField := m.GetFields()[0] - want := utils.ToPlural(firstField.GetName()) + + // If the field is a resource, use the `plural` annotation to decide the + // appropriate plural field name. + var want string + if res := utils.GetResource(firstField.GetMessageType()); res != nil && res.GetPlural() != "" { + want = strcase.SnakeCase(res.GetPlural()) + } else { + want = utils.ToPlural(firstField.GetName()) + } + if want != firstField.GetName() { return []lint.Problem{{ Message: "First field of Paginated RPCs' response should be plural.", diff --git a/rules/aip0158/response_plural_first_field_test.go b/rules/aip0158/response_plural_first_field_test.go index 42a1ad633..3db6334e0 100644 --- a/rules/aip0158/response_plural_first_field_test.go +++ b/rules/aip0158/response_plural_first_field_test.go @@ -55,6 +55,44 @@ func TestResponsePluralFirstField(t *testing.T) { }) } + tests = []struct { + testName string + MessageName string + problems testutils.Problems + }{ + {"ValidResourcePlural", "library_books", testutils.Problems{}}, + {"InvalidResourcePlural", "books", testutils.Problems{{Suggestion: "library_books"}}}, + } + for _, test := range tests { + t.Run(test.testName, func(t *testing.T) { + // Create the proto message. + f := testutils.ParseProto3Tmpl(t, ` + import "google/api/resource.proto"; + + message LibraryBook { + option (google.api.resource) = { + type: "example.com/LibraryBook" + pattern: "libraryBooks/{libraryBook}" + singular: "libraryBook" + plural: "libraryBooks" + }; + string name = 1; + } + + message ListLibraryBooksResponse { + repeated LibraryBook {{.MessageName}} = 1; + string next_page_token = 2; + } + `, test) + + // Run the lint rule and establish we get the correct problems. + problems := responsePluralFirstField.Lint(f) + if diff := test.problems.SetDescriptor(f.GetMessageTypes()[1].FindFieldByNumber(1)).Diff(problems); diff != "" { + t.Errorf(diff) + } + }) + } + t.Run("ValidNoFields", func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` message ListStudentProfilesResponse {} From 017c668ba038c8afb38c27e5a1c0333caf38a28c Mon Sep 17 00:00:00 2001 From: noahdietz Date: Fri, 26 May 2023 18:40:03 -0700 Subject: [PATCH 2/2] address feedback --- rules/aip0158/response_plural_first_field.go | 6 ++-- rules/internal/utils/resource.go | 14 +++++++- rules/internal/utils/resource_test.go | 35 +++++++++++++++++++- 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/rules/aip0158/response_plural_first_field.go b/rules/aip0158/response_plural_first_field.go index 1a43a3d90..97f6777f1 100644 --- a/rules/aip0158/response_plural_first_field.go +++ b/rules/aip0158/response_plural_first_field.go @@ -34,9 +34,9 @@ var responsePluralFirstField = &lint.MessageRule{ // If the field is a resource, use the `plural` annotation to decide the // appropriate plural field name. - var want string - if res := utils.GetResource(firstField.GetMessageType()); res != nil && res.GetPlural() != "" { - want = strcase.SnakeCase(res.GetPlural()) + want := utils.GetResourcePlural(utils.GetResource(firstField.GetMessageType())) + if want != "" { + want = strcase.SnakeCase(want) } else { want = utils.ToPlural(firstField.GetName()) } diff --git a/rules/internal/utils/resource.go b/rules/internal/utils/resource.go index 72191cf8c..8e6e8bee6 100644 --- a/rules/internal/utils/resource.go +++ b/rules/internal/utils/resource.go @@ -14,7 +14,9 @@ package utils -import apb "google.golang.org/genproto/googleapis/api/annotations" +import ( + apb "google.golang.org/genproto/googleapis/api/annotations" +) // GetResourceSingular returns the resource singular. The // empty string is returned if the singular cannot be found. @@ -37,3 +39,13 @@ func GetResourceSingular(r *apb.ResourceDescriptor) string { } return "" } + +// GetResourcePlural is a convenience method for getting the `plural` field of a +// resource. +func GetResourcePlural(r *apb.ResourceDescriptor) string { + if r == nil { + return "" + } + + return r.Plural +} diff --git a/rules/internal/utils/resource_test.go b/rules/internal/utils/resource_test.go index 5f276733b..cc38b2e59 100644 --- a/rules/internal/utils/resource_test.go +++ b/rules/internal/utils/resource_test.go @@ -20,7 +20,7 @@ import ( apb "google.golang.org/genproto/googleapis/api/annotations" ) -func TestMatches(t *testing.T) { +func TestGetResourceSingular(t *testing.T) { for _, test := range []struct { name string resource *apb.ResourceDescriptor @@ -69,3 +69,36 @@ func TestMatches(t *testing.T) { }) } } + +func TestGetResourcePlural(t *testing.T) { + for _, test := range []struct { + name string + resource *apb.ResourceDescriptor + want string + }{ + { + name: "PluralSpecified", + resource: &apb.ResourceDescriptor{ + Plural: "bookShelves", + }, + want: "bookShelves", + }, + { + name: "NothingSpecified", + resource: &apb.ResourceDescriptor{}, + want: "", + }, + { + name: "Nil", + resource: nil, + want: "", + }, + } { + t.Run(test.name, func(t *testing.T) { + got := GetResourcePlural(test.resource) + if got != test.want { + t.Errorf("GetResourcePlural: expected %v, got %v", test.want, got) + } + }) + } +}