Opened 12 years ago

Closed 11 years ago

#17582 closed Cleanup/optimization (fixed)

DoesNotExist ambiguous exception when accessed via foreign key

Reported by: simon@… Owned by: JordanPowell
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: JordanPowell, Simon Charette, timograham@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Hi.

When a Foo.DoesNotExist exception is raised, the debug page only prints 'DoesNotExist', without showing which model it belongs to.

Is there some way we can fix this? I would like to see as much information as possible.

Thanks.
Simon

Attachments (2)

#17582.patch (2.4 KB ) - added by JordanPowell 11 years ago.
#17582.2.patch (2.3 KB ) - added by JordanPowell 11 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by anonymous, 12 years ago

Below the exception type is the exception value which will usually say something like

FooBar matching query does not exist

Then below that is a full traceback which should point you to which piece of code which made the erroneous query.

comment:2 by Jannis Leidel, 12 years ago

Resolution: needsinfo
Status: newclosed
Triage Stage: UnreviewedDesign decision needed

Yeah, this needs more information. What is the expected output?

comment:3 by Simon Williams, 12 years ago

Resolution: needsinfo
Status: closedreopened
Summary: Debug page - ambiguous exception type (should show full path)DoesNotExist ambiguous exception when accessed via foreign key

When accessed via a query, this does indeed give the model name in the message (e.g. 'FooBar matching query does not exist').

However, when accessing a foreign key which either has an invalid value or is null, the exception is raised without any message:

.../django/db/models/fields/related.py:301 in __get__
raise self.field.rel.to.DoesNotExist

This causes the debug page just to show 'DoesNotExist' and 'No exception supplied'.

Since Model.DoesNotExist is created dynamically when the model is loaded, surely it would be possible to the name of the model as the default message or similar?

Thanks.

comment:4 by Aymeric Augustin, 11 years ago

Component: Template systemDatabase layer (models, ORM)

comment:5 by JordanPowell, 11 years ago

Cc: JordanPowell added

Any idea why this is DDN? I can't see any problem with making the exception more informative. It looks like someone just forgot to instantiate it.

I'm happy to put together a patch and tests for this, if someone agrees that there's no Design Decision Needed.

I'd probably just change the line to:

raise self.field.rel.to.DoesNotExist(
    "This %s has no %s (the %s is null)" %
    (self.field.model._meta.object_name, self.field.name,
    self.field.rel.to._meta.object_name))

so it reads:

This Model has no field (the RelatedModel is null).

Any comments on the wording?

comment:6 by Simon Charette, 11 years ago

Cc: Simon Charette added
Needs documentation: set
Needs tests: set
Triage Stage: Design decision neededAccepted
Type: BugCleanup/optimization
Version: 1.3master

Accepting the ticket as an improvement.

raise self.field.rel.to.DoesNotExist(
    "%s has no %s." % (self.field.model.__name__, self.field.name)
)

Model has no field. sounds better to me but I'd also like comments on the wording.

I removed the null part since this exception can also be raised if the field as an invalid value as pointed out in comment:3.

This needs testing and an addition to the release note.

comment:7 by JordanPowell, 11 years ago

I'll put something together tomorrow if no one beats me to it.

comment:8 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:9 by JordanPowell, 11 years ago

Owner: changed from nobody to JordanPowell
Status: newassigned

by JordanPowell, 11 years ago

Attachment: #17582.patch added

comment:10 by JordanPowell, 11 years ago

Just added a patch for this - let me know if anything needs changing and if I should issue a pull request.

comment:11 by Simon Charette, 11 years ago

Patch needs improvement: set

Instead of testing for the exception message being not empty you should use assertRaisesMessage to make sure the correct message is raised.

by JordanPowell, 11 years ago

Attachment: #17582.2.patch added

comment:12 by JordanPowell, 11 years ago

Ok cool - how's that? I wasn't sure if I should tie the test so closely to the message, thanks for clarifying.

comment:13 by Simon Charette, 11 years ago

Triage Stage: AcceptedReady for checkin

Apart from the missing point in the release note the patch looks RFC. Will commit soon with this minor change.

comment:14 by Tim Graham, 11 years ago

Cc: timograham@… added
Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: Ready for checkinAccepted

In reviewing this, I found another instance of raise DoesNotExist without a message. Updated in the following pull request: https://github.com/django/django/pull/1212

(moving back to accepted to get another review)

comment:15 by Aymeric Augustin, 11 years ago

Triage Stage: AcceptedReady for checkin

Looks good!

comment:16 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 6f8627dd7fd482007694dd1e4a99f9e207e6d8c2:

Fixed #17582 - Added message to DoesNotExist exceptions.

Thanks simon@ for the suggestion and JordanPowell
for the initial patch.

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