From 21e21c7bc2b8bf7ae127e2aa75048a60d05a6e0f Mon Sep 17 00:00:00 2001 From: Patryk Zawadzki Date: Sat, 15 Nov 2014 20:25:43 +0100 Subject: [PATCH] Fixed #23844 -- Used topological sort for migration operation dependency resolution. This removes the concept of equality between operations to guarantee compatilibity with Python 3. Python 3 requires equality to result in identical object hashes. It's impossible to implement a unique hash that preserves equality as operations such as field creation depend on being able to accept arbitrary dicts that cannot be hashed reliably. Thanks Klaas van Schelven for the original patch in 13d613f80011852404198dfafd1f09c0c0ea42e6. --- AUTHORS | 1 + django/db/migrations/autodetector.py | 36 ++++------ django/db/migrations/operations/base.py | 6 -- django/db/migrations/operations/fields.py | 50 ++++++++----- django/db/migrations/operations/models.py | 70 ++++++++++++++++--- django/db/migrations/operations/special.py | 36 ++++++++++ django/db/migrations/topological_sort.py | 31 ++++++++ .../more_operations.py | 7 ++ .../custom_migration_operations/operations.py | 7 ++ tests/migrations/test_autodetector.py | 6 +- tests/migrations/test_optimizer.py | 35 +--------- 11 files changed, 197 insertions(+), 88 deletions(-) create mode 100644 django/db/migrations/topological_sort.py diff --git a/AUTHORS b/AUTHORS index 949de416f5..dd78347071 100644 --- a/AUTHORS +++ b/AUTHORS @@ -378,6 +378,7 @@ answer newbie questions, and generally made Django that much better: Kevin McConnell Kieran Holland kilian + Klaas van Schelven knox konrad@gwu.edu Kowito Charoenratchatabhan diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index 7369cfad5c..fcc8a3774e 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -14,6 +14,8 @@ from django.db.migrations.questioner import MigrationQuestioner from django.db.migrations.optimizer import MigrationOptimizer from django.db.migrations.operations.models import AlterModelOptions +from .topological_sort import stable_topological_sort + class MigrationAutodetector(object): """ @@ -191,28 +193,18 @@ class MigrationAutodetector(object): # isn't bad, but we need to pull a few things around so FKs work nicely # inside the same app for app_label, ops in sorted(self.generated_operations.items()): - for i in range(10000): - found = False - for i, op in enumerate(ops): - for dep in op._auto_deps: - if dep[0] == app_label: - # Alright, there's a dependency on the same app. - for j, op2 in enumerate(ops): - if j > i and self.check_dependency(op2, dep): - # shift the operation from position i after - # the operation at position j - ops = ops[:i] + ops[i + 1:j + 1] + [op] + ops[j + 1:] - found = True - break - if found: - break - if found: - break - if not found: - break - else: - raise ValueError("Infinite loop caught in operation dependency resolution") - self.generated_operations[app_label] = ops + + # construct a dependency graph for intra-app dependencies + dependency_graph = {op: set() for op in ops} + for op in ops: + for dep in op._auto_deps: + if dep[0] == app_label: + for op2 in ops: + if self.check_dependency(op2, dep): + dependency_graph[op].add(op2) + + # we use a stable sort for deterministic tests & general behavior + self.generated_operations[app_label] = stable_topological_sort(ops, dependency_graph) # Now, we need to chop the lists of operations up into migrations with # dependencies on each other. diff --git a/django/db/migrations/operations/base.py b/django/db/migrations/operations/base.py index 8d2c491add..655f03a4c6 100644 --- a/django/db/migrations/operations/base.py +++ b/django/db/migrations/operations/base.py @@ -116,9 +116,3 @@ class Operation(object): ", ".join(map(repr, self._constructor_args[0])), ",".join(" %s=%r" % x for x in self._constructor_args[1].items()), ) - - def __eq__(self, other): - return (self.__class__ == other.__class__) and (self.deconstruct() == other.deconstruct()) - - def __ne__(self, other): - return not (self == other) diff --git a/django/db/migrations/operations/fields.py b/django/db/migrations/operations/fields.py index afa3fde818..ecfa5d6387 100644 --- a/django/db/migrations/operations/fields.py +++ b/django/db/migrations/operations/fields.py @@ -16,6 +16,16 @@ class AddField(Operation): self.field = field self.preserve_default = preserve_default + def deconstruct(self): + kwargs = {} + if self.preserve_default is not True: + kwargs['preserve_default'] = self.preserve_default + return ( + self.__class__.__name__, + [self.model_name, self.name, self.field], + kwargs + ) + def state_forwards(self, app_label, state): # If preserve default is off, don't use the default for future state if not self.preserve_default: @@ -47,14 +57,6 @@ class AddField(Operation): def describe(self): return "Add field %s to %s" % (self.name, self.model_name) - def __eq__(self, other): - return ( - (self.__class__ == other.__class__) and - (self.name == other.name) and - (self.model_name.lower() == other.model_name.lower()) and - (self.field.deconstruct()[1:] == other.field.deconstruct()[1:]) - ) - def references_model(self, name, app_label=None): return name.lower() == self.model_name.lower() @@ -71,6 +73,13 @@ class RemoveField(Operation): self.model_name = model_name self.name = name + def deconstruct(self): + return ( + self.__class__.__name__, + [self.model_name, self.name], + {} + ) + def state_forwards(self, app_label, state): new_fields = [] for name, instance in state.models[app_label, self.model_name.lower()].fields: @@ -110,6 +119,16 @@ class AlterField(Operation): self.field = field self.preserve_default = preserve_default + def deconstruct(self): + kwargs = {} + if self.preserve_default is not True: + kwargs['preserve_default'] = self.preserve_default + return ( + self.__class__.__name__, + [self.model_name, self.name, self.field], + kwargs + ) + def state_forwards(self, app_label, state): if not self.preserve_default: field = self.field.clone() @@ -146,14 +165,6 @@ class AlterField(Operation): def describe(self): return "Alter field %s on %s" % (self.name, self.model_name) - def __eq__(self, other): - return ( - (self.__class__ == other.__class__) and - (self.name == other.name) and - (self.model_name.lower() == other.model_name.lower()) and - (self.field.deconstruct()[1:] == other.field.deconstruct()[1:]) - ) - def references_model(self, name, app_label=None): return name.lower() == self.model_name.lower() @@ -171,6 +182,13 @@ class RenameField(Operation): self.old_name = old_name self.new_name = new_name + def deconstruct(self): + return ( + self.__class__.__name__, + [self.model_name, self.old_name, self.new_name], + {} + ) + def state_forwards(self, app_label, state): # Rename the field state.models[app_label, self.model_name.lower()].fields = [ diff --git a/django/db/migrations/operations/models.py b/django/db/migrations/operations/models.py index e195d9d841..8672cf4515 100644 --- a/django/db/migrations/operations/models.py +++ b/django/db/migrations/operations/models.py @@ -20,6 +20,18 @@ class CreateModel(Operation): self.options = options or {} self.bases = bases or (models.Model,) + def deconstruct(self): + kwargs = {} + if self.options: + kwargs['options'] = self.options + if self.bases and self.bases != (models.Model,): + kwargs['bases'] = self.bases + return ( + self.__class__.__name__, + [self.name, self.fields], + kwargs + ) + def state_forwards(self, app_label, state): state.models[app_label, self.name.lower()] = ModelState( app_label, @@ -61,15 +73,6 @@ class CreateModel(Operation): return True return False - def __eq__(self, other): - return ( - (self.__class__ == other.__class__) and - (self.name == other.name) and - (self.options == other.options) and - (self.bases == other.bases) and - ([(k, f.deconstruct()[1:]) for k, f in self.fields] == [(k, f.deconstruct()[1:]) for k, f in other.fields]) - ) - class DeleteModel(Operation): """ @@ -79,6 +82,13 @@ class DeleteModel(Operation): def __init__(self, name): self.name = name + def deconstruct(self): + return ( + self.__class__.__name__, + [self.name], + {} + ) + def state_forwards(self, app_label, state): del state.models[app_label, self.name.lower()] @@ -110,6 +120,13 @@ class RenameModel(Operation): self.old_name = old_name self.new_name = new_name + def deconstruct(self): + return ( + self.__class__.__name__, + [self.old_name, self.new_name], + {} + ) + def state_forwards(self, app_label, state): # Get all of the related objects we need to repoint apps = state.render(skip_cache=True) @@ -196,6 +213,13 @@ class AlterModelTable(Operation): self.name = name self.table = table + def deconstruct(self): + return ( + self.__class__.__name__, + [self.name, self.table], + {} + ) + def state_forwards(self, app_label, state): state.models[app_label, self.name.lower()].options["db_table"] = self.table @@ -241,6 +265,13 @@ class AlterUniqueTogether(Operation): unique_together = normalize_together(unique_together) self.unique_together = set(tuple(cons) for cons in unique_together) + def deconstruct(self): + return ( + self.__class__.__name__, + [self.name, self.unique_together], + {} + ) + def state_forwards(self, app_label, state): model_state = state.models[app_label, self.name.lower()] model_state.options[self.option_name] = self.unique_together @@ -279,6 +310,13 @@ class AlterIndexTogether(Operation): index_together = normalize_together(index_together) self.index_together = set(tuple(cons) for cons in index_together) + def deconstruct(self): + return ( + self.__class__.__name__, + [self.name, self.index_together], + {} + ) + def state_forwards(self, app_label, state): model_state = state.models[app_label, self.name.lower()] model_state.options[self.option_name] = self.index_together @@ -314,6 +352,13 @@ class AlterOrderWithRespectTo(Operation): self.name = name self.order_with_respect_to = order_with_respect_to + def deconstruct(self): + return ( + self.__class__.__name__, + [self.name, self.order_with_respect_to], + {} + ) + def state_forwards(self, app_label, state): model_state = state.models[app_label, self.name.lower()] model_state.options['order_with_respect_to'] = self.order_with_respect_to @@ -366,6 +411,13 @@ class AlterModelOptions(Operation): self.name = name self.options = options + def deconstruct(self): + return ( + self.__class__.__name__, + [self.name, self.options], + {} + ) + def state_forwards(self, app_label, state): model_state = state.models[app_label, self.name.lower()] model_state.options = dict(model_state.options) diff --git a/django/db/migrations/operations/special.py b/django/db/migrations/operations/special.py index 3a29a33a6b..c5181dd294 100644 --- a/django/db/migrations/operations/special.py +++ b/django/db/migrations/operations/special.py @@ -15,6 +15,18 @@ class SeparateDatabaseAndState(Operation): self.database_operations = database_operations or [] self.state_operations = state_operations or [] + def deconstruct(self): + kwargs = {} + if self.database_operations: + kwargs['database_operations'] = self.database_operations + if self.state_operations: + kwargs['state_operations'] = self.state_operations + return ( + self.__class__.__name__, + [], + kwargs + ) + def state_forwards(self, app_label, state): for state_operation in self.state_operations: state_operation.state_forwards(app_label, state) @@ -55,6 +67,18 @@ class RunSQL(Operation): self.reverse_sql = reverse_sql self.state_operations = state_operations or [] + def deconstruct(self): + kwargs = {} + if self.reverse_sql is not None: + kwargs['reverse_sql'] = self.reverse_sql + if self.state_operations: + kwargs['state_operations'] = self.state_operations + return ( + self.__class__.__name__, + [self.sql], + kwargs + ) + @property def reversible(self): return self.reverse_sql is not None @@ -112,6 +136,18 @@ class RunPython(Operation): raise ValueError("RunPython must be supplied with callable arguments") self.reverse_code = reverse_code + def deconstruct(self): + kwargs = {} + if self.reverse_code: + kwargs['reverse_code'] = self.reverse_code + if self.atomic is not True: + kwargs['atomic'] = self.atomic + return ( + self.__class__.__name__, + [self.code], + kwargs + ) + @property def reversible(self): return self.reverse_code is not None diff --git a/django/db/migrations/topological_sort.py b/django/db/migrations/topological_sort.py new file mode 100644 index 0000000000..560091c754 --- /dev/null +++ b/django/db/migrations/topological_sort.py @@ -0,0 +1,31 @@ +def topological_sort_as_sets(dependency_graph): + """Variation of Kahn's algorithm (1962) that returns sets. + + Takes a dependency graph as a dictionary of node => dependencies. + + Yields sets of items in topological order, where the first set contains + all nodes without dependencies, and each following set contains all + nodes that depend on the nodes in the previously yielded sets. + """ + todo = dependency_graph.copy() + while todo: + current = {node for node, deps in todo.items() if len(deps) == 0} + + if not current: + raise ValueError('Cyclic dependency in graph: {}'.format( + ', '.join(repr(x) for x in todo.items()))) + + yield current + + # remove current from todo's nodes & dependencies + todo = {node: (dependencies - current) for node, dependencies in + todo.items() if node not in current} + + +def stable_topological_sort(l, dependency_graph): + result = [] + for layer in topological_sort_as_sets(dependency_graph): + for node in l: + if node in layer: + result.append(node) + return result diff --git a/tests/custom_migration_operations/more_operations.py b/tests/custom_migration_operations/more_operations.py index 6fe3d1cf93..30cf246ceb 100644 --- a/tests/custom_migration_operations/more_operations.py +++ b/tests/custom_migration_operations/more_operations.py @@ -5,6 +5,13 @@ class TestOperation(Operation): def __init__(self): pass + def deconstruct(self): + return ( + self.__class__.__name__, + [], + {} + ) + @property def reversible(self): return True diff --git a/tests/custom_migration_operations/operations.py b/tests/custom_migration_operations/operations.py index fc084f8412..3a4127d753 100644 --- a/tests/custom_migration_operations/operations.py +++ b/tests/custom_migration_operations/operations.py @@ -5,6 +5,13 @@ class TestOperation(Operation): def __init__(self): pass + def deconstruct(self): + return ( + self.__class__.__name__, + [], + {} + ) + @property def reversible(self): return True diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index 3f024fe523..25aaf939ce 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -1268,12 +1268,12 @@ class AutodetectorTests(TestCase): # Right number/type of migrations? self.assertNumberMigrations(changes, "testapp", 1) self.assertOperationTypes(changes, "testapp", 0, [ - "RemoveField", "RemoveField", "DeleteModel", "RemoveField", "DeleteModel" + "RemoveField", "RemoveField", "RemoveField", "DeleteModel", "DeleteModel" ]) self.assertOperationAttributes(changes, "testapp", 0, 0, name="publishers", model_name='author') self.assertOperationAttributes(changes, "testapp", 0, 1, name="author", model_name='contract') - self.assertOperationAttributes(changes, "testapp", 0, 2, name="Author") - self.assertOperationAttributes(changes, "testapp", 0, 3, name="publisher", model_name='contract') + self.assertOperationAttributes(changes, "testapp", 0, 2, name="publisher", model_name='contract') + self.assertOperationAttributes(changes, "testapp", 0, 3, name="Author") self.assertOperationAttributes(changes, "testapp", 0, 4, name="Contract") def test_non_circular_foreignkey_dependency_removal(self): diff --git a/tests/migrations/test_optimizer.py b/tests/migrations/test_optimizer.py index d36b93355e..dba07c2878 100644 --- a/tests/migrations/test_optimizer.py +++ b/tests/migrations/test_optimizer.py @@ -20,43 +20,14 @@ class OptimizerTests(TestCase): def assertOptimizesTo(self, operations, expected, exact=None, less_than=None): result, iterations = self.optimize(operations) + result = [repr(f.deconstruct()) for f in result] + expected = [repr(f.deconstruct()) for f in expected] self.assertEqual(expected, result) if exact is not None and iterations != exact: raise self.failureException("Optimization did not take exactly %s iterations (it took %s)" % (exact, iterations)) if less_than is not None and iterations >= less_than: raise self.failureException("Optimization did not take less than %s iterations (it took %s)" % (less_than, iterations)) - def test_operation_equality(self): - """ - Tests the equality operator on lists of operations. - If this is broken, then the optimizer will get stuck in an - infinite loop, so it's kind of important. - """ - self.assertEqual( - [migrations.DeleteModel("Test")], - [migrations.DeleteModel("Test")], - ) - self.assertEqual( - [migrations.CreateModel("Test", [("name", models.CharField(max_length=255))])], - [migrations.CreateModel("Test", [("name", models.CharField(max_length=255))])], - ) - self.assertNotEqual( - [migrations.CreateModel("Test", [("name", models.CharField(max_length=255))])], - [migrations.CreateModel("Test", [("name", models.CharField(max_length=100))])], - ) - self.assertEqual( - [migrations.AddField("Test", "name", models.CharField(max_length=255))], - [migrations.AddField("Test", "name", models.CharField(max_length=255))], - ) - self.assertNotEqual( - [migrations.AddField("Test", "name", models.CharField(max_length=255))], - [migrations.AddField("Test", "name", models.CharField(max_length=100))], - ) - self.assertNotEqual( - [migrations.AddField("Test", "name", models.CharField(max_length=255))], - [migrations.AlterField("Test", "name", models.CharField(max_length=255))], - ) - def test_single(self): """ Tests that the optimizer does nothing on a single operation, @@ -331,7 +302,7 @@ class OptimizerTests(TestCase): migrations.AlterField("Foo", "age", models.FloatField(default=2.4)), ], [ - migrations.AddField("Foo", "age", models.FloatField(default=2.4)), + migrations.AddField("Foo", name="age", field=models.FloatField(default=2.4)), ], )