Opened 8 years ago

Closed 8 years ago

#23531 closed New feature (fixed)

Add CommonMiddleware.response_redirect_class to customize the redirect class

Reported by: Matt Robenolt Owned by: nobody
Component: Core (URLs) Version: dev
Severity: Normal Keywords: common, middleware, redirect
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Daniele Procida)

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+.

Change History (14)

comment:1 Changed 8 years ago by Daniele Procida

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

comment:2 Changed 8 years ago by Daniele Procida

Description: modified (diff)

comment:3 Changed 8 years ago by Tim Graham

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.

comment:4 Changed 8 years ago by Matt Robenolt

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.

comment:5 Changed 8 years ago by Matt Robenolt

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?

comment:6 Changed 8 years ago by Tim Graham

An example of making it configurable is LocaleMiddleware.response_redirect_class.

comment:7 Changed 8 years ago by Matt Robenolt

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.

comment:8 Changed 8 years ago by Matt Robenolt

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

comment:9 Changed 8 years ago by Matt Robenolt

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.

comment:10 Changed 8 years ago by Tim Graham

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.

Last edited 8 years ago by Russell Keith-Magee (previous) (diff)

comment:11 Changed 8 years ago by Aymeric Augustin

Triage Stage: UnreviewedAccepted

comment:12 Changed 8 years ago by Tim Graham

Patch needs improvement: set
Summary: APPEND_SLASHES behavior shouldn't redirect with a 301Add CommonMiddleware.response_redirect_class to customize the redirect class
Type: BugNew feature

comment:13 Changed 8 years ago by Berker Peksag

Patch needs improvement: unset

comment:14 Changed 8 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In df0523debcc2d0984f1bc11d323f04227d4b388b:

Fixed #23531 -- Added CommonMiddleware.response_redirect_class.

Note: See TracTickets for help on using tickets.
Back to Top