From 6fcc60498986524728f94008576d80691de096d6 Mon Sep 17 00:00:00 2001 From: noahdietz <ndietz@google.com> Date: Wed, 29 May 2024 15:13:34 -0700 Subject: [PATCH 1/4] feat(AIP-134): update_mask must be OPTIONAL --- .../0134/update-mask-optional-behavior.md | 72 +++++++++++++++++++ locations/field_locations.go | 6 ++ rules/aip0134/aip0134.go | 1 + .../aip0134/update_mask_optional_behavior.go | 42 +++++++++++ .../update_mask_optional_behavior_test.go | 51 +++++++++++++ 5 files changed, 172 insertions(+) create mode 100644 docs/rules/0134/update-mask-optional-behavior.md create mode 100644 rules/aip0134/update_mask_optional_behavior.go create mode 100644 rules/aip0134/update_mask_optional_behavior_test.go diff --git a/docs/rules/0134/update-mask-optional-behavior.md b/docs/rules/0134/update-mask-optional-behavior.md new file mode 100644 index 000000000..1a4f443b4 --- /dev/null +++ b/docs/rules/0134/update-mask-optional-behavior.md @@ -0,0 +1,72 @@ +--- +rule: + aip: 134 + name: [core, '0134', update-mask-optional-behavior] + summary: Standard Update `update_mask` field must be `OPTIONAL`. +permalink: /134/update-mask-optional-behavior +redirect_from: + - /0134/update-mask-optional-behavior +--- + +# Update methods: Mask field expected behavior + +This rule enforces that the `update_mask` field of a Standard `Update` request +uses `google.api.field_behavior = OPTIONAL`, as mandated in [AIP-134][]. + +## Details + +This rule looks at any field named `update_mask` that's in a `Update*Request` +and complains if it the field is not annotated with +`google.api.field_behavior = OPTIONAL`. + +## Examples + +**Incorrect** code for this rule: + +```proto +message UpdateBookRequest { + Book book = 1; + + // Incorrect. Must be `OPTIONAL`. + google.protobuf.FieldMask update_mask = 2 [ + (google.api.field_behavior) = REQUIRED + ]; +} +``` + + +**Correct** code for this rule: + +```proto +message UpdateBookRequest { + Book book = 1; + + // Correct. + google.protobuf.FieldMask update_mask = 2 [ + (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 +message UpdateBookRequest { + Book book = 1; + + // (-- api-linter: core::0134::update-mask-optional-behavior=disabled + // aip.dev/not-precedent: We need to do this because reasons. --) + google.protobuf.FieldMask update_mask = 2 [ + (google.api.field_behavior) = REQUIRED + ]; +} +``` + +If you need to violate this rule for an entire file, place the comment at the +top of the file. + +[aip-134]: https://aip.dev/134 +[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/locations/field_locations.go b/locations/field_locations.go index 586f94869..b8172c0d4 100644 --- a/locations/field_locations.go +++ b/locations/field_locations.go @@ -35,6 +35,12 @@ func FieldResourceReference(f *desc.FieldDescriptor) *dpb.SourceCodeInfo_Locatio return pathLocation(f, 8, int(apb.E_ResourceReference.TypeDescriptor().Number())) // FieldDescriptor.options == 8 } +// FieldBehavior returns the precise location for a field's +// field_behavior annotation. +func FieldBehavior(f *desc.FieldDescriptor) *dpb.SourceCodeInfo_Location { + return pathLocation(f, 8, int(apb.E_FieldBehavior.TypeDescriptor().Number())) // FieldDescriptor.options == 8 +} + // FieldType returns the precise location for a field's type. func FieldType(f *desc.FieldDescriptor) *dpb.SourceCodeInfo_Location { if f.GetMessageType() != nil || f.GetEnumType() != nil { diff --git a/rules/aip0134/aip0134.go b/rules/aip0134/aip0134.go index 46c8143b5..d0b05ada0 100644 --- a/rules/aip0134/aip0134.go +++ b/rules/aip0134/aip0134.go @@ -42,6 +42,7 @@ func AddRules(r lint.RuleRegistry) error { responseLRO, synonyms, unknownFields, + updateMaskOptionalBehavior, ) } diff --git a/rules/aip0134/update_mask_optional_behavior.go b/rules/aip0134/update_mask_optional_behavior.go new file mode 100644 index 000000000..73ed59f90 --- /dev/null +++ b/rules/aip0134/update_mask_optional_behavior.go @@ -0,0 +1,42 @@ +// 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 aip0134 + +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 updateMaskOptionalBehavior = &lint.FieldRule{ + Name: lint.NewRuleName(134, "update-mask-optional-behavior"), + OnlyIf: func(f *desc.FieldDescriptor) bool { + return f.GetName() == "update_mask" && utils.IsUpdateRequestMessage(f.GetOwner()) + }, + LintField: func(f *desc.FieldDescriptor) []lint.Problem { + behaviors := utils.GetFieldBehavior(f) + if !behaviors.Contains("OPTIONAL") { + return []lint.Problem{ + { + Message: "Standard Update field `update_mask` must have `OPTIONAL` behavior", + Descriptor: f, + Location: locations.FieldBehavior(f), + }, + } + } + return nil + }, +} diff --git a/rules/aip0134/update_mask_optional_behavior_test.go b/rules/aip0134/update_mask_optional_behavior_test.go new file mode 100644 index 000000000..ac1747d48 --- /dev/null +++ b/rules/aip0134/update_mask_optional_behavior_test.go @@ -0,0 +1,51 @@ +// 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 aip0134 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +func TestUpdateMaskOptionalBehavior(t *testing.T) { + tests := []struct { + name string + Behavior string + problems testutils.Problems + }{ + {"Valid", "[(google.api.field_behavior) = OPTIONAL]", nil}, + {"InvalidWrong", "[(google.api.field_behavior) = REQUIRED]", testutils.Problems{{Message: "must have `OPTIONAL`"}}}, + {"InvalidMissing", "", testutils.Problems{{Message: "must have `OPTIONAL`"}}}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + file := testutils.ParseProto3Tmpl(t, ` + import "google/api/field_behavior.proto"; + import "google/protobuf/field_mask.proto"; + message UpdateBookRequest { + Book book = 1; + google.protobuf.FieldMask update_mask = 2 {{.Behavior}}; + } + message Book {} + `, test) + field := file.GetMessageTypes()[0].FindFieldByName("update_mask") + problems := updateMaskOptionalBehavior.Lint(file) + if diff := test.problems.SetDescriptor(field).Diff(problems); diff != "" { + t.Errorf(diff) + } + }) + } +} From 84bc2722274ec6b20e5ae7a2becf421fe9cde977 Mon Sep 17 00:00:00 2001 From: noahdietz <ndietz@google.com> Date: Wed, 29 May 2024 15:16:23 -0700 Subject: [PATCH 2/4] chore(AIP-203): use locations.FieldBehavior helper --- rules/aip0203/resource_name_identifier.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/aip0203/resource_name_identifier.go b/rules/aip0203/resource_name_identifier.go index b5d664c58..88708d52b 100644 --- a/rules/aip0203/resource_name_identifier.go +++ b/rules/aip0203/resource_name_identifier.go @@ -34,7 +34,7 @@ var resourceNameIdentifier = &lint.MessageRule{ return []lint.Problem{{ Message: "resource name field must have field_behavior IDENTIFIER", Descriptor: f, - Location: locations.FieldOption(f, fpb.E_FieldBehavior), + Location: locations.FieldBehavior(f), }} } From 0f6b2e00519f1115c2f387ed324b659a1da4d93f Mon Sep 17 00:00:00 2001 From: Noah Dietz <noahdietz@users.noreply.github.com> Date: Tue, 11 Jun 2024 13:33:44 -0700 Subject: [PATCH 3/4] update license header year --- rules/aip0134/update_mask_optional_behavior_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/aip0134/update_mask_optional_behavior_test.go b/rules/aip0134/update_mask_optional_behavior_test.go index ac1747d48..845412236 100644 --- a/rules/aip0134/update_mask_optional_behavior_test.go +++ b/rules/aip0134/update_mask_optional_behavior_test.go @@ -1,4 +1,4 @@ -// Copyright 2019 Google LLC +// 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. From 9c628b9a41b41ba499521690632f51ad6c6ea5c8 Mon Sep 17 00:00:00 2001 From: noahdietz <ndietz@google.com> Date: Wed, 10 Jul 2024 08:15:56 -0700 Subject: [PATCH 4/4] chore: fix typos --- docs/rules/0134/update-mask-optional-behavior.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/rules/0134/update-mask-optional-behavior.md b/docs/rules/0134/update-mask-optional-behavior.md index 1a4f443b4..172ac1fd4 100644 --- a/docs/rules/0134/update-mask-optional-behavior.md +++ b/docs/rules/0134/update-mask-optional-behavior.md @@ -15,8 +15,8 @@ uses `google.api.field_behavior = OPTIONAL`, as mandated in [AIP-134][]. ## Details -This rule looks at any field named `update_mask` that's in a `Update*Request` -and complains if it the field is not annotated with +This rule looks at any field named `update_mask` that's in an `Update*Request` +and complains if the field is not annotated with `google.api.field_behavior = OPTIONAL`. ## Examples