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.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@13766 bcc190cf-cafb-0310-a4f2-bffc1f526a37

django/core/cache/__init__.py

@@ -18,7 +18,7 @@ See docs/cache.txt for information on the public API.
 from cgi import parse_qsl
 from django.conf import settings
 from django.core import signals
-from django.core.cache.backends.base import InvalidCacheBackendError
+from django.core.cache.backends.base import InvalidCacheBackendError, CacheKeyWarning
 from django.utils import importlib
 
 # Name for use in settings file --> name of module in "backends" directory.

django/core/cache/backends/base.py

@@ -1,10 +1,18 @@
 "Base Cache class."
 
-from django.core.exceptions import ImproperlyConfigured
+import warnings
+
+from django.core.exceptions import ImproperlyConfigured, DjangoRuntimeWarning
 
 class InvalidCacheBackendError(ImproperlyConfigured):
     pass
 
+class CacheKeyWarning(DjangoRuntimeWarning):
+    pass
+
+# Memcached does not accept keys longer than this.
+MEMCACHE_MAX_KEY_LENGTH = 250
+
 class BaseCache(object):
     def __init__(self, params):
         timeout = params.get('timeout', 300)
@@ -116,3 +124,21 @@ class BaseCache(object):
     def clear(self):
         """Remove *all* values from the cache at once."""
         raise NotImplementedError
+
+    def validate_key(self, key):
+        """
+        Warn about keys that would not be portable to the memcached
+        backend. This encourages (but does not force) writing backend-portable
+        cache code.
+
+        """
+        if len(key) > MEMCACHE_MAX_KEY_LENGTH:
+            warnings.warn('Cache key will cause errors if used with memcached: '
+                    '%s (longer than %s)' % (key, MEMCACHE_MAX_KEY_LENGTH),
+                    CacheKeyWarning)
+        for char in key:
+            if ord(char) < 33 or ord(char) == 127:
+                warnings.warn('Cache key contains characters that will cause '
+                        'errors if used with memcached: %r' % key,
+                              CacheKeyWarning)
+

django/core/cache/backends/db.py

@@ -46,6 +46,7 @@ class CacheClass(BaseCache):
             self._cull_frequency = 3
 
     def get(self, key, default=None):
+        self.validate_key(key)
         db = router.db_for_read(self.cache_model_class)
         table = connections[db].ops.quote_name(self._table)
         cursor = connections[db].cursor()
@@ -65,9 +66,11 @@ class CacheClass(BaseCache):
         return pickle.loads(base64.decodestring(value))
 
     def set(self, key, value, timeout=None):
+        self.validate_key(key)
         self._base_set('set', key, value, timeout)
 
     def add(self, key, value, timeout=None):
+        self.validate_key(key)
         return self._base_set('add', key, value, timeout)
 
     def _base_set(self, mode, key, value, timeout=None):
@@ -103,6 +106,7 @@ class CacheClass(BaseCache):
             return True
 
     def delete(self, key):
+        self.validate_key(key)
         db = router.db_for_write(self.cache_model_class)
         table = connections[db].ops.quote_name(self._table)
         cursor = connections[db].cursor()
@@ -111,6 +115,7 @@ class CacheClass(BaseCache):
         transaction.commit_unless_managed(using=db)
 
     def has_key(self, key):
+        self.validate_key(key)
         db = router.db_for_read(self.cache_model_class)
         table = connections[db].ops.quote_name(self._table)
         cursor = connections[db].cursor()

django/core/cache/backends/dummy.py

@@ -6,22 +6,25 @@ class CacheClass(BaseCache):
     def __init__(self, *args, **kwargs):
         pass
 
-    def add(self, *args, **kwargs):
+    def add(self, key, *args, **kwargs):
+        self.validate_key(key)
         return True
 
     def get(self, key, default=None):
+        self.validate_key(key)
         return default
 
-    def set(self, *args, **kwargs):
+    def set(self, key, *args, **kwargs):
-        pass
+        self.validate_key(key)
 
-    def delete(self, *args, **kwargs):
+    def delete(self, key, *args, **kwargs):
-        pass
+        self.validate_key(key)
 
     def get_many(self, *args, **kwargs):
         return {}
 
-    def has_key(self, *args, **kwargs):
+    def has_key(self, key, *args, **kwargs):
+        self.validate_key(key)
         return False
 
     def set_many(self, *args, **kwargs):

django/core/cache/backends/filebased.py

