Code

Opened 7 years ago

Closed 4 years ago

Last modified 3 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: UI/UX:

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 7 years ago.
common middleware patch
common_middleware_urlconf_patch.diff (7.0 KB) - added by skevy 4 years ago.
Working patch for this ticket w/tests. Disregard other.
common_middleware_urlconf_patch_improved.diff (7.4 KB) - added by mikexstudios 4 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)

Changed 7 years ago by trey

common middleware patch

comment:1 Changed 6 years ago by Simon Greenhill <dev@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Ready for checkin

comment:2 Changed 6 years ago by russellm

  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

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

comment:3 Changed 5 years ago by kmtracey

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

comment:4 Changed 5 years ago by skip

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

comment:5 Changed 4 years ago by 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

comment:6 Changed 4 years ago by mikexstudios

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

comment:7 Changed 4 years ago by skevy

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

Changed 4 years ago by skevy

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

comment:8 Changed 4 years ago by skevy

  • Needs tests unset

comment:9 Changed 4 years ago by skevy

  • Version changed from SVN to 1.2-beta

comment:10 Changed 4 years ago by skevy

  • milestone set to 1.2

comment:11 Changed 4 years ago by anonymous

  • Cc adam.skevy@… added

comment:12 Changed 4 years ago by mikexstudios

  • 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 Changed 4 years ago by kmtracey

  • 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 Changed 4 years ago by mikexstudios

  • 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 Changed 4 years ago by 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 :-)

Changed 4 years ago by mikexstudios

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

comment:16 Changed 4 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:17 Changed 4 years ago by kmtracey

(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 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.