Add CACHE_KEY_PREFIX setting that prefixes the low-level cache API

[Byron Ruth]

I started the thread on django-developers ​http://groups.google.com/group/django-developers/browse_thread/thread/c6bfcaca1be25025.

[Russell Keith-Magee]

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.

2. 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.

3. 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.

4. 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.

5. 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.

[Byron Ruth]

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

[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.

2. 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.

3. 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.

4. 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.

5. 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.

[Austin Gabel]

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

[Austin Gabel]

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

[Austin Gabel]

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

[Byron Ruth]

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?

[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.

[Byron Ruth]

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

[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.

[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.

[Russell Keith-Magee]

RC1 of site-wide caching prefix

[Russell Keith-Magee]

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

[Jacob]

Milestone 1.3 deleted