Generate savepoint only if necessary

Now some methods got an additional 'allow_abort' parameter. By default
this is True. When False the postgres backend generates a savepoint.
The methods are called with allow_abort=False from some of the cgi
methods which can produce a traceback when called with data from the
web-interface.

Author: schlatterbeck

roundup/backends/back_anydbm.py

@@ -381,11 +381,13 @@ def savenode(self, classname, nodeid, node):
                 'save %s%s %r' % (classname, nodeid, node))
         self.transactions.append((self.doSaveNode, (classname, nodeid, node)))
 
-    def getnode(self, classname, nodeid, db=None, cache=1):
+    def getnode(self, classname, nodeid, db=None, cache=1, allow_abort=True):
         """ get a node from the database
 
             Note the "cache" parameter is not used, and exists purely for
             backward compatibility!
+
+            'allow_abort' is used only in sql backends.
         """
         # try the cache
         cache_dict = self.cache.setdefault(classname, {})
@@ -1026,7 +1028,7 @@ def create_inner(self, **propvalues):
 
         return newid
 
-    def get(self, nodeid, propname, default=_marker, cache=1):
+    def get(self, nodeid, propname, default=_marker, cache=1, allow_abort=True):
         """Get the value of a property on an existing node of this class.
 
         'nodeid' must be the id of an existing node of this class or an
@@ -1035,6 +1037,8 @@ def get(self, nodeid, propname, default=_marker, cache=1):
 
         'cache' exists for backward compatibility, and is not used.
 
+        'allow_abort' is used only in sql backends.
+
         Attempts to get the "creation" or "activity" properties should
         do the right thing.
         """
@@ -1459,8 +1463,9 @@ def restore(self, nodeid):
 
         self.fireReactors('restore', nodeid, None)
 
-    def is_retired(self, nodeid, cldb=None):
+    def is_retired(self, nodeid, cldb=None, allow_abort=True):
         """Return true if the node is retired.
+           'allow_abort' is used only in sql backends.
         """
         node = self.db.getnode(self.classname, nodeid, cldb)
         if self.db.RETIRED_FLAG in node:

roundup/backends/back_postgresql.py

@@ -560,14 +560,17 @@ def clear(self):
             self.cursor.execute('DROP SEQUENCE _%s_ids' % cn)
             self.cursor.execute('CREATE SEQUENCE _%s_ids' % cn)
 
-    def getnode (self, classname, nodeid, fetch_multilinks=True):
+    def getnode(self, classname, nodeid, fetch_multilinks=True,
+                allow_abort=True):
         """ For use of savepoint see 'Class' below """
-        self.sql('savepoint sp')
+        if not allow_abort:
+            self.sql('savepoint sp')
         try:
             getnode = rdbms_common.Database.getnode
             return getnode(self, classname, nodeid, fetch_multilinks)
         except psycopg2.errors.DataError as err:
-            self.sql('rollback to savepoint sp')
+            if not allow_abort:
+                self.sql('rollback to savepoint sp')
             raise hyperdb.HyperdbValueError(str (err).split('\n')[0])
 
 
@@ -583,28 +586,26 @@ class Class(PostgresqlClass, rdbms_common.Class):
     """
 
     def filter(self, *args, **kw):
-        self.db.sql('savepoint sp')
         try:
             return rdbms_common.Class.filter(self, *args, **kw)
         except psycopg2.errors.DataError as err:
-            self.db.sql('rollback to savepoint sp')
             raise hyperdb.HyperdbValueError(str (err).split('\n')[0])
 
     def filter_iter(self, *args, **kw):
-        self.db.sql('savepoint sp')
         try:
             for v in rdbms_common.Class.filter_iter(self, *args, **kw):
                 yield v
         except psycopg2.errors.DataError as err:
-            self.db.sql('rollback to savepoint sp')
             raise hyperdb.HyperdbValueError(str (err).split('\n')[0])
 
-    def is_retired(self, nodeid):
-        self.db.sql('savepoint sp')
+    def is_retired(self, nodeid, allow_abort=True):
+        if not allow_abort:
+            self.db.sql('savepoint sp')
         try:
             return rdbms_common.Class.is_retired(self, nodeid)
         except psycopg2.errors.DataError as err:
-            self.db.sql('rollback to savepoint sp')
+            if not allow_abort:
+                self.db.sql('rollback to savepoint sp')
             raise hyperdb.HyperdbValueError (str (err).split('\n')[0])
 
 

roundup/backends/rdbms_common.py

@@ -1,4 +1,4 @@
-#
+
 # Copyright (c) 2001 Bizar Software Pty Ltd (http://www.bizarsoftware.com.au/)
 # This module is free software, and you may redistribute it and/or modify
 # under the same terms as Python, so long as this copyright message and
@@ -1251,11 +1251,14 @@ def _materialize_multilinks(self, classname, nodeid, node, props=None):
             if propname not in node:
                 self._materialize_multilink(classname, nodeid, node, propname)
 
-    def getnode(self, classname, nodeid, fetch_multilinks=True):
+    def getnode(self, classname, nodeid, fetch_multilinks=True,
+                allow_abort=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.
+            'allow_abort' determines if we allow that the current
+            transaction is aborted due to a data error (e.g. invalid nodeid).
         """
         # see if we have this node cached
         key = (classname, nodeid)
