#19772 closed Bug (fixed)
Redirects don't honor APPEND_SLASH
Reported by: | Aymeric Augustin | Owned by: | nobody |
---|---|---|---|
Component: | contrib.redirects | Version: | 1.4 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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).
Change History (12)
comment:1 by , 12 years ago
comment:2 by , 12 years ago
I just understood the current construct -- it's probably designed to handle querystrings.
comment:3 by , 12 years ago
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.
comment:4 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
I'm selfishly going to backport this to 1.5 because it breaks djangoproject.com...
comment:5 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:8 by , 11 years ago
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.
comment:9 by , 11 years ago
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!
comment:10 by , 11 years ago
I'm not sure I understand your explanation. I have placed RedirectFallbackMiddleware after CommonMiddleware.
comment:11 by , 11 years ago
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
comment:12 by , 11 years ago
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.
I've provisionally applied this change on the server.