changeset 8305:a81a3cd067fa

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 Ralf Schlatterbeck <rsc@runtux.com>
date Sat, 01 Mar 2025 18:55:54 +0100
parents 24549122f9b1
children 6d559739456a
files roundup/backends/back_anydbm.py roundup/backends/back_postgresql.py roundup/backends/rdbms_common.py roundup/cgi/actions.py roundup/cgi/templating.py roundup/hyperdb.py test/test_templating.py
diffstat 7 files changed, 67 insertions(+), 33 deletions(-) [+]
line wrap: on
line diff
--- a/roundup/backends/back_anydbm.py	Sat Mar 01 13:08:09 2025 +0100
+++ b/roundup/backends/back_anydbm.py	Sat Mar 01 18:55:54 2025 +0100
@@ -381,11 +381,13 @@
                 '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 @@
 
         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 @@
 
         '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 @@
 
         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:
--- a/roundup/backends/back_postgresql.py	Sat Mar 01 13:08:09 2025 +0100
+++ b/roundup/backends/back_postgresql.py	Sat Mar 01 18:55:54 2025 +0100
@@ -560,14 +560,17 @@
             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 @@
     """
 
     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])
 
 
--- a/roundup/backends/rdbms_common.py	Sat Mar 01 13:08:09 2025 +0100
+++ b/roundup/backends/rdbms_common.py	Sat Mar 01 18:55:54 2025 +0100
@@ -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 @@
             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,7 +1866,7 @@
         # 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
@@ -1871,12 +1874,16 @@
         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 @@
 
         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:
--- a/roundup/cgi/actions.py	Sat Mar 01 13:08:09 2025 +0100
+++ b/roundup/cgi/actions.py	Sat Mar 01 18:55:54 2025 +0100
@@ -661,8 +661,13 @@
             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:
--- a/roundup/cgi/templating.py	Sat Mar 01 13:08:09 2025 +0100
+++ b/roundup/cgi/templating.py	Sat Mar 01 18:55:54 2025 +0100
@@ -587,7 +587,7 @@
     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 @@
         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 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 @@
             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 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)):
--- a/roundup/hyperdb.py	Sat Mar 01 13:08:09 2025 +0100
+++ b/roundup/hyperdb.py	Sat Mar 01 18:55:54 2025 +0100
@@ -1065,10 +1065,12 @@
         """
         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,7 +1237,7 @@
         """
         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
@@ -1243,6 +1245,8 @@
         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 @@
         """
         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 @@
         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 @@
             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
--- a/test/test_templating.py	Sat Mar 01 13:08:09 2025 +0100
+++ b/test/test_templating.py	Sat Mar 01 18:55:54 2025 +0100
@@ -143,7 +143,9 @@
         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 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}

Roundup Issue Tracker: http://roundup-tracker.org/