fixer: add --on-conflict flag to support renaming (#1127)

This PR adds a new flag to the fix command, `--on-conflict`. This flag
can be used to specify the behavior of the fix command when a file's new
name conflicts with an existing file or another file's fixed name.

Setting --on-conflict to "rename" will cause the fix command to rename
the file to a different name at the target location. The new name will
be generated by appending a number to the end of the file name.

Signed-off-by: Charlie Egan <charlie@styra.com>

cmd/fix.go

@@ -50,6 +50,7 @@ type fixCommandParams struct {
 	dryRun          bool
 	verbose         bool
 	force           bool
+	conflictMode    string
 }
 
 func (p *fixCommandParams) getConfigFile() string {
@@ -150,6 +151,9 @@ The linter rules with automatic fixes available are currently:
 	fixCommand.Flags().BoolVarP(&params.force, "force", "", false,
 		"allow fixing of files that have uncommitted changes in git or when git is not being used")
 
+	fixCommand.Flags().StringVarP(&params.conflictMode, "on-conflict", "", "error",
+		"configure behavior when filename conflicts are detected. Options are 'error' (default) or 'rename'")
+
 	addPprofFlag(fixCommand.Flags())
 
 	RootCommand.AddCommand(fixCommand)
@@ -276,6 +280,15 @@ func fix(args []string, params *fixCommandParams) error {
 		},
 	)
 
+	if !slices.Contains([]string{"error", "rename"}, params.conflictMode) {
+		return fmt.Errorf("invalid conflict mode: %s, expected 'error' or 'rename'", params.conflictMode)
+	}
+
+	// the default is error, so this is only set when it's rename
+	if params.conflictMode == "rename" {
+		f.SetOnConflictOperation(fixer.OnConflictRename)
+	}
+
 	ignore := userConfig.Ignore.Files
 
 	if len(params.ignoreFiles.v) > 0 {

e2e/cli_test.go

@@ -1011,6 +1011,116 @@ Cannot move multiple files to: bar/bar.rego
 	}
 }
 
