Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#21473 closed Bug (fixed)

Cookie based language detection no longer practical

Reported by: Ludwik Trammer Owned by: nobody
Component: Internationalization Version: dev
Severity: Release blocker Keywords:
Cc: Sergey Kolosov Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I use cookies to save user's custom language preference (and that's not just a matter of personal taste - I use Varnish, so it's important for me to defer putting a session cookie on user's computer as long as possible, because it is unique per user, so does not work good with caching).

Up until Django 1.6 I successfully used the following method:

  1. Use Django's LocaleMiddleware so the correct locale is detected and used.
  2. When user switches languages set a cookie with her new preference, with name based on settings.LANGUAGE_COOKIE_NAME.

I think that sounds pretty responsible and in line with what's in documentation. And it worked as expected. LocaleMiddleware used the cookie's value during language detection.

Unfortunately it all break after upgrading to Django 1.6. In Django 1.6 the following code was added to LocaleMiddleware's process_response() method:

# Store language back into session if it is not present
if hasattr(request, 'session'):
    request.session.setdefault('django_language', language)

It has two unfortunate consequences for me. The former more obvious, the latter more important:

  1. Session cookie is now always present on user's computer. A session is created as soon a she opens the site for the first time. That's defeats the whole purpose of using django_language cookie instead of using sessions.
  1. When a user opens the site for the first time her current language is saved to her session. She haven't had a chance to manually set her preference yet, so the value set to session in based on request headers or LANGUAGE_CODE settings. From now on django_language cookie value is ignored (and for that matter her changing language preferences in her browser is ignored also), because the value from session has the highest priority.

So, to summarise: cookie value is ignored if session value is present, but starting from Django 1.6 session value is always present.

My code broke after upgrading to Django 1.6, just because I used a cookie-based method, that is part of documented Django futures. The only way to use cookies for setting language now, is to disable sessions completely in your project (not an option for me). That's a really big change, and there is literally nothing about it in "backwards incompatible changes" section of Django 1.6 release notes. I also don't see an easy work around this problem for me.

Attachments (1)

21473-master.patch (5.6 KB ) - added by Claude Paroz 11 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 by Ludwik Trammer, 11 years ago

The change was caused by #14825.

comment:2 by Ludwik Trammer, 11 years ago

Maybe behaviour requested by #14825 could by a setting, so it stops breaking backward compatibility?

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

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

The solution in #14825 seems a bit over-aggressive. The implementation in there is "force-assign language back to session for each response". For example this means it is impossible to force-clear the session language if you actually want that. And it also means that the language is stored in the session no matter if you actually want to use session to store the language. As session language overrides cookie language this essentially breaks cookie based language selection. This will also break a view that does this:

def change_language(request, lang):
    request.session['django_language'] = lang
    return HttpResponseRedirect(somepage)

Some proposals for fix:

  1. Assign the language back to session in contrib.auth.logout() right after session.flush(). Revert the change to localemiddleware
  2. In LocaleMiddleware, store the language back only if it came from session. This can be done by setting request._language_was_from_session = True in process_request and checking against that in process_response.
  3. Just revert the change.

I don't particularly like the idea that you can't flush the language code from session, and with option 2 above it will be force-restored on process_response. So, I vote for approach 1. This means that session.flush() will remove the language for next request. But if you store the language in a session and then flush it, isn't that what is supposed to happen?

I haven't actually ran any tests to verify this issue but the analysis in the description are convincing enough for me. Marking this as release blocker as this is a regression in 1.6.

comment:4 by Claude Paroz, 11 years ago

I'm fine with proposal #1.

comment:5 by Ludwik Trammer, 11 years ago

Has patch: set

I wrote a possible fix. It's in my ticket_21473_1_6 topic branch here: https://github.com/ludwiktrammer/django/tree/ticket_21473_1_6

This only work on stable/1.6.x branch and won't work on master. I will create a separate version for master, provided this looks ok.

Tests are included.

Version 1, edited 11 years ago by Ludwik Trammer (previous) (next) (diff)

comment:6 by Claude Paroz, 11 years ago

Triage Stage: AcceptedReady for checkin

