Catch database exceptions in a backend neutral way

[jruigrok]

When you, within trac-admin, add a permission to a user that already has that permission you will receive a Python traceback.

Trac [/usr/local/trac-rangaku]> permission list

User           Action
-------------------------------
asmodai        MILESTONE_ADMIN
asmodai        PERMISSION_ADMIN
asmodai        TICKET_ADMIN
asmodai        TRAC_ADMIN

Trac [/usr/local/trac-rangaku]> permission add asmodai MILESTONE_ADMIN
Traceback (most recent call last):
  File "/usr/local/bin/trac-admin", line 8, in <module>
    load_entry_point('Trac==0.11dev-r6153', 'console_scripts', 'trac-admin')()
  File "/usr/local/lib/python2.5/site-packages/Trac-0.11dev_r6153-py2.5.egg/trac/admin/console.py", line 1190, in run
    admin.run()
  File "/usr/local/lib/python2.5/site-packages/Trac-0.11dev_r6153-py2.5.egg/trac/admin/console.py", line 119, in run
    self.cmdloop()
  File "/usr/local/lib/python2.5/cmd.py", line 142, in cmdloop
    stop = self.onecmd(line)
  File "/usr/local/lib/python2.5/site-packages/Trac-0.11dev_r6153-py2.5.egg/trac/admin/console.py", line 102, in onecmd
    rv = cmd.Cmd.onecmd(self, line) or 0
  File "/usr/local/lib/python2.5/cmd.py", line 219, in onecmd
    return func(arg)
  File "/usr/local/lib/python2.5/site-packages/Trac-0.11dev_r6153-py2.5.egg/trac/admin/console.py", line 393, in do_permission
    self._do_permission_add(user, action)
  File "/usr/local/lib/python2.5/site-packages/Trac-0.11dev_r6153-py2.5.egg/trac/admin/console.py", line 429, in _do_permission_add
    self._permsys.grant_permission(user, action)
  File "/usr/local/lib/python2.5/site-packages/Trac-0.11dev_r6153-py2.5.egg/trac/perm.py", line 274, in grant_permission
    self.store.grant_permission(username, action)
  File "/usr/local/lib/python2.5/site-packages/Trac-0.11dev_r6153-py2.5.egg/trac/perm.py", line 204, in grant_permission
    (username, action))
  File "/usr/local/lib/python2.5/site-packages/Trac-0.11dev_r6153-py2.5.egg/trac/db/util.py", line 50, in execute
    return self.cursor.execute(sql_escape_percent(sql), args)
  File "/usr/local/lib/python2.5/site-packages/Trac-0.11dev_r6153-py2.5.egg/trac/db/sqlite_backend.py", line 58, in execute
    args or [])
  File "/usr/local/lib/python2.5/site-packages/Trac-0.11dev_r6153-py2.5.egg/trac/db/sqlite_backend.py", line 50, in _rollback_on_error
    return function(self, *args, **kwargs)
pysqlite2.dbapi2.OperationalError: SQL logic error or missing database

Please note that the database does exist and initial permission assignments work as intended.

[cboos]

We should transform DB integrity exception in a backend independent TracIntegrityError that we could handle at higher levels.

The problem is that it's not always obvious to detect what is an integrity error.

In the above you had:

pysqlite2.dbapi2.OperationalError: SQL logic error or missing database

whereas on my side I have:

pysqlite2.dbapi2.IntegrityError: columns username, action are not unique

... which is more specific.
What versions of SQLite / PySqlite do you use?

[jruigrok]

On FreeBSD 6.2-STABLE:

SQLite 3.4.1
PySQLite 2.5.1

[cboos]

#6095 closed as duplicate.

We must transform the SQL-specific exception into a TracIntegrityError that we can intercept at a higher level, see discussion in ​googlegroups:trac-dev:2f82a333d6b9eaa7.

[cboos]

See also #5445.

[osimons]

#6841 closed as duplicate.

[cboos]

See also #6808 (that one is not a duplicate though, as it's dedicated to fixing the way we can try to avoid the exception to happen in the first place).

[osimons]

#7351 closed as duplicate (Traceback when renaming a milestone to an existing name).

[cboos]

#7754 closed as duplicate.

Interestingly, the traceback is mostly identical as the one in the current description, but the exception is different:

pysqlite2.dbapi2.IntegrityError: columns username, action are not unique

vs.

pysqlite2.dbapi2.OperationalError: SQL logic error or missing database

