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

feat(AIP-217): add various rules for return_partial_success support #1406

Merged
merged 8 commits into from
Jul 25, 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
1 change: 1 addition & 0 deletions docs/rules/0132/request-unknown-fields.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ comes across any fields other than:
- `string request_id` ([AIP-155][])
- `google.protobuf.FieldMask read_mask` ([AIP-157][])
- `View view` ([AIP-157][])
- `bool return_partial_success` ([AIP-217][])

It only checks field names; it does not validate type correctness. This is
handled by other rules, such as
Expand Down
67 changes: 67 additions & 0 deletions docs/rules/0217/return-partial-success-type.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
---
rule:
aip: 217
name: [core, '0217', return-partial-success-type]
summary: The return_partial_success field should be a bool.
permalink: /217/return-partial-success-type
redirect_from:
- /0217/return-partial-success-type
---

# States

This rule enforces that the `return_partial_success` field is a `bool`, as
mandated in [AIP-217][].

## Details

This rule looks at fields named `return_partial_success`, and complains if they
are anything other than a `bool`.

## Examples

**Incorrect** code for this rule:

```proto
// Incorrect.
message ListBooksRequest {
string parent = 1;
int32 page_size = 2;
string page_token = 3;
string return_partial_success = 4; // Should be bool.
}
```

**Correct** code for this rule:

```proto
// Correct.
message ListBooksRequest {
string parent = 1;
int32 page_size = 2;
string page_token = 3;
bool return_partial_success = 4;
}
```

## Disabling

If you need to violate this rule, use a leading comment above the field.
Remember to also include an [aip.dev/not-precedent][] comment explaining why.

```proto
message ListBooksRequest {
string parent = 1;
int32 page_size = 2;
string page_token = 3;
// (-- api-linter: core::0217::return-partial-success-type=disabled
// aip.dev/not-precedent: We need to do this because reasons. --)
string return_partial_success = 4;
}
```

If you need to violate this rule for an entire file, place the comment at the
top of the file.

[aip-217]: https://aip.dev/217
[aip.dev/not-precedent]: https://aip.dev/not-precedent
91 changes: 91 additions & 0 deletions docs/rules/0217/return-partial-success-with-unreachable.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
---
rule:
aip: 217
name: [core, '0217', return-partial-success-with-unreachable]
summary: The return_partial_success field should be paired with unreachable.
permalink: /217/return-partial-success-with-unreachable
redirect_from:
- /0217/return-partial-success-with-unreachable
---

# States

This rule enforces that the `return_partial_success` field is paired with a
corresponding `repeated string unreachable` field, as mandated in [AIP-217][].

## Details

This rule looks at methods that have a request field named
`return_partial_success`, and complains if the response does not have a
`repeated string unreachable` field.

## Examples

**Incorrect** code for this rule:

```proto
message ListBooksRequest {
string parent = 1;
int32 page_size = 2;
string page_token = 3;
// Incorrect. Missing unreachable response field.
string return_partial_success = 4;
}

message ListBooksResponse {
repeated Book books = 1;

string next_page_token = 2;

// Incorrect. Missing unreachable field.
}
```

**Correct** code for this rule:

```proto
message ListBooksRequest {
string parent = 1;
int32 page_size = 2;
string page_token = 3;
// Correct.
bool return_partial_success = 4;
}

message ListBooksResponse {
repeated Book books = 1;

string next_page_token = 2;

// Correct.
repeated string unreachable = 3;
}
```

## Disabling

If you need to violate this rule, use a leading comment above the field.
Remember to also include an [aip.dev/not-precedent][] comment explaining why.

```proto
message ListBooksRequest {
string parent = 1;
int32 page_size = 2;
string page_token = 3;
// (-- api-linter: core::0217::return-partial-success-with-unreachable=disabled
// aip.dev/not-precedent: We need to do this because reasons. --)
string return_partial_success = 4;
}

message ListBooksResponse {
repeated Book books = 1;

string next_page_token = 2;
}
```

If you need to violate this rule for an entire file, place the comment at the
top of the file.

[aip-217]: https://aip.dev/217
[aip.dev/not-precedent]: https://aip.dev/not-precedent
21 changes: 11 additions & 10 deletions rules/aip0132/request_unknown_fields.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,17 @@ import (
)

var allowedFields = stringset.New(
"parent", // AIP-132
"page_size", // AIP-158
"page_token", // AIP-158
"skip", // AIP-158
"filter", // AIP-132
"order_by", // AIP-132
"show_deleted", // AIP-135
"request_id", // AIP-155
"read_mask", // AIP-157
"view", // AIP-157
"parent", // AIP-132
"page_size", // AIP-158
"page_token", // AIP-158
"skip", // AIP-158
"filter", // AIP-132
"order_by", // AIP-132
"show_deleted", // AIP-135
"request_id", // AIP-155
"read_mask", // AIP-157
"view", // AIP-157
"return_partial_success", // AIP-217
)

