issue2551142 - Import of retired node ... unique constraint failure.

Title: Import of retired node with username after active node fails
with unique constraint failure.

More fixes needed for mysql and postgresql.

mysql: add unique constraint for (keyvalue, __retired__) when
       creating class in the database.

       On schema change if class is changed, remove the unique
       constraint too.

       upgrade version of rdbms database from 5 to 6 to add constraint
       to all version 5 databases that were created as version 5
       and didn't get the unique constraint. Make no changes
       on version 5 databases upgraded from version 4, the upgrade
       process to 5 added the constraint. Make no changes
       to other databases (sqlite, postgres) during upgrade from
       version 5 to 6.

postgres: Handle the exception raised on unique constraint violation.
          The exception invalidates the database connection so it
          can't be used to recover from the exception.

	  Added two new database methods:

	     checkpoint_data - performs a db.commit under postgres
                               does nothing on other backends
	     restore_connection_on_error - does a db.rollback on
                               postgres, does nothing on other
                               backends

	 with the rollback() done on the connection I can use the
	 database connection to fixup the import that failed on the
	 unique constraint. This makes postgres slower but without the
	 commit after every imported object, the rollback will delete
	 all the entries done up to this point.

	 Trying to figure out how to make the caller do_import batch
	 and recover from this failure is beyond me.

         Also dismissed having to process the export csv file before
         importing. Pushing that onto a user just seems wrong.  Also
         since import/export isn't frequently done the lack of
         surprise on having a failing import and reduced
         load/frustration for the user seems worth it. Also the import
         can be run in verbose mode where it prints out a row as it is
         processed, so it may take a while, ut the user can get
         feedback.

db_test-base.py: add test for upgrade from 5 to 6.

Author: rouilj

roundup/backends/back_mysql.py

@@ -395,6 +395,21 @@ def fix_version_2_tables(self):
         # Convert all String properties to TEXT
         self._convert_string_properties()
 
+    def fix_version_5_tables(self):
+        # A bug caused the _<class>_key_retired_idx to be missing
+        # unless the database was upgraded from version 4 to 5.
+        # If it was created at version 5, the index is missing.
+        # The user class is always present and has a key.
+        # Check it for the index. If missing, add index to all
+        # classes by rerunning self.fix_version_4_tables().
+
+        # if this fails abort. Probably means no user class
+        # so we should't be doing anything.
+        if not self.sql_index_exists("_user", "_user_key_retired_idx"):
+            self.fix_version_4_tables()
+        else:
+            self.log_info('No changes needed.')
+
     def __repr__(self):
         return '<myroundsql 0x%x>'%id(self)
 
@@ -446,6 +461,10 @@ def create_class_table_indexes(self, spec):
                         spec.classname, idx)
             self.sql(index_sql3)
 
+            # and the unique index for key / retired(id)
+            self.add_class_key_required_unique_constraint(spec.classname,
+                                                          spec.key)
+
         # TODO: create indexes on (selected?) Link property columns, as
         # they're more likely to be used for lookup
 
@@ -530,6 +549,12 @@ def drop_class_table_key_index(self, cn, key):
         sql = 'drop index %s on %s'%(index_name, table_name)
         self.sql(sql)
 
