Skip to content

Logic Bug with ExecVersion #255

@its-a-feature

Description

@its-a-feature

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions