Opened 13 years ago

Closed 13 years ago

Last modified 6 years ago

#17734 closed Bug (fixed)

302 redirect to standard language for non-i18n URL's when returning 404.

Reported by: petar@… Owned by: Jannis Leidel
Component: Internationalization Version: dev
Severity: Release blocker Keywords: urlconf, i18n
Cc: Orne Brocaar, Jannis Leidel Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I'm using [Tastypie] to supply an API for my project. This API should be a language independent URI, while some other applications have language dependent URI's. This leaves me with the following URLConf:

from django.utils.translation import ugettext_lazy as _
from django.conf.urls.defaults import patterns, include, url
from django.conf.urls.i18n import i18n_patterns

urlpatterns = patterns('',
    url(r'^api/', include(v1_api.urls)),
)

urlpatterns += i18n_patterns('',
    url(_(r'^something/'), include('something.urls')),
)

When I raise a 404 in the API, the LocaleMiddleware detects this and tries to redirect to /en/api/[..]. I think this happens because is_language_prefix_patterns_used always returns True because it loops through all the URL's in the URLConf and finds a locale URL. However, the URL for the API is not an Locale URL.

[Tastypie]: https://github.com/toastdriven/django-tastypie

Attachments (1)

17734.1.diff (9.1 KB ) - added by Jannis Leidel 13 years ago.
Proposed patch..

Download all attachments as: .zip

Change History (11)

comment:1 by Carl Meyer, 13 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Marking as a release blocker since this is a significant bug in a new feature.

comment:2 by Orne Brocaar, 13 years ago

When implementing this feature during the DjangoCon sprints, jezdez and I discussed this issue / side-effect. From what I can remember, we discussed different approaches to fix this side-effect. In the end, we decided to leave it as it was (I forgot the exact reason, but probably since the solutions we found made it too complex).

The most simple approach would be (I think, but feedback is welcome) to use the same logic as the CommonMiddleware uses for the APPEND_SLASH logic. Before redirecting the user to the language-prefixed URL, it will then first test if the prefixed URL version is resolvable, if not, it doesn't redirect the user. This would then mean that the LocaleMiddleware would share some logic with the CommonMiddleware because it should know about the APPEND_SLASH setting. If this setting is set to True, it should test for both /en/something and /en/something/. I'm not sure if that is desirable...

What happens now when the user requests /something, the LocaleMiddleware will redirect the user (assuming it detects the language as en) to /en/something, then the CommonMiddleware will redirect the user to /en/something/.

What do you guys think?

comment:3 by Anssi Kääriäinen, 13 years ago

The problem isn't about the /something pattern. It is about treating the /api pattern as locale aware while it isn't one. I might be missing the point in comment:2, but I think it is about a different issue?

comment:4 by Orne Brocaar, 13 years ago

Yes, but what happens now, when you request /api/raising/404/, it is redirecting the user to /en/api/raising/404/ (assuming it detects the language as en). If it tests before redirecting the user if /en/api/raising/404/ is resolvable, it would find out that this would raise a 404 as well and thus would not redirect the user.

comment:5 by Aymeric Augustin, 13 years ago

Owner: changed from nobody to Jannis Leidel

comment:6 by Jannis Leidel, 13 years ago

Nah, @akaariai, @brocaar is right, the LocaleMiddleware does the redirection when it detects that a i18n enabled URL is used so we better check if that URL exists before redirecting to the URL path -- in that sense pretty much what APPEND_SLASH does, except we need to check if the full path matches instead of just appended an /.

@brocaar I think this might change a bunch of existing tests, am I right?

by Jannis Leidel, 13 years ago

Attachment: 17734.1.diff added

Proposed patch..

comment:7 by Jannis Leidel, 13 years ago

Has patch: set

comment:8 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: newclosed

In [17621]:

Fixed #17734 -- Made sure to only redirect translated URLs if they can actually be resolved to prevent unwanted redirects. Many thanks to Orne Brocaar and Anssi Kääriäinen for input.

comment:9 by geoffhing@…, 11 years ago

I'm not sure if this should be the default behavior, but I feel that process_response() should also check:

  • If the non-language path is valid
  • If the non-language path resolves to the same view as the language path

If the non-language path is valid, the redirect should only be performed if the redirect goes to the same view.

I'm running into the same problem as the original poster when my URL configuration contains a "catch-all" regex, as is required by Django CMS:

from django.conf.urls.defaults import patterns, include, url
from django.conf.urls.i18n import i18n_patterns

from cms.views import details

urlpatterns = patterns('',
    url(r'^api/', include(v1_api.urls)),
)

urlpatterns += i18n_patterns('',
    url(r'^something/', include('something.urls')),
    url(r'^(?P<slug>[0-9A-Za-z-_.//]+)/$', details, name='pages-details-by-slug')
)

So, a request to an API endpoint (for example "/api/endpoint/NOT-FOUND-ID/") that returns a 404, will be redirected to "/en/api/endpoint/NOT-FOUND-ID/" because is_valid() will return true for the language path because it matches the regex for pages-details-by-slug.

comment:10 by Rik, 6 years ago

For what it's worth (this is a old ticket), I ran into the same problem as @geoffhing, where I had a API which on 404 would redirect to a language-prefixed URL because I was using a catch-all url pattern:

urlpatterns = [
    url(r'^api/', include(api.urls)),
]

urlpatterns += i18n_patterns(
    url(r'^something/', include('something.urls')),
    url(r'^', include(wagtail_urls)),
)

I fixed it by extending the default Django LocaleMiddleware:

from django.middleware.locale import LocaleMiddleware as DjangoLocaleMiddleware


class LocaleMiddleware(DjangoLocaleMiddleware):

    def process_response(self, request, response):

        if request.path.startswith('/api/'):
            # Prevent `/api/` URLs from redirecting to language-prefixed URLs,
            # because the API client will get a 302 redirect response instead
            # of 404, which is not what we want.
            return response

        return super().process_response(request, response)

This was the best solution I could come up with. I preferrably don't override default django classes but in this case I couldn't think of any other solution. It works quite fine though.

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