From 245c36d7b6934fb0ca50eed2414253f4793f1ff5 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Fri, 7 Sep 2018 08:33:19 +0100 Subject: [PATCH] Fixed #29642 -- Added check for arguments of custom error handler views. --- django/urls/resolvers.py | 32 ++++++++++++++++--- docs/ref/checks.txt | 2 ++ tests/check_framework/test_urls.py | 24 +++++++++++++- .../urls/bad_error_handlers.py | 10 ++++++ .../urls/good_error_handlers.py | 10 ++++++ 5 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 tests/check_framework/urls/bad_error_handlers.py create mode 100644 tests/check_framework/urls/good_error_handlers.py diff --git a/django/urls/resolvers.py b/django/urls/resolvers.py index 5bfab0c0672..91d21c9da99 100644 --- a/django/urls/resolvers.py +++ b/django/urls/resolvers.py @@ -6,13 +6,14 @@ a string) and returns a ResolverMatch object which provides access to all attributes of the resolved URL match. """ import functools +import inspect import re import threading from importlib import import_module from urllib.parse import quote from django.conf import settings -from django.core.checks import Warning +from django.core.checks import Error, Warning from django.core.checks.urls import check_resolver from django.core.exceptions import ImproperlyConfigured from django.utils.datastructures import MultiValueDict @@ -392,10 +393,33 @@ class URLResolver: ) def check(self): - warnings = [] + messages = [] for pattern in self.url_patterns: - warnings.extend(check_resolver(pattern)) - return warnings or self.pattern.check() + messages.extend(check_resolver(pattern)) + messages.extend(self._check_custom_error_handlers()) + return messages or self.pattern.check() + + def _check_custom_error_handlers(self): + messages = [] + # All handlers take (request, exception) arguments except handler500 + # which takes (request). + for status_code, num_parameters in [(400, 2), (403, 2), (404, 2), (500, 1)]: + handler, param_dict = self.resolve_error_handler(status_code) + signature = inspect.signature(handler) + args = [None] * num_parameters + try: + signature.bind(*args) + except TypeError: + msg = ( + "The custom handler{status_code} view '{path}' does not " + "take the correct number of arguments ({args})." + ).format( + status_code=status_code, + path=handler.__module__ + '.' + handler.__qualname__, + args='request, exception' if num_parameters == 2 else 'request', + ) + messages.append(Error(msg, id='urls.E007')) + return messages def _populate(self): # Short-circuit if called recursively in this thread to prevent diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt index 9c149d46507..6895a93eff7 100644 --- a/docs/ref/checks.txt +++ b/docs/ref/checks.txt @@ -457,6 +457,8 @@ The following checks are performed on your URL configuration: able to reverse all URLs in this namespace. * **urls.E006**: The :setting:`MEDIA_URL`/ :setting:`STATIC_URL` setting must end with a slash. +* **urls.E007**: The custom ``handlerXXX`` view ``'path.to.view'`` does not + take the correct number of arguments (…). ``contrib`` app checks ====================== diff --git a/tests/check_framework/test_urls.py b/tests/check_framework/test_urls.py index aa53af930e2..cdc95957315 100644 --- a/tests/check_framework/test_urls.py +++ b/tests/check_framework/test_urls.py @@ -1,5 +1,5 @@ from django.conf import settings -from django.core.checks.messages import Warning +from django.core.checks.messages import Error, Warning from django.core.checks.urls import ( E006, check_url_config, check_url_namespaces_unique, check_url_settings, get_warning_for_invalid_pattern, @@ -165,6 +165,28 @@ class UpdatedToPathTests(SimpleTestCase): self.assertIn(expected_msg, warning.msg) +class CheckCustomErrorHandlersTests(SimpleTestCase): + + @override_settings(ROOT_URLCONF='check_framework.urls.bad_error_handlers') + def test_bad_handlers(self): + result = check_url_config(None) + self.assertEqual(len(result), 4) + for code, num_params, error in zip([400, 403, 404, 500], [2, 2, 2, 1], result): + with self.subTest('handler{}'.format(code)): + self.assertEqual(error, Error( + "The custom handler{} view " + "'check_framework.urls.bad_error_handlers.bad_handler' " + "does not take the correct number of arguments (request{})." + .format(code, ', exception' if num_params == 2 else ''), + id='urls.E007', + )) + + @override_settings(ROOT_URLCONF='check_framework.urls.good_error_handlers') + def test_good_handlers(self): + result = check_url_config(None) + self.assertEqual(result, []) + + class CheckURLSettingsTests(SimpleTestCase): @override_settings(STATIC_URL='a/', MEDIA_URL='b/') diff --git a/tests/check_framework/urls/bad_error_handlers.py b/tests/check_framework/urls/bad_error_handlers.py new file mode 100644 index 00000000000..d639d707df6 --- /dev/null +++ b/tests/check_framework/urls/bad_error_handlers.py @@ -0,0 +1,10 @@ +urlpatterns = [] + +handler400 = __name__ + '.bad_handler' +handler403 = __name__ + '.bad_handler' +handler404 = __name__ + '.bad_handler' +handler500 = __name__ + '.bad_handler' + + +def bad_handler(): + pass diff --git a/tests/check_framework/urls/good_error_handlers.py b/tests/check_framework/urls/good_error_handlers.py new file mode 100644 index 00000000000..69bea650f7e --- /dev/null +++ b/tests/check_framework/urls/good_error_handlers.py @@ -0,0 +1,10 @@ +urlpatterns = [] + +handler400 = __name__ + '.good_handler' +handler403 = __name__ + '.good_handler' +handler404 = __name__ + '.good_handler' +handler500 = __name__ + '.good_handler' + + +def good_handler(request, exception=None, foo='bar'): + pass