diff --git a/docs/rules/0132/request-unknown-fields.md b/docs/rules/0132/request-unknown-fields.md index 82957ff4d..422dd7973 100644 --- a/docs/rules/0132/request-unknown-fields.md +++ b/docs/rules/0132/request-unknown-fields.md @@ -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 diff --git a/docs/rules/0217/return-partial-success-type.md b/docs/rules/0217/return-partial-success-type.md new file mode 100644 index 000000000..16411336d --- /dev/null +++ b/docs/rules/0217/return-partial-success-type.md @@ -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 diff --git a/docs/rules/0217/return-partial-success-with-unreachable.md b/docs/rules/0217/return-partial-success-with-unreachable.md new file mode 100644 index 000000000..586cf30f5 --- /dev/null +++ b/docs/rules/0217/return-partial-success-with-unreachable.md @@ -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 diff --git a/rules/aip0132/request_unknown_fields.go b/rules/aip0132/request_unknown_fields.go index 9de013ca6..395cc92d3 100644 --- a/rules/aip0132/request_unknown_fields.go +++ b/rules/aip0132/request_unknown_fields.go @@ -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. diff --git a/rules/aip0132/request_unknown_fields_test.go b/rules/aip0132/request_unknown_fields_test.go index 63e8574a1..a2eead277 100644 --- a/rules/aip0132/request_unknown_fields_test.go +++ b/rules/aip0132/request_unknown_fields_test.go @@ -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{}}, diff --git a/rules/aip0217/aip0217.go b/rules/aip0217/aip0217.go index deb40ffac..5cfcf360d 100644 --- a/rules/aip0217/aip0217.go +++ b/rules/aip0217/aip0217.go @@ -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, ) diff --git a/rules/aip0217/return_partial_success_type.go b/rules/aip0217/return_partial_success_type.go new file mode 100644 index 000000000..d4f0127f1 --- /dev/null +++ b/rules/aip0217/return_partial_success_type.go @@ -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 + }, +} diff --git a/rules/aip0217/return_partial_success_type_test.go b/rules/aip0217/return_partial_success_type_test.go new file mode 100644 index 000000000..7540f1965 --- /dev/null +++ b/rules/aip0217/return_partial_success_type_test.go @@ -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) + } + }) + } +} diff --git a/rules/aip0217/return_partial_success_with_unreachable.go b/rules/aip0217/return_partial_success_with_unreachable.go new file mode 100644 index 000000000..0d438764c --- /dev/null +++ b/rules/aip0217/return_partial_success_with_unreachable.go @@ -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 + }, +} diff --git a/rules/aip0217/return_partial_success_with_unreachable_test.go b/rules/aip0217/return_partial_success_with_unreachable_test.go new file mode 100644 index 000000000..5e3725aee --- /dev/null +++ b/rules/aip0217/return_partial_success_with_unreachable_test.go @@ -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) + } + }) + } +}