Edgewall Software
Modify

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#11978 closed enhancement (fixed)

Add drop_schema method to ConnectionBase class

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.5
Component: database backend Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
  • Added destroy_db method to the DatabaseManager class, the IDatabaseConnector interface and implementations of the IDatabaseConnector interface.
  • The keyword parameters of EnvironmentStub's destroy_db method are deprecated.
Internal Changes:

Description

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

Attachments (1)

t11978-postgres-quote.diff (8.7 KB ) - added by Jun Omae 6 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 by Ryan J Ollos, 6 years ago

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?

comment:2 by Jun Omae, 6 years ago

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

comment:3 by Ryan J Ollos, 6 years ago

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.

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

comment:4 by Jun Omae, 6 years ago

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

comment:5 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

comment:7 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Milestone: next-dev-1.1.x1.1.5
Owner: set to Ryan J Ollos
Release Notes: modified (diff)
Status: newassigned

comment:8 by Jun Omae, 6 years ago

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

    diff --git a/trac/test.py b/trac/test.py
    index 76aa37e..daa3e8f 100755
    a b try:  
    3131    from babel import Locale
    3232except ImportError:
    3333    locale_en = None
    34 finally:
     34else:
    3535    locale_en = Locale.parse('en_US')
    3636
    3737import trac.db.mysql_backend

by Jun Omae, 6 years ago

Attachment: t11978-postgres-quote.diff added

comment:9 by Jun Omae, 6 years ago

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​.

comment:10 by Ryan J Ollos, 6 years ago

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

comment:11 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [13924:13925].

comment:12 by Ryan J Ollos, 6 years ago

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

in reply to:  12 comment:13 by Ryan J Ollos, 6 years ago

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.

comment:14 by Ryan J Ollos, 6 years ago

Resolution: fixed
Status: closedreopened

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

comment:15 by Ryan J Ollos, 5 years ago

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?

in reply to:  15 comment:16 by Jun Omae, 5 years ago

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

    diff --git a/trac/db/sqlite_backend.py b/trac/db/sqlite_backend.py
    index c431f00..e8c8c62 100644
    a b  
    1414#
    1515# Author: Christopher Lenz <cmlenz@gmx.de>
    1616
     17import errno
    1718import os
    1819import re
    1920import weakref
    class SQLiteConnector(Component):  
    223224        if path != ':memory:':
    224225            if not os.path.isabs(path):
    225226                path = os.path.join(self.env.path, path)
    226             os.remove(path)
     227            try:
     228                os.remove(path)
     229            except OSError as e:
     230                if e.errno != errno.ENOENT:
     231                    raise
    227232
    228233    def to_sql(self, table):
    229234        return _to_sql(table)
  • trac/test.py

    diff --git a/trac/test.py b/trac/test.py
    index f06ffa7..48b0f0a 100755
    a b class EnvironmentStub(Environment):  
    335335                db.rollback()  # make sure there's no transaction in progress
    336336                # check the database version
    337337                db_version = dbm.get_database_version()
    338         except TimeoutError:
     338        except (TimeoutError, self.db_exc.DatabaseError):
    339339            pass
    340340        else:
    341341            if db_version == db_default.db_version:
    class EnvironmentStub(Environment):  
    365365        """
    366366        try:
    367367            self.global_databasemanager.destroy_db()
    368         except (TimeoutError, OSError, self.db_exc.DatabaseError):
     368        except (TimeoutError, self.db_exc.DatabaseError):
    369369            pass
    370370        return False
    371371

comment:17 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)

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.

comment:18 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)
Resolution: fixed
Status: reopenedclosed

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

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.