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
13d613f800.

AUTHORS

@@ -378,6 +378,7 @@ answer newbie questions, and generally made Django that much better:
     Kevin McConnell <kevin.mcconnell@gmail.com>
     Kieran Holland <http://www.kieranholland.com>
     kilian <kilian.cavalotti@lip6.fr>
+    Klaas van Schelven <klaas@vanschelven.com>
     knox <christobzr@gmail.com>
     konrad@gwu.edu
     Kowito Charoenratchatabhan <kowito@felspar.com>

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):
+
+            # 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:
-                            # 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
+                        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.

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)

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 = [

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)

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

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

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

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

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):

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