From ab2819aa7b09d36d9ff24830a9825aa52b87fdb4 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Mon, 17 Nov 2014 10:13:47 -0700 Subject: [PATCH] Fixed #23410 -- Avoided unnecessary rollbacks in related apps when migrating backwards. --- django/db/migrations/executor.py | 17 ++++-- docs/ref/django-admin.txt | 9 +-- docs/releases/1.7.2.txt | 3 + tests/migrations/test_executor.py | 96 ++++++++++++++++++++++++++++++- 4 files changed, 114 insertions(+), 11 deletions(-) diff --git a/django/db/migrations/executor.py b/django/db/migrations/executor.py index 32aa82b82c..f5811bb795 100644 --- a/django/db/migrations/executor.py +++ b/django/db/migrations/executor.py @@ -36,12 +36,17 @@ class MigrationExecutor(object): # If the migration is already applied, do backwards mode, # otherwise do forwards mode. elif target in applied: - backwards_plan = self.loader.graph.backwards_plan(target)[:-1] - # We only do this if the migration is not the most recent one - # in its app - that is, another migration with the same app - # label is in the backwards plan - if any(node[0] == target[0] for node in backwards_plan): - for migration in backwards_plan: + # Don't migrate backwards all the way to the target node (that + # may roll back dependencies in other apps that don't need to + # be rolled back); instead roll back through target's immediate + # child(ren) in the same app, and no further. + next_in_app = sorted( + n for n in + self.loader.graph.dependents.get(target, set()) + if n[0] == target[0] + ) + for node in next_in_app: + for migration in self.loader.graph.backwards_plan(node): if migration in applied: plan.append((self.loader.graph.nodes[migration], True)) applied.remove(migration) diff --git a/docs/ref/django-admin.txt b/docs/ref/django-admin.txt index 66c721574b..b0393e35c2 100644 --- a/docs/ref/django-admin.txt +++ b/docs/ref/django-admin.txt @@ -737,10 +737,11 @@ The behavior of this command changes depending on the arguments provided: * ````: The specified app has its migrations run, up to the most recent migration. This may involve running other apps' migrations too, due to dependencies. -* `` ``: Brings the database schema to a state where it - would have just run the given migration, but no further - this may involve - unapplying migrations if you have previously migrated past the named - migration. Use the name ``zero`` to unapply all migrations for an app. +* `` ``: Brings the database schema to a state where + the named migration is applied, but no later migrations in the same app are + applied. This may involve unapplying migrations if you have previously + migrated past the named migration. Use the name ``zero`` to unapply all + migrations for an app. Unlike ``syncdb``, this command does not prompt you to create a superuser if one doesn't exist (assuming you are using :mod:`django.contrib.auth`). Use diff --git a/docs/releases/1.7.2.txt b/docs/releases/1.7.2.txt index 4955f30946..8af832d1a5 100644 --- a/docs/releases/1.7.2.txt +++ b/docs/releases/1.7.2.txt @@ -68,3 +68,6 @@ Bugfixes * Made :class:`~django.db.migrations.operations.RenameModel` reversible (:ticket:`22248`) + +* Avoided unnecessary rollbacks of migrations from other apps when migrating + backwards (:ticket:`23410`). diff --git a/tests/migrations/test_executor.py b/tests/migrations/test_executor.py index b8eddc6f97..61da17b341 100644 --- a/tests/migrations/test_executor.py +++ b/tests/migrations/test_executor.py @@ -1,6 +1,7 @@ from django.db import connection from django.db.migrations.executor import MigrationExecutor -from django.test import modify_settings, override_settings +from django.db.migrations.graph import MigrationGraph +from django.test import modify_settings, override_settings, TestCase from django.apps.registry import apps as global_apps from .test_base import MigrationTestBase @@ -231,3 +232,96 @@ class ExecutorTests(MigrationTestBase): executor.migrate([("migrations", None)]) self.assertTableNotExists("migrations_author") self.assertTableNotExists("migrations_tribble") + + +class FakeLoader(object): + def __init__(self, graph, applied): + self.graph = graph + self.applied_migrations = applied + + +class FakeMigration(object): + """Really all we need is any object with a debug-useful repr.""" + def __init__(self, name): + self.name = name + + def __repr__(self): + return 'M<%s>' % self.name + + +class ExecutorUnitTests(TestCase): + """(More) isolated unit tests for executor methods.""" + def test_minimize_rollbacks(self): + """ + Minimize unnecessary rollbacks in connected apps. + + When you say "./manage.py migrate appA 0001", rather than migrating to + just after appA-0001 in the linearized migration plan (which could roll + back migrations in other apps that depend on appA 0001, but don't need + to be rolled back since we're not rolling back appA 0001), we migrate + to just before appA-0002. + """ + a1_impl = FakeMigration('a1') + a1 = ('a', '1') + a2_impl = FakeMigration('a2') + a2 = ('a', '2') + b1_impl = FakeMigration('b1') + b1 = ('b', '1') + graph = MigrationGraph() + graph.add_node(a1, a1_impl) + graph.add_node(a2, a2_impl) + graph.add_node(b1, b1_impl) + graph.add_dependency(None, b1, a1) + graph.add_dependency(None, a2, a1) + + executor = MigrationExecutor(None) + executor.loader = FakeLoader(graph, {a1, b1, a2}) + + plan = executor.migration_plan({a1}) + + self.assertEqual(plan, [(a2_impl, True)]) + + def test_minimize_rollbacks_branchy(self): + """ + Minimize rollbacks when target has multiple in-app children. + + a: 1 <---- 3 <--\ + \ \- 2 <--- 4 + \ \ + b: \- 1 <--- 2 + """ + a1_impl = FakeMigration('a1') + a1 = ('a', '1') + a2_impl = FakeMigration('a2') + a2 = ('a', '2') + a3_impl = FakeMigration('a3') + a3 = ('a', '3') + a4_impl = FakeMigration('a4') + a4 = ('a', '4') + b1_impl = FakeMigration('b1') + b1 = ('b', '1') + b2_impl = FakeMigration('b2') + b2 = ('b', '2') + graph = MigrationGraph() + graph.add_node(a1, a1_impl) + graph.add_node(a2, a2_impl) + graph.add_node(a3, a3_impl) + graph.add_node(a4, a4_impl) + graph.add_node(b1, b1_impl) + graph.add_node(b2, b2_impl) + graph.add_dependency(None, a2, a1) + graph.add_dependency(None, a3, a1) + graph.add_dependency(None, a4, a2) + graph.add_dependency(None, a4, a3) + graph.add_dependency(None, b2, b1) + graph.add_dependency(None, b1, a1) + graph.add_dependency(None, b2, a2) + + executor = MigrationExecutor(None) + executor.loader = FakeLoader(graph, {a1, b1, a2, b2, a3, a4}) + + plan = executor.migration_plan({a1}) + + should_be_rolled_back = [b2_impl, a4_impl, a2_impl, a3_impl] + exp = [(m, True) for m in should_be_rolled_back] + self.assertEqual(plan, exp)