Opened 3 years ago

Closed 2 years ago

Last modified 4 months ago

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

26428-test.diff (1.1 KB) - added by Tim Graham 3 years ago.
26428-extra.diff (2.4 KB) - added by Ian Tyrrell 2 years ago.
Also fixes Client.get()

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by Tim Graham

Severity: NormalRelease blocker
Summary: test.Client doesn't fully support relative-path reference in RedirectsAdd support for relative path redirects to the test Client
Triage Stage: UnreviewedAccepted
Type: BugNew feature

I think we should try to fix it in 1.9 given the regression nature of the report. A test is attached.

Changed 3 years ago by Tim Graham

Attachment: 26428-test.diff added

comment:2 Changed 3 years ago by Tim Graham

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 in reply to:  2 Changed 3 years ago by master

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)

comment:4 Changed 3 years ago by Tim Graham

Thanks for the feedback. I updated the pull request for your suggestion and added some release notes for 1.9.6.

comment:5 in reply to:  4 Changed 3 years ago by master

Replying to timgraham

Typo in 1.9.6.txt

comment:6 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In d2569f89:

Fixed #26428 -- Added support for relative path redirects in assertRedirects().

Thanks Trac alias master for the report and review.

comment:7 Changed 3 years ago by Tim Graham <timograham@…>

In 192b0659:

[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

Changed 2 years ago by Ian Tyrrell

Attachment: 26428-extra.diff added

Also fixes Client.get()

comment:8 Changed 2 years ago by Ian Tyrrell

Resolution: fixed
Status: closednew

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:9 Changed 2 years ago by Tim Graham <timograham@…>

In 2f698cd:

Refs #26428 -- Added support for relative path redirects to the test client.

Thanks iktyrrell for the patch.

comment:10 Changed 2 years ago by Tim Graham <timograham@…>

In d2338ba:

[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

comment:11 Changed 2 years ago by Tim Graham

Resolution: fixed
Status: newclosed

comment:12 Changed 4 months ago 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?

comment:13 Changed 4 months ago by Tim Graham

New ticket, please, as the fix for this one has been released for years.

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