+func TestFixWithConflictRenaming(t *testing.T) {
+	t.Parallel()
+
+	stdout := bytes.Buffer{}
+	stderr := bytes.Buffer{}
+	td := t.TempDir()
+
+	initialState := map[string]string{
+		".regal/config.yaml": "", // needed to find the root in the right place
+		// this file is in the correct location
+		"foo/foo.rego": `package foo
+
+import rego.v1
+`,
+		// this file is in the correct location
+		"foo/foo_test.rego": `package foo_test
+
+import rego.v1
+`,
+		// this file should be at foo/foo.rego, but that file already exists
+		"quz/foo.rego": `package foo
+
+import rego.v1
+`,
+		// this file should be at foo/foo_test.rego, but that file already exists
+		"quz/foo_test.rego": `package foo_test
+
+import rego.v1
+`,
+		// this file should be at bar/bar.rego and is not a conflict
+		"foo/bar.rego": `package bar
+
+import rego.v1
+`,
+	}
+
+	for file, content := range initialState {
+		mustWriteToFile(t, filepath.Join(td, file), string(content))
+	}
+
+	// --force is required to make the changes when there is no git repo
+	// --conflict=rename will rename inbound files when there is a conflict
+	err := regal(&stdout, &stderr)("fix", "--force", "--on-conflict=rename", td)
+
+	// 0 exit status is expected as all violations should have been fixed
+	expectExitCode(t, err, 0, &stdout, &stderr)
+
+	expStdout := fmt.Sprintf(`3 fixes applied:
+In project root: %[1]s
+foo/bar.rego -> bar/bar.rego:
+- directory-package-mismatch
+quz/foo.rego -> foo/foo_1.rego:
+- directory-package-mismatch
+quz/foo_test.rego -> foo/foo_1_test.rego:
+- directory-package-mismatch
+`, td)
+
+	if act := stdout.String(); expStdout != act {
+		t.Errorf("expected stdout:\n%s\ngot\n%s", expStdout, act)
+	}
+
+	expectedState := map[string]string{
+		".regal/config.yaml": "", // needed to find the root in the right place
+		// unchanged
+		"foo/foo.rego": `package foo
+
+import rego.v1
+`,
+		// renamed to permit its new location
+		"foo/foo_1.rego": `package foo
+
+import rego.v1
+`,
+		// renamed to permit its new location
+		"foo/foo_1_test.rego": `package foo_test
+
+import rego.v1
+`,
+		// unchanged
+		"bar/bar.rego": `package bar
+
+import rego.v1
+`,
+	}
+
+	for file, expectedContent := range expectedState {
+		bs, err := os.ReadFile(filepath.Join(td, file))
+		if err != nil {
+			t.Errorf("failed to read %s: %v", file, err)
+
+			continue
+		}
+
+		if act := string(bs); expectedContent != act {
+			t.Errorf("expected %s contents:\n%s\ngot\n%s", file, expectedContent, act)
+		}
+	}
+
+	expectedMissing := []string{
+		"quz/foo.rego",
+		"quz/foo_test.rego",
+	}
+
+	for _, file := range expectedMissing {
+		if _, err := os.Stat(filepath.Join(td, file)); err == nil {
+			t.Errorf("expected %s to have been removed", file)
+		}
+	}
+}
+
 // verify fix for https://github.com/StyraInc/regal/issues/1082
 func TestLintAnnotationCustomAttributeMultipleItems(t *testing.T) {
 	t.Parallel()

pkg/fixer/fixer.go

@@ -16,12 +16,20 @@ import (
 	"github.com/styrainc/regal/pkg/linter"
 )
 
+type OnConflictOperation string
+
+const (
+	OnConflictError  OnConflictOperation = "error"
+	OnConflictRename OnConflictOperation = "rename"
+)
+
 // NewFixer instantiates a Fixer.
 func NewFixer() *Fixer {
 	return &Fixer{
 		registeredFixes:          make(map[string]any),
 		registeredMandatoryFixes: make(map[string]any),
 		registeredRoots:          make([]string, 0),
+		onConflictOperation:      OnConflictError,
 	}
 }
 
@@ -30,6 +38,12 @@ type Fixer struct {
 	registeredFixes          map[string]any
 	registeredMandatoryFixes map[string]any
 	registeredRoots          []string
+	onConflictOperation      OnConflictOperation
+}
+
+// SetOnConflictOperation sets the fixer's behavior when a conflict occurs.
+func (f *Fixer) SetOnConflictOperation(operation OnConflictOperation) {
+	f.onConflictOperation = operation
 }
 
 // RegisterFixes sets the fixes that will be fixed if there are related linter
@@ -217,6 +231,10 @@ func (f *Fixer) applyLinterFixes(
 			return fmt.Errorf("failed to lint before fixing: %w", err)
 		}
 
+		if len(rep.Violations) == 0 {
+			break
+		}
+
 		for _, violation := range rep.Violations {
 			file := violation.Location.File
 
@@ -266,42 +284,14 @@ func (f *Fixer) applyLinterFixes(
 			fixResult := fixResults[0]
 
 			if fixResult.Rename != nil {
-				to := fixResult.Rename.ToPath
-				from := fixResult.Rename.FromPath
-
-				var isConflict bool
-
-				err := fp.Rename(from, to)
-				// A conflict occurs if attempting to rename to an existing path
-				// which exists due to another renaming fix
-				if errors.As(err, &fileprovider.RenameConflictError{}) {
-					isConflict = true
-				}
-
-				if err != nil && !isConflict {
-					return fmt.Errorf("failed to rename file: %w", err)
-				}
-
-				fixReport.AddFileFix(to, fixResult)
-				fixReport.MergeFixes(to, from)
-				fixReport.RegisterOldPathForFile(to, from)
-
-				if isConflict {
-					// Clean the old file to prevent repeated fixes
-					if err := fp.Delete(from); err != nil {
-						return fmt.Errorf("failed to delete file %s: %w", from, err)
-					}
-
-					if slices.Contains(startingFiles, to) {
-						fixReport.RegisterConflictSourceFile(fixResult.Root, to, from)
-					} else {
-						fixReport.RegisterConflictManyToOne(fixResult.Root, to, from)
-					}
+				err = f.handleRename(fp, fixReport, startingFiles, fixResult)
+				if err != nil {
+					return err
 				}
 
 				fixMadeInIteration = true
 
-				break
+				break // Restart the loop after handling a rename
 			}
 
 			// Write the fixed content to the file
@@ -321,3 +311,73 @@ func (f *Fixer) applyLinterFixes(
 
 	return nil
 }
+
+// handleRename processes the rename operation and resolves conflicts if necessary.
+func (f *Fixer) handleRename(
+	fp fileprovider.FileProvider,
+	fixReport *Report,
+	startingFiles []string,
+	fixResult fixes.FixResult,
+) error {
+	to := fixResult.Rename.ToPath
+	from := fixResult.Rename.FromPath
+
+	for {
+		err := fp.Rename(from, to)
+		if err == nil {
+			// if there is no error, and no conflict, we have nothing to do
+			break
+		}
+
+		var isConflict bool
+		if errors.As(err, &fileprovider.RenameConflictError{}) {
+			isConflict = true
+		} else {
+			return fmt.Errorf("failed to rename file: %w", err)
+		}
+
+		if isConflict {
+			switch f.onConflictOperation {
+			case OnConflictError:
+				// OnConflictError is the default, these operations are taken to
+				// ensure the correct state in the report for outputting the
+				// verbose conflict report.
+				// clean the old file to prevent repeated fixes
+				if err := fp.Delete(from); err != nil {
+					return fmt.Errorf("failed to delete file %s: %w", from, err)
+				}
+
+				if slices.Contains(startingFiles, to) {
+					fixReport.RegisterConflictSourceFile(fixResult.Root, to, from)
+				} else {
+					fixReport.RegisterConflictManyToOne(fixResult.Root, to, from)
+				}
+
+				fixReport.AddFileFix(to, fixResult)
+				fixReport.MergeFixes(to, from)
+				fixReport.RegisterOldPathForFile(to, from)
+
+				return nil
+			case OnConflictRename:
+				// OnConflictRename will select a new filename until there is no
+				// conflict.
+				to = renameCandidate(to)
+
+				continue
+			default:
+				return fmt.Errorf("unsupported conflict operation: %v", f.onConflictOperation)
+			}
+		}
+	}
+
+	// update the fix result with the new path for consistency
+	if to != fixResult.Rename.ToPath {
+		fixResult.Rename.ToPath = to
+	}
+
+	fixReport.AddFileFix(to, fixResult)
+	fixReport.MergeFixes(to, from)
+	fixReport.RegisterOldPathForFile(to, from)
+
+	return nil
+}

pkg/fixer/rename.go

@@ -0,0 +1,44 @@
+package fixer
+
+import (
+	"fmt"
+	"path/filepath"
+	"regexp"
+	"strconv"
+	"strings"
+)
+
+// renameCandidate takes a filename and produces a new name with an incremented
+// numeric suffix. It correctly handles test files by inserting the increment
+// before the "_test" suffix and preserves the original directory.
+func renameCandidate(oldName string) string {
+	dir := filepath.Dir(oldName)
+	baseWithExt := filepath.Base(oldName)
+
+	ext := filepath.Ext(baseWithExt)
+	base := strings.TrimSuffix(baseWithExt, ext)
+
+	suffix := ""
+	if strings.HasSuffix(base, "_test") {
+		suffix = "_test"
+		base = strings.TrimSuffix(base, "_test")
+	}
+
+	re := regexp.MustCompile(`^(.*)_(\d+)$`)
+	matches := re.FindStringSubmatch(base)
+
+	if len(matches) == 3 {
+		baseName := matches[1]
+		numStr := matches[2]
+		num, _ := strconv.Atoi(numStr)
+		num++
+		base = fmt.Sprintf("%s_%d", baseName, num)
+	} else {
+		base += "_1"
+	}
+
+	newBase := base + suffix + ext
+	newName := filepath.Join(dir, newBase)
+
+	return newName
+}