Opened 10 years ago

Closed 10 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 by Daniele Procida, 10 years ago

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

comment:2 by Daniele Procida, 10 years ago

Description: modified (diff)

comment:3 by Tim Graham, 10 years ago

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 by Matt Robenolt, 10 years ago

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 by Matt Robenolt, 10 years ago

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 by Tim Graham, 10 years ago

An example of making it configurable is LocaleMiddleware.response_redirect_class.

comment:7 by Matt Robenolt, 10 years ago

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 by Matt Robenolt, 10 years ago

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

comment:9 by Matt Robenolt, 10 years ago

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 by Tim Graham, 10 years ago

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 10 years ago by Russell Keith-Magee (previous) (diff)

comment:11 by Aymeric Augustin, 10 years ago

Triage Stage: UnreviewedAccepted

comment:12 by Tim Graham, 10 years ago

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 by Berker Peksag, 10 years ago

Patch needs improvement: unset

comment:14 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In df0523debcc2d0984f1bc11d323f04227d4b388b:

Fixed #23531 -- Added CommonMiddleware.response_redirect_class.

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