fix: return clear error when migration file already exists - #1104
fix: return clear error when migration file already exists#1104faiyaz032 wants to merge 2 commits into
Conversation
When two migrations are created in the same second they resolve to the same timestamp-based filename. The existence check wrapped a nil error with %w, producing '%!w(<nil>)' instead of a useful message. Return os.ErrExist so the user sees 'file already exists'. Closes pressly#707
There was a problem hiding this comment.
🟡 Changes recommended
The new test is timing-dependent and potentially flaky, and the current Stat-then-Create flow still allows duplicate creates to race and succeed (truncating) without reliably returning os.ErrExist.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes goose create error handling when a migration filename collides (same timestamp-based name), so the user gets a meaningful “file already exists” (os.ErrExist) instead of %!w(<nil>).
Changes:
- Update
CreateWithTemplateto treat an existing migration file asos.ErrExistand to distinguish that case from otherStatfailures. - Add a regression test that creates the same migration twice and asserts the second call returns
os.ErrExist.
File summaries
| File | Description |
|---|---|
| create.go | Adjusts duplicate-file detection and error wrapping for clearer “already exists” failures. |
| create_test.go | Adds a regression test for duplicate migration creation. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| path := filepath.Join(dir, filename) | ||
| if _, err := os.Stat(path); !os.IsNotExist(err) { | ||
| if _, err := os.Stat(path); err == nil { | ||
| return fmt.Errorf("failed to create migration file: %w", os.ErrExist) | ||
| } else if !errors.Is(err, os.ErrNotExist) { | ||
| return fmt.Errorf("failed to create migration file: %w", err) |
There was a problem hiding this comment.
Switched to an atomic O_EXCL open, so duplicate creates fail consistently instead of racing past a separate Stat check. This also closes the concurrent create race I'd flagged as out of scope.
| func TestCreateDuplicateFile(t *testing.T) { | ||
| dir := t.TempDir() | ||
| if err := Create(nil, dir, "add_users", "sql"); err != nil { |
There was a problem hiding this comment.
Now it overrides timestampFormat via t. Cleanup now, so the test is deterministic instead of depending on both creates landing in the same second.
- atomic O_EXCL open instead of Stat-then-Create (fixes the race Copilot flagged) - test overrides timestampFormat so it's deterministic Relates to pressly#707
Fixes the
%!w(<nil>)error fromgoose createwhen a migration with the samename is created within the same second (same timestamp filename).
The old check
!os.IsNotExist(err)wrapped anilerror with%win thefile exists case. Now it returns a clear
os.ErrExist("file already exists")and still surfaces other stat errors.
Builds on #708 / @obalunenko's review. Uses
os.ErrNotExist(notfs.ErrNotExist)to avoid a new import.
Testing: added
TestCreateDuplicateFile— verified it fails on the old codeand passes with the fix; full suite green under
-race.Out of scope: the same-second collision comes from second-granularity versioning,
and concurrent creates can still race the check — happy to file a separate issue.
Closes #707