Opened 16 years ago

Closed 10 years ago

Last modified 10 years ago

#5789 closed Cleanup/optimization (fixed)

Django LocaleMiddleware django_language should be _language

Reported by: Jeremy Dunck Owned by: Bouke Haarsma
Component: Internationalization Version: dev
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 John DeRosa 16 years ago.
session_language_migration.py (1.3 KB ) - added by Jeremy Dunck 16 years ago.
django_language -> _language migration script for sessions.

Download all attachments as: .zip

Change History (18)

comment:1 by Jacob, 16 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Malcolm Tredinnick, 16 years ago

Component: UncategorizedInternationalization

comment:3 by John DeRosa, 16 years ago

Owner: changed from nobody to John DeRosa
Status: newassigned

comment:4 by John DeRosa, 16 years ago

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 by Malcolm Tredinnick, 16 years ago

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 by Malcolm Tredinnick, 16 years ago

Patch needs improvement: set

by John DeRosa, 16 years ago

Attachment: 5789.patch added

in reply to:  5 ; comment:7 by John DeRosa, 16 years ago

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?

in reply to:  7 comment:8 by Malcolm Tredinnick, 16 years ago

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 by Jeremy Dunck, 16 years ago

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.

by Jeremy Dunck, 16 years ago

django_language -> _language migration script for sessions.

comment:10 by Jeremy Dunck, 16 years ago

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 by Gabriel Hurley, 13 years ago

Severity: Normal
Type: Cleanup/optimization

comment:12 by patchhammer, 13 years ago

Easy pickings: unset
Patch needs improvement: set

5789.patch fails to apply cleanly on to trunk

comment:13 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:14 by Bouke Haarsma, 10 years ago

Owner: changed from John DeRosa to Bouke Haarsma

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 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

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 by Tim Graham <timograham@…>, 10 years ago

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