Mercurial > p > roundup > code

changeset 5319:62de601bdf6f

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Fix commits although a Reject exception is raised Fix the problem that changes are committed to the database (due to commits to otk handling) even when a Reject exception occurs. The fix implements separate database connections for otk/session handling and normal database operation.
author Ralf Schlatterbeck <rsc@runtux.com>
date Fri, 20 Apr 2018 18:46:28 +0200
parents 506c7ee9a385
children bb1125433de6
files CHANGES.txt roundup/backends/back_anydbm.py roundup/backends/back_mysql.py roundup/backends/back_postgresql.py roundup/backends/back_sqlite.py roundup/backends/rdbms_common.py roundup/backends/sessions_rdbms.py roundup/cgi/actions.py roundup/cgi/client.py roundup/cgi/templating.py roundup/hyperdb.py roundup/roundupdb.py test/db_test_base.py test/session_common.py test/test_anydbm.py test/test_memorydb.py test/test_mysql.py test/test_postgresql.py test/test_sqlite.py
diffstat 19 files changed, 106 insertions(+), 92 deletions(-) [+]
line wrap: on
line diff
--- a/CHANGES.txt	Thu Apr 19 20:01:43 2018 +0200
+++ b/CHANGES.txt	Fri Apr 20 18:46:28 2018 +0200
@@ -493,6 +493,12 @@
   obsolete_history_roles in the main section that defines which roles
   may see removed properties. By default only role Admin is allowed to
   see these.
+- Fix issue2550955: Roundup commits although a Reject exception is raised
+  Fix the problem that changes are committed to the database (due to 
+  commits to otk handling) even when a Reject exception occurs. The fix 
+  implements separate database connections for otk/session handling and
+  normal database operation.
+
 
 2016-01-11: 1.5.1
 
--- a/roundup/backends/back_anydbm.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/roundup/backends/back_anydbm.py	Fri Apr 20 18:46:28 2018 +0200
@@ -191,6 +191,9 @@
         self.lockfile.write(str(os.getpid()))
         self.lockfile.flush()
 
+        self.Session = None
+        self.Otk     = None
+
     def post_init(self):
         """Called once the schema initialisation has finished.
         """
@@ -204,10 +207,14 @@
         self.reindex()
 
     def getSessionManager(self):
-        return Sessions(self)
+        if not self.Session:
+            self.Session = Sessions(self)
+        return self.Session
 
     def getOTKManager(self):
-        return OneTimeKeys(self)
+        if not self.Otk:
+            self.Otk = OneTimeKeys(self)
+        return self.Otk
 
     def reindex(self, classname=None, show_progress=False):
         if classname:
@@ -698,17 +705,11 @@
     #
     # Basic transaction support
     #
-    def commit(self, fail_ok=False):
+    def commit(self):
         """ Commit the current transactions.
 
         Save all data changed since the database was opened or since the
         last commit() or rollback().
-
-        fail_ok indicates that the commit is allowed to fail. This is used
-        in the web interface when committing cleaning of the session
-        database. We don't care if there's a concurrency issue there.
-
-        The only backend this seems to affect is postgres.
         """
         logging.getLogger('roundup.hyperdb.backend').info('commit %s transactions'%(
             len(self.transactions)))
--- a/roundup/backends/back_mysql.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/roundup/backends/back_mysql.py	Fri Apr 20 18:46:28 2018 +0200
@@ -563,7 +563,7 @@
         vals = (spec.classname, 1)
         self.sql(sql, vals)
 
-    def sql_commit(self, fail_ok=False):
+    def sql_commit(self):
         ''' Actually commit to the database.
         '''
         self.log_info('commit')
--- a/roundup/backends/back_postgresql.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/roundup/backends/back_postgresql.py	Fri Apr 20 18:46:28 2018 +0200
@@ -52,7 +52,7 @@
     logging.getLogger('roundup.hyperdb').info(command)
     db_command(config, command)
 
-def db_nuke(config, fail_ok=0):
+def db_nuke(config):
     """Clear all database contents and drop database itself"""
     command = 'DROP DATABASE "%s"'% config.RDBMS_NAME
     logging.getLogger('roundup.hyperdb').info(command)
