Skip to content

Commit

Permalink
feat(AIP-217): add various rules for return_partial_success support (#…
Browse files Browse the repository at this point in the history
…1406)

* fix(AIP-132): allow return_partial_success in list

* feat(AIP-217): add return-partial-success-type rule

* feat(AIP-217): add RPS with unreachable rule

* format finding text

* update requirement in finding to must

* fix typo in docs
  • Loading branch information
noahdietz authored Jul 25, 2024
1 parent 088ec19 commit cf4ba12
Show file tree
Hide file tree
Showing 10 changed files with 351 additions and 10 deletions.
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
},
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)
}
})
}
}

0 comments on commit cf4ba12

Please sign in to comment.