From 471a6564406ef0c34206cef8f9a2f525611fef83 Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Mon, 26 Jun 2023 12:16:03 -0700 Subject: [PATCH] fix(AIP-203): remove optional conflict The AIPs now allow OPTIONAL to be used with other descriptive annotations (e.g. INPUT_ONLY or IMMUTABLE). Removing the rule accordingly to minimize confusion. --- docs/rules/0203/optional-conflict.md | 71 ------------------------- rules/aip0203/aip0203.go | 1 - rules/aip0203/optional_conflict.go | 44 --------------- rules/aip0203/optional_conflict_test.go | 71 ------------------------- 4 files changed, 187 deletions(-) delete mode 100644 docs/rules/0203/optional-conflict.md delete mode 100644 rules/aip0203/optional_conflict.go delete mode 100644 rules/aip0203/optional_conflict_test.go diff --git a/docs/rules/0203/optional-conflict.md b/docs/rules/0203/optional-conflict.md deleted file mode 100644 index ea5142e2e..000000000 --- a/docs/rules/0203/optional-conflict.md +++ /dev/null @@ -1,71 +0,0 @@ ---- -rule: - aip: 203 - name: [core, '0203', optional-conflict] - summary: Optional fields must not have any other field behavior. -permalink: /203/optional-conflict -redirect_from: - - /0203/optional-conflict ---- - -# Optional fields: Conflicts - -This rule enforces that fields that are annotated as `OPTIONAL` do not have any -other annotation (such as `REQUIRED`), as mandated by [AIP-203][]. - -## Details - -This rule looks at any field with a `google.api.field_behavior` annotation of -`OPTIONAL`, and complains if it also finds any other field behavior. - -## Examples - -**Incorrect** code for this rule: - -```proto -// Incorrect. -message Book { - string name = 1; - - // The foreword for the book. - string foreword = 2 [ - (google.api.field_behavior) = OPTIONAL, // "Optional" can not co-exist with other field behaviors. - (google.api.field_behavior) = IMMUTABLE]; -} -``` - -**Correct** code for this rule: - -```proto -// Correct. -message Book { - string name = 1; - - // The foreword for the book. - string foreword = 2 [(google.api.field_behavior) = OPTIONAL]; -} -``` - -## 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; - - // Optional. The foreword for the book. - // (-- api-linter: core::0203::optional-conflict=disabled - // aip.dev/not-precedent: We need to do this because reasons. --) - string foreword = 2 [ - (google.api.field_behavior) = OPTIONAL, - (google.api.field_behavior) = IMMUTABLE]; -} -``` - -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/aip0203.go b/rules/aip0203/aip0203.go index 53f43ab71..2d067aecc 100644 --- a/rules/aip0203/aip0203.go +++ b/rules/aip0203/aip0203.go @@ -36,7 +36,6 @@ func AddRules(r lint.RuleRegistry) error { inputOnly, immutable, optional, - optionalBehaviorConflict, outputOnly, required, unorderedListRepeated, diff --git a/rules/aip0203/optional_conflict.go b/rules/aip0203/optional_conflict.go deleted file mode 100644 index bbfb32bad..000000000 --- a/rules/aip0203/optional_conflict.go +++ /dev/null @@ -1,44 +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" -) - -var optionalBehaviorConflict = &lint.FieldRule{ - Name: lint.NewRuleName(203, "optional-conflict"), - OnlyIf: func(f *desc.FieldDescriptor) bool { - return !withoutOptionalFieldBehavior(f) - }, - LintField: func(f *desc.FieldDescriptor) []lint.Problem { - // APIs may use the OPTIONAL value to describe a field which doesn't use - // REQUIRED, IMMUTABLE, INPUT_ONLY or OUTPUT_ONLY. If a field is described - // as optional, it can't be others. - if len(utils.GetFieldBehavior(f)) > 1 { - return []lint.Problem{{ - Message: "Field behavior `(google.api.field_behavior) = OPTIONAL` shouldn't be used together with other field behaviors.", - Descriptor: f, - }} - } - return nil - }, -} - -func withoutOptionalFieldBehavior(f *desc.FieldDescriptor) bool { - return !utils.GetFieldBehavior(f).Contains("OPTIONAL") -} diff --git a/rules/aip0203/optional_conflict_test.go b/rules/aip0203/optional_conflict_test.go deleted file mode 100644 index 0a4b683cd..000000000 --- a/rules/aip0203/optional_conflict_test.go +++ /dev/null @@ -1,71 +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 TestOptionalBehaviorConflict(t *testing.T) { - testCases := []struct { - name string - field string - problems testutils.Problems - }{ - { - name: "Valid", - field: "string title = 1 [(google.api.field_behavior) = OPTIONAL];", - problems: nil, - }, - { - name: "Valid", - field: ` -string title = 1 [ - (google.api.field_behavior) = IMMUTABLE, - (google.api.field_behavior) = OUTPUT_ONLY];`, - problems: nil, - }, - { - name: "Invalid-optional-conflict", - field: ` -string title = 1 [ - (google.api.field_behavior) = IMMUTABLE, - (google.api.field_behavior) = OUTPUT_ONLY, - (google.api.field_behavior) = OPTIONAL];`, - problems: testutils.Problems{{ - Message: "Field behavior `(google.api.field_behavior) = OPTIONAL` shouldn't be used together with other field behaviors", - }}, - }, - } - - 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}) - f := file.GetMessageTypes()[0].GetFields()[0] - problems := optionalBehaviorConflict.Lint(file) - if diff := test.problems.SetDescriptor(f).Diff(problems); diff != "" { - t.Errorf(diff) - } - }) - } -}