Code

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#17828 closed Cleanup/optimization (fixed)

Admin should not handle IncorrectLookupParameters if settings.DEBUG is set

Reported by: Glenn Washburn <development@…> Owned by: acdha
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 acdha 2 years ago.
GitHub-generated patch
17828.listfilter-proper-error-handling.diff (6.5 KB) - added by julien 2 years ago.
17828.listfilter-proper-error-handling.2.diff (7.5 KB) - added by julien 2 years ago.
17828.listfilter-proper-error-handling.3.diff (7.5 KB) - added by julien 2 years ago.
Without naked except

Download all attachments as: .zip

Change History (16)

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

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

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

  • Triage Stage changed from Unreviewed to Accepted

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

comment:3 Changed 2 years ago by acdha

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

comment:4 Changed 2 years ago by acdha

  • Cc chris@… added
  • Easy pickings set
  • Has patch set
  • Type changed from Uncategorized to Cleanup/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 2 years ago by acdha

GitHub-generated patch

comment:5 Changed 2 years ago by aaugustin

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

Last edited 2 years ago by aaugustin (previous) (diff)

comment:6 Changed 2 years ago by julien

  • Severity changed from Normal to Release 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 2 years ago by julien

comment:7 follow-up: Changed 2 years ago by jezdez

  • Triage Stage changed from Accepted to Ready 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 2 years ago by acdha

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

Without naked except

comment:9 Changed 2 years ago by julien

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

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

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

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.