Fixed bug in circular dependency algo for migration dependencies.

Previous algo only worked if the first item was a part of the loop,
and you would get an infinite loop otherwise (see test).

To fix this, it was much easier to do a pre-pass.

A bonus is that you now get an error message that actually helps you debug
the dependency problem.

2 files changed, 35 insertions, 10 deletions

diff --git a/django/db/migrations/graph.py b/django/db/migrations/graph.py
index a2c14f2ba6..9046ec5ed1 100644
--- a/django/db/migrations/graph.py
+++ b/django/db/migrations/graph.py
@@ -99,29 +99,40 @@ class MigrationGraph(object):
                 leaves.add(node)
         return sorted(leaves)
 
+    def ensure_not_cyclic(self, start, get_children):
+        # Algo from GvR:
+        # http://neopythonic.blogspot.co.uk/2009/01/detecting-cycles-in-directed-graph.html
+        todo = set(self.nodes.keys())
+        while todo:
+            node = todo.pop()
+            stack = [node]
+            while stack:
+                top = stack[-1]
+                for node in get_children(top):
+                    if node in stack:
+                        cycle = stack[stack.index(node):]
+                        raise CircularDependencyError(", ".join("%s.%s" % n for n in cycle))
+                    if node in todo:
+                        stack.append(node)
+                        todo.remove(node)
+                        break
+                else:
+                    node = stack.pop()
+
     def dfs(self, start, get_children):
         """
         Iterative depth first search, for finding dependencies.
         """
+        self.ensure_not_cyclic(start, get_children)
         visited = deque()
         visited.append(start)
-        path = [start]
         stack = deque(sorted(get_children(start)))
         while stack:
             node = stack.popleft()
-
-            if node in path:
-                raise CircularDependencyError()
-            path.append(node)
-
             visited.appendleft(node)
             children = sorted(get_children(node), reverse=True)
             # reverse sorting is needed because prepending using deque.extendleft
             # also effectively reverses values
-
-            if path[-1] == node:
-                path.pop()
-
             stack.extendleft(children)
 
         return list(OrderedSet(visited))
diff --git a/tests/migrations/test_graph.py b/tests/migrations/test_graph.py
index 5b3f5b8467..abb2b28dc1 100644
--- a/tests/migrations/test_graph.py
+++ b/tests/migrations/test_graph.py
@@ -134,6 +134,20 @@ class GraphTests(TestCase):
             graph.forwards_plan, ("app_a", "0003"),
         )
 
+    def test_circular_graph_2(self):
+        graph = MigrationGraph()
+        graph.add_node(('A', '0001'), None)
+        graph.add_node(('C', '0001'), None)
+        graph.add_node(('B', '0001'), None)
+        graph.add_dependency('A.0001', ('A', '0001'),('B', '0001'))
+        graph.add_dependency('B.0001', ('B', '0001'),('A', '0001'))
+        graph.add_dependency('C.0001', ('C', '0001'),('B', '0001'))
+
+        self.assertRaises(
+            CircularDependencyError,
+            graph.forwards_plan, ('C', '0001')
+        )
+
     def test_dfs(self):
         graph = MigrationGraph()
         root = ("app_a", "1")