[Django] #27398: AssertRedirects expects ordered query parameters
Django
-----------------------------------+--------------------
Reporter: Cédric Codet | Owner: nobody
Type: Uncategorized | Status: new
Component: Testing framework | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------
When checking for a url redirection in tests with `assertRedirects`, the
order of redirection url query parameters matters (since it is matched
with `expected_url` string argument).
Example: if the expected url is `example.com/?foo=1&bar=2`,
`example.com/?bar=2&foo=1` is not matched (while
`example.com/?foo=1&bar=2` is)
This is an issue since Django does not enforce query parameters ordering
itself
--
Ticket URL: <https://code.djangoproject.com/ticket/27398>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
query parameters
--------------------------------------+------------------------------------
Reporter: Cédric Codet | Owner: nobody
Component: Testing framework | Version: 1.10
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 Tim Graham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
Seems useful as Django's test suite has
[https://github.com/django/django/blob/80e742d991b276023a3aa51a29ed757879051282/tests/auth_tests/test_views.py#L74-L89
a workaround] for this issue. I'm not sure if we need a way to toggle the
behavior -- always ignoring ordering seems fine unless anyone can think of
a use case where it would be problematic?
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:1>
Django
query parameters
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by favll):
* owner: nobody => favll
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:2>
Django
query parameters
--------------------------------------+------------------------------------
Reporter: Cédric Codet | Owner: favll
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jan Pieter Waagmeester):
* Attachment "assertRedirects.patch" added.
Patch adding normalization to the url/expected_url prior to
self.assertEqual
Django
query parameters
--------------------------------------+------------------------------------
Reporter: Cédric Codet | Owner: favll
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jan Pieter Waagmeester):
Just attached a patch which fixes this issue, if the general direction of
this fix is considered the way to go, I am willing to open a pull request
on Github.
Do we need the strict_query_order param to provide a way to fall back to
current behavior?
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:3>
Django
query parameters
--------------------------------------+------------------------------------
Reporter: Cédric Codet | Owner: favll
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
Please make the PR.
I wouldn't keep the `strict_query_order` param, unless we can find a
reference stating that param ordering in query string is significant and
kept.
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:4>
Django
query parameters
Reporter: Cédric Codet | Owner: Jan
Type: | Pieter Waagmeester
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jan Pieter Waagmeester):
* owner: favll => Jan Pieter Waagmeester
* has_patch: 0 => 1
Comment:
Pull request: https://github.com/django/django/pull/9480
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:5>
Django
query parameters
-------------------------------------+-------------------------------------
Reporter: Cédric Codet | Owner: Jan
Type: | Pieter Waagmeester
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
I'm not sure if this is relevant here but Django makes a distinction
between `?foo=1&foo=2` and `?foo=2&foo=1` so we might have to make the
`strict_query_order` parameter default to `True`?
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:6>
Django
query parameters
-------------------------------------+-------------------------------------
Reporter: Cédric Codet | Owner: Jan
Type: | Pieter Waagmeester
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jan Pieter Waagmeester):
I just pushed a commit allowing multiple values for a key in the
`redirect_view` test view. Indeed this allows reordering `foo=1&foo=2` to
`foo=2&foo=1` in `assertRedirects` without failures.
There are currently no tests to test that.
There is also
[https://github.com/django/django/blob/80e742d991b276023a3aa51a29ed757879051282/tests/auth_tests/test_views.py#L74-L89
AuthViewTestCase.assertURLEqual()] which compares two `QueryDict` objects,
which ensures order for multiple values is checked.
We can move the `assertUrlRedirects` method to `SimpleTestCase` and use
that method (or a similar implementation) to do the final url equality
assertion in `SimpleTestCase.assertRedirects()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:7>
Django
query parameters
-------------------------------------+-------------------------------------
Reporter: Cédric Codet | Owner: Jan
Type: | Pieter Waagmeester
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
Hi Jan,
Thanks for your effort here!
I think the points you make in your last comment are all correct.
The first step is adding the test cases to cover the `foo=1&foo=2` vs
`foo=2&foo=1` example. These are indeed different URLs.
You have `foo=1&bar=1` vs `bar=1&foo=1` already. Are there other examples
we need to cover?
Excepting the multi-value case, I don't see order being important.
(Anyone?)
I think your proposal to move and reuse `assertURLEqual()` method seems
very sensible. (Going via `QueryDict` should be more reliable that trying
to normalise the URL by-hand.)
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:8>
Django
query parameters
-------------------------------------+-------------------------------------
Reporter: Cédric Codet | Owner: Jan
Type: | Pieter Waagmeester
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Jan Pieter Waagmeester):
I just pushed a commit splitting of `assertURLRedirects()`, and included
some extra tests.
I tried using `QueryDict`, but they preserve their creation order when
`.urlencode()` is called and thus do not allow re-creating the url. I like
to do that because it simplifies the output, but if you still think the
way it was done in the implementation I removed, I'll update my
implementation to look more like that one.
Do we need to add tests checking the message produced if the assertion
fails?
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:9>
Django
query parameters
-------------------------------------+-------------------------------------
Reporter: Cédric Codet | Owner: Jan
Type: | Pieter Waagmeester
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jan Pieter Waagmeester):
* needs_better_patch: 1 => 0
* version: 1.10 => master
Comment:
I just pushed a new commit:
- documentation for the new assertion to `docs/topics/testing/tools.txt`
- Wrapped some docsgtrings to 79 chars.
- Added a changelog entry
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:10>
Django
query parameters
-------------------------------------+-------------------------------------
Reporter: Cédric Codet | Owner: Jan
Type: | Pieter Waagmeester
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:11>
Django
query parameters
-------------------------------------+-------------------------------------
Reporter: Cédric Codet | Owner: Jan
Type: | Pieter Waagmeester
Component: Testing framework | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"24959e48d949a20be969f649ece3576dbc7ce422" 24959e48]:
{{{
#!CommitTicketReference repository=""
revision="24959e48d949a20be969f649ece3576dbc7ce422"
Fixed #27398 -- Added an assertion to compare URLs, ignoring the order of
their query strings.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:12>
Django
query parameters
-------------------------------------+-------------------------------------
Reporter: Cédric Codet | Owner: Jan
Type: | Pieter Waagmeester
Cleanup/optimization | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"5d98d53fab874ec21b313499a7e6aa05791bde85" 5d98d53]:
{{{
#!CommitTicketReference repository=""
revision="5d98d53fab874ec21b313499a7e6aa05791bde85"
Refs #27398 -- Simplified some tests with assertRedirects().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27398#comment:13>