Opened 3 years ago

Closed 3 years ago

#32260 closed Cleanup/optimization (fixed)

handler500 as a Class-based view raises SystemCheckError

Reported by: Daniyal Abbasi Owned by: Daniyal Abbasi
Component: Core (System checks) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Daniyal Abbasi)

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.

Change History (10)

comment:1 by Daniyal Abbasi, 3 years ago

Description: modified (diff)
Owner: changed from nobody to Daniyal Abbasi
Status: newassigned

comment:2 by Carlton Gibson, 3 years ago

Has patch: set
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

PR. OK, yes, using a CBV here is reasonable. It would be nice if the check handled that sensibly. Thanks.

comment:3 by Mariusz Felisiak, 3 years ago

Patch needs improvement: set

Marking as "needs improvement" per Adam's comment.

comment:4 by Daniyal Abbasi, 3 years ago

Patch needs improvement: unset

comment:5 by Daniyal Abbasi, 3 years ago

An update PR has been created, as suggested in the comments of the previous PR!

comment:6 by Nick Pope, 3 years ago

Needs tests: set
Version: 3.1dev

comment:7 by Nick Pope, 3 years ago

Needs tests: unset

comment:8 by Nick Pope, 3 years ago

Triage Stage: AcceptedReady for checkin

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 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 PR that stops using functools.update_wrapper() and only sets certain special attributes.

Marking this as RFC.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 0c0b8772:

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.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 7c08f26b:

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.pope@…>

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