From 9fc0d05f3d89905ea7d3b22c9b44fbfa79edac07 Mon Sep 17 00:00:00 2001 From: Noah Dietz Date: Wed, 10 Jul 2024 09:08:48 -0700 Subject: [PATCH] feat(AIP-134): update_mask must be OPTIONAL (#1394) * feat(AIP-134): update_mask must be OPTIONAL * chore(AIP-203): use locations.FieldBehavior helper * update license header year * chore: fix typos --- .../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 +++++++++++++ rules/aip0203/resource_name_identifier.go | 2 +- 6 files changed, 173 insertions(+), 1 deletion(-) 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..172ac1fd4 --- /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 an `Update*Request` +and complains if 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..845412236 --- /dev/null +++ b/rules/aip0134/update_mask_optional_behavior_test.go @@ -0,0 +1,51 @@ +// 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 ( + "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) + } + }) + } +} 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), }} }