Skip to content

Commit

Permalink
feat(AIP-215): augment foreign type checking (#1467)
Browse files Browse the repository at this point in the history
  • Loading branch information
shollyman authored Feb 11, 2025
1 parent a14ed3d commit 6c514fb
Show file tree
Hide file tree
Showing 4 changed files with 310 additions and 0 deletions.
75 changes: 75 additions & 0 deletions docs/rules/0215/foreign-type-reference.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
---
rule:
aip: 215
name: [core, '0215', foreign-type-reference]
summary: API should not reference foreign types outside of the API scope.
permalink: /215/foreign-type-reference
redirect_from:
- /0215/foreign-type-reference
---

# Versioned packages

This rule enforces that none of the fields in an API reference message types in a different
proto package namespace other than well-known common packages.

## Details

This rule examines all fields in an API's messages and complains if the package of the
referenced message type is not the same or from one of the common packages such as
`google.api`, `google.protobuf`, etc.

## Examples

**Incorrect** code for this rule:

```proto
// Incorrect.
package foo.bar;
import "some/other.proto";
message SomeMessage {
some.OtherMessage other_message = 1;
}
```

**Correct** code for this rule:

```proto
// Correct.
package foo.bar;
import "other.proto";
message SomeMessage {
OtherMessage other_message = 1;
}
```

## Known issues

This check only allows subpackage usage within a versioned path, but generates warnings for unversioned subpackage usage.
It also ignores if the referenced package is a "common" package like `google.api`, or if the package path ends in `.type` indicating
the package is an AIP-213 component package.

Examples of foreign type references and their expected results:

| Calling Package | Referenced Package | Result |
| --------------- | ------------------ | ------------ |
| foo.bar | foo.xyz | lint warning |
| foo.v2.bar | foo.v2.xyz | ok |
| foo.bar | foo.type | ok |
| foo.bar | google.api | ok |

## Disabling

If you need to violate this rule, place the comment above the package statement.
Remember to also include an [aip.dev/not-precedent][] comment explaining why.

```proto
// (-- api-linter: core::0215::foreign-type-reference=disabled
// aip.dev/not-precedent: We need to do this because reasons. --)
package foo.bar;
```

[aip-215]: https://aip.dev/215
[aip.dev/not-precedent]: https://aip.dev/not-precedent
1 change: 1 addition & 0 deletions rules/aip0215/aip0215.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,6 @@ func AddRules(r lint.RuleRegistry) error {
return r.Register(
215,
versionedPackages,
foreignTypeReference,
)
}
82 changes: 82 additions & 0 deletions rules/aip0215/foreign_type_reference.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// Copyright 2025 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 aip0215

import (
"fmt"
"regexp"
"strings"

"github.com/googleapis/api-linter/lint"
"github.com/googleapis/api-linter/rules/internal/utils"
"github.com/jhump/protoreflect/desc"
"google.golang.org/protobuf/types/descriptorpb"
)

var foreignTypeReference = &lint.FieldRule{
Name: lint.NewRuleName(215, "foreign-type-reference"),
OnlyIf: func(fd *desc.FieldDescriptor) bool {
return fd.GetType() == descriptorpb.FieldDescriptorProto_TYPE_MESSAGE
},
LintField: func(fd *desc.FieldDescriptor) []lint.Problem {
curPkg := getNormalizedPackage(fd)
if curPkg == "" {
return nil // Empty or unavailable package.
}
msg := fd.GetMessageType()
if msg == nil {
return nil // Couldn't resolve type.
}
msgPkg := getNormalizedPackage(msg)
if msgPkg == "" {
return nil // Empty or unavailable package.
}

if utils.IsCommonProto(msg.GetFile()) {
return nil // reference to a well known proto package.
}

if strings.HasSuffix(msgPkg, ".type") {
return nil // AIP-213 component type.
}

if curPkg != msgPkg {
return []lint.Problem{{
Message: fmt.Sprintf("foreign type referenced, current field in %q message in %q", curPkg, msgPkg),
Descriptor: fd,
}}
}

return nil
},
}

// Regexp to capture everything up to a versioned segment.
var versionedPrefix = regexp.MustCompile(`^.*\.v[\d]+(p[\d]+)?(alpha|beta|eap|test)?[\d]*`)

// getNormalizedPackage returns a normalized package path.
// If package cannot be resolved it returns the empty string.
// If the package path has a "versioned" segment, the path is truncated to that segment.
func getNormalizedPackage(d desc.Descriptor) string {
f := d.GetFile()
if f == nil {
return ""
}
pkg := f.GetPackage()
if normPkg := versionedPrefix.FindString(pkg); normPkg != "" {
pkg = normPkg
}
return pkg
}
152 changes: 152 additions & 0 deletions rules/aip0215/foreign_type_reference_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
// Copyright 2025 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 aip0215

import (
"testing"

"github.com/googleapis/api-linter/rules/internal/testutils"
)

// Tests our regexp normalizes strings to the expected path.
func TestVersionNormalization(t *testing.T) {
for _, tc := range []struct {
in string
want string
}{
{
in: "",
want: "",
},
{
in: "foo.bar.baz",
want: "",
},
{
// This one's a bit iffy. Should a version be allowed as the first segment?
in: "v1beta",
want: "",
},
{
in: "foo.v3",
want: "foo.v3",
},
{
in: "foo.v99alpha.bar",
want: "foo.v99alpha",
},
{
in: "foo.v2.bar.v2",
want: "foo.v2.bar.v2",
},
} {
got := versionedPrefix.FindString(tc.in)
if got != tc.want {
t.Errorf("mismatch: in %q, got %q, want %q", tc.in, got, tc.want)
}
}
}

func TestForeignTypeReference(t *testing.T) {

for _, tc := range []struct {
description string
CallingPkg string
ReferencePkg string
ReferencedMsg string
problems testutils.Problems
}{
{
description: "same pkg",
CallingPkg: "same",
ReferencePkg: "same",
ReferencedMsg: "OtherMessage",
problems: nil,
},
{
description: "refers to google.api",
CallingPkg: "same",
ReferencePkg: "google.api",
ReferencedMsg: "google.api.OtherMessage",
problems: nil,
},
{
description: "refers to google.protobuf",
CallingPkg: "same",
ReferencePkg: "google.protobuf",
ReferencedMsg: "google.protobuf.OtherMessage",
problems: nil,
},
{
description: "refers to foreign pkg",
CallingPkg: "same",
ReferencePkg: "other",
ReferencedMsg: "other.OtherMessage",
problems: testutils.Problems{{Message: "foreign type referenced"}},
},
{
description: "unversioned subpackage",
CallingPkg: "somepackage",
ReferencePkg: "somepackage.sub",
ReferencedMsg: "somepackage.sub.OtherMessage",
problems: testutils.Problems{{Message: "foreign type referenced"}},
},
{
description: "versioned subpackage",
CallingPkg: "somepackage.v6",
ReferencePkg: "somepackage.v6.sub",
ReferencedMsg: "somepackage.v6.sub.OtherMessage",
problems: nil,
},
{
description: "versioned deep subpackaging",
CallingPkg: "somepackage.v1.abc",
ReferencePkg: "somepackage.v1.lol.xyz",
ReferencedMsg: "somepackage.v1.lol.xyz.OtherMessage",
problems: nil,
},
{
description: "refers to component package",
CallingPkg: "somepackage",
ReferencePkg: "otherpackage.type",
ReferencedMsg: "otherpackage.type.OtherMessage",
problems: nil,
},
} {
t.Run(tc.description, func(t *testing.T) {
files := testutils.ParseProto3Tmpls(t, map[string]string{
"calling.proto": `
package {{.CallingPkg}};
import "ref.proto";
message Caller {
string foo = 1;
{{.ReferencedMsg}} bar = 2;
}
`,
"ref.proto": `
package {{.ReferencePkg}};
message OtherMessage {
int32 baz = 1;
}
`,
}, tc)
file := files["calling.proto"]
field := file.GetMessageTypes()[0].GetFields()[1]
if diff := tc.problems.SetDescriptor(field).Diff(foreignTypeReference.Lint(file)); diff != "" {
t.Error(diff)
}
})
}
}

0 comments on commit 6c514fb

Please sign in to comment.