Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#23474 closed Bug (duplicate)

Schema migrations can inadvertently destroy data

Reported by: no Owned by: nobody
Component: Migrations Version: 1.7
Severity: Release blocker Keywords:
Cc: vtiriac@…, valtron, info+coding@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Attachments (2)

destructive-example.zip (9.9 KB ) - added by no 10 years ago.
unapp.zip (7.7 KB ) - added by no 10 years ago.

Download all attachments as: .zip

Change History (18)

by no, 10 years ago

Attachment: destructive-example.zip added

comment:1 by no, 10 years ago

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

by no, 10 years ago

Attachment: unapp.zip added

comment:2 by valtron, 10 years ago

The bug is in https://github.com/django/django/blob/master/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.

Version 0, edited 10 years ago by valtron (next)

comment:3 by valtron, 10 years ago

Cc: valtron added
Has patch: set
Type: UncategorizedBug

comment:4 by Tim Graham, 10 years ago

Patch needs improvement: set
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Left some minor comments for improvement on the PR.

comment:5 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In abcf28a07695a45cb5fb15b81bffc97bea5e0be3:

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

comment:6 by Tim Graham <timograham@…>, 10 years ago

In 563eaf04998da3ef88de4553f54eccd900a12805:

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

Backport of abcf28a076 from master

comment:7 by Tim Graham, 10 years ago

Has patch: unset
Patch needs improvement: unset
Resolution: fixed
Status: closednew

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).

comment:8 by Markus Holtermann, 10 years ago

Cc: info+coding@… added

comment:9 by no, 10 years ago

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?

comment:10 by Anssi Kääriäinen, 10 years ago

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?)

comment:11 by Carl Meyer, 10 years ago

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.

comment:12 by Markus Holtermann, 10 years ago

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.

comment:13 by Carl Meyer, 10 years ago

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.

comment:14 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In d7ab2cefb7ec94bf45d37a02c79b6703ea2880e5:

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

This reverts commit abcf28a07695a45cb5fb15b81bffc97bea5e0be3.

comment:15 by Tim Graham <timograham@…>, 10 years ago

In d7b32d31205834262758ca5f48e5fe3a453f7ad3:

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

Backport of d7ab2cefb7 from master

comment:16 by Tim Graham, 10 years ago

Resolution: fixedduplicate
Note: See TracTickets for help on using tickets.
Back to Top