#26428 closed New feature (fixed)
Add support for relative path redirects to the test Client
Reported by: | master | Owned by: | nobody |
---|---|---|---|
Component: | Testing framework | Version: | 1.9 |
Severity: | Release blocker | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Consider this definition, in a reusable app:
url(r'^inbox/$', InboxView.as_view(), name='inbox'), url(r'^$', RedirectView.as_view(url='inbox/')),
And
urlpatterns = [ url(r'^messages/', include((app_name_patterns, 'app_name'), namespace='app_name')), ]
This was working on 1.8, thanks to http.fix_location_header, which converts the url to something like http://testserver/messages/inbox/
.
1.9 introduced the "HTTP redirects no longer forced to absolute URIs" change and the fix has disappeared, the reason being "... allows relative URIs in Location, recognizing the actual practice of user agents, almost all of which support them.".
Unfortunately, this is not fully the case of test.Client. It doesn't support relative-path reference - not beginning with a slash character, but only absolute-path reference - beginning with a single slash character (ref). A GET to inbox/
leads to 404.
I'm using a workaround with ... url=reverse_lazy('app_name:inbox') ...
, referred by a TestCase.urls attribute, to produce /messages/inbox/
, but I'm not happy with this hardcoded namespace.
Attachments (2)
Change History (15)
comment:1 by , 9 years ago
Severity: | Normal → Release blocker |
---|---|
Summary: | test.Client doesn't fully support relative-path reference in Redirects → Add support for relative path redirects to the test Client |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → New feature |
by , 9 years ago
Attachment: | 26428-test.diff added |
---|
follow-up: 3 comment:2 by , 9 years ago
Has patch: | set |
---|
Does this patch fix your issue? If not, could you provide a sample of your test code so we understand exactly what you're doing? Thanks.
comment:3 by , 9 years ago
Replying to timgraham
The patch does fix my issue but a simple concatenation doesn't cover all cases.
Suppose your test scenario as: url(r'^accounts/optional_extra$', RedirectView.as_view(url='login/')),
with: response = self.client.get('/accounts/optional_extra')
I think a more appropriate code is:
# Prepend the request path to handle relative-path redirects. if not path.startswith('/'): url = urljoin(response.request['PATH_INFO'], url) path = urljoin(response.request['PATH_INFO'], path)
follow-up: 5 comment:4 by , 9 years ago
Thanks for the feedback. I updated the pull request for your suggestion and added some release notes for 1.9.6.
comment:8 by , 9 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
Though the fix solves the original problem i.e. assertRedirects(), the summary of the ticket suggests broader support.
I've attached a patch based on the original fix, that also fixes Client.get() / Client.post() etc. when follow=True.
comment:11 by , 9 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:12 by , 6 years ago
This issue was not fully addressed. There are still a couple problems:
1) It assumes that a relative redirect does not start with '/'. But what if you redirect relative to host with a '/', as in redirecting to '/subpath' instead of 'https://example.com/subpath'?
2) It does not set the secure kwarg on the client.get call based on the original host scheme, because urlsplit is called only on the relative path. It seems that urlsplit should be called after building an absolute url.
Should this ticket be re-opened, or a separate ticket created?
comment:13 by , 6 years ago
New ticket, please, as the fix for this one has been released for years.
I think we should try to fix it in 1.9 given the regression nature of the report. A test is attached.