[Django] #29642: Add check for signatures of custom error views
Django
-------------------------------------------------+------------------------
Reporter: Adam (Chainz) Johnson | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: 2.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29642>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: New feature | Status: new
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* version: 2.1 => master
* stage: Unreviewed => Accepted
Comment:
Yes. That seems more than reasonable.
--
Ticket URL: <https://code.djangoproject.com/ticket/29642#comment:1>
Django
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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? ;-)
--
Ticket URL: <https://code.djangoproject.com/ticket/29642#comment:2>
Django
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29642#comment:3>
Django
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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." ;-)
--
Ticket URL: <https://code.djangoproject.com/ticket/29642#comment:4>
Django
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Yep. I can see that.
--
Ticket URL: <https://code.djangoproject.com/ticket/29642#comment:5>
Django
Reporter: Adam (Chainz) | Owner: Adam
Johnson | (Chainz) Johnson
Type: New feature | Status: assigned
Component: Core (System | Version: master
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam (Chainz) Johnson):
* owner: nobody => Adam (Chainz) Johnson
* status: new => assigned
Comment:
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29642#comment:6>
Django
Reporter: Adam (Chainz) | Owner: Adam
Johnson | (Chainz) Johnson
Type: New feature | Status: assigned
Component: Core (System | Version: master
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/10373 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29642#comment:7>
Django
Reporter: Adam (Chainz) | Owner: Adam
Johnson | (Chainz) Johnson
Component: Core (System | Version: master
checks) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"245c36d7b6934fb0ca50eed2414253f4793f1ff5" 245c36d7]:
{{{
#!CommitTicketReference repository=""
revision="245c36d7b6934fb0ca50eed2414253f4793f1ff5"
Fixed #29642 -- Added check for arguments of custom error handler views.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29642#comment:8>