#5691 closed (fixed)
Add the active language to generate the cache key
| Reported by: | Owned by: | aaloy | |
|---|---|---|---|
| Component: | Core (Cache system) | Version: | dev | 
| Severity: | Keywords: | cache i18n jgd-blackboard | |
| Cc: | Ramiro Morales | Triage Stage: | Accepted | 
| Has patch: | yes | Needs documentation: | no | 
| Needs tests: | no | Patch needs improvement: | no | 
| Easy pickings: | no | UI/UX: | no | 
Description
If you use a cache per parge approach and i18n, and you use a cookie to change your default language, if the page are cached previously you have some chances to get the wrong translation.
lang = translation.get_language() cache_key = 'views.decorators.cache.cache_header.%s.%s_%s' % (key_prefix, iri_to_uri(request.path),lang)
Attachments (7)
Change History (34)
by , 18 years ago
| Attachment: | django_utils_cache_patch.diff added | 
|---|
comment:1 by , 18 years ago
aaloy: I don't think all keys should be affected by i18n.  Perhaps there should be an alternate to the low-level cache that does add the active lang as a prefix?
comment:2 by , 18 years ago
| Triage Stage: | Unreviewed → Accepted | 
|---|
This should probably only be done when USE_I18N == True, but otherwise this is a good idea.
comment:3 by , 17 years ago
| Owner: | changed from to | 
|---|
comment:4 by , 17 years ago
| Needs documentation: | set | 
|---|---|
| Needs tests: | set | 
| Patch needs improvement: | set | 
by , 17 years ago
| Attachment: | i18n_cache_keys_r7960_t5691.diff added | 
|---|
comment:5 by , 17 years ago
| Owner: | changed from to | 
|---|
This patch works against trunk and checks the USE_I18N setting. However, this patch is current blocked by #730. Incoming requests need to use the Session middleware, then Locale, then Cache, so that the language is determined by the time we hit the cache middleware. However, outgoing responses need Session before Cache as well so that the cookie-based Vary headers can be set.
comment:6 by , 17 years ago
Can someone please test attachment:middleware_ordering_with_path-weight_tuples-backwards-compatible.diff:ticket:730 ? Also see comment:26:ticket:730 .
comment:7 by , 17 years ago
Hrm, Django trac doesn't read links properly...
Patch: http://code.djangoproject.com/attachment/ticket/730/middleware_ordering_with_path-weight_tuples-backwards-compatible.diff
Comment: http://code.djangoproject.com/ticket/730#comment:26
comment:8 by , 17 years ago
Note that this is viable again after [8260]. Patch probably still works, but I didn't like the duplication of code, and it still needs tests and docs of course.
comment:9 by , 17 years ago
I'm working on a patch which could work as:
    cache_key = 'views.decorators.cache.cache_header.%s.%s' % (
                      key_prefix, iri_to_uri(request.path))
    if settings.USE_I18N:
       cache_key += ".%s" % translation.get_language()
I have tested using other ways of string concatenations and this one is the fast way (0.6 s for 106 interactions) for the kind of keys we're generating.
comment:10 by , 17 years ago
| Owner: | changed from to | 
|---|---|
| Status: | new → assigned | 
by , 17 years ago
| Attachment: | patch_cache_i18n.patch added | 
|---|
patch and tests for cache i18n key generation
comment:11 by , 17 years ago
| Needs documentation: | unset | 
|---|---|
| Needs tests: | unset | 
Added the patch an the tests to check that we generate the right keys and that middleware recovers different content depending on the language.
comment:12 by , 17 years ago
| Patch needs improvement: | unset | 
|---|
comment:13 by , 17 years ago
| Cc: | added | 
|---|
comment:14 by , 17 years ago
| Keywords: | jgd-blackboard added | 
|---|
comment:15 by , 17 years ago
/me wonders how the heck did my totally unrelated comments end up in this ticket!? My guess: nazi transgender aliens did it (it's still April 1 in my timezone, so don't you dare to disagree!).
by , 17 years ago
| Attachment: | 5691-use-lan-for-cache-key-r10338.diff added | 
|---|
Updated patch to port-r10335 and simplified tests
comment:16 by , 17 years ago
I've updated Antoni Aloy's patch to apply cleanly after r10335 and simplified tests to only include what's needed to run them as part of the Django test suite, also fixed them so they don't break other tests leaving test environment umodified.
comment:18 by , 16 years ago
| milestone: | → 1.2 | 
|---|
comment:19 by , 16 years ago
| Needs documentation: | set | 
|---|
I'm not sure if we break a promise changing the cache key in 1.2 or if it's ok since _generate_cache_header_key is undocumented API anyway.
Anyway, a change needs to be documented.
comment:20 by , 16 years ago
| Resolution: | → fixed | 
|---|---|
| Status: | assigned → closed | 
jezdez in my oppinion this is not a new feature, is just a bug correction, as one may expect that on a multilingual content page the cache should depend also on the language. Anyway I would be glad to document it.
comment:21 by , 16 years ago
| Resolution: | fixed | 
|---|---|
| Status: | closed → reopened | 
Please don't close the ticket if there wasn't an actual commit.
comment:23 by , 16 years ago
| Needs documentation: | unset | 
|---|
Added an updated patch with documentation warning about the new cache key generation. As is an internal feature I'm not really sure about if it has to be documented or not.
comment:24 by , 16 years ago
| Resolution: | → fixed | 
|---|---|
| Status: | reopened → closed | 
a patch with the proposed cache key