[Django] #23531: APPEND_SLASHES behavior shouldn't redirect with a 301

28 views
Skip to first unread message

Django

unread,
Sep 21, 2014, 6:13:17 PM9/21/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
------------------------------+------------------------------------------
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Keywords: common, middleware, redirect
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
------------------------------+------------------------------------------
Redirecting with a permanent redirect (HttpResponsePermanentRedirect) can
lead to 404s and other unexpected behavior when things move around.

For example:

In settings.py, `APPEND_SLAHSES = True` (the default behavior)

In urls.py `url(r'foo/$', ..._`

A user visits `example.com/foo` and gets redirected to `example.com/foo/`
as expected.

Later, we decide that we don't like slashes in our urls, and change to
`APPEND_SLASHES = False` and change our url route to `url(r'foo$', ...`

A user visits `example.com/foo` again (which is now the correct url) and
their browser redirects them to `example.com/foo/` which is now a 404.

This behavior happens specifically because Django has told it that this
redirect is 100% going to happen, so the browser caches it and doesn't ask
the server again. In my opinion, it's a bad idea that Django makes this
assumption because it has no insight into what future plans are and
whatnot.

This behavior has also existed since at least 2007, so I'm not sure how
much effort needs to go into changing this moving forward for 1.8+.

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

Django

unread,
Sep 21, 2014, 6:17:19 PM9/21/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
-------------------------------------+-------------------------------------

Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

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

* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0


Comment:

I can confirm the behaviour, though I'm not sure what the correct
behaviour should be.

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

Django

unread,
Sep 21, 2014, 6:17:55 PM9/21/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
-------------------------------------+-------------------------------------

Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by EvilDMP:

Old description:

> Redirecting with a permanent redirect (HttpResponsePermanentRedirect) can
> lead to 404s and other unexpected behavior when things move around.
>
> For example:
>
> In settings.py, `APPEND_SLAHSES = True` (the default behavior)
>
> In urls.py `url(r'foo/$', ..._`
>
> A user visits `example.com/foo` and gets redirected to `example.com/foo/`
> as expected.
>
> Later, we decide that we don't like slashes in our urls, and change to
> `APPEND_SLASHES = False` and change our url route to `url(r'foo$', ...`
>
> A user visits `example.com/foo` again (which is now the correct url) and
> their browser redirects them to `example.com/foo/` which is now a 404.
>
> This behavior happens specifically because Django has told it that this
> redirect is 100% going to happen, so the browser caches it and doesn't
> ask the server again. In my opinion, it's a bad idea that Django makes
> this assumption because it has no insight into what future plans are and
> whatnot.
>
> This behavior has also existed since at least 2007, so I'm not sure how
> much effort needs to go into changing this moving forward for 1.8+.

New description:

Redirecting with a permanent redirect (HttpResponsePermanentRedirect) can
lead to 404s and other unexpected behavior when things move around.

For example:

In settings.py, `APPEND_SLASHES = True` (the default behavior)

In urls.py `url(r'foo/$', ..._`

A user visits `example.com/foo` and gets redirected to `example.com/foo/`
as expected.

Later, we decide that we don't like slashes in our urls, and change to
`APPEND_SLASHES = False` and change our url route to `url(r'foo$', ...`

A user visits `example.com/foo` again (which is now the correct url) and
their browser redirects them to `example.com/foo/` which is now a 404.

This behavior happens specifically because Django has told it that this
redirect is 100% going to happen, so the browser caches it and doesn't ask
the server again. In my opinion, it's a bad idea that Django makes this
assumption because it has no insight into what future plans are and
whatnot.

This behavior has also existed since at least 2007, so I'm not sure how
much effort needs to go into changing this moving forward for 1.8+.

--

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

Django

unread,
Sep 21, 2014, 6:18:59 PM9/21/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
-------------------------------------+-------------------------------------

Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by timgraham):

See #21587 which suggests to default `RedirectView` to `permanent=False`.
There are backwards compatibility concerns so at least a deprecation would
be needed. Making the redirect class a class attribute so it's more easily
customizable would be a good start.

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

Django

unread,
Sep 21, 2014, 6:23:22 PM9/21/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
-------------------------------------+-------------------------------------

Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by mattrobenolt):

How do you see this happening inside `CommonMiddleware` since it doesn't
utilize `RedirectView` or anything user configurable at the moment? We can
have two different middlewares, but that's really gross imo. This is just
some magical behavior that users have little insight into.

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

Django

unread,
Sep 21, 2014, 6:25:15 PM9/21/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
-------------------------------------+-------------------------------------

Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by mattrobenolt):

I can see introducing a new setting to settings.py.

`APPEND_SLASHES_REDIRECT_PERMANENT = True` which defaults to true, and we
switch to False in 1.9 or whatever makes sense in the deprecation cycle.

