Code

Opened 2 years ago

Closed 11 months 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, charettes, 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 13 months ago.
#17582.2.patch (2.3 KB) - added by JordanPowell 13 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 2 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 2 years ago by jezdez

  • Resolution set to needsinfo
  • Status changed from new to closed
  • Triage Stage changed from Unreviewed to Design decision needed

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

comment:3 Changed 2 years ago by systemparadox2

  • Resolution needsinfo deleted
  • Status changed from closed to reopened
  • Summary changed from Debug page - ambiguous exception type (should show full path) to 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 13 months ago by aaugustin

  • Component changed from Template system to Database layer (models, ORM)

comment:5 Changed 13 months 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 13 months ago by charettes

  • Cc charettes added
  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Design decision needed to Accepted
  • Type changed from Bug to Cleanup/optimization
  • Version changed from 1.3 to master

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 13 months ago by JordanPowell

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

comment:8 Changed 13 months ago by aaugustin

  • Status changed from reopened to new

comment:9 Changed 13 months ago by JordanPowell

  • Owner changed from nobody to JordanPowell
  • Status changed from new to assigned

Changed 13 months ago by JordanPowell

comment:10 Changed 13 months 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 13 months ago by charettes

  • 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 13 months ago by JordanPowell

comment:12 Changed 13 months 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 13 months ago by charettes

  • Triage Stage changed from Accepted to Ready 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 11 months ago by timo

  • Cc timograham@… added
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Ready for checkin to Accepted

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 11 months ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

Looks good!

comment:16 Changed 11 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 6f8627dd7fd482007694dd1e4a99f9e207e6d8c2:

Fixed #17582 - Added message to DoesNotExist exceptions.

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.