- Fixed bug in unit of work whereby detection of

    "cycles" among classes in highly interlinked patterns
    would not produce a deterministic
    result; thereby sometimes missing some nodes that
    should be considered cycles and causing further
    issues down the road.  Note this bug is in 0.6
    also; not backported at the moment.
    [ticket:2282]

diff --git a/CHANGES b/CHANGES
index dd10286576a66cf79524713477904b1af4cdc0a0..598b8be67273da02c2d8b6c97230d5f79b09da7a 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -31,6 +31,15 @@ CHANGES
     attribute, provided the name is the same as that
     of the entity mapped column.
 
+  - Fixed bug in unit of work whereby detection of 
+    "cycles" among classes in highly interlinked patterns
+    would not produce a deterministic
+    result; thereby sometimes missing some nodes that
+    should be considered cycles and causing further
+    issues down the road.  Note this bug is in 0.6 
+    also; not backported at the moment.
+    [ticket:2282]
+
   - Fixed a variety of synonym()-related regressions
     from 0.6:
         - making a synonym against a synonym now works.

diff --git a/lib/sqlalchemy/orm/unitofwork.py b/lib/sqlalchemy/orm/unitofwork.py
index 5e0c939385662619731a2074761731cdc146fd43..5c1f3b1a708af21f8732d3fc54524979da1870df 100644 (file)
--- a/lib/sqlalchemy/orm/unitofwork.py
+++ b/lib/sqlalchemy/orm/unitofwork.py
@@ -308,9 +308,10 @@ class UOWTransaction(object):
 
         #sort = topological.sort(self.dependencies, postsort_actions)
         #print "--------------"
-        #print self.dependencies
-        #print list(sort)
-        #print "COUNT OF POSTSORT ACTIONS", len(postsort_actions)
+        #print "\ndependencies:", self.dependencies
+        #print "\ncycles:", self.cycles
+        #print "\nsort:", list(sort)
+        #print "\nCOUNT OF POSTSORT ACTIONS", len(postsort_actions)
 
         # execute
         if self.cycles:

diff --git a/lib/sqlalchemy/util/topological.py b/lib/sqlalchemy/util/topological.py
index 8f340647222e376347c76c7a99caadaed42a0f44..ba68b713605c44c39a7891499f55e84a7c0d51f3 100644 (file)
--- a/lib/sqlalchemy/util/topological.py
+++ b/lib/sqlalchemy/util/topological.py
@@ -48,17 +48,26 @@ def sort(tuples, allitems):
 
 def find_cycles(tuples, allitems):
     # straight from gvr with some mods
-    todo = set(allitems)
 
     edges = util.defaultdict(set)
     for parent, child in tuples:
         edges[parent].add(child)
+    nodes_to_test = set(edges)
 
     output = set()
 
-    while todo:
-        node = todo.pop()
+    # we'd like to find all nodes that are 
+    # involved in cycles, so we do the full
+    # pass through the whole thing for each
+    # node in the original list.
+
+    # we can go just through parent edge nodes.
+    # if a node is only a child and never a parent,
+    # by definition it can't be part of a cycle.  same
+    # if it's not in the edges at all.
+    for node in nodes_to_test:
         stack = [node]
+        todo = nodes_to_test.difference(stack)
         while stack:
             top = stack[-1]
             for node in edges[top]:

diff --git a/test/base/test_dependency.py b/test/base/test_dependency.py
index 85347ad913a2c9524a8fb502ce9f617773b7ab21..4be3c839018defd2a254343ca35eeb64f9599515 100644 (file)
--- a/test/base/test_dependency.py
+++ b/test/base/test_dependency.py
@@ -222,8 +222,11 @@ class DependencySortTest(fixtures.TestBase):
             node5,
             node6,
             ])
-        eq_(topological.find_cycles(tuples, allnodes), set(['node1',
-            'node2', 'node4']))
+        # node6 only became present here once [ticket:2282] was addressed.
+        eq_(
+            topological.find_cycles(tuples, allnodes), 
+            set(['node1','node2', 'node4', 'node6'])
+        )
 
     def test_find_multiple_cycles_three(self):
         node1 = 'node1'
@@ -252,3 +255,26 @@ class DependencySortTest(fixtures.TestBase):
             node6,
             ])
         eq_(topological.find_cycles(tuples, allnodes), allnodes)
+
+    def test_find_multiple_cycles_four(self):
+        tuples = [
+            ('node6', 'node2'), 
+            ('node15', 'node19'), 
+            ('node19', 'node2'), ('node4', 'node10'),
+            ('node15', 'node13'),
+            ('node17', 'node11'), ('node1', 'node19'), ('node15', 'node8'), 
+            ('node6', 'node20'), ('node14', 'node11'), ('node6', 'node14'), 
+            ('node11', 'node2'), ('node10', 'node20'), ('node1', 'node11'),
+             ('node20', 'node19'), ('node4', 'node20'), ('node15', 'node20'),
+             ('node9', 'node19'), ('node11', 'node10'), ('node11', 'node19'),
+              ('node13', 'node6'), ('node3', 'node15'), ('node9', 'node11'),
+              ('node4', 'node17'), ('node2', 'node20'), ('node19', 'node10'), 
+              ('node8', 'node4'), ('node11', 'node3'), ('node6', 'node1')
+        ]
+        allnodes = ['node%d' % i for i in xrange(1, 21)]
+        eq_(
+            topological.find_cycles(tuples, allnodes), 
+            set(['node11', 'node10', 'node13', 'node15', 'node14', 'node17', 
+            'node19', 'node20', 'node8', 'node1', 'node3', 
+            'node2', 'node4', 'node6'])
+        )

diff --git a/test/bootstrap/config.py b/test/bootstrap/config.py
index bdfc8189f52eddcebca26eb43667191a6de0db36..edf94fae5ff566a5949aee7392a640f4a9624e35 100644 (file)
--- a/test/bootstrap/config.py
+++ b/test/bootstrap/config.py
@@ -168,7 +168,7 @@ post_configure.append(_set_table_options)
 def _reverse_topological(options, file_config):
     if options.reversetop:
         from sqlalchemy.orm import unitofwork, session, mapper, dependency
-        from sqlalchemy import topological
+        from sqlalchemy.util import topological
         from test.lib.util import RandomSet
         topological.set = unitofwork.set = session.set = mapper.set = dependency.set = RandomSet
 post_configure.append(_reverse_topological)