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.
   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)
 
 
 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)
 
 
         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.
         """ 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)
         """
         # 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 __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__:
             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
 
                 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)
         # 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
             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:
         # 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):
 
         # 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:
 
         # 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"), [])
 
             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
     def testMultilinkChangeIterable(self):
         for commit in (0,1):
             # invalid nosy value assertion