From 751ea4f8729d46287e19776e162c21056beff726 Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Thu, 11 May 2023 10:53:55 -0700 Subject: [PATCH 1/7] feat(AIP-203): require field_behavior Adding guidance from https://github.com/aip-dev/google.aip.dev/pull/1100 --- docs/rules/0203/field-behavior-required.md | 69 +++++++++++++++++ docs/rules/0203/required-and-optional.md | 2 +- rules/aip0203/aip0203.go | 1 + rules/aip0203/field_behavior_required.go | 51 +++++++++++++ rules/aip0203/field_behavior_required_test.go | 76 +++++++++++++++++++ 5 files changed, 198 insertions(+), 1 deletion(-) create mode 100644 docs/rules/0203/field-behavior-required.md create mode 100644 rules/aip0203/field_behavior_required.go create mode 100644 rules/aip0203/field_behavior_required_test.go diff --git a/docs/rules/0203/field-behavior-required.md b/docs/rules/0203/field-behavior-required.md new file mode 100644 index 000000000..eac162f58 --- /dev/null +++ b/docs/rules/0203/field-behavior-required.md @@ -0,0 +1,69 @@ +--- +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 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 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/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..b7ce5fb5d 100644 --- a/rules/aip0203/aip0203.go +++ b/rules/aip0203/aip0203.go @@ -31,6 +31,7 @@ var standardFields = stringset.New("etag") func AddRules(r lint.RuleRegistry) error { return r.Register( 203, + fieldBehaviorRequired, inputOnly, immutable, optional, diff --git a/rules/aip0203/field_behavior_required.go b/rules/aip0203/field_behavior_required.go new file mode 100644 index 000000000..3783fbadf --- /dev/null +++ b/rules/aip0203/field_behavior_required.go @@ -0,0 +1,51 @@ +// 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 requiredOneOfFieldBehavior = stringset.New( + "OPTIONAL", "REQUIRED", "OUTPUT_ONLY", +) + +var fieldBehaviorRequired = &lint.FieldRule{ + Name: lint.NewRuleName(203, "field-behavior-required"), + LintField: func(f *desc.FieldDescriptor) []lint.Problem { + fieldBehavior := utils.GetFieldBehavior(f) + if len(fieldBehavior) == 0 { + return []lint.Problem{{ + Message: "google.api.field_behavior annotation must be set", + Descriptor: f, + }} + } + // check for at least one valid annotation + if !requiredOneOfFieldBehavior.Intersects(fieldBehavior) { + return []lint.Problem{{ + Message: fmt.Sprintf( + "google.api.field_behavior must have at least one of the following behaviors set: %v", + requiredOneOfFieldBehavior, + ), + Descriptor: f, + }} + } + return nil + }, +} diff --git a/rules/aip0203/field_behavior_required_test.go b/rules/aip0203/field_behavior_required_test.go new file mode 100644 index 000000000..a7f3cb68d --- /dev/null +++ b/rules/aip0203/field_behavior_required_test.go @@ -0,0 +1,76 @@ +// 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 + Annotations string + problems testutils.Problems + }{ + { + "ValidRequired", + "[(google.api.field_behavior) = REQUIRED]", + nil, + }, + { + "ValidOptional", + "[(google.api.field_behavior) = OPTIONAL]", + nil, + }, + { + "ValidOutputOnly", + "[(google.api.field_behavior) = OUTPUT_ONLY]", + nil, + }, + { + "ValidOptionalImmutable", + `[(google.api.field_behavior) = OUTPUT_ONLY, + (google.api.field_behavior) = OPTIONAL]`, + nil, + }, + { + "InvalidImmutable", + "[(google.api.field_behavior) = IMMUTABLE]", + testutils.Problems{{Message: "must have at least one"}}, + }, + { + "InvalidEmpty", + "", + 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"; + + message Book { + int32 page_count = 1 {{.Annotations}}; + } + `, test) + field := f.GetMessageTypes()[0].GetFields()[0] + if diff := test.problems.SetDescriptor(field).Diff(fieldBehaviorRequired.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} From 4964c4c2ffc7db7803b093faf55d8f9186c13c02 Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Tue, 23 May 2023 16:56:02 -0700 Subject: [PATCH 2/7] addressing feedback --- docs/rules/0203/field-behavior-required.md | 4 ++-- rules/aip0203/field_behavior_required.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/rules/0203/field-behavior-required.md b/docs/rules/0203/field-behavior-required.md index eac162f58..3e9477715 100644 --- a/docs/rules/0203/field-behavior-required.md +++ b/docs/rules/0203/field-behavior-required.md @@ -12,14 +12,14 @@ redirect_from: # Field Behavior Required This rule enforces that each field has a `google.api.field_behavior` annotation -with valid values, as mandated by[AIP-203][] +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 of these categories. +fall into one of these categories. ## Examples diff --git a/rules/aip0203/field_behavior_required.go b/rules/aip0203/field_behavior_required.go index 3783fbadf..b3ef78f0c 100644 --- a/rules/aip0203/field_behavior_required.go +++ b/rules/aip0203/field_behavior_required.go @@ -22,7 +22,7 @@ import ( "github.com/jhump/protoreflect/desc" ) -var requiredOneOfFieldBehavior = stringset.New( +var minimumRequiredFieldBehavior = stringset.New( "OPTIONAL", "REQUIRED", "OUTPUT_ONLY", ) @@ -37,11 +37,11 @@ var fieldBehaviorRequired = &lint.FieldRule{ }} } // check for at least one valid annotation - if !requiredOneOfFieldBehavior.Intersects(fieldBehavior) { + if !minimumRequiredFieldBehavior.Intersects(fieldBehavior) { return []lint.Problem{{ Message: fmt.Sprintf( "google.api.field_behavior must have at least one of the following behaviors set: %v", - requiredOneOfFieldBehavior, + minimumRequiredFieldBehavior, ), Descriptor: f, }} From 112fe23e60a2d3fd9509b47d5290fddd8b0891c4 Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Wed, 24 May 2023 09:14:09 -0700 Subject: [PATCH 3/7] addressing feedback - making it a method rule, checkin the request. - handling the nested message case. --- docs/rules/0203/field-behavior-required.md | 5 +- rules/aip0203/field_behavior_required.go | 62 ++++++++---- rules/aip0203/field_behavior_required_test.go | 95 ++++++++++++++++--- 3 files changed, 129 insertions(+), 33 deletions(-) diff --git a/docs/rules/0203/field-behavior-required.md b/docs/rules/0203/field-behavior-required.md index 3e9477715..5e4ab0831 100644 --- a/docs/rules/0203/field-behavior-required.md +++ b/docs/rules/0203/field-behavior-required.md @@ -11,8 +11,9 @@ redirect_from: # Field Behavior Required -This rule enforces that each field has a `google.api.field_behavior` annotation -with valid values, as mandated by [AIP-203][] +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 diff --git a/rules/aip0203/field_behavior_required.go b/rules/aip0203/field_behavior_required.go index b3ef78f0c..7990750e4 100644 --- a/rules/aip0203/field_behavior_required.go +++ b/rules/aip0203/field_behavior_required.go @@ -26,26 +26,50 @@ var minimumRequiredFieldBehavior = stringset.New( "OPTIONAL", "REQUIRED", "OUTPUT_ONLY", ) -var fieldBehaviorRequired = &lint.FieldRule{ +var fieldBehaviorRequired = &lint.MethodRule{ Name: lint.NewRuleName(203, "field-behavior-required"), - LintField: func(f *desc.FieldDescriptor) []lint.Problem { - fieldBehavior := utils.GetFieldBehavior(f) - if len(fieldBehavior) == 0 { - return []lint.Problem{{ - Message: "google.api.field_behavior annotation must be set", - Descriptor: f, - }} + 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)...) } - // check for at least one valid annotation - if !minimumRequiredFieldBehavior.Intersects(fieldBehavior) { - return []lint.Problem{{ - Message: fmt.Sprintf( - "google.api.field_behavior must have at least one of the following behaviors set: %v", - minimumRequiredFieldBehavior, - ), - Descriptor: f, - }} + p := checkFieldBehavior(f) + if p != nil { + problems = append(problems, *p) } - return nil - }, + } + return problems +} + +func checkFieldBehavior(f *desc.FieldDescriptor) *lint.Problem { + fieldBehavior := utils.GetFieldBehavior(f) + if len(fieldBehavior) == 0 { + return &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 + if !minimumRequiredFieldBehavior.Intersects(fieldBehavior) { + return &lint.Problem{ + Message: fmt.Sprintf( + "google.api.field_behavior must have at least one of the following behaviors set: %v", + minimumRequiredFieldBehavior, + ), + Descriptor: f, + } + } + return nil } diff --git a/rules/aip0203/field_behavior_required_test.go b/rules/aip0203/field_behavior_required_test.go index a7f3cb68d..408151ee8 100644 --- a/rules/aip0203/field_behavior_required_test.go +++ b/rules/aip0203/field_behavior_required_test.go @@ -23,39 +23,46 @@ const () func TestFieldBehaviorRequired(t *testing.T) { for _, test := range []struct { - name string - Annotations string - problems testutils.Problems + name string + Fields string + problems testutils.Problems }{ { "ValidRequired", - "[(google.api.field_behavior) = REQUIRED]", + "int32 page_count = 1 [(google.api.field_behavior) = REQUIRED];", nil, }, { "ValidOptional", - "[(google.api.field_behavior) = OPTIONAL]", + "int32 page_count = 1 [(google.api.field_behavior) = OPTIONAL];", nil, }, { "ValidOutputOnly", - "[(google.api.field_behavior) = OUTPUT_ONLY]", + "int32 page_count = 1 [(google.api.field_behavior) = OUTPUT_ONLY];", + nil, + }, + { + "ValidOutputOnly", + "int32 page_count = 1 [(google.api.field_behavior) = OUTPUT_ONLY];", nil, }, { "ValidOptionalImmutable", - `[(google.api.field_behavior) = OUTPUT_ONLY, - (google.api.field_behavior) = OPTIONAL]`, + `int32 page_count = 1 [ + (google.api.field_behavior) = OUTPUT_ONLY, + (google.api.field_behavior) = OPTIONAL + ];`, nil, }, { "InvalidImmutable", - "[(google.api.field_behavior) = IMMUTABLE]", + "int32 page_count = 1 [(google.api.field_behavior) = IMMUTABLE];", testutils.Problems{{Message: "must have at least one"}}, }, { "InvalidEmpty", - "", + "int32 page_count = 1;", testutils.Problems{{Message: "annotation must be set"}}, }, } { @@ -63,8 +70,19 @@ func TestFieldBehaviorRequired(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` import "google/api/field_behavior.proto"; - message Book { - int32 page_count = 1 {{.Annotations}}; + 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] @@ -74,3 +92,56 @@ func TestFieldBehaviorRequired(t *testing.T) { }) } } + +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) + } + }) + } +} From 487e5a19665873807bdc11e7966d469dd632f04f Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Wed, 24 May 2023 09:25:56 -0700 Subject: [PATCH 4/7] addressing feedback removing optional_consistency, which is now redundant. --- docs/rules/0203/optional-consistency.md | 94 ---------------- rules/aip0203/optional_consistency.go | 39 ------- rules/aip0203/optional_consistency_test.go | 122 --------------------- 3 files changed, 255 deletions(-) delete mode 100644 docs/rules/0203/optional-consistency.md delete mode 100644 rules/aip0203/optional_consistency.go delete mode 100644 rules/aip0203/optional_consistency_test.go 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/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) - } - }) - } -} From 034cdd5815eccd85bc020c931b81f72aff7a8837 Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Wed, 24 May 2023 09:32:22 -0700 Subject: [PATCH 5/7] addressing feedback - adding immutable as one of the required behaviors. --- rules/aip0203/field_behavior_required.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/aip0203/field_behavior_required.go b/rules/aip0203/field_behavior_required.go index 7990750e4..4657eea52 100644 --- a/rules/aip0203/field_behavior_required.go +++ b/rules/aip0203/field_behavior_required.go @@ -23,7 +23,7 @@ import ( ) var minimumRequiredFieldBehavior = stringset.New( - "OPTIONAL", "REQUIRED", "OUTPUT_ONLY", + "OPTIONAL", "REQUIRED", "OUTPUT_ONLY", "IMMUTABLE", ) var fieldBehaviorRequired = &lint.MethodRule{ From 8620256b68ed7cacebbc643085a0644c43b149f5 Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Wed, 24 May 2023 09:36:04 -0700 Subject: [PATCH 6/7] fixing tests --- rules/aip0203/aip0203.go | 1 - rules/aip0203/field_behavior_required_test.go | 15 +++++---------- rules/aip0203/optional.go | 11 ----------- 3 files changed, 5 insertions(+), 22 deletions(-) diff --git a/rules/aip0203/aip0203.go b/rules/aip0203/aip0203.go index b7ce5fb5d..f45cb86b3 100644 --- a/rules/aip0203/aip0203.go +++ b/rules/aip0203/aip0203.go @@ -36,7 +36,6 @@ func AddRules(r lint.RuleRegistry) error { immutable, optional, optionalBehaviorConflict, - optionalBehaviorConsistency, outputOnly, required, requiredAndOptional, diff --git a/rules/aip0203/field_behavior_required_test.go b/rules/aip0203/field_behavior_required_test.go index 408151ee8..3479656fc 100644 --- a/rules/aip0203/field_behavior_required_test.go +++ b/rules/aip0203/field_behavior_required_test.go @@ -27,6 +27,11 @@ func TestFieldBehaviorRequired(t *testing.T) { 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];", @@ -42,11 +47,6 @@ func TestFieldBehaviorRequired(t *testing.T) { "int32 page_count = 1 [(google.api.field_behavior) = OUTPUT_ONLY];", nil, }, - { - "ValidOutputOnly", - "int32 page_count = 1 [(google.api.field_behavior) = OUTPUT_ONLY];", - nil, - }, { "ValidOptionalImmutable", `int32 page_count = 1 [ @@ -55,11 +55,6 @@ func TestFieldBehaviorRequired(t *testing.T) { ];`, nil, }, - { - "InvalidImmutable", - "int32 page_count = 1 [(google.api.field_behavior) = IMMUTABLE];", - testutils.Problems{{Message: "must have at least one"}}, - }, { "InvalidEmpty", "int32 page_count = 1;", 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 -} From 4821e4ed85e4e44307a67836789103148b20692a Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Thu, 25 May 2023 14:47:38 -0700 Subject: [PATCH 7/7] address feedback fix nits, lint problem pointer -> list --- rules/aip0203/field_behavior_required.go | 31 +++++++++--------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/rules/aip0203/field_behavior_required.go b/rules/aip0203/field_behavior_required.go index 4657eea52..89475eff8 100644 --- a/rules/aip0203/field_behavior_required.go +++ b/rules/aip0203/field_behavior_required.go @@ -43,33 +43,26 @@ func checkFields(m *desc.MessageDescriptor) []lint.Problem { if asMessage != nil { problems = append(problems, checkFields(asMessage)...) } - p := checkFieldBehavior(f) - if p != nil { - problems = append(problems, *p) - } + problems = append(problems, checkFieldBehavior(f)...) } return problems } -func checkFieldBehavior(f *desc.FieldDescriptor) *lint.Problem { +func checkFieldBehavior(f *desc.FieldDescriptor) []lint.Problem { + problems := []lint.Problem{} fieldBehavior := utils.GetFieldBehavior(f) if len(fieldBehavior) == 0 { - return &lint.Problem{ - Message: fmt.Sprintf("google.api.field_behavior annotation must be set, and have one of %v", - minimumRequiredFieldBehavior, - ), + 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 - if !minimumRequiredFieldBehavior.Intersects(fieldBehavior) { - return &lint.Problem{ + }) + // 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, - ), + "google.api.field_behavior must have at least one of the following behaviors set: %v", minimumRequiredFieldBehavior), Descriptor: f, - } + }) } - return nil + return problems }