Patch looks good (and tests pass).

comment:7 by Ramiro Morales, 11 years ago

See also #12794 and #15902.

comment:8 by anonymous, 11 years ago

I really hope Django adopts behaviour described in #12794 or #15902. It will make my life much easier, because the current behaviour is not really compatible with solutions like Varnish, which prevents me from using the built-in @set_language@ view.
However this bigger change surely won't happen in the stable 1.6.x branch, so my patch for this branch still stands.

comment:9 by Claude Paroz, 11 years ago

I admit I'm struggling porting the patch to master, especially with the i18n.LocaleMiddlewareTests.test_backwards_session_language test.

comment:10 by Ludwik Trammer, 11 years ago

i18n.LocaleMiddlewareTests.test_backwards_session_language "checks that language is stored in session if missing". I believe that means that it tests for the behavior that was explicitly deemed undesirable (or at least this ticket postulates it is undesirable) - the whole point of this ticket is we don't want a missing language to be automatically add to session. So I believe the test should be simply removed and replaced with i18n.LocaleMiddlewareTests.test_language_not_saved_to_session. The latter tests for a behavior that is the exact opposite of the behavior expected by i18n.LocaleMiddlewareTests.test_backwards_session_language.

comment:11 by Claude Paroz, 11 years ago

I don't think so, test_backwards_session_language is testing that even with old django_language cookie name, the language is still kept in the session (with the new _language cookie name). However, I admit this is going very far, and I wouldn't oppose to simply removing that test.

comment:12 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

In c558a43fd6bbcea9972b66965f7e8619bc247df1:

[1.6.x] Fixed #21473 -- Limited language preservation to logout

Current language is no longer saved to session by LocaleMiddleware
on every response (the behavior introduced in #14825).
Instead language stored in session is reintroduced into new session
after logout.

comment:13 by Claude Paroz, 11 years ago

Resolution: fixed
Status: closednew
Triage Stage: Ready for checkinAccepted
Version: 1.6master

Reopening until fixed in master.

in reply to:  11 comment:14 by Ludwik Trammer, 11 years ago

Replying to claudep:

I don't think so, test_backwards_session_language is testing that even with old django_language cookie name, the language is still kept in the session (with the new _language cookie name). However, I admit this is going very far, and I wouldn't oppose to simply removing that test.

You are right, sorry. I got confused by the method description, which turns out to be pretty poor (or maybe it's just me not being a native speaker?). Anyhow. This tests something more specific, but it still just tests a specific part of the behavior that is being removed. More importantly this tests requires a behavior that is entirely unnecessary. There is already code for backward compatybility with `django_language` session key in django.utils.translations. There is no point for the locale middleware to get involved and additionaly rename the key on the fly. I'd say you should just remove the test.

by Claude Paroz, 11 years ago

Attachment: 21473-master.patch added

comment:15 by Claude Paroz, 11 years ago

Ludwik, mind having a look at the attached patch?

comment:16 by Ludwik Trammer, 11 years ago

I think it looks perfect.

comment:17 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 9922ed46e2e81845006792310f3f1a3bf0d7b2a6:

Fixed #21473 -- Limited language preservation to logout

Current language is no longer saved to session by LocaleMiddleware
on every response (the behavior introduced in #14825).
Instead language stored in session is reintroduced into new session
after logout.

Forward port of c558a43fd6 to master.

comment:18 by Sergey Kolosov, 11 years ago

Cc: Sergey Kolosov added

I would like to make an appeal to core developers: all design decisions involving involuntary session creation MUST be made with a great caution.

In case of a high-load project, avoiding to create a session for non-authenticated users is a vital strategy with a critical influence on application performance.

It doesn't really make a big difference, whether you use a database backend, or Redis, or whatever else; eventually, your load would be high enough, and scaling further would not help anymore, so that either network access to the session backend or its “INSERT” performance would become a bottleneck.

In my case, it's an application with 20-25 ms response time under a 20000-30000 RPM load.
Having to create a session for an each session-less request would be critical enough to decide not to upgrade Django, or to fork and rewrite the corresponding components.

Thanks.

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