Check for table existence in DatabaseManager.upgrade_tables

[Ryan J Ollos]

If upgrade_tables checked for table existence before attempting to copy the data to a temporary table, then upgrade_tables could handle a mix of new and revised tables. This would make the upgrade function slightly similar for a case such as that demonstrated in db4.py for SpamFilter​.

[Ryan J Ollos]

Committed to trunk in r15316.

[Jun Omae]

I think the table4 should be removed in ModifyTableTestCase.tearDown().

In addition, table names in MySQL on Windows are case-insensitive. The changes in [15316] may not work as expected.

.get_column_names() with non-existent table returns an empty list. We could check whether the returned value is empty.

trac/db/api.py

@@ -357 +357 @@
 
         :since: version 1.2
         """
-        existing_table_names = self.get_table_names()
         for new_table in new_schema:
             temp_table_name = new_table.name + '_old'
             old_column_names = set(self.get_column_names(new_table))
             new_column_names = {col.name for col in new_table.columns}
             cols_to_copy = ','.join(old_column_names & new_column_names)
             with self.env.db_transaction as db:
-                has_table = new_table.name in existing_table_names
+                has_table = bool(old_column_names)
                 if has_table:
                     cursor = db.cursor()
                     cursor.execute("""

I think .get_column_names() may be better to raise an exception if the given table is not existent….

[Ryan J Ollos]

Replying to Jun Omae:

I think the table4 should be removed in ModifyTableTestCase.tearDown().

Make sense. I'm sure I had added this, so I'm stumped as to why it wasn't in my final changes.

In addition, table names in MySQL on Windows are case-insensitive. The changes in [15316] may not work as expected.

Perhaps we should add has_table(...) method to DatabaseManager and ConnectionBase?

I think .get_column_names() may be better to raise an exception if the given table is not existent….

I agree with .get_column_names() raising exception when table doesn't exist.

[Jun Omae]

Replying to Ryan J Ollos:

Replying to Jun Omae:

I think the table4 should be removed in ModifyTableTestCase.tearDown().

Make sense. I'm sure I had added this, so I'm stumped as to why it wasn't in my final changes.

See [b051fb622/jomae.git].

In addition, table names in MySQL on Windows are case-insensitive. The changes in [15316] may not work as expected.

Perhaps we should add has_table(...) method to DatabaseManager and ConnectionBase?

Sounds good. See [ead3a3383/jomae.git].

I think .get_column_names() may be better to raise an exception if the given table is not existent….

I agree with .get_column_names() raising exception when table doesn't exist.

Thanks. I consider .drop_columns() also should raise an exception with missing table, [f03bc446c/jomae.git].

Proposed changes in jomae.git@t12643.1.

[Ryan J Ollos]

Looks good, thanks for fixing. I might suggest changing the error message to Table "blah" not found, consistent with tags/trac-1.2/trac/db/sqlite_backend.py@:302#L298​.

[Jun Omae]

Thanks for the suggesting. Committed changes with your suggestions in [15322].