Code

Opened 2 years ago

Closed 13 months ago

#17601 closed Cleanup/optimization (fixed)

Error code from database exception should not be lost during exception handling (psycopg2)

Reported by: zimnyx Owned by: jaylett
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: james@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Here is a fragment from source:django/trunk/django/db/backends/postgresql_psycopg2/base.py?rev=17244#L50

    def execute(self, query, args=None):
        try:
            return self.cursor.execute(query, args)
        except Database.IntegrityError, e:
            raise utils.IntegrityError, utils.IntegrityError(*tuple(e)), sys.exc_info()[2]
        except Database.DatabaseError, e:
            raise utils.DatabaseError, utils.DatabaseError(*tuple(e)), sys.exc_info()[2]

In case of Psycopg2 and database exceptions, error code is set as psycopg2 exception propery called pgcode instead of being passed as exception argument, so this way we're loosing info about "pgcode" which is valuable. Handling database exception by parsing error message is a bit awkward.

I'm not sure if other backends are affected by this issue, but I can investigate and provide a patch if we agree on this issue.

P.S.
I've just filled a bug in this subject against Psycopg2: http://psycopg.lighthouseapp.com/projects/62710-psycopg/tickets/96-pgcode-should-be-present-in-exceptionargs
If they fix it, this ticket will become "worksforme".

Attachments (0)

Change History (9)

comment:1 Changed 2 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

Sure, all value-added information we can bring might be useful for the poor debugging dev :-)

comment:2 Changed 14 months ago by jaylett

The bug filed against psycopg2 was closed, rightly. I think what is needed (within Django) is a way of getting at the original exception given the common wrapper, so that (in this case) it's possible to access pgcode and particularly diag from the psycopg2 exception. In python 3 this is available via __cause__ (AIUI; I haven't actually used py3 in anger). Could we just set __cause__ whether we're running under py3 or not? Something like (at [c36b75c814f068fcb722d46bd5e71cbaddf9bf2d]):

--- a/django/db/utils.py
+++ b/django/db/utils.py
@@ -91,8 +91,7 @@ class DatabaseErrorWrapper(object):
                 except AttributeError:
                     args = (exc_value,)
                 dj_exc_value = dj_exc_type(*args)
-                if six.PY3:
-                    dj_exc_value.__cause__ = exc_value
+                dj_exc_value.__cause__ = exc_value
                 # Only set the 'errors_occurred' flag for errors that may make
                 # the connection unusable.
                 if dj_exc_type not in (DataError, IntegrityError):

comment:3 Changed 14 months ago by jaylett

  • Cc james@… added

comment:4 Changed 14 months ago by aaugustin

I suppose setting cause unconditionally can't hurt...

comment:5 Changed 13 months ago by jaylett

I can't think of a way it would, since it doesn't exist at the moment. (It's possible that poorly-written code might use it to assume it's running under py3, but that seems a stretch.)

I'm not quite sure where we'd want to document this; probably a small note in ref/exceptions?. (There are no tests that use __cause__ as far as I can tell, so I don't know if we need an explicit test case for this.)

comment:6 Changed 13 months ago by jaylett

  • Owner changed from nobody to jaylett
  • Status changed from new to assigned

comment:7 Changed 13 months ago by jaylett

Pull request 1241 (https://github.com/django/django/pull/1241) rather than messing around with tiny patches in comments.

comment:8 Changed 13 months ago by jaylett

  • Has patch set

comment:9 Changed 13 months ago by James Aylett <james@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 54485557855d58d9a5027511025d5ab22f721c6d:

Fixed #17601 -- expose underlying db exceptions under py2

Use cause to expose the underlying database exceptions even
under python 2.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.