Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#13795 closed Uncategorized (fixed)

Add CACHE_KEY_PREFIX setting that prefixes the low-level cache API

Reported by: bruth Owned by: bruth
Component: Core (Cache system) Version: master
Severity: Normal Keywords:
Cc: inactivist@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Attachments (8)

cache_key_prefix_13357.diff (16.4 KB) - added by bruth 5 years ago.
cache_key_prefix_13448.diff (17.2 KB) - added by bruth 5 years ago.
cache_key_prefix_13959.diff (18.3 KB) - added by agabel 5 years ago.
cache_key_prefix_13891.diff (18.1 KB) - added by agabel 5 years ago.
cache_key_prefix_14190.diff (34.2 KB) - added by bruth 5 years ago.
cache_key_prefix_14233.diff (34.2 KB) - added by bruth 5 years ago.
cache_key_prefix_14405.diff (34.2 KB) - added by bruth 5 years ago.
t13795-rc1.diff (48.1 KB) - added by russellm 5 years ago.
RC1 of site-wide caching prefix

Download all attachments as: .zip

Change History (24)

Changed 5 years ago by bruth

comment:1 Changed 5 years ago by bruth

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from Add optional CACHE_KEY PREFIX setting that prefixes the low-level cache API to Add CACHE_KEY_PREFIX setting that prefixes the low-level cache API

comment:2 Changed 5 years ago by bruth

  • Status changed from new to assigned

comment:3 follow-up: Changed 5 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

This looks pretty good. A couple of quick points of feedback:

  1. I think the _key() utility method should actually be public API, not a private method. Although a cache prefix is the obvious use case, I'm sure there are other potential uses -- modifying the cache key based on language settings is one possibility that springs to mind. If we're going to add a function for computing the cache key, it makes sense that it should be exposed and documented.


  1. The key_prefix argument in cache backend constructors needs to have a default value (of ). Any existing subclasses of a cache class won't be passing down the key_prefix argument, so the default value needs to be provided down.
  1. When instantiating the default cache instance, you should use the keyword argument by name (i.e., key_prefix=settings.CACHE_KEY_PREFIX). This is just protection against backwards compatibility, plus a little bit of readability.
  1. The tests are very thorough, but they miss one important test case -- that the prefix is being used at all. The tests could be simplified quite a bit, because there really no need to check the empty cache prefix case -- that's a degenerate case of the non-empty cache prefix case. However, it is important to check that when a cache prefix is in use, that the same key *without* the prefix isn't a cache hit (unless the cache key is empty). Much of the test duplication could be removed by making the non-empty cache prefix the default testing case, adding an extra test to ensure that the empty prefix case isn't a cache hit, and then adding a separate test case that runs exactly one test for the empty prefix case.
  1. In the documentation for CACHE_MIDDLEWARE_PREFIX you should make mention of the fact that the CACHE_MIDDLEWARE_PREFIX will be appended to the CACHE_KEY_PREFIX - i.e., that CACHE_KEY_PREFIX is a global prefix.

comment:4 Changed 5 years ago by bruth

Thanks for the feedback Russell, I will be updating the patch shortly.

comment:5 in reply to: ↑ 3 Changed 5 years ago by bruth

Replying to russellm:

  1. I think the _key() utility method should actually be public API, not a private method. Although a cache prefix is the obvious use case, I'm sure there are other potential uses -- modifying the cache key based on language settings is one possibility that springs to mind. If we're going to add a function for computing the cache key, it makes sense that it should be exposed and documented.

Now named make_key(). Added a docstring noting that this can be overridden in subclassing for custom key making behavior.

  1. The key_prefix argument in cache backend constructors needs to have a default value (of ). Any existing subclasses of a cache class won't be passing down the key_prefix argument, so the default value needs to be provided down.

Fixed.

  1. When instantiating the default cache instance, you should use the keyword argument by name (i.e., key_prefix=settings.CACHE_KEY_PREFIX). This is just protection against backwards compatibility, plus a little bit of readability.

