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

Jump to bottom

fix(AIP-158): reference resource plural for field name #1158

Merged
merged 3 commits into from
May 27, 2023
Merged

fix(AIP-158): reference resource plural for field name #1158

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef21d6d
fix(AIP-158): reference resource plural for field name
noahdietz May 26, 2023
e3f1180
Merge branch 'main' into first-plural
noahdietz May 27, 2023
017c668
address feedback
noahdietz May 27, 2023
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
View file
Open in desktop
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.
var want string
if res := utils.GetResource(firstField.GetMessageType()); res != nil && res.GetPlural() != "" {
noahdietz marked this conversation as resolved.
Show resolved Hide resolved
want = strcase.SnakeCase(res.GetPlural())
} 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
View file
Open in desktop
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