diff --git a/docs/rules/0203/field-behavior-required.md b/docs/rules/0203/field-behavior-required.md new file mode 100644 index 000000000..5e4ab0831 --- /dev/null +++ b/docs/rules/0203/field-behavior-required.md @@ -0,0 +1,70 @@ +--- +rule: + aip: 203 + name: [core, '0203', field-behavior-required] + summary: Field behavior is required, and must have one of OUTPUT_ONLY, + REQUIRED, or OPTIONAL. +permalink: /203/field-behavior-required +redirect_from: + - /0203/field-behavior-required +--- + +# Field Behavior Required + +This rule enforces that each field in a message used in a request has a +`google.api.field_behavior` annotation with valid values, as mandated by +[AIP-203][]. + +## Details + +This rule looks at all fields and ensures they have a +`google.api.field_behavior` annotation. It also verifies that they have at least +one of the options `OUTPUT_ONLY`, `REQUIRED`, or `OPTIONAL`: all fields must +fall into one of these categories. + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect. +message Book { + string name = 1; + + // No field behavior + optional string title = 2; +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +message Book { + string name = 1; + + string title = 2 [(google.api.field_behavior) = REQUIRED]; +} +``` + +## 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 Book { + string name = 1; + + // Required. The title of the book. + // (-- api-linter: core::0203::field-behavior-required=disabled + // aip.dev/not-precedent: We need to do this because reasons. --) + optional string title = 2; +} +``` + +If you need to violate this rule for an entire file, place the comment at the +top of the file. + +[aip-203]: https://aip.dev/203 +[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/docs/rules/0203/optional-consistency.md b/docs/rules/0203/optional-consistency.md deleted file mode 100644 index c0410f53d..000000000 --- a/docs/rules/0203/optional-consistency.md +++ /dev/null @@ -1,94 +0,0 @@ ---- -rule: - aip: 203 - name: [core, '0203', optional-consistency] - summary: Optional fields should either be always or never annotated. -permalink: /203/optional-consistency -redirect_from: - - /0203/optional-consistency ---- - -# Optional fields: Consistency - -This rule enforces that messages containing fields that have a -`google.api.field_behavior` annotation of `OPTIONAL` uses this consistently -throughout the message, as mandated by [AIP-203][]. - -## Details - -This rule looks at messages with at least one field with a -`google.api.field_behavior` annotation of `OPTIONAL`, and complains if it finds -other fields on the message with no `google.api.field_behavior` annotation at -all. - -**Note:** It is generally recommended not to document "optional" at all. - -## Examples - -**Incorrect** code for this rule: - -```proto -// Incorrect. -message Book { - string name = 1 [(google.api.field_behavior) = REQUIRED]; - - // The foreword for the book. - string foreword = 2 [(google.api.field_behavior) = OPTIONAL]; - - // The afterword for the book. - string afterword = 3; // Inconsistent use of OPTIONAL. -} -``` - -**Correct** code for this rule: - -```proto -// Correct. -message Book { - string name = 1 [(google.api.field_behavior) = REQUIRED]; - - // The foreword for the book. - string foreword = 2; - - // The afterword for the book. - string afterword = 3; -} -``` - -```proto -// Correct. -message Book { - string name = 1 [(google.api.field_behavior) = REQUIRED]; - - // The foreword for the book. - string foreword = 2 [(google.api.field_behavior) = OPTIONAL]; - - // The afterword for the book. - string afterword = 3 [(google.api.field_behavior) = OPTIONAL]; -} -``` - -## Disabling - -If you need to violate this rule, use a leading comment above the message. -Remember to also include an [aip.dev/not-precedent][] comment explaining why. - -```proto -// (-- api-linter: core::0203::optional-consistency=disabled -// aip.dev/not-precedent: We need to do this because reasons. --) -message Book { - string name = 1; - - // The foreword for the book. - string foreword = 2 [(google.api.field_behavior) = OPTIONAL]; - - // The afterword for the book. - string afterword = 3; -} -``` - -If you need to violate this rule for an entire file, place the comment at the -top of the file. - -[aip-203]: https://aip.dev/203 -[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/docs/rules/0203/required-and-optional.md b/docs/rules/0203/required-and-optional.md index fcc6f9e2f..9b4df4f46 100644 --- a/docs/rules/0203/required-and-optional.md +++ b/docs/rules/0203/required-and-optional.md @@ -35,7 +35,7 @@ message Book { **Correct** code for this rule: ```proto -// Incorrect. +// Correct. message Book { string name = 1; diff --git a/rules/aip0203/aip0203.go b/rules/aip0203/aip0203.go index b085a0d37..f45cb86b3 100644 --- a/rules/aip0203/aip0203.go +++ b/rules/aip0203/aip0203.go @@ -31,11 +31,11 @@ var standardFields = stringset.New("etag") func AddRules(r lint.RuleRegistry) error { return r.Register( 203, + fieldBehaviorRequired, inputOnly, immutable, optional, optionalBehaviorConflict, - optionalBehaviorConsistency, outputOnly, required, requiredAndOptional, diff --git a/rules/aip0203/field_behavior_required.go b/rules/aip0203/field_behavior_required.go new file mode 100644 index 000000000..89475eff8 --- /dev/null +++ b/rules/aip0203/field_behavior_required.go @@ -0,0 +1,68 @@ +// Copyright 2023 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 aip0203 + +import ( + "fmt" + + "bitbucket.org/creachadair/stringset" + "github.com/googleapis/api-linter/lint" + "github.com/googleapis/api-linter/rules/internal/utils" + "github.com/jhump/protoreflect/desc" +) + +var minimumRequiredFieldBehavior = stringset.New( + "OPTIONAL", "REQUIRED", "OUTPUT_ONLY", "IMMUTABLE", +) + +var fieldBehaviorRequired = &lint.MethodRule{ + Name: lint.NewRuleName(203, "field-behavior-required"), + LintMethod: func(m *desc.MethodDescriptor) []lint.Problem { + // we only check requests, as OutputTypes are always + // OUTPUT_ONLY + it := m.GetInputType() + return checkFields(it) + }, +} + +func checkFields(m *desc.MessageDescriptor) []lint.Problem { + problems := []lint.Problem{} + for _, f := range m.GetFields() { + asMessage := f.GetMessageType() + if asMessage != nil { + problems = append(problems, checkFields(asMessage)...) + } + problems = append(problems, checkFieldBehavior(f)...) + } + return problems +} + +func checkFieldBehavior(f *desc.FieldDescriptor) []lint.Problem { + problems := []lint.Problem{} + fieldBehavior := utils.GetFieldBehavior(f) + if len(fieldBehavior) == 0 { + problems = append(problems, lint.Problem{ + Message: fmt.Sprintf("google.api.field_behavior annotation must be set, and have one of %v", minimumRequiredFieldBehavior), + Descriptor: f, + }) + // check for at least one valid annotation + } else if !minimumRequiredFieldBehavior.Intersects(fieldBehavior) { + problems = append(problems, lint.Problem{ + Message: fmt.Sprintf( + "google.api.field_behavior must have at least one of the following behaviors set: %v", minimumRequiredFieldBehavior), + Descriptor: f, + }) + } + return problems +} diff --git a/rules/aip0203/field_behavior_required_test.go b/rules/aip0203/field_behavior_required_test.go new file mode 100644 index 000000000..3479656fc --- /dev/null +++ b/rules/aip0203/field_behavior_required_test.go @@ -0,0 +1,142 @@ +// Copyright 2023 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 aip0203 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +const () + +func TestFieldBehaviorRequired(t *testing.T) { + for _, test := range []struct { + name string + Fields string + problems testutils.Problems + }{ + { + "ValidImmutable", + "int32 page_count = 1 [(google.api.field_behavior) = IMMUTABLE];", + nil, + }, + { + "ValidRequired", + "int32 page_count = 1 [(google.api.field_behavior) = REQUIRED];", + nil, + }, + { + "ValidOptional", + "int32 page_count = 1 [(google.api.field_behavior) = OPTIONAL];", + nil, + }, + { + "ValidOutputOnly", + "int32 page_count = 1 [(google.api.field_behavior) = OUTPUT_ONLY];", + nil, + }, + { + "ValidOptionalImmutable", + `int32 page_count = 1 [ + (google.api.field_behavior) = OUTPUT_ONLY, + (google.api.field_behavior) = OPTIONAL + ];`, + nil, + }, + { + "InvalidEmpty", + "int32 page_count = 1;", + testutils.Problems{{Message: "annotation must be set"}}, + }, + } { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/api/field_behavior.proto"; + + service Library { + rpc UpdateBook(UpdateBookRequest) returns (UpdateBookResponse) { + } + } + + message UpdateBookRequest { + {{.Fields}} + } + + message UpdateBookResponse { + // verifies that no error was raised on lack + // of field behavior in the response. + string name = 1; + } + `, test) + field := f.GetMessageTypes()[0].GetFields()[0] + if diff := test.problems.SetDescriptor(field).Diff(fieldBehaviorRequired.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} + +func TestFieldBehaviorRequiredNested(t *testing.T) { + for _, test := range []struct { + name string + Fields string + problems testutils.Problems + }{ + { + "ValidAnnotatedAndChildAnnotated", + "Annotated annotated = 1 [(google.api.field_behavior) = REQUIRED];", + nil, + }, + { + "InvalidChildNotAnnotated", + "NonAnnotated non_annotated = 1 [(google.api.field_behavior) = REQUIRED];", + testutils.Problems{{Message: "must be set"}}, + }, + } { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/api/field_behavior.proto"; + + service Library { + rpc UpdateBook(UpdateBookRequest) returns (UpdateBookResponse) { + } + } + + message NonAnnotated { + string nested = 1; + } + + message Annotated { + string nested = 1 [(google.api.field_behavior) = REQUIRED]; + } + + message UpdateBookRequest { + {{.Fields}} + } + + message UpdateBookResponse { + // verifies that no error was raised on lack + // of field behavior in the response. + string name = 1; + } + `, test) + it := f.GetServices()[0].GetMethods()[0].GetInputType() + nestedField := it.GetFields()[0].GetMessageType().GetFields()[0] + if diff := test.problems.SetDescriptor(nestedField).Diff(fieldBehaviorRequired.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} diff --git a/rules/aip0203/optional.go b/rules/aip0203/optional.go index 0bd0bc74a..78383b382 100644 --- a/rules/aip0203/optional.go +++ b/rules/aip0203/optional.go @@ -18,8 +18,6 @@ import ( "regexp" "github.com/googleapis/api-linter/lint" - "github.com/googleapis/api-linter/rules/internal/utils" - "github.com/jhump/protoreflect/desc" ) var optional = &lint.FieldRule{ @@ -29,12 +27,3 @@ var optional = &lint.FieldRule{ } var optionalRegexp = regexp.MustCompile("(?i).*optional.*") - -func messageHasOptionalFieldBehavior(m *desc.MessageDescriptor) bool { - for _, f := range m.GetFields() { - if utils.GetFieldBehavior(f).Contains("OPTIONAL") { - return true - } - } - return false -} diff --git a/rules/aip0203/optional_consistency.go b/rules/aip0203/optional_consistency.go deleted file mode 100644 index ddcaef82a..000000000 --- a/rules/aip0203/optional_consistency.go +++ /dev/null @@ -1,39 +0,0 @@ -// Copyright 2019 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 aip0203 - -import ( - "github.com/googleapis/api-linter/lint" - "github.com/googleapis/api-linter/rules/internal/utils" - "github.com/jhump/protoreflect/desc" -) - -// If a message has a field which is described as optional, ensure that every -// optional field on the message has this indicator. Oneof fields do not count. -var optionalBehaviorConsistency = &lint.MessageRule{ - Name: lint.NewRuleName(203, "optional-consistency"), - OnlyIf: messageHasOptionalFieldBehavior, - LintMessage: func(m *desc.MessageDescriptor) (problems []lint.Problem) { - for _, f := range m.GetFields() { - if utils.GetFieldBehavior(f).Len() == 0 && !standardFields.Contains(f.GetName()) && f.GetOneOf() == nil { - problems = append(problems, lint.Problem{ - Message: "Within a single message, either all optional fields should be indicated, or none of them should be.", - Descriptor: f, - }) - } - } - return - }, -} diff --git a/rules/aip0203/optional_consistency_test.go b/rules/aip0203/optional_consistency_test.go deleted file mode 100644 index 2d3f68919..000000000 --- a/rules/aip0203/optional_consistency_test.go +++ /dev/null @@ -1,122 +0,0 @@ -// Copyright 2019 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 aip0203 - -import ( - "testing" - - "github.com/googleapis/api-linter/rules/internal/testutils" -) - -func TestOptionalBehaviorConsistency(t *testing.T) { - testCases := []struct { - name string - field string - problems testutils.Problems - }{ - { - name: "Valid-NoneOptional", - field: ` -string name = 1 [ - (google.api.field_behavior) = IMMUTABLE, - (google.api.field_behavior) = OUTPUT_ONLY]; - -string title = 2 [(google.api.field_behavior) = REQUIRED]; - -string summary = 3; - -string author = 4;`, - problems: nil, - }, - { - name: "Valid-AllOptional", - field: ` -string name = 1 [ - (google.api.field_behavior) = IMMUTABLE, - (google.api.field_behavior) = OUTPUT_ONLY]; - -string title = 2 [(google.api.field_behavior) = REQUIRED]; - -string summary = 3 [(google.api.field_behavior) = OPTIONAL]; - -string author = 4 [(google.api.field_behavior) = OPTIONAL];`, - problems: nil, - }, - { - name: "Invalid-PartialOptional", - field: ` -string name = 1 [ - (google.api.field_behavior) = IMMUTABLE, - (google.api.field_behavior) = OUTPUT_ONLY]; - -string title = 2 [(google.api.field_behavior) = REQUIRED]; - -string summary = 3 [(google.api.field_behavior) = OPTIONAL]; - -string author = 4;`, - problems: testutils.Problems{{ - Message: "Within a single message, either all optional fields should be indicated, or none of them should be.", - }}, - }, - { - name: "Valid-IgnoreOneofFields", - field: ` -string name = 1 [ - (google.api.field_behavior) = IMMUTABLE, - (google.api.field_behavior) = OUTPUT_ONLY]; - -string title = 2 [(google.api.field_behavior) = REQUIRED]; - -string summary = 3 [(google.api.field_behavior) = OPTIONAL]; - -oneof other { - string author = 4; -}`, - problems: nil, - }, - { - name: "Valid-IgnoreEtag", - field: ` -string name = 1 [ - (google.api.field_behavior) = IMMUTABLE, - (google.api.field_behavior) = OUTPUT_ONLY]; - -string title = 2 [(google.api.field_behavior) = REQUIRED]; - -string summary = 3 [(google.api.field_behavior) = OPTIONAL]; - -string etag = 4;`, - problems: nil, - }, - } - - for _, test := range testCases { - t.Run(test.name, func(t *testing.T) { - template := ` - import "google/api/field_behavior.proto"; - message Book { - // Title of the book - {{.Field}} - }` - file := testutils.ParseProto3Tmpl(t, template, struct{ Field string }{test.field}) - // author field in the test will get the warning - f := file.GetMessageTypes()[0].GetFields()[3] - problems := optionalBehaviorConsistency.Lint(file) - if diff := test.problems.SetDescriptor(f).Diff(problems); diff != "" { - t.Errorf(diff) - } - }) - } -}