Opened 2 years ago

Closed 2 years ago

#29642 closed New feature (fixed)

Add check for signatures of custom error views

Reported by: Adam (Chainz) Johnson Owned by: Adam (Chainz) Johnson
Component: Core (System checks) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In https://github.com/django/django/pull/10249 I fixed the documentation for the change in #24733 to add the 'exception' argument to 3 out of the 4 custom error handlers. This came from a real life problem where I'd added handlers but without the 'exception' argument, then when the handler was triggered, an error was raised due to it not accepting the right types. The feedback loop for this failure is pretty long as these only get hit when DEBUG=False and not during tests.

We should check the signatures of these handler functions with a django check (i.e. that they accept 1 or 2 arguments, depending on which one they are) in order to prevent this happening to others.

Change History (8)

comment:1 Changed 2 years ago by Carlton Gibson

Triage Stage: UnreviewedAccepted
Version: 2.1master

Yes. That seems more than reasonable.

comment:2 Changed 2 years ago by Tim Graham

I'm not immediately convinced. These "simple checks" often turn out to be more complicated than they seem. Didn't you write some tests for your custom views? ;-)

comment:3 Changed 2 years ago by Carlton Gibson

It’s also about what our ‘’Average User™’’ might do. Yes, Adam should write tests, but I can easily see people putting a handler in place and never really trying it out.

The feedback loop for this failure is pretty long as these only get hit when DEBUG=False and not during tests.

This is the bit that convinced me. You write an error handler, and then realise that it’s not straitforwardly testable, with the test client etc.

comment:4 Changed 2 years ago by Tim Graham

Maybe a documentation suggestion to facilitate testing would be helpful:

if settings.DEBUG:
    urlpatterns += [
        path('500/', 'path.to.custom_500_handler'),
    )

I think it's more empowering than "check the signature and assume the rest of the view works." ;-)

comment:5 Changed 2 years ago by Carlton Gibson

Yep. I can see that.

comment:6 Changed 2 years ago by Adam (Chainz) Johnson

Owner: changed from nobody to Adam (Chainz) Johnson
Status: newassigned

Maybe a documentation suggestion to facilitate testing would be helpful

I did this for my initial unit tests, it only half helps since then exception isn't passed. So a view which only takes one argument would work with such a test view.

In fact the better way to test handlers is like django's test suite for them, with other views that raise exceptions: https://github.com/django/django/blob/master/tests/handlers/tests_custom_error_handlers.py#L23

These "simple checks" often turn out to be more complicated than they seem. Didn't you write some tests for your custom views? ;-)

That's the worst thing here, I did, and they were wrong. So there was a double loop.

The check is actually quite simple and I have a draft implementation. Also there was no check before that the given paths are genuine so this will add defence to two different potential failures, for each of the 4 handlers.

comment:7 Changed 2 years ago by Tim Graham

Has patch: set

comment:8 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 245c36d7:

Fixed #29642 -- Added check for arguments of custom error handler views.

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