diff --git a/cmd/api-linter/cli.go b/cmd/api-linter/cli.go index ae37cde9d..73288c8d8 100644 --- a/cmd/api-linter/cli.go +++ b/cmd/api-linter/cli.go @@ -19,6 +19,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "strings" "sync" @@ -79,7 +80,7 @@ func newCli(args []string) *cli { fs.StringArrayVar(&protoDescFlag, "descriptor-set-in", nil, "The file containing a FileDescriptorSet for searching proto imports.\nMay be specified multiple times.") fs.StringArrayVar(&ruleEnableFlag, "enable-rule", nil, "Enable a rule with the given name.\nMay be specified multiple times.") fs.StringArrayVar(&ruleDisableFlag, "disable-rule", nil, "Disable a rule with the given name.\nMay be specified multiple times.") - fs.BoolVar(&listRulesFlag, "list-rules", false, "Print the rules and exit. Honors the output-format flag.") + fs.BoolVar(&listRulesFlag, "list-rules", false, "Print the rules and exit. Honors the output-format flag.") fs.BoolVar(&debugFlag, "debug", false, "Run in debug mode. Panics will print stack.") fs.BoolVar(&ignoreCommentDisablesFlag, "ignore-comment-disables", false, "If set to true, disable comments will be ignored.\nThis is helpful when strict enforcement of AIPs are necessary and\nproto definitions should not be able to disable checks.") @@ -150,9 +151,12 @@ func (c *cli) lint(rules lint.RuleRegistry, configs lint.Configs) error { } var errorsWithPos []protoparse.ErrorWithPos var lock sync.Mutex + + imports := resolveImports(c.ProtoImportPaths) + // Parse proto files into `protoreflect` file descriptors. p := protoparse.Parser{ - ImportPaths: append(c.ProtoImportPaths, "."), + ImportPaths: imports, IncludeSourceCodeInfo: true, LookupImport: lookupImport, ErrorReporter: func(errorWithPos protoparse.ErrorWithPos) error { @@ -165,21 +169,7 @@ func (c *cli) lint(rules lint.RuleRegistry, configs lint.Configs) error { }, } // Resolve file absolute paths to relative ones. - // Using supplied import paths first. - protoFiles := c.ProtoFiles - if len(c.ProtoImportPaths) > 0 { - protoFiles, err = protoparse.ResolveFilenames(c.ProtoImportPaths, c.ProtoFiles...) - if err != nil { - return err - } - } - // Then resolve again against ".", the local directory. - // This is necessary because ResolveFilenames won't resolve a path if it - // relative to *at least one* of the given import paths, which can result - // in duplicate file parsing and compilation errors, as seen in #1465 and - // #1471. So we resolve against local (default) and flag specified import - // paths separately. - protoFiles, err = protoparse.ResolveFilenames([]string{"."}, protoFiles...) + protoFiles, err := protoparse.ResolveFilenames(p.ImportPaths, c.ProtoFiles...) if err != nil { return err } @@ -305,3 +295,75 @@ func getOutputFormatFunc(formatType string) formatFunc { } return yaml.Marshal } + +func resolveImports(imports []string) []string { + // If no import paths are provided, default to the current directory. + if len(imports) == 0 { + return []string{"."} + } + + // Get the absolute path of the current working directory. + cwd, err := os.Getwd() + if err != nil { + // Fallback: If we can't get CWD, return only the provided paths and "." + seen := map[string]bool{ + ".": true, + } + result := []string{"."} // Always include "." + for _, p := range imports { + if !seen[p] { + seen[p] = true + result = append(result, p) + } + } + return result + } + + // Resolve the canonical path for the current working directory. + // This helps with symlinks (e.g., /var vs /private/var on macOS). + evaluatedCwd, err := filepath.EvalSymlinks(cwd) + if err != nil { + // Fallback to Clean if EvalSymlinks fails (e.g., path does not exist) + evaluatedCwd = filepath.Clean(cwd) + } + + // Initialize resolvedImports with "." and track its canonical absolute path. + resolvedImports := []string{"."} + seenAbsolutePaths := map[string]bool{ + evaluatedCwd: true, // Mark canonical CWD as seen + } + + for _, p := range imports { + absPath, err := filepath.Abs(p) + if err != nil { + // If we can't get the absolute path, treat it as an external path + // and add it if not already seen (by its original string form). + if !seenAbsolutePaths[p] { + seenAbsolutePaths[p] = true + resolvedImports = append(resolvedImports, p) + } + continue + } + + // Resolve the canonical path for the current import path. + evaluatedAbsPath, err := filepath.EvalSymlinks(absPath) + if err != nil { + // Fallback to Clean if EvalSymlinks fails + 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)) { + continue + } + + // Add the original path if its canonical absolute form has not been seen before. + if !seenAbsolutePaths[evaluatedAbsPath] { + seenAbsolutePaths[evaluatedAbsPath] = true + resolvedImports = append(resolvedImports, p) + } + } + + return resolvedImports +} diff --git a/cmd/api-linter/cli_test.go b/cmd/api-linter/cli_test.go index 4e25a7811..246027c15 100644 --- a/cmd/api-linter/cli_test.go +++ b/cmd/api-linter/cli_test.go @@ -1,6 +1,9 @@ package main import ( + "os" + "path/filepath" + "sort" "testing" "github.com/google/go-cmp/cmp" @@ -54,3 +57,145 @@ func TestNewCli(t *testing.T) { }) } } + +func TestResolveImports(t *testing.T) { + // Save the original working directory and restore it at the end of the test. + originalCWD, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get original working directory: %v", err) + } + defer func() { + if err := os.Chdir(originalCWD); err != nil { + t.Errorf("Failed to restore original working directory: %v", err) + } + }() + + defaultSetup := func(t *testing.T, cwd, externalDir string) {} + + tests := []struct { + name string + protoImportPaths func(cwd, externalDir string) []string + setup func(t *testing.T, cwd, externalDir string) + want func(externalDir string) []string + }{ + { + name: "NoProtoImportPaths", + protoImportPaths: func(_, _ string) []string { + return []string{} + }, + setup: defaultSetup, + want: func(_ string) []string { + return []string{"."} + }, + }, + { + name: "ExplicitDot", + protoImportPaths: func(_, _ string) []string { + return []string{"."} + }, + setup: defaultSetup, + want: func(_ string) []string { + return []string{"."} + }, + }, + { + name: "SubdirectoryOfCWD", + protoImportPaths: func(_, _ string) []string { + return []string{"test_dir"} + }, + setup: func(t *testing.T, cwd, _ string) { + if err := os.Mkdir(filepath.Join(cwd, "test_dir"), 0755); err != nil { + t.Fatalf("Failed to create temp subdirectory: %v", err) + } + }, + want: func(_ string) []string { + return []string{"."} // "test_dir" should be covered by "." + }, + }, + { + name: "SubdirectoryOfCWDWithDot", + protoImportPaths: func(_, _ string) []string { + return []string{".", "test_dir"} + }, + setup: func(t *testing.T, cwd, _ string) { + if err := os.Mkdir(filepath.Join(cwd, "test_dir"), 0755); err != nil { + t.Fatalf("Failed to create temp subdirectory: %v", err) + } + }, + want: func(_ string) []string { + return []string{"."} + }, + }, + { + name: "ExternalAbsolutePath", + protoImportPaths: func(_, externalDir string) []string { + return []string{externalDir} + }, + setup: defaultSetup, + want: func(externalDir string) []string { + return []string{".", externalDir} + }, + }, + { + name: "MixedPaths", + protoImportPaths: func(_, externalDir string) []string { + return []string{"./relative_dir", externalDir, "test_dir"} + }, + setup: func(t *testing.T, cwd, _ string) { + if err := os.Mkdir(filepath.Join(cwd, "relative_dir"), 0755); err != nil { + t.Fatalf("Failed to create relative_dir: %v", err) + } + if err := os.Mkdir(filepath.Join(cwd, "test_dir"), 0755); err != nil { + t.Fatalf("Failed to create test_dir: %v", err) + } + }, + want: func(externalDir string) []string { + return []string{".", externalDir} + }, + }, + { + name: "NonExistentRelativePath", + protoImportPaths: func(_, _ string) []string { + return []string{"non_existent_dir"} + }, + setup: defaultSetup, + want: func(_ string) []string { + return []string{"."} // Should still resolve to just "." as non_existent_dir is relative to CWD + }, + }, + { + name: "CurrentDirAsAbsolutePath", + protoImportPaths: func(cwd, _ string) []string { + return []string{cwd} + }, + setup: defaultSetup, + want: func(_ string) []string { + return []string{"."} + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + cwd := t.TempDir() + externalDir := t.TempDir() + + if err := os.Chdir(cwd); err != nil { + t.Fatalf("Failed to change directory to %q: %v", cwd, err) + } + + test.setup(t, cwd, externalDir) + + protoImportPaths := test.protoImportPaths(cwd, externalDir) + want := test.want(externalDir) + + got := resolveImports(protoImportPaths) + sort.Strings(got) + sort.Strings(want) + + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("resolveImports() mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/cmd/api-linter/integration_test.go b/cmd/api-linter/integration_test.go index 68eac77d0..3d92881e3 100644 --- a/cmd/api-linter/integration_test.go +++ b/cmd/api-linter/integration_test.go @@ -182,9 +182,6 @@ func TestMultipleFilesFromParentDir(t *testing.T) { // This test addresses a previously found bug: // https://github.com/googleapis/api-linter/issues/1465 - // Skipping until Issue # 1465 is addressed - t.Skip("Skipping until Issue # 1465 is addressed") - projDir, err := os.MkdirTemp("", "proj") if err != nil { t.Fatal(err)