Opened 3 years ago

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

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 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 3 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 3 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 2 years ago by aaugustin

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

comment:5 Changed 2 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 2 years 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 2 years ago by JordanPowell

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

comment:8 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:9 Changed 2 years ago by JordanPowell

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

Changed 2 years ago by JordanPowell

comment:10 Changed 2 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 2 years 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 2 years ago by JordanPowell

comment:12 Changed 2 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 2 years 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 2 years 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 2 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

Looks good!

comment:16 Changed 2 years 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.

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