Fixed bug in filter_iter refactoring (lazy multilinks), in rare cases

this would result in duplicate multilinks to the same node. We're now
going the safe route and doing lazy evaluation only for read-only
access, whenever updates are done we fetch everything.

git-svn-id: http://svn.roundup-tracker.org/svnroot/roundup/roundup/trunk@4599 57a73879-2fb5-44c3-a270-3262357dd7e2

diff --git a/CHANGES.txt b/CHANGES.txt
index 3a071349d5f1762e9dddd241c7be30f0eccd6525..c5ea367346d54433d90c26ea51c789e89e7b72d5 100644 (file)
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -96,6 +96,10 @@ Fixed:
   parameter for binding to all interfaces now (still left in for
   compatibility). Thanks to Toni Mueller for providing the first version
   of this patch and discussing implementations.
+- Fixed bug in filter_iter refactoring (lazy multilinks), in rare cases
+  this would result in duplicate multilinks to the same node. We're now
+  going the safe route and doing lazy evaluation only for read-only
+  access, whenever updates are done we fetch everything.
 
 2010-10-08 1.4.16 (r4541)
 

diff --git a/roundup/backends/rdbms_common.py b/roundup/backends/rdbms_common.py
index 5798c8d646e1ab223a16db221e06f52cf47a05e2..3a4d6f5c276ee9c7fe5a226a5bdd7bc5f9edbf38 100644 (file)
--- a/roundup/backends/rdbms_common.py
+++ b/roundup/backends/rdbms_common.py
@@ -1080,8 +1080,34 @@ class Database(FileStorage, hyperdb.Database, roundupdb.Database):
 
         raise ValueError('%r is not a hyperdb property class' % propklass)
 
-    def getnode(self, classname, nodeid):
+    def _materialize_multilink(self, classname, nodeid, node, propname):
+        """ evaluation of single Multilink (lazy eval may have skipped this)
+        """
+        if propname not in node:
+            sql = 'select linkid from %s_%s where nodeid=%s'%(classname,
+                propname, self.arg)
+            self.sql(sql, (nodeid,))
+            # extract the first column from the result
+            # XXX numeric ids
+            items = [int(x[0]) for x in self.cursor.fetchall()]
+            items.sort ()
+            node[propname] = [str(x) for x in items]
+
+    def _materialize_multilinks(self, classname, nodeid, node, props=None):
+        """ get all Multilinks of a node (lazy eval may have skipped this)
+        """
+        cl = self.classes[classname]
+        props = props or [pn for (pn, p) in cl.properties.iteritems()
+                          if isinstance(p, Multilink)]
+        for propname in props:
+            if propname not in node:
+                self._materialize_multilink(classname, nodeid, node, propname)
+
+    def getnode(self, classname, nodeid, fetch_multilinks=True):
         """ Get a node from the database.
+            For optimisation optionally we don't fetch multilinks
+            (lazy Multilinks).
+            But for internal database operations we need them.
         """
         # see if we have this node cached
         key = (classname, nodeid)
@@ -1091,6 +1117,8 @@ class Database(FileStorage, hyperdb.Database, roundupdb.Database):
             if __debug__:
                 self.stats['cache_hits'] += 1
             # return the cached information
+            if fetch_multilinks:
+                self._materialize_multilinks(classname, nodeid, self.cache[key])
             return self.cache[key]
 
         if __debug__:
@@ -1124,6 +1152,9 @@ class Database(FileStorage, hyperdb.Database, roundupdb.Database):
                 value = self.to_hyperdb_value(props[name].__class__)(value)
             node[name] = value
 
+        if fetch_multilinks and mls:
+            self._materialize_multilinks(classname, nodeid, node, mls)
+
         # save off in the cache
         key = (classname, nodeid)
         self._cache_save(key, node)
@@ -1616,7 +1647,7 @@ class Class(hyperdb.Class):
             return nodeid
 
         # get the node's dict
-        d = self.db.getnode(self.classname, nodeid)
+        d = self.db.getnode(self.classname, nodeid, fetch_multilinks=False)
         # handle common case -- that property is in dict -- first
         # if None and one of creator/creation actor/activity return None
         if propname in d:
@@ -1640,14 +1671,7 @@ class Class(hyperdb.Class):
 
         # lazy evaluation of Multilink
         if propname not in d and isinstance(prop, Multilink):
-            sql = 'select linkid from %s_%s where nodeid=%s'%(self.classname,
-                propname, self.db.arg)
-            self.db.sql(sql, (nodeid,))
-            # extract the first column from the result
-            # XXX numeric ids
-            items = [int(x[0]) for x in self.db.cursor.fetchall()]
-            items.sort ()
-            d[propname] = [str(x) for x in items]
+            self.db._materialize_multilink(self.classname, nodeid, d, propname)
 
         # handle there being no value in the table for the property
         if propname not in d or d[propname] is None:

diff --git a/test/db_test_base.py b/test/db_test_base.py
index fe35b5d86b30577cc97f2390c094346e5da10edb..428bc1d5e6b287eb8066cbd397386b57ee766777 100644 (file)
--- a/test/db_test_base.py
+++ b/test/db_test_base.py
@@ -321,6 +321,23 @@ class DBTest(commonDBTest):
             if commit: self.db.commit()
             self.assertEqual(self.db.issue.get(nid, "nosy"), [])
 
+    def testMakeSeveralMultilinkedNodes(self):
+        for commit in (0,1):
+            u1 = self.db.user.create(username='foo%s'%commit)
+            u2 = self.db.user.create(username='bar%s'%commit)
+            u3 = self.db.user.create(username='baz%s'%commit)
+            nid = self.db.issue.create(title="spam", nosy=[u1])
+            if commit: self.db.commit()
+            self.assertEqual(self.db.issue.get(nid, "nosy"), [u1])
+            self.db.issue.set(nid, deadline=date.Date('.'))
+            self.db.issue.set(nid, nosy=[u1,u2], title='ta%s'%commit)
+            if commit: self.db.commit()
+            self.assertEqual(self.db.issue.get(nid, "nosy"), [u1,u2])
+            self.db.issue.set(nid, deadline=date.Date('.'))
+            self.db.issue.set(nid, nosy=[u1,u2,u3], title='tb%s'%commit)
+            if commit: self.db.commit()
+            self.assertEqual(self.db.issue.get(nid, "nosy"), [u1,u2,u3])
+
     def testMultilinkChangeIterable(self):
         for commit in (0,1):
             # invalid nosy value assertion