Redirects don't honor APPEND_SLASH

[Aymeric Augustin]

seanbrant reports on IRC that the following command no longer works.

pip install https://www.djangoproject.com/download/1.5c1/tarball/

/download/1.5c1/tarball/ is a URL in the redirect table. But pip strips the trailing slash, and despite APPEND_SLASH = True, Django doesn't restore it.

It could be a side effect of the upgrade of djangoproject.com to 1.4 this morning but I haven't found a corresponding change.

---

I don't understand how ​RedirectFallbackMiddleware deals with APPEND_SLASH. APPEND_SLASH tests if the resulting URL exists, and if not, it doesn't add a slash. Since RedirectFallbackMiddleware only handles 404, it can never operate on an URL processed by APPEND_SLASH.

Therefore, I believe the fallback lookup should be:

Redirect.objects.get(site__id__exact=current_site.id, old_path=path + '/')

rather than:

Redirect.objects.get(site__id__exact=current_site.id, old_path__exact=path[:path.rfind('/')]+path[path.rfind('/')+1:])

(which doesn't make any sense to me, actually).

[Aymeric Augustin]

I've provisionally applied this change on the server.

[Aymeric Augustin]

I just understood the current construct -- it's probably designed to handle querystrings.

[Carl Meyer]

Yes, that's what I was about to say.

But I still agree with you that it seems like RedirectFallbackMiddleware ought to be trying with an added slash, not a removed one, in this case, since APPEND_SLASH would never actually have appended a slash in this situation. IOW RedirectFallbackMiddleware needs to have its own implementation of APPEND_SLASH, not reverse its effect like the current code seems to be trying to do.

Unfortunately adding a slash while retaining querystring is a bit uglier than removing, but doable.

I have no idea how this would have broken with the recent dp.com changes.

[Aymeric Augustin]

I'm selfishly going to backport this to 1.5 because it breaks djangoproject.com...

[Aymeric Augustin <aymeric.augustin@…>]

In 833ccd4b5b4b294e7ca8d32faa4461be7fafb13c:

[1.5.x] Fixed #19772 -- Handled APPEND_SLASH correctly in the redirects app.

Backport of 64623a2.

[Aymeric Augustin <aymeric.augustin@…>]

In 64623a2e115d781c99c7df4dbb67a1e576c0165a:

Fixed #19772 -- Handled APPEND_SLASH correctly in the redirects app.

[Aymeric Augustin <aymeric.augustin@…>]

In 833ccd4b5b4b294e7ca8d32faa4461be7fafb13c:

[1.5.x] Fixed #19772 -- Handled APPEND_SLASH correctly in the redirects app.

Backport of 64623a2.

[Julian Bez]

I believe this change introduces a changed handling of the following situation:

• Old URL was e.g. /download.htm
• Create redirect for /download.htm to /downloads/
• on request /download.htm becomes /download.htm/ because of APPEND_SLASH = True
• RedirectFallbackMiddleware cannot find /download.htm/ in old_path because of this ticket's change. If I pull the code from before, it works.

My current workaround is just to add the slash in the created redirect, because the middleware doesn't try without it anymore.

[Aymeric Augustin]

In the scenario you're describing, RedirectFallbackMiddleware should perform the redirect, not CommonMiddleware ie. step 4 should happen instead of step 3.

Since middleware run in reverse order during the response phase, RedirectFallbackMiddleware must be placed after CommonMiddleware in MIDDLEWARE_CLASSES. Is this your problem? If so, maybe the docs should clarify that.

To make sure we don't lose track of this, would you mind opening a new ticket? Since this one is closed, we aren't keeping track of it. Thank you!

[Julian Bez]

I'm not sure I understand your explanation. I have placed RedirectFallbackMiddleware after CommonMiddleware.

[anonymous]

Middleware order does not matter here, as CommmonMiddleware redirects during the process_request phase, whereas RedirectFallbackMiddleware redirects during process_response. So, RedirectFallbackMiddleware always goes last. Given that CommonMiddleware adds the slash, RedirectFallbackMiddleware should in fact remove it and try looking up a redirect that way.

As a matter of fact, it used to do this (Django 1.4.x):

​https://github.com/django/django/blob/1.4.6/django/contrib/redirects/middleware.py#L15

[anonymous]

Also confirmed what julianb and anonymous are seeing.

CommonMiddleware appends the slash before RedirectFallbackMiddleware is run, no matter the order. The result is that the middleware is always checking against a url with a trailing slash, so creating a new redirect instance without the trailing slash is never found by the middleware. You have to explicitly add a trailing slash to every redirect you create in order for it to work.