@@ -151,9 +151,6 @@
     # used by some code to switch styles of query
     implements_intersect = 1
 
-    def getSessionManager(self):
-        return Sessions(self)
-
     def sql_open_connection(self):
         db = connection_dict(self.config, 'database')
         logging.getLogger('roundup.hyperdb').info(
@@ -187,6 +184,9 @@
             self.sql("CREATE TABLE dual (dummy integer)")
             self.sql("insert into dual values (1)")
             self.create_version_2_tables()
+            # Need to commit here, otherwise otk/session will not find
+            # the necessary tables (in a parallel connection!)
+            self.commit()
 
     def create_version_2_tables(self):
         # OTK store
@@ -244,25 +244,6 @@
     def __repr__(self):
         return '<roundpsycopgsql 0x%x>' % id(self)
 
-    def sql_commit(self, fail_ok=False):
-        ''' Actually commit to the database.
-        '''
-        logging.getLogger('roundup.hyperdb').info('commit')
-
-        try:
-            self.conn.commit()
-        except ProgrammingError as message:
-            # we've been instructed that this commit is allowed to fail
-            if fail_ok and str(message).endswith('could not serialize '
-                    'access due to concurrent update'):
-                logging.getLogger('roundup.hyperdb').info(
-                    'commit FAILED, but fail_ok')
-            else:
-                raise
-
-        # open a new cursor for subsequent work
-        self.cursor = self.conn.cursor()
-
     def sql_stringquote(self, value):
         ''' psycopg.QuotedString returns a "buffer" object with the
             single-quotes around it... '''
--- a/roundup/backends/back_sqlite.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/roundup/backends/back_sqlite.py	Fri Apr 20 18:46:28 2018 +0200
@@ -12,6 +12,8 @@
 
 from roundup import hyperdb, date, password
 from roundup.backends import rdbms_common
+from roundup.backends.sessions_dbm import Sessions, OneTimeKeys
+
 sqlite_version = None
 try:
     import sqlite3 as sqlite
@@ -94,6 +96,19 @@
         hyperdb.Multilink : lambda x: x,    # used in journal marshalling
     }
 
+    # We're using DBM for managing session info and one-time keys:
+    # For SQL database storage of this info we would need two concurrent
+    # connections to the same database which SQLite doesn't support
+    def getSessionManager(self):
+        if not self.Session:
+            self.Session = Sessions(self)
+        return self.Session
+
+    def getOTKManager(self):
+        if not self.Otk:
+            self.Otk = OneTimeKeys(self)
+        return self.Otk
+
     def sqlite_busy_handler(self, data, table, count):
         """invoked whenever SQLite tries to access a database that is locked"""
         now = time.time()
@@ -349,7 +364,7 @@
     def __repr__(self):
         return '<roundlite 0x%x>'%id(self)
 
-    def sql_commit(self, fail_ok=False):
+    def sql_commit(self):
         """ Actually commit to the database.
 
             Ignore errors if there's nothing to commit.
--- a/roundup/backends/rdbms_common.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/roundup/backends/rdbms_common.py	Fri Apr 20 18:46:28 2018 +0200
@@ -189,6 +189,10 @@
         # database lock
         self.lockfile = None
 
+        # Uppercase to not collide with Class names
+        self.Session = None
+        self.Otk     = None
+
         # open a connection to the database, creating the "conn" attribute
         self.open_connection()
 
@@ -199,10 +203,14 @@
         roundupdb.Database.clearCache(self)
 
     def getSessionManager(self):
-        return Sessions(self)
+        if not self.Session:
+            self.Session = Sessions(self)
+        return self.Session
 
     def getOTKManager(self):
-        return OneTimeKeys(self)
+        if not self.Otk:
+            self.Otk = OneTimeKeys(self)
+        return self.Otk
 
     def open_connection(self):
         """ Open a connection to the database, creating it if necessary.
@@ -1411,7 +1419,7 @@
                 "action<>'create'"%(classname, self.arg)
             self.sql(sql, (date_stamp,))
 
