cache backends that are primarily for local dev should enforce same key restrictions as memcached

[nicois <nicois.nicois@…>]

Using a locmem:// or postgres cache backend, I can generate a cache key using the following expression:
repr(f.__module__) + repr(f.__name__) + repr(args) + repr(kwargs)
but if I configure memcached, it fails silently, causing an empty page to be sent to the client.

I have confirmed this is the source of the problem by generating an md5.hexdigest of the above key - in which case, memcached works correctly.

I have included the context for the above key, in case it is relevant. It is a decorator method which caches the return value of an arbitrary function, assuming its value depeonds solely on its parameters, and that the method never returns None

from django.core.cache import cache as _djcache
def cache(seconds = 3600):
    def doCache(f):
        def x(*args, **kwargs):
                key = md5(repr(f.__module__) + repr(f.__name__) + repr(args) + repr(kwargs)).hexdigest()
                result = _djcache.get(key)
                if result is None:
                    result = f(*args, **kwargs)
                    _djcache.set(key, result, seconds)
                return result
        return x
    return doCache

[john@…]

The problem is that memcached doesn't allow whitespace in keys. I ran into this a while back, and dealing with it in your code is the right solution. Changing the memcached backend to do it makes examining the cache next to impossible, as Jacob explained while closing #3241 as "wontfix". You could try urllib.quote(key) or re.sub('\s+','',key) if you want to make the keys less opaque (and if you know they'll end up under 250 characters, I suppose).

[MichaelBishop]

Decision needed. It appears that John has a good point. I'd suggest to either fix the code or add a note to the docs. At the very least I'd suggest adding a note to the docs. I don't feel qualified in this area to attempt to take a shot at adding this to the docs.

[Julian Bez]

#7460 tackles the issue, too.

[wam]

This ticket was apparently closed because #7460 dealt with bad keys and a solution was committed for that ticket. The problem is that this happened only at the template tag interface. Applications that use the low level interface to the cache system still break with bad keys. The documentation for the cache system still does not mention this limitation of the memcache backend, and having inconsistent keying limitations (especially one of such a basic level as whitespace vs no whitespace) in various backends makes it much harder to build reusable apps (a cache key that works for my localmem:// cache may not work with the memcached backend being used on someone elses server).

Ideally, I'd like to see the memcached backend have similar logic applied to it as was used in in #7460 (e.g. use urlquote to ensure keys have no spaces in them) so that all interfaces are protected from spaces. Lacking that, I'd suggest an update to the cache docs that draws attention to memcached's limitation and provides a recommendation to application programmers who are writing applications that use the low-level cache layer to always escape their keys if their app might be used on a server that utilizes memcached.

[Carl Meyer]

Following discussion with jacobkm and jezdez in IRC, the plan is to leave the memcached backend unmodified (as any key mangling there could slow down a critical code path), but modify the other builtin backends (locmem, dummy, file, db) so they throw an error (or perhaps just a warning?) if you pass them a key that would be invalid on memcached. This discourages/prevents writing cache code that would not be portable to memcached.

I'm working on a patch.

[Carl Meyer]

Oh, and #13066 was a duplicate of this.

[Carl Meyer]

Marking as accepted per the above IRC conversation with core committers.

[Carl Meyer]

patch to enforce same key restrictions on all cache backends

[Carl Meyer]

Comments on the above patch:

1. I chose to make an invalid key an exception, rather than a warning. My feeling is cache code ought to be backend-portable, so if it'll blow up on memcached it might as well blow up elsewhere too. I'll change this if core devs prefer it as a warning.

2. If the idea is that cache-code ought to be backend-portable, is there any reason for the dummy backend to still accept *args, kwargs on most of its methods, where the other backends do not? What's the rationale for keeping this?

3. As noted before, the design decision here is not to apply any additional key checking to the memcached backend, for speed reasons. The restrictions applied match memcached's restrictions (it appears that some control characters are accepted in keys by memcached itself, but python-memcached applies the same restrictions we apply here), so the result should be the same on all backends, except that memcached will raise a different exception.

[Carl Meyer]

same as previous patch, with added doc note re key restrictions

[Carl Meyer]

updated patch, applies to trunk

[Malcolm Tredinnick]

Since Django supports third-party cache backends, we can't control (thankfully!) what possible external caches people might choose to use in production. So an error here, restricting everybody to memcache's particular limitations, feels inappropriate. A warning -- in it's own class so that it can be easily silenced via warnings.ignore() -- would be my preference. Definitely warning people of importabilities is a good plan. Forcing them to effectively choose memcache for now and forever is bad.

[Carl Meyer]

updated patch using warnings

[Carl Meyer]

Added a new patch following design discussion with Russell K-M, Malcolm T, Jannis L, and Jeremy Dunck.

[Carl Meyer]

fixed a couple documentation issues in the previous patch

[Malcolm Tredinnick]

(In [13766]) Add warning when using cache keys that might not work with memcached.

This means testing with local dev caches (not memcache) will warn
developers if they are introducing inadvertent importabilities. There is
also the ability to silence the warning if a dev is not planning to use
memcache and knows what they are doing with their keys.

Thanks to Carl Meyer for the patch. Fixed #6447.

[Malcolm Tredinnick]

(In [13767]) [1.2.X] Add warning when using cache keys that might not work with
memcached.

This means testing with local dev caches (not memcache) will warn
developers if they are introducing inadvertent importabilities. There
is also the ability to silence the warning if a dev is not planning to
use memcache and knows what they are doing with their keys.

Thanks to Carl Meyer for the patch. Fixed #6447.

Backport of r13766 from trunk.