diff --git a/.golangci.yaml b/.golangci.yaml index de7867c0..196d7470 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -5,6 +5,7 @@ linters: - rowserrcheck - wastedassign # annoying + - gocyclo - tagliatelle - nestif - gocognit diff --git a/README.md b/README.md index 4d34d99c..822a3d81 100644 --- a/README.md +++ b/README.md @@ -671,6 +671,7 @@ The Regal language server currently supports the following LSP features: - [x] [use-rego-v1](https://docs.styra.com/regal/rules/imports/use-rego-v1) - [x] [use-assignment-operator](https://docs.styra.com/regal/rules/style/use-assignment-operator) - [x] [no-whitespace-comment](https://docs.styra.com/regal/rules/style/no-whitespace-comment) + - [x] [directory-package-mismatch](https://docs.styra.com/regal/rules/idiomatic/directory-package-mismatch) - [x] [Code lenses](https://docs.styra.com/regal/language-server#code-lenses-evaluation) (click to evaluate any package or rule directly in the editor) diff --git a/bundle/regal/rules/idiomatic/directory-package-mismatch/directory_package_mismatch.rego b/bundle/regal/rules/idiomatic/directory-package-mismatch/directory_package_mismatch.rego index 617f50b5..8a2dd105 100644 --- a/bundle/regal/rules/idiomatic/directory-package-mismatch/directory_package_mismatch.rego +++ b/bundle/regal/rules/idiomatic/directory-package-mismatch/directory_package_mismatch.rego @@ -7,27 +7,19 @@ import rego.v1 import data.regal.config import data.regal.result -# METADATA +# - METADATA # description: | # emit warning notice when package has more parts than the directory, -# as this should likely **not** fail -notices contains _notice(message, "warning") if { - count(_file_path_values) > 0 - count(_file_path_values) < count(_pkg_path_values) - - message := sprintf( - "package '%s' has more parts than provided directory path '%s'", - [concat(".", _pkg_path_values), concat("/", _file_path_values)], - ) -} - -# METADATA -# description: emit notice when single file is provided, but with no severity -notices contains _notice(message, "none") if { - count(_file_path_values) == 0 +# # as this should likely **not** fail +# notices contains _notice(message, "warning") if { +# count(_file_path_values) > 0 +# count(_file_path_values) < count(_pkg_path_values) - message := "provided file has no directory components in its path... try linting a directory" -} +# message := sprintf( +# "package '%s' has more parts than provided directory path '%s'", +# [concat(".", _pkg_path_values), concat("/", _file_path_values)], +# ) +# } report contains violation if { # get the last n components from file path, where n == count(_pkg_path_values) @@ -49,10 +41,10 @@ _pkg_path := [p.value | i > 0 ] -_pkg_path_values := without_test_suffix if { - cfg := config.for_rule("idiomatic", "directory-package-mismatch") +_pkg_path_values := _pkg_path if not config.for_rule("idiomatic", "directory-package-mismatch")["exclude-test-suffix"] - cfg["exclude-test-suffix"] +_pkg_path_values := without_test_suffix if { + config.for_rule("idiomatic", "directory-package-mismatch")["exclude-test-suffix"] without_test_suffix := array.concat( array.slice(_pkg_path, 0, count(_pkg_path) - 1), diff --git a/bundle/regal/rules/idiomatic/directory-package-mismatch/directory_package_mismatch_test.rego b/bundle/regal/rules/idiomatic/directory-package-mismatch/directory_package_mismatch_test.rego index e6fb94c7..f1c7f318 100644 --- a/bundle/regal/rules/idiomatic/directory-package-mismatch/directory_package_mismatch_test.rego +++ b/bundle/regal/rules/idiomatic/directory-package-mismatch/directory_package_mismatch_test.rego @@ -36,32 +36,6 @@ test_success_directory_structure_package_path_match_shorter_directory_path if { r == set() } -test_notice_severity_warning_when_directory_path_shorter_than_package_path if { - module := regal.parse_module("bar/baz/p.rego", "package foo.bar.baz") - r := rule.notices with input as module with config.for_rule as default_config - - r == {{ - "category": "idiomatic", - "description": "package 'foo.bar.baz' has more parts than provided directory path 'bar/baz'", - "level": "notice", - "severity": "warning", - "title": "directory-package-mismatch", - }} -} - -test_notice_severity_none_when_no_path_likely_single_file_provided if { - module := regal.parse_module("p.rego", "package p") - r := rule.notices with input as module with config.for_rule as default_config - - r == {{ - "category": "idiomatic", - "description": "provided file has no directory components in its path... try linting a directory", - "level": "notice", - "severity": "none", - "title": "directory-package-mismatch", - }} -} - with_location(location) := {{ "category": "idiomatic", "description": "Directory structure should mirror package", diff --git a/cmd/fix.go b/cmd/fix.go index 1de770f3..8773d64b 100644 --- a/cmd/fix.go +++ b/cmd/fix.go @@ -18,6 +18,7 @@ import ( "github.com/open-policy-agent/opa/format" rio "github.com/styrainc/regal/internal/io" + "github.com/styrainc/regal/internal/util" "github.com/styrainc/regal/pkg/config" "github.com/styrainc/regal/pkg/fixer" "github.com/styrainc/regal/pkg/fixer/fileprovider" @@ -240,7 +241,13 @@ func fix(args []string, params *fixCommandParams) error { log.Println("no user-provided config file found, will use the default config") } + roots, err := getPotentialRoots(args) + if err != nil { + return fmt.Errorf("could not find potential roots: %w", err) + } + f := fixer.NewFixer() + f.RegisterRoots(roots...) f.RegisterFixes(fixes.NewDefaultFixes()...) f.RegisterMandatoryFixes( &fixes.Fmt{ @@ -276,3 +283,42 @@ func fix(args []string, params *fixCommandParams) error { return nil } + +func getPotentialRoots(paths []string) ([]string, error) { + dirMap := make(map[string]struct{}) + + for _, path := range paths { + abs, err := filepath.Abs(path) + if err != nil { + return nil, fmt.Errorf("failed to get absolute path for %s: %w", path, err) + } + + if isDir(abs) { + dirMap[abs] = struct{}{} + } else { + dirMap[filepath.Dir(abs)] = struct{}{} + } + } + + for _, dir := range util.Keys(dirMap) { + brds, err := config.FindBundleRootDirectories(dir) + if err != nil { + return nil, fmt.Errorf("failed to find bundle root directories in %s: %w", dir, err) + } + + for _, brd := range brds { + dirMap[brd] = struct{}{} + } + } + + return util.Keys(dirMap), nil +} + +func isDir(path string) bool { + info, err := os.Stat(path) + if err != nil { + return false + } + + return info.IsDir() +} diff --git a/e2e/cli_test.go b/e2e/cli_test.go index 5847e068..5b3be1a9 100644 --- a/e2e/cli_test.go +++ b/e2e/cli_test.go @@ -778,6 +778,7 @@ func TestFix(t *testing.T) { stderr := bytes.Buffer{} td := t.TempDir() + // note the package name, and how it will be used to move the file to a corresponding directory unformattedContents := []byte(`package wow import rego.v1 @@ -788,26 +789,22 @@ allow if { true } `) - err := os.WriteFile(filepath.Join(td, "main.rego"), unformattedContents, 0o644) - if err != nil { - t.Fatalf("failed to write main.rego: %v", err) - } - unrelatedFileContents := []byte(`foobar`) - err = os.WriteFile(filepath.Join(td, "unrelated.txt"), unrelatedFileContents, 0o644) - if err != nil { - t.Fatalf("failed to write unrelated.txt: %v", err) - } - err = regal(&stdout, &stderr)("fix", td) + mustWriteToFile(t, filepath.Join(td, "main.rego"), string(unformattedContents)) + mustWriteToFile(t, filepath.Join(td, "unrelated.txt"), string(unrelatedFileContents)) + + err := regal(&stdout, &stderr)("fix", td) // 0 exit status is expected as all violations should have been fixed expectExitCode(t, err, 0, &stdout, &stderr) - exp := fmt.Sprintf(`2 fixes applied: -%s/main.rego: -- no-whitespace-comment + exp := fmt.Sprintf(`3 fixes applied: +%[1]s/main.rego: - use-rego-v1 +%[1]s/wow/main.rego: +- directory-package-mismatch +- no-whitespace-comment `, td) if act := stdout.String(); exp != act { @@ -818,10 +815,10 @@ allow if { t.Errorf("expected stderr %q, got %q", exp, act) } - // check that the file was formatted - bs, err := os.ReadFile(filepath.Join(td, "main.rego")) + // check that the (now moved) file was formatted + bs, err := os.ReadFile(filepath.Join(td, "wow/main.rego")) if err != nil { - t.Fatalf("failed to read main.rego: %v", err) + t.Fatalf("failed to read wow/main.rego: %v", err) } expectedContent := `package wow @@ -905,3 +902,12 @@ func expectExitCode(t *testing.T, err error, exp int, stdout *bytes.Buffer, stde exp, act, stdout.String(), stderr.String()) } } + +func mustWriteToFile(t *testing.T, path string, content string) { + t.Helper() + + err := os.WriteFile(path, []byte(content), 0o644) + if err != nil { + t.Fatalf("failed to write to %s: %v", path, err) + } +} diff --git a/internal/io/io.go b/internal/io/io.go index 471a4a47..6812410d 100644 --- a/internal/io/io.go +++ b/internal/io/io.go @@ -53,16 +53,6 @@ func MustLoadRegalBundleFS(fs files.FS) bundle.Bundle { return regalBundle } -// MustLoadRegalBundlePath loads bundle from path, exit on failure. -func MustLoadRegalBundlePath(path string) bundle.Bundle { - regalBundle, err := LoadRegalBundlePath(path) - if err != nil { - log.Fatal(err) - } - - return regalBundle -} - // ToMap convert any value to map[string]any, or panics on failure. func ToMap(a any) map[string]any { r := make(map[string]any) @@ -122,3 +112,43 @@ func FindInput(file string, workspacePath string) (string, io.Reader) { return "", nil } + +func IsSkipWalkDirectory(info files.DirEntry) bool { + return info.IsDir() && (info.Name() == ".git" || info.Name() == ".idea" || info.Name() == "node_modules") +} + +// WalkFiles walks the file system rooted at root, calling f for each file. This is +// a less ceremonious version of filepath.WalkDir where only file paths (not dirs) +// are passed to the callback, and where directories that should commonly be ignored +// (.git, node_modules, etc.) are skipped. +func WalkFiles(root string, f func(path string) error) error { + return filepath.WalkDir(root, func(path string, info os.DirEntry, _ error) error { // nolint:wrapcheck + if IsSkipWalkDirectory(info) { + return filepath.SkipDir + } + + if info.IsDir() { + return nil + } + + return f(path) + }) +} + +func FindManifestLocations(root string) ([]string, error) { + var foundBundleRoots []string + + if err := WalkFiles(root, func(path string) error { + if filepath.Base(path) == ".manifest" { + if rel, err := filepath.Rel(root, path); err == nil { + foundBundleRoots = append(foundBundleRoots, filepath.Dir(rel)) + } + } + + return nil + }); err != nil { + return nil, fmt.Errorf("failed to walk workspace path: %w", err) + } + + return foundBundleRoots, nil +} diff --git a/internal/io/io_test.go b/internal/io/io_test.go index eea0c158..f8d577e1 100644 --- a/internal/io/io_test.go +++ b/internal/io/io_test.go @@ -1,7 +1,10 @@ package io import ( + "slices" "testing" + + "github.com/open-policy-agent/opa/util/test" ) func TestJSONRoundTrip(t *testing.T) { @@ -22,3 +25,32 @@ func TestJSONRoundTrip(t *testing.T) { t.Errorf("expected JSON roundtrip to set struct value") } } + +func TestFindManifestLocations(t *testing.T) { + t.Parallel() + + fs := map[string]string{ + "/.git": "", + "/foo/bar/baz/.manifest": "", + "/foo/bar/qux/.manifest": "", + "/foo/bar/.regal/.manifest.yaml": "", + "/node_modules/.manifest": "", + } + + test.WithTempFS(fs, func(root string) { + locations, err := FindManifestLocations(root) + if err != nil { + t.Error(err) + } + + if len(locations) != 2 { + t.Errorf("expected 2 locations, got %d", len(locations)) + } + + expected := []string{"foo/bar/baz", "foo/bar/qux"} + + if !slices.Equal(locations, expected) { + t.Errorf("expected %v, got %v", expected, locations) + } + }) +} diff --git a/internal/lsp/bundles/cache.go b/internal/lsp/bundles/cache.go index 20d6d15b..4afe1238 100644 --- a/internal/lsp/bundles/cache.go +++ b/internal/lsp/bundles/cache.go @@ -13,6 +13,7 @@ import ( "github.com/open-policy-agent/opa/bundle" + rio "github.com/styrainc/regal/internal/io" "github.com/styrainc/regal/internal/util" ) @@ -56,26 +57,7 @@ func (c *Cache) Refresh() ([]string, error) { } // find all the bundle roots that are currently present on disk - var foundBundleRoots []string - - err := filepath.Walk(c.workspacePath, func(path string, info os.FileInfo, _ error) error { - if info.IsDir() && (info.Name() == ".git" || info.Name() == ".idea" || info.Name() == "node_modules") { - return filepath.SkipDir - } - - if info.IsDir() { - return nil - } - - if filepath.Base(path) == ".manifest" { - foundBundleRoots = append( - foundBundleRoots, - strings.TrimPrefix(filepath.Dir(path), c.workspacePath), - ) - } - - return nil - }) + foundBundleRoots, err := rio.FindManifestLocations(c.workspacePath) if err != nil { return nil, fmt.Errorf("failed to walk workspace path: %w", err) } diff --git a/internal/lsp/command.go b/internal/lsp/command.go index 70e5eef0..be831244 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go @@ -46,3 +46,12 @@ func NoWhiteSpaceCommentCommand(args []string) types.Command { Arguments: toAnySlice(args), } } + +func DirectoryStructureMismatchCommand(args []string) types.Command { + return types.Command{ + Title: "Fix directory structure / package path mismatch", + Command: "regal.fix.directory-package-mismatch", + Tooltip: "Fix directory structure / package path mismatch", + Arguments: toAnySlice(args), + } +} diff --git a/internal/lsp/server.go b/internal/lsp/server.go index 20993f37..fe90f11c 100644 --- a/internal/lsp/server.go +++ b/internal/lsp/server.go @@ -52,8 +52,11 @@ const ( methodTextDocumentPublishDiagnostics = "textDocument/publishDiagnostics" methodWorkspaceApplyEdit = "workspace/applyEdit" - ruleNameOPAFmt = "opa-fmt" - ruleNameUseRegoV1 = "use-rego-v1" + ruleNameOPAFmt = "opa-fmt" + ruleNameUseRegoV1 = "use-rego-v1" + ruleNameUseAssignmentOperator = "use-assignment-operator" + ruleNameNoWhitespaceComment = "no-whitespace-comment" + ruleNameDirectoryPackageMismatch = "directory-package-mismatch" ) type LanguageServerOptions struct { @@ -113,7 +116,6 @@ type fileUpdateEvent struct { Content string } -//nolint:gocyclo func (l *LanguageServer) Handle( ctx context.Context, conn *jsonrpc2.Conn, @@ -429,7 +431,7 @@ func (l *LanguageServer) StartConfigWorker(ctx context.Context) { } } -func (l *LanguageServer) StartCommandWorker(ctx context.Context) { +func (l *LanguageServer) StartCommandWorker(ctx context.Context) { // nolint:maintidx // note, in this function conn.Call is used as the workspace/applyEdit message is a request, not a notification // as per the spec. In order to be 'routed' to the correct handler on the client it must have an ID // receive responses too. @@ -474,6 +476,34 @@ func (l *LanguageServer) StartCommandWorker(ctx context.Context) { commands.ParseOptions{TargetArgIndex: 0, RowArgIndex: 1, ColArgIndex: 2}, params, ) + case "regal.fix.directory-package-mismatch": + var renameParams types.ApplyWorkspaceRenameEditParams + + fileURL, ok := params.Arguments[0].(string) + if !ok { + l.logError(fmt.Errorf("expected first argument to be a string, got %T", params.Arguments[0])) + + break + } + + renameParams, err = l.fixRenameParams( + "Rename file to match package path", + &fixes.DirectoryPackageMismatch{DryRun: true}, + fileURL, + ) + if err != nil { + l.logError(fmt.Errorf("failed to fix directory package mismatch: %w", err)) + + break + } + + // handle this ourselves as it's a rename and not a content edit + fixed = false + + err = l.conn.Call(ctx, methodWorkspaceApplyEdit, renameParams, nil) + if err != nil { + l.logError(fmt.Errorf("failed %s notify: %v", methodWorkspaceApplyEdit, err.Error())) + } case "regal.eval": if len(params.Arguments) != 3 { l.logError(fmt.Errorf("expected three arguments, got %d", len(params.Arguments))) @@ -526,8 +556,7 @@ func (l *LanguageServer) StartCommandWorker(ctx context.Context) { // if there are none, then it's a package evaluation ruleHeadLocations := allRuleHeadLocations[path] - workspacePath := uri.ToPath(l.clientIdentifier, l.workspaceRootURI) - _, input := rio.FindInput(uri.ToPath(l.clientIdentifier, file), workspacePath) + _, input := rio.FindInput(uri.ToPath(l.clientIdentifier, file), l.workspacePath()) result, err := l.EvalWorkspacePath(ctx, path, input) if err != nil { @@ -571,7 +600,7 @@ func (l *LanguageServer) StartCommandWorker(ctx context.Context) { l.logError(fmt.Errorf("regal/showEvalResult failed: %v", err.Error())) } } else { - output := filepath.Join(workspacePath, "output.json") + output := filepath.Join(l.workspacePath(), "output.json") var f *os.File @@ -743,6 +772,52 @@ func (l *LanguageServer) fixEditParams( return true, editParams, nil } +func (l *LanguageServer) fixRenameParams( + label string, + fix fixes.Fix, + fileURL string, +) (types.ApplyWorkspaceRenameEditParams, error) { + contents, ok := l.cache.GetFileContents(fileURL) + if !ok { + return types.ApplyWorkspaceRenameEditParams{}, fmt.Errorf("failed to get module for file %q", fileURL) + } + + results, err := fix.Fix( + &fixes.FixCandidate{ + Filename: uri.ToPath(l.clientIdentifier, fileURL), + Contents: []byte(contents), + }, + &fixes.RuntimeOptions{ + Config: l.loadedConfig, + BaseDir: l.workspacePath(), + }, + ) + if err != nil { + return types.ApplyWorkspaceRenameEditParams{}, fmt.Errorf("failed attempted fix: %w", err) + } + + result := results[0] + + renameEdit := types.WorkspaceRenameEdit{ + DocumentChanges: []types.RenameFile{ + { + Kind: "rename", + OldURI: uri.FromPath(l.clientIdentifier, result.Rename.FromPath), + NewURI: uri.FromPath(l.clientIdentifier, result.Rename.ToPath), + Options: &types.RenameFileOptions{ + Overwrite: false, + IgnoreIfExists: false, + }, + }, + }, + } + + return types.ApplyWorkspaceRenameEditParams{ + Label: label, + Edit: renameEdit, + }, nil +} + // processTextContentUpdate updates the cache with the new content for the file at the given URI, attempts to parse the // file, and returns whether the parse was successful. If it was not successful, the parse errors will be sent // on the diagnostic channel. @@ -983,7 +1058,7 @@ func (l *LanguageServer) handleTextDocumentCodeAction( IsPreferred: &yes, Command: FmtV1Command([]string{params.TextDocument.URI}), }) - case "use-assignment-operator": + case ruleNameUseAssignmentOperator: actions = append(actions, types.CodeAction{ Title: "Replace = with := in assignment", Kind: "quickfix", @@ -995,7 +1070,7 @@ func (l *LanguageServer) handleTextDocumentCodeAction( strconv.FormatUint(uint64(diag.Range.Start.Character+1), 10), }), }) - case "no-whitespace-comment": + case ruleNameNoWhitespaceComment: actions = append(actions, types.CodeAction{ Title: "Format comment to have leading whitespace", Kind: "quickfix", @@ -1007,6 +1082,16 @@ func (l *LanguageServer) handleTextDocumentCodeAction( strconv.FormatUint(uint64(diag.Range.Start.Character+1), 10), }), }) + case ruleNameDirectoryPackageMismatch: + actions = append(actions, types.CodeAction{ + Title: "Move file so that directory structure mirrors package path", + Kind: "quickfix", + Diagnostics: []types.Diagnostic{diag}, + IsPreferred: &yes, + Command: DirectoryStructureMismatchCommand([]string{ + params.TextDocument.URI, + }), + }) } if l.clientIdentifier == clients.IdentifierVSCode { @@ -1832,6 +1917,7 @@ func (l *LanguageServer) handleInitialize( "regal.fix.use-rego-v1", "regal.fix.use-assignment-operator", "regal.fix.no-whitespace-comment", + "regal.fix.directory-package-mismatch", }, }, DocumentFormattingProvider: true, @@ -1850,7 +1936,7 @@ func (l *LanguageServer) handleInitialize( } if l.workspaceRootURI != "" { - workspaceRootPath := uri.ToPath(l.clientIdentifier, l.workspaceRootURI) + workspaceRootPath := l.workspacePath() l.bundleCache = bundles.NewCache(&bundles.CacheOptions{ WorkspacePath: workspaceRootPath, @@ -1874,23 +1960,13 @@ func (l *LanguageServer) handleInitialize( } func (l *LanguageServer) loadWorkspaceContents(ctx context.Context, newOnly bool) ([]string, error) { - workspaceRootPath := uri.ToPath(l.clientIdentifier, l.workspaceRootURI) + workspaceRootPath := l.workspacePath() changedOrNewURIs := make([]string, 0) - err := filepath.WalkDir(workspaceRootPath, func(path string, d os.DirEntry, err error) error { - if err != nil { - return fmt.Errorf("failed to walk workspace dir %q: %w", path, err) - } - - // These directories often have thousands of items we don't care about, - // so don't even traverse them. - if d.IsDir() && (d.Name() == ".git" || d.Name() == ".idea" || d.Name() == "node_modules") { - return filepath.SkipDir - } - + if err := rio.WalkFiles(workspaceRootPath, func(path string) error { // TODO(charlieegan3): make this configurable for things like .rq etc? - if d.IsDir() || !strings.HasSuffix(path, ".rego") { + if !strings.HasSuffix(path, ".rego") { return nil } @@ -1924,8 +2000,7 @@ func (l *LanguageServer) loadWorkspaceContents(ctx context.Context, newOnly bool changedOrNewURIs = append(changedOrNewURIs, fileURI) return nil - }) - if err != nil { + }); err != nil { return nil, fmt.Errorf("failed to walk workspace dir %q: %w", workspaceRootPath, err) } @@ -2043,7 +2118,7 @@ func (l *LanguageServer) ignoreURI(fileURI string) bool { []string{uri.ToPath(l.clientIdentifier, fileURI)}, l.loadedConfig.Ignore.Files, false, - uri.ToPath(l.clientIdentifier, l.workspaceRootURI), + l.workspacePath(), ) if err != nil || len(paths) == 0 { return true @@ -2052,6 +2127,10 @@ func (l *LanguageServer) ignoreURI(fileURI string) bool { return false } +func (l *LanguageServer) workspacePath() string { + return uri.ToPath(l.clientIdentifier, l.workspaceRootURI) +} + func positionToOffset(text string, p types.Position) int { bytesRead := 0 lines := strings.Split(text, "\n") diff --git a/internal/lsp/server_test.go b/internal/lsp/server_test.go index ebd5eb4f..fa49772f 100644 --- a/internal/lsp/server_test.go +++ b/internal/lsp/server_test.go @@ -17,8 +17,12 @@ import ( "github.com/anderseknert/roast/pkg/encoding" "github.com/sourcegraph/jsonrpc2" + "github.com/styrainc/regal/internal/lsp/cache" + "github.com/styrainc/regal/internal/lsp/clients" "github.com/styrainc/regal/internal/lsp/rego" "github.com/styrainc/regal/internal/lsp/types" + "github.com/styrainc/regal/pkg/config" + "github.com/styrainc/regal/pkg/fixer/fixes" ) const mainRegoFileName = "/main.rego" @@ -59,7 +63,7 @@ const fileURIScheme = "file://" // This test also ensures that updating the config to point to a non-default engine and capabilities version works // and causes that engine's builtins to work with completions. // -//nolint:gocyclo,maintidx +//nolint:maintidx func TestLanguageServerSingleFile(t *testing.T) { t.Parallel() @@ -460,7 +464,7 @@ allow := neo4j.q // workspace diagnostics are run, this test validates that the correct diagnostics are sent to the client in this // scenario. // -// nolint:maintidx,gocyclo +// nolint:maintidx func TestLanguageServerMultipleFiles(t *testing.T) { t.Parallel() @@ -874,6 +878,63 @@ allow := true } } +func TestLanguageServerFixRenameParams(t *testing.T) { + t.Parallel() + + l := NewLanguageServer(&LanguageServerOptions{ErrorLog: newTestLogger(t)}) + c := cache.NewCache() + f := &fixes.DirectoryPackageMismatch{DryRun: true} + + fileURL := "file:///workspace/foo/bar/policy.rego" + + c.SetFileContents(fileURL, "package authz.main.rules") + + l.clientIdentifier = clients.IdentifierVSCode + l.workspaceRootURI = "file:///workspace" + l.cache = c + l.loadedConfig = &config.Config{ + Rules: map[string]config.Category{ + "idiomatic": { + "directory-package-mismatch": config.Rule{ + Level: "ignore", + Extra: map[string]any{ + "exclude-test-suffix": true, + }, + }, + }, + }, + } + + params, err := l.fixRenameParams("fix my file!", f, fileURL) + if err != nil { + t.Fatalf("failed to fix rename params: %s", err) + } + + if params.Label != "fix my file!" { + t.Fatalf("expected label to be 'Fix my file!', got %s", params.Label) + } + + if len(params.Edit.DocumentChanges) != 1 { + t.Fatalf("expected 1 document change, got %d", len(params.Edit.DocumentChanges)) + } + + change := params.Edit.DocumentChanges[0] + + if change.Kind != "rename" { + t.Fatalf("expected kind to be 'rename', got %s", change.Kind) + } + + if change.OldURI != fileURL { + t.Fatalf("expected old URI to be %s, got %s", fileURL, change.OldURI) + } + + expectedNewURI := "file:///workspace/authz/main/rules/policy.rego" + + if change.NewURI != expectedNewURI { + t.Fatalf("expected new URI to be %s, got %s", expectedNewURI, change.NewURI) + } +} + func testRequestDataCodes(t *testing.T, requestData types.FileDiagnostics, fileURI string, codes []string) bool { t.Helper() diff --git a/internal/lsp/types/types.go b/internal/lsp/types/types.go index 5d2283ef..1087a032 100644 --- a/internal/lsp/types/types.go +++ b/internal/lsp/types/types.go @@ -83,8 +83,8 @@ type ShowMessageParams struct { } type StaleRequestSupportClientCapabilities struct { - Cancel bool `json:"cancel"` - RetryOnContentModifieds []string `json:"retryOnContentModified"` + Cancel bool `json:"cancel"` + RetryOnContentModified []string `json:"retryOnContentModified"` } type InitializeResult struct { @@ -225,6 +225,30 @@ type ApplyWorkspaceEditParams struct { Edit WorkspaceEdit `json:"edit"` } +type ApplyWorkspaceRenameEditParams struct { + Label string `json:"label"` + Edit WorkspaceRenameEdit `json:"edit"` +} + +type RenameFileOptions struct { + Overwrite bool `json:"overwrite"` + IgnoreIfExists bool `json:"ignoreIfExists"` +} + +type RenameFile struct { + Kind string `json:"kind"` // must always be "rename" + OldURI string `json:"oldUri"` + NewURI string `json:"newUri"` + Options *RenameFileOptions `json:"options,omitempty"` + AnnotationIdentifier *string `json:"annotationId,omitempty"` +} + +// WorkspaceRenameEdit is a WorkspaceEdit that is used for renaming files. +// Perhaps we should use generics and a union type here instead. +type WorkspaceRenameEdit struct { + DocumentChanges []RenameFile `json:"documentChanges"` +} + type WorkspaceEdit struct { DocumentChanges []TextDocumentEdit `json:"documentChanges"` } diff --git a/internal/util/util.go b/internal/util/util.go index c02a649b..b756cdfc 100644 --- a/internal/util/util.go +++ b/internal/util/util.go @@ -2,6 +2,7 @@ package util import ( "fmt" + "path/filepath" "strings" ) @@ -71,3 +72,49 @@ func Must[T any](v T, err error) T { return v } + +func Map[T, U any](f func(T) U, a []T) []U { + b := make([]U, len(a)) + + for i, v := range a { + b[i] = f(v) + } + + return b +} + +// FindClosestMatchingRoot finds the closest matching root for a given path. +// If no matching root is found, an empty string is returned. +func FindClosestMatchingRoot(path string, roots []string) string { + currentLongestSuffix := 0 + longestSuffixIndex := -1 + + for i, root := range roots { + if root == path { + return path + } + + if !strings.HasPrefix(path, root) { + continue + } + + suffix := strings.TrimPrefix(root, path) + + if len(suffix) > currentLongestSuffix { + currentLongestSuffix = len(suffix) + longestSuffixIndex = i + } + } + + if longestSuffixIndex == -1 { + return "" + } + + return roots[longestSuffixIndex] +} + +func FilepathJoiner(base string) func(string) string { + return func(path string) string { + return filepath.Join(base, path) + } +} diff --git a/internal/util/util_test.go b/internal/util/util_test.go new file mode 100644 index 00000000..66ebbea3 --- /dev/null +++ b/internal/util/util_test.go @@ -0,0 +1,50 @@ +package util + +import "testing" + +func TestFindClosestMatchingRoot(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + roots []string + path string + expected string + }{ + { + name: "different length roots", + roots: []string{"/a/b/c", "/a/b", "/a"}, + path: "/a/b/c/d/e/f", + expected: "/a/b/c", + }, + { + name: "exact match", + roots: []string{"/a/b/c", "/a/b", "/a"}, + path: "/a/b", + expected: "/a/b", + }, + { + name: "mixed roots", + roots: []string{"/a/b/c/b/a", "/c/b", "/a/d/c/f"}, + path: "/c/b/a", + expected: "/c/b", + }, + { + name: "no matching root", + roots: []string{"/a/b/c"}, + path: "/d", + expected: "", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + got := FindClosestMatchingRoot(test.path, test.roots) + if got != test.expected { + t.Fatalf("expected %v, got %v", test.expected, got) + } + }) + } +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 1c54cc14..c7d46900 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "slices" "strings" "github.com/anderseknert/roast/pkg/encoding" @@ -15,6 +16,7 @@ import ( "github.com/styrainc/regal/internal/capabilities" rio "github.com/styrainc/regal/internal/io" + "github.com/styrainc/regal/internal/util" ) const ( @@ -36,6 +38,12 @@ type Config struct { Features *Features `json:"features,omitempty" yaml:"features,omitempty"` CapabilitiesURL string `json:"capabilities_url,omitempty" yaml:"capabilities_url,omitempty"` + + Project *Project `json:"project,omitempty" yaml:"project,omitempty"` +} + +type Project struct { + Roots []string `json:"roots" yaml:"roots"` } type Category map[string]Rule @@ -162,6 +170,96 @@ func FindRegalDirectory(path string) (*os.File, error) { } } +// FindBundleRootDirectories finds all bundle root directories from the provided path, +// which **must** be an absolute path. Bundle root directories may be found either by: +// +// - Configuration (`project.roots`) +// - By the presence of a .manifest file anywhere under the path +// - By the presence of a .regal directory anywhere under or above the path ... TODO (anders): might reconsider "above"? +// +// All returned paths are absolute paths. If the provided path itself +// is determined to be a bundle root directory it will be included in the result. +func FindBundleRootDirectories(path string) ([]string, error) { + var foundBundleRoots []string + + // This will traverse the tree **upwards** searching for a .regal directory + regalDir, err := FindRegalDirectory(path) + if err == nil { + roots, err := rootsFromRegalDirectory(regalDir) + if err != nil { + return nil, fmt.Errorf("failed to get roots from .regal directory: %w", err) + } + + foundBundleRoots = append(foundBundleRoots, roots...) + } + + // This will traverse the tree **downwards** searching for .regal directories + // Not using rio.WalkFiles here as we're specifically looking for directories + if err := filepath.WalkDir(path, func(path string, info os.DirEntry, _ error) error { + if info.IsDir() && info.Name() == regalDirName { + // Opening files as part of walking is generally not a good idea... + // but I think we can assume the number of .regal directories in a project + // is limited to a reasonable number. + rd, err := os.Open(path) + if err != nil { + return fmt.Errorf("failed to open .regal directory: %w", err) + } + + defer rd.Close() + + roots, err := rootsFromRegalDirectory(rd) + if err != nil { + return fmt.Errorf("failed to get roots from .regal directory: %w", err) + } + + foundBundleRoots = append(foundBundleRoots, roots...) + } + + // rather than calling rio.FindManifestLocations later, let's + // check for .manifest directories as part of the same walk + if !info.IsDir() && info.Name() == ".manifest" { + foundBundleRoots = append(foundBundleRoots, filepath.Dir(path)) + } + + return nil + }); err != nil { + return nil, fmt.Errorf("failed to walk path: %w", err) + } + + slices.Sort(foundBundleRoots) + + return slices.Compact(foundBundleRoots), nil +} + +func rootsFromRegalDirectory(regalDir *os.File) ([]string, error) { + foundBundleRoots := make([]string, 0) + + defer regalDir.Close() + + parent, _ := filepath.Split(regalDir.Name()) + + parent = filepath.Clean(parent) + + // add the parent directory of .regal + foundBundleRoots = append(foundBundleRoots, parent) + + file, err := os.ReadFile(filepath.Join(regalDir.Name(), "config.yaml")) + if err == nil { + var conf Config + + err = yaml.Unmarshal(file, &conf) + if err != nil { + return nil, fmt.Errorf("failed to unmarshal config file: %w", err) + } + + if conf.Project != nil { + foundBundleRoots = append(foundBundleRoots, util.Map(util.FilepathJoiner(parent), conf.Project.Roots)...) + } + } + + return foundBundleRoots, nil +} + func FindConfig(path string) (*os.File, error) { regalDir, err := FindRegalDirectory(path) if err != nil { @@ -250,6 +348,7 @@ type marshallingIntermediary struct { CheckVersion bool `yaml:"check_version"` } `yaml:"remote"` } `yaml:"features"` + Project *Project `yaml:"project"` } func (config *Config) UnmarshalYAML(value *yaml.Node) error { @@ -336,6 +435,8 @@ func (config *Config) UnmarshalYAML(value *yaml.Node) error { // of the information that the LSP needs. config.CapabilitiesURL = capabilitiesURL + config.Project = result.Project + // remove any builtins referenced in the minus config for _, minusBuiltin := range result.Capabilities.Minus.Builtins { delete(config.Capabilities.Builtins, minusBuiltin.Name) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 6b63d627..b2de8e94 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -3,6 +3,7 @@ package config import ( "os" "path/filepath" + "slices" "testing" "gopkg.in/yaml.v3" @@ -12,6 +13,7 @@ import ( rio "github.com/styrainc/regal/internal/io" "github.com/styrainc/regal/internal/testutil" + "github.com/styrainc/regal/internal/util" ) const levelError = "error" @@ -81,6 +83,41 @@ func TestFindConfig(t *testing.T) { }) } +func TestFindBundleRootDirectories(t *testing.T) { + t.Parallel() + + cfg := ` +project: + roots: + - foo/bar + - baz +` + + fs := map[string]string{ + "/.regal/config.yaml": cfg, // root from config + "/bundle/.manifest": "", // bundle from .manifest + "/foo/bar/baz/policy.rego": "", // foo/bar from config + "/baz": "", // baz from config + } + + test.WithTempFS(fs, func(root string) { + locations, err := FindBundleRootDirectories(root) + if err != nil { + t.Error(err) + } + + if len(locations) != 4 { + t.Errorf("expected 4 locations, got %d", len(locations)) + } + + expected := util.Map(util.FilepathJoiner(root), []string{"", "baz", "bundle", "foo/bar"}) + + if !slices.Equal(expected, locations) { + t.Errorf("expected %v, got %v", expected, locations) + } + }) +} + func TestMarshalConfig(t *testing.T) { t.Parallel() diff --git a/pkg/config/filter.go b/pkg/config/filter.go index 7ede79b7..6094de2d 100644 --- a/pkg/config/filter.go +++ b/pkg/config/filter.go @@ -10,6 +10,8 @@ import ( "github.com/gobwas/glob" "github.com/open-policy-agent/opa/bundle" + + rio "github.com/styrainc/regal/internal/io" ) func FilterIgnoredPaths(paths, ignore []string, checkFileExists bool, rootDir string) ([]string, error) { @@ -22,9 +24,10 @@ func FilterIgnoredPaths(paths, ignore []string, checkFileExists bool, rootDir st filtered := make([]string, 0, len(paths)) if err := walkPaths(paths, func(path string, info os.DirEntry, err error) error { - if info.IsDir() && (info.Name() == ".git" || info.Name() == ".idea" || info.Name() == "node_modules") { + if rio.IsSkipWalkDirectory(info) { return filepath.SkipDir } + if !info.IsDir() && strings.HasSuffix(path, bundle.RegoExt) { filtered = append(filtered, path) } diff --git a/pkg/fixer/fileprovider/fp.go b/pkg/fixer/fileprovider/fp.go index ab7832c8..eb10edc2 100644 --- a/pkg/fixer/fileprovider/fp.go +++ b/pkg/fixer/fileprovider/fp.go @@ -6,5 +6,6 @@ type FileProvider interface { ListFiles() ([]string, error) GetFile(string) ([]byte, error) PutFile(string, []byte) error + DeleteFile(string) error ToInput() (rules.Input, error) } diff --git a/pkg/fixer/fileprovider/fs.go b/pkg/fixer/fileprovider/fs.go index 6ab7fb93..5da8e19c 100644 --- a/pkg/fixer/fileprovider/fs.go +++ b/pkg/fixer/fileprovider/fs.go @@ -3,33 +3,43 @@ package fileprovider import ( "fmt" "os" + "slices" "github.com/open-policy-agent/opa/ast" "github.com/styrainc/regal/internal/parse" + "github.com/styrainc/regal/internal/util" "github.com/styrainc/regal/pkg/config" "github.com/styrainc/regal/pkg/rules" ) type FSFileProvider struct { - roots []string + roots map[string]struct{} ignore []string } func NewFSFileProvider(ignore []string, roots ...string) *FSFileProvider { + rootsMap := make(map[string]struct{}, len(roots)) + for _, root := range roots { + rootsMap[root] = struct{}{} + } + return &FSFileProvider{ - roots: roots, + roots: rootsMap, ignore: ignore, } } func (p *FSFileProvider) ListFiles() ([]string, error) { - filtered, err := config.FilterIgnoredPaths(p.roots, p.ignore, true, "") + filtered, err := config.FilterIgnoredPaths(util.Keys(p.roots), p.ignore, true, "") if err != nil { return nil, fmt.Errorf("failed to filter ignored paths: %w", err) } - return filtered, nil + slices.Sort(filtered) + + // TODO: Figure out why filtered returns duplicates in the first place + return slices.Compact(filtered), nil } func (*FSFileProvider) GetFile(file string) ([]byte, error) { @@ -41,7 +51,7 @@ func (*FSFileProvider) GetFile(file string) ([]byte, error) { return content, nil } -func (*FSFileProvider) PutFile(file string, content []byte) error { +func (p *FSFileProvider) PutFile(file string, content []byte) error { stat, err := os.Stat(file) if err != nil { return fmt.Errorf("failed to stat file %s: %w", file, err) @@ -52,6 +62,27 @@ func (*FSFileProvider) PutFile(file string, content []byte) error { return fmt.Errorf("failed to write file %s: %w", file, err) } + p.roots[file] = struct{}{} + + return nil +} + +// DeleteFile removes the file from the filesystem if it exists. If it does not exist +// DeleteFile will return nil, as it's already gone. Returns error on any other issues. +func (p *FSFileProvider) DeleteFile(file string) error { + if _, err := os.Stat(file); err != nil && os.IsNotExist(err) { + delete(p.roots, file) + + return nil + } + + err := os.Remove(file) + if err != nil { + return fmt.Errorf("failed to remove file %s: %w", file, err) + } + + delete(p.roots, file) + return nil } diff --git a/pkg/fixer/fileprovider/inmem.go b/pkg/fixer/fileprovider/inmem.go index 099be797..964c7bcc 100644 --- a/pkg/fixer/fileprovider/inmem.go +++ b/pkg/fixer/fileprovider/inmem.go @@ -43,6 +43,12 @@ func (p *InMemoryFileProvider) PutFile(file string, content []byte) error { return nil } +func (p *InMemoryFileProvider) DeleteFile(file string) error { + delete(p.files, file) + + return nil +} + func (p *InMemoryFileProvider) ToInput() (rules.Input, error) { modules := make(map[string]*ast.Module) diff --git a/pkg/fixer/fixer.go b/pkg/fixer/fixer.go index aa43f7fa..9475345d 100644 --- a/pkg/fixer/fixer.go +++ b/pkg/fixer/fixer.go @@ -4,9 +4,11 @@ import ( "bytes" "context" "fmt" + "path/filepath" "github.com/open-policy-agent/opa/ast" + "github.com/styrainc/regal/internal/util" "github.com/styrainc/regal/pkg/fixer/fileprovider" "github.com/styrainc/regal/pkg/fixer/fixes" "github.com/styrainc/regal/pkg/linter" @@ -17,6 +19,7 @@ func NewFixer() *Fixer { return &Fixer{ registeredFixes: make(map[string]any), registeredMandatoryFixes: make(map[string]any), + registeredRoots: make([]string, 0), } } @@ -24,6 +27,7 @@ func NewFixer() *Fixer { type Fixer struct { registeredFixes map[string]any registeredMandatoryFixes map[string]any + registeredRoots []string } // RegisterFixes sets the fixes that will be fixed if there are related linter @@ -47,6 +51,14 @@ func (f *Fixer) RegisterMandatoryFixes(fixes ...fixes.Fix) { } } +// RegisterRoots sets the roots of the files that will be fixed. +// Certain fixes may require the nearest root of the file to be known, +// as fix operations could involve things like moving files, which +// will be moved relative to their nearest root. +func (f *Fixer) RegisterRoots(roots ...string) { + f.registeredRoots = append(f.registeredRoots, roots...) +} + func (f *Fixer) GetFixForName(name string) (fixes.Fix, bool) { fix, ok := f.registeredFixes[name] if !ok { @@ -146,6 +158,8 @@ func (f *Fixer) Fix(ctx context.Context, l *linter.Linter, fp fileprovider.FileP } } + movedFiles := make(map[string]string) + for { fixMadeInIteration := false @@ -164,22 +178,36 @@ func (f *Fixer) Fix(ctx context.Context, l *linter.Linter, fp fileprovider.FileP } for _, violation := range rep.Violations { + file, ok := movedFiles[violation.Location.File] + if !ok { + file = violation.Location.File + } + fixInstance, ok := f.GetFixForName(violation.Title) if !ok { return nil, fmt.Errorf("no fix for violation %s", violation.Title) } - fc, err := fp.GetFile(violation.Location.File) + fc, err := fp.GetFile(file) if err != nil { - return nil, fmt.Errorf("failed to get file %s: %w", violation.Location.File, err) + return nil, fmt.Errorf("failed to get file %s: %w", file, err) } - fixCandidate := fixes.FixCandidate{ - Filename: violation.Location.File, - Contents: fc, + fixCandidate := fixes.FixCandidate{Filename: file, Contents: fc} + + config, err := l.GetConfig() + if err != nil { + return nil, fmt.Errorf("failed to get config: %w", err) + } + + abs, err := filepath.Abs(file) + if err != nil { + return nil, fmt.Errorf("failed to get absolute path for %s: %w", file, err) } fixResults, err := fixInstance.Fix(&fixCandidate, &fixes.RuntimeOptions{ + BaseDir: util.FindClosestMatchingRoot(abs, f.registeredRoots), + Config: config, Locations: []ast.Location{ { Row: violation.Location.Row, @@ -188,19 +216,36 @@ func (f *Fixer) Fix(ctx context.Context, l *linter.Linter, fp fileprovider.FileP }, }) if err != nil { - return nil, fmt.Errorf("failed to fix %s: %w", violation.Location.File, err) + return nil, fmt.Errorf("failed to fix %s: %w", file, err) } if len(fixResults) > 0 { // Note: Only one content update fix result is currently supported fixResult := fixResults[0] - err = fp.PutFile(violation.Location.File, fixResult.Contents) + if bytes.Equal(fc, fixResult.Contents) { + // if file was moved, we need to update the file provider accordingly + if fixResult.Rename != nil { + err := fp.DeleteFile(fixResult.Rename.FromPath) + if err != nil { + return nil, fmt.Errorf("failed to delete file %s: %w", fixResult.Rename.FromPath, err) + } + + movedFiles[file] = fixResult.Rename.ToPath + file = fixResult.Rename.ToPath + + // PutFile called further below + + fixReport.SetFileFixedViolation(file, violation.Title) + } + } + + err = fp.PutFile(file, fixResult.Contents) if err != nil { return nil, fmt.Errorf("failed to write fixed content to file %s: %w", violation.Location.File, err) } - fixReport.SetFileFixedViolation(violation.Location.File, violation.Title) + fixReport.SetFileFixedViolation(file, violation.Title) fixMadeInIteration = true } diff --git a/pkg/fixer/fixes/directorypackagemismatch.go b/pkg/fixer/fixes/directorypackagemismatch.go new file mode 100644 index 00000000..75670aed --- /dev/null +++ b/pkg/fixer/fixes/directorypackagemismatch.go @@ -0,0 +1,180 @@ +package fixes + +import ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + "regexp" + "strings" + + "github.com/open-policy-agent/opa/ast" + + "github.com/styrainc/regal/pkg/config" +) + +type DirectoryPackageMismatch struct { + // DryRun indicates whether this fixer should perform the actual rename or not, and if true simply signals to the + // client which file should be moved and to where. Whether to actually treat it as a "dry run" is up to the client. + DryRun bool +} + +func (*DirectoryPackageMismatch) Name() string { + return "directory-package-mismatch" +} + +// For now, just handle the "normal" set of characters, plus hyphens. +// We can broaden this later, but we should avoid characters that may have +// special meaning in files and directories. +var regularName = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_-]*$`) + +// Fix moves a file to the correct directory based on the package name, and relative to the base +// directory provided in opts. If the file is already in the correct directory, no action is taken. +// If the DryRun field is set to true, the file is not moved by the fixer, but will rather have the +// old location and new location passed back to the client who will decide what to do with that. +func (d *DirectoryPackageMismatch) Fix(fc *FixCandidate, opts *RuntimeOptions) ([]FixResult, error) { + pkgPath, err := getPackagePathDirectory(fc, opts.Config) + if err != nil { + return nil, err + } + + rootPath := opts.BaseDir + + if rootPath == "" { + rootPath = filepath.Dir(fc.Filename) + } + + rootPath = filepath.Clean(rootPath) + + newPath := filepath.Join(rootPath, pkgPath, filepath.Base(fc.Filename)) + + if newPath == fc.Filename { + return nil, nil // File is where it should be. We are done! + } + + if d.DryRun { + newRel, err := filepath.Rel(rootPath, newPath) + if err != nil { + return nil, fmt.Errorf("failed to calculate relative path: %w", err) + } + + return []FixResult{{ + Contents: fc.Contents, + Rename: &Rename{ + FromPath: fc.Filename, + ToPath: filepath.Join(rootPath, newRel), + }, + }}, nil + } + + if err := recursiveMoveFile(fc.Filename, newPath); err != nil { + return nil, fmt.Errorf("unexpected error when moving file %w", err) + } + + if err := cleanOldPath(rootPath, fc.Filename); err != nil { + return nil, fmt.Errorf("failed to clean up directory made empty by moving file %w", err) + } + + return []FixResult{{ + Contents: fc.Contents, + Rename: &Rename{ + FromPath: fc.Filename, + ToPath: newPath, // TODO: should we check that this is relative to base somewhere? + }, + }}, nil +} + +func getPackagePathDirectory(fc *FixCandidate, config *config.Config) (string, error) { + module, err := ast.ParseModule(fc.Filename, string(fc.Contents)) + if err != nil { + return "", err // nolint:wrapcheck + } + + parts := make([]string, len(module.Package.Path)-1) + excludeTestSuffix := shouldExcludeTestSuffix(config) + pathWithoutData := module.Package.Path[1:] + + for i, part := range pathWithoutData { + text := strings.Trim(part.Value.String(), "\"") + + if !regularName.MatchString(text) { + return "", fmt.Errorf("can only handle [a-zA-Z0-9_-] characters in package name, got: %s", text) + } + + if i == len(pathWithoutData)-1 && excludeTestSuffix { + text = strings.TrimSuffix(text, "_test") + } + + parts[i] = text + } + + return filepath.Join(parts...), nil +} + +// nolint:wrapcheck +func recursiveMoveFile(oldPath, newPath string) error { + _, err := os.Stat(filepath.Dir(newPath)) + if err != nil { + if os.IsNotExist(err) { + if err := os.MkdirAll(filepath.Dir(newPath), 0o755); err != nil { + return err + } + } else { + return err + } + } + + err = os.Rename(oldPath, newPath) + if err != nil { + return err + } + + return nil +} + +// nolint:wrapcheck +func cleanOldPath(rootPath, oldPath string) error { + // Remove empty directories + dir := filepath.Dir(oldPath) + + f, err := os.Open(dir) + if err != nil { + return err + } + + defer f.Close() + + _, err = f.Readdirnames(1) + if errors.Is(err, io.EOF) { // dir is empty + if err := os.Remove(dir); err != nil { + return err + } + + // traverse one level up in the directory tree to see + // if we've left more empty directories behind + up, _ := filepath.Split(dir) + + if up != rootPath { + return cleanOldPath(rootPath, up) + } + } else if err != nil { + return err + } + + return nil +} + +func shouldExcludeTestSuffix(config *config.Config) bool { + if category, ok := config.Rules["idiomatic"]; ok { + if rule, ok := category["directory-package-mismatch"]; ok { + if exclude, ok := rule.Extra["exclude-test-suffix"].(bool); ok { + return exclude + } + } + } + + // this is the default, and this should be unreachable provided that the + // provided configuration was included (which it always would be) + return true +} diff --git a/pkg/fixer/fixes/directorypackagemismatch_test.go b/pkg/fixer/fixes/directorypackagemismatch_test.go new file mode 100644 index 00000000..c74be4fa --- /dev/null +++ b/pkg/fixer/fixes/directorypackagemismatch_test.go @@ -0,0 +1,252 @@ +package fixes + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/styrainc/regal/pkg/config" +) + +func TestFixDirectoryPackageMismatch(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + files map[string]string + expected map[string]string + wantErr string + includeTestSuffix bool // inverse of exclude-text-suffix to avoid providing the default everywhere + }{ + { + name: "files are moved according to their package", + files: map[string]string{ + "main.rego": "package main", + "foo/bar.rego": "package bar", + "foo/baz.rego": "package foo.baz", + }, + expected: map[string]string{ + "main/main.rego": "package main", + "bar/bar.rego": "package bar", + "foo/baz/baz.rego": "package foo.baz", + }, + }, + { + name: "empty directories are cleaned up", + files: map[string]string{ + "foo/bar.rego": "package bar", + }, + expected: map[string]string{ + "bar/bar.rego": "package bar", + }, + }, + { + name: "non-empty directories are not cleaned up", + files: map[string]string{ + "foo/bar.rego": "package bar", + "foo/README.md": "This is docs!", + }, + expected: map[string]string{ + "bar/bar.rego": "package bar", + "foo/README.md": "This is docs!", + }, + }, + { + name: "nested directories are created correctly", + files: map[string]string{ + "baz.rego": "package foo.bar.baz", + "qux.rego": "package foo.bar", + }, + expected: map[string]string{ + "foo/bar/baz/baz.rego": "package foo.bar.baz", + "foo/bar/qux.rego": "package foo.bar", + }, + }, + { + name: "nested directories are cleaned up correctly", + files: map[string]string{ + "foo/bar/qux/yoo/foo.rego": "package foo", + "foo/bar/foo.bar.rego": "package foo.bar", + }, + expected: map[string]string{ + "foo/foo.rego": "package foo", + "foo/bar/foo.bar.rego": "package foo.bar", + }, + }, + { + name: "package name with hyphens creates directories with hyphens", + files: map[string]string{ + "foo.rego": `package foo["bar-baz"].qux`, + }, + expected: map[string]string{ + "foo/bar-baz/qux/foo.rego": `package foo["bar-baz"].qux`, + }, + }, + { + name: "package name with special characters returns an error", + files: map[string]string{ + "foo.rego": `package foo["bar/baz"].qux`, + }, + expected: map[string]string{ + "foo.rego": `package foo["bar/baz"].qux`, + }, + wantErr: "can only handle [a-zA-Z0-9_-] characters in package name, got: bar/baz", + }, + { + name: "package names with _test suffix are by default treated as if without the suffix", + files: map[string]string{ + "foo_test.rego": "package foo_test", + "foo.rego": "package foo", + }, + expected: map[string]string{ + "foo/foo_test.rego": "package foo_test", + "foo/foo.rego": "package foo", + }, + }, + { + name: "package names with _test suffix are accounted for when config exclude-test-suffix is false", + files: map[string]string{ + "foo_test.rego": "package foo_test", + "foo.rego": "package foo", + }, + expected: map[string]string{ + "foo_test/foo_test.rego": "package foo_test", + "foo/foo.rego": "package foo", + }, + includeTestSuffix: true, + }, + { + name: "nested package names with _test suffix are accounted for when config exclude-test-suffix is false", + files: map[string]string{ + "foo_test.rego": "package foo.bar.baz_test", + "foo.rego": "package foo.bar.baz", + "qux.rego": "package foo[\"bar-baz\"].qux", + "bar/bar/bar.rego": "package foo[\"bar-baz\"].qux_test", + }, + expected: map[string]string{ + "foo/bar/baz_test/foo_test.rego": "package foo.bar.baz_test", + "foo/bar/baz/foo.rego": "package foo.bar.baz", + "foo/bar-baz/qux/qux.rego": "package foo[\"bar-baz\"].qux", + "foo/bar-baz/qux_test/bar.rego": "package foo[\"bar-baz\"].qux_test", + }, + includeTestSuffix: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + baseDir := filepath.Join(tmpDir, "bundle") + + writeFiles(t, baseDir, tc.files) + + dpm := DirectoryPackageMismatch{} + + for file, contents := range tc.files { + if !strings.HasSuffix(file, ".rego") { + continue + } + + _, err := dpm.Fix(&FixCandidate{ + Filename: filepath.Join(baseDir, file), + Contents: []byte(contents), + }, &RuntimeOptions{ + BaseDir: baseDir, + Config: configWithExcludeTestSuffix(!tc.includeTestSuffix), + }) + if err != nil { + if tc.wantErr == "" { + t.Errorf("failed to fix %s: %v", file, err) + } + + if strings.Contains(err.Error(), tc.wantErr) { + continue + } else { + t.Errorf("expected error to contain %q, got %q", tc.wantErr, err.Error()) + } + } + } + + result := readFiles(t, baseDir) + + if len(result) != len(tc.expected) { + t.Errorf("expected %d files, got %d", len(tc.expected), len(result)) + } + + for file, contents := range tc.expected { + if result[file] != contents { + t.Errorf("expected %s to be %s, got %s", file, contents, result[file]) + } + } + }) + } +} + +func writeFiles(t *testing.T, path string, files map[string]string) { + t.Helper() + + for file, contents := range files { + filePath := filepath.Join(path, file) + + dir := filepath.Dir(filePath) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("failed to create directory %s: %v", dir, err) + } + + err := os.WriteFile(filePath, []byte(contents), 0o600) + if err != nil { + t.Fatalf("failed to write file %s: %v", filePath, err) + } + } +} + +// recursively traverse dir entry and create a map[string]string of all files +// where the key is the full path, and the value is the file contents. +func readFiles(t *testing.T, baseDir string) map[string]string { + t.Helper() + + files := make(map[string]string) + + err := filepath.Walk(baseDir, func(path string, info os.FileInfo, err error) error { + if err != nil { + t.Fatalf("failed to walk path %s: %v", path, err) + } + + if info.IsDir() { + return nil + } + + contents, err := os.ReadFile(path) + if err != nil { + t.Fatalf("failed to read file %s: %v", path, err) + } + + relPath, _ := filepath.Rel(baseDir, path) + files[relPath] = string(contents) + + return nil + }) + if err != nil { + t.Fatalf("failed to walk path %s: %v", baseDir, err) + } + + return files +} + +func configWithExcludeTestSuffix(exclude bool) *config.Config { + return &config.Config{ + Rules: map[string]config.Category{ + "idiomatic": { + "directory-package-mismatch": config.Rule{ + Level: "ignore", + Extra: map[string]any{ + "exclude-test-suffix": exclude, + }, + }, + }, + }, + } +} diff --git a/pkg/fixer/fixes/fixes.go b/pkg/fixer/fixes/fixes.go index 74140cbc..729f34c9 100644 --- a/pkg/fixer/fixes/fixes.go +++ b/pkg/fixer/fixes/fixes.go @@ -3,6 +3,9 @@ package fixes import ( "github.com/open-policy-agent/opa/ast" "github.com/open-policy-agent/opa/format" + + "github.com/styrainc/regal/internal/lsp/clients" + "github.com/styrainc/regal/pkg/config" ) // NewDefaultFixes returns a list of default fixes that are applied by the fix command. @@ -17,6 +20,7 @@ func NewDefaultFixes() []Fix { }, &UseAssignmentOperator{}, &NoWhitespaceComment{}, + &DirectoryPackageMismatch{}, } } @@ -31,6 +35,11 @@ type Fix interface { // RuntimeOptions are the options that are passed to the Fix method when the Fix is executed. // Location based fixes will have the locations populated by the caller. type RuntimeOptions struct { + // BaseDir is the base directory for the files being fixed. This is often the same as the + // workspace root directory, but not necessarily. + BaseDir string + Config *config.Config + Client clients.Identifier Locations []ast.Location } @@ -40,8 +49,22 @@ type FixCandidate struct { Contents []byte } +// Rename represents a file that has been moved (renamed). +type Rename struct { + FromPath string + ToPath string +} + // FixResult is returned from the Fix method and contains the new contents or fix recommendations. -// In future this might support diff based updates, or renames. +// In future this might support diff based updates. type FixResult struct { + // Contents is the new contents of the file. May be nil or identical to the original contents, + // as not all fixes involve content changes. It is the responsibility of the caller to handle + // this. Contents []byte + // Rename is used to indicate that a rename operation should be performed by the **caller**. + // An example of this would be the DirectoryPackageMismatch fix, which in the context of + // `regal fix` renames files as part of the fix, while when invoked as a LSP Code Action will + // defer the actual rename back to the client. + Rename *Rename } diff --git a/pkg/fixer/fixes/fmt.go b/pkg/fixer/fixes/fmt.go index 1763dcc0..b8f90a4b 100644 --- a/pkg/fixer/fixes/fmt.go +++ b/pkg/fixer/fixes/fmt.go @@ -41,9 +41,5 @@ func (f *Fmt) Fix(fc *FixCandidate, _ *RuntimeOptions) ([]FixResult, error) { return nil, nil } - return []FixResult{ - { - Contents: formatted, - }, - }, nil + return []FixResult{{Contents: formatted}}, nil } diff --git a/pkg/linter/linter.go b/pkg/linter/linter.go index cfb992de..33834d15 100644 --- a/pkg/linter/linter.go +++ b/pkg/linter/linter.go @@ -226,7 +226,7 @@ func (l Linter) Lint(ctx context.Context) (report.Report, error) { return report.Report{}, errors.New("nothing provided to lint") } - conf, err := l.combinedConfig() + conf, err := l.GetConfig() if err != nil { return report.Report{}, fmt.Errorf("failed to merge config: %w", err) } @@ -862,7 +862,9 @@ func resultSetToReport(resultSet rego.ResultSet) (report.Report, error) { return r, nil } -func (l Linter) combinedConfig() (*config.Config, error) { +// GetConfig returns the final configuration for the linter, i.e. Regal's default +// configuration plus any user-provided configuration merged on top of it. +func (l Linter) GetConfig() (*config.Config, error) { if l.combinedCfg != nil { return l.combinedCfg, nil } @@ -918,7 +920,7 @@ func (l Linter) enabledGoRules() ([]rules.Rule, error) { return enabledGoRules, nil } - conf, err := l.combinedConfig() + conf, err := l.GetConfig() if err != nil { return nil, fmt.Errorf("failed to create merged config: %w", err) } diff --git a/pkg/linter/linter_test.go b/pkg/linter/linter_test.go index c0220d82..9fd2decb 100644 --- a/pkg/linter/linter_test.go +++ b/pkg/linter/linter_test.go @@ -438,7 +438,7 @@ func TestLintMergedConfigInheritsLevelFromProvided(t *testing.T) { WithUserConfig(userConfig). WithInputModules(&input) - mergedConfig := testutil.Must(linter.combinedConfig())(t) + mergedConfig := testutil.Must(linter.GetConfig())(t) // Since no level was provided, "error" should be inherited from the provided configuration for the rule if mergedConfig.Rules["style"]["file-length"].Level != "error" { @@ -477,7 +477,7 @@ func TestLintMergedConfigUsesProvidedDefaults(t *testing.T) { WithUserConfig(userConfig). WithInputModules(&input) - mergedConfig := testutil.Must(linter.combinedConfig())(t) + mergedConfig := testutil.Must(linter.GetConfig())(t) // specifically configured rule should not be affected by the default if mergedConfig.Rules["style"]["opa-fmt"].Level != "warning" {