Opened 3 months ago
Last modified 2 days ago
#36488 assigned Bug
RedirectView(query_string=True) produces double '?' when both destination URL and request have query parameters
Reported by: | Ethan Jucovy | Owned by: | Samriddha Kumar Tripathi |
---|---|---|---|
Component: | Generic views | Version: | 5.2 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
When using the generic RedirectView(query_string=True)
with a query string in the destination URL, the view always appends query string parameters from the incoming request with a ?
separator. It should use &
if there's also an existing query string in the incoming URL.
Currently, requests that come in with a query string end up redirected to a URL containing two question marks, e.g.:
path("redirect-me/", RedirectView.as_view(url="/destination/?pork=spam", query_string=True))
GET /redirect-me/?utm_source=foo
Currently results in:
Location: /destination/?pork=spam?utm_source=foo
Expected:
Location: /destination/?pork=spam&utm_source=foo
Change History (8)
comment:1 by , 3 months ago
Has patch: | set |
---|
comment:2 by , 3 months ago
comment:3 by , 3 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
Triage Stage: | Unreviewed → Accepted |
Hello Ethan Jucovy, thank you for taking the time to create this ticket and the PR. I think your report makes sense, I would also expect ?
and &
to be properly handled. Having said that, the current url manipulation logic feels somehow brittle and I wonder if we should consider a more robust implementation using urlparse. Have your given this some thought?
comment:4 by , 3 months ago
Patch needs improvement: | set |
---|
comment:6 by , 2 weeks ago
Hi everyone, I see this ticket is still open and marked 'easy pickings'. Following Natalia's suggestion, I'd like to work on a new patch that uses urllib.parse for a more robust solution. Please let me know if it's okay for me to take this on
comment:7 by , 13 days ago
Proposed fix in PR: https://github.com/django/django/pull/19833
comment:8 by , 13 days ago
Owner: | changed from | to
---|---|
Patch needs improvement: | unset |
comment:9 by , 2 days ago
Triage Stage: | Accepted → Ready for checkin |
---|
PR: https://github.com/django/django/pull/19610