[Django] #24713: Redirect loop detection in test client is incorrect

31 views
Skip to first unread message

Django

unread,
Apr 27, 2015, 9:44:04 AM4/27/15
to django-...@googlegroups.com
#24713: Redirect loop detection in test client is incorrect
-------------------------+-------------------------------------------------
Reporter: | Owner: nobody
agmathew |
Type: Bug | Status: new
Component: Testing | Version: 1.8
framework |
Severity: Normal | Keywords: redirect loop detection test client
Triage Stage: | Has patch: 0
Unreviewed |
Easy pickings: 0 | UI/UX: 0
-------------------------+-------------------------------------------------
We recently upgraded our Django installation from 1.6 and 1.8 and noticed
our unit tests were failing in multiple places. We tracked down the
problem to changes stemming from the fix for #23682, which introduces
stronger looper detection for redirects. The problem is that we have logic
in our codebase that goes like this:

* Go to /view/
* If a dirty bit is set, redirect to /update/ and remove the dirty bit
* Redirect back to /view/

The loop detection erroneously considers this a redirect loop because
/view/ is visited twice. Browsers don't have a problem with our redirects.
I propose that the redirect loop detection should check that a previous
site has been visited N times (for some value of N) before reporting a
redirect loop. If this sounds reasonable, I can create a patch.

--
Ticket URL: <https://code.djangoproject.com/ticket/24713>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,
Apr 27, 2015, 10:21:29 AM4/27/15
to django-...@googlegroups.com
#24713: Redirect loop detection in test client is incorrect
-------------------------------------+-------------------------------------
Reporter: agmathew | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: redirect loop | Triage Stage: Accepted
detection test client |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):

* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* severity: Normal => Release blocker
* needs_tests: => 0
* needs_docs: => 0


Comment:

Yes, it does seem reasonable.

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

Django

unread,
May 1, 2015, 8:02:15 AM5/1/15
to django-...@googlegroups.com
#24713: Redirect loop detection in test client is incorrect
-------------------------------------+-------------------------------------
Reporter: agmathew | Owner:
| MounirMesselmeni
Type: Bug | Status: assigned

Component: Testing framework | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: redirect loop | Triage Stage: Accepted
detection test client |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by MounirMesselmeni):

* status: new => assigned
* owner: nobody => MounirMesselmeni


--
Ticket URL: <https://code.djangoproject.com/ticket/24713#comment:2>

Django

unread,
May 1, 2015, 8:13:52 AM5/1/15
to django-...@googlegroups.com
#24713: Redirect loop detection in test client is incorrect
-------------------------------------+-------------------------------------
Reporter: agmathew | Owner:
| MounirMesselmeni
Type: Bug | Status: assigned
Component: Testing framework | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: redirect loop | Triage Stage: Accepted
detection test client |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by MounirMesselmeni):

Please review my [https://github.com/django/django/pull/4591 PullRequest]
and check if we raise the RedirectCycleError when redirecting to the same
page more than 2 time is okay or we need to make it greater.

--
Ticket URL: <https://code.djangoproject.com/ticket/24713#comment:3>

Django

unread,
May 3, 2015, 11:33:58 AM5/3/15
to django-...@googlegroups.com
#24713: Redirect loop detection in test client is incorrect
-------------------------------------+-------------------------------------
Reporter: agmathew | Owner:
Type: Bug | Status: new

Component: Testing framework | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: redirect loop | Triage Stage: Accepted
detection test client |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by MounirMesselmeni):

* status: assigned => new
* owner: MounirMesselmeni =>


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

Django

unread,
May 4, 2015, 8:47:35 AM5/4/15
to django-...@googlegroups.com
#24713: Redirect loop detection in test client is incorrect
-------------------------------------+-------------------------------------
Reporter: agmathew | Owner:
Type: Bug | Status: new

Component: Testing framework | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: redirect loop | Triage Stage: Accepted
detection test client |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):

* has_patch: 0 => 1
* needs_tests: 0 => 1


--
Ticket URL: <https://code.djangoproject.com/ticket/24713#comment:5>

Django

unread,
May 13, 2015, 11:27:19 AM5/13/15
to django-...@googlegroups.com
#24713: Redirect loop detection in test client is incorrect
-------------------------------------+-------------------------------------
Reporter: agmathew | Owner:
Type: Bug | Status: new

Component: Testing framework | Version: 1.8
Severity: Release blocker | Resolution:
Keywords: redirect loop | Triage Stage: Accepted
detection test client |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by timgraham):

From Mounir on the pull request:

I don't think making multiple redirects for a client is the right way.
Let take an example: A user is trying to access a home page "/home" and
the server need to update an information, let's say a last_visit datetime
field or something else -- do we need to redirect him to "/update" make
this update and redirect him again to "/home"? I think it's better to run
a signal, an asynchronous task, make the update by calling a model method
or something else or even do the logic from the "/home" view. If we are
not sure that this is a common issue for many (now it look like only one
person have this problem) I think it's better to keep things like they
are.

I'd agree with this. Is there a reason you can't restructure your code to
avoid the redirect? See also the [https://groups.google.com/d/topic
/django-developers/8oXwD_KGjvc/discussion discussion on the django-
developers mailing list].

--
Ticket URL: <https://code.djangoproject.com/ticket/24713#comment:6>

Django

unread,
May 17, 2015, 8:18:27 PM5/17/15
to django-...@googlegroups.com
#24713: Redirect loop detection in test client is incorrect
-------------------------------------+-------------------------------------
Reporter: agmathew | Owner:
Type: Bug | Status: closed

Component: Testing framework | Version: 1.8
Severity: Release blocker | Resolution: wontfix

Keywords: redirect loop | Triage Stage: Accepted
detection test client |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):

* status: new => closed
* resolution: => wontfix


--
Ticket URL: <https://code.djangoproject.com/ticket/24713#comment:7>

Reply all
Reply to author
Forward
0 new messages