Thoughts on that?

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

Django

unread,
Sep 21, 2014, 6:26:42 PM9/21/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
-------------------------------------+-------------------------------------

Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by timgraham):

An example of making it configurable is
`LocaleMiddleware.response_redirect_class`.

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

Django

unread,
Sep 21, 2014, 6:30:13 PM9/21/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
-------------------------------------+-------------------------------------

Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by mattrobenolt):

Ahh, ok, I like that.

Is it then suggested to subclass the middleware and override?

While on this topic... should the `APPEND_SLASHES` behavior be considered
to be stripped out into it's own middleware? Or is there value in having
it crammed inside `CommonMiddleware`?

I ask because if we introduce `CommonMiddleware.response_redirect_class`,
it feels ambiguous. Though there's only one redirect path in
`CommonMiddleware`, it just doesn't feel as concrete as
`LocaleMiddleware`.

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

Django

unread,
Sep 21, 2014, 6:31:03 PM9/21/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
-------------------------------------+-------------------------------------

Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by mattrobenolt):

Oh, I guess this would affect the `PREPEND_WWW` setting as well.

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

Django

unread,
Sep 21, 2014, 6:41:20 PM9/21/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
-------------------------------------+-------------------------------------

Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by mattrobenolt):

I updated the patch to introduce
`CommonMiddleware.response_redirect_class` so CommonMiddleware can be
subclassed with the default being `HttpResponsePermanentRedirect` still
for backwards compatibility. I'm ok with this solution and going through
deprecation process to change default to `HttpResponseRedirect`.

--
Ticket URL: <https://code.djangoproject.com/ticket/23531#comment:9>

Django

unread,
Sep 23, 2014, 9:36:57 AM9/23/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
-------------------------------------+-------------------------------------

Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by timgraham):

I am not convinced about changing the default redirect as I expect the
majority of users with `APPEND_SLASHES` and `PREPEND_WWW` would want the
SEO benefits of a 301 redirect and are not going to change their URLs
after their site is deployed. This likely needs a wider discussion on
django-developers.

In the meantime, I think we can move forward with adding
`CommonMiddleware.response_redirect_class` as a separate ticket.

--
Ticket URL: <https://code.djangoproject.com/ticket/23531#comment:10>

Django

unread,
Sep 28, 2014, 4:57:02 AM9/28/14
to django-...@googlegroups.com
#23531: APPEND_SLASHES behavior shouldn't redirect with a 301
-------------------------------------+-------------------------------------

Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage: Accepted
redirect | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):

* stage: Unreviewed => Accepted


--
Ticket URL: <https://code.djangoproject.com/ticket/23531#comment:11>

Django

unread,
Oct 2, 2014, 9:17:08 AM10/2/14
to django-...@googlegroups.com
#23531: Add CommonMiddleware.response_redirect_class to customize the redirect
class
-------------------------------------+-------------------------------------
Reporter: mattrobenolt | Owner: nobody
Type: New feature | Status: new

Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage: Accepted
redirect | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1

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

* needs_better_patch: 0 => 1
* type: Bug => New feature


--
Ticket URL: <https://code.djangoproject.com/ticket/23531#comment:12>

Django

unread,
Nov 4, 2014, 3:21:06 PM11/4/14
to django-...@googlegroups.com
#23531: Add CommonMiddleware.response_redirect_class to customize the redirect
class
-------------------------------------+-------------------------------------
Reporter: mattrobenolt | Owner: nobody
Type: New feature | Status: new

Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: common, middleware, | Triage Stage: Accepted
redirect | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0

Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by berkerpeksag):

* needs_better_patch: 1 => 0


Comment:

https://github.com/django/django/pull/3466

--
Ticket URL: <https://code.djangoproject.com/ticket/23531#comment:13>

Django

unread,
Nov 4, 2014, 5:57:11 PM11/4/14
to django-...@googlegroups.com
#23531: Add CommonMiddleware.response_redirect_class to customize the redirect
class
-------------------------------------+-------------------------------------
Reporter: mattrobenolt | Owner: nobody
Type: New feature | Status: closed

Component: Core (URLs) | Version: master
Severity: Normal | Resolution: fixed

Keywords: common, middleware, | Triage Stage: Accepted
redirect | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by Tim Graham <timograham@…>):

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


Comment:

In [changeset:"df0523debcc2d0984f1bc11d323f04227d4b388b"]:
{{{
#!CommitTicketReference repository=""
revision="df0523debcc2d0984f1bc11d323f04227d4b388b"
Fixed #23531 -- Added CommonMiddleware.response_redirect_class.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/23531#comment:14>

Reply all
Reply to author
Forward
0 new messages