Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#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 Aymeric Augustin, 12 years ago

I've provisionally applied this change on the server.

comment:2 by Aymeric Augustin, 12 years ago

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

comment:3 by Carl Meyer, 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 Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedAccepted

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

comment:5 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

Resolution: fixed
Status: newclosed

In 833ccd4b5b4b294e7ca8d32faa4461be7fafb13c:

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

Backport of 64623a2.

comment:6 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

In 64623a2e115d781c99c7df4dbb67a1e576c0165a:

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

comment:7 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

In 833ccd4b5b4b294e7ca8d32faa4461be7fafb13c:

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

Backport of 64623a2.

comment:8 by Julian Bez, 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.

Last edited 11 years ago by Julian Bez (previous) (diff)

comment:9 by Aymeric Augustin, 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 Julian Bez, 11 years ago

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

comment:11 by anonymous, 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 anonymous, 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.

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