Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#13795 closed Uncategorized (fixed)

Add CACHE_KEY_PREFIX setting that prefixes the low-level cache API

Reported by: Byron Ruth Owned by: Byron Ruth
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 Byron Ruth 6 years ago.
cache_key_prefix_13448.diff (17.2 KB) - added by Byron Ruth 6 years ago.
cache_key_prefix_13959.diff (18.3 KB) - added by Austin Gabel 6 years ago.
cache_key_prefix_13891.diff (18.1 KB) - added by Austin Gabel 6 years ago.
cache_key_prefix_14190.diff (34.2 KB) - added by Byron Ruth 6 years ago.
cache_key_prefix_14233.diff (34.2 KB) - added by Byron Ruth 6 years ago.
cache_key_prefix_14405.diff (34.2 KB) - added by Byron Ruth 6 years ago.
t13795-rc1.diff (48.1 KB) - added by Russell Keith-Magee 6 years ago.
RC1 of site-wide caching prefix

Download all attachments as: .zip

Change History (24)

Changed 6 years ago by Byron Ruth

Attachment: cache_key_prefix_13357.diff added

comment:1 Changed 6 years ago by Byron Ruth

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Summary: Add optional CACHE_KEY PREFIX setting that prefixes the low-level cache APIAdd CACHE_KEY_PREFIX setting that prefixes the low-level cache API

comment:2 Changed 6 years ago by Byron Ruth

Status: newassigned

comment:3 Changed 6 years ago by Russell Keith-Magee

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 6 years ago by Byron Ruth

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

comment:5 in reply to:  3 Changed 6 years ago by Byron Ruth

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 6 years ago by Byron Ruth

Attachment: cache_key_prefix_13448.diff added

Changed 6 years ago by Austin Gabel

Attachment: cache_key_prefix_13959.diff added

comment:6 Changed 6 years ago by Austin Gabel

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

Changed 6 years ago by Austin Gabel

Attachment: cache_key_prefix_13891.diff added

comment:7 Changed 6 years ago by Austin Gabel

milestone: 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 6 years ago by Austin Gabel

Triage Stage: AcceptedReady 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 6 years ago by Byron Ruth

Triage Stage: Ready for checkinAccepted

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 6 years ago by Byron Ruth

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 6 years ago by Byron Ruth

Attachment: cache_key_prefix_14190.diff added

comment:11 Changed 6 years ago by Byron Ruth

Triage Stage: AcceptedReady for checkin

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

comment:12 Changed 6 years ago by Austin Gabel

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 6 years ago by Byron Ruth

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 6 years ago by Byron Ruth

Attachment: cache_key_prefix_14233.diff added

Changed 6 years ago by Byron Ruth

Attachment: cache_key_prefix_14405.diff added

Changed 6 years ago by Russell Keith-Magee

Attachment: t13795-rc1.diff added

RC1 of site-wide caching prefix

comment:14 Changed 6 years ago by Russell Keith-Magee

Resolution: fixed
Status: assignedclosed

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

comment:15 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:16 Changed 5 years ago by Michael Curry

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