Opened 12 years ago

Closed 12 years ago

Last modified 12 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 12 years ago.
GitHub-generated patch
17828.listfilter-proper-error-handling.diff (6.5 KB ) - added by Julien Phalip 12 years ago.
17828.listfilter-proper-error-handling.2.diff (7.5 KB ) - added by Julien Phalip 12 years ago.
17828.listfilter-proper-error-handling.3.diff (7.5 KB ) - added by Julien Phalip 12 years ago.
Without naked except

Download all attachments as: .zip

Change History (16)

comment:1 by Glenn Washburn <development@…>, 12 years ago

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 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedAccepted

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

comment:3 by Chris Adams, 12 years ago

Owner: changed from nobody to Chris Adams
Status: newassigned

comment:4 by Chris Adams, 12 years ago

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.

by Chris Adams, 12 years ago

GitHub-generated patch

comment:5 by Aymeric Augustin, 12 years ago

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

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

comment:6 by Julien Phalip, 12 years ago

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.

by Julien Phalip, 12 years ago

comment:7 by Jannis Leidel, 12 years ago

Triage Stage: AcceptedReady for checkin

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

in reply to:  7 comment:8 by Chris Adams, 12 years ago

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?

by Julien Phalip, 12 years ago

Without naked except

comment:9 by Julien Phalip, 12 years ago

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 by Julien Phalip, 12 years ago

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 by Glenn Washburn <devlopment@…>, 12 years ago

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 by Glenn Washburn <devlopment@…>, 12 years ago

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