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

Fixes against AEPC #93

Merged
merged 5 commits into from
Sep 19, 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
8 changes: 4 additions & 4 deletions docs/rules/0132/request-unknown-fields.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ This rule looks at any message matching `List*Request` and complains if it
comes across any fields other than:

- `string parent` ([AEP-132][])
- `int32 page_size` ([AEP-158][])
- `int32 max_page_size` ([AEP-158][])
- `string page_token` ([AEP-158][])
- `int32 skip` ([AEP-158][])
- `string filter` ([AEP-132][])
Expand All @@ -41,7 +41,7 @@ handled by other rules, such as
// Incorrect.
message ListBooksRequest {
string parent = 1;
int32 page_size = 2;
int32 max_page_size = 2;
string page_token = 3;
string library_id = 4; // Non-standard field.
}
Expand All @@ -53,7 +53,7 @@ message ListBooksRequest {
// Correct.
message ListBooksRequest {
string parent = 1;
int32 page_size = 2;
int32 max_page_size = 2;
string page_token = 3;
}
```
Expand All @@ -66,7 +66,7 @@ Remember to also include an [aep.dev/not-precedent][] comment explaining why.
```proto
message ListBooksRequest {
string parent = 1;
int32 page_size = 2;
int32 max_page_size = 2;
string page_token = 3;

// (-- api-linter: core::0132::request-unknown-fields=disabled
Expand Down
6 changes: 3 additions & 3 deletions docs/rules/0132/response-unknown-fields.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ repeated-ness.
```proto
// Incorrect.
message ListBooksResponse {
repeated Book books = 1;
repeated Book results = 1;
string next_page_token = 2;
string publisher_id = 3; // Unrecognized field.
}
Expand All @@ -44,7 +44,7 @@ message ListBooksResponse {
```proto
// Correct.
message ListBooksResponse {
repeated Book books = 1;
repeated Book results = 1;
string next_page_token = 2;
}
```
Expand All @@ -56,7 +56,7 @@ Remember to also include an [aep.dev/not-precedent][] comment explaining why.

```proto
message ListBooksResponse {
repeated Book books = 1;
repeated Book results = 1;
string next_page_token = 2;
// (-- api-linter: core::0132::response-unknown-fields=disabled
// aep.dev/not-precedent: We really need this field because reasons. --)
Expand Down
6 changes: 3 additions & 3 deletions docs/rules/0134/http-uri-path.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ separation uses `snake_case`.
// Incorrect.
rpc UpdateBookRequest(UpdateBookRequest) returns (Book) {
option (google.api.http) = {
post: "/v1/{path=publishers/*/books/*}" // Should be `book.path`.
post: "/v1/{book.path=publishers/*/books/*}" // Should be `path`
body: "book"
};
}
Expand All @@ -42,7 +42,7 @@ rpc UpdateBookRequest(UpdateBookRequest) returns (Book) {
// Correct.
rpc UpdateBookRequest(UpdateBookRequest) returns (Book) {
option (google.api.http) = {
post: "/v1/{book.path=publishers/*/books/*}"
post: "/v1/{path=publishers/*/books/*}"
body: "book"
};
}
Expand All @@ -58,7 +58,7 @@ Remember to also include an [aep.dev/not-precedent][] comment explaining why.
// aep.dev/not-precedent: We need to do this because reasons. --)
rpc UpdateBookRequest(UpdateBookRequest) returns (Book) {
option (google.api.http) = {
post: "/v1/{path=publishers/*/books/*}"
post: "/v1/{book.path=publishers/*/books/*}"
body: "book"
};
}
Expand Down
5 changes: 3 additions & 2 deletions docs/rules/0134/request-required-fields.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@ message UpdateBookRequest {
```proto
// Correct.
message UpdateBookRequest {
Book book = 1 [(google.api.field_behavior) = REQUIRED];
bool allow_missing = 2 [(google.api.field_behavior) = OPTIONAL];
string path = 1 [(google.api.field_behavior) = REQUIRED];
Book book = 2 [(google.api.field_behavior) = REQUIRED];
bool allow_missing = 3 [(google.api.field_behavior) = OPTIONAL];
}
```

Expand Down
2 changes: 1 addition & 1 deletion rules/aep0132/request_unknown_fields.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (

var allowedFields = stringset.New(
"parent", // AEP-132
"page_size", // AEP-158
"max_page_size", // AEP-158
"page_token", // AEP-158
"skip", // AEP-158
"filter", // AEP-132
Expand Down
2 changes: 1 addition & 1 deletion rules/aep0132/request_unknown_fields_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func TestUnknownFields(t *testing.T) {
fieldType *builder.FieldType
problems testutils.Problems
}{
{"PageSize", "ListBooksRequest", "page_size", builder.FieldTypeInt32(), testutils.Problems{}},
{"PageSize", "ListBooksRequest", "max_page_size", builder.FieldTypeInt32(), testutils.Problems{}},
{"PageToken", "ListBooksRequest", "page_token", builder.FieldTypeString(), testutils.Problems{}},
{"Skip", "ListBooksRequest", "skip", builder.FieldTypeInt32(), testutils.Problems{}},
{"Filter", "ListBooksRequest", "filter", builder.FieldTypeString(), testutils.Problems{}},
Expand Down
17 changes: 1 addition & 16 deletions rules/aep0132/response_unknown_fields.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
package aep0132

import (
"strings"

"bitbucket.org/creachadair/stringset"
"github.com/aep-dev/api-linter/lint"
"github.com/aep-dev/api-linter/rules/internal/utils"
Expand All @@ -26,6 +24,7 @@ import (
// The resource itself is not included here, but also permitted.
// This is covered in code in the rule itself.
var respAllowedFields = stringset.New(
"results",
"max_page_size", // AEP-132
"next_page_token", // AEP-158
"total_size", // AEP-132
Expand All @@ -40,20 +39,6 @@ var responseUnknownFields = &lint.FieldRule{
return utils.IsListResponseMessage(f.GetOwner())
},
LintField: func(f *desc.FieldDescriptor) []lint.Problem {
// A repeated variant of the resource should be permitted.
resource := utils.ListResponseResourceName(f.GetOwner())
if strings.HasSuffix(resource, "_revisions") {
// This is an AEP-162 ListFooRevisions response, which is subtly
// different from an AEP-132 List response. We need to modify the RPC
// name to what the AEP-132 List response would be in order to permit
// the resource field properly.
resource = utils.ToPlural(strings.TrimSuffix(resource, "_revisions"))
}
if f.GetName() == resource {
return nil
}

// It is not the resource field; check it against the whitelist.
if !respAllowedFields.Contains(f.GetName()) {
return []lint.Problem{{
Message: "List responses should only contain fields explicitly described in AEPs.",
Expand Down
9 changes: 5 additions & 4 deletions rules/aep0132/response_unknown_fields_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,23 @@ func TestResponseUnknownFields(t *testing.T) {
problems testutils.Problems
}{
{"ListBooksResponse", "total_size", nil},
{"ListBooksResponse", "results", nil},
{"ListBooksResponse", "max_page_size", nil},
{"ListBooksResponse", "unreachable", nil},
{"ListBooksResponse", "unreachable_locations", nil},
{"ListBookRevisionsResponse", "total_size", nil},
{"ListBooksResponse", "extra", testutils.Problems{{Message: "List responses"}}},
{"ListBooksResponse", "books", testutils.Problems{{Message: "List responses"}}},
} {
t.Run(strcase.UpperCamelCase(test.FieldName), func(t *testing.T) {
f := testutils.ParseProto3Tmpl(t, `
message {{.MessageName}} {
repeated Book books = 1;
string next_page_token = 2;
string {{.FieldName}} = 3;
string next_page_token = 1;
string {{.FieldName}} = 2;
}
message Book {}
`, test)
field := f.GetMessageTypes()[0].GetFields()[2]
field := f.GetMessageTypes()[0].GetFields()[1]
if diff := test.problems.SetDescriptor(field).Diff(responseUnknownFields.Lint(f)); diff != "" {
t.Error(diff)
}
Expand Down
7 changes: 1 addition & 6 deletions rules/aep0134/http_uri_path.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,9 @@
package aep0134

import (
"fmt"

"github.com/aep-dev/api-linter/lint"
"github.com/aep-dev/api-linter/rules/internal/utils"
"github.com/jhump/protoreflect/desc"
"github.com/stoewer/go-strcase"
)

// Update methods should have a proper HTTP pattern.
Expand All @@ -29,8 +26,6 @@ var httpNameField = &lint.MethodRule{
RuleType: lint.NewRuleType(lint.MustRule),
OnlyIf: utils.IsUpdateMethod,
LintMethod: func(m *desc.MethodDescriptor) []lint.Problem {
fieldName := strcase.SnakeCase(m.GetName()[6:])
want := fmt.Sprintf("%s.path", fieldName)
return utils.LintHTTPURIHasVariable(m, want)
return utils.LintHTTPURIHasVariable(m, "path")
},
}
20 changes: 11 additions & 9 deletions rules/aep0134/http_uri_path_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,32 +27,34 @@ func TestHttpNameField(t *testing.T) {
MethodName string
problems testutils.Problems
}{
{"Valid", "/v1/{big_book.path=publishers/*/books/*}", "UpdateBigBook", nil},
{"ValidWithNumber", "/v1/{dv360.path=publishers/*/dv360s/*}", "UpdateDv360", nil},
{
"InvalidWithExtraName", "/v1/{big_book.path=publishers/*/books/*}",
"UpdateBigBook",
testutils.Problems{{Message: "`path`"}},
},
{"InvalidWithNumber", "/v1/{dv360.path=publishers/*/dv360s/*}", "UpdateDv360", testutils.Problems{{Message: "`path`"}}},
{
"InvalidNoUnderscore", "/v1/{bigbook.path=publishers/*/books/*}",
"UpdateBigBook",
testutils.Problems{{Message: "`big_book.path`"}},
testutils.Problems{{Message: "`path`"}},
},
{
"InvalidVarNameBook", "/v1/{big_book=publishers/*/books/*}",
"UpdateBigBook",
testutils.Problems{{Message: "`big_book.path`"}},
testutils.Problems{{Message: "`path`"}},
},
{
"InvalidVarNameName", "/v1/{path=publishers/*/books/*}",
"UpdateBigBook",
testutils.Problems{{Message: "`big_book.path`"}},
"Valid", "/v1/{path=publishers/*/books/*}", "UpdateBigBook", nil,
},
{
"InvalidVarNameReversed", "/v1/{path.big_book=publishers/*/books/*}",
"UpdateBigBook",
testutils.Problems{{Message: "`big_book.path`"}},
testutils.Problems{{Message: "`path`"}},
},
{
"NoVarName", "/v1/publishers/*/books/*",
"UpdateBigBook",
testutils.Problems{{Message: "`big_book.path`"}},
testutils.Problems{{Message: "`path`"}},
},
{
"Irrelevant", "/v1/{book=publishers/*/books/*}",
Expand Down
2 changes: 1 addition & 1 deletion rules/aep0134/request_path_required.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (

var requestPathRequired = &lint.MessageRule{
Name: lint.NewRuleName(134, "request-path-required"),
OnlyIf: utils.IsListRequestMessage,
OnlyIf: utils.IsUpdateRequestMessage,
LintMessage: func(m *desc.MessageDescriptor) []lint.Problem {
if m.FindFieldByName("path") == nil {
return []lint.Problem{{
Expand Down
4 changes: 2 additions & 2 deletions rules/aep0134/request_path_required_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ func TestRequestNameRequired(t *testing.T) {
FieldName string
problems testutils.Problems
}{
{"Valid", "ListBookRequest", "path", nil},
{"InvalidName", "ListBookRequest", "id", testutils.Problems{{Message: "path"}}},
{"Valid", "UpdateBookRequest", "path", nil},
{"InvalidName", "UpdateBookRequest", "id", testutils.Problems{{Message: "path"}}},
{"Irrelevant", "RemoveBookRequest", "id", nil},
}

Expand Down
2 changes: 1 addition & 1 deletion rules/aep0134/request_required_fields.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ var requestRequiredFields = &lint.MethodRule{
ot := utils.GetResponseType(m)
it := m.GetInputType()

allowedRequiredFields := stringset.New("update_mask")
allowedRequiredFields := stringset.New("update_mask", "path")

for _, f := range it.GetFields() {
if !utils.GetFieldBehavior(f).Contains("REQUIRED") {
Expand Down
1 change: 1 addition & 0 deletions rules/aep0134/request_resource_field_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ func TestResourceField(t *testing.T) {
problems testutils.Problems
}{
{"Valid", "UpdateBookRequest", "Book", "book", nil},
{"Valid", "UpdateBookRequest", "string", "path", nil},
{"InvalidFieldName", "UpdateBookRequest", "Book", "big_book", testutils.Problems{{Suggestion: "book"}}},
{"IrrelevantMessage", "ModifyBookRequest", "Book", "big_book", nil},
{"IrrelevantFieldType", "UpdateBookRequest", "string", "big_book", nil},
Expand Down
2 changes: 1 addition & 1 deletion rules/aep0136/aep0136.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func isCustomMethod(m *desc.MethodDescriptor) bool {

// Methods with no `:` in the URI are standard methods if they begin with
// one of the standard method names.
for _, prefix := range []string{"Get", "List", "Create", "Update", "Delete", "Replace"} {
for _, prefix := range []string{"Get", "List", "Create", "Update", "Delete", "Replace", "Apply"} {
Copy link
Member

Choose a reason for hiding this comment

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

Apply technically isn't in the AEPs yet - but I'm fine with adding it! or just commenting out that line from aepc (feel free to resolve this comment either way)

if strings.HasPrefix(m.GetName(), prefix) {
return false
}
Expand Down