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

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:
 * ``<app_label>``: The specified app has its migrations run, up to the most
   recent migration. This may involve running other apps' migrations too, due
   to dependencies.
-* ``<app_label> <migrationname>``: 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.
+* ``<app_label> <migrationname>``: 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)