From 89bb7d81bad5f9d1113628d7d7ca61938bccd6cc Mon Sep 17 00:00:00 2001 From: Anthony Amadeo Date: Sat, 9 Dec 2023 05:57:40 +0000 Subject: [PATCH 1/2] feat(AIP-213): Lint message fields that should use common types --- docs/rules/0213/common-types-fields.md | 90 +++++++++++++++++++++ docs/rules/0213/common-types-messages.md | 67 +++++++++++++++ docs/rules/0213/index.md | 10 +++ rules/aip0213/aip0213.go | 29 +++++++ rules/aip0213/aip0213_test.go | 27 +++++++ rules/aip0213/common_types_fields.go | 63 +++++++++++++++ rules/aip0213/common_types_fields_test.go | 48 +++++++++++ rules/aip0213/common_types_messages.go | 61 ++++++++++++++ rules/aip0213/common_types_messages_test.go | 60 ++++++++++++++ rules/rules.go | 2 + 10 files changed, 457 insertions(+) create mode 100644 docs/rules/0213/common-types-fields.md create mode 100644 docs/rules/0213/common-types-messages.md create mode 100644 docs/rules/0213/index.md create mode 100644 rules/aip0213/aip0213.go create mode 100644 rules/aip0213/aip0213_test.go create mode 100644 rules/aip0213/common_types_fields.go create mode 100644 rules/aip0213/common_types_fields_test.go create mode 100644 rules/aip0213/common_types_messages.go create mode 100644 rules/aip0213/common_types_messages_test.go diff --git a/docs/rules/0213/common-types-fields.md b/docs/rules/0213/common-types-fields.md new file mode 100644 index 000000000..cee99ba32 --- /dev/null +++ b/docs/rules/0213/common-types-fields.md @@ -0,0 +1,90 @@ +--- +rule: + aip: 213 + name: [core, '0213', common-types-fields] + summary: Message fields with certain names should use a common type. +permalink: /213/common-types-fields +redirect_from: + - /0213/common-types-fields +--- + +# Common types fields + +This rule enforces that message fields with specific names use a common type, as +recommended in [AIP-213][]. + +## Details + +This rule looks at the fields in a message, and complains if it finds a field +with a specific name that doesn't have the corresponding common type. + +Some example pairings of common types and field names that are checked are: + +* `google.protobuf.Duration`: "duration" +* `google.type.Color`: "color", "colour" +* `google.type.PhoneNumber`: "mobile_number", "phone", "phone_number" + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect. +message Book { + string name = 1; + + // Should use `google.type.Color`. + string color = 2; +} +``` + +```proto +// Incorrect. +message Book { + string name = 1; + + // Should use `google.type.PhoneNumber`. + string phone_number = 2; +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +message Book { + string name = 1; + + google.type.Color color = 2; +} +``` + +```proto +// Correct. +message Book { + string name = 1; + + google.type.PhoneNumber phone_number = 2; +} +``` + +## Disabling + +If you need to violate this rule, use a leading comment above the enum. +Remember to also include an [aip.dev/not-precedent][] comment explaining why. + +```proto +message Book { + string name = 1; + // (-- api-linter: core::0213::common-types-fields=disabled + // aip.dev/not-precedent: We need to do this because reasons. --) + google.protobuf.Timestamp expire_time = 2 + [(google.api.field_behavior) = OUTPUT_ONLY]; +} +``` + +If you need to violate this rule for an entire file, place the comment at the +top of the file. + +[aip-213]: https://aip.dev/213 +[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/docs/rules/0213/common-types-messages.md b/docs/rules/0213/common-types-messages.md new file mode 100644 index 000000000..da48afe82 --- /dev/null +++ b/docs/rules/0213/common-types-messages.md @@ -0,0 +1,67 @@ +--- +rule: + aip: 213 + name: [core, '0213', common-types-messages] + summary: Messages containing fields of a common type should use the common type. +permalink: /213/common-types-messages +redirect_from: + - /0213/common-types-messages +--- + +# Common types messages + +This rule enforces that messages that contain the fields of a common type use the +common type itself, as recommended in [AIP-213][]. + +## Details + +This rule looks at the fields in a message, and complains if it finds a set of +field names that all belong to a common type. + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect. +message Book { + string name = 1; + + // Should use `google.type.Interval`. + google.protobuf.Timestamp start_time = 2; + google.protobuf.Timestamp end_time = 3; +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +message Book { + string name = 1; + + // Should use `google.type.Interval`. + google.type.Interval interval = 2; +} +``` + +## Disabling + +If you need to violate this rule, use a leading comment above the enum. +Remember to also include an [aip.dev/not-precedent][] comment explaining why. + +```proto +message Book { + string name = 1; + // (-- api-linter: core::0213::common-types-messages=disabled + // aip.dev/not-precedent: We need to do this because reasons. --) + google.protobuf.Timestamp expire_time = 2 + [(google.api.field_behavior) = OUTPUT_ONLY]; +} +``` + +If you need to violate this rule for an entire file, place the comment at the +top of the file. + +[aip-213]: https://aip.dev/213 +[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/docs/rules/0213/index.md b/docs/rules/0213/index.md new file mode 100644 index 000000000..6064fe14a --- /dev/null +++ b/docs/rules/0213/index.md @@ -0,0 +1,10 @@ +--- +aip_listing: 213 +permalink: /213/ +redirect_from: + - /0213/ +--- + +# Common components + +{% include linter-aip-listing.md aip=213 %} diff --git a/rules/aip0213/aip0213.go b/rules/aip0213/aip0213.go new file mode 100644 index 000000000..fd93c2107 --- /dev/null +++ b/rules/aip0213/aip0213.go @@ -0,0 +1,29 @@ +// 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 aip0213 contains rules defined in https://aip.dev/213. +package aip0213 + +import ( + "github.com/googleapis/api-linter/lint" +) + +// AddRules adds all of the AIP-213 rules to the provided registry. +func AddRules(r lint.RuleRegistry) error { + return r.Register( + 213, + commonTypesFields, + commonTypesMessages, + ) +} diff --git a/rules/aip0213/aip0213_test.go b/rules/aip0213/aip0213_test.go new file mode 100644 index 000000000..bd5057cf4 --- /dev/null +++ b/rules/aip0213/aip0213_test.go @@ -0,0 +1,27 @@ +// 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 aip0213 + +import ( + "testing" + + "github.com/googleapis/api-linter/lint" +) + +func TestAddRules(t *testing.T) { + if err := AddRules(lint.NewRuleRegistry()); err != nil { + t.Errorf("AddRules got an error: %v", err) + } +} diff --git a/rules/aip0213/common_types_fields.go b/rules/aip0213/common_types_fields.go new file mode 100644 index 000000000..f26701221 --- /dev/null +++ b/rules/aip0213/common_types_fields.go @@ -0,0 +1,63 @@ +// 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 aip0213 + +import ( + "fmt" + + "github.com/googleapis/api-linter/lint" + "github.com/jhump/protoreflect/desc" +) + +var fieldNameToCommonType = map[string]string{ + "duration": "google.protobuf.Duration", + + "color": "google.type.Color", + "colour": "google.type.Color", + + "dollars": "google.type.Money", + "euros": "google.type.Money", + "money": "google.type.Money", + "pounds": "google.type.Money", + "yen": "google.type.Money", + "yuan": "google.type.Money", + + "mobile_number": "google.type.PhoneNumber", + "phone": "google.type.PhoneNumber", + "phone_number": "google.type.PhoneNumber", + + "clock_time": "google.type.TimeOfDay", + "time_of_day": "google.type.TimeOfDay", + + "timezone": "google.type.TimeZone", + "time_zone": "google.type.TimeZone", +} + +var commonTypesFields = &lint.FieldRule{ + Name: lint.NewRuleName(213, "common-types-fields"), + LintField: func(f *desc.FieldDescriptor) []lint.Problem { + // Flag this field if it has a name in `fieldNameToCommonType` but + // doesn't have its corresponding type. + if messageType, ok := fieldNameToCommonType[f.GetName()]; ok { + if f.GetMessageType() == nil || f.GetMessageType().GetFullyQualifiedName() != messageType { + return []lint.Problem{{ + Message: fmt.Sprintf("Consider using the common type %q.", messageType), + Descriptor: f, + }} + } + } + return nil + }, +} diff --git a/rules/aip0213/common_types_fields_test.go b/rules/aip0213/common_types_fields_test.go new file mode 100644 index 000000000..75817e6b2 --- /dev/null +++ b/rules/aip0213/common_types_fields_test.go @@ -0,0 +1,48 @@ +// 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 aip0213 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +func TestCommonTypesFields(t *testing.T) { + for _, test := range []struct { + name string + Field string + problems testutils.Problems + }{ + {"ValidCommonType", "google.protobuf.Duration duration", nil}, + {"ValidOtherType", "string bar", nil}, + {"FieldDoesNotUseMessageType", "map duration", testutils.Problems{{Message: "common type"}}}, + {"FieldDoesNotUseCommonType", "int32 duration", testutils.Problems{{Message: "common type"}}}, + } { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/protobuf/duration.proto"; + + message Foo { + {{.Field}} = 1; + } + `, test) + field := f.GetMessageTypes()[0].GetFields()[0] + if diff := test.problems.SetDescriptor(field).Diff(commonTypesFields.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} diff --git a/rules/aip0213/common_types_messages.go b/rules/aip0213/common_types_messages.go new file mode 100644 index 000000000..3f427695f --- /dev/null +++ b/rules/aip0213/common_types_messages.go @@ -0,0 +1,61 @@ +// 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 aip0213 + +import ( + "fmt" + + "github.com/googleapis/api-linter/lint" + "github.com/jhump/protoreflect/desc" +) + +// A map from each common type to its constituent fields. +var commonTypesToFields = map[string][]string{ + "google.type.Color": {"red", "green", "blue"}, + "google.type.Date": {"year", "month", "day"}, + "google.type.Fraction": {"numerator", "denominator"}, + "google.type.Interval": {"start_time", "end_time"}, + "google.type.LatLng": {"latitude", "longitude"}, + "google.type.TimeOfDay": {"hours", "minutes", "seconds"}, + "google.type.Quaternion": {"x", "y", "z", "w"}, +} + +var commonTypesMessages = &lint.MessageRule{ + Name: lint.NewRuleName(213, "common-types-messages"), + LintMessage: func(m *desc.MessageDescriptor) []lint.Problem { + problems := []lint.Problem{} + + // If a message has all the value fields, it should consider using the + // key type. + for commonType, fields := range commonTypesToFields { + if messageContainsAllFields(m, fields) { + problems = append(problems, lint.Problem{ + Message: fmt.Sprintf("Message contains fields %v. Consider using the common type %q.", fields, commonType), + Descriptor: m, + }) + } + } + return problems + }, +} + +func messageContainsAllFields(m *desc.MessageDescriptor, fieldNames []string) bool { + for _, fieldName := range fieldNames { + if m.FindFieldByName(fieldName) == nil { + return false + } + } + return true +} diff --git a/rules/aip0213/common_types_messages_test.go b/rules/aip0213/common_types_messages_test.go new file mode 100644 index 000000000..6d7d49359 --- /dev/null +++ b/rules/aip0213/common_types_messages_test.go @@ -0,0 +1,60 @@ +// 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 aip0213 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +func TestCommonTypesMessages(t *testing.T) { + for _, test := range []struct { + name string + Field1 string + Field2 string + Field3 string + problems testutils.Problems + }{ + {"ValidCommonType", "google.protobuf.Duration duration", "", "", nil}, + {"ValidOtherType", "string bar", "", "", nil}, + {"ValidMessageDoesNotHaveAllFieldsFromCommonType", "string red", "string green", "", nil}, + {"MessageHasAllFieldsFromCommonType", "string red", "string green", "string blue", testutils.Problems{{Message: "common type"}}}, + {"MessageHasReorderedFieldsFromCommonType", "string green", "string blue", "string red", testutils.Problems{{Message: "common type"}}}, + {"MessageHasAdditionalFieldsOutsideCommonType", "double latitude", "double longitude", "double altitude", testutils.Problems{{Message: "common type"}}}, + } { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + import "google/protobuf/duration.proto"; + + message Foo { + {{.Field1}} = 1; + + {{if .Field2}} + {{.Field2}} = 2; + {{end}} + + {{if .Field3}} + {{.Field3}} = 3; + {{end}} + } + `, test) + message := f.GetMessageTypes()[0] + if diff := test.problems.SetDescriptor(message).Diff(commonTypesMessages.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} diff --git a/rules/rules.go b/rules/rules.go index b22fa0217..e1eecb278 100644 --- a/rules/rules.go +++ b/rules/rules.go @@ -87,6 +87,7 @@ import ( "github.com/googleapis/api-linter/rules/aip0192" "github.com/googleapis/api-linter/rules/aip0202" "github.com/googleapis/api-linter/rules/aip0203" + "github.com/googleapis/api-linter/rules/aip0213" "github.com/googleapis/api-linter/rules/aip0214" "github.com/googleapis/api-linter/rules/aip0215" "github.com/googleapis/api-linter/rules/aip0216" @@ -137,6 +138,7 @@ var aipAddRulesFuncs = []addRulesFuncType{ aip0192.AddRules, aip0202.AddRules, aip0203.AddRules, + aip0213.AddRules, aip0214.AddRules, aip0215.AddRules, aip0216.AddRules, From 016aa21ca9a6e402c4d77b0a6e92de714c9b919d Mon Sep 17 00:00:00 2001 From: Anthony Amadeo Date: Wed, 7 Feb 2024 00:06:45 +0000 Subject: [PATCH 2/2] Address comments --- docs/rules/0213/common-types-fields.md | 2 +- rules/aip0213/common_types_fields.go | 58 +++++++++++++++++++---- rules/aip0213/common_types_fields_test.go | 5 ++ 3 files changed, 56 insertions(+), 9 deletions(-) diff --git a/docs/rules/0213/common-types-fields.md b/docs/rules/0213/common-types-fields.md index cee99ba32..2be5bcfa9 100644 --- a/docs/rules/0213/common-types-fields.md +++ b/docs/rules/0213/common-types-fields.md @@ -20,7 +20,7 @@ with a specific name that doesn't have the corresponding common type. Some example pairings of common types and field names that are checked are: -* `google.protobuf.Duration`: "duration" +* `google.protobuf.Duration`: "delay", "duration", "timeout" * `google.type.Color`: "color", "colour" * `google.type.PhoneNumber`: "mobile_number", "phone", "phone_number" diff --git a/rules/aip0213/common_types_fields.go b/rules/aip0213/common_types_fields.go index f26701221..d83bb8fda 100644 --- a/rules/aip0213/common_types_fields.go +++ b/rules/aip0213/common_types_fields.go @@ -16,23 +16,26 @@ package aip0213 import ( "fmt" + "strings" "github.com/googleapis/api-linter/lint" "github.com/jhump/protoreflect/desc" ) var fieldNameToCommonType = map[string]string{ + "delay": "google.protobuf.Duration", "duration": "google.protobuf.Duration", + "timeout": "google.protobuf.Duration", "color": "google.type.Color", "colour": "google.type.Color", - "dollars": "google.type.Money", - "euros": "google.type.Money", - "money": "google.type.Money", - "pounds": "google.type.Money", - "yen": "google.type.Money", - "yuan": "google.type.Money", + "balance": "google.type.Money", + "cost": "google.type.Money", + "fare": "google.type.Money", + "fee": "google.type.Money", + "price": "google.type.Money", + "spend": "google.type.Money", "mobile_number": "google.type.PhoneNumber", "phone": "google.type.PhoneNumber", @@ -48,9 +51,10 @@ var fieldNameToCommonType = map[string]string{ var commonTypesFields = &lint.FieldRule{ Name: lint.NewRuleName(213, "common-types-fields"), LintField: func(f *desc.FieldDescriptor) []lint.Problem { - // Flag this field if it has a name in `fieldNameToCommonType` but + // Flag this field if it ends with a name in `fieldNameToCommonType` but // doesn't have its corresponding type. - if messageType, ok := fieldNameToCommonType[f.GetName()]; ok { + commonFieldName := commonFieldNameSuffix(f.GetName()) + if messageType, ok := fieldNameToCommonType[commonFieldName]; ok { if f.GetMessageType() == nil || f.GetMessageType().GetFullyQualifiedName() != messageType { return []lint.Problem{{ Message: fmt.Sprintf("Consider using the common type %q.", messageType), @@ -61,3 +65,41 @@ var commonTypesFields = &lint.FieldRule{ return nil }, } + +// Returns the common field name if `fieldName` ends in one. +func commonFieldNameSuffix(fieldName string) string { + fieldParts := strings.Split(fieldName, "_") + for name := range fieldNameToCommonType { + nameParts := strings.Split(name, "_") + // Check if `fieldName` ends with the same underscore-separated terms as + // `name`. + if strListContains(reverseStrList(fieldParts), reverseStrList(nameParts)) { + return name + } + } + return "" +} + +// Returns true if list `s` contains list `subList` in order. +func strListContains(s []string, subList []string) bool { + if len(subList) > len(s) { + return false + } + for i := 0; i < len(subList); i++ { + if s[i] != subList[i] { + return false + } + } + return true +} + +// Returns list `s` in reverse order. +func reverseStrList(s []string) []string { + out := make([]string, len(s)) + copy(out, s) + + for i, j := 0, len(s)-1; i < j; i, j = i+1, j-1 { + out[i], out[j] = out[j], out[i] + } + return out +} diff --git a/rules/aip0213/common_types_fields_test.go b/rules/aip0213/common_types_fields_test.go index 75817e6b2..84d6b8e5a 100644 --- a/rules/aip0213/common_types_fields_test.go +++ b/rules/aip0213/common_types_fields_test.go @@ -27,13 +27,18 @@ func TestCommonTypesFields(t *testing.T) { problems testutils.Problems }{ {"ValidCommonType", "google.protobuf.Duration duration", nil}, + {"ValidFieldEndingInCommonTerm", "google.protobuf.Duration max_duration", nil}, + {"ValidFieldEndingInMultipleCommonTerms", "google.type.TimeOfDay end_time_of_day", nil}, {"ValidOtherType", "string bar", nil}, {"FieldDoesNotUseMessageType", "map duration", testutils.Problems{{Message: "common type"}}}, + {"FieldEndingInCommonTermDoesNotUseMessageType", "map max_duration", testutils.Problems{{Message: "common type"}}}, + {"FieldEndingInMultipleCommonTermsDoesNotUseMessageType", "map end_time_of_day", testutils.Problems{{Message: "common type"}}}, {"FieldDoesNotUseCommonType", "int32 duration", testutils.Problems{{Message: "common type"}}}, } { t.Run(test.name, func(t *testing.T) { f := testutils.ParseProto3Tmpl(t, ` import "google/protobuf/duration.proto"; + import "google/type/timeofday.proto"; message Foo { {{.Field}} = 1;