fix(cli): resolve import path resolution (#1545)

* fix(cli): fix import resolution

* chore(cli): fix integration test

* chore(cli): compile every proto file separatelly to avoid import and compilation collisions

* chore(cli): add integration test for thrid_party dir scenario

* chore(cli): simplify test

* chore(cli): expand comment

Author: quirogas

cmd/api-linter/cli.go

@@ -24,6 +24,7 @@ import (
 	"strings"
 
 	"github.com/bufbuild/protocompile"
+	"github.com/bufbuild/protocompile/linker"
 	"github.com/bufbuild/protocompile/reporter"
 	"github.com/googleapis/api-linter/v2/internal"
 	"github.com/googleapis/api-linter/v2/lint"
@@ -183,25 +184,33 @@ func (c *cli) lint(rules lint.RuleRegistry, configs lint.Configs) error {
 		Reporter:       rep,
 	}
 
-	// Compile files.
-	files, err := compiler.Compile(context.Background(), c.ProtoFiles...)
-
-	// After compilation, check if the handler collected any errors.
-	// This is the primary source of truth for parse errors when using a
-	// custom reporter that continues on error.
-	if len(collectedErrors) > 0 {
-		errorStrings := make([]string, len(collectedErrors))
-		for i, e := range collectedErrors {
-			errorStrings[i] = e.Error()
+	// Compile each file individually to avoid possible collisions
+	// between a linted file that imports other files that are also being linted.
+	// Otherwise, both the import resolver and the file will be "duplicated".
+	var compiledFiles linker.Files
+	for _, protoFile := range c.ProtoFiles {
+		// The compiler returns a slice of files, even for a single input file.
+		f, err := compiler.Compile(context.Background(), protoFile)
+		// After compilation, check if the handler collected any errors.
+		// This is the primary source of truth for parse errors when using a
+		// custom reporter that continues on error.
+		if len(collectedErrors) > 0 {
+			errorStrings := make([]string, len(collectedErrors))
+			for i, e := range collectedErrors {
+				errorStrings[i] = e.Error()
+			}
+			return errors.New(strings.Join(errorStrings, "\n"))
 		}
-		return errors.New(strings.Join(errorStrings, "\n"))
-	}
 
-	// If the reporter has no errors, but the compiler still returned one,
-	// it's a fatal, non-recoverable error.
-	if err != nil {
-		return err
+		// If the reporter has no errors, but the compiler still returned one,
+		// it's a fatal, non-recoverable error.
+		if err != nil {
+			return err
+		}
+		// Append the compiled file(s) to the slice.
+		compiledFiles = append(compiledFiles, f...)
 	}
+	files := compiledFiles
 
 	// The compiler returns a slice of `*linker.File`, which is the compiler's
 	// internal representation. We convert this to a slice of the standard
@@ -413,9 +422,9 @@ func resolveImports(imports []string) []string {
 			evaluatedAbsPath = filepath.Clean(absPath)
 		}
 
-		// Check if the current import path's canonical form is the CWD's canonical form
-		// or a subdirectory of it. If so, it's covered by ".", so we skip it.
-		if evaluatedAbsPath == evaluatedCwd || strings.HasPrefix(evaluatedAbsPath, evaluatedCwd+string(os.PathSeparator)) {
+		// Check if the current import path's canonical form is the CWD's canonical form.
+		// If so, it's covered by ".", so we skip it.
+		if evaluatedAbsPath == evaluatedCwd {
 			continue
 		}
 

cmd/api-linter/cli_test.go

@@ -109,7 +109,7 @@ func TestResolveImports(t *testing.T) {
 				}
 			},
 			want: func(_ string) []string {
-				return []string{"."} // "test_dir" should be covered by "."
+				return []string{".", "test_dir"}
 			},
 		},
 		{
@@ -123,7 +123,7 @@ func TestResolveImports(t *testing.T) {
 				}
 			},
 			want: func(_ string) []string {
-				return []string{"."}
+				return []string{".", "test_dir"}
 			},
 		},
 		{
@@ -150,7 +150,7 @@ func TestResolveImports(t *testing.T) {
 				}
 			},
 			want: func(externalDir string) []string {
-				return []string{".", externalDir}
+				return []string{".", "./relative_dir", "test_dir", externalDir}
 			},
 		},
 		{
@@ -160,7 +160,7 @@ func TestResolveImports(t *testing.T) {
 			},
 			setup: defaultSetup,
 			want: func(_ string) []string {
-				return []string{"."} // Should still resolve to just "." as non_existent_dir is relative to CWD
+				return []string{".", "non_existent_dir"}
 			},
 		},
 		{

cmd/api-linter/integration_test.go

@@ -233,8 +233,8 @@ func TestMultipleFilesFromParentDir(t *testing.T) {
 	// a.proto imports b.proto.
 	if err := writeFile(filepath.Join(protoDir, "a.proto"), `
 		syntax = "proto3";
-		package grandparent.parent;
-		import "grandparent/parent/b.proto";
+		package parent;
+		import "parent/b.proto";
 		message A {
 			B b_field = 1;
 		}
@@ -244,7 +244,7 @@ func TestMultipleFilesFromParentDir(t *testing.T) {
 
 	if err := writeFile(filepath.Join(protoDir, "b.proto"), `
 		syntax = "proto3";
-		package grandparent.parent;
+		package parent;
 		message B {}
 	`); err != nil {
 		t.Fatal(err)
@@ -277,6 +277,108 @@ func TestMultipleFilesFromParentDir(t *testing.T) {
 	}
 }
 
+func TestImportFromAnotherRoot(t *testing.T) {
+	// This test case is based on a scenario described in:
+	// https://github.com/googleapis/api-linter/pull/1519
+	//
+	// It checks that the linter can correctly resolve imports when
+	// one import is in a directory provided by `-I` and another
+	// is relative to the working directory. i.e.
+	//
+	// .
+	// ├── api
+	// │   ├── common
+	// │   │   └── common.proto
+	// │   └── v1
+	// │       └── test.proto
+	// └── third_party
+	//     └── google
+	//         └── api
+	//             └── field_behavior.proto
+
+	projDir, err := os.MkdirTemp("", "proj")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(projDir)
+
+	// Create the subdirectory for protos.
+	apiV1Dir := filepath.Join(projDir, "api", "v1")
+	if err := os.MkdirAll(apiV1Dir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	apiCommonDir := filepath.Join(projDir, "api", "common")
+	if err := os.MkdirAll(apiCommonDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	thirdPartyDir := filepath.Join(projDir, "third_party", "other", "api")
+	if err := os.MkdirAll(thirdPartyDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+
+	// Write the proto files.
+	if err := writeFile(filepath.Join(apiV1Dir, "test.proto"), `
+		syntax = "proto3";
+
+		package api.v1;
+
+		import "other/api/useful.proto";
+		import "api/common/common.proto";
+
+		message Test {
+		  other.api.Bar bar = 1;
+		  api.common.Foo foo = 2;
+		}
+	`); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := writeFile(filepath.Join(apiCommonDir, "common.proto"), `
+		syntax = "proto3";
+
+		package api.common;
+
+		message Foo {}
+	`); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := writeFile(filepath.Join(thirdPartyDir, "useful.proto"), `
+		syntax = "proto3";
+
+		package other.api;
+
+		message Bar{}
+	`); err != nil {
+		t.Fatal(err)
+	}
+
+	// Change the working directory to the project root.
+	oldWD, err := os.Getwd()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Chdir(projDir); err != nil {
+		t.Fatal(err)
+	}
+	defer os.Chdir(oldWD)
+
+	args := []string{
+		"-I", "third_party",
+		filepath.Join("api", "v1", "test.proto"),
+	}
+
+	err = runCLI(args)
+
+	if err != nil && !errors.Is(err, ExitForLintFailure) {
+		if strings.Contains(err.Error(), "not found") {
+			t.Errorf("Linter failed with unexpected 'file not found' error: %v", err)
+		} else {
+			t.Fatalf("Linter failed with unexpected error: %v", err)
+		}
+	}
+}
+
 func TestExitStatusForLintFailure(t *testing.T) {
 	type testCase struct{ testName, rule, proto string }
 	failCase := testCase{

internal/version.go

@@ -14,4 +14,4 @@
 package internal
 
 // Version is the current tagged release of the library.
-const Version = "2.0.0-beta.3"
+const Version = "2.0.0-beta.4"