#6228 closed (fixed)
Common middleware does not respect urlconf override in request
Reported by: | Trey | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | 1.2-beta |
Severity: | Keywords: | middleware, common | |
Cc: | adam.skevy@…, mike.huynh+django@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
When running the append_slash chunk of code the common middleware does not adhere to the overridden urlconf when specified in request.
Attachments (3)
Change History (21)
by , 17 years ago
Attachment: | 6941_common_middleware_urlresolver.patch added |
---|
comment:1 by , 17 years ago
Triage Stage: | Unreviewed → Ready for checkin |
---|
comment:2 by , 16 years ago
Needs tests: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
Note to triage team - Tests are not an optional extra. Any nontrivial change that can be tested, should be tested.
comment:4 by , 15 years ago
#5034 may address this issue, though I haven't tested it against the append_slash case.
comment:5 by , 15 years ago
@skip No, #5034 does not fix this issue. I tested this against the APPEND_SLASH config.
I would really like to see this be part of 1.2 and would be happy to write tests for this patch. But where should the test go? I can see two places where it might exist:
django/trunk/tests/regressiontests/urlpatterns_reverse/tests.py under class RequestURLconfTests: (line 253)
or
django/trunk/tests/regressiontests/middleware/tests.py under class CommonMiddlewareTest
comment:6 by , 15 years ago
Also, the patch included in this ticket does not work with trunk. However, the patch from #10481 works.
comment:7 by , 15 years ago
I too would like to see this get fixed asap. Mikexstudios - lets work on fixing this.
by , 15 years ago
Attachment: | common_middleware_urlconf_patch.diff added |
---|
Working patch for this ticket w/tests. Disregard other.
comment:8 by , 15 years ago
Needs tests: | unset |
---|
comment:9 by , 15 years ago
Version: | SVN → 1.2-beta |
---|
comment:10 by , 15 years ago
milestone: | → 1.2 |
---|
comment:11 by , 15 years ago
Cc: | added |
---|
comment:12 by , 15 years ago
Cc: | added |
---|
@skevy Wow, that's awesome. Thanks for making a new patch and creating tests.
I reviewed skevy's patch and it is sound. The patch applies cleanly to django trunk (r12657) and middlware tests all pass.
comment:13 by , 15 years ago
Patch needs improvement: | set |
---|
Unfortunately the new tests all pass both with and without the code change. Somehow they are not testing the actual bug the code is intended to fix?
comment:14 by , 15 years ago
Patch needs improvement: | unset |
---|
Doh! I looked at the tests again. The reason why all the tests pass without the patch is that the new tests test for the exact same urls as the tests without the overridden urlconf (that is, the tests using ROOT_URLCONF). Thus, when the patch wasn't applied, the new tests still pass because they fallback on ROOT_URLCONF, which has the exact same urlpattern as extra_urls.py!
Therefore, I changed the urls in extra_urls.py by prepending them with "customurlconf/". For example:
(r'^middleware/noslash$', 'view'),
becomes:
(r'^middleware/customurlconf/noslash$', 'view'),
Then, I made similar changes to the new tests. For example:
request = self._get_request('slash')
becomes:
request = self._get_request('customurlconf/slash')
So now 4 of the 10 new tests fail when run on unpatched trunk. The other tests succeed without patch because:
- For test_append_slash_have_slash_custom_urlconf (line 133), we are testing that URLs with slashes go unmolested. So even if we have an unknown URL, the test will succeed because we have a slash on the end.
- For test_append_slash_slashless_resource_custom_urlconf (line 142), we are testing that explicit slashless URLs go unmolested. Again, if unknown URL, test will pass because CommonMiddlware will check for the unknown URL + '/', which is not valid. Therefore, None is returned and the test passes.
- For test_append_slash_slashless_unknown_custom_urlconf (line 151). We are testing unknown URL. Test passes because of reason given in #2. This is expected behavior.
- For test_append_slash_disabled_custom_urlconf (line 192), we are testing that nothing happens when APPEND_SLASH = False. Test passes because this is expected behavior.
- For test_prepend_www_custom_urlconf (line 215), test passes because we have APPEND_SLASH = False.
- For test_prepend_www_append_slash_have_slash_custom_urlconf (line 226), we are testing that 'www' is prepended to a url ending with a slash. Test passes despite the url being unknown because there is already a slash at the end of the URL.
Updated patch with new tests are attached.
comment:15 by , 15 years ago
Thanks Mike, I saw that this morning when trey updated the ticket, but didn't have time to post a new patch. Good work :-)
by , 15 years ago
Attachment: | common_middleware_urlconf_patch_improved.diff added |
---|
Improved previous patch by skevy so that some of the new tests fail without patch.
comment:16 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
common middleware patch