Opened 2 years ago

Closed 2 years ago

Last modified 14 months ago

#19772 closed Bug (fixed)

Redirects don't honor APPEND_SLASH

Reported by: aaugustin 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 Changed 2 years ago by aaugustin

I've provisionally applied this change on the server.

comment:2 Changed 2 years ago by aaugustin

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

comment:3 Changed 2 years ago by carljm

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 Changed 2 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

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

comment:5 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 833ccd4b5b4b294e7ca8d32faa4461be7fafb13c:

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

Backport of 64623a2.

comment:6 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

In 64623a2e115d781c99c7df4dbb67a1e576c0165a:

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

comment:7 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

In 833ccd4b5b4b294e7ca8d32faa4461be7fafb13c:

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

Backport of 64623a2.

comment:8 Changed 2 years ago by julianb

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 2 years ago by julianb (previous) (diff)

comment:9 Changed 2 years ago by aaugustin

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 Changed 2 years ago by julianb

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

comment:11 Changed 20 months ago by 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

comment:12 Changed 14 months ago by 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.

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