[Django] #36488: RedirectView(query_string=True) produces double '?' when both destination URL and request have query parameters

[Django]

#36488: RedirectView(query_string=True) produces double '?' when both destination
URL and request have query parameters
------------------------------+-----------------------------------------
Reporter: Ethan Jucovy | Type: Bug
Status: new | Component: Generic views
Version: 5.2 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+-----------------------------------------
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36488>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36488: RedirectView(query_string=True) produces double '?' when both destination
URL and request have query parameters

-------------------------------+--------------------------------------
Reporter: Ethan Jucovy | Owner: (none)

Type: Bug | Status: new
Component: Generic views | Version: 5.2

Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0

-------------------------------+--------------------------------------
Changes (by Ethan Jucovy):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36488#comment:1>

[Django]

#36488: RedirectView(query_string=True) produces double '?' when both destination
URL and request have query parameters
-------------------------------+--------------------------------------
Reporter: Ethan Jucovy | Owner: (none)
Type: Bug | Status: new
Component: Generic views | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by Ethan Jucovy):

PR: https://github.com/django/django/pull/19610
--
Ticket URL: <https://code.djangoproject.com/ticket/36488#comment:2>

[Django]

#36488: RedirectView(query_string=True) produces double '?' when both destination
URL and request have query parameters

-------------------------------+----------------------------------------
Reporter: Ethan Jucovy | Owner: Ethan Jucovy
Type: Bug | Status: assigned

Component: Generic views | Version: 5.2
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0

-------------------------------+----------------------------------------
Changes (by Natalia Bidart):

* owner: (none) => Ethan Jucovy
* stage: Unreviewed => Accepted
* status: new => assigned

Comment:

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?
--
Ticket URL: <https://code.djangoproject.com/ticket/36488#comment:3>

[Django]

#36488: RedirectView(query_string=True) produces double '?' when both destination
URL and request have query parameters
-------------------------------+----------------------------------------
Reporter: Ethan Jucovy | Owner: Ethan Jucovy
Type: Bug | Status: assigned
Component: Generic views | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 1 | UI/UX: 0
-------------------------------+----------------------------------------
Changes (by Natalia Bidart):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36488#comment:4>

[Django]

#36488: RedirectView(query_string=True) produces double '?' when both destination
URL and request have query parameters
-------------------------------+----------------------------------------
Reporter: Ethan Jucovy | Owner: Ethan Jucovy
Type: Bug | Status: assigned
Component: Generic views | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------+----------------------------------------

Comment (by umesh singh bisht):

Hi @EthanJucovy!
I'd love to take on this ticket as my first contribution to Django.
I've analyzed the problem in the get_redirect_url method and have a clean,
backward-compatible solution ready. The fix should be just changing the
hardcoded '?' separator to use '&' when the destination URL already
contains query parameters. Whats your thought about this.
--
Ticket URL: <https://code.djangoproject.com/ticket/36488#comment:5>

[Django]

#36488: RedirectView(query_string=True) produces double '?' when both destination
URL and request have query parameters
-------------------------------+----------------------------------------
Reporter: Ethan Jucovy | Owner: Ethan Jucovy
Type: Bug | Status: assigned
Component: Generic views | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------+----------------------------------------

Comment (by Samriddha Kumar Tripathi):

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
--
Ticket URL: <https://code.djangoproject.com/ticket/36488#comment:6>

[Django]

#36488: RedirectView(query_string=True) produces double '?' when both destination
URL and request have query parameters
-------------------------------+----------------------------------------
Reporter: Ethan Jucovy | Owner: Ethan Jucovy
Type: Bug | Status: assigned
Component: Generic views | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------+----------------------------------------
Comment (by Samriddha Kumar Tripathi):

Proposed fix in PR: https://github.com/django/django/pull/19833
--
Ticket URL: <https://code.djangoproject.com/ticket/36488#comment:7>

[Django]

#36488: RedirectView(query_string=True) produces double '?' when both destination
URL and request have query parameters

-------------------------------------+-------------------------------------
Reporter: Ethan Jucovy | Owner: Samriddha
| Kumar Tripathi

Type: Bug | Status: assigned
Component: Generic views | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 1 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* needs_better_patch: 1 => 0
* owner: Ethan Jucovy => Samriddha Kumar Tripathi

--
Ticket URL: <https://code.djangoproject.com/ticket/36488#comment:8>