From: schlatterbeck Date: Sat, 16 Apr 2011 11:02:01 +0000 (+0000) Subject: Fixed bug in filter_iter refactoring (lazy multilinks), in rare cases X-Git-Url: https://git.tokkee.org/?a=commitdiff_plain;h=0bb9d682bb0fc2ea037807639400967bc66aa1eb;p=roundup.git 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 3a07134..c5ea367 100644 --- 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 5798c8d..3a4d6f5 100644 --- 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 fe35b5d..428bc1d 100644 --- 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