@@ -32,6 +32,7 @@ class CacheClass(BaseCache):
             self._createdir()
 
     def add(self, key, value, timeout=None):
+        self.validate_key(key)
         if self.has_key(key):
             return False
 
@@ -39,6 +40,7 @@ class CacheClass(BaseCache):
         return True
 
     def get(self, key, default=None):
+        self.validate_key(key)
         fname = self._key_to_file(key)
         try:
             f = open(fname, 'rb')
@@ -56,6 +58,7 @@ class CacheClass(BaseCache):
         return default
 
     def set(self, key, value, timeout=None):
+        self.validate_key(key)
         fname = self._key_to_file(key)
         dirname = os.path.dirname(fname)
 
@@ -79,6 +82,7 @@ class CacheClass(BaseCache):
             pass
 
     def delete(self, key):
+        self.validate_key(key)
         try:
             self._delete(self._key_to_file(key))
         except (IOError, OSError):
@@ -95,6 +99,7 @@ class CacheClass(BaseCache):
             pass
 
     def has_key(self, key):
+        self.validate_key(key)
         fname = self._key_to_file(key)
         try:
             f = open(fname, 'rb')

django/core/cache/backends/locmem.py

@@ -30,6 +30,7 @@ class CacheClass(BaseCache):
         self._lock = RWLock()
 
     def add(self, key, value, timeout=None):
+        self.validate_key(key)
         self._lock.writer_enters()
         try:
             exp = self._expire_info.get(key)
@@ -44,6 +45,7 @@ class CacheClass(BaseCache):
             self._lock.writer_leaves()
 
     def get(self, key, default=None):
+        self.validate_key(key)
         self._lock.reader_enters()
         try:
             exp = self._expire_info.get(key)
@@ -76,6 +78,7 @@ class CacheClass(BaseCache):
         self._expire_info[key] = time.time() + timeout
 
     def set(self, key, value, timeout=None):
+        self.validate_key(key)
         self._lock.writer_enters()
         # Python 2.4 doesn't allow combined try-except-finally blocks.
         try:
@@ -87,6 +90,7 @@ class CacheClass(BaseCache):
             self._lock.writer_leaves()
 
     def has_key(self, key):
+        self.validate_key(key)
         self._lock.reader_enters()
         try:
             exp = self._expire_info.get(key)
@@ -127,6 +131,7 @@ class CacheClass(BaseCache):
             pass
 
     def delete(self, key):
+        self.validate_key(key)
         self._lock.writer_enters()
         try:
             self._delete(key)

django/core/exceptions.py

@@ -1,4 +1,9 @@
-"Global Django exceptions"
+"""
+Global Django exception and warning classes.
+"""
+
+class DjangoRuntimeWarning(RuntimeWarning):
+   pass
 
 class ObjectDoesNotExist(Exception):
     "The requested object does not exist"

docs/topics/cache.txt

@@ -641,6 +641,45 @@ nonexistent cache key.::
     However, if the backend doesn't natively provide an increment/decrement
     operation, it will be implemented using a two-step retrieve/update.
 
+Cache key warnings
+------------------
+
+.. versionadded:: 1.3
+
+Memcached, the most commonly-used production cache backend, does not allow
+cache keys longer than 250 characters or containing whitespace or control
+characters, and using such keys will cause an exception. To encourage
+cache-portable code and minimize unpleasant surprises, the other built-in cache
+backends issue a warning (``django.core.cache.backends.base.CacheKeyWarning``)
+if a key is used that would cause an error on memcached.
+
+If you are using a production backend that can accept a wider range of keys (a
+custom backend, or one of the non-memcached built-in backends), and want to use
+this wider range without warnings, you can silence ``CacheKeyWarning`` with
+this code in the ``management`` module of one of your
+:setting:`INSTALLED_APPS`::
+
+     import warnings
+
+     from django.core.cache import CacheKeyWarning
+
+     warnings.simplefilter("ignore", CacheKeyWarning)
+
+If you want to instead provide custom key validation logic for one of the
+built-in backends, you can subclass it, override just the ``validate_key``
+method, and follow the instructions for `using a custom cache backend`_. For
+instance, to do this for the ``locmem`` backend, put this code in a module::
+
+    from django.core.cache.backends.locmem import CacheClass as LocMemCacheClass
+
+    class CacheClass(LocMemCacheClass):
+        def validate_key(self, key):
+            """Custom validation, raising exceptions or warnings as needed."""
+            # ...
+
+...and use the dotted Python path to this module as the scheme portion of your
+:setting:`CACHE_BACKEND`.
+
 Upstream caches
 ===============
 

