Opened 12 years ago
Closed 10 years ago
#18314 closed Bug (fixed)
request.build_absolute_uri() functions incorrectly when the path starts with //
Reported by: | anonymous | Owned by: | Unai Zalakain |
---|---|---|---|
Component: | HTTP handling | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Abhaya Agarwal | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
A request to:
leads to request.build_absolute_uri() returning:
Attachments (2)
Change History (21)
comment:1 by , 12 years ago
Needs tests: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:2 by , 12 years ago
Owner: | changed from | to
---|
comment:3 by , 12 years ago
Has patch: | set |
---|---|
Resolution: | → fixed |
Status: | new → closed |
comment:4 by , 12 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Pardon my resolving of the ticket.
comment:5 by , 12 years ago
Needs tests: | unset |
---|
follow-up: 8 comment:6 by , 12 years ago
Cc: | added |
---|---|
Patch needs improvement: | set |
Since the original function as well as your patch is using self.path
to construct the base_uri
, query parameters are not preserved anyway. So branching doesn't serve any purpose.
One simpler way is to strip out the additional slashes in the beginning of self.path
in this function. It may be better to do that where the request is initialized.
comment:7 by , 12 years ago
Also str.format was introduced in Python 2.6 while Django 1.4 supports Python 2.5 also. So that should be reverted back to old style string interpolation.
comment:8 by , 12 years ago
Replying to abhaga:
Since the original function as well as your patch is using
self.path
to construct thebase_uri
, query parameters are not preserved anyway. So branching doesn't serve any purpose.
One simpler way is to strip out the additional slashes in the beginning ofself.path
in this function. It may be better to do that where the request is initialized.
That is incorrect. location
, if provided or not is joined with the base_uri
, and when not provided is built using request.get_full_path(). This happens in both my patch and the original.
>>> from urlparse import urljoin >>> urljoin('http://www.example.com/foo/', 'http://www.example.com/foo/?bar=baz') 'http://www.example.com/foo/?bar=baz'
Regarding the usage of basestring.format()
, I'll provide an updated patch which uses string interpolation.
by , 12 years ago
Attachment: | TICKET-18314.diff added |
---|
Patch to fix bug, updated to remove basestring.format() usage
by , 12 years ago
Attachment: | 18314.diff added |
---|
follow-up: 10 comment:9 by , 12 years ago
Patch needs improvement: | unset |
---|
Here's a simpler way to do it.
comment:10 by , 12 years ago
Replying to SmileyChris:
Here's a simpler way to do it.
I don't mean to sound too competitive, but I think my patch is as simple as possible, but no simpler :P It only has 2 very simple logical branches, and requires no hacking. I think prepending '//'
to self.get_full_path()
is a bit hacky, as it simply takes advantage urlsplit
setting netloc
to ''
when a URL starts with //
(explicit over implicit comes to mind here).
Also worth noting is:
if not location:
Not using is not None
here could lead to an unintended result, if ever a bug were to cause this method to be called with an empty string provided as the location
argument.
comment:11 by , 12 years ago
Any word on which patch will go to master? Both include passing tests for regression, etc., and both provide the exact same result.
comment:12 by , 12 years ago
Status: | reopened → new |
---|
comment:13 by , 11 years ago
Patch needs improvement: | set |
---|
If I am not mistaken the patch requires updating. The test files being patched no longer exist at that location.
comment:14 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:15 by , 11 years ago
The 1st test fails.But the other 3 pass. Can someone also test this patch? Hmm. Well, I'm going to have to fix up the patch.
comment:17 by , 11 years ago
Easy pickings: | unset |
---|
comment:18 by , 11 years ago
Owner: | changed from | to
---|---|
Patch needs improvement: | unset |
Version: | 1.4 → master |
Enhanced docs, all test passing plus correct handling of leading slashes by WSGIRequest
.
Ups, I forgot to link to the PR: https://github.com/django/django/pull/1866
comment:19 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
I've attached the fix, but if it needs improvements, let me know and I'll update the diff ASAP.