imports_test.go: check package dependencies
Add a test to prevent unwanted dependencies.
Change-Id: Ia6755057b1f7fd047ab7c33c3125e417acb04615
Reviewed-on: https://go-review.googlesource.com/c/oscar/+/630356
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/go.mod b/go.mod
index b466639..eca6661 100644
--- a/go.mod
+++ b/go.mod
@@ -11,9 +11,10 @@
github.com/shurcooL/githubv4 v0.0.0-20240727222349-48295856cce7
go.opentelemetry.io/otel v1.32.0
go.opentelemetry.io/otel/metric v1.32.0
+ golang.org/x/mod v0.22.0
golang.org/x/net v0.31.0
golang.org/x/oauth2 v0.24.0
- golang.org/x/oscar/internal/gcp v0.0.0-20241121213744-9a4fac3173f0
+ golang.org/x/oscar/internal/gcp v0.0.0-20241122155449-dd28296a9ae0
golang.org/x/term v0.26.0
golang.org/x/tools v0.27.0
gopkg.in/yaml.v3 v3.0.1
diff --git a/go.sum b/go.sum
index 4c9ec0d..d0d4972 100644
--- a/go.sum
+++ b/go.sum
@@ -195,6 +195,8 @@
golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc=
golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
+golang.org/x/mod v0.22.0 h1:D4nJWe9zXqHOmWqj4VMOJhvzj7bEZg4wEYa759z1pH4=
+golang.org/x/mod v0.22.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY=
golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20190213061140-3a22650c66bd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
@@ -209,8 +211,8 @@
golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
golang.org/x/oauth2 v0.24.0 h1:KTBBxWqUa0ykRPLtV69rRto9TLXcqYkeswu48x/gvNE=
golang.org/x/oauth2 v0.24.0/go.mod h1:XYTD2NtWslqkgxebSiOHnXEap4TF09sJSc7H1sXbhtI=
-golang.org/x/oscar/internal/gcp v0.0.0-20241121213744-9a4fac3173f0 h1:AcWfPRR5l8xzk+kTPEegvYBNvSK5aA38OVRv5T0vIU0=
-golang.org/x/oscar/internal/gcp v0.0.0-20241121213744-9a4fac3173f0/go.mod h1:O1+YtaRykkq9M83UlGvCcYLPgEwofG6KbD+FwDXwj/s=
+golang.org/x/oscar/internal/gcp v0.0.0-20241122155449-dd28296a9ae0 h1:C/djlnYqFIzAhEl6bswKdJhu6UM/ATByB668fStVx1M=
+golang.org/x/oscar/internal/gcp v0.0.0-20241122155449-dd28296a9ae0/go.mod h1:O1+YtaRykkq9M83UlGvCcYLPgEwofG6KbD+FwDXwj/s=
golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
diff --git a/internal/importtest/import_test.go b/internal/importtest/import_test.go
new file mode 100644
index 0000000..0012360
--- /dev/null
+++ b/internal/importtest/import_test.go
@@ -0,0 +1,303 @@
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// The test in this file enforces restrictions on package dependencies.
+
+package oscar
+
+import (
+ "errors"
+ "fmt"
+ "maps"
+ "slices"
+ "strings"
+ "testing"
+
+ "golang.org/x/mod/module"
+ "golang.org/x/tools/go/packages"
+)
+
+// A rule describes the allowed dependencies for one or more packages.
+// The patterns in the rule are a subset of the patterns used by the go command.
+// A pattern is either a literal import path, which matches only itself,
+// or of the form "P/...", which matches P and any path with the prefix "P/".
+// As a special case, the pattern "..." matches every path.
+type rule struct {
+ // A pattern describing the packages to which this rule applies.
+ // Paths are relative to the module root (this directory).
+ // For example, "internal/gaby".
+ packages string
+
+ // Patterns describing paths that the above packages can import.
+ // A package to which this rule applies must only import a package
+ // matched by one of these patterns.
+ // These patterns describe full import paths, for example "rsc.io/...".
+ // Standard library packages are always allowed.
+ // A value of nil means only standard library packages are allowed.
+ allow []string
+}
+
+// Packages that are always allowed.
+var alwaysAllowed = []string{
+ "golang.org/...",
+ "google.golang.org/...",
+ "github.com/golang/...",
+ "github.com/google/...",
+ "github.com/cockroachdb/pebble",
+ "rsc.io/markdown",
+ "rsc.io/omap",
+ "rsc.io/ordered",
+ "rsc.io/top",
+ "github.com/shurcooL/githubv4",
+ "github.com/shurcooL/graphql/...",
+ "gopkg.in/yaml.v3",
+}
+
+var anything = []string{"..."}
+
+// rules is the list of rules for this module.
+// For each package, only the first matching rule is used.
+var rules = []*rule{
+ {
+ packages: "internal/syncdb/...",
+ allow: anything,
+ },
+ {
+ packages: "internal/devtools/...",
+ allow: anything,
+ },
+ {
+ packages: "internal/gaby/...",
+ allow: anything,
+ },
+ {
+ packages: "internal/gcp/...",
+ allow: anything,
+ },
+ {
+ packages: "internal/pebble/...",
+ allow: anything,
+ },
+ // The remaining packages under internal should not depend on GCP.
+ {
+ packages: "internal/...",
+ allow: alwaysAllowed,
+ },
+}
+
+const pkgPathPrefix = "golang.org/x/oscar/"
+
+type Package = packages.Package
+
+func TestDependencies(t *testing.T) {
+ for _, r := range rules {
+ if err := r.check(); err != nil {
+ t.Fatal(err)
+ }
+ }
+
+ cfg := &packages.Config{
+ Mode: packages.NeedName | packages.NeedImports | packages.NeedDeps,
+ Tests: true,
+ }
+ pkgs, err := packages.Load(cfg, pkgPathPrefix+"...")
+ if err != nil {
+ t.Fatal(err)
+ }
+ for _, pkg := range pkgs {
+ if len(pkg.Errors) > 0 {
+ t.Errorf("package %s has errors: %v", pkg.PkgPath, pkg.Errors)
+ continue
+ }
+ relativePkgPath := strings.TrimPrefix(pkg.PkgPath, pkgPathPrefix)
+ var mr *rule
+ for _, r := range rules {
+ if matchPattern(r.packages, relativePkgPath) {
+ mr = r
+ break
+ }
+ }
+ if mr == nil {
+ t.Fatalf("no rule matches import path %q", relativePkgPath)
+ }
+ deps := dependencies(pkg)
+ for _, d := range deps {
+ if !mr.allowed(d) {
+ t.Errorf("package %s depends on disallowed package %s", relativePkgPath, d)
+ }
+ }
+ }
+}
+
+// dependencies returns all the package paths that pkgdepends on, recursively.
+func dependencies(pkg *Package) []string {
+ depset := map[string]bool{}
+
+ var deps func(*Package, int)
+ deps = func(p *Package, level int) {
+ if inStdLib(p.PkgPath) || depset[p.PkgPath] {
+ return
+ }
+ for _, ip := range p.Imports {
+ deps(ip, level+1)
+ depset[ip.PkgPath] = true
+ }
+ }
+
+ deps(pkg, 1)
+ return slices.Collect(maps.Keys(depset))
+}
+
+func (r *rule) allowed(pkg string) bool {
+ // Standard library packages are always allowed.
+ if inStdLib(pkg) {
+ return true
+ }
+ for _, a := range r.allow {
+ if matchPattern(a, pkg) {
+ return true
+ }
+ }
+ return false
+}
+
+func TestAllowed(t *testing.T) {
+ r := &rule{packages: "...", allow: []string{"a.com/b/...", "x.org/y/z"}}
+ if err := r.check(); err != nil {
+ t.Fatal(err)
+ }
+ for _, tc := range []struct {
+ pkg string
+ want bool
+ }{
+ {"a.com/b", true},
+ {"a.com/b/c", true},
+ {"a.com/b/c/d/e", true},
+ {"x.org/y", false},
+ {"x.org/y/z", true},
+ {"x.org/y/z/w", false},
+ {"a.org/b/c", false},
+ } {
+ got := r.allowed(tc.pkg)
+ if got != tc.want {
+ t.Errorf("%q: got %t, want %t", tc.pkg, got, tc.want)
+ }
+ }
+}
+
+// inStdLib reports whether the given import path could be part of the Go standard library,
+// by reporting whether the first component lacks a '.'.
+func inStdLib(path string) bool {
+ if i := strings.IndexByte(path, '/'); i != -1 {
+ path = path[:i]
+ }
+ return !strings.Contains(path, ".")
+}
+
+// check validates a rule.
+func (r *rule) check() error {
+ if err := checkPattern(r.packages); err != nil {
+ return err
+ }
+ for _, a := range r.allow {
+ if err := checkPattern(a); err != nil {
+ return err
+ }
+ if inStdLib(a) {
+ return fmt.Errorf("%q matches standard library packages, which are always allowed", a)
+ }
+ }
+ return nil
+}
+
+// checkPattern validates a pattern.
+func checkPattern(pattern string) error {
+ prefix, found := strings.CutSuffix(pattern, "...")
+ if found {
+ if prefix == "" {
+ return nil
+ }
+ if prefix[len(prefix)-1] != '/' {
+ return fmt.Errorf("invalid pattern %q: does not end in '/...'", pattern)
+ }
+ prefix = prefix[:len(prefix)-1]
+ }
+ if strings.Contains(prefix, "...") {
+ return fmt.Errorf("invalid pattern %q: '...' not at the end", pattern)
+ } else if prefix == "" {
+ return errors.New("empty pattern")
+ }
+ return module.CheckImportPath(prefix)
+}
+
+func TestCheckPattern(t *testing.T) {
+ for _, tc := range []struct {
+ in string
+ want string
+ }{
+ {"a/b/c", ""},
+ {"a/b/c/...", ""},
+ {"...", ""},
+ {"", "empty"},
+ {"a/.../b", "invalid"},
+ {"a/b/", "malformed"},
+ {"a/b/....", "invalid"},
+ {"/...", "empty"},
+ {"/a/b/...", "malformed"},
+ } {
+ got := checkPattern(tc.in)
+ if got == nil {
+ if tc.want != "" {
+ t.Errorf("%q: unexpected success", tc.in)
+ }
+ } else if tc.want == "" {
+ t.Errorf("%q: unexpected error %q", tc.in, got)
+ } else if !strings.Contains(got.Error(), tc.want) {
+ t.Errorf("%q: error %q does not contain %q", tc.in, got, tc.want)
+ }
+ }
+}
+
+// matchPattern reports whether pattern matches target.
+// It assumes pattern has been validated with checkPattern.
+// matchPattern will treat a test package path, with the suffix ".test",
+// as equivalent to the non-test package.
+func matchPattern(pattern, target string) bool {
+ prefix, found := strings.CutSuffix(pattern, "...")
+ if !found {
+ return target == pattern || strings.TrimSuffix(target, ".test") == pattern
+ }
+ if prefix == "" {
+ // The pattern "..." matches everything.
+ return true
+ }
+
+ matches := func(targ string) bool {
+ return targ == prefix[:len(prefix)-1] || strings.HasPrefix(targ, prefix)
+ }
+
+ return matches(target) || matches(strings.TrimSuffix(target, ".test"))
+}
+
+func TestMatchPattern(t *testing.T) {
+ target := "some/import/path.test"
+ for _, tc := range []struct {
+ pattern string
+ want bool
+ }{
+ {"some/import/path", true},
+ {"some/import/...", true},
+ {"some/...", true},
+ {"...", true},
+ {"some/other/...", false},
+ } {
+ if err := checkPattern(tc.pattern); err != nil {
+ t.Fatal(err)
+ }
+ got := matchPattern(tc.pattern, target)
+ if got != tc.want {
+ t.Errorf("%q: got %t, want %t", tc.pattern, got, tc.want)
+ }
+ }
+}
