Common middleware does not respect urlconf override in request

[Trey]

When running the append_slash chunk of code the common middleware does not adhere to the overridden urlconf when specified in request.

[Trey]

common middleware patch

[Russell Keith-Magee]

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

[Karen Tracey]

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

[skip]

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

[mikexstudios]

@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

[mikexstudios]

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

[skevy]

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

[skevy]

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

[mikexstudios]

@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.

[Karen Tracey]

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?

[mikexstudios]

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.

2. 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.

3. 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.

4. 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.

5. For test_prepend_www_custom_urlconf (line 215), test passes because we have APPEND_SLASH = False.

6. 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.

[skevy]

Thanks Mike, I saw that this morning when trey updated the ticket, but didn't have time to post a new patch. Good work :-)

[mikexstudios]

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

[Karen Tracey]

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

[Karen Tracey]

(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.

[Jacob]

Milestone 1.2 deleted