rubenv / sql-migrate

SQL schema migration tool for Go.
MIT License
3.2k stars 275 forks source link

Logic Bug with ExecVersion #255

Open its-a-feature opened 1 year ago

its-a-feature commented 1 year ago

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.

rubenv commented 12 months ago

Would you mind sending a PR with a unit test to illustrate this?