Edgewall Software
Modify

Opened 14 years ago

Closed 10 years ago

Last modified 10 years ago

#6348 closed defect (fixed)

Catch database exceptions in a backend neutral way

Reported by: Jeroen Ruigrok van der Werven Owned by: Peter Suter
Priority: high Milestone: 1.0
Component: database backend Version: devel
Severity: major Keywords: db integrityerror needmajor
Cc: felix.schwarz@… Branch:
Release Notes:

Database backends now raise unified exceptions

API Changes:
  • Added the attribute db_exc to Environment to access DB-specific exception types.
  • Added the method get_exceptions() to IDatabaseConnector.
Internal Changes:

Description

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.

Attachments (0)

Change History (49)

comment:1 by Christian Boos, 14 years ago

Keywords: db added
Milestone: 0.110.11.1

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?

comment:2 by Jeroen Ruigrok van der Werven, 14 years ago

On FreeBSD 6.2-STABLE:

SQLite 3.4.1 PySQLite 2.5.1

comment:3 by Christian Boos, 14 years ago

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

comment:4 by Christian Boos, 14 years ago

See also #5445.

comment:5 by Christian Boos, 14 years ago

Component: trac-admingeneral
Owner: changed from Christopher Lenz to Christian Boos
Priority: normalhigh
Summary: Duplicate permission causes tracebackCatch database exceptions in a backend neutral wa

comment:6 by Christian Boos, 14 years ago

Summary: Catch database exceptions in a backend neutral waCatch database exceptions in a backend neutral way

comment:7 by osimons, 14 years ago

#6841 closed as duplicate.

comment:8 by Christian Boos, 14 years ago

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

comment:9 by osimons, 13 years ago

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

comment:10 by Christian Boos, 13 years ago

Milestone: 0.11-retriage0.11.3

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

comment:11 by Christian Boos, 13 years ago

Keywords: integrityerror added

#8039 closed as duplicate.

comment:12 by AllenJB <trac@…>, 13 years ago

Cc: trac@… added

comment:13 by Christian Boos, 13 years ago

Severity: normalmajor

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

comment:14 by Christian Boos, 13 years ago

Milestone: 0.11.40.11.5

comment:15 by tlk, 12 years ago

See also #4984

comment:16 by Christian Boos, 12 years ago

See also issue #8379 (rollback on error).

comment:17 by Christian Boos, 12 years ago

Component: generaldatabase backend

comment:18 by Christian Boos, 12 years ago

Milestone: 0.11.60.12

comment:19 by Felix Schwarz <felix.schwarz@…>, 12 years ago

Cc: felix.schwarz@… added

comment:20 by Christian Boos, 12 years ago

Milestone: 0.120.12.1

Can wait for 0.12.1.

comment:21 by Christian Boos, 12 years ago

#8379 was closed as duplicate.

comment:22 by Carsten Klein <carsten.klein@…>, 12 years ago

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.

comment:23 by Christian Boos, 12 years ago

Another duplicate, #9153.

comment:24 by Christian Boos, 11 years ago

Keywords: needmajor added
Milestone: 0.12.10.13

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

comment:25 by psuter <petsuter@…>, 10 years ago

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

Assumptions:

(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? :)

comment:26 by Christian Boos, 10 years ago

Owner: changed from Christian Boos to Peter Suter

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)

comment:27 by psuter <petsuter@…>, 10 years ago

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.

comment:28 by psuter <petsuter@…>, 10 years ago

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

in reply to:  26 comment:29 by psuter <petsuter@…>, 10 years ago

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.

comment:30 by Remy Blank, 10 years ago

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

comment:31 by trac@…, 10 years ago

Cc: trac@… removed

in reply to:  30 ; comment:32 by psuter <petsuter@…>, 10 years ago

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.

in reply to:  32 ; comment:33 by Remy Blank, 10 years ago

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.

in reply to:  33 comment:34 by psuter <petsuter@…>, 10 years ago

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

comment:35 by osimons, 10 years ago

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.

in reply to:  35 comment:36 by Remy Blank, 10 years ago

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

in reply to:  35 comment:37 by psuter <petsuter@…>, 10 years ago

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?

comment:38 by psuter <petsuter@…>, 10 years ago

Sorry, that was supposed to be Python DB API.

comment:39 by osimons, 10 years ago

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.

in reply to:  39 comment:40 by psuter <petsuter@…>, 10 years ago

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.

comment:41 by osimons, 10 years ago

Nice! Looks good & ready to me.

comment:42 by Remy Blank, 10 years ago

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?

in reply to:  42 ; comment:43 by psuter <petsuter@…>, 10 years ago

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.

in reply to:  43 ; comment:44 by Remy Blank, 10 years ago

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.

in reply to:  44 comment:45 by psuter <petsuter@…>, 10 years ago

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.

comment:46 by Remy Blank, 10 years ago

API Changes: modified (diff)

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?

comment:47 by psuter <petsuter@…>, 10 years ago

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].)

comment:48 by Remy Blank, 10 years ago

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

Time to close, in that case :)

comment:49 by al.willmer@…, 10 years ago

Release Notes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Peter Suter.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Peter Suter 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.