@@ -1863,20 +1866,24 @@ def create_inner(self, **propvalues):
         # XXX numeric ids
         return str(newid)
 
-    def get(self, nodeid, propname, default=_marker, cache=1):
+    def get(self, nodeid, propname, default=_marker, cache=1, allow_abort=True):
         """Get the value of a property on an existing node of this class.
 
         'nodeid' must be the id of an existing node of this class or an
         IndexError is raised.  'propname' must be the name of a property
         of this class or a KeyError is raised.
 
         'cache' exists for backwards compatibility, and is not used.
+
+        'allow_abort' determines if we allow that the current
+        transaction is aborted due to a data error (e.g.  invalid nodeid).
         """
         if propname == 'id':
             return nodeid
 
         # get the node's dict
-        d = self.db.getnode(self.classname, nodeid, fetch_multilinks=False)
+        d = self.db.getnode(self.classname, nodeid, fetch_multilinks=False,
+                            allow_abort=allow_abort)
         # handle common case -- that property is in dict -- first
         # if None and one of creator/creation actor/activity return None
         if propname in d:
@@ -2245,8 +2252,10 @@ def restore(self, nodeid):
 
         self.fireReactors('restore', nodeid, None)
 
-    def is_retired(self, nodeid):
+    def is_retired(self, nodeid, allow_abort=True):
         """Return true if the node is retired
+           The allow_abort parameter determines if we allow the
+           transaction to be aborted when an invalid nodeid has been passed.
         """
         # Do not produce invalid sql, the id must be numeric
         try:

roundup/cgi/actions.py

@@ -661,8 +661,13 @@ def _editnodes(self, all_props, all_links):
             cn, nodeid = needed
             if props:
                 if nodeid is not None and int(nodeid) > 0:
-                    # make changes to the node
-                    props = self._changenode(cn, nodeid, props)
+                    # make changes to the node, if an error occurs the
+                    # db may be in a state that needs rollback
+                    try:
+                        props = self._changenode(cn, nodeid, props)
+                    except (IndexError, ValueError):
+                        self.db.rollback ()
+                        raise
 
                     # and some nice feedback for the user
                     if props:

roundup/cgi/templating.py

@@ -587,7 +587,7 @@ def lookupKeys(linkcl, key, ids, num_re=num_re):
     for entry in ids:
         if num_re.match(entry):
             try:
-                label = linkcl.get(entry, key)
+                label = linkcl.get(entry, key, allow_abort=False)
             except IndexError:
                 # fall back to id if illegal (avoid template crash)
                 label = entry
@@ -1121,7 +1121,8 @@ def __getitem__(self, item):
         value = None
         try:
             if int(self._nodeid) > 0:
-                value = self._klass.get(self._nodeid, items[0], None)
+                value = self._klass.get(self._nodeid, items[0], None,
+                                        allow_abort=False)
         except (IndexError, ValueError):
             value = self._nodeid
         if value is None:
@@ -1157,7 +1158,7 @@ def designator(self):
 
     def is_retired(self):
         """Is this item retired?"""
