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

Change AEP-158 to look for any repeated field #84

Merged
merged 2 commits into from
Aug 22, 2024
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
Original file line number Diff line number Diff line change
@@ -1,25 +1,24 @@
---
rule:
aep: 158
name: [core, '0158', response-repeated-first-field]
name: [core, '0158', response-repeated-field]
summary: |
First field (by both position and field number) of Paginated RPCs' response
One field of a paginated RPCs' response
should be repeated.
permalink: /158/response-repeated-first-field
permalink: /158/response-repeated-field
redirect_from:
- /0158/response-repeated-first-field
- /0158/response-repeated-field
---

# Paginated methods: Page token field

This rule enforces that all `List` and `Search` methods have a repeatable field
as a first field in the response message, as mandated in [AEP-158][].
in the response message, as mandated in [AEP-158][].

## Details

This rule looks at any message matching `List*Response` or `Search*Response`
that has `next_page_token` field and complains if the first field (by both
position and field number) is not repeated.
that has `next_page_token` field and complains if there does not exist a field that is repeated.

## Examples

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,21 @@ import (
)

var responseRepeatedFirstField = &lint.MessageRule{
Name: lint.NewRuleName(158, "response-repeated-first-field"),
Name: lint.NewRuleName(158, "response-repeated-field"),
RuleType: lint.NewRuleType(lint.MustRule),
OnlyIf: func(m *desc.MessageDescriptor) bool {
return isPaginatedResponseMessage(m) && len(m.GetFields()) > 0
},
LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
// Sanity check: Is the first field (positionally) and the field with
// an ID of 1 actually the same field?
if m.GetFields()[0] != m.FindFieldByNumber(1) {
return []lint.Problem{{
Message: "The first field of paginated RPCs must have a protobuf ID of 1.",
Descriptor: m.GetFields()[0],
}}
for _, f := range m.GetFields() {
if(f.IsRepeated()) {
return nil;
}
}

// Make sure the field is repeated.
if !m.GetFields()[0].IsRepeated() {
return []lint.Problem{{
Message: "The first field of a paginated response should be repeated.",
Descriptor: m.GetFields()[0],
}}
}

return nil
return []lint.Problem{{
Message: "There does not exist a repeated field for pagination results.",
Descriptor: m,
}}
},
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func TestResponseRepeatedFirstField(t *testing.T) {
}{
{"Valid", "repeated Book books = 1;", "string next_page_token = 2;", nil},
{"SkippedZeroFields", "", "", nil},
{"InvalidFirstFieldsNoMatch", "string next_page_token = 2;", "repeated Book books = 1;", testutils.Problems{{Message: "protobuf ID"}}},
{"ProtobufIdIsNotOne", "string next_page_token = 1;", "repeated Book books = 2;", nil},
{"InvalidNotRepeated", "Book book = 1;", "string next_page_token = 2;", testutils.Problems{{Message: "repeated"}}},
}
for _, test := range tests {
Expand All @@ -45,14 +45,9 @@ func TestResponseRepeatedFirstField(t *testing.T) {
}
`, test)

// Determine the descriptor that a failing test will attach to.
if m := f.GetMessageTypes()[1]; len(m.GetFields()) > 0 {
test.problems.SetDescriptor(m.GetFields()[0])
}

// Run the lint rule and establish we get the correct problems.
problems := responseRepeatedFirstField.Lint(f)
if diff := test.problems.Diff(problems); diff != "" {
if diff := test.problems.SetDescriptor(f.GetMessageTypes()[1]).Diff(problems); diff != "" {
t.Errorf(diff)
}
})
Expand Down
Loading