Allow fixing directory structure not matching package paths

Following the recent addition of the `directory-package-mismatch` rule,
this PR brings a corresponding `regal fix` remediator as well as a code
action to fix this directly in the editor.

Fixes #1025

Signed-off-by: Anders Eknert <anders@styra.com>

.golangci.yaml

@@ -5,6 +5,7 @@ linters:
     - rowserrcheck
     - wastedassign
     # annoying
+    - gocyclo
     - tagliatelle
     - nestif
     - gocognit

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)
 

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),

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",

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()
+}

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)
+	}
+}

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
+}

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)
+		}
+	})
+}

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)
 	}

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),
+	}
+}