Missing up migrations applied during "down"

[JasonMunchhof-msr]

I had a situation with switching branches where the latest migration in the DB was "newer" than 2 migration files that had not yet been applied.

When running a "Down" migration, I noticed the "Up" code for both migrations was unintentionally applied and even worse they weren't recorded in the migrations table.

I tracked it down to the PlanMigration() function where it adds the results of ToCatchup(), regardless of the migration direction.

Obviously there a few issues with this. 1. They are up migrations when applying down. 2. The "catchup" migrations aren't considered in the check against the "max" parameter (which is 1 in my case). 3. The up migrations aren't recorded in the DB at all, leaving us in a broken state.

I'm not fluent in Go quite yet, otherwise I'd make a PR... but maybe the "catchup" logic should be ignored altogether for Down migrations.

[pasztorpisti]

I had the same issue and looked at the code but I wasn't sure how to fix it or whether it is possible to fix it without changing the public interface of the library. In my opinion migrations should be executed from the CI/CD pipeline as a commandline tool and not by a service at startup (by using the migration tool as a library). I find the latter to be an antipattern and it makes changing/refactoring the code more difficult (because of the exported public stuff).

In my opinion what you need to perform migrations is only a goto <target> command. The up and down commands can be translated to goto latest and goto latest-1 respectively.

If we assume that we have a goto <target> command then it should do the following:

1. Migrations that are newer than <target> and have been up-migrated have to be reverted by down-migrating them in descending order.
2. The <target> migration along with the older migrations have to be up-migrated in ascending order (only those that haven't yet been applied).

Based on the above algorithm if we have a situation like this:

[X] 0001.sql
[ ] 0002.sql
[X] 0003.sql
[ ] 0004.sql
[X] 0005.sql
[X] 0006.sql
[X] 0007.sql
[ ] 0008.sql

... and we perform a goto 0005.sql then it should result in the following operations:

1. down-migrate 0007.sql
2. down-migrate 0006.sql
3. up-migrate 0002.sql
4. up-migrate 0004.sql

Result:

[X] 0001.sql
[X] 0002.sql
[X] 0003.sql
[X] 0004.sql
[X] 0005.sql
[ ] 0006.sql
[ ] 0007.sql
[ ] 0008.sql

However, I think if you have unapplied migrations that are older than some applied migrations then you already have a problem that should be resolved manually - which means that the goto <target> command should fail in the above situation instead of doing both down- and up- migrations.

You can easily end up with gaps in the range of your applied migrations by working on several branches and merging them.

[rubenv]

@pasztorpisti Cool to see you build your own tool, but please name it something else. Right now it's going to lead to a ton of confusion and it makes it impossible to have both installed at the same time (not to mention broken builds).

[pasztorpisti]

@rubenv For now I think it's better if I keep the tool private for myself (less hassle for many reasons). I'll have to think about a name and need free time to change the references everywhere (CI, docker hub) if I decide to make the change. I'm a barbarian who names things by simply joins keywords so "migrate" and "sql-migrate" are pretty obvious choices.

[rangzen]

Same problem.
When you go down and a previous up query is not done, it is added to the migrations (with this up query, see the call to catchup). The problem is that the direction of the migration is always down, so when you do the switch dir { of applyMigrations:508, you go to the down part, so the query is executed (probably a create), but the id is deleted (but not existing anyway) instead of being inserted (as an up query).

[rangzen]

Proposed PR #235