Backward migrations change overall state rather than reverting single migration

[Markus Holtermann]

When rolling back to a previous migration (or more precisely: state), Django reverts all migrations between the current state and the one that point in history looking at the linear migration plan. Taking the example from the tests (​https://github.com/django/django/blob/1a918806ca67e8e4818aeb1c304e4546baab9e4d/tests/migrations/test_graph.py#L47-L50) and the behavior how South handled backwards migrations, this is somehow inconsistent and might even lead to data loss. I'd expect nothing to happen in the example above.

Looking at the more complex example from the tests (​https://github.com/django/django/blob/1a918806ca67e8e4818aeb1c304e4546baab9e4d/tests/migrations/test_graph.py#L101-L104) I wouldn't expect c.0001 being rolled back, instead a.0001, a.0002, b.0001 and c.0001 should still be applied.

[Collin Anderson]

marking as unaccepted release blocker to give this attention, as this could have data loss issues. It may be we simply need to better document the situation.

[Aymeric Augustin]

This is by design AFAIK.

[Andrew Godwin]

The test is exactly correct; the contract is that backwards migrations roll back to just after the application of the named migration, and all its dependencies are unapplied.

The problem here is the way we specify nodes on the graph - the choice of the same namespace and format as forwards migrations is unfortunate, and perhaps instead we should have backwards migrations go to *before* the named migration rather than afterward (so you're asking to end up with the one you named unapplied) - we'd need to have an alternate syntax for this now, though, since we've shipped something as it is.

I'm going to close this as INVALID, as it's performing exactly as designed (though unfortunately different to South). I wouldn't be averse to a new ticket for better docs or perhaps even a second input argument format for "before this migration" rather than "just after", it's the sort of thing that would work well for a sprint.

[Carl Meyer]

Andrew: I understand the contract you described, and I understand that it preserves the invariant that the arguments you pass to "migrate" always specify precisely one database state, regardless of whether you are migrating forwards or backwards. But I think there is a competing value here (reducing the risk of data loss inherent in rolling back migrations that arguably were not requested for rollback), which to me seems a higher priority than the name-refers-to-single-state invariant.

If I were to describe the ideal behavior precisely, it would be this: given a request to migrate to "appname 0001", we pick whatever target spot on the linearized migration plan between "appname 0001" and "appname 0002" results in the fewest possible migrations from other apps requiring migrate/rollback. The only guaranteed invariant would be that "appname 0001" refers to a state where "appname 0001" is applied and "appname 0002" is not, and all dependency constraints are satisfied.

This choice would accept that a name like "appname 0001" is imprecise rather than precise, but would consider that a reasonable tradeoff in exchange for behavior that is (arguably?) more intuitive and less likely to result in data loss.

If you'd rather discuss on the mailing list, I can bring up the question there.

[Carl Meyer]

#23474 was a duplicate.

[Shai Berger]

Reopening based on #comment:4. Note that the duplicate #23474 was marked a release-blocker because of the data-loss aspects.

[Aymeric Augustin]

Yes, let's accept the ticket according to Carl's suggestion in comment 4.

A convention such as migrate appname -0002 to migrate to just before migration 0002 might work.

[Carl Meyer]

Pull request ​https://github.com/django/django/pull/3562 implements the simplest version of this.

When migrating backwards, instead of migrating to "just after the named migration", it migrates to "just before the named migration's immediate children in the same app".

I think this is more likely to be the desired behavior, and runs less risk of unintentional rollbacks. When someone names "appA 0001" they are expressing a desire about which migrations in appA will be applied/unapplied, not about other apps. They are likely to assume that migrations in other apps will be applied / rolled back only inasmuch as needed required to keep dependencies fulfilled. After this PR, that is the case.

Because of the potential for data loss with unintended rollbacks, I think this change should be backported to 1.7.X (and I've updated the 1.7.2 release notes accordingly).

There are two other suggestions made in this thread. One is to introduce a special syntax that means "right before the named migration". I don't think this is necessary. I think the change made here already provides the right intuitive behavior for both forwards and backwards migration.

The other is Anssi's suggestion to require a special flag for all backwards migrations, to even further reduce the likelihood of unintentional rollback. I think this may be a good idea, but should be handled in a separate ticket (and should be a new feature in 1.8, not backported to 1.7).

[Richard Eames]

Perhaps it would also be prudent to issue a warning if the migrator detects that there is potential for data loss?

[Carl Meyer]

Replying to Naddiseo:

Perhaps it would also be prudent to issue a warning if the migrator detects that there is potential for data loss?

I don't think it makes sense to try to have the migrations system classify migrations that way (in the general case, it's impossible -- who knows what happens inside a RunSQL or RunPython migration). Adding a flag that is required for migrating backwards achieves the same benefit with much less complexity.

[Carl Meyer <carl@…>]

In ab2819aa7b09d36d9ff24830a9825aa52b87fdb4:

Fixed #23410 -- Avoided unnecessary rollbacks in related apps when migrating backwards.

[Carl Meyer <carl@…>]

In 03e8c182888e27c7609cbc7705a46ab7b7107f12:

[1.7.x] Fixed #23410 -- Avoided unnecessary rollbacks in related apps when migrating backwards.

Backport of ab2819aa7b09d36d9ff24830a9825aa52b87fdb4 from master.