Opened 15 years ago
Closed 15 years ago
#13012 closed (invalid)
Model.objects.get_or_create() try/except is slightly yet insideously broken
| Reported by: | 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: | no | UI/UX: | no |
Description
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)
Change History (3)
by , 15 years ago
| Attachment: | get_or_create_fix.patch added |
|---|
comment:1 by , 15 years ago
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 by , 15 years ago
| Resolution: | → invalid |
|---|---|
| Status: | new → closed |
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.
Fix for ticket #13012