From 85086c815873c4cf0bc2f1f2809119088cbe55f1 Mon Sep 17 00:00:00 2001 From: Markus Holtermann Date: Fri, 26 Sep 2014 00:24:17 +0200 Subject: [PATCH] Fixed #23556 -- Raised a more meaningful error message when migrations refer to an unavailable node --- django/db/migrations/graph.py | 34 ++++-- django/db/migrations/loader.py | 62 ++++++++++- tests/migrations/test_graph.py | 10 +- tests/migrations/test_loader.py | 104 ++++++++++++++++++ .../1_auto.py | 11 ++ .../2_auto.py | 13 +++ .../3_auto.py | 13 +++ .../3_squashed_5.py | 19 ++++ .../4_auto.py | 13 +++ .../5_auto.py | 13 +++ .../6_auto.py | 13 +++ .../7_auto.py | 13 +++ .../__init__.py | 0 .../1_auto.py | 11 ++ .../2_auto.py | 13 +++ .../3_squashed_5.py | 19 ++++ .../6_auto.py | 13 +++ .../7_auto.py | 13 +++ .../__init__.py | 0 19 files changed, 369 insertions(+), 18 deletions(-) create mode 100644 tests/migrations/test_migrations_squashed_complex/1_auto.py create mode 100644 tests/migrations/test_migrations_squashed_complex/2_auto.py create mode 100644 tests/migrations/test_migrations_squashed_complex/3_auto.py create mode 100644 tests/migrations/test_migrations_squashed_complex/3_squashed_5.py create mode 100644 tests/migrations/test_migrations_squashed_complex/4_auto.py create mode 100644 tests/migrations/test_migrations_squashed_complex/5_auto.py create mode 100644 tests/migrations/test_migrations_squashed_complex/6_auto.py create mode 100644 tests/migrations/test_migrations_squashed_complex/7_auto.py create mode 100644 tests/migrations/test_migrations_squashed_complex/__init__.py create mode 100644 tests/migrations/test_migrations_squashed_erroneous/1_auto.py create mode 100644 tests/migrations/test_migrations_squashed_erroneous/2_auto.py create mode 100644 tests/migrations/test_migrations_squashed_erroneous/3_squashed_5.py create mode 100644 tests/migrations/test_migrations_squashed_erroneous/6_auto.py create mode 100644 tests/migrations/test_migrations_squashed_erroneous/7_auto.py create mode 100644 tests/migrations/test_migrations_squashed_erroneous/__init__.py diff --git a/django/db/migrations/graph.py b/django/db/migrations/graph.py index 56b78d90567..23808e2af15 100644 --- a/django/db/migrations/graph.py +++ b/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( - "Migration %s dependencies reference nonexistent child node %r" % (migration, child) + raise NodeNotFoundError( + "Migration %s dependencies reference nonexistent child node %r" % (migration, child), + child ) if parent not in self.nodes: - raise KeyError( - "Migration %s dependencies reference nonexistent parent node %r" % (migration, parent) + raise NodeNotFoundError( + "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 diff --git a/django/db/migrations/loader.py b/django/db/migrations/loader.py index 408649847b9..245879f83ba 100644 --- a/django/db/migrations/loader.py +++ b/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 - self.graph.add_dependency(migration, key, parent) + 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: - self.graph.add_dependency(migration, key, parent) + 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: - self.graph.add_dependency(migration, child, key) + 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): """ diff --git a/tests/migrations/test_graph.py b/tests/migrations/test_graph.py index 3fcdd12667d..e70481a4625 100644 --- a/tests/migrations/test_graph.py +++ b/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")) diff --git a/tests/migrations/test_loader.py b/tests/migrations/test_loader.py index 676c843e9d6..a9213215251 100644 --- a/tests/migrations/test_loader.py +++ b/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() diff --git a/tests/migrations/test_migrations_squashed_complex/1_auto.py b/tests/migrations/test_migrations_squashed_complex/1_auto.py new file mode 100644 index 00000000000..25edcfa2901 --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_complex/2_auto.py b/tests/migrations/test_migrations_squashed_complex/2_auto.py new file mode 100644 index 00000000000..9107c358680 --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_complex/3_auto.py b/tests/migrations/test_migrations_squashed_complex/3_auto.py new file mode 100644 index 00000000000..98e679fc12c --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_complex/3_squashed_5.py b/tests/migrations/test_migrations_squashed_complex/3_squashed_5.py new file mode 100644 index 00000000000..ac1bcdee6d1 --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_complex/4_auto.py b/tests/migrations/test_migrations_squashed_complex/4_auto.py new file mode 100644 index 00000000000..8fab746542f --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_complex/5_auto.py b/tests/migrations/test_migrations_squashed_complex/5_auto.py new file mode 100644 index 00000000000..5dda63aae5b --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_complex/6_auto.py b/tests/migrations/test_migrations_squashed_complex/6_auto.py new file mode 100644 index 00000000000..5ca558bc3b3 --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_complex/7_auto.py b/tests/migrations/test_migrations_squashed_complex/7_auto.py new file mode 100644 index 00000000000..e9f5533364a --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_complex/__init__.py b/tests/migrations/test_migrations_squashed_complex/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/migrations/test_migrations_squashed_erroneous/1_auto.py b/tests/migrations/test_migrations_squashed_erroneous/1_auto.py new file mode 100644 index 00000000000..25edcfa2901 --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_erroneous/2_auto.py b/tests/migrations/test_migrations_squashed_erroneous/2_auto.py new file mode 100644 index 00000000000..9107c358680 --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_erroneous/3_squashed_5.py b/tests/migrations/test_migrations_squashed_erroneous/3_squashed_5.py new file mode 100644 index 00000000000..ac1bcdee6d1 --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_erroneous/6_auto.py b/tests/migrations/test_migrations_squashed_erroneous/6_auto.py new file mode 100644 index 00000000000..5ca558bc3b3 --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_erroneous/7_auto.py b/tests/migrations/test_migrations_squashed_erroneous/7_auto.py new file mode 100644 index 00000000000..e9f5533364a --- /dev/null +++ b/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) + ] diff --git a/tests/migrations/test_migrations_squashed_erroneous/__init__.py b/tests/migrations/test_migrations_squashed_erroneous/__init__.py new file mode 100644 index 00000000000..e69de29bb2d