Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logic Bug with ExecVersion #255

Open
its-a-feature opened this issue Sep 27, 2023 · 1 comment
Open

Logic Bug with ExecVersion #255

its-a-feature opened this issue Sep 27, 2023 · 1 comment

Comments

@its-a-feature
Copy link

its-a-feature commented Sep 27, 2023

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
Copy link
Owner

rubenv commented Oct 9, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants