Add drop_schema method to ConnectionBase class

[Ryan J Ollos]

After adding the method we can refactor the code in EnvironmentStub to eliminate the SQL query: trunk/trac/test.py@13818:355#L349​.

[Ryan J Ollos]

Or we could make the table argument of drop_table default to None and drop all the tables when table = None: trunk/trac/db/api.py@13595:200#L184​. Is one of the two approaches more preferable?

[Jun Omae]

I like first approach and destroy_db rather than drop_schema. init_db method already exists and only PostgreSQL uses schema.

[Ryan J Ollos]

Should we add the destroy_db method to the DatabaseManager class (tags/trac-1.1.3/trac/db/api.py@:324#L299​) and IDatabaseConnector interface (tags/trac-1.1.3/trac/db/api.py@:287#L262​)? I started working on a patch with those changes and I'll finish it after the 1.1.4 release.

[Jun Omae]

Well, I think that it would be good to add the destroy_db method to those class and interface.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11978-add-destroy_db-method.

[Jun Omae]

At least, I think use of finally statement is wrong in [f247fb2d7/rjollos.git].

Traceback (most recent call last):
  File "./trac/test.py", line 35, in <module>
    locale_en = Locale.parse('en_US')
NameError: name 'Locale' is not defined

trac/test.py

@@ -31 +31 @@
     from babel import Locale
 except ImportError:
     locale_en = None
-finally:
+else:
     locale_en = Locale.parse('en_US')
 
 import trac.db.mysql_backend

[Jun Omae]

Another thing: to_sql and alter_column_types methods of PostgresConnector have the same issue in [d21966f3/rjollos.git]. Proposed changes in attachment:t11978-postgres-quote.diff​​.

[Ryan J Ollos]

Thanks, I made ticket #11998 so we can fix the quoting issues on 1.0-stable.

[Ryan J Ollos]

Committed to trunk in [13924:13925].

[Ryan J Ollos]

Removed use of deprecated EnvironmentStub.destroy_db in [13943].

[Ryan J Ollos]

Replying to rjollos:

Removed use of deprecated EnvironmentStub.destroy_db in [13943].

Test cases have failed, so some modification is needed. I'll post proposed changes shortly. Sorry for the test failures.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t11978.1, including a few unrelated changes.

[Ryan J Ollos]

I should commit these changes since tests are still failing on the trunk. I've been debating [792052f2/rjollos.git], whether to just catch Exception, or to be more specific, as in the patch that catches (TimeoutError, OSError, self.db_exc.DatabaseError). Although another option is to have the IDatabaseConnector implementation catch the database-specific exceptions (e.g. with SQLite an OSError can result), and raise a TracError.

Any thoughts on which approach is best?

[Jun Omae]

Replying to rjollos:

I should commit these changes since tests are still failing on the trunk. I've been debating [792052f2/rjollos.git], whether to just catch Exception, or to be more specific, as in the patch that catches (TimeoutError, OSError, self.db_exc.DatabaseError). Although another option is to have the IDatabaseConnector implementation catch the database-specific exceptions (e.g. with SQLite an OSError can result), and raise a TracError.

Any thoughts on which approach is best?

I consider that we shouldn't raise OSError from SQLiteConnector.destroy_db when the file is missing. Instead, we should trap ENOENT error in the method. So, I think we should catch only the database-specific exceptions and TimeoutError.

trac/db/sqlite_backend.py

@@ -14 +14 @@
 #
 # Author: Christopher Lenz <cmlenz@gmx.de>
 
+import errno
 import os
 import re
 import weakref
@@ -223 +224 @@
         if path != ':memory:':
             if not os.path.isabs(path):
                 path = os.path.join(self.env.path, path)
-            os.remove(path)
+            try:
+                os.remove(path)
+            except OSError as e:
+                if e.errno != errno.ENOENT:
+                    raise
 
     def to_sql(self, table):
         return _to_sql(table)

trac/test.py

@@ -335 +335 @@
                 db.rollback()  # make sure there's no transaction in progress
                 # check the database version
                 db_version = dbm.get_database_version()
-        except TimeoutError:
+        except (TimeoutError, self.db_exc.DatabaseError):
             pass
         else:
             if db_version == db_default.db_version:
@@ -365 +365 @@
         """
         try:
             self.global_databasemanager.destroy_db()
-        except (TimeoutError, OSError, self.db_exc.DatabaseError):
+        except (TimeoutError, self.db_exc.DatabaseError):
             pass
         return False
 

[Ryan J Ollos]

Thank you for the refinements. Committed to trunk in [13957:13958].

A frequent mistake that I repeated in [13957] is not having a valid Makefile.cfg, thinking that I've run the tests successfully with one of the databases, but they've actually only been executed for in-memory SQLite.

[Ryan J Ollos]

Changed verbosity of functional tests executed by Makefile in [13963], merged to trunk in [13964].