PREPEND_WWW and APPEND_SLASH settings don't work with flatpages middleware

[Jason Davies]

The PREPEND_WWW and APPEND_SLASH settings don't force a redirect when FlatpageFallbackMiddleware is being used.

This is because a redirect only happens if the target URL can be resolved using urlresolvers.resolve() in the code.

This behaviour has occurred since the change in [6852].

If the target URL cannot be resolved immediately using urlresolvers.resolve(), perhaps we should check in process_response() to make absolutely sure the response status code will be 404? If some intermediate middleware, e.g. FlatpageFallbackMiddleware has returned a successful response code, we can then issue a redirect as per the old behaviour.

[Chris Beaven]

#6167 handles APPEND_SLASH (someone didn't search before adding a ticket :P). Rewording summary to just cover PREPEND_WWW

[Jason Davies]

Ah, I didn't see #6167, but I don't think it's exactly the same: this ticket concerns the use of FlatpageFallbackMiddleware whereas #6167 seems to concern the use of the flatpage view (at least from looking at the patch).

It's actually a more general problem, in that if a middleware returns a successful response code (even when no view is mapped to the current URL), then ideally we should honour the APPEND_SLASH and PREPEND_WWW settings and issue a redirect.

[Chris Beaven]

Correct, they are different issues (hence why I didn't close).

To make APPEND_SLASH always append a slash (on a successful response code) goes against the new behaviour. It should only try the slash if it 404s.
Sure, fix PREPEND_WWW like that though (even though I think it's a stupid setting that should belong to a web server, not the Django framework).

[Jason Davies]

Ah yes, what I should have said is that in CommonMiddleware's process_response() we should append a slash if it 404s and APPEND_SLASH is set.

The problem is that if you use FlatpageFallbackMiddleware, redirects do not happen even if they did before the change in [6852], as the change relies on being able to resolve the URL using the URLconf.

For example, if the URL "/foo" isn't found by FlatpageFallbackMiddleware, we should still redirect if APPEND_SLASH is set.

[Jason Davies]

Patch for CommonMiddleware to fix handling of APPEND_SLASH and PREPEND_WWW

[Jason Davies]

Changed the summary back since this ticket still covers the APPEND_SLASH problem.

[Jason Davies]

This patch also fixes #6167.

[Chris Beaven]

From a brief overview, I'm not 100% sure this fixes #6167 (it will always resolve the non-slash if you're using flatpages as the fallback in urlconf rather than using the middleware).

Go ahead and steal the tests in the other ticket, make sure this ticket fixes every case and then close #6167 as superceeded by this one. (You'll probably need some more tests for the things you're fixing)

[Jason Davies]

Updated patch with unit tests.

[Jason Davies]

Updated patch with slightly improved unit tests (more descriptive docstring comments).

[Jason Davies]

The unit tests catch the problem reported by this ticket, as well as checking some cases in #6167, which this ticket supercedes.

[Luke Plant]

Jason: In your patch, file django/middleware/common.py, line 108,

if resolve and new_url[0] == new_url[0] and ...

?

[Jacob]

(In [8218]) Fixed #6213: flatpage view now correctly redirects if settings.APPEND_SLASH. Thanks, crankycoder.

[Jason Davies]

Replying to lukeplant:

Jason: In your patch, file django/middleware/common.py, line 108,

if resolve and new_url[0] == new_url[0] and ...

Oops, that should be:

if resolve and new_url[0] == old_url[0] and ...

[Jason Davies]

This issue has still not been fixed: if settings.PREPEND_WWW is True and I have a flatpage mapped to '/', ​http://jasondavies.com/ doesn't redirect to ​http://www.jasondavies.com/

This is because in the code at the moment, a redirect only happens if the target URL can be resolved using urlresolvers.resolve() , which obviously doesn't work when FlatpageFallbackMiddleware is used. My patch fixes this problem by checking for a successful response code from any preceding middleware, and issuing a redirect if necessary as per the old behaviour.

[Jason Davies]

Updated patch

[jonas]

The current solution also leads to an infinite "302 Found" redirect if APPEND_SLASH is True and a flatpage exists at "/".

Should I file a new bug for this?

[che@…]

Replying to jacob:

(In [8218]) Fixed #6213: flatpage view now correctly redirects if settings.APPEND_SLASH. Thanks, crankycoder.

Changeset [8218] also causes flatpages to trigger redirect on all URLs that don't end with a slash and return 404 response, regardless whether any page exists.

For example, if you have flatpage '/about/', flagpages are going to redirect '/about' -> '/about/', but also '/robots.txt' to '/robots.txt/' (if you don't have the file handled in urlconf), etc.

[Clay McClure]

I echo the concern noted above, that the patch in [8218] blindly redirects without testing to see whether the target of the redirect is valid (i.e., matches a URLconf or a FlatPage). At best, this adds extra request-response latency to 404 handling. At worst, it obscures the nature of the 404. Clients requesting /robots.txt should receive a 404 if that resource does not exist, and should not be redirected to a different resource (/robots.txt/).

For me, this problem manifests as a failing test case. I expect my REST API (which uses non-slash-terminated URLs) to return a 404 under certain conditions, but was surprised to find it returning a 302 -- due to FlatpageFallbackMiddleware blindly redirecting clients to a hypothetical slash-terminated resource instead of passing the 404 through to the client.

In short, FlatpageFallbackMiddleware should honor the APPEND_SLASH logic described in #3228, with one addition [shown in CAPS]:

"If APPEND_SLASH is set and the initial URL doesn't end with a slash, and it is not found in urlpatterns, a new URL is formed by appending a slash at the end. If this new URL is found in urlpatterns OR THE FLATPAGE TABLE, then an HTTP-redirect is returned to this new URL; otherwise the initial URL is processed as usual."

[sjl]

I've sent a pull request to fix the issue clay mentioned (and add some tests): ​https://github.com/django/django/pull/14

[sjl]

I've merged that pull request with upstream so it's up to date. Anyone want to take a look at it?

[Jannis Leidel]

In [16048]:

Fixed #6213 -- Updated the flatpages app to only append a slash if the flatpage actually exist.

The FlatpageFallbackMiddleware (and the view) now only add a trailing slash and redirect if the resulting URL refers to an existing flatpage. Previously requesting /notaflatpageoravalidurl would redirect to /notaflatpageoravalidurl/, which would then raise a 404. Requesting /notaflatpageoravalidurl now will immediately raise a 404. Also, Redirects returned by flatpages are now permanent (301 status code) to match the behaviour of the CommonMiddleware.

Thanks to Steve Losh for the initial work on the patch.