Code

Opened 2 years ago

Closed 2 years ago

Last modified 11 months ago

#17734 closed Bug (fixed)

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

Reported by: petar@… Owned by: jezdez
Component: Internationalization Version: master
Severity: Release blocker Keywords: urlconf, i18n
Cc: brocaar, jezdez 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 jezdez 2 years ago.
Proposed patch..

Download all attachments as: .zip

Change History (10)

comment:1 Changed 2 years ago by carljm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 Changed 2 years ago by brocaar

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

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

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

  • Owner changed from nobody to jezdez

comment:6 Changed 2 years ago by jezdez

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?

Changed 2 years ago by jezdez

Proposed patch..

comment:7 Changed 2 years ago by jezdez

  • Has patch set

comment:8 Changed 2 years ago by jezdez

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

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 Changed 11 months ago by geoffhing@…

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.