-    def sql_commit(self, fail_ok=False):
+    def sql_commit(self):
         """ Actually commit to the database.
         """
         logging.getLogger('roundup.hyperdb.backend').info('commit')
@@ -1421,20 +1429,21 @@
         # open a new cursor for subsequent work
         self.cursor = self.conn.cursor()
 
-    def commit(self, fail_ok=False):
+    def commit(self):
         """ Commit the current transactions.
 
         Save all data changed since the database was opened or since the
         last commit() or rollback().
-
-        fail_ok indicates that the commit is allowed to fail. This is used
-        in the web interface when committing cleaning of the session
-        database. We don't care if there's a concurrency issue there.
-
-        The only backend this seems to affect is postgres.
         """
         # commit the database
-        self.sql_commit(fail_ok)
+        self.sql_commit()
+
+        # session and otk are committed with the db but not the other
+        # way round
+        if self.Session:
+            self.Session.commit()
+        if self.Otk:
+            self.Otk.commit()
 
         # now, do all the other transaction stuff
         for method, args in self.transactions:
@@ -1483,6 +1492,12 @@
         """
         self.indexer.close()
         self.sql_close()
+        if self.Session:
+            self.Session.close()
+            self.Session = None
+        if self.Otk:
+            self.Otk.close()
+            self.Otk = None
 
 #
 # The base Class class
--- a/roundup/backends/sessions_rdbms.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/roundup/backends/sessions_rdbms.py	Fri Apr 20 18:46:28 2018 +0200
@@ -5,7 +5,7 @@
 class. It's now also used for One Time Key handling too.
 """
 __docformat__ = 'restructuredtext'
-import os, time
+import os, time, logging
 from cgi import escape
 
 class BasicDatabase:
@@ -16,7 +16,7 @@
     name = None
     def __init__(self, db):
         self.db = db
-        self.cursor = self.db.cursor
+        self.conn, self.cursor = self.db.sql_open_connection()
 
     def clear(self):
         self.cursor.execute('delete from %ss'%self.name)
@@ -113,6 +113,15 @@
         self.cursor.execute('delete from %ss where %s_time < %s'%(self.name,
             self.name, self.db.arg), (old, ))
 
+    def commit(self):
+        logger = logging.getLogger('roundup.hyperdb.backend')
+        logger.info('commit %s' % self.name)
+        self.conn.commit()
+        self.cursor = self.conn.cursor()
+
+    def close(self):
+        self.conn.close()
+
 class Sessions(BasicDatabase):
     name = 'session'
 
--- a/roundup/cgi/actions.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/roundup/cgi/actions.py	Fri Apr 20 18:46:28 2018 +0200
@@ -911,7 +911,7 @@
                 cl.set(uid, password=password.Password(newpw, config=self.db.config))
                 # clear the props from the otk database
                 otks.destroy(otk)
-                self.db.commit()
+                otks.commit()
             except (ValueError, KeyError) as message:
                 self.client.add_error_message(str(message))
                 return
@@ -965,7 +965,7 @@
         while otks.exists(otk):
             otk = ''.join([random.choice(chars) for x in range(32)])
         otks.set(otk, uid=uid, uaddress=address)
-        self.db.commit()
+        otks.commit()
 
         # send the email
         tracker_name = self.db.config.TRACKER_NAME
--- a/roundup/cgi/client.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/roundup/cgi/client.py	Fri Apr 20 18:46:28 2018 +0200
@@ -182,7 +182,7 @@
         self.client.add_cookie(self.cookie_name, None)
         self._data = {}
         self.session_db.destroy(self._sid)
-        self.client.db.commit()
+        self.session_db.commit()
 
     def get(self, name, default=None):
         return self._data.get(name, default)
@@ -200,7 +200,7 @@
             self.client.session = self._sid
         else:
             self.session_db.set(self._sid, **self._data)
-            self.client.db.commit()
+            self.session_db.commit()
 
     def update(self, set_cookie=False, expire=None):
         """ update timestamp in db to avoid expiration
@@ -212,7 +212,7 @@
                 lifetime is longer
         """
         self.session_db.updateTimestamp(self._sid)
