Opened 22 months ago

Closed 21 months ago

Last modified 21 months ago

#21473 closed Bug (fixed)

Cookie based language detection no longer practical

Reported by: ludwik Owned by: nobody
Component: Internationalization Version: master
Severity: Release blocker Keywords:
Cc: sergeykolosov 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 claudep 21 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 22 months ago by ludwik

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The change was caused by #14825.

comment:2 Changed 22 months ago by ludwik

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

comment:3 Changed 22 months ago by akaariai

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 22 months ago by claudep

I'm fine with proposal #1.

comment:5 Changed 22 months ago by ludwik

  • 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 22 months ago by ludwik (previous) (next) (diff)

comment:6 Changed 21 months ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

Patch looks good (and tests pass).

comment:7 Changed 21 months ago by ramiro

See also #12794 and #15902.

comment:8 Changed 21 months ago by anonymous

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 Changed 21 months ago by claudep

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

comment:10 Changed 21 months ago by ludwik

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 follow-up: Changed 21 months ago by 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.

comment:12 Changed 21 months ago by Claude Paroz <claude@…>

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

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 Changed 21 months ago by claudep

  • Resolution fixed deleted
  • Status changed from closed to new
  • Triage Stage changed from Ready for checkin to Accepted
  • Version changed from 1.6 to master

Reopening until fixed in master.

comment:14 in reply to: ↑ 11 Changed 21 months ago by ludwik

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.

Changed 21 months ago by claudep

comment:15 Changed 21 months ago by claudep

Ludwik, mind having a look at the attached patch?

comment:16 Changed 21 months ago by ludwik

I think it looks perfect.

comment:17 Changed 21 months ago by Claude Paroz <claude@…>

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

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 Changed 21 months ago by sergeykolosov

  • Cc sergeykolosov 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