-        return self._klass.is_retired(self._nodeid)
+        return self._klass.is_retired(self._nodeid, allow_abort=False)
 
     def submit(self, label=''"Submit Changes", action="edit", html_kwargs={}):
         """Generate a submit button.
@@ -2572,7 +2573,7 @@ def field(self, showid=0, size=None, **kwargs):
             idparse = self._prop.try_id_parsing
             if k and num_re.match(self._value):
                 try:
-                    value = linkcl.get(self._value, k)
+                    value = linkcl.get(self._value, k, allow_abort=False)
                 except (IndexError, hyperdb.HyperdbValueError) as err:
                     if idparse:
                         self._client.add_error_message(str(err))
@@ -3043,7 +3044,7 @@ def make_key_function(db, classname, sort_on=None):
 
     def keyfunc(a):
         if num_re.match(a):
-            a = linkcl.get(a, sort_on)
+            a = linkcl.get(a, sort_on, allow_abort=False)
             # In Python3 we may not compare numbers/strings and None
             if a is None:
                 if isinstance(prop, (hyperdb.Number, hyperdb.Integer)):

roundup/hyperdb.py

@@ -1065,10 +1065,12 @@ def unserialise(self, classname, node):
         """
         return node
 
-    def getnode(self, classname, nodeid):
+    def getnode(self, classname, nodeid, allow_abort=True):
         """Get a node from the database.
 
         'cache' exists for backwards compatibility, and is not used.
+        'allow_abort' determines if we allow that the current
+        transaction is aborted due to a data error (e.g. invalid nodeid).
         """
         raise NotImplementedError
 
@@ -1235,14 +1237,16 @@ def create(self, **propvalues):
         """
         raise NotImplementedError
 
-    def get(self, nodeid, propname, default=_marker, cache=1):
+    def get(self, nodeid, propname, default=_marker, cache=1, allow_abort=True):
         """Get the value of a property on an existing node of this class.
 
         'nodeid' must be the id of an existing node of this class or an
         IndexError is raised.  'propname' must be the name of a property
         of this class or a KeyError is raised.
 
         'cache' exists for backwards compatibility, and is not used.
+        'allow_abort' determines if we allow that the current
+        transaction is aborted due to a data error (e.g. invalid nodeid).
         """
         raise NotImplementedError
 
@@ -1300,8 +1304,10 @@ def restore(self, nodeid):
         """
         raise NotImplementedError
 
-    def is_retired(self, nodeid):
+    def is_retired(self, nodeid, allow_abort=True):
         """Return true if the node is rerired
+           'allow_abort' specifies if we allow the transaction to be
+           aborted if a syntactically invalid nodeid is passed.
         """
         raise NotImplementedError
 
@@ -2182,10 +2188,13 @@ def export_files(self, dirname, nodeid):
         ensureParentsExist(dest)
         shutil.copyfile(source, dest)
 
-    def get(self, nodeid, propname, default=_marker, cache=1):
+    def get(self, nodeid, propname, default=_marker, cache=1, allow_abort=True):
         """ Trap the content propname and get it from the file
 
         'cache' exists for backwards compatibility, and is not used.
+
+        'allow_abort' determines if we allow that the current
+        transaction is aborted due to a data error (e.g. invalid nodeid).
         """
         poss_msg = 'Possibly an access right configuration problem.'
         if propname == 'content':
@@ -2212,9 +2221,11 @@ def get(self, nodeid, propname, default=_marker, cache=1):
             return self.db.getfile(self.classname, nodeid, None)
 
         if default is not _marker:
-            return self.subclass.get(self, nodeid, propname, default)
+            return self.subclass.get(self, nodeid, propname, default,
+                                     allow_abort=allow_abort)
         else:
-            return self.subclass.get(self, nodeid, propname)
+            return self.subclass.get(self, nodeid, propname,
+                                     allow_abort=allow_abort)
 
     def import_files(self, dirname, nodeid):
         """ Import the "content" property as a file

test/test_templating.py

@@ -143,7 +143,9 @@ def test_HTMLDatabase_list(self):
         db = MockNull(issue = HTMLDatabase(self.client).issue)
         db.issue._klass.list = lambda : ['23', 'a', 'b']
         # Make db.getclass return something that has a sensible 'get' method
-        mock = MockNull(get = lambda x, y : None)
+        def get(x, y, allow_abort=True):
+            return None
+        mock = MockNull(get = get)
         db.issue._db.getclass = lambda x : mock
         l = db.issue.list()
 
@@ -170,7 +172,7 @@ def lookup(key):
 
     def test_lookupKeys(self):
         db = HTMLDatabase(self.client)
-        def get(entry, key):
+        def get(entry, key, allow_abort=True):
             return {'1': 'green', '2': 'eggs'}.get(entry, entry)
         shrubbery = MockNull(get=get)
         db._db.classes = {'shrubbery': shrubbery}