Logic Bug with ExecVersion
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 3.4k
- Forks
- 292
- PR merge metrics
- No merged PRs in 30d
Description
I think there's an issue with the logic for determining which migrations to apply when you specify a version here:
https://github.com/rubenv/sql-migrate/blob/master/migrate.go#L669-L684
Expectation
When specifying a version to apply, if we're already at that version, no migrations should be applied and we should continue on successfully.
Reality
If there are no migrations to apply when you specify a version, then there is a vague error thrown about Unknown migration with version id (your specified version) in database.
Code
Specifically:
if version >= 0 {
targetIndex := 0
for targetIndex < len(toApply) {
tempVersion := toApply[targetIndex].VersionInt()
if dir == Up && tempVersion > version || dir == Down && tempVersion < version {
return nil, nil, newPlanError(&Migration{}, fmt.Errorf("unknown migration with version id %d in database", version).Error())
}
if tempVersion == version {
toApplyCount = targetIndex + 1
break
}
targetIndex++
}
if targetIndex == len(toApply) {
return nil, nil, newPlanError(&Migration{}, fmt.Errorf("unknown migration with version id %d in database", version).Error())
}
} else if max > 0 && max < toApplyCount {
toApplyCount = max
}
Say you have one migration to apply, version 1. The first time you go through this on a database, that migration hasn't been applied, so it's applied and all is good. If you restart though and go back through this, there will be no migrations to apply (the one that's there has already been applied). The issue here is that the loop for targetIndex < len(toApply) doesn't execute because targetIndex is 0 and len(toApply) is 0. It then falls to the next line that reports it as an error with an unknown migration.
The error messages are also pretty ambiguous and misleading. Ex: I spent a while trying to figure out an unknown version in database when my database had no entries yet - turns out it was a bad version specified but since it meant that there were 0 to apply, it resulted in the same error.
Solution
I think all this needs is a check that if len(toApply) is 0, then we have 0 migrations to apply (which is ok) and return success.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in migrate.go around the ExecVersion logic at lines 669-684 and inspect how an empty toApply list is handled. Verify the behavior for an already-applied version and for an invalid version against an empty database. Done means an already-current database succeeds with no migrations, while invalid versions produce an accurate error.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100