-        self.client.db.commit()
+        self.session_db.commit()
 
         if set_cookie:
             self.client.add_cookie(self.cookie_name, self._sid, expire=expire)
@@ -697,14 +697,15 @@
 
         # XXX: hack - use OTK table to store last_clean time information
         #      'last_clean' string is used instead of otk key
-        last_clean = self.db.getOTKManager().get('last_clean', 'last_use', 0)
+        otks = self.db.getOTKManager()
+        last_clean = otks.get('last_clean', 'last_use', 0)
         if now - last_clean < hour:
             return
 
         self.session_api.clean_up()
-        self.db.getOTKManager().clean()
-        self.db.getOTKManager().set('last_clean', last_use=now)
-        self.db.commit(fail_ok=True)
+        otks.clean()
+        otks.set('last_clean', last_use=now)
+        otks.commit()
 
     def determine_charset(self):
         """Look for client charset in the form parameters or browser cookie.
@@ -982,7 +983,7 @@
                         self._("csrf key used with wrong method from: %s"),
                         referer)
                     otks.destroy(key)
-                    self.db.commit()
+                    otks.commit()
             # do return here. Keys have been obsoleted.
             # we didn't do a expire cycle of session keys, 
             # but that's ok.
@@ -1105,7 +1106,7 @@
 
         if xmlrpc:
             # Save removal of expired keys from database.
-            self.db.commit()
+            otks.commit()
             # Return from here since we have done housekeeping
             # and don't use csrf tokens for xmlrpc.
             return True
@@ -1125,7 +1126,7 @@
             otks.destroy(key)
 
         # commit the deletion/expiration of all keys
-        self.db.commit()
+        otks.commit()
 
         enforce=config['WEB_CSRF_ENFORCE_TOKEN']
         if key is None: # we do not have an @csrf token
@@ -1172,7 +1173,7 @@
            self.form["@action"].value == "Login":
             if header_pass > 0:
                 otks.destroy(key)
-                self.db.commit()
+                otks.commit()
                 return True
             else:
                 self.add_error_message("Reload window before logging in.")
--- a/roundup/cgi/templating.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/roundup/cgi/templating.py	Fri Apr 20 18:46:28 2018 +0200
@@ -104,7 +104,7 @@
     otks.set(key, uid=client.db.getuid(),
              sid=client.session_api._sid,
              __timestamp=ts )
-    client.db.commit()
+    otks.commit()
     return key
 
 ### templating
--- a/roundup/hyperdb.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/roundup/hyperdb.py	Fri Apr 20 18:46:28 2018 +0200
@@ -808,12 +808,6 @@
 
         Save all data changed since the database was opened or since the
         last commit() or rollback().
-
-        fail_ok indicates that the commit is allowed to fail. This is used
-        in the web interface when committing cleaning of the session
-        database. We don't care if there's a concurrency issue there.
-
-        The only backend this seems to affect is postgres.
         """
         raise NotImplementedError
 
--- a/roundup/roundupdb.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/roundup/roundupdb.py	Fri Apr 20 18:46:28 2018 +0200
@@ -128,6 +128,7 @@
         userid = cl.create(**props)
         # clear the props from the otk database
         self.getOTKManager().destroy(otk)
+        # commit cl.create (and otk changes)
         self.commit()
 
         return userid
--- a/test/db_test_base.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/test/db_test_base.py	Fri Apr 20 18:46:28 2018 +0200
@@ -3356,7 +3356,6 @@
             #os.remove(self.SENDMAILDEBUG)
             pass
 
-    @pytest.mark.xfail
     def testInnerMain(self):
         cl = self.client
         cl.session_api = MockNull(_sid="1234567890")
--- a/test/session_common.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/test/session_common.py	Fri Apr 20 18:46:28 2018 +0200
@@ -10,12 +10,10 @@
             shutil.rmtree(config.DATABASE)
         os.makedirs(config.DATABASE + '/files')
         self.db = self.module.Database(config, 'admin')
