Mercurial > p > roundup > code
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}
