APPEND_SLASH doesn't work with URLs that have query strings

[Andrzej Winnicki]

Hi,

In middleware/common.py there is a code, which checks whether the slash should be appended to URL.

def should_redirect_with_slash(self, request):
        """
        Return True if settings.APPEND_SLASH is True and appending a slash to
        the request path turns an invalid path into a valid one.
        """

        if settings.APPEND_SLASH and not request.get_full_path().endswith('/'):
            urlconf = getattr(request, 'urlconf', None)

            return (
                not urlresolvers.is_valid_path(request.path_info, urlconf)
                and urlresolvers.is_valid_path('%s/' % request.path_info, urlconf)
            )
        return False

In most cases it works fine, but it fails to append slash, when used with LOGIN_URL without slash and with "next" parameter. For example:

It works fine: /accounts/login/?next=/polls/3/ -> LOGIN_URL = /accounts/login/
But this one returns 404: /accounts/login?next=/polls/3/ -> LOGIN_URL = /accounts/login (no trailing slash).

It is because: request.get_full_path().endswith('/') checks also query string and therefore returns true and the slash is never appended.

[Tim Graham]

It seems odd to have LOGIN_URL = /accounts/login (no trailing slash) but APPEND_SLASH = True. Do you have a practical use case for that configuration?

[Tim Graham]

I'm not convinced that the complexity of making APPEND_SLASH work with URLs that have query strings is worth it. Feel free to explain the use case is more detail.

[Sven Engström]

The problem is the inconsistency in CommonMiddleware between should_redirect_with_slash and get_full_path_with_slash (with force_append_slash=True) where the first checks if the url including query string ends with a slash and the later adds a slash between the path and the query string.

In my opinion the best way to solve this is to replace request.get_full_path().endswith('/') with request.path_info.endswith('/') in should_redirect_with_slash.

[Sven Engström]

I wrote a patch and opened a pull request that does what I mentioned earlier even though this ticket isn't accepted yet. Feel free to close the request if this ticket is rejected.

[Simon Charette]

Patch LGTM. I think it's worth shipping as relying on path_info makes more sense there.

[Tim Graham <timograham@…>]

In f46a838e:

Fixed #26812 -- Fixed APPEND_SLASH on a URL including querystring with a trailing slash.