[Django] #26428: test.Client doesn't fully support relative-path reference in Redirects
Django
-----------------------------------+--------------------
Reporter: master | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------
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
([https://tools.ietf.org/html/rfc3986#section-4.2, 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26428>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-----------------------------------+------------------------------------
Reporter: master | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* severity: Normal => Release blocker
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
* type: Bug => New feature
* stage: Unreviewed => Accepted
Comment:
I think we should try to fix it in 1.9 given the regression nature of the
report. A test is attached.
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:1>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* Attachment "26428-test.diff" added.
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
Comment:
Does [https://github.com/django/django/pull/6365 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:2>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by master):
Replying to [comment:2 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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:3>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Thanks for the feedback. I updated the pull request for your suggestion
and added some release notes for 1.9.6.
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:4>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by master):
Replying to [comment:4 timgraham]
Typo in 1.9.6.txt
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:5>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: closed
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"d2569f89f28883d07ede0e44a0a69ae678d3b35f" d2569f89]:
{{{
#!CommitTicketReference repository=""
revision="d2569f89f28883d07ede0e44a0a69ae678d3b35f"
Fixed #26428 -- Added support for relative path redirects in
assertRedirects().
Thanks Trac alias master for the report and review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:6>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: closed
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"192b06594958b701a4b47dd51148a15357ac8f76" 192b0659]:
{{{
#!CommitTicketReference repository=""
revision="192b06594958b701a4b47dd51148a15357ac8f76"
[1.9.x] Fixed #26428 -- Added support for relative path redirects in
assertRedirects().
Thanks Trac alias master for the report and review.
Backport of d2569f89f28883d07ede0e44a0a69ae678d3b35f from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:7>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: closed
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by iktyrrell):
* Attachment "26428-extra.diff" added.
Also fixes Client.get()
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by iktyrrell):
* status: closed => new
* resolution: fixed =>
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:8>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"2f698cd9916cb3d64932248c7114049e02b74fb3" 2f698cd]:
{{{
#!CommitTicketReference repository=""
revision="2f698cd9916cb3d64932248c7114049e02b74fb3"
Refs #26428 -- Added support for relative path redirects to the test
client.
Thanks iktyrrell for the patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:9>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"d2338bae786f2fabfd01c30522fda9d79e02eb45" d2338ba]:
{{{
#!CommitTicketReference repository=""
revision="d2338bae786f2fabfd01c30522fda9d79e02eb45"
[1.9.x] Refs #26428 -- Added support for relative path redirects to the
test client.
Thanks iktyrrell for the patch.
Backport of 2f698cd9916cb3d64932248c7114049e02b74fb3 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:10>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: closed
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:11>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: closed
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jacob):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:12>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: closed
Component: Testing framework | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
New ticket, please, as the fix for this one has been released for years.
--
Ticket URL: <https://code.djangoproject.com/ticket/26428#comment:13>