Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#17828 closed Cleanup/optimization (fixed)

Admin should not handle IncorrectLookupParameters if settings.DEBUG is set

Reported by: Glenn Washburn <development@…> Owned by: Chris Adams
Component: contrib.admin Version: 1.4-beta-1
Severity: Release blocker Keywords:
Cc: chris@… 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

Right now its difficult to debug exceptions raised in ListFilter code which raises an exception because the exception will be wrapped in an IncorrectLookupParameters exception, then caught by ModelAdmin.changelist_view which renders an uninformative invalid_setup.html template.

I propose adding in the except IncorrectLookupParameters block the following:

    if settings.DEBUG:
        raise

So that the main django exception handler can render the much more informative exception template. I've tested this and it seems to work fine.

Attachments (4)

17828-debug-IncorrectLookupParameters.patch (4.7 KB) - added by Chris Adams 5 years ago.
GitHub-generated patch
17828.listfilter-proper-error-handling.diff (6.5 KB) - added by Julien Phalip 5 years ago.
17828.listfilter-proper-error-handling.2.diff (7.5 KB) - added by Julien Phalip 5 years ago.
17828.listfilter-proper-error-handling.3.diff (7.5 KB) - added by Julien Phalip 5 years ago.
Without naked except

Download all attachments as: .zip

Change History (16)

comment:1 Changed 5 years ago by Glenn Washburn <development@…>

Also as an optional addon to this fix, the traceback should preserve the original exception's traceback. I've implemented this by modifying the raise statement in admin.views.main.ChangeList.get_query_set to be raise IncorrectLookupParameters, IncorrectLookupParameters(e), sys.exc_info()[2]

comment:2 Changed 5 years ago by Aymeric Augustin

Triage Stage: UnreviewedAccepted

I got a totally absurd error page while working on #17830; I suppose it's because of this bug.

comment:3 Changed 5 years ago by Chris Adams

Owner: changed from nobody to Chris Adams
Status: newassigned

comment:4 Changed 5 years ago by Chris Adams

Cc: chris@… added
Easy pickings: set
Has patch: set
Type: UncategorizedCleanup/optimization

I have a basic patch here which adds the settings.DEBUG check and some tests:

https://github.com/acdha/django/compare/17828-debug-IncorrectLookupParameters

It seems like there should be a logging.exception call in the non-DEBUG path so sysadmins have an easy way to learn about problems.

Changed 5 years ago by Chris Adams

GitHub-generated patch

comment:5 Changed 5 years ago by Aymeric Augustin

#17907 appears to be a duplicate and has a patch.

Last edited 5 years ago by Aymeric Augustin (previous) (diff)

comment:6 Changed 5 years ago by Julien Phalip

Severity: NormalRelease blocker

So it appears that the problem is in fact deeper. When a list filter's queryset() method fails, it should fail loudly instead of getting swallowed by a IncorrectLookupParameters exception (which then triggers a redirection, which in turn leads to the display of this confusing 'Database Error` page). This problem was in fact introduced by... me, in [16705], where I attempted to fix a regression spotted in #16716. The attached patch fixes that regression properly and also prevents this confusing error to be displayed.

Marking as a release blocker since it's a bug in a new feature.

Changed 5 years ago by Julien Phalip

comment:7 Changed 5 years ago by Jannis Leidel

Triage Stage: AcceptedReady for checkin

Looks good to me in general, I wish we didn't have to catch all exceptions though.

comment:8 in reply to:  7 Changed 5 years ago by Chris Adams

Replying to jezdez:

Looks good to me in general, I wish we didn't have to catch all exceptions though.

I share that distaste – perhaps a second round which only catches the ORM exceptions?

Changed 5 years ago by Julien Phalip

Changed 5 years ago by Julien Phalip

Without naked except

comment:9 Changed 5 years ago by Julien Phalip

Good catch. There was actually no need for a naked except there, so I've corrected that in the new patch. I'll push it unless you spot something else.

comment:10 Changed 5 years ago by Julien Phalip

Resolution: fixed
Status: assignedclosed

In [17763]:

Fixed #17828 -- Ensured that when a list filter's queryset() method fails, it does so loudly instead of getting swallowed by a IncorrectLookupParameters exception. This also properly fixes #16705, which hadn't been addressed correctly in [16705].

comment:11 Changed 5 years ago by Glenn Washburn <devlopment@…>

So, why was my first comment apparently not taken into consideration? Now that I think about it, every naked except which raises a new exception 'should' as a matter of policy include the stack trace from the previous exception. Are there any reasons why this should not be done? Just the exception object without the previous stack turns debugging into a spin of the roulette wheel. I prefer to have a reasonable chance of debugging exceptions.

comment:12 Changed 5 years ago by Glenn Washburn <devlopment@…>

After talking with Julien, I realize I was partly wrong, what I wanted is done by this fix. And I need to file a new issue about this naked except.

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