-        self.sessions = self.sessions_module.Sessions(self.db)
-        self.otks = self.sessions_module.OneTimeKeys(self.db)
+        self.sessions = self.db.getSessionManager()
+        self.otks = self.db.getOTKManager()
 
     def tearDown(self):
-        del self.otks
-        del self.sessions
         if hasattr(self, 'db'):
             self.db.close()
         if os.path.exists(config.DATABASE):
@@ -50,9 +48,3 @@
         self.sessions.set('random_key', text='nope')
         self.assertEqual(self.sessions.get('random_key', 'text'), 'nope')
 
-class DBMTest(SessionTest):
-    import roundup.backends.sessions_dbm as sessions_module
-
-class RDBMSTest(SessionTest):
-    import roundup.backends.sessions_rdbms as sessions_module
-
--- a/test/test_anydbm.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/test/test_anydbm.py	Fri Apr 20 18:46:28 2018 +0200
@@ -48,8 +48,8 @@
     backend = 'anydbm'
 
 
-from session_common import DBMTest
-class anydbmSessionTest(anydbmOpener, DBMTest, unittest.TestCase):
+from session_common import SessionTest
+class anydbmSessionTest(anydbmOpener, SessionTest, unittest.TestCase):
     pass
 
 class anydbmSpecialActionTestCase(anydbmOpener, SpecialActionTest,
--- a/test/test_memorydb.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/test/test_memorydb.py	Fri Apr 20 18:46:28 2018 +0200
@@ -48,8 +48,8 @@
     pass
 
 
-from session_common import DBMTest
-class memorydbSessionTest(memorydbOpener, DBMTest, unittest.TestCase):
+from session_common import SessionTest
+class memorydbSessionTest(memorydbOpener, SessionTest, unittest.TestCase):
     def setUp(self):
         self.db = self.module.Database(config, 'admin')
         setupSchema(self.db, 1, self.module)
--- a/test/test_mysql.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/test/test_mysql.py	Fri Apr 20 18:46:28 2018 +0200
@@ -123,14 +123,14 @@
         self.nuke_database()
 
 
-from session_common import RDBMSTest
+from session_common import SessionTest
 @skip_mysql
-class mysqlSessionTest(mysqlOpener, RDBMSTest, unittest.TestCase):
+class mysqlSessionTest(mysqlOpener, SessionTest, unittest.TestCase):
     def setUp(self):
         mysqlOpener.setUp(self)
-        RDBMSTest.setUp(self)
+        SessionTest.setUp(self)
     def tearDown(self):
-        RDBMSTest.tearDown(self)
+        SessionTest.tearDown(self)
         mysqlOpener.tearDown(self)
 
 @skip_mysql
--- a/test/test_postgresql.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/test/test_postgresql.py	Fri Apr 20 18:46:28 2018 +0200
@@ -203,14 +203,14 @@
         postgresqlOpener.tearDown(self)
 
 
-from session_common import RDBMSTest
+from session_common import SessionTest
 @skip_postgresql
-class postgresqlSessionTest(postgresqlOpener, RDBMSTest, unittest.TestCase):
+class postgresqlSessionTest(postgresqlOpener, SessionTest, unittest.TestCase):
     def setUp(self):
         postgresqlOpener.setUp(self)
-        RDBMSTest.setUp(self)
+        SessionTest.setUp(self)
     def tearDown(self):
-        RDBMSTest.tearDown(self)
+        SessionTest.tearDown(self)
         postgresqlOpener.tearDown(self)
 
 @skip_postgresql
--- a/test/test_sqlite.py	Thu Apr 19 20:01:43 2018 +0200
+++ b/test/test_sqlite.py	Fri Apr 20 18:46:28 2018 +0200
@@ -58,6 +58,6 @@
     backend = 'sqlite'
 
 
-from session_common import RDBMSTest
-class sqliteSessionTest(sqliteOpener, RDBMSTest, unittest.TestCase):
+from session_common import SessionTest
+class sqliteSessionTest(sqliteOpener, SessionTest, unittest.TestCase):
     pass
Roundup Issue Tracker: http://roundup-tracker.org/