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: schlatterbeck

CHANGES.txt

@@ -493,6 +493,12 @@ Fixed:
   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
 

roundup/backends/back_anydbm.py

@@ -191,6 +191,9 @@ def __init__(self, config, journaltag=None):
         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 @@ def refresh_database(self):
         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 @@ def pack(self, pack_before):
     #
     # 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)))

roundup/backends/back_mysql.py

@@ -563,7 +563,7 @@ def create_class(self, spec):
         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')

roundup/backends/back_postgresql.py

@@ -52,7 +52,7 @@ def db_create(config):
     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 @@ class Database(rdbms_common.Database):
     # 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 @@ def open_connection(self):
             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 add_actor_column(self):
     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... '''

roundup/backends/back_sqlite.py

@@ -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 @@ class Database(rdbms_common.Database):
         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 sql_rollback(self):
     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.

roundup/backends/rdbms_common.py

@@ -189,6 +189,10 @@ def __init__(self, config, journaltag=None):
         # 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 @@ def clearCache(self):
         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 @@ def pack(self, pack_before):
                 "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 @@ def sql_commit(self, fail_ok=False):
         # 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 @@ def close(self):
         """
         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

roundup/backends/sessions_rdbms.py

@@ -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 @@ class BasicDatabase:
     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 @@ def clean(self):
         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'
 

roundup/cgi/actions.py

@@ -911,7 +911,7 @@ def handle(self):
                 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 @@ def handle(self):
         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

roundup/cgi/client.py

@@ -182,7 +182,7 @@ def destroy(self):
         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 @@ def set(self, **kwargs):
             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 @@ def update(self, set_cookie=False, expire=None):
                 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 @@ def clean_up(self):
 
         # 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 @@ def handle_csrf(self, xmlrpc=False):
                         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 @@ def handle_csrf(self, xmlrpc=False):
 
         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 @@ def handle_csrf(self, xmlrpc=False):
             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 @@ def handle_csrf(self, xmlrpc=False):
            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.")

roundup/cgi/templating.py

@@ -104,7 +104,7 @@ def anti_csrf_nonce(self, client, lifetime=None):
     otks.set(key, uid=client.db.getuid(),
              sid=client.session_api._sid,
              __timestamp=ts )
-    client.db.commit()
+    otks.commit()
     return key
 
 ### templating