which highlights the need to handle all those db exception in a more generic way.

[cboos]

#8039 closed as duplicate.

[cboos]

#7931 closed as duplicate (another "create milestone with already existing name" one).

[tlk]

See also #4984

[cboos]

See also issue #8379 (rollback on error).

[cboos]

Can wait for 0.12.1.

[cboos]

#8379 was closed as duplicate.

[Carsten Klein <carsten.klein@…>]

with trac-0.12dev i get

IntegrityError?: columns username, action are not unique

however, this should rather be

"The user 'user' already has the permission 'permission'."

or something similar.

I do not get an exception stack trace though.

[cboos]

Another duplicate, #9153.

[cboos]

Another postponing... Anyway, this didn't really fit in a minor release.

[psuter <petsuter@…>]

This may be foolish and naive, but I experimented with this in a ​forked branch on bitbucket.

Assumptions:

• All exceptions defined in ​PEP 249 (the Python DB API) should be wrapped individually.
• All backend APIs implement these.
  • MySQL: ​MySQLdb documentation, ​MySQLdb source
  • PostgreSQL: ​psycopg2 documentation, ​psycopg2 source
  • SQLite: ​sqlite3 documentation, ​pysqlite source
• Only cursor.execute() or cursor.executemany() calls throw exceptions that have to be translated, and this can be done in the cursor wrappers of the db backend. (PyFormatCursor for SQLite, MySQLUnicodeCursor for MySQL and a new cursor wrapper for PostgresSQL)
• Defining a exception mapping dict and utility function is nicer than manually mapping all those exceptions.

(Most of these seem to be what the ​Django DB wrappers do. Except they ​only wrap IntegrityError and DatabaseError (​yet). And they wrap each exception manually (​example). Interestingly, they map some exceptions around, e.g. ​MySQL OperationalError 1048 to IntegrityError.)

So far I only experimented with the SQLite backend and the specific example originally reported in this ticket (duplicate permission add). But at least that seemed to work well, and I don't see why this could not work in general.

Or am I completely on the wrong track? :)

[cboos]

This seems to be a very nice approach and I don't see why it couldn't work.
Of course there might be some corner cases, like the one mentioned in comment:1. The map for Pysqlite could somehow take the exact version into account, though it might not be easy to find which exact version introduced which change of behavior. One approach would be to verify what happens with the builtin sqlite3 module of Python 2.5, 2.6 and 2.7, each having a different version and potentially a slightly different mapping. If OTOH, Python 2.5 behaves like the others and the problem mentioned in comment:1 is no longer an issue, then nevermind, keep a simple 1-1 mapping.

So by all means have a try at wrapping also exceptions for MySQL and PostgreSQL and I'll do my best to test your changes.

(re-assigning from me to you)

[psuter <petsuter@…>]

According to the Python release notes the following pysqlite versions were bundled:

Python release	Bundled pysqlite	Release notes
2.5	2.1.3	"​Added the sqlite3 package. This is based on pysqlite2.1.3"
2.6	2.4.1	"​sqlite3 module, [...] updated from version 2.3.2[???] in Python 2.5 to version 2.4.1."
2.7	2.6.0	"​sqlite3 module has been updated to version 2.6.0 of the pysqlite"

The mapping of SQLite eror codes to exception types in pysqlite has last changed in ​May 2005, before pysqlite 2.0, so all bundled versions should use the same mapping.

---

So maybe the difference is in SQLite itself?

Error message	Exception type	SQLite error code	SQLite source
"SQL logic error or missing database"	OperationalError	SQLITE_ERROR	General SQLite message defined in ​src/main.c
"[...] not unique"	IntegrityError	SQLITE_CONSTRAINT	Specific SQLite message constructed in ​src/insert.c

However I don't see anything related in their ​release notes. One may really have to experimentally confirm by installing the different versions.

---

Note also ​pysqlite issue 7394: "sqlite3: some OperationalError exceptions should be ProgrammingError" (wontfix, not feasible to differentiate)

---

Another approach would be to expect the more general DatabaseError which both derive from.

[psuter <petsuter@…>]

At least a superficial test with the latest available binary Python 2.5 release suggests it raises IntegrityError (identical to 2.6.6 and 2.7.1):

