-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(AIP-213): Lint message fields that should use common types #1297
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
--- | ||
aip_listing: 213 | ||
permalink: /213/ | ||
redirect_from: | ||
- /0213/ | ||
--- | ||
|
||
# Common components | ||
|
||
{% include linter-aip-listing.md aip=213 %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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, | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest checking for postfixes instead of equality. It's common for fields to contain more than just the type, for example "background_color", "max_retry_duration" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
// 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 | ||
}, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<int32, int32> 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) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for selecting common field names, I suggest using the public google API definitions here to identify commonly used patterns, for example: https://github.com/search?q=repo%3Agoogleapis%2Fgoogleapis+google.type.Money+lang%3Aproto&type=code
To give one specific example, for google.type.Money, I suggest dropping all proposed aliases, and instead just linting fields ending in "price", "cost", "_fee". you can look up the public google API repository here to see what commonly used patterns we use today: https://github.com/search?q=repo%3Agoogleapis%2Fgoogleapis+google.type.Money+lang%3Aproto&type=code
if the currency is implied by the field name we probably don't want to enforce using the Money type, since it's duplicating information and can lead to ambiguous / confusing behavior when the currency conflicts with the field name
also, "money" is very unlikely to be used as a field name, I have not seen examples of that. Perhaps "money_value" since it's used in 3 examples in public google APIs? https://github.com/search?q=repo%3Agoogleapis%2Fgoogleapis+%22google.type.Money+money%22+lang%3Aproto&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done