Updated.

  1. The tests are very thorough, but they miss one important test case -- that the prefix is being used at all. The tests could be simplified quite a bit, because there really no need to check the empty cache prefix case -- that's a degenerate case of the non-empty cache prefix case. However, it is important to check that when a cache prefix is in use, that the same key *without* the prefix isn't a cache hit (unless the cache key is empty). Much of the test duplication could be removed by making the non-empty cache prefix the default testing case, adding an extra test to ensure that the empty prefix case isn't a cache hit, and then adding a separate test case that runs exactly one test for the empty prefix case.

I simplified things a bit. Rather than subclassing for each backend, I merely added another test in BaseCacheTests to test for key clashes when using the prefix vs. not since that is of greatest concern.

  1. In the documentation for CACHE_MIDDLEWARE_PREFIX you should make mention of the fact that the CACHE_MIDDLEWARE_PREFIX will be appended to the CACHE_KEY_PREFIX - i.e., that CACHE_KEY_PREFIX is a global prefix.

Add a note to the CACHE_KEY_PREFIX section regarding CACHE_MIDDLEWARE_KEY_PREFIX.

Changed 5 years ago by bruth

Changed 5 years ago by agabel

comment:6 Changed 5 years ago by agabel

I updated the patch to work with Malcolm's cache key validation.

Changed 5 years ago by agabel

comment:7 Changed 5 years ago by agabel

  • milestone set to 1.3
  • Patch needs improvement unset

My last patch had a few problems with file based caching. The latest is up to date with trunk and passes all tests.

comment:8 Changed 5 years ago by agabel

  • Triage Stage changed from Accepted to Ready for checkin

Unless someone is working on a better patch or sees a problem with mine, I'd say this one is ready for checkin.

comment:9 Changed 5 years ago by bruth

  • Triage Stage changed from Ready for checkin to Accepted

I have an addition to the patch based on a suggestion from Jacob regarding cache versioning. I wrote a post yesterday on google groups, but it seems like it never got posted. I will reiterate here. Based on Eric Flo's http://github.com/ericflo/django-newcache cache backend, I think adding a CACHE_VERSION works well as a base setting, but more flexibility at runtime can be added by allowing for passing in a version keyword arg when getting/setting cache values:

cache.set('key', 'value', version=2)
cache.get('key', version=1) # None 

The default version will be CACHE_VERSION, but being able to pass a version during runtime could allow for more flexible cache versioning such as syncing versions of cache between multiple objects like a page and its components and cache invalidation, e.g. if the page's version is 2, and its components' version is 1, rebuild and cache.

Thoughts?

comment:10 Changed 5 years ago by bruth

I should clarify that I have not written the patch just yet. I wanted to get some quick feedback if people would find it useful, then I will write it.

Changed 5 years ago by bruth

comment:11 Changed 5 years ago by bruth

  • Triage Stage changed from Accepted to Ready for checkin

I implemented cache versioning and added the appropriate unit tests. Please provide feedback.

comment:12 follow-up: Changed 5 years ago by agabel

I don't like the fact that validate_key() was moved to make_key(). If you override make_key() it makes it too easy to forget to validate the key before continuing. The rest of the patch looks fine.

comment:13 in reply to: ↑ 12 Changed 5 years ago by bruth

Replying to agabel:

I don't like the fact that validate_key() was moved to make_key(). If you override make_key() it makes it too easy to forget to validate the key before continuing. The rest of the patch looks fine.

I agree except that validate_key() is being called within the BaseCache.make_key method, not in the make_key function outside of the class. Because that is confusing, I will make the BaseCache.make_key method *private* since the method is really a proxy for the make_key function.

Changed 5 years ago by bruth

Changed 5 years ago by bruth

Changed 5 years ago by russellm

RC1 of site-wide caching prefix

comment:14 Changed 5 years ago by russellm

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

(In [14623]) Fixed #13795 -- Added a site-wide cache prefix and cache versioning. Thanks to bruth for the patch.

comment:15 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:16 Changed 4 years ago by inactivist

  • Cc inactivist@… added
  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset
Note: See TracTickets for help on using tickets.
Back to Top