Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions docs/rules/0213/common-types-fields.md
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
67 changes: 67 additions & 0 deletions docs/rules/0213/common-types-messages.md
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
10 changes: 10 additions & 0 deletions docs/rules/0213/index.md
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 %}
29 changes: 29 additions & 0 deletions rules/aip0213/aip0213.go
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,
)
}
27 changes: 27 additions & 0 deletions rules/aip0213/aip0213_test.go
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)
}
}
63 changes: 63 additions & 0 deletions rules/aip0213/common_types_fields.go
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",
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"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
Copy link
Contributor

Choose a reason for hiding this comment

The 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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
},
}
48 changes: 48 additions & 0 deletions rules/aip0213/common_types_fields_test.go
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)
}
})
}
}
Loading