Opened 8 years ago

Closed 8 years ago

#13012 closed (invalid)

Model.objects.get_or_create() try/except is slightly yet insideously broken

Reported by: brantley (deadwisdom@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.1
Severity: Keywords: get_or_create
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


What is wrong with this code:

    except self.model.DoesNotExist:
        raise e

That's right, e is not defined. But what if e is previously defined in a surrounding except block? Then we will be raising the previous exception, not the current one. That's exactly what is happening in get_or_create(), and caused me some consternation as I was getting a "PRIMARY_KEY must be unique" exception, even though the real exception was something else.

Patch, in all its three character glory, is included.

Attachments (1)

get_or_create_fix.patch (529 bytes) - added by brantley (deadwisdom@… 8 years ago.
Fix for ticket #13012

Download all attachments as: .zip

Change History (3)

Changed 8 years ago by brantley (deadwisdom@…

Attachment: get_or_create_fix.patch added

Fix for ticket #13012

comment:1 Changed 8 years ago by brantley (deadwisdom@…

No, I was all wrong on this. The existing way is correct.

But something feels terribly off here. And now that I think about it, it's the second time I've been bitten by this non-bug.

comment:2 Changed 8 years ago by Karen Tracey

Resolution: invalid
Status: newclosed

That's exactly what the get_or_create code wants to do: reflect the earlier exception, not the does not exist, to the caller. I doubt raising DoesNotExist from get_or_create would make things any clearer. The problem is a subtle bug in the way you are calling it or how you have set unique constraints on the table, and you're likely going to be scratching your head on receipt of either the integrity error or the does not exist.

Note: See TracTickets for help on using tickets.
Back to Top