Admin should not handle IncorrectLookupParameters if settings.DEBUG is set

[Glenn Washburn <development@…>]

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.

[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]

[Aymeric Augustin]

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

[Chris Adams]

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.

[Chris Adams]

GitHub-generated patch

[Aymeric Augustin]

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

[Julien Phalip]

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.

[Jannis Leidel]

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

[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?

[Julien Phalip]

Without naked except

[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.

[Julien Phalip]

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].

[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.

[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.