Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#25850 closed Cleanup/optimization (fixed)

Migrations ignore inconsistent history silently

Reported by: Shai Berger Owned by: nobody
Component: Migrations Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Doing some bad things, it is possible to get to a state where a migration is recorded as executed without all of its dependencies being executed. For example, if we have migrations for the same app developed on separate branches, and the person who merges them decides to resolve the conflict not by adding a "merge migration" as suggested by Django, but by editing the dependencies on migrations which were already run.

One should not do that, of course, but if one does do that, Django should refuse to do anything with these migrations until the situation is resolved. Currently, it just happily ignores the unfulfilled dependencies, and will migrate forwards and even cheerfully create new migrations. We don't even have an InconsistentMigrationHistory exception.

A failing test draft (add at the end of tests/migrations/test_executor.py):

    def test_reject_holes_in_history(self):
        """
        If the history is inconsistent with the sources, cry foul
        a: 1 <--- 2 <--- 3
        If a2 is applied already and a1 is not, and we're asked to migrate to
        a3, then something is very wrong and we should not proceed
        """
        a1_impl = FakeMigration('a1')
        a1 = ('a', '1')
        a2_impl = FakeMigration('a2')
        a2 = ('a', '2')
        a3_impl = FakeMigration('a3')
        a3 = ('a', '3')
        graph = MigrationGraph()
        graph.add_node(a1, a1_impl)
        graph.add_node(a2, a2_impl)
        graph.add_node(a3, a3_impl)
        graph.add_dependency(None, a2, a1)
        graph.add_dependency(None, a3, a2)

        executor = MigrationExecutor(None)
        executor.loader = FakeLoader(graph, {a2})

        with self.assertRaises(Exception):
            plan = executor.migration_plan({a3})

Change History (12)

comment:1 Changed 5 years ago by Markus Holtermann

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

Yeah, as discussed at DutH Django should bail out loudly.

comment:2 Changed 5 years ago by Attila Tovt

Has patch: set
Needs tests: unset

comment:3 Changed 5 years ago by Tim Graham

Patch needs improvement: set

comment:4 Changed 5 years ago by Attila Tovt

I completed the previous fix and opened a new PR

comment:5 Changed 5 years ago by Tim Graham

Patch needs improvement: unset

(Don't forget to uncheck "Patch needs improvement" so the ticket appears in the review queue.)

comment:6 Changed 5 years ago by Tim Graham

Patch needs improvement: set

comment:7 Changed 5 years ago by Attila Tovt

Patch needs improvement: unset

comment:8 Changed 5 years ago by Tim Graham

Patch needs improvement: set

comment:9 Changed 5 years ago by Attila Tovt

Patch needs improvement: unset

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

Resolution: fixed
Status: newclosed

In 02ae5fd:

Fixed #25850 -- Made migrate/makemigrations error on inconsistent history.

comment:11 Changed 4 years ago by Tim Graham <timograham@…>

In c93ac9cf:

Refs #25850, #27142, #27110 -- Documented migration history consistency checks.

comment:12 Changed 4 years ago by Tim Graham <timograham@…>

In 57f7d896:

[1.10.x] Refs #25850, #27142, #27110 -- Documented migration history consistency checks.

Backport of c93ac9cf42bff259ab71b70a89b693b9c38e4666 from master

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