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) + } + }) + } +}