diff --git a/django/core/cache/__init__.py b/django/core/cache/__init__.py index 1b602908cb..680f724f94 100644 --- a/django/core/cache/__init__.py +++ b/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. diff --git a/django/core/cache/backends/base.py b/django/core/cache/backends/base.py index e58267a2e9..83dd461804 100644 --- a/django/core/cache/backends/base.py +++ b/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) + diff --git a/django/core/cache/backends/db.py b/django/core/cache/backends/db.py index 833fa749aa..c4429c80b3 100644 --- a/django/core/cache/backends/db.py +++ b/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() diff --git a/django/core/cache/backends/dummy.py b/django/core/cache/backends/dummy.py index 4337484cb1..f73b7408bc 100644 --- a/django/core/cache/backends/dummy.py +++ b/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): diff --git a/django/core/cache/backends/filebased.py b/django/core/cache/backends/filebased.py index 6057f11ef7..46e69f3091 100644 --- a/django/core/cache/backends/filebased.py +++ b/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') diff --git a/django/core/cache/backends/locmem.py b/django/core/cache/backends/locmem.py index eff1201b97..fe33d33307 100644 --- a/django/core/cache/backends/locmem.py +++ b/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) diff --git a/django/core/exceptions.py b/django/core/exceptions.py index ee6d5fe37b..21be8702fa 100644 --- a/django/core/exceptions.py +++ b/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" diff --git a/docs/topics/cache.txt b/docs/topics/cache.txt index e5ca526494..5797199411 100644 --- a/docs/topics/cache.txt +++ b/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 =============== diff --git a/tests/regressiontests/cache/liberal_backend.py b/tests/regressiontests/cache/liberal_backend.py new file mode 100644 index 0000000000..5c7e312690 --- /dev/null +++ b/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 + diff --git a/tests/regressiontests/cache/tests.py b/tests/regressiontests/cache/tests.py index 0f9f5c9fff..1e0a4046bb 100644 --- a/tests/regressiontests/cache/tests.py +++ b/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."""