Fixed #24720 -- Moved append slash checking in CommonMiddleware to process_response() #4574
Conversation
…called To avoid resolving the url twice for all urls that do not end in a slash - only check if we should add the slash if we receive a 404 response in process_response (if we are prepending www it still does the check in process_request to avoid multiple redirects) This exposes a bug(?!?) in the TestClient (see test_client_regress.tests.ContextTests.test_nested_requests). It appears that returning a redirect from process_response behaves differently than returning a redirect from process_request that leads to extra context being added to the response.context.
| @@ -39,6 +39,8 @@ def test_append_slash_have_slash(self): | |||
| """ | |||
| request = self.rf.get('/slash/') | |||
| self.assertEqual(CommonMiddleware().process_request(request), None) | |||
| response = HttpResponseNotFound | |||
spotted by kezabelle
|
After some research I have determined that what I thought was a bug in the test client is just a minor issue with the test test_client_regress.tests.ContextTests.test_nested_requests. The triggering of the common middleware appears to be accidental and not an intentional part of the test. The extra context is added to the response because a 404 response is generated using a template (but never sent to the client) when the original url lookup fails. We may want to note this in the documentation, but I am not sure of the best place for it. |
The triggering of the “add slash” functionality of the common middleware appears to be accidental and not an intentional part of the test.
timgraham
changed the title
|
This looks good. Here are some cosmetic edits: http://dpaste.com/329WCVC |
|
@electricjay, are you able to apply my patch, add release notes, and squash commits? |
|
merged in 434d309, thanks! |
Addresses issue #24720 - using middleware.common to append slashes causes extra overhead to all requests that do not end in a slash
https://code.djangoproject.com/ticket/24720
To avoid resolving the url twice for all urls that do not end in a
slash - only check if we should add the slash if we receive a 404
response in process_response (if we are prepending www it still does
the check in process_request to avoid multiple redirects)
Avoiding this double url resolution speeds up the affected requests by about 5%
This pull request exposes a bug(?!?) in the TestClient (see
test_client_regress.tests.ContextTests.test_nested_requests). It
appears that returning a redirect from process_response behaves
differently than returning a redirect from process_request that leads
to extra context being added to the response.context when making
nested calls. Possibly related to the template_rendered signal (or not).