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.

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(AIP-158): reference resource plural for field name #1158

Merged
merged 3 commits into from
May 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 11 additions & 1 deletion rules/aip0158/response_plural_first_field.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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.
want := utils.GetResourcePlural(utils.GetResource(firstField.GetMessageType()))
if want != "" {
want = strcase.SnakeCase(want)
} else {
want = utils.ToPlural(firstField.GetName())
Copy link
Contributor

Choose a reason for hiding this comment

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

this already existed of course, but I feel like pluralizing the resource singular or the type would be more robust than the field name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is meant to support the scenario where the paginated field isn't a resource. I agree with you, but I don't know if we should change it right now. There are maybe one or two Standard List-like fields that paginate a repeated string field that I know of. This rule being in AIP-158, not AIP-132 is poignant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opened #1160

}

if want != firstField.GetName() {
return []lint.Problem{{
Message: "First field of Paginated RPCs' response should be plural.",
Expand Down
38 changes: 38 additions & 0 deletions rules/aip0158/response_plural_first_field_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand Down
14 changes: 13 additions & 1 deletion rules/internal/utils/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
}
35 changes: 34 additions & 1 deletion rules/internal/utils/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
})
}
}