Code

Opened 7 years ago

Closed 4 years ago

Last modified 2 years ago

#5691 closed (fixed)

Add the active language to generate the cache key

Reported by: aaloy@… Owned by: aaloy
Component: Core (Cache system) Version: master
Severity: Keywords: cache i18n jgd-blackboard
Cc: ramiro Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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)

django_utils_cache_patch.diff (2.4 KB) - added by aaloy <aaloy@…> 7 years ago.
a patch with the proposed cache key
i18n_cache_keys_r7960_t5691.diff (2.7 KB) - added by andrewbadr 6 years ago.
cache.patch (2.5 KB) - added by aaloy 5 years ago.
cache pacth to use lang in the cache key
patch_cache_i18n.patch (14.5 KB) - added by aaloy 5 years ago.
patch and tests for cache i18n key generation
5691-use-lan-for-cache-key-r10338.diff (7.8 KB) - added by ramiro 5 years ago.
Updated patch to port-r10335 and simplified tests
5691.patch (6.8 KB) - added by yml 4 years ago.
update the patch against the latest trunk r12453
5691.2.patch (7.3 KB) - added by aaloy 4 years ago.
Patch documentation

Download all attachments as: .zip

Change History (33)

Changed 7 years ago by aaloy <aaloy@…>

a patch with the proposed cache key

comment:1 Changed 6 years ago by jdunck

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

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 Changed 6 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

This should probably only be done when USE_I18N == True, but otherwise this is a good idea.

comment:3 Changed 6 years ago by andrewbadr

  • Owner changed from nobody to andrewbadr

comment:4 Changed 6 years ago by andrewbadr

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set

Changed 6 years ago by andrewbadr

comment:5 Changed 6 years ago by andrewbadr

  • Owner changed from andrewbadr to nobody

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:8 Changed 6 years ago by andrewbadr

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 Changed 5 years ago by aaloy

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 Changed 5 years ago by aaloy

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

Changed 5 years ago by aaloy

cache pacth to use lang in the cache key

Changed 5 years ago by aaloy

patch and tests for cache i18n key generation

comment:11 Changed 5 years ago by aaloy

  • 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 Changed 5 years ago by aaloy

  • Patch needs improvement unset

comment:13 Changed 5 years ago by ramiro

  • Cc ramiro added

comment:14 Changed 5 years ago by jdunck

  • Keywords jgd-blackboard added

comment:15 Changed 5 years ago by mrts

/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!).

Changed 5 years ago by ramiro

Updated patch to port-r10335 and simplified tests

comment:16 Changed 5 years ago by ramiro

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:17 Changed 5 years ago by floledermann

Any progress on checking this in? This is looking good, isn't it?

comment:18 Changed 4 years ago by jezdez

  • milestone set to 1.2

Changed 4 years ago by yml

update the patch against the latest trunk r12453

comment:19 Changed 4 years ago by jezdez

  • 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 Changed 4 years ago by aaloy

  • Resolution set to fixed
  • Status changed from assigned to 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 Changed 4 years ago by jezdez

  • Resolution fixed deleted
  • Status changed from closed to reopened

Please don't close the ticket if there wasn't an actual commit.

comment:22 Changed 4 years ago by aaloy

Opps. Sorry it was not my intention :(

Changed 4 years ago by aaloy

Patch documentation

comment:23 Changed 4 years ago by aaloy

  • 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 Changed 4 years ago by jezdez

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

(In [12546]) Fixed #5691 - Adds the active language to the cache key. Thanks, Antoni Aloy, Ramiro Morales and Yann Malet.

comment:25 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

comment:11 Changed 2 years ago by aaugustin

In [17061]:

Made the cache locale-dependant when USE_L10N is True, even if USE_I18N is False. Refs #5691.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.