Django should wrap all PEP 249 exceptions in db.utils

[Xia Kai(夏恺)]

There are only two Exceptions defined in db.utils, namely DatabaseError and IntegrityError. This does not conform with ​PEP 249, Python Database API Specification v2.0. IMHO, more exceptions should be added here. Moreover, db backends for python binding of various databases conforming with PEP 249 should be changed to reflect this change.

Sorry that I cannot provide a full patch for this ticket, for I'm not familiar with all db backends.

This change would give more flexibility in db exception handling.

[Luke Plant]

Our ORM is not intended to conform to PEP 249, since it does not provide access to a database, but wraps such access.

However, I think what you could be asking is for us to wrap all the PEP 249 exceptions the way that we wrap DatabaseError and IntegrityError, and I'm accepting on that basis, and changing the summary accordingly.

[Xia Kai(夏恺)]

thanks luke, that's exactly what I meant. I think Django should provide these Exceptions for developers.

Replying to lukeplant:

Our ORM is not intended to conform to PEP 249, since it does not provide access to a database, but wraps such access.

However, I think what you could be asking is for us to wrap all the PEP 249 exceptions the way that we wrap DatabaseError and IntegrityError, and I'm accepting on that basis, and changing the summary accordingly.

[Malcolm Tredinnick]

We have intentionally and very deliberately not done this to date, because the exception types vary across the various database backends. Even when they offer the standard PEP 249 exceptions, their contents cannot be treated portably because they are often implemented quite differently. Wanting to handle arbitrary exceptions from an unknown database is a very big ask for application code and, as I said, we decided not to support this use-case. We expose the two, including the base class, that are most commonly going to be raised to the user level.

More useful for practical purposes would be an explanation of what the big use-cases are that need particular exceptions exposed. Remember that once Django adds support for something like this, we have to support it forever, so it's not to be done lightly.

[absurdhero]

I have a specific real-world use case which I can describe. My web application has an accompanying daemon that inserts data into the database from another source and it wakes up periodically to do its work. If the database has restarted while it was asleep or if there is a connection problem, the next query will raise OperationalError per PEP 249. The daemon catches this exception and goes back to sleep so that it may retry later. It cannot catch all DatabaseError subclasses or it would gobble up important DataError or ProgrammingError exceptions which should be catastrophic errors and do not warrant retrying later.

This use case would be satisfied if OperationalError were separately wrapped so that intermittent (non-programmer) errors could be handled differently.

Another option I see is providing the ability to disable cursor exception wrapping for users who care to deal with their database driver's exception behavior directly. I'm not convinced that Django should provide a complete and total database abstraction layer and driver abstraction layer for all purposes. Covering up errors and losing important exception information is not always acceptable but, as xiaket stated, handling every driver's exceptions uniformly may not be feasible. The only solution is to allow users to disable this kind of deep driver abstraction while keeping the database model abstraction. If I understand correctly, this is currently not supported.

[Carl Meyer]

As a point of reference, SQLAlchemy wraps all the PEP 249 exceptions individually - see help(sqlalchemy.exc).

The reasons given here for not doing this in the Django ORM seem a bit handwavy to me. Since all PEP 249 exceptions (but one) should inherit from DatabaseError in a proper DB API driver, and we already catch and wrap DatabaseError in all the backends, we are already wrapping everything anyway and papering over implementation differences in the backends; only we're doing it in a way that hides a lot more information than we'd need to (we hide the distinction between OperationalError, ProgrammingError, InternalError, DataError, and NotSupportedError - they all end up as django.db.utils.DatabaseError).

Adding the appropriate three-line "except-raise" clauses in execute and execute_many of each backend's CursorWrapper to wrap each of these individually (no need for smart introspection of the contents or caring about how they differ from backend to backend, just wrap and pass along the arguments exactly as we do now with DatabaseError) would not add a significant maintenance burden and would hide a lot less information than we do now. If backends are inconsistent in when they use the various errors, or what arguments they pass to them, fine: the inconsistency would be no worse than it is now (application code could still catch DatabaseError if it doesn't care about the distinction), but code that does care about the distinction, needs to support more than one backend, and knows what the backends it cares about actually do, would be a lot easier to write.

