Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #32260 handler500 as a Class-based view raises SystemCheckError #13766

Closed
wants to merge 4 commits into from

Conversation

abbasidaniyal
Copy link
Contributor

_check_custom_error_handlers in django/urls/resolver.py now checks if the handler is a function based view or class based view.
For class based view, num_parameters needs to be 2 as class based views take two parameters (self, request).

Copy link
Member

@charettes charettes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add tests for CBV handlers as well

Comment on lines 433 to 437
if signature.parameters.get('self'):
args = [None] * 2 # Class based views always take two arguements, (self, request).
else:
args = [None] * num_parameters
Copy link
Member

@charettes charettes Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to allow an extra parameter when it's a class based view

In other words

# Class based views always take two arguements, (self, request).

Is not not true

Suggested change
if signature.parameters.get('self'):
args = [None] * 2 # Class based views always take two arguements, (self, request).
else:
args = [None] * num_parameters
if signature.parameters.get('self'):
num_parameters += 1
args = [None] * num_parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your suggestion!
Simply added +1 to the num_parameters will make the value of num_parameters for handler404, handler403 and handler400 as 3 which is not correct.

for status_code, num_parameters in [(400, 2), (403, 2), (404, 2), (500, 1)]:

I can change it as if signature.parameters.get('self') and status_code==500: but that would be the greatest solution in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add tests for CBV handlers as well

Yes I will do that right away!

_check_custom_error_handlers in django/urls/resolver.py now checks if the handler is a function based view or class based view.
For class based view, num_parameters needs to be 2 as class based views take two parameters (self, request).

Added tests for class based views too.
@adamchainz
Copy link
Sponsor Member

adamchainz commented Jan 4, 2021

I think this ticket is going in the wrong direction and the check has really exposed a bug in the return value of View.as_view().

View.as_view() returns a function that has signature def view(request, *args, **kwargs): - it does not take a self argument. The self is created per-request inside that view() function. However the functools.update_wrapper() calls inside as_view() incorrectly copy the function signature from the class methods, including their self argument, onto the view() function.

I think we should fix View.as_view() to drop the first positional argument (self) from the copied signature. Then the check will not need changing, and any other tools can see the true function signature.

@abbasidaniyal
Copy link
Contributor Author

I think this ticket is going in the wrong direction and the check has really exposed a bug in the return value of View.as_view().

View.as_view() returns a function that has signature def view(request, *args, **kwargs): - it does not take a self argument. The self is created per-request inside that view() function. However the functools.update_wrapper() calls inside as_view() incorrectly copy the function signature from the class methods, including their self argument, onto the view() function.

I think we should fix View.as_view() to drop the first positional argument (self) from the copied signature. Then the check will not need changing, and any other tools can see the true function signature.

That does make a lot of sense. Self should not be present on the signature in the first place!
I'll create a separate PR with the fix soon. Will close this as soon as a new one is created!

@felixxm
Copy link
Member

felixxm commented Jan 27, 2021

@abbasidaniyal Thanks, feel-free to send a new PR.

@felixxm felixxm closed this Jan 27, 2021
@abbasidaniyal
Copy link
Contributor Author

@felixxm Yes. I'm extremely sorry for delaying this. I'll be sending a new PR soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants