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)), ], )