[Django] #32260: handler500 as a Class-based view raises SystemCheckError
Django
------------------------------------------------+------------------------
Reporter: Daniyal Abbasi | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: 3.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 |
------------------------------------------------+------------------------
Setting handler500 as a Class-Based view raises the following error which
running checks.
{{{
$ python manage.py check
SystemCheckError: System check identified some issues:
ERRORS:
?: (urls.E007) The custom handler500 view 'path.to.my.MyView' does not
take the correct number of arguments (request).
}}}
In my root urls.py, I have the following configuration,
{{{
handler404 = MyView.as_view()
handler500 = MyView.as_view()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32260>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Daniyal Abbasi | Owner: Daniyal
| Abbasi
Type: Bug | Status: assigned
Component: Core (System | Version: 3.1
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Daniyal Abbasi):
* owner: nobody => Daniyal Abbasi
* status: new => assigned
Old description:
> Setting handler500 as a Class-Based view raises the following error which
> running checks.
>
> {{{
> $ python manage.py check
> SystemCheckError: System check identified some issues:
>
> ERRORS:
> ?: (urls.E007) The custom handler500 view 'path.to.my.MyView' does not
> take the correct number of arguments (request).
> }}}
>
> In my root urls.py, I have the following configuration,
>
> {{{
> handler404 = MyView.as_view()
> handler500 = MyView.as_view()
> }}}
New description:
Setting handler500 as a Class-Based view raises the following error which
running checks.
{{{
$ python manage.py check
SystemCheckError: System check identified some issues:
ERRORS:
?: (urls.E007) The custom handler500 view 'path.to.my.MyView' does not
take the correct number of arguments (request).
}}}
In my root urls.py, I have the following configuration,
{{{
handler404 = MyView.as_view()
handler500 = MyView.as_view()
}}}
I believe this is due to the function `_check_custom_error_handlers` in
`django/urls/resolver.py`. The signature variable in this function is
expected to match (request, exception) for all handlers except for
handler500 which is expected to have only (request). A positional
argument, template_name is also present.
While using class based views, we get two positional arguments (self,
request) and then it recieves *args and * *kwargs. The check is permitting
other handlers as the number of arguments coincidentally match.
I suggest a fix in the `_check_custom_error_handlers` which first checks
if the handler* are function based or class based, and then it preceed the
check with the appropriate number of arguments.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32260#comment:1>
Django
Reporter: Daniyal Abbasi | Owner: Daniyal
Cleanup/optimization | Status: assigned
Component: Core (System | Version: 3.1
checks) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* has_patch: 0 => 1
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
[https://github.com/django/django/pull/13766 PR]. OK, yes, using a CBV
here is reasonable. It would be nice if the check handled that sensibly.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/32260#comment:2>
Django
Reporter: Daniyal Abbasi | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: assigned
Component: Core (System | Version: 3.1
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
Comment:
Marking as "needs improvement" per
[https://github.com/django/django/pull/13766#issuecomment-754053247 Adam's
comment].
--
Ticket URL: <https://code.djangoproject.com/ticket/32260#comment:3>
Django
Reporter: Daniyal Abbasi | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: assigned
Component: Core (System | Version: 3.1
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Daniyal Abbasi):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32260#comment:4>
Django
Reporter: Daniyal Abbasi | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: assigned
Component: Core (System | Version: 3.1
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Daniyal Abbasi):
An update [https://github.com/django/django/pull/14124 PR] has been
created, as suggested in the comments of the previous PR!
--
Ticket URL: <https://code.djangoproject.com/ticket/32260#comment:5>
Django
Reporter: Daniyal Abbasi | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: assigned
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nick Pope):
* version: 3.1 => dev
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32260#comment:6>
Django
Reporter: Daniyal Abbasi | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: assigned
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nick Pope):
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32260#comment:7>
Django
Reporter: Daniyal Abbasi | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: assigned
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nick Pope):
* stage: Accepted => Ready for checkin
Comment:
So we've got consensus on the approach from a few people reviewing on
GitHub.
It turned out that this was a bit more messy than expected due to the use
of `functools.update_wrapper()`. This adds `__wrapped__` which was causing
the issue with the signature inspection. We also noticed that this caused
clobbering of the `__name__` and `__qualname__` of the generated view
function with those of the class which is misleading. We already have
`.view_class` available, so that should be used.
The first part of the solution is Adam's
[https://github.com/django/django/pull/14138 PR] that makes use of
`.view_class` appropriately in various places.
The second part (which will need rebasing on top of the first part) is
Daniyal's [https://github.com/django/django/pull/14124 PR] that stops
using `functools.update_wrapper()` and only sets certain special
attributes.
Marking this as RFC.
--
Ticket URL: <https://code.djangoproject.com/ticket/32260#comment:8>
Django
Reporter: Daniyal Abbasi | Owner: Daniyal
Type: | Abbasi
Cleanup/optimization | Status: assigned
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0c0b87725bbcffca3bc3a7a2c649995695a5ae3b" 0c0b8772]:
{{{
#!CommitTicketReference repository=""
revision="0c0b87725bbcffca3bc3a7a2c649995695a5ae3b"
Refs #32260 -- Made admindocs and technical 404 debug page use
view_func.view_class.
Internals of admindocs and technical 404 debug page should use the
view_class attribute and do not rely on __name__.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32260#comment:9>
Django
Reporter: Daniyal Abbasi | Owner: Daniyal
Type: | Abbasi
Component: Core (System | Version: dev
checks) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"7c08f26bf0439c1ed593b51b51ad847f7e262bc1" 7c08f26b]:
{{{
#!CommitTicketReference repository=""
revision="7c08f26bf0439c1ed593b51b51ad847f7e262bc1"
Fixed #32260 -- Made View.as_view() do not use update_wrapper().
View.as_view() should not use update_wrapper() for copying attributes
it's unintended and have side-effects such as adding `self` to the
signature.
This also fixes system check for arguments of custom error handler
views with class-based views.
Co-authored-by: Nick Pope <nick...@flightdataservices.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32260#comment:10>