Schema migrations can inadvertently destroy data

[no]

I attached a sample project that will show that migrations can destroy data.

There are two apps, A and B, both of which have two migrations. Both of app Bs migrations depend on A's first migration. Both of app A's migrations are empty noops. Migrating B first should apply migrations A.0001, B.0001, B.0002, which will leave the database with the B.B table creating via B.0001.

If you then explicitly migrate A.0001 again, it will unapply B.0001 and B.0002 which removes the B.B table causing data loss. A.0001 should be a noop since it's already been applied.

Interestingly, this bug only occurs if A.0002 exists.

Here are the steps to reproduce:

$ unzip destructive-example.zip
$ cd foo
$ sqlite3 db.sqlite3 ".tables";
django_migrations
# Create the "B" table. Both, b migrations depend on a.0001
$ ./manage.py migrate b
Operations to perform:
  Apply all migrations: b
Running migrations:
  Applying b.0001_initial... OK
  Applying b.0002_b... OK
$ sqlite3 db.sqlite3 ".tables";
b_b                django_migrations
$ ./manage.py migrate a 0001
Operations to perform:
  Target specific migration: 0001_initial, from a
Running migrations:
  Unapplying b.0002_b... OK
  Unapplying b.0001_initial... OK
$ sqlite3 db.sqlite3 ".tables";
django_migrations

[no]

Here a more relevant use-case. I'm moving my project to django 1.7 and already have data, so I create the initial migrations, then fake them. Next I migrate the first migration of another app (maybe by accident I forget the --fake), and voila, my data is gone.

$ unzip unapp.zip 
$ cd unapp/
$ sqlite3 db.sqlite3 ".tables"
B_b
$ manage migrate --fake B
Operations to perform:
  Apply all migrations: B
Running migrations:
  Applying A.0001_initial... FAKED
  Applying B.0001_initial... FAKED
  Applying B.0002_b... FAKED
$ sqlite3 db.sqlite3 ".tables"
B_b                django_migrations
$ manage migrate A  0001
Operations to perform:
  Target specific migration: 0001_initial, from A
Running migrations:
  Unapplying B.0002_b... OK
  Unapplying B.0001_initial... OK
$ sqlite3 db.sqlite3 ".tables"
django_migrations

[valtron]

The bug is in ​https://github.com/django/django/blob/44185591385ac278b66e2007208c8656f323f9b6/django/db/migrations/executor.py#L39-L43.

The code should be (I think)

next_migration = get_next_migration_in_same_app(target)
# If `target` is the most recent one in its app, there is nothing to do.
if next_migration:
    backwards_plan = self.loader.graph.backwards_plan(next_migration)

The reasoning: forwards migrations to app.000X are inclusive; backwards migrations to app.000X are exclusive (app.000X isn't actually unapplied). The not-unapplying of app.000X is currently handled by the [:-1] in line 39; however, the dependents of app.000X are still unapplied.

I'll submit a pull request as soon as I figure out how to implement get_next_migration_in_same_app.

[Tim Graham]

Left some minor comments for improvement on the PR.

[Tim Graham <timograham@…>]

In abcf28a07695a45cb5fb15b81bffc97bea5e0be3:

Fixed #23474 -- Prevented migrating backwards from unapplying the wrong migrations.

[Tim Graham <timograham@…>]

In 563eaf04998da3ef88de4553f54eccd900a12805:

[1.7.x] Fixed #23474 -- Prevented migrating backwards from unapplying the wrong migrations.

Backport of abcf28a076 from master

[Tim Graham]

We might have to revert this change as apparently the current behavior is considered correct (see #23410). Maybe a solution to prevent inadvertent data loss would be to add a confirmation step that shows what migrations will be unapplied (see #23359 for a general display mechanism).

[no]

My understanding of the PR was that it stopped the unapplication of migrations that weren't applied to begin with; rolling back a no-op should still be a no-op.

The issue I was facing was that m.0001 is already applied (and along with it some other that it depends on) then I try to migrate it again. Since it's already applied, the operation shouldn't touch the database, but instead it rolls back its dependencies.

Perhaps @andrewgodwin can shed some light on this since he wrote it -> is the outcome of the use case I provided in my second comment the expected behaviour?

[Anssi Kääriäinen]

I think the problem is that the user never asked for backwards migration. Having "./manage.py migrate a 0001" unapply dependencies is a dangerous API. Either add --backwards flag to migrate or at least ask the user if they want to unapply the dependent migrations.

Also, I think --noinput should *not* assume that the user wants to unapply dependencies. Unapply should only happen if the user explicitly tells Django to do so. For this reason the --backwards flag seems better - it is much harder to unapply migrations accidently, and there is no problem for --noinput interpretation (what if the user wants to unapply dependencies, and wants to use --noinput?)

[Carl Meyer]

I think akaariai's idea of requiring more explicit intent to rollback migrations is worth consideration, but entirely apart from that it still seems to me that the previous behaviour was bad and this PR made it better.

In #23410 I see that Andrew stated that the previous behavior was intentional, to wit: "the contract is that backwards migrations roll back to just after the application of the named migration, and all its dependencies are unapplied."

I don't see any clear reasoning provided there as to _why_ that contract is more desirable than the alternatives. Based on the following paragraph, it _seems_ that the reasoning is that a "graph node specification" such as "appname 0001" should always precisely specify a single state, regardless of whether you are rolling backwards or forwards to that state.

I don't find that convincing. Although I see the purity argument, I think that in practice a much more useful (and intuitive, and less likely to result in data loss) contract is that "migrate appname 0001" will ensure that appname-0001 is applied and appname-0002 is not applied, and will otherwise apply or unapply as few migrations as possible.

I also don't see a backwards-compatibility issue with switching to that contract in 1.7.1. This is code that runs interactively and tells you exactly what it is doing as it is doing it.

[Markus Holtermann]

With the patch [abcf28a076] being applied I cannot rollback to a previous migration if. Furthermore the detection of the "next_migration" is wrong and depends on a leading number in the migration name which is not required. Dependencies are defined explicitly and not implicit by their name.

[Carl Meyer]

After discussion with MarkusH and looking at the patch here more closely, I agree with him that this particular implementation is problematic and should be rolled back.

I still favor a change in behavior as described in my comment above, but it needs a better implementation. That discussion can happen on the mailing list and/or #23410.

[Tim Graham <timograham@…>]

In d7ab2cefb7ec94bf45d37a02c79b6703ea2880e5:

Revert "Fixed #23474 -- Prevented migrating backwards from unapplying the wrong migrations."

This reverts commit abcf28a07695a45cb5fb15b81bffc97bea5e0be3.

[Tim Graham <timograham@…>]

In d7b32d31205834262758ca5f48e5fe3a453f7ad3:

[1.7.x] Revert "Fixed #23474 -- Prevented migrating backwards from unapplying the wrong migrations."

Backport of d7ab2cefb7 from master