[Django] #23410: Backward migrations change overall state rather than reverting single migration
Django
migration
-----------------------------+--------------------
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23410>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* needs_better_patch: => 0
* severity: Normal => Release blocker
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:1>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
This is by design AFAIK.
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:2>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by andrewgodwin):
* status: new => closed
* resolution: => invalid
* severity: Release blocker => Normal
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:3>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:4>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
#23474 was a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:5>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Naddiseo):
* cc: github@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:6>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* cc: shaib (added)
* status: closed => new
* resolution: invalid =>
Comment:
Reopening based on #comment:4. Note that the duplicate #23474 was marked a
release-blocker because of the data-loss aspects.
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:7>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: New feature | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* type: Bug => New feature
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:8>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: New feature | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:9>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: New feature | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Naddiseo):
Perhaps it would also be prudent to issue a warning if the migrator
detects that there is potential for data loss?
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:10>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: New feature | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
Replying to [comment:10 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:11>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: New feature | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:12>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: New feature | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carl Meyer <carl@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"ab2819aa7b09d36d9ff24830a9825aa52b87fdb4"]:
{{{
#!CommitTicketReference repository=""
revision="ab2819aa7b09d36d9ff24830a9825aa52b87fdb4"
Fixed #23410 -- Avoided unnecessary rollbacks in related apps when
migrating backwards.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:13>
Django
migration
Reporter: Markush2010 | Owner: nobody
Type: New feature | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carl Meyer <carl@…>):
In [changeset:"03e8c182888e27c7609cbc7705a46ab7b7107f12"]:
{{{
#!CommitTicketReference repository=""
revision="03e8c182888e27c7609cbc7705a46ab7b7107f12"
[1.7.x] Fixed #23410 -- Avoided unnecessary rollbacks in related apps when
migrating backwards.
Backport of ab2819aa7b09d36d9ff24830a9825aa52b87fdb4 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23410#comment:14>