Opened 5 years ago

Closed 4 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: master
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 4 years ago.
#17582.2.patch (2.3 KB) - added by JordanPowell 4 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 5 years ago by anonymous

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 Changed 5 years ago by Jannis Leidel

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

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

comment:3 Changed 5 years ago by Simon Williams

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 Changed 4 years ago by Aymeric Augustin

Component: Template systemDatabase layer (models, ORM)

comment:5 Changed 4 years ago by JordanPowell

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 Changed 4 years ago by Simon Charette

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 Changed 4 years ago by JordanPowell

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

comment:8 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:9 Changed 4 years ago by JordanPowell

Owner: changed from nobody to JordanPowell
Status: newassigned

Changed 4 years ago by JordanPowell

Attachment: #17582.patch added

comment:10 Changed 4 years ago by JordanPowell

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

comment:11 Changed 4 years ago by Simon Charette

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.

Changed 4 years ago by JordanPowell

Attachment: #17582.2.patch added

comment:12 Changed 4 years ago by JordanPowell

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 Changed 4 years ago by Simon Charette

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 Changed 4 years ago by Tim Graham

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 Changed 4 years ago by Aymeric Augustin

Triage Stage: AcceptedReady for checkin

Looks good!

comment:16 Changed 4 years ago by Tim Graham <timograham@…>

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