From 434d309ef6dbecbfd2b322d3a1da78aa5cb05fa8 Mon Sep 17 00:00:00 2001 From: Jay Cox Date: Mon, 27 Apr 2015 22:23:42 -0700 Subject: [PATCH] Fixed #24720 -- Avoided resolving URLs that don't end in a slash twice in CommonMiddleware. This speeds up affected requests by about 5%. --- django/middleware/common.py | 83 +++++++++++++++++++----------- tests/middleware/tests.py | 60 +++++++++++++++------ tests/test_client_regress/views.py | 2 +- 3 files changed, 96 insertions(+), 49 deletions(-) diff --git a/django/middleware/common.py b/django/middleware/common.py index 3d7f365af41..6773eff6140 100644 --- a/django/middleware/common.py +++ b/django/middleware/common.py @@ -50,47 +50,68 @@ class CommonMiddleware(object): if user_agent_regex.search(request.META['HTTP_USER_AGENT']): raise PermissionDenied('Forbidden user agent') - # Check for a redirect based on settings.APPEND_SLASH - # and settings.PREPEND_WWW + # Check for a redirect based on settings.PREPEND_WWW host = request.get_host() - old_url = [host, request.get_full_path()] - new_url = old_url[:] - if (settings.PREPEND_WWW and old_url[0] and - not old_url[0].startswith('www.')): - new_url[0] = 'www.' + old_url[0] + if settings.PREPEND_WWW and host and not host.startswith('www.'): + host = 'www.' + host - # Append a slash if APPEND_SLASH is set and the URL doesn't have a - # trailing slash and there is no pattern for the current path - if settings.APPEND_SLASH and (not old_url[1].endswith('/')): + # Check if we also need to append a slash so we can do it all + # with a single redirect. + if self.should_redirect_with_slash(request): + path = self.get_full_path_with_slash(request) + else: + path = request.get_full_path() + + return self.response_redirect_class('%s://%s%s' % (request.scheme, host, path)) + + def should_redirect_with_slash(self, request): + """ + Return True if settings.APPEND_SLASH is True and appending a slash to + the request path turns an invalid path into a valid one. + """ + if settings.APPEND_SLASH and not request.get_full_path().endswith('/'): urlconf = getattr(request, 'urlconf', None) - if (not urlresolvers.is_valid_path(request.path_info, urlconf) and - urlresolvers.is_valid_path("%s/" % request.path_info, urlconf)): - new_url[1] = request.get_full_path(force_append_slash=True) - if settings.DEBUG and request.method in ('POST', 'PUT', 'PATCH'): - raise RuntimeError(("" - "You called this URL via %(method)s, but the URL doesn't end " - "in a slash and you have APPEND_SLASH set. Django can't " - "redirect to the slash URL while maintaining %(method)s data. " - "Change your form to point to %(url)s (note the trailing " - "slash), or set APPEND_SLASH=False in your Django " - "settings.") % {'method': request.method, 'url': ''.join(new_url)}) + return ( + not urlresolvers.is_valid_path(request.path_info, urlconf) + and urlresolvers.is_valid_path('%s/' % request.path_info, urlconf) + ) + return False - if new_url == old_url: - # No redirects required. - return - if new_url[0] != old_url[0]: - newurl = "%s://%s%s" % ( - request.scheme, - new_url[0], new_url[1]) - else: - newurl = new_url[1] - return self.response_redirect_class(newurl) + def get_full_path_with_slash(self, request): + """ + Return the full path of the request with a trailing slash appended. + + Raise a RuntimeError if settings.DEBUG is True and request.method is + GET, PUT, or PATCH. + """ + new_path = request.get_full_path(force_append_slash=True) + if settings.DEBUG and request.method in ('POST', 'PUT', 'PATCH'): + raise RuntimeError( + "You called this URL via %(method)s, but the URL doesn't end " + "in a slash and you have APPEND_SLASH set. Django can't " + "redirect to the slash URL while maintaining %(method)s data. " + "Change your form to point to %(url)s (note the trailing " + "slash), or set APPEND_SLASH=False in your Django settings." % { + 'method': request.method, + 'url': request.get_host() + new_path, + } + ) + return new_path def process_response(self, request, response): """ Calculate the ETag, if needed. + + When the status code of the response is 404, it may redirect to a path + with an appended slash if should_redirect_with_slash() returns True. """ + # If the given URL is "Not Found", then check if we should redirect to + # a path with a slash appended. + if response.status_code == 404: + if self.should_redirect_with_slash(request): + return self.response_redirect_class(self.get_full_path_with_slash(request)) + if settings.USE_ETAGS: if response.has_header('ETag'): etag = response['ETag'] diff --git a/tests/middleware/tests.py b/tests/middleware/tests.py index aa84a69ca1d..21f45419c0b 100644 --- a/tests/middleware/tests.py +++ b/tests/middleware/tests.py @@ -11,8 +11,8 @@ from django.conf import settings from django.core import mail from django.core.exceptions import PermissionDenied from django.http import ( - FileResponse, HttpRequest, HttpResponse, HttpResponsePermanentRedirect, - HttpResponseRedirect, StreamingHttpResponse, + FileResponse, HttpRequest, HttpResponse, HttpResponseNotFound, + HttpResponsePermanentRedirect, HttpResponseRedirect, StreamingHttpResponse, ) from django.middleware.clickjacking import XFrameOptionsMiddleware from django.middleware.common import ( @@ -39,6 +39,8 @@ class CommonMiddlewareTest(SimpleTestCase): """ request = self.rf.get('/slash/') self.assertEqual(CommonMiddleware().process_request(request), None) + response = HttpResponseNotFound() + self.assertEqual(CommonMiddleware().process_response(request, response), response) @override_settings(APPEND_SLASH=True) def test_append_slash_slashless_resource(self): @@ -47,6 +49,8 @@ class CommonMiddlewareTest(SimpleTestCase): """ request = self.rf.get('/noslash') self.assertEqual(CommonMiddleware().process_request(request), None) + response = HttpResponse("Here's the text of the Web page.") + self.assertEqual(CommonMiddleware().process_response(request, response), response) @override_settings(APPEND_SLASH=True) def test_append_slash_slashless_unknown(self): @@ -54,7 +58,8 @@ class CommonMiddlewareTest(SimpleTestCase): APPEND_SLASH should not redirect to unknown resources. """ request = self.rf.get('/unknown') - self.assertEqual(CommonMiddleware().process_request(request), None) + response = HttpResponseNotFound() + self.assertEqual(CommonMiddleware().process_response(request, response), response) @override_settings(APPEND_SLASH=True) def test_append_slash_redirect(self): @@ -62,7 +67,8 @@ class CommonMiddlewareTest(SimpleTestCase): APPEND_SLASH should redirect slashless URLs to a valid pattern. """ request = self.rf.get('/slash') - r = CommonMiddleware().process_request(request) + response = HttpResponseNotFound() + r = CommonMiddleware().process_response(request, response) self.assertEqual(r.status_code, 301) self.assertEqual(r.url, '/slash/') @@ -72,7 +78,8 @@ class CommonMiddlewareTest(SimpleTestCase): APPEND_SLASH should preserve querystrings when redirecting. """ request = self.rf.get('/slash?test=1') - r = CommonMiddleware().process_request(request) + response = HttpResponseNotFound() + r = CommonMiddleware().process_response(request, response) self.assertEqual(r.url, '/slash/?test=1') @override_settings(APPEND_SLASH=True, DEBUG=True) @@ -85,16 +92,17 @@ class CommonMiddlewareTest(SimpleTestCase): msg = "maintaining %s data. Change your form to point to testserver/slash/" request = self.rf.get('/slash') request.method = 'POST' + response = HttpResponseNotFound() with six.assertRaisesRegex(self, RuntimeError, msg % request.method): - CommonMiddleware().process_request(request) + CommonMiddleware().process_response(request, response) request = self.rf.get('/slash') request.method = 'PUT' with six.assertRaisesRegex(self, RuntimeError, msg % request.method): - CommonMiddleware().process_request(request) + CommonMiddleware().process_response(request, response) request = self.rf.get('/slash') request.method = 'PATCH' with six.assertRaisesRegex(self, RuntimeError, msg % request.method): - CommonMiddleware().process_request(request) + CommonMiddleware().process_response(request, response) @override_settings(APPEND_SLASH=False) def test_append_slash_disabled(self): @@ -102,7 +110,8 @@ class CommonMiddlewareTest(SimpleTestCase): Disabling append slash functionality should leave slashless URLs alone. """ request = self.rf.get('/slash') - self.assertEqual(CommonMiddleware().process_request(request), None) + response = HttpResponseNotFound() + self.assertEqual(CommonMiddleware().process_response(request, response), response) @override_settings(APPEND_SLASH=True) def test_append_slash_quoted(self): @@ -110,7 +119,8 @@ class CommonMiddlewareTest(SimpleTestCase): URLs which require quoting should be redirected to their slash version ok. """ request = self.rf.get(quote('/needsquoting#')) - r = CommonMiddleware().process_request(request) + response = HttpResponseNotFound() + r = CommonMiddleware().process_response(request, response) self.assertEqual(r.status_code, 301) self.assertEqual( r.url, @@ -152,6 +162,8 @@ class CommonMiddlewareTest(SimpleTestCase): request = self.rf.get('/customurlconf/slash/') request.urlconf = 'middleware.extra_urls' self.assertEqual(CommonMiddleware().process_request(request), None) + response = HttpResponseNotFound() + self.assertEqual(CommonMiddleware().process_response(request, response), response) @override_settings(APPEND_SLASH=True) def test_append_slash_slashless_resource_custom_urlconf(self): @@ -161,6 +173,8 @@ class CommonMiddlewareTest(SimpleTestCase): request = self.rf.get('/customurlconf/noslash') request.urlconf = 'middleware.extra_urls' self.assertEqual(CommonMiddleware().process_request(request), None) + response = HttpResponse("Here's the text of the Web page.") + self.assertEqual(CommonMiddleware().process_response(request, response), response) @override_settings(APPEND_SLASH=True) def test_append_slash_slashless_unknown_custom_urlconf(self): @@ -170,6 +184,8 @@ class CommonMiddlewareTest(SimpleTestCase): request = self.rf.get('/customurlconf/unknown') request.urlconf = 'middleware.extra_urls' self.assertEqual(CommonMiddleware().process_request(request), None) + response = HttpResponseNotFound() + self.assertEqual(CommonMiddleware().process_response(request, response), response) @override_settings(APPEND_SLASH=True) def test_append_slash_redirect_custom_urlconf(self): @@ -178,7 +194,8 @@ class CommonMiddlewareTest(SimpleTestCase): """ request = self.rf.get('/customurlconf/slash') request.urlconf = 'middleware.extra_urls' - r = CommonMiddleware().process_request(request) + response = HttpResponseNotFound() + r = CommonMiddleware().process_response(request, response) self.assertIsNotNone(r, "CommonMiddlware failed to return APPEND_SLASH redirect using request.urlconf") self.assertEqual(r.status_code, 301) @@ -194,8 +211,9 @@ class CommonMiddlewareTest(SimpleTestCase): request = self.rf.get('/customurlconf/slash') request.urlconf = 'middleware.extra_urls' request.method = 'POST' + response = HttpResponseNotFound() with six.assertRaisesRegex(self, RuntimeError, 'end in a slash'): - CommonMiddleware().process_request(request) + CommonMiddleware().process_response(request, response) @override_settings(APPEND_SLASH=False) def test_append_slash_disabled_custom_urlconf(self): @@ -205,6 +223,8 @@ class CommonMiddlewareTest(SimpleTestCase): request = self.rf.get('/customurlconf/slash') request.urlconf = 'middleware.extra_urls' self.assertEqual(CommonMiddleware().process_request(request), None) + response = HttpResponseNotFound() + self.assertEqual(CommonMiddleware().process_response(request, response), response) @override_settings(APPEND_SLASH=True) def test_append_slash_quoted_custom_urlconf(self): @@ -213,7 +233,8 @@ class CommonMiddlewareTest(SimpleTestCase): """ request = self.rf.get(quote('/customurlconf/needsquoting#')) request.urlconf = 'middleware.extra_urls' - r = CommonMiddleware().process_request(request) + response = HttpResponseNotFound() + r = CommonMiddleware().process_response(request, response) self.assertIsNotNone(r, "CommonMiddlware failed to return APPEND_SLASH redirect using request.urlconf") self.assertEqual(r.status_code, 301) @@ -262,12 +283,16 @@ class CommonMiddlewareTest(SimpleTestCase): """Regression test for #15152""" request = self.rf.get('/slash') request.META['QUERY_STRING'] = force_str('drink=café') - response = CommonMiddleware().process_request(request) - self.assertEqual(response.status_code, 301) + r = CommonMiddleware().process_request(request) + self.assertIsNone(r) + response = HttpResponseNotFound() + r = CommonMiddleware().process_response(request, response) + self.assertEqual(r.status_code, 301) def test_response_redirect_class(self): request = self.rf.get('/slash') - r = CommonMiddleware().process_request(request) + response = HttpResponseNotFound() + r = CommonMiddleware().process_response(request, response) self.assertEqual(r.status_code, 301) self.assertEqual(r.url, '/slash/') self.assertIsInstance(r, HttpResponsePermanentRedirect) @@ -277,7 +302,8 @@ class CommonMiddlewareTest(SimpleTestCase): response_redirect_class = HttpResponseRedirect request = self.rf.get('/slash') - r = MyCommonMiddleware().process_request(request) + response = HttpResponseNotFound() + r = MyCommonMiddleware().process_response(request, response) self.assertEqual(r.status_code, 302) self.assertEqual(r.url, '/slash/') self.assertIsInstance(r, HttpResponseRedirect) diff --git a/tests/test_client_regress/views.py b/tests/test_client_regress/views.py index d21448cba11..6ba419833a7 100644 --- a/tests/test_client_regress/views.py +++ b/tests/test_client_regress/views.py @@ -66,7 +66,7 @@ def nested_view(request): """ setup_test_environment() c = Client() - c.get("/no_template_view") + c.get("/no_template_view/") return render_to_response('base.html', {'nested': 'yes'})