Opened 16 years ago

Closed 14 years ago

Last modified 12 years ago

#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)

6941_common_middleware_urlresolver.patch (1011 bytes ) - added by Trey 16 years ago.
common middleware patch
common_middleware_urlconf_patch.diff (7.0 KB ) - added by skevy 14 years ago.
Working patch for this ticket w/tests. Disregard other.
common_middleware_urlconf_patch_improved.diff (7.4 KB ) - added by mikexstudios 14 years ago.
Improved previous patch by skevy so that some of the new tests fail without patch.

Download all attachments as: .zip

Change History (21)

by Trey, 16 years ago

common middleware patch

comment:1 by Simon Greenhill <dev@…>, 16 years ago

Triage Stage: UnreviewedReady for checkin

comment:2 by Russell Keith-Magee, 16 years ago

Needs tests: set
Triage Stage: Ready for checkinAccepted

Note to triage team - Tests are not an optional extra. Any nontrivial change that can be tested, should be tested.

comment:3 by Karen Tracey, 15 years ago

#10481 is a dupe, I believe. Different patch, still no test.

comment:4 by skip, 14 years ago

#5034 may address this issue, though I haven't tested it against the append_slash case.

comment:5 by mikexstudios, 14 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 mikexstudios, 14 years ago

Also, the patch included in this ticket does not work with trunk. However, the patch from #10481 works.

comment:7 by skevy, 14 years ago

I too would like to see this get fixed asap. Mikexstudios - lets work on fixing this.

by skevy, 14 years ago

Working patch for this ticket w/tests. Disregard other.

comment:8 by skevy, 14 years ago

Needs tests: unset

comment:9 by skevy, 14 years ago

Version: SVN1.2-beta

comment:10 by skevy, 14 years ago

milestone: 1.2

comment:11 by anonymous, 14 years ago

Cc: adam.skevy@… added

comment:12 by mikexstudios, 14 years ago

Cc: mike.huynh+django@… 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 Karen Tracey, 14 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 mikexstudios, 14 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:

  1. 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.
  1. 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.
  1. 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.
  1. 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.
  1. For test_prepend_www_custom_urlconf (line 215), test passes because we have APPEND_SLASH = False.
  1. 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 skevy, 14 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 mikexstudios, 14 years ago

Improved previous patch by skevy so that some of the new tests fail without patch.

comment:16 by Karen Tracey, 14 years ago

Resolution: fixed
Status: newclosed

(In [12704]) Fixed #6228: Changed common middleware to respect request-specific urlconf. Thanks trey, skevy, and mikexstudios.

comment:17 by Karen Tracey, 14 years ago

(In [12706]) [1.1.X] Fixed #6228: Changed common middleware to respect request-specific urlconf. Thanks trey, skevy, and mikexstudios.

r12704 and r12705 from trunk.

comment:18 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top