C:\Python25>python.exe
Python 2.5.4 (r254:67916, Dec 23 2008, 15:10:54) [MSC v.1310 32 bit (Intel)] on
win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import sqlite3
>>> cnx=sqlite3.connect(":memory:")
>>> cur=cnx.cursor()
>>> cur.execute("CREATE TABLE t (a, b, UNIQUE (a, b));")
<sqlite3.Cursor object at 0x025635C0>
>>> cur.execute("INSERT INTO t VALUES (1, 1);")
<sqlite3.Cursor object at 0x025635C0>
>>> cur.execute("INSERT INTO t VALUES (1, 1);")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
sqlite3.IntegrityError: columns a, b are not unique
>>>

[psuter <petsuter@…>]

Replying to cboos:

So by all means have a try at wrapping also exceptions for MySQL and PostgreSQL and I'll do my best to test your changes.

​My branch now also wraps exceptions for MySQL and PostgreSQL. I again tested only the duplicate permission add case. I tested both on Windows with Python 2.5.4.

For MySQL testing I installed MySQL 5.5.13 and ​MySQL_python-1.2.2-py2.5-win32.egg. For PostgreSQL I installed PostgreSQL 9.0.4-1 and ​win-psycopg 2.4.2. Both seemed to work fine.

[rblank]

The code looks very nice. I have two comments:

• In _maperror(), you iterate over reversed(self.errormap.keys()), but the order of keys in a dict is undefined, so you may get a superclass before a child class in that loop. Also, you should use et in self.errormap instead of self.errormap.has_key(et).
• I wonder if it wouldn't be more readable to catch the errors "manually", as they do in Django. Sure, it would introduce some code duplication, as you would have to have the same snippet in both execute() and executemany(). But you wouldn't have to check for superclasses, and the exception mapping is constant per DB backend anyway (i.e. no version-specific maps).

[psuter <petsuter@…>]

Replying to rblank:

• In _maperror(), you iterate over reversed(self.errormap.keys()), but the order of keys in a dict is undefined, so you may get a superclass before a child class in that loop. Also, you should use et in self.errormap instead of self.errormap.has_key(et).

Oops.

• I wonder if it wouldn't be more readable to catch the errors "manually", as they do in Django. Sure, it would introduce some code duplication, as you would have to have the same snippet in both execute() and executemany(). But you wouldn't have to check for superclasses, and the exception mapping is constant per DB backend anyway (i.e. no version-specific maps).

Simply like this?

try:
    ...
except MySQLdb.ProgrammingError, e:
    raise raise trac.db.ProgrammingError, trac.db.ProgrammingError(*tuple(e)), sys.exc_info()[2]
except MySQLdb.IntegrityError, e:
    raise raise trac.db.IntegrityError, trac.db.IntegrityError(*tuple(e)), sys.exc_info()[2]
except MySQLdb.DataError, e:
    raise raise trac.db.DataError, trac.db.DataError(*tuple(e)), sys.exc_info()[2]
except MySQLdb.NotSupportedError, e:
    raise raise trac.db.NotSupportedError, trac.db.NotSupportedError(*tuple(e)), sys.exc_info()[2]
except MySQLdb.OperationalError, e:
    raise raise trac.db.OperationalError, trac.db.OperationalError(*tuple(e)), sys.exc_info()[2]
except MySQLdb.InternalError, e:
    raise raise trac.db.InternalError, trac.db.InternalError(*tuple(e)), sys.exc_info()[2]
except MySQLdb.DatabaseError, e:
    raise raise trac.db.DatabaseError, trac.db.DatabaseError(*tuple(e)), sys.exc_info()[2]
except MySQLdb.InterfaceError, e:
    raise raise trac.db.InterfaceError, trac.db.InterfaceError(*tuple(e)), sys.exc_info()[2]
except MySQLdb.Error, e:
    raise raise trac.db.Error, trac.db.Error(*tuple(e)), sys.exc_info()[2]
except MySQLdb.Warning, e:
    raise raise trac.db.Warning, trac.db.Warning(*tuple(e)), sys.exc_info()[2]

In light of your first observation this may indeed turn out to be the simpler approach.

[rblank]

Replying to psuter <petsuter@…>:

Simply like this?

