Migrate: CLI `down` runs all down migrations by default, making it unnecessarily dangerous

[samjbobb]

I'm creating this issue to bring up an issue already raised on mattes/migrate: https://github.com/mattes/migrate/issues/289. I think it's appropriate to re-create the issue because that issue received no response and that repo is deprecated.

Copying from there:

The current behavior of down is as follows:

migrate down 1 # run one down migration
migrate down # run *all* down migrations

Since the down migrations typically drop tables or columns, they are destructive. Running all down migrations is often equivalent of dropped the entire DB.

This make it very easy to run a large destructive operation. In my use case, running all down migrations is rarely what I intend.

I would work on a PR to make down require an argument. Does this align with the project goals? Has this been discussed before?

[dhui]

Instead of making migrate down require an arg, I'd add a warning and confirmation prompt that the user intends to run all down migrations.

[dhui]

I stand corrected. Requiring an arg is a cleaner approach since it's easier to script against if you don't interact w/ stdin confirmation prompts.

I'm not sure if we should make down require an arg like --all or use a separate command like downall. This change would be backwards incompatible and break existing scripts.

[samjbobb]

Unfortunately I don't see a great way to avoid making this a backward incompatible change. I'm advocating for different behavior given the same CLI inputs.

Another option for the UI is to add the warning and confirmation prompt before running all down migrations (like you suggested). And then provide a -yes or -force option to override the confirmation prompt. That's a fairly common pattern.

[brblck]

Definitely concerned about this same issue. As-is, the default behavior for migrate down presents a massive risk of data loss. In our usage of this tool, we've written a wrapper around it to customize behavior and effectively neuter migrate down for safety.

Another possible and completely backward-compatible solution may be to introduce a migrate rollback command in the CLI that defaults to migrate down 1 behavior in combination with a warning on migrate down so that folks know they're about to unwind their whole schema.

[strindhaug]

Ok, so the pro/con consideration is something like this, I'd think:

Behaviour |Pro| Con
--|-------|----------
down deletes everything (no change) | Being "backward compatible" | Very surprising behavior. Lots of users continue to accidentally delete their database when intending to revert just one migration
down defaults to 1| Intuitive behavior. Users won't accidentally delete their database when intending to revert just one migration|Not "backward compatible"

Do we really need a migration script to have a shortcut to delete your whole database if you forget one digit in the command, just because it's backwards compatible?

How much trouble would it _really_ be to break backwards compatibility for this?
Wouldn't most people who actually want a script to delete everything usually explicitly call drop table on all the tables they want to delete? The number of users who use this "feature" deliberately to routinely delete their databases cannot possibly be that large; and certainly this number must be way smaller than the number of users who accidentally deletes everything when they wanted to just roll back one migration.

Not fixing this issue is more like being "bug compatible" than "backwards compatible".

I agree that it's good to be very conservative and not modify commands that is used regularly by automated scripts. And migrate up is a command that is probably run automatically in hundreds of scripts.
But migrating down is probably NOT a very common automated thing. I'm pretty sure migrating down one step is something you probably always do manually; and usually only locally while developing the migration, or occacionally on the dev server to revert a migration that turned out to be bad, and extremely rarely something you do in production. And migrating down EVERYTHING, i.e. deleting everything is something I hope nobody does deliberately in automated scripts very often. (And if there is 5 people in the world who does "abuse" this command to deliberately delete everything; I'm sure they can just fix their scripts if it stops working)

The point is migrating down is something you probably almost always do manually in an interactive shell and not in a script. And it's very easy to accidentally omit the final argument when copy/pasting the command (because you really don't want to mistype this command), or accidentally delete it with backspace just before you hit enter; or simply forget it since you usually don't specify a number when migrating up anyway. So you'd rather want this command to be as forgiving as possible rather than being backwards compatible at all cost.

I myself just deleted everything in one of our databases even though I KNEW it had this surprising behavior. (Fortunately it was just one less important statistical database for the development server)

[brblck]

Do we really need a migration script to have a shortcut to delete your whole database if you forget one digit in the command, just because it's backwards compatible?

:100: this.

I don’t even view this change as a backward compatibility issue.

The command should either be renamed to “unwind your whole database” or this change should be considered a behavioral correction.

[dhui]

I agree that the CLI leaves a lot to be desired.

The rationale for the current behavior:
down is the analog to up. When migrate up is run without any arguments, it runs every migration that has not been applied to the DB yet. So if no migrations have been run yet, then all migrations will be run. Semantically, migrate down should behave the same way migrate up does but in the other direction. Obviously this is a huge foot-gun. Also, the migrate CLI is a pretty thin wrapper around migrate the Go library.

One use-case for using migration up and migrate down to run all migrations is for test data setup and teardown. That being said, if you're using Go, tests should be using the migrate library directly. The main use-cases for the migrate CLI is for:

1. testing migrations (shouldn't be done in shared qa/integration or prod environments)
2. manually fixing a bad migration e.g. an automated run of your migrations broke
3. test data setup/teardown for another language or ecosystem

I'd suggest maturing your operational processes if you're regularly using the migrate CLI in prod or in shared dev/qa/integration environments.

However, given the issues that the current behavior of the migrate down CLI command has caused in the last year, I think it makes sense to make a backwards incompatible change to make the command safer to run. e.g. add a confirmation prompt to migrate down with args to override the prompt
More explicitly:

1. migrate down when run without any args would confirm that the user wants to run all down migrations.
2. migrate down --all will run all down migrations without a confirmation i.e. mirror the current behavior
3. The behavior of Migrate.Down() will not change.

I'll review and merge a PR that makes the changes describe above.
Thanks everyone for your patience and for improving migrate!

[brblck]

I'd suggest maturing your operational processes if you're regularly using the migrate CLI in prod or in shared dev/qa/integration environments.

We did. 🤷‍♂️

We re-wrote your CLI the very second we saw the behavior, long before we allowed anyone to run it in a production or pre-release environments.

I guess what we’re advocating for here is that this library and its CLI also take a moment to “mature” with its user’s experience and feedback to avoid unsafe behavior. That was the whole purpose in reporting this.

Experience has taught me tools with behavior like this are just wrecking balls for higher environments. Command symmetry is a pretty poor rationale for unsafe behavior, but I’m glad you’ve come around.

If no one beats me to it, myself or someone on my team will send you a PR shortly.

[KlotzAndrew]

something like this? https://github.com/golang-migrate/migrate/pull/236

[dhui]

Thanks @KlotzAndrew for implementing the changes listed here: https://github.com/golang-migrate/migrate/issues/29#issuecomment-475903643!

The changes are now in the master branch and will go out with the next release.

[dhui]

Fixed in v4.5.0