tests/regressiontests/cache/liberal_backend.py

@@ -0,0 +1,9 @@
+from django.core.cache.backends.locmem import CacheClass as LocMemCacheClass
+
+class LiberalKeyValidationMixin(object):
+    def validate_key(self, key):
+        pass
+
+class CacheClass(LiberalKeyValidationMixin, LocMemCacheClass):
+    pass
+

tests/regressiontests/cache/tests.py

@@ -8,11 +8,12 @@ import shutil
 import tempfile
 import time
 import unittest
+import warnings
 
 from django.conf import settings
 from django.core import management
 from django.core.cache import get_cache
-from django.core.cache.backends.base import InvalidCacheBackendError
+from django.core.cache.backends.base import InvalidCacheBackendError, CacheKeyWarning
 from django.http import HttpResponse, HttpRequest
 from django.middleware.cache import FetchFromCacheMiddleware, UpdateCacheMiddleware
 from django.utils import translation
@@ -366,6 +367,33 @@ class BaseCacheTests(object):
                 count = count + 1
         self.assertEqual(count, final_count)
 
+    def test_invalid_keys(self):
+        """
+        All the builtin backends (except memcached, see below) should warn on
+        keys that would be refused by memcached. This encourages portable
+        caching code without making it too difficult to use production backends
+        with more liberal key rules. Refs #6447.
+
+        """
+        # On Python 2.6+ we could use the catch_warnings context
+        # manager to test this warning nicely. Since we can't do that
+        # yet, the cleanest option is to temporarily ask for
+        # CacheKeyWarning to be raised as an exception.
+        warnings.simplefilter("error", CacheKeyWarning)
+
+        # memcached does not allow whitespace or control characters in keys
+        self.assertRaises(CacheKeyWarning, self.cache.set, 'key with spaces', 'value')
+        # memcached limits key length to 250
+        self.assertRaises(CacheKeyWarning, self.cache.set, 'a' * 251, 'value')
+
+        # The warnings module has no public API for getting the
+        # current list of warning filters, so we can't save that off
+        # and reset to the previous value, we have to globally reset
+        # it. The effect will be the same, as long as the Django test
+        # runner doesn't add any global warning filters (it currently
+        # does not).
+        warnings.resetwarnings()
+
 class DBCacheTests(unittest.TestCase, BaseCacheTests):
     def setUp(self):
         # Spaces are used in the table name to ensure quoting/escaping is working
@@ -397,6 +425,22 @@ if settings.CACHE_BACKEND.startswith('memcached://'):
         def setUp(self):
             self.cache = get_cache(settings.CACHE_BACKEND)
 
+        def test_invalid_keys(self):
+            """
+            On memcached, we don't introduce a duplicate key validation
+            step (for speed reasons), we just let the memcached API
+            library raise its own exception on bad keys. Refs #6447.
+
+            In order to be memcached-API-library agnostic, we only assert
+            that a generic exception of some kind is raised.
+
+            """
+            # memcached does not allow whitespace or control characters in keys
+            self.assertRaises(Exception, self.cache.set, 'key with spaces', 'value')
+            # memcached limits key length to 250
+            self.assertRaises(Exception, self.cache.set, 'a' * 251, 'value')
+
+
 class FileBasedCacheTests(unittest.TestCase, BaseCacheTests):
     """
     Specific test cases for the file-based cache.
@@ -429,6 +473,22 @@ class FileBasedCacheTests(unittest.TestCase, BaseCacheTests):
     def test_cull(self):
         self.perform_cull_test(50, 28)
 
+class CustomCacheKeyValidationTests(unittest.TestCase):
+    """
+    Tests for the ability to mixin a custom ``validate_key`` method to
+    a custom cache backend that otherwise inherits from a builtin
+    backend, and override the default key validation. Refs #6447.
+
+    """
+    def test_custom_key_validation(self):
+        cache = get_cache('regressiontests.cache.liberal_backend://')
+
+        # this key is both longer than 250 characters, and has spaces
+        key = 'some key with spaces' * 15
+        val = 'a value'
+        cache.set(key, val)
+        self.assertEqual(cache.get(key), val)
+
 class CacheUtils(unittest.TestCase):
     """TestCase for django.utils.cache functions."""