[Django] #23531: APPEND_SLASHES behavior shouldn't redirect with a 301
Django
------------------------------+------------------------------------------
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
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
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
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 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
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 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
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 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
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 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
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 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
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 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
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 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
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 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
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Keywords: common, middleware, | Triage Stage:
redirect | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 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
Reporter: mattrobenolt | Owner: nobody
Type: Bug | Status: new
Component: Core (URLs) | Version: master
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
class
-------------------------------------+-------------------------------------
Reporter: mattrobenolt | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Keywords: common, middleware, | Triage Stage: Accepted
redirect | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* type: Bug => New feature
--
Ticket URL: <https://code.djangoproject.com/ticket/23531#comment:12>
Django
class
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Keywords: common, middleware, | Triage Stage: Accepted
redirect | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
https://github.com/django/django/pull/3466
--
Ticket URL: <https://code.djangoproject.com/ticket/23531#comment:13>
Django
class
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Core (URLs) | Version: master
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 |
-------------------------------------+-------------------------------------
* 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>