Merge pull request #3280 from Markush2010/ticket23556

Fixed #23556 -- Raise a more meaningful error message when migrations refer to an unavailable node

django/db/migrations/graph.py

@@ -1,7 +1,7 @@
 from __future__ import unicode_literals
 
-from django.utils.datastructures import OrderedSet
 from django.db.migrations.state import ProjectState
+from django.utils.datastructures import OrderedSet
 
 
 class MigrationGraph(object):
@@ -37,12 +37,14 @@ class MigrationGraph(object):
 
     def add_dependency(self, migration, child, parent):
         if child not in self.nodes:
-            raise KeyError(
+            raise NodeNotFoundError(
-                "Migration %s dependencies reference nonexistent child node %r" % (migration, child)
+                "Migration %s dependencies reference nonexistent child node %r" % (migration, child),
+                child
             )
         if parent not in self.nodes:
-            raise KeyError(
+            raise NodeNotFoundError(
-                "Migration %s dependencies reference nonexistent parent node %r" % (migration, parent)
+                "Migration %s dependencies reference nonexistent parent node %r" % (migration, parent),
+                parent
             )
         self.dependencies.setdefault(child, set()).add(parent)
         self.dependents.setdefault(parent, set()).add(child)
@@ -55,7 +57,7 @@ class MigrationGraph(object):
         a database.
         """
         if node not in self.nodes:
-            raise ValueError("Node %r not a valid node" % (node, ))
+            raise NodeNotFoundError("Node %r not a valid node" % (node, ), node)
         return self.dfs(node, lambda x: self.dependencies.get(x, set()))
 
     def backwards_plan(self, node):
@@ -66,7 +68,7 @@ class MigrationGraph(object):
         a database.
         """
         if node not in self.nodes:
-            raise ValueError("Node %r not a valid node" % (node, ))
+            raise NodeNotFoundError("Node %r not a valid node" % (node, ), node)
         return self.dfs(node, lambda x: self.dependents.get(x, set()))
 
     def root_nodes(self, app=None):
@@ -160,3 +162,21 @@ class CircularDependencyError(Exception):
     Raised when there's an impossible-to-resolve circular dependency.
     """
     pass
+
+
+class NodeNotFoundError(LookupError):
+    """
+    Raised when an attempt on a node is made that is not available in the graph.
+    """
+
+    def __init__(self, message, node):
+        self.message = message
+        self.node = node
+
+    def __str__(self):
+        return self.message
+
+    __unicode__ = __str__
+
+    def __repr__(self):
+        return "NodeNotFoundError(%r)" % self.node

django/db/migrations/loader.py

@@ -6,7 +6,7 @@ import sys
 
 from django.apps import apps
 from django.db.migrations.recorder import MigrationRecorder
-from django.db.migrations.graph import MigrationGraph
+from django.db.migrations.graph import MigrationGraph, NodeNotFoundError
 from django.utils import six
 from django.conf import settings
 
@@ -118,7 +118,7 @@ class MigrationLoader(object):
                 self.unmigrated_apps.add(app_config.label)
 
     def get_migration(self, app_label, name_prefix):
-        "Gets the migration exactly named, or raises KeyError"
+        "Gets the migration exactly named, or raises `graph.NodeNotFoundError`"
         return self.graph.nodes[app_label, name_prefix]
 
     def get_migration_by_prefix(self, app_label, name_prefix):
@@ -156,7 +156,7 @@ class MigrationLoader(object):
             try:
                 if key[1] == "__first__":
                     return list(self.graph.root_nodes(key[0]))[0]
-                else:
+                else:  # "__latest__"
                     return list(self.graph.leaf_nodes(key[0]))[0]
             except IndexError:
                 if self.ignore_no_migrations:
@@ -194,6 +194,12 @@ class MigrationLoader(object):
         for key, migration in normal.items():
             for parent in migration.dependencies:
                 reverse_dependencies.setdefault(parent, set()).add(key)
+        # Remeber the possible replacements to generate more meaningful error
+        # messages
+        reverse_replacements = {}
+        for key, migration in replacing.items():
+            for replaced in migration.replaces:
+                reverse_replacements.setdefault(replaced, set()).add(key)
         # Carry out replacements if we can - that is, if all replaced migrations
         # are either unapplied or missing.
         for key, migration in replacing.items():
@@ -225,6 +231,32 @@ class MigrationLoader(object):
         self.graph = MigrationGraph()
         for key, migration in normal.items():
             self.graph.add_node(key, migration)
+
+        def _reraise_missing_dependency(migration, missing, exc):
+            """
+            Checks if ``missing`` could have been replaced by any squash
+            migration but wasn't because the the squash migration was partially
+            applied before. In that case raise a more understandable exception.
+
+            #23556
+            """
+            if missing in reverse_replacements:
+                candidates = reverse_replacements.get(missing, set())
+                is_replaced = any(candidate in self.graph.nodes for candidate in candidates)
+                if not is_replaced:
+                    tries = ', '.join('%s.%s' % c for c in candidates)
+                    exc_value = NodeNotFoundError(
+                        "Migration {0} depends on nonexistent node ('{1}', '{2}'). "
+                        "Django tried to replace migration {1}.{2} with any of [{3}] "
+                        "but wasn't able to because some of the replaced migrations "
+                        "are already applied.".format(
+                            migration, missing[0], missing[1], tries
+                        ),
+                        missing)
+                    exc_value.__cause__ = exc
+                    six.reraise(NodeNotFoundError, exc_value, sys.exc_info()[2])
+            raise exc
+
         # Add all internal dependencies first to ensure __first__ dependencies
         # find the correct root node.
         for key, migration in normal.items():
@@ -232,7 +264,15 @@ class MigrationLoader(object):
                 if parent[0] != key[0] or parent[1] == '__first__':
                     # Ignore __first__ references to the same app (#22325)
                     continue
+                try:
                     self.graph.add_dependency(migration, key, parent)
+                except NodeNotFoundError as e:
+                    # Since we added "key" to the nodes before this implies
+                    # "parent" is not in there. To make the raised exception
+                    # more understandable we check if parent could have been
+                    # replaced but hasn't (eg partially applied squashed
+                    # migration)
+                    _reraise_missing_dependency(migration, parent, e)
         for key, migration in normal.items():
             for parent in migration.dependencies:
                 if parent[0] == key[0]:
@@ -240,11 +280,21 @@ class MigrationLoader(object):
                     continue
                 parent = self.check_key(parent, key[0])
                 if parent is not None:
+                    try:
                         self.graph.add_dependency(migration, key, parent)
+                    except NodeNotFoundError as e:
+                        # Since we added "key" to the nodes before this implies
+                        # "parent" is not in there.
+                        _reraise_missing_dependency(migration, parent, e)
             for child in migration.run_before:
                 child = self.check_key(child, key[0])
                 if child is not None:
+                    try:
                         self.graph.add_dependency(migration, child, key)
+                    except NodeNotFoundError as e:
+                        # Since we added "key" to the nodes before this implies
+                        # "child" is not in there.
+                        _reraise_missing_dependency(migration, child, e)
 
     def detect_conflicts(self):
         """

tests/migrations/test_graph.py

@@ -1,5 +1,5 @@
 from django.test import TestCase
-from django.db.migrations.graph import MigrationGraph, CircularDependencyError
+from django.db.migrations.graph import CircularDependencyError, MigrationGraph, NodeNotFoundError
 
 
 class GraphTests(TestCase):
@@ -156,10 +156,10 @@ class GraphTests(TestCase):
         graph = MigrationGraph()
         message = "Node ('app_b', '0001') not a valid node"
 
-        with self.assertRaisesMessage(ValueError, message):
+        with self.assertRaisesMessage(NodeNotFoundError, message):
             graph.forwards_plan(("app_b", "0001"))
 
-        with self.assertRaisesMessage(ValueError, message):
+        with self.assertRaisesMessage(NodeNotFoundError, message):
             graph.backwards_plan(("app_b", "0001"))
 
     def test_missing_parent_nodes(self):
@@ -175,7 +175,7 @@ class GraphTests(TestCase):
         graph.add_dependency("app_a.0003", ("app_a", "0003"), ("app_a", "0002"))
         graph.add_dependency("app_a.0002", ("app_a", "0002"), ("app_a", "0001"))
         msg = "Migration app_a.0001 dependencies reference nonexistent parent node ('app_b', '0002')"
-        with self.assertRaisesMessage(KeyError, msg):
+        with self.assertRaisesMessage(NodeNotFoundError, msg):
             graph.add_dependency("app_a.0001", ("app_a", "0001"), ("app_b", "0002"))
 
     def test_missing_child_nodes(self):
@@ -186,5 +186,5 @@ class GraphTests(TestCase):
         graph = MigrationGraph()
         graph.add_node(("app_a", "0001"), None)
         msg = "Migration app_a.0002 dependencies reference nonexistent child node ('app_a', '0002')"
-        with self.assertRaisesMessage(KeyError, msg):
+        with self.assertRaisesMessage(NodeNotFoundError, msg):
             graph.add_dependency("app_a.0002", ("app_a", "0002"), ("app_a", "0001"))

tests/migrations/test_loader.py

@@ -1,7 +1,10 @@
+from __future__ import unicode_literals
+
 from unittest import skipIf
 
 from django.test import TestCase, override_settings
 from django.db import connection, connections
+from django.db.migrations.graph import NodeNotFoundError
 from django.db.migrations.loader import MigrationLoader, AmbiguityError
 from django.db.migrations.recorder import MigrationRecorder
 from django.test import modify_settings
@@ -187,3 +190,104 @@ class LoaderTests(TestCase):
             2,
         )
         recorder.flush()
+
+    @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations_squashed_complex"})
+    def test_loading_squashed_complex(self):
+        "Tests loading a complex set of squashed migrations"
+
+        loader = MigrationLoader(connection)
+        recorder = MigrationRecorder(connection)
+
+        def num_nodes():
+            plan = set(loader.graph.forwards_plan(('migrations', '7_auto')))
+            return len(plan - loader.applied_migrations)
+
+        # Empty database: use squashed migration
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 5)
+
+        # Starting at 1 or 2 should use the squashed migration too
+        recorder.record_applied("migrations", "1_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 4)
+
+        recorder.record_applied("migrations", "2_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 3)
+
+        # However, starting at 3 to 5 cannot use the squashed migration
+        recorder.record_applied("migrations", "3_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 4)
+
+        recorder.record_applied("migrations", "4_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 3)
+
+        # Starting at 5 to 7 we are passed the squashed migrations
+        recorder.record_applied("migrations", "5_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 2)
+
+        recorder.record_applied("migrations", "6_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 1)
+
+        recorder.record_applied("migrations", "7_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 0)
+
+        recorder.flush()
+
+    @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations_squashed_erroneous"})
+    def test_loading_squashed_erroneous(self):
+        "Tests loading a complex but erroneous set of squashed migrations"
+
+        loader = MigrationLoader(connection)
+        recorder = MigrationRecorder(connection)
+
+        def num_nodes():
+            plan = set(loader.graph.forwards_plan(('migrations', '7_auto')))
+            return len(plan - loader.applied_migrations)
+
+        # Empty database: use squashed migration
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 5)
+
+        # Starting at 1 or 2 should use the squashed migration too
+        recorder.record_applied("migrations", "1_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 4)
+
+        recorder.record_applied("migrations", "2_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 3)
+
+        # However, starting at 3 or 4 we'd need to use non-existing migrations
+        msg = ("Migration migrations.6_auto depends on nonexistent node ('migrations', '5_auto'). "
+               "Django tried to replace migration migrations.5_auto with any of "
+               "[migrations.3_squashed_5] but wasn't able to because some of the replaced "
+               "migrations are already applied.")
+
+        recorder.record_applied("migrations", "3_auto")
+        with self.assertRaisesMessage(NodeNotFoundError, msg):
+            loader.build_graph()
+
+        recorder.record_applied("migrations", "4_auto")
+        with self.assertRaisesMessage(NodeNotFoundError, msg):
+            loader.build_graph()
+
+        # Starting at 5 to 7 we are passed the squashed migrations
+        recorder.record_applied("migrations", "5_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 2)
+
+        recorder.record_applied("migrations", "6_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 1)
+
+        recorder.record_applied("migrations", "7_auto")
+        loader.build_graph()
+        self.assertEqual(num_nodes(), 0)
+
+        recorder.flush()

tests/migrations/test_migrations_squashed_complex/1_auto.py

@@ -0,0 +1,11 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_complex/2_auto.py

@@ -0,0 +1,13 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("migrations", "1_auto")]
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_complex/3_auto.py

@@ -0,0 +1,13 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("migrations", "2_auto")]
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_complex/3_squashed_5.py

@@ -0,0 +1,19 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    replaces = [
+        ("migrations", "3_auto"),
+        ("migrations", "4_auto"),
+        ("migrations", "5_auto"),
+    ]
+
+    dependencies = [("migrations", "2_auto")]
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_complex/4_auto.py

@@ -0,0 +1,13 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("migrations", "3_auto")]
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_complex/5_auto.py

@@ -0,0 +1,13 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("migrations", "4_auto")]
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_complex/6_auto.py

@@ -0,0 +1,13 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("migrations", "5_auto")]
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_complex/7_auto.py

@@ -0,0 +1,13 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("migrations", "6_auto")]
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_erroneous/1_auto.py

@@ -0,0 +1,11 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_erroneous/2_auto.py

@@ -0,0 +1,13 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("migrations", "1_auto")]
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_erroneous/3_squashed_5.py

@@ -0,0 +1,19 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    replaces = [
+        ("migrations", "3_auto"),
+        ("migrations", "4_auto"),
+        ("migrations", "5_auto"),
+    ]
+
+    dependencies = [("migrations", "2_auto")]
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_erroneous/6_auto.py

@@ -0,0 +1,13 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("migrations", "5_auto")]
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]

tests/migrations/test_migrations_squashed_erroneous/7_auto.py

@@ -0,0 +1,13 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [("migrations", "6_auto")]
+
+    operations = [
+        migrations.RunPython(lambda apps, schema_editor: None)
+    ]