Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#6970 closed (fixed)

misleading error message if get_or_create not provided with defaults

Reported by: James Tauber Owned by: Gary Wilson
Component: Core (Other) Version: master
Severity: Keywords:
Cc: em@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

If get_or_create is given arguments that would necessitate a create but the necessary defaults are not provided, the exception thrown is DoesNotExist: Blah matching query does not exist.

It would be more useful if the error message explained the possibility that create failed because of missing defaults.

Attachments (1)

6970.patch (577 bytes) - added by Brantley 8 years ago.
Integrity!

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by Huvet

Cc: em@… added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

I had a similar problem with get_or_create that took most of today to find (I'm a Django newbie). Seems as if get_or_create throws DoesNotExist errors if you get database problems creating too. I had a Foreign key contraint that failed, but that error was somehow hidden by get_or_create. Really annoying! (Tip to anyone experiencing something similar: don't use get_or_create, use the two methods separately instead).

comment:2 Changed 8 years ago by bailey@…

specifically in django.db.models.query, line 228

if a value passed in defaults to get_or_create would cause a null constraint violation, handled like this:

except IntegrityError, e:

return self.get(kwargs), False

guaranteed to reraise a DoesNotExist, which is not helpful in the context of get_or_create

comment:3 Changed 8 years ago by Brantley

Has patch: set

Yeah, this is horridly annoying. At some point, staring at the screen, I actually began to think that my Python interpreter was messed up. It seemed to not be catching an exception that it explicitly says to catch.

Anyhow, here's the patch to make some sense.

Changed 8 years ago by Brantley

Attachment: 6970.patch added

Integrity!

comment:4 Changed 8 years ago by shawnr

Owner: changed from nobody to shawnr

comment:5 Changed 8 years ago by shawnr

Owner: shawnr deleted
Triage Stage: UnreviewedReady for checkin

I've tested this out at the DC Sprint and it seems to be working like a charm. Much nicer error message!

comment:6 Changed 8 years ago by Malcolm Tredinnick

milestone: 1.0

Better just to catch the generic DoesNotExist error right the end (saves a couple of variable resolutions) and there's no need to capture e2 (it's never used). They can be fixed at checkin. Other than that, this is fine and does make things clearer.

comment:7 Changed 8 years ago by Malcolm Tredinnick

Component: UncategorizedCore framework
Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

On second thoughts, this should have tests so that we don't inadvertently break it again in the future.

comment:8 Changed 8 years ago by Gary Wilson

Owner: set to Gary Wilson
Status: newassigned

comment:9 Changed 8 years ago by Gary Wilson

Resolution: fixed
Status: assignedclosed

(In [8398]) Fixed #6970 -- Raise the original IntegrityError when all required fields aren't specified in get_or_create instead of a misleading DoesNotExist error, thanks deadwisdom.

comment:10 Changed 5 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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