[1.7.x] 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. Backport offf3d746e8d
from master, with additional tests fromc5def493d0
This commit is contained in:
parent
2a20bccda9
commit
8e3c3be32d
|
@ -1,4 +1,5 @@
|
||||||
from __future__ import unicode_literals
|
from __future__ import unicode_literals
|
||||||
|
from collections import deque
|
||||||
|
|
||||||
from django.utils.datastructures import OrderedSet
|
from django.utils.datastructures import OrderedSet
|
||||||
from django.db.migrations.state import ProjectState
|
from django.db.migrations.state import ProjectState
|
||||||
|
@ -96,28 +97,41 @@ class MigrationGraph(object):
|
||||||
leaves.add(node)
|
leaves.add(node)
|
||||||
return sorted(leaves)
|
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):
|
def dfs(self, start, get_children):
|
||||||
"""
|
"""
|
||||||
Dynamic programming based depth first search, for finding dependencies.
|
Dynamic programming based depth first search, for finding dependencies.
|
||||||
"""
|
"""
|
||||||
visited = []
|
self.ensure_not_cyclic(start, get_children)
|
||||||
|
visited = deque()
|
||||||
visited.append(start)
|
visited.append(start)
|
||||||
path = [start]
|
stack = deque(sorted(get_children(start)))
|
||||||
stack = sorted(get_children(start))
|
|
||||||
while stack:
|
while stack:
|
||||||
node = stack.pop(0)
|
node = stack.popleft()
|
||||||
|
visited.appendleft(node)
|
||||||
if node in path:
|
children = sorted(get_children(node), reverse=True)
|
||||||
raise CircularDependencyError()
|
# reverse sorting is needed because prepending using deque.extendleft
|
||||||
path.append(node)
|
# also effectively reverses values
|
||||||
|
stack.extendleft(children)
|
||||||
visited.insert(0, node)
|
|
||||||
children = sorted(get_children(node))
|
|
||||||
|
|
||||||
if not children:
|
|
||||||
path = []
|
|
||||||
|
|
||||||
stack = children + stack
|
|
||||||
|
|
||||||
return list(OrderedSet(visited))
|
return list(OrderedSet(visited))
|
||||||
|
|
||||||
|
|
|
@ -134,6 +134,20 @@ class GraphTests(TestCase):
|
||||||
graph.forwards_plan, ("app_a", "0003"),
|
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):
|
def test_dfs(self):
|
||||||
graph = MigrationGraph()
|
graph = MigrationGraph()
|
||||||
root = ("app_a", "1")
|
root = ("app_a", "1")
|
||||||
|
@ -188,3 +202,30 @@ class GraphTests(TestCase):
|
||||||
msg = "Migration app_a.0002 dependencies reference nonexistent child node ('app_a', '0002')"
|
msg = "Migration app_a.0002 dependencies reference nonexistent child node ('app_a', '0002')"
|
||||||
with self.assertRaisesMessage(KeyError, msg):
|
with self.assertRaisesMessage(KeyError, msg):
|
||||||
graph.add_dependency("app_a.0002", ("app_a", "0002"), ("app_a", "0001"))
|
graph.add_dependency("app_a.0002", ("app_a", "0002"), ("app_a", "0001"))
|
||||||
|
|
||||||
|
def test_infinite_loop(self):
|
||||||
|
"""
|
||||||
|
Tests a complex dependency graph:
|
||||||
|
|
||||||
|
app_a: 0001 <-
|
||||||
|
\
|
||||||
|
app_b: 0001 <- x 0002 <-
|
||||||
|
/ \
|
||||||
|
app_c: 0001<- <------------- x 0002
|
||||||
|
|
||||||
|
And apply sqashing on app_c.
|
||||||
|
"""
|
||||||
|
graph = MigrationGraph()
|
||||||
|
|
||||||
|
graph.add_node(("app_a", "0001"), None)
|
||||||
|
graph.add_node(("app_b", "0001"), None)
|
||||||
|
graph.add_node(("app_b", "0002"), None)
|
||||||
|
graph.add_node(("app_c", "0001_squashed_0002"), None)
|
||||||
|
|
||||||
|
graph.add_dependency("app_b.0001", ("app_b", "0001"), ("app_c", "0001_squashed_0002"))
|
||||||
|
graph.add_dependency("app_b.0002", ("app_b", "0002"), ("app_a", "0001"))
|
||||||
|
graph.add_dependency("app_b.0002", ("app_b", "0002"), ("app_b", "0001"))
|
||||||
|
graph.add_dependency("app_c.0001_squashed_0002", ("app_c", "0001_squashed_0002"), ("app_b", "0002"))
|
||||||
|
|
||||||
|
with self.assertRaises(CircularDependencyError):
|
||||||
|
graph.forwards_plan(("app_c", "0001_squashed_0002"))
|
||||||
|
|
Loading…
Reference in New Issue