// List methods should not have unrecognized fields.
Expand Down
1 change: 1 addition & 0 deletions rules/aip0132/request_unknown_fields_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func TestUnknownFields(t *testing.T) {
{"ShowDeleted", "ListBooksRequest", "show_deleted", builder.FieldTypeBool(), testutils.Problems{}},
{"RequestId", "ListBooksRequest", "request_id", builder.FieldTypeImportedMessage(fieldMask), testutils.Problems{}},
{"ReadMask", "ListBooksRequest", "read_mask", builder.FieldTypeImportedMessage(fieldMask), testutils.Problems{}},
{"ReturnPartialSuccess", "ListBooksRequest", "return_partial_success", builder.FieldTypeImportedMessage(fieldMask), testutils.Problems{}},
{"View", "ListBooksRequest", "view", builder.FieldTypeEnum(builder.NewEnum("View").AddValue(builder.NewEnumValue("BASIC"))), testutils.Problems{}},
{"Invalid", "ListBooksRequest", "application_id", builder.FieldTypeString(), testutils.Problems{{Message: "explicitly described"}}},
{"Irrelevant", "EnumerteBooksRequest", "application_id", builder.FieldTypeString(), testutils.Problems{}},
Expand Down
2 changes: 2 additions & 0 deletions rules/aip0217/aip0217.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import "github.com/googleapis/api-linter/lint"
func AddRules(r lint.RuleRegistry) error {
return r.Register(
217,
returnPartialSuccessType,
returnPartialSuccessWithUnreachable,
synonyms,
unreachableFieldType,
)
Expand Down
40 changes: 40 additions & 0 deletions rules/aip0217/return_partial_success_type.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package aip0217

import (
"github.com/googleapis/api-linter/lint"
"github.com/googleapis/api-linter/locations"
"github.com/googleapis/api-linter/rules/internal/utils"
"github.com/jhump/protoreflect/desc"
)

var returnPartialSuccessType = &lint.FieldRule{
Name: lint.NewRuleName(217, "return-partial-success-type"),
OnlyIf: func(f *desc.FieldDescriptor) bool {
return f.GetName() == "return_partial_success"
},
LintField: func(f *desc.FieldDescriptor) (problems []lint.Problem) {
if utils.GetTypeName(f) != "bool" {
problems = append(problems, lint.Problem{
Message: "`return_partial_success` field must be a `bool`.",
Suggestion: "bool",
Descriptor: f,
Location: locations.FieldType(f),
})
}
return
},
}
47 changes: 47 additions & 0 deletions rules/aip0217/return_partial_success_type_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package aip0217

import (
"testing"

"github.com/googleapis/api-linter/rules/internal/testutils"
)

func TestReturnPartialSuccessType(t *testing.T) {
for _, test := range []struct {
name string
Type string
problems testutils.Problems
}{
{"Valid", "bool", testutils.Problems{}},
{"InvalidType", "string", testutils.Problems{{Suggestion: "bool"}}},
} {
t.Run(test.name, func(t *testing.T) {
f := testutils.ParseProto3Tmpl(t, `
message ListBooksRequest {
string parent = 1;
int32 page_size = 2;
string page_token = 3;
{{.Type}} return_partial_success = 4;
}
`, test)
field := f.GetMessageTypes()[0].FindFieldByName("return_partial_success")
if diff := test.problems.SetDescriptor(field).Diff(returnPartialSuccessType.Lint(f)); diff != "" {
t.Errorf(diff)
}
})
}
}
36 changes: 36 additions & 0 deletions rules/aip0217/return_partial_success_with_unreachable.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package aip0217

import (
"github.com/googleapis/api-linter/lint"
"github.com/jhump/protoreflect/desc"
)

var returnPartialSuccessWithUnreachable = &lint.MethodRule{
Name: lint.NewRuleName(217, "return-partial-success-with-unreachable"),
OnlyIf: func(m *desc.MethodDescriptor) bool {
return m.GetInputType().FindFieldByName("return_partial_success") != nil
noahdietz marked this conversation as resolved.
Show resolved Hide resolved
},
LintMethod: func(m *desc.MethodDescriptor) (problems []lint.Problem) {
if m.GetOutputType().FindFieldByName("unreachable") == nil {
problems = append(problems, lint.Problem{
Message: "`return_partial_success` must be added in conjunction with response field `repeated string unreachable`.",
Descriptor: m.GetInputType().FindFieldByName("return_partial_success"),
})
}
return
},
}
55 changes: 55 additions & 0 deletions rules/aip0217/return_partial_success_with_unreachable_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright 2024 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package aip0217

import (
"testing"

"github.com/googleapis/api-linter/rules/internal/testutils"
)

func TestReturnPartialSuccessWithUnreachable(t *testing.T) {
for _, test := range []struct {
name string
ResponseField string
problems testutils.Problems
}{
{"Valid", "repeated string unreachable = 1;", testutils.Problems{}},
{"InvalidMissingUnreachable", "", testutils.Problems{{Message: "repeated string unreachable"}}},
} {
t.Run(test.name, func(t *testing.T) {
f := testutils.ParseProto3Tmpl(t, `
service Library {
rpc ListBooks(ListBooksRequest) returns (ListBooksResponse);
}

message ListBooksRequest {
string parent = 1;
int32 page_size = 2;
string page_token = 3;
bool return_partial_success = 4;
}

message ListBooksResponse {
{{.ResponseField}}
}
`, test)
field := f.GetMessageTypes()[0].FindFieldByName("return_partial_success")
if diff := test.problems.SetDescriptor(field).Diff(returnPartialSuccessWithUnreachable.Lint(f)); diff != "" {
t.Errorf(diff)
}
})
}
}
Loading