Opened 14 years ago
Closed 12 years ago
#15901 closed New feature (fixed)
Django should wrap all PEP 249 exceptions in db.utils
Reported by: | Xia Kai(夏恺) | Owned by: | Aymeric Augustin |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.3 |
Severity: | Normal | Keywords: | |
Cc: | xiaket@…, jonas-django@…, James Henstridge, gcbirzan | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Attachments (2)
Change History (31)
comment:1 by , 14 years ago
Cc: | added |
---|
follow-up: 3 comment:2 by , 14 years ago
Summary: | Exceptions defined in db.utils should conform with PEP 249 → Django should wrap all PEP 249 exceptions in db.utils |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 14 years ago
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
andIntegrityError
, and I'm accepting on that basis, and changing the summary accordingly.
comment:4 by , 13 years ago
Triage Stage: | Accepted → Design decision needed |
---|---|
UI/UX: | unset |
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.
comment:5 by , 13 years ago
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.
comment:6 by , 13 years ago
Triage Stage: | Design decision needed → Accepted |
---|
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.
comment:7 by , 13 years ago
see also #16948 regarding passing through DB specific exception attributes
comment:8 by , 13 years ago
Cc: | added |
---|
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?
by , 13 years ago
Attachment: | db-backend-exceptions.patch added |
---|
follow-up: 10 comment:9 by , 13 years ago
Has patch: | set |
---|
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?
comment:10 by , 12 years ago
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.
comment:11 by , 12 years ago
Cc: | added |
---|
follow-up: 22 comment:12 by , 12 years ago
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?
follow-up: 14 comment:13 by , 12 years ago
You lose the ability to know what types of exceptions queryset execution can raise.
comment:14 by , 12 years ago
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.
comment:15 by , 12 years ago
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.
comment:16 by , 12 years ago
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...
comment:17 by , 12 years ago
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. thepgcode
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
comment:18 by , 12 years ago
Actually, it seems to work in Python 2. I attached a patch for the PostgreSQL driver.
by , 12 years ago
Attachment: | django-db-exceptions.patch added |
---|
comment:19 by , 12 years ago
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.
EDIT: I should really start reading all messages in the thread before posting... So, is there a problem for exceptions + ABC or not? If there is, in what versions? Python3 incompatibility is a show-stopper.
follow-up: 21 comment:20 by , 12 years ago
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.
comment:21 by , 12 years ago
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.
comment:22 by , 12 years ago
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.
comment:23 by , 12 years ago
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...
comment:24 by , 12 years ago
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.
comment:26 by , 12 years ago
The Python ticket hasn't moved forward. So, I think it might be time to inspect the __bases__
approach at least for Python 3.
comment:27 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:28 by , 12 years ago
This is fixed in the last commit of this PR: https://github.com/django/django/pull/855
comment:29 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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
andIntegrityError
, and I'm accepting on that basis, and changing the summary accordingly.