+        # and now the retired unique index too
+        index_name = '_%s_key_retired_idx' % cn
+        if self.sql_index_exists(table_name, index_name):
+            sql = 'drop index %s on _%s'%(index_name, cn)
+            self.sql(sql)
+
     # old-skool id generation
     def newid(self, classname):
         ''' Generate a new id for the given class

roundup/backends/back_postgresql.py

@@ -200,6 +200,22 @@ def open_connection(self):
             # the necessary tables (in a parallel connection!)
             self.commit()
 
+    def checkpoint_data(self):
+        """Commit the state of the database. Allows recovery/retry
+           of operation in exception handler because postgres
+           requires a rollback in case of error generating exception
+        """
+        self.commit()
+
+    def restore_connection_on_error(self):
+        """Postgres leaves a cursor in an unusable state after
+           an error. Rollback the transaction to recover and
+           permit a retry of the failed statement. Used with
+           checkpoint_data to handle uniqueness conflict in
+           import_table()
+        """
+        self.rollback()
+
     def create_version_2_tables(self):
         # OTK store
         self.sql('''CREATE TABLE otks (otk_key VARCHAR(255),

roundup/backends/rdbms_common.py

@@ -330,7 +330,7 @@ def post_init(self):
 
     # update this number when we need to make changes to the SQL structure
     # of the backen database
-    current_db_version = 5
+    current_db_version = 6
     db_version_updated = False
 
     def upgrade_db(self):
@@ -372,6 +372,10 @@ def upgrade_db(self):
             self.log_info('upgrade to version 5')
             self.fix_version_4_tables()
 
+        if version < 6:
+            self.log_info('upgrade to version 6')
+            self.fix_version_5_tables()
+
         self.database_schema['version'] = self.current_db_version
         self.db_version_updated = True
         return 1
@@ -399,6 +403,14 @@ def fix_version_4_tables(self):
             if klass.key:
                 self.add_class_key_required_unique_constraint(cn, klass.key)
 
+    def fix_version_5_tables(self):
+        # Default (used by sqlite, postgres): NOOP
+        # mysql overrides this because it is missing
+        # _<class>_key_retired_idx index used to make
+        # sure that the key is unique if it was created
+        # as version 5.
+        pass
+
     def _convert_journal_tables(self):
         """Get current journal table contents, drop the table and re-create"""
         c = self.cursor
@@ -467,6 +479,21 @@ def reindex(self, classname=None, show_progress=False):
                     klass.index(nodeid)
         self.indexer.save_index()
 
+    def checkpoint_data(self):
+        """Call if you need to commit the state of the database
+           so you can try to fix the error rather than rolling back
+
+           Needed for postgres when importing data.
+        """
+        pass
+
+    def restore_connection_on_error(self):
+        """on a database error/exception recover the db connection
+           if left in an unusable state (e.g. postgres requires
+           a rollback).
+        """
+        pass
+
     # Used here in the generic backend to determine if the database
     # supports 'DOUBLE PRECISION' for floating point numbers.
     implements_double_precision = True
@@ -3187,19 +3214,21 @@ def import_list(self, propnames, proplist):
                 self.db.addnode(self.classname, newid, d) # insert
             else:
                 self.db.setnode(self.classname, newid, d) # update
+            self.db.checkpoint_data()
         # Blech, different db's return different exceptions
         # so I can't list them here as some might not be defined
         # on a given system. So capture all exceptions from the
         # code above and try to correct it. If it's correctable its
         # some form of Uniqueness Failure/Integrity Error otherwise
         # undo the fixup and pass on the error.
-        except Exception as e:
+        except Exception as e:  # nosec
             logger.info('Attempting to handle import exception '
                         'for id %s: %s' % (newid,e))
 
             keyname = self.db.user.getkey()
             if has_node or not keyname:  # Not an integrity error
                 raise
+            self.db.restore_connection_on_error()
             activeid = self.db.user.lookup(d[keyname])
             self.db.sql(retired_sql, (-1, activeid)) # clear the active node
             # this can only happen on an addnode, so retry

test/db_test_base.py

@@ -242,6 +242,59 @@ def inject_fixtures(self, caplog):
     def testRefresh(self):
         self.db.refresh_database()
 
+    
+    def testUpgrade_5_to_6(self):
+
+        if(self.db.dbtype in ['anydbm', 'memorydb']):
+           self.skipTest('No schema upgrade needed on non rdbms backends')
+
+        # load the database
+        self.db.issue.create(title="flebble frooz")
+        self.db.commit()
+
+        self.assertEqual(self.db.database_schema['version'], 6,
+                         "This test only runs for database version 6")
+        self.db.database_schema['version'] = 5
+        if self.db.dbtype == 'mysql':
+            # version 6 has 5 indexes
+            self.db.sql('show indexes from _user;')
+            self.assertEqual(5,len(self.db.cursor.fetchall()),
+                             "Database created with wrong number of indexes")
+
+            self.drop_key_retired_idx()
+
+            # after dropping (key.__retired__) composite index we have
+            # 3 index entries
+            self.db.sql('show indexes from _user;')
+            self.assertEqual(3,len(self.db.cursor.fetchall()))
+
+            # test upgrade adding index
+            self.db.post_init()
+
+            # they're back
+            self.db.sql('show indexes from _user;')
+            self.assertEqual(5,len(self.db.cursor.fetchall()))
+
+            # test a database already upgraded from 4 to 5
+            # so it has the index to enforce key uniqueness
+            self.db.database_schema['version'] = 5
+            self.db.post_init()
+
+            # they're still here.
+            self.db.sql('show indexes from _user;')
+            self.assertEqual(5,len(self.db.cursor.fetchall()))
+        else:
+            # this should be a no-op
+            # test upgrade
+            self.db.post_init()
+
+    def drop_key_retired_idx(self):
+        c = self.db.cursor
+        for cn, klass in self.db.classes.items():
+            if klass.key:
+                sql = '''drop index _%s_key_retired_idx on _%s''' % (cn, cn)
+                self.db.sql(sql)
+
     #
     # automatic properties (well, the two easy ones anyway)
     #
@@ -2901,12 +2954,24 @@ def testImportExport(self):
 
             if self.db.dbtype not in ['anydbm', 'memorydb']:
                 # no logs or fixup needed under anydbm
-                self.assertEqual(2, len(self._caplog.record_tuples))
+                # postgres requires commits and rollbacks
+                # as part of error recovery, so we get commit
+                # logging that we need to account for
+                if self.db.dbtype == 'postgres':
+                    log_count=24
+                    handle_msg_location=16
+                    # add two since rollback is logged
+                    success_msg_location = handle_msg_location+2
+                else:
+                    log_count=2
+                    handle_msg_location=0
+                    success_msg_location = handle_msg_location+1
+                self.assertEqual(log_count, len(self._caplog.record_tuples))
                 self.assertIn('Attempting to handle import exception for id 7:',
-                              self._caplog.record_tuples[0][2])
+                              self._caplog.record_tuples[handle_msg_location][2])
                 self.assertIn('Successfully handled import exception for id 7 '
                               'which conflicted with 6',
-                              self._caplog.record_tuples[1][2])
+                              self._caplog.record_tuples[success_msg_location][2])
 
             # This is needed, otherwise journals won't be there for anydbm
             self.db.commit()