Checked this with Jacob in IRC - moving back to Accepted.

[Preston Holmes]

see also #16948 regarding passing through DB specific exception attributes

[James Henstridge]

I filed #16948, and have put together a patch attached to that bug that might satisfy the needs for this bug too. I'd appreciate some testing if possible.

It doesn't add new Django backend independent exceptions, but instead patches django.db.utils.DatabaseError and IntegrityError into the adapter's exception hierarchy. This way we can pass the adapter's exceptions through unmodified while still allowing code to catch the backend independent exceptions.

I suspect that if any application code cares about exceptions more specific than DatabaseError and IntegrityError that it has dependencies on a particular backend, so why not use the adapter's exceptions directly?

[James Henstridge]

Attaching my patch from the duplicate ticket so it doesn't get lost. Has anyone else given the patch a test to see if it covers their use cases?

[gcbirzan]

Replying to jamesh:

Attaching my patch from the duplicate ticket so it doesn't get lost. Has anyone else given the patch a test to see if it covers their use cases?

That is an absolutely terrible way of doing things. Monkey patching should be avoided, and this is a perfect example of when it would be a bad idea to do so. Monkey patching the backend exceptions would mean that there's no way to distinguish between exceptions raised by the django wrapper and by the backend itself (by backend here I mean psycopg2, oursql, sqlite3, whathaveyou).

There are two other solutions to this. The first is the established way of handling this, passing a value to the outer exception:

try:
    do_stuff()
except Database.DatabaseError as e:
    raise util.DatabaseError, e, sys.exc_info()[2]

This preserves the message, e.message will still contain the original message, while e.args[0] contains the original exception. A variation thereof can be using an attribute on e for the inner exception. Testing the inner exception would be awkward, but it would be possible.

Another way, albeit a bit unconventional, would be:

try:
    do_stuff()
except Database.DatabaseError as e:
    raise type('DatabaseError', (Database.DatabaseError, util.DatabaseError), {}), e.args, sys.exc_info()[2]

