Opened 5 years ago

Closed 23 months ago

#13400 closed Bug (duplicate)

model.DoesNotExist exception raised by QuerySet.get() should include the same nice description of parameters that the MultipleObjectsReturned exception does.

Reported by: aaron Owned by: trentm
Component: Database layer (models, ORM) Version:
Severity: Normal Keywords: model.DoesNotExist, ObjectDoesNotExist, QuerySet.get error message kwargs parameters
Cc: asokoloski@…, boxed Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The current message contained in a DoesNotExist (ObjectDoesNotExist) exception raised by QuerySet.get(...) is roughly:
"%s matching query does not exist." % MODEL_NAME

But the MultipleObjectsReturned exception raised by the same function looks like:
"get() returned more than one %s -- it returned %s! Lookup parameters were %s" % (MODEL_NAME, NUMBER_OF_OBJECTS, KWARGS)

with KWARGS being the keyword arguments passed to the get(...) method. This is great for investigating error message emails sent from a live site -- it makes it really easy to find out which object you were trying to get, and how to fix the root cause.

I suggest that the DoesNotExist error message would benefit from the same descriptiveness. I am attaching a patch that adds this information to the model.DoesNotExist exception.

Attachments (3)

13400-better-doesnotexist-error-message.diff (835 bytes) - added by aaron 5 years ago.
Patch to give better error message when QuerySet.get(...) raises model.DoesNotExist
13400-better-doesnotexist-error-message-tests-fixed.diff (6.4 KB) - added by aaron 5 years ago.
Same as previous patch, but with tests fixed.
13400-alt-doesnotexist-error-message.diff (10.9 KB) - added by trentm 5 years ago.
My alternative improved DoesNotExist error message patch (with updates for tests)

Download all attachments as: .zip

Change History (19)

Changed 5 years ago by aaron

Patch to give better error message when QuerySet.get(...) raises model.DoesNotExist

comment:1 Changed 5 years ago by aaron

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by russellm

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

A reasonable suggestion. However the patch needs work - in particular, this change will break several existing doctests that are relying on the current wording of the error.

Changed 5 years ago by aaron

Same as previous patch, but with tests fixed.

comment:3 Changed 5 years ago by trentm@…

The format of the message I came up with was, e.g.:

DoesNotExist: User matching query (id__exact=2L) does not exist.

The patch for that (without the patch for the test cases) is:

Index: django/db/models/query.py
===================================================================
--- django/db/models/query.py   (revision 13020)
+++ django/db/models/query.py   (working copy)
@@ -301,8 +301,9 @@
         if num == 1:
             return clone._result_cache[0]
         if not num:
-            raise self.model.DoesNotExist("%s matching query does not exist."
-                    % self.model._meta.object_name)
+            q = ', '.join([repr(a) for a in args] + ["%s=%r" % i for i in kwargs.items()])
+            raise self.model.DoesNotExist(u"%s matching query (`%s`) does not exist."
+                    % (self.model._meta.object_name, q))
         raise self.model.MultipleObjectsReturned("get() returned more than one %s -- it returned %s! Lookup parameters were %s"
                 % (self.model._meta.object_name, num, kwargs))

If others preferred that wording, then I could make a patch along with the test case updates.

Pros:

  • My patch includes the "args" in the formatting, tho I don't really know if the 'args' are ever non-empty

Cons:

  • Construction of the error string in my patch is slightly more computationally expensive.

comment:4 Changed 5 years ago by trentm@…

That example string in last message should be (wiki formatting error of mine):

DoesNotExist: User matching query (`id__exact=2L`) does not exist.

comment:5 Changed 5 years ago by aaron

Sounds good to me, trentm.

comment:6 Changed 5 years ago by aaron

  • Cc asokoloski@… added

Changed 5 years ago by trentm

My alternative improved DoesNotExist error message patch (with updates for tests)

comment:7 Changed 5 years ago by trentm

  • Needs documentation set
  • Needs tests unset

Note to self: there are a couple examples of this traceback in the docs that should probably be added to the patch.

comment:8 Changed 5 years ago by aaron

  • Owner changed from aaron to trentm

Trentm, I'm going to assign this to you now since you're the one working on it. Thanks.

comment:9 Changed 5 years ago by adamnelson

  • Patch needs improvement set

Noting that patch needs improvement, as per Trentm

comment:10 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:11 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 Changed 3 years ago by boxed

Is anyone gonna integrate this? Such a simple fix that improves usability so much seems like a no brainer to me.

comment:14 Changed 3 years ago by boxed

  • Cc boxed added

comment:15 Changed 3 years ago by russellm

@boxed: Yes, the idea is a no brainer. That's why the ticket was accepted. However, the fact that the idea is a no-brainer doesn't change the fact that the ticket is marked as needing improvement. That means we need someone to fix the patch, and we need someone else to review that updated patch and mark the ticket as ready for checkin.

You don't need to have special permission to do either of these things -- we just need a cleanly applying patch, addressing all the issues raised, that has been vetted by two people independently. Once it's marked ready for checkin, the core team will commit it (or bump it back to needs improvement, with more comments).

comment:16 Changed 23 months ago by bmispelon

  • Resolution set to duplicate
  • Status changed from new to closed

This was a duplicate of #10494 which was fixed in d5b93d3281fe93cbef5de84a52001f64dbd839bd

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