Opened 8 years ago

Closed 23 months ago

Last modified 18 months ago

#5789 closed Cleanup/optimization (fixed)

Django LocaleMiddleware django_language should be _language

Reported by: jdunck Owned by: bouke
Component: Internationalization Version: master
Severity: Normal Keywords: session i18n backwards-incompatible
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

According to the Django book, all Django-reserved session keys begin with "_", but the framework currently uses "django_language" rather than "_language".

Attachments (2)

5789.patch (3.7 KB) - added by stugots 7 years ago.
session_language_migration.py (1.3 KB) - added by jdunck 7 years ago.
django_language -> _language migration script for sessions.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 8 years ago by mtredinnick

  • Component changed from Uncategorized to Internationalization

comment:3 Changed 7 years ago by stugots

  • Owner changed from nobody to stugots
  • Status changed from new to assigned

comment:4 Changed 7 years ago by stugots

  • Has patch set

Patch file attached.

Note, svn thinks all of i18n.txt changed due to bug #6545, which refers to a line-ending problem with pool text files.

comment:5 follow-up: Changed 7 years ago by mtredinnick

The goal here is only to change the name of the variable stored inside the session dictionary to be consistent with other Django internal session keys. It is not about changing the cookie name. So the first part of the patch can be removed.

This does introduce an interesting backwards incompatibility. If a site upgrades to this version of the code, their users will lose the session language setting (since Django will start looking for a different key name) and if the session language is different from their browser preference, it will revert to the browser preference. It's a little annoying, because we try to avoid breaking things for users.

For bonus points, does anybody want to come up with a little Python script we can put on the backwards incompatible page in the wiki that will upgrade all sessions in the database to use the new format: it should unpack the session, moves the django_language key to _language and then resaves the session,preferably without modifying the expiry time. I guess this isn't a showstopper, since the number of people affected will be small (usually the browser and the language preference match, but conflicts like Catalan/Spanish do exist).

comment:6 Changed 7 years ago by mtredinnick

  • Patch needs improvement set

Changed 7 years ago by stugots

comment:7 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by stugots

  • Patch needs improvement unset

Replying to mtredinnick:

The goal here is only to change the name of the variable stored inside the session dictionary to be consistent with other Django internal session keys. It is not about changing the cookie name. So the first part of the patch can be removed.

My bad. That was sloppy of me. I feel rather sheepish about it.

A new and improved .patch file is attached.

This does introduce an interesting backwards incompatibility. If a site upgrades to this version of the code, their users will lose the session language setting (since Django will start looking for a different key name) and if the session language is different from their browser preference, it will revert to the browser preference. It's a little annoying, because we try to avoid breaking things for users.

You're right. If I may offer my opinion, this is a minor incompatibility. Nothing permanently breaks, no code changes are needed, and no fatal errors will occur. Just a, "Hmm, that's odd," for the user...

For bonus points, does anybody want to come up with a little Python script we can put on the backwards incompatible page in the wiki that will upgrade all sessions in the database to use the new format: it should unpack the session, moves the django_language key to _language and then resaves the session,preferably without modifying the expiry time. I guess this isn't a showstopper, since the number of people affected will be small (usually the browser and the language preference match, but conflicts like Catalan/Spanish do exist).

Well, a better way would be to add code to check for the "django_language" variable if the "_language" variable doesn't exist.

This wouldn't be documented on the Django site. So it would support the deprecated session variable name for those users existing prior to this change. This would be better than an upgrade-sessions script. Effectively transparent. Do you want me to add this to the patch?

comment:8 in reply to: ↑ 7 Changed 7 years ago by mtredinnick

Replying to stugots:

You're right. If I may offer my opinion, this is a minor incompatibility. Nothing permanently breaks, no code changes are needed, and no fatal errors will occur. Just a, "Hmm, that's odd," for the user...

Breakage is a much better failure mode than hard to detect "that's odd" cases that affect an unknown number of users. Developers can do something about breakage. They can't do something anything about the fact that all their affected users (who they don't know about and we don't either) now have to remember how to reset language preference. Imagine if you were on the support desk of a product that was affected by this. If it actually broke the code, one developer would need to fix something before the rollout. This change has a different effect.

Yes, it's minor and not a showstopper, but it is something we're going to have to make a big effort to communicate and it's why I'd like the script to update the sessions table. It's more serious than if there was direct code breakage as a result of this because this breakage isn't quite fixable and the script isn't going to be hard for somebody to write.

Well, a better way would be to add code to check for the "django_language" variable if the "_language" variable doesn't exist.

No. That would defeat the whole purpose, which is to free up the django_language variable for other uses. Somebody who used that variable would be inadvertently corrupting the language preference of their users. If we were happy to keep using that variable name we wouldn't apply this change.

comment:9 Changed 7 years ago by jdunck

This upgrade script turns out to be non-trivial, since the number of sessions involved can be very large, Session.get_decoded() is expensive, and QuerySet._result_cache is rather greedy.

I'm attaching a migration script, but I'm not sure where it belongs in the tree.

In the case that there are no languages to port, I don't save a session back unless mutation was necessary. Also, re-runs of the file should be safe, since I check for existence of _language before assigning to it.

Do you think it'd be better to throw up our hands if _language already exists? Perhaps an app already uses that? ...I think simply documenting that "if you run this script, _language will be overwritten" is reasonable.

Changed 7 years ago by jdunck

django_language -> _language migration script for sessions.

comment:10 Changed 7 years ago by jdunck

Also, on my laptop, the bottlneck was surprisingly in *selecting* the pages. I think it may be that I don't have enough shared buffers to allow in-memory sorting of the session keys. I chose order_by('session_key') because otherwise, I was afraid the update itself might change the order of the session results out from under me...

comment:11 Changed 4 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:12 Changed 4 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set

5789.patch fails to apply cleanly on to trunk

comment:13 Changed 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:14 Changed 23 months ago by bouke

  • Owner changed from stugots to bouke

I've updated the patch and created a PR: https://github.com/django/django/pull/1771

How should the migration be carried out? It could be done using a one-time management command, but is it desirable to add it?

comment:15 Changed 23 months ago by Tim Graham <timograham@…>

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

In 0d0f4f020afe516f23fd2305f13ff0a6a539b344:

Fixed #5789 -- Changed LocaleMiddleware session variable to '_language'.

The old 'django_language' variable will still be read from in order
to migrate users. The backwards-compatability shim will be removed in
Django 1.8.

Thanks to jdunck for the report and stugots for the initial patch.

comment:16 Changed 18 months ago by Tim Graham <timograham@…>

In 6d1ae5e27c3fe612209023bacd8a201fffedc375:

Removed reading of old 'django_language' session variable per deprecation timeline.

refs #5789.

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