Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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

git-svn-id: http://code.djangoproject.com/svn/django/branches/releases/1.2.X@13767 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
commit 686d6a414ca1ed30ff529616e0364427ea3678bc 1 parent 6bc74f0
@malcolmt malcolmt authored
View
2  django/core/cache/__init__.py
@@ -18,7 +18,7 @@
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.
View
28 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 @@ def delete_many(self, keys):
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)
+
View
5 django/core/cache/backends/db.py
@@ -46,6 +46,7 @@ class CacheEntry(object):
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 @@ def get(self, key, default=None):
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 @@ def _base_set(self, mode, key, value, timeout=None):
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 @@ def delete(self, key):
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()
View
15 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):
- pass
+ def set(self, key, *args, **kwargs):
+ self.validate_key(key)
- def delete(self, *args, **kwargs):
- pass
+ def delete(self, key, *args, **kwargs):
+ 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):
View
5 django/core/cache/backends/filebased.py
@@ -32,6 +32,7 @@ def __init__(self, dir, params):
self._createdir()
def add(self, key, value, timeout=None):
+ self.validate_key(key)
if self.has_key(key):
return False
@@ -39,6 +40,7 @@ def add(self, key, value, timeout=None):
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 @@ def get(self, key, default=None):
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 @@ def set(self, key, value, timeout=None):
pass
def delete(self, key):
+ self.validate_key(key)
try:
self._delete(self._key_to_file(key))
except (IOError, OSError):
@@ -95,6 +99,7 @@ def _delete(self, fname):
pass
def has_key(self, key):
+ self.validate_key(key)
fname = self._key_to_file(key)
try:
f = open(fname, 'rb')
View
5 django/core/cache/backends/locmem.py
@@ -30,6 +30,7 @@ def __init__(self, _, params):
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 @@ def add(self, key, value, timeout=None):
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 @@ def _set(self, key, value, timeout=None):
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 @@ def set(self, key, value, timeout=None):
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 @@ def _delete(self, key):
pass
def delete(self, key):
+ self.validate_key(key)
self._lock.writer_enters()
try:
self._delete(key)
View
7 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"
View
39 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
===============
View
9 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
+
View
62 tests/regressiontests/cache/tests.py
@@ -8,11 +8,12 @@
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 @@ def perform_cull_test(self, initial_count, final_count):
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 @@ class MemcachedCacheTests(unittest.TestCase, BaseCacheTests):
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 @@ def test_subdirectory_removal(self):
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."""

0 comments on commit 686d6a4

Please sign in to comment.
Something went wrong with that request. Please try again.