[Django] #23682: Stronger redirect loop detection in the test client
Django
-----------------------------------+--------------------
Reporter: wrwrwr | Owner: nobody
Type: Uncategorized | Status: new
Component: Testing framework | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------
The test client has a mechanism to detect circular redirects. I'd like to
propose two changes to it;
* when a loop is detected, throw an exception rather than break silently;
* detect and complain about very (possibly infinitely) long redirect
chains with differing URLs (at least when they only differ by a view
argument value or query string).
[https://github.com/wrwrwr/django/compare/dropped/redirect-with-view-name-
loops A few tests] that are not part of the proposed changes, but
demonstrate how one could try to test for loops (currently 3 of these just
past, while a browser would run into a loop, and 2 actually loop
indefinitely).
--
Ticket URL: <https://code.djangoproject.com/ticket/23682>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: wrwrwr | Owner: nobody
Type: Uncategorized | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wrwrwr):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
Comment:
[https://github.com/wrwrwr/django/compare/ticket_23682 Proposed changes]:
* defined a new `CircularRedirectError`;
* changed the loop detection to throw the exception on repeated URLs and
chains longer than 20;
* added a "redirect to self" test with a changing query argument.
Could all the loop prevention tests maybe live in `test_client` rather
than `test_client_regress`?
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:1>
Django
Reporter: wrwrwr | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Uncategorized => New feature
* stage: Unreviewed => Accepted
Comment:
It seems reasonable to me as infinite redirects could indeed be something
a developer would want to be notified about. Skimming the original ticket
where this was added (#4476), I couldn't find anything about why the
decision was made.
Combining the `test_client` and `test_client_regress` tests would probably
be welcome, but making it a separate commit/ticket is best.
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:2>
Django
Reporter: wrwrwr | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
I reviewed and tested this patch. It looks good to me, although the import
statements should reflect Django's convention of being alphabetized.
Do you want to create a pull request from your branch? This has the nice
side-effect of triggering an automated test run and verifying the merge
status.
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:3>
Django
Reporter: wrwrwr | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by wrwrwr):
Thanks. Did some imports reordering (see also #23860) and created
[https://github.com/django/django/pull/3537 a pull request].
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:4>
Django
Reporter: wrwrwr | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by prestontimmons):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
Thanks for adding the pull request. I left a few comments on it.
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:5>
Django
Reporter: wrwrwr | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (by wrwrwr):
Thanks again; rebased, undid the unnecessary reordering. added docs on the
new exception and a point to the 1.8 release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:6>
Django
Reporter: wrwrwr | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wrwrwr):
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:7>
Django
Reporter: wrwrwr | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
This patch looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:8>
Django
Reporter: wrwrwr | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by prestontimmons):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:9>
Django
Reporter: wrwrwr | Owner: nobody
Type: New feature | Status: closed
Component: Testing framework | Version: master
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"056a3c6c374f15e23746ea8568cd5b11bfe7d442"]:
{{{
#!CommitTicketReference repository=""
revision="056a3c6c374f15e23746ea8568cd5b11bfe7d442"
Fixed #23682 -- Enhanced circular redirects detection in tests.
When the test client detects a redirect to a URL seen in the
currently followed chain it will now raise a RedirectCycleError
instead of just returning the first repeated response.
It will also complain when a single chain of redirects is longer
than 20, as this often means a redirect loop with varying URLs,
and even if it's not actually one, such long chains are likely
to be treated as loops by browsers.
Thanks Preston Timmons, Berker Peksag, and Tim Graham for reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23682#comment:10>