This would allow you to do catch both types of the exception, just through the except class as e syntax, but it would still be awkward for testing if an exception was raised by django or not (you'd have to do except backend.DatabaseError as e: first, and only then do util.DatabaseError).

The latter also has the benefit of not breaking most of the current implementations, whereas the first one might if someone relies on e.args (though, as mentioned, that can be kept intact, and the inner exception can be put in an attribute).

I also think that this bug should get back to design decision needed, but I'll leave that up the actual developers of django.

[James Henstridge]

Both of your suggestions prevent users from writing standard Python "except" clauses to catch particular classes of database errors, which seems to be one of the goals of this ticket.

And for some database adapters like psycopg2, there is more useful information in the exception than you'll find in e.args (e.g. the pgcode attribute holding the SQL error code).

If your concern is about not being able to distinguish exceptions that come directly from the database adapter and exceptions from the Django backend, what do you see as the consequences of not being able to do this?

Django used to let the adapter exceptions bubble up through the backends. It only changed when support for multiple databases were added and it was necessary to have some backend independent exceptions to catch. If we let the adapter exceptions bubble up but keep the ability to catch backend independent exceptions, what do we lose?

[Anssi Kääriäinen]

You lose the ability to know what types of exceptions queryset execution can raise.

[James Henstridge]

Replying to akaariai:

You lose the ability to know what types of exceptions queryset execution can raise.

At the moment, you know that if a database error occurs in executing a query, then you'll be able to catch it as django.utils.DatabaseError or django.utils.IntegrityError (anything else passes through unchanged). With my patch, you can still catch those same two exceptions in the same situations.

Yes there are more possible exception types that will pass up, but if you are happily using the django.utils exceptions you don't need to worry about them.

[James Henstridge]

Thinking a bit more about this, if poking around in __bases__ is unacceptable I could probably achieve the same effect using the abc module that was added to the standard library in 2.6. If that would help, I can put together an updated patch.

[Anssi Kääriäinen]

I didn't read your patch before posting the above...

Using ABC is definitely worth a shot. I don't know that module, so I can't say if it will actually work or be clean enough...

[gcbirzan]

Replying to jamesh:

Both of your suggestions prevent users from writing standard Python "except" clauses to catch particular classes of database errors, which seems to be one of the goals of this ticket.

And for some database adapters like psycopg2, there is more useful information in the exception than you'll find in e.args (e.g. the pgcode attribute holding the SQL error code).

I don't understand what you mean. Your confusion might stem from the fact that I use code as would be in the database backend, where you have import psycopg2 as Database. With the first one:

try:
    do_stuff()
except util.DatabaseError as e:
    if isinstance(e.args[0], psycopg2.ProgrammingError):
        print e.args[0].pgcode

second:

try:
    do_stuff()
except psycopg2.ProgrammingError as e:
    pass

Granted, you cannot access pgcode, but that could be hanlded either in a backend specific manner, or, you could just put the exception's dictionary in the new type (ugly, I know.). But either way, you're better off than now.

I personally am leaning towards the first method, to be honest, since it's the closest to pep3134. The original doesn't have to be put in the args, it can be:

try:
    do_stuff()
except Database.DatabaseError as e:
    exc = util.DatabaseError(*e.args)
    exc.__cause__ = e
    raise util.DatabaseError, exc, sys.exc_info()[2]

You can even combine the two:

try:
    do_stuff()
except Database.DatabaseError as e:
    exc_type = type('DatabaseError', (Database.DatabaseError, util.DatabaseError), {})
    raise exc_type, exc_type(e), sys.exc_info()[2]

or

try:
    do_stuff()
except Database.DatabaseError as e:
    exc_type = type('DatabaseError', (Database.DatabaseError, util.DatabaseError), {})
    exc = exc_type(*e.args)
    exc.__cause__ = e
    raise exc_type, exc, sys.exc_info()[2]

Now you can catch it with both psycopg2.DatabaseError, and util.DatabaseError. In the first, you will have e.args[0] for the original, in the second, e.cause.

If your concern is about not being able to distinguish exceptions that come directly from the database adapter and exceptions from the Django backend, what do you see as the consequences of not being able to do this?

Django used to let the adapter exceptions bubble up through the backends. It only changed when support for multiple databases were added and it was necessary to have some backend independent exceptions to catch. If we let the adapter exceptions bubble up but keep the ability to catch backend independent exceptions, what do we lose?

The fact that it will violate the principle of least astonishment, like all monkey patching.

As for your ABC suggestion, it won't work. ​http://bugs.python.org/issue12029

[gcbirzan]

Actually, it seems to work in Python 2. I attached a patch for the PostgreSQL driver.

[Anssi Kääriäinen]

I like that ABC-approach. It seems very clean and nice. If you can write a patch for all backends I think this will have a high chance of getting into Django 1.5. The patch passes at least the "usual suspects" tests on PostgreSQL. Running the full test suite now...

This naturally needs tests also, and maybe a sentence in the docs, too.

[gcbirzan]

It turns out that this behaviour was introduced in a fix by Antoine Pitrou (​http://hg.python.org/cpython/rev/d6e86a96f9b3/#l8.10). The same patch didn't make it into 2.x, and that branch got a fix that didn't break ABC (​http://hg.python.org/cpython/rev/7e86fa255fc2), which was later refined (​http://hg.python.org/cpython/rev/57de1ad15c54) to account for insane recursion limits. I'll take it up on python-dev tomorrow, to see if applying the 2.x fix would be a good idea for 3.x, so I think this should be put on hold until then.

Of course, there's the alternative of patching the bases for Python 3, but I still think that's just... weird.

[gcbirzan]

Replying to gcbirzan:

I'll take it up on python-dev tomorrow, to see if applying the 2.x fix would be a good idea for 3.x, so I think this should be put on hold until then.

Just to clarify, I'll hold on making a patch for all until I'll know if that bug will get fixed or not.

[gcbirzan]

Replying to jamesh:

Both of your suggestions prevent users from writing standard Python "except" clauses to catch particular classes of database errors, which seems to be one of the goals of this ticket.

Wow, I'm a retard. I just reread my examples, and they all use the base class. Yeah, it should be e.class instead of Database.DatabaseError.

[Anssi Kääriäinen]

I'm not sure how much it helps if the ABC issue will be fixed in a future Python 3 release. Django is going most likely to support 3.2, and if 3.2.0 has this issue then it will be a big problem for the use of ABC module. Too bad, the ABC module solution looks like exactly what we would want... It seems hard to hack around this issue, as the problem is at C code level.

If the ABC can't be used, then I think using this style would be good:

try:
    do_stuff()
except Database.DatabaseError as e:
    exc = util.DatabaseError(*e.args)
    exc.__cause__ = e
    raise util.DatabaseError, exc, sys.exc_info()[2]

If I understand it correctly, everything works exactly as now but you will have the original exception available in the __cause__. It is not the perfect solution, but allows users to access the original exception if they really need. I think you can even write your little context manager which re-raises the __cause__ exception if you really need it...

[James Henstridge]

If you don't mind special casing for the broken versions of Python 3.x, one option would be to use the __bases__ version of the patch on those versions. The ugly bits could easily be wrapped up in a class method:

class DatabaseError(Exception):
    @classmethod
    def register(cls, other):
        if not issubclass(other, cls):
            other.__bases__ += (cls,)

This would allow the use of the same code in the backends as gcbirzan's patch, whether the exception is defined as an abc or not. When you decide to drop support for the broken versions of Python, the compatibility code could be removed.

[Aymeric Augustin]

See also #19507.

[Anssi Kääriäinen]

The Python ticket hasn't moved forward. So, I think it might be time to inspect the __bases__ approach at least for Python 3.

[Aymeric Augustin]

This is fixed in the last commit of this PR: ​https://github.com/django/django/pull/855

[Aymeric Augustin <aymeric.augustin@…>]

In 59a352087591a26023412cbcb830cd1d34fc9b99:

Refactored database exceptions wrapping.

Squashed commit of the following:

commit 2181d833ed1a2e422494738dcef311164c4e097e
Author: Aymeric Augustin <aymeric.augustin@…>
Date: Wed Feb 27 14:28:39 2013 +0100

Fixed #15901 -- Wrapped all PEP-249 exceptions.

commit 5476a5d93c19aa2f928c497d39ce6e33f52694e2
Author: Aymeric Augustin <aymeric.augustin@…>
Date: Tue Feb 26 17:26:52 2013 +0100

Added PEP 3134 exception chaining.

Thanks Jacob Kaplan-Moss for the suggestion.

commit 9365fad0a650328002fb424457d675a273c95802
Author: Aymeric Augustin <aymeric.augustin@…>
Date: Tue Feb 26 17:13:49 2013 +0100

Improved API for wrapping database errors.

Thanks Alex Gaynor for the proposal.

commit 1b463b765f2826f73a8d9266795cd5da4f8d5e9e
Author: Aymeric Augustin <aymeric.augustin@…>
Date: Tue Feb 26 15:00:39 2013 +0100

Removed redundant exception wrapping.

This is now taken care of by the cursor wrapper.

commit 524bc7345a724bf526bdd2dd1bcf5ede67d6bb5c
Author: Aymeric Augustin <aymeric.augustin@…>
Date: Tue Feb 26 14:55:10 2013 +0100

Wrapped database exceptions in the base backend.

This covers the most common PEP-249 APIs:

• Connection APIs: close(), commit(), rollback(), cursor()
• Cursor APIs: callproc(), close(), execute(), executemany(), fetchone(), fetchmany(), fetchall(), nextset().

Fixed #19920.

commit a66746bb5f0839f35543222787fce3b6a0d0a3ea
Author: Aymeric Augustin <aymeric.augustin@…>
Date: Tue Feb 26 14:53:34 2013 +0100

Added a wrap_database_exception context manager and decorator.

It re-throws backend-specific exceptions using Django's common wrappers.