Yes, something like that (you've got your raise doubled). It's not pretty, but probably quite effective.

[psuter <petsuter@…>]

​db-exceptions-manual branch implements the "manual" try: ... except ... approach. (Sorry about the double raise copy-paste blunder.)

I also updated the original ​db-exceptions branch in ​an attempt to save the dict approach. (It may still be useful if it turns out exception mapping is needed in other cases besides execute and executemany.)

[osimons]

Great work! I've skimmed the current fork diff and it makes sense to me. The simple exception remapping is much more accessible than hiding it all in a DbErrorMapContextManager class.

I think errors should not be defined in a new trac.db.errors module. Exceptions should be defined in the existing trac.db.api module - it is very much part of the API that backends need to support. And, as part of the API the exception classes needs to be documented so it becomes clear what their usage and differences are.

[rblank]

Replying to osimons:

I think errors should not be defined in a new trac.db.errors module. Exceptions should be defined in the existing trac.db.api module

+1

[psuter <petsuter@…>]

Replying to osimons:

the exception classes needs to be documented so it becomes clear what their usage and differences are.

Do you mean docstrings? Or something like doc/api/trac_db.rst? Could we just copy the descriptions from the ​Python DB API?

...

• IntegrityError: Exception raised when the relational integrity of the database is affected, e.g. a foreign key check fails.
• InternalError: Exception raised when the database encounters an internal error, e.g. the cursor is not valid anymore, the transaction is out of sync, etc.

...

Or what do you have in mind?

[psuter <petsuter@…>]

Sorry, that was supposed to be ​Python DB API.

[osimons]

Ah, so if it is all based on ​pep:0249 then just a helpful link should be all that is needed in our source/docs.

[psuter <petsuter@…>]

​Updated: Merged trac.db.errors into trac.db.api and added the docstrings / link. Bonus: Handle renaming milestone (e.g. duplicate #7351 from above), component, version and enum to already existing names.

[osimons]

Nice! Looks good & ready to me.

[rblank]

I have tried to get rid of the duplication (on the db-exceptions-manual branch) by moving the exception remapping into a function. This works nicely, but the duplication of the whole exception hierarchy still feels a bit wrong.

So, how about not remapping exceptions at all (on the db-exceptions-nomap branch)? The idea is to extend the IDatabaseConnector with a function returning an object containing all exception types as attributes (typically the DB module). This object is then accessed through env.db_exc:

try:
    comp.update()
except self.env.db_exc.IntegrityError:
    raise TracError(_('Component "%(name)s" already '
                      'exists.', name=name))

I see the following advantages:

• Access is nicely symmetric with the access to the DB context managers (env.db_transaction and env.db_query).
• Avoids all duplication.
• Has no runtime cost.

Thoughts?

[psuter <petsuter@…>]

Replying to rblank:

I have tried to get rid of the duplication (on the db-exceptions-manual branch) by moving the exception remapping into a function.

Sounds almost like what I started with (even before I switched to the context manager), just much nicer as I didn't think of the pass in the module trick or the nice reraise method.

the duplication of the whole exception hierarchy still feels a bit wrong. So, how about not remapping exceptions at all

Looks very clean and simple. It seems a bit less flexible if one ever needs to remap exceptions (as in ​django), but maybe that's something we don't really need and want to avoid anyway? Still my favorite approach so far.

[rblank]

Replying to psuter <petsuter@…>:

Looks very clean and simple. It seems a bit less flexible if one ever needs to remap exceptions (as in ​django),

Not necessarily, we can still do the remapping in the .execute*() methods of the backends, for only those cases where we want it.

One thing I forgot to mention, the first tries here have all focused on the .execute*() methods. I assume it's also possible to get exceptions in the .fetch*() methods, and only the last approach handles those correctly. That was actually the starting point of the idea.

[psuter <petsuter@…>]

Replying to rblank:

One thing I forgot to mention, the first tries here have all focused on the .execute*() methods. I assume it's also possible to get exceptions in the .fetch*() methods, and only the last approach handles those correctly. That was actually the starting point of the idea.

Good point. As I mentioned in my first comment here the initial assumption was that handling .execute*() would be enough in practice, mainly based on the observation that django only seems to handle those. If that assumption turned out to be false, I hoped the reusable error mapping would help to extend the scheme to any additional cases.

But your approach nicely circumvents the entire issue.

[rblank]

I have committed the last approach in [10737]. Also, instead of showing an error and bailing out when adding a permission that was already granted, I print a message but continue with the following permissions. I feel that this strategy is more useful to the user.

Peter, did you want to go through other parts of the code and add relevant try: except: blocks to display more sensible error messages? Or are all the most frequent cases covered?

[psuter <petsuter@…>]

I'm not aware of any other remaining cases. All the duplicate tickets mentioned here are covered as far as I can tell. (Some scenarios were already fixed by e.g. [6758] ... [6775].)

[rblank]

Time to close, in that case :)