Code

Ticket #6447: ticket_6447_warnings.diff

File ticket_6447_warnings.diff, 13.5 KB (added by carljm, 4 years ago)

updated patch using warnings

Line 
1diff --git a/django/core/cache/__init__.py b/django/core/cache/__init__.py
2--- a/django/core/cache/__init__.py
3+++ b/django/core/cache/__init__.py
4@@ -18,7 +18,7 @@
5 from cgi import parse_qsl
6 from django.conf import settings
7 from django.core import signals
8-from django.core.cache.backends.base import InvalidCacheBackendError
9+from django.core.cache.backends.base import InvalidCacheBackendError, CacheKeyWarning
10 from django.utils import importlib
11 
12 # Name for use in settings file --> name of module in "backends" directory.
13diff --git a/django/core/cache/backends/base.py b/django/core/cache/backends/base.py
14--- a/django/core/cache/backends/base.py
15+++ b/django/core/cache/backends/base.py
16@@ -1,10 +1,19 @@
17 "Base Cache class."
18 
19+import warnings
20+
21 from django.core.exceptions import ImproperlyConfigured
22+from django.core.warnings import DjangoRuntimeWarning
23 
24 class InvalidCacheBackendError(ImproperlyConfigured):
25     pass
26 
27+class CacheKeyWarning(DjangoRuntimeWarning):
28+    pass
29+
30+# memcached does not accept keys longer than 250 characters
31+MAX_KEY_LENGTH = 250
32+
33 class BaseCache(object):
34     def __init__(self, params):
35         timeout = params.get('timeout', 300)
36@@ -116,3 +125,22 @@
37     def clear(self):
38         """Remove *all* values from the cache at once."""
39         raise NotImplementedError
40+
41+    def validate_key(self, key):
42+        """
43+        Warn about keys that would not be portable to the memcached
44+        backend. This encourages (but does not force) writing backend-portable
45+        cache code.
46+
47+        """
48+        if len(key) > MAX_KEY_LENGTH:
49+            warnings.warn('Cache key is too long for memcached, and '
50+                          'will cause errors if used with memcached backend.'
51+                          ' (Max length %s)' % MAX_KEY_LENGTH,
52+                          CacheKeyWarning)
53+        for char in key:
54+            if ord(char) < 33 or ord(char) == 127:
55+                warnings.warn('Cache key contains control character, and '
56+                              'will cause errors if used with memcached backend.',
57+                              CacheKeyWarning)
58+
59diff --git a/django/core/cache/backends/db.py b/django/core/cache/backends/db.py
60--- a/django/core/cache/backends/db.py
61+++ b/django/core/cache/backends/db.py
62@@ -46,6 +46,7 @@
63             self._cull_frequency = 3
64 
65     def get(self, key, default=None):
66+        self.validate_key(key)
67         db = router.db_for_read(self.cache_model_class)
68         table = connections[db].ops.quote_name(self._table)
69         cursor = connections[db].cursor()
70@@ -65,9 +66,11 @@
71         return pickle.loads(base64.decodestring(value))
72 
73     def set(self, key, value, timeout=None):
74+        self.validate_key(key)
75         self._base_set('set', key, value, timeout)
76 
77     def add(self, key, value, timeout=None):
78+        self.validate_key(key)
79         return self._base_set('add', key, value, timeout)
80 
81     def _base_set(self, mode, key, value, timeout=None):
82@@ -103,6 +106,7 @@
83             return True
84 
85     def delete(self, key):
86+        self.validate_key(key)
87         db = router.db_for_write(self.cache_model_class)
88         table = connections[db].ops.quote_name(self._table)
89         cursor = connections[db].cursor()
90@@ -111,6 +115,7 @@
91         transaction.commit_unless_managed(using=db)
92 
93     def has_key(self, key):
94+        self.validate_key(key)
95         db = router.db_for_read(self.cache_model_class)
96         table = connections[db].ops.quote_name(self._table)
97         cursor = connections[db].cursor()
98diff --git a/django/core/cache/backends/dummy.py b/django/core/cache/backends/dummy.py
99--- a/django/core/cache/backends/dummy.py
100+++ b/django/core/cache/backends/dummy.py
101@@ -6,22 +6,25 @@
102     def __init__(self, *args, **kwargs):
103         pass
104 
105-    def add(self, *args, **kwargs):
106+    def add(self, key, *args, **kwargs):
107+        self.validate_key(key)
108         return True
109 
110     def get(self, key, default=None):
111+        self.validate_key(key)
112         return default
113 
114-    def set(self, *args, **kwargs):
115-        pass
116+    def set(self, key, *args, **kwargs):
117+        self.validate_key(key)
118 
119-    def delete(self, *args, **kwargs):
120-        pass
121+    def delete(self, key, *args, **kwargs):
122+        self.validate_key(key)
123 
124     def get_many(self, *args, **kwargs):
125         return {}
126 
127-    def has_key(self, *args, **kwargs):
128+    def has_key(self, key, *args, **kwargs):
129+        self.validate_key(key)
130         return False
131 
132     def set_many(self, *args, **kwargs):
133diff --git a/django/core/cache/backends/filebased.py b/django/core/cache/backends/filebased.py
134--- a/django/core/cache/backends/filebased.py
135+++ b/django/core/cache/backends/filebased.py
136@@ -32,6 +32,7 @@
137             self._createdir()
138 
139     def add(self, key, value, timeout=None):
140+        self.validate_key(key)
141         if self.has_key(key):
142             return False
143 
144@@ -39,6 +40,7 @@
145         return True
146 
147     def get(self, key, default=None):
148+        self.validate_key(key)
149         fname = self._key_to_file(key)
150         try:
151             f = open(fname, 'rb')
152@@ -56,6 +58,7 @@
153         return default
154 
155     def set(self, key, value, timeout=None):
156+        self.validate_key(key)
157         fname = self._key_to_file(key)
158         dirname = os.path.dirname(fname)
159 
160@@ -79,6 +82,7 @@
161             pass
162 
163     def delete(self, key):
164+        self.validate_key(key)
165         try:
166             self._delete(self._key_to_file(key))
167         except (IOError, OSError):
168@@ -95,6 +99,7 @@
169             pass
170 
171     def has_key(self, key):
172+        self.validate_key(key)
173         fname = self._key_to_file(key)
174         try:
175             f = open(fname, 'rb')
176diff --git a/django/core/cache/backends/locmem.py b/django/core/cache/backends/locmem.py
177--- a/django/core/cache/backends/locmem.py
178+++ b/django/core/cache/backends/locmem.py
179@@ -30,6 +30,7 @@
180         self._lock = RWLock()
181 
182     def add(self, key, value, timeout=None):
183+        self.validate_key(key)
184         self._lock.writer_enters()
185         try:
186             exp = self._expire_info.get(key)
187@@ -44,6 +45,7 @@
188             self._lock.writer_leaves()
189 
190     def get(self, key, default=None):
191+        self.validate_key(key)
192         self._lock.reader_enters()
193         try:
194             exp = self._expire_info.get(key)
195@@ -76,6 +78,7 @@
196         self._expire_info[key] = time.time() + timeout
197 
198     def set(self, key, value, timeout=None):
199+        self.validate_key(key)
200         self._lock.writer_enters()
201         # Python 2.4 doesn't allow combined try-except-finally blocks.
202         try:
203@@ -87,6 +90,7 @@
204             self._lock.writer_leaves()
205 
206     def has_key(self, key):
207+        self.validate_key(key)
208         self._lock.reader_enters()
209         try:
210             exp = self._expire_info.get(key)
211@@ -127,6 +131,7 @@
212             pass
213 
214     def delete(self, key):
215+        self.validate_key(key)
216         self._lock.writer_enters()
217         try:
218             self._delete(key)
219diff --git a/django/core/warnings.py b/django/core/warnings.py
220new file mode 100644
221--- /dev/null
222+++ b/django/core/warnings.py
223@@ -0,0 +1,4 @@
224+"Global Django warnings"
225+
226+class DjangoRuntimeWarning(RuntimeWarning):
227+    pass
228diff --git a/docs/topics/cache.txt b/docs/topics/cache.txt
229--- a/docs/topics/cache.txt
230+++ b/docs/topics/cache.txt
231@@ -641,6 +641,45 @@
232     However, if the backend doesn't natively provide an increment/decrement
233     operation, it will be implemented using a two-step retrieve/update.
234 
235+Cache key warnings
236+------------------
237+
238+.. versionadded:: 1.3
239+
240+Memcached, the most commonly-used production cache backend, does not allow
241+cache keys longer than 250 characters or containing whitespace or control
242+characters, and using such keys will cause an exception. To encourage
243+cache-portable code and minimize unpleasant surprises, the other built-in cache
244+backends issue a warning (``django.core.cache.backends.base.CacheKeyWarning``)
245+if a key is used that would cause an error on memcached.
246+
247+If you are using a production backend that can accept a wider range of keys (a
248+custom backend, or one of the non-memcached built-in backends), and want to use
249+this wider range without warnings, you can silence ``CacheKeyWarning`` with
250+this code in the ``management`` module of one of your
251+:setting:`INSTALLED_APPS`::
252+
253+     import warnings
254+
255+     from django.core.cache import CacheKeyWarning
256+
257+     warnings.simplefilter("ignore", CacheKeyWarning)
258+
259+If you want to instead provide custom key validation logic for one of the
260+built-in backends, you can subclass it, override just the ``validate_key``
261+method, and follow the instructions for `using a custom cache backend`_. For
262+instance, to do this for the ``locmem`` backend, put this code in a module::
263+
264+    from django.core.cache.backends.locmem import CacheClass as LocMemCacheClass
265+
266+    class CacheClass(LiberalKeyValidationMixin, LocMemCacheClass):
267+        def validate_key(self, key):
268+            # do custom validation, raising an exception or a warning,
269+            # depending on your preference
270+
271+And use the dotted Python path to this module as the scheme portion of your
272+:setting:`CACHE_BACKEND`.
273+
274 Upstream caches
275 ===============
276 
277diff --git a/tests/regressiontests/cache/liberal_backend.py b/tests/regressiontests/cache/liberal_backend.py
278new file mode 100644
279--- /dev/null
280+++ b/tests/regressiontests/cache/liberal_backend.py
281@@ -0,0 +1,9 @@
282+from django.core.cache.backends.locmem import CacheClass as LocMemCacheClass
283+
284+class LiberalKeyValidationMixin(object):
285+    def validate_key(self, key):
286+        pass
287+
288+class CacheClass(LiberalKeyValidationMixin, LocMemCacheClass):
289+    pass
290+
291diff --git a/tests/regressiontests/cache/tests.py b/tests/regressiontests/cache/tests.py
292--- a/tests/regressiontests/cache/tests.py
293+++ b/tests/regressiontests/cache/tests.py
294@@ -8,11 +8,12 @@
295 import tempfile
296 import time
297 import unittest
298+import warnings
299 
300 from django.conf import settings
301 from django.core import management
302 from django.core.cache import get_cache
303-from django.core.cache.backends.base import InvalidCacheBackendError
304+from django.core.cache.backends.base import InvalidCacheBackendError, CacheKeyWarning
305 from django.http import HttpResponse, HttpRequest
306 from django.middleware.cache import FetchFromCacheMiddleware, UpdateCacheMiddleware
307 from django.utils import translation
308@@ -366,6 +367,33 @@
309                 count = count + 1
310         self.assertEqual(count, final_count)
311 
312+    def test_invalid_keys(self):
313+        """
314+        All the builtin backends (except memcached, see below) should warn on
315+        keys that would be refused by memcached. This encourages portable
316+        caching code without making it too difficult to use production backends
317+        with more liberal key rules. Refs #6447.
318+
319+        """
320+        # On Python 2.6+ we could use the catch_warnings context
321+        # manager to test this warning nicely. Since we can't do that
322+        # yet, the cleanest option is to temporarily ask for
323+        # CacheKeyWarning to be raised as an exception.
324+        warnings.simplefilter("error", CacheKeyWarning)
325+
326+        # memcached does not allow whitespace or control characters in keys
327+        self.assertRaises(CacheKeyWarning, self.cache.set, 'key with spaces', 'value')
328+        # memcached limits key length to 250
329+        self.assertRaises(CacheKeyWarning, self.cache.set, 'a' * 251, 'value')
330+
331+        # The warnings module has no public API for getting the
332+        # current list of warning filters, so we can't save that off
333+        # and reset to the previous value, we have to globally reset
334+        # it. The effect will be the same, as long as the Django test
335+        # runner doesn't add any global warning filters (it currently
336+        # does not).
337+        warnings.resetwarnings()
338+
339 class DBCacheTests(unittest.TestCase, BaseCacheTests):
340     def setUp(self):
341         # Spaces are used in the table name to ensure quoting/escaping is working
342@@ -397,6 +425,22 @@
343         def setUp(self):
344             self.cache = get_cache(settings.CACHE_BACKEND)
345 
346+        def test_invalid_keys(self):
347+            """
348+            On memcached, we don't introduce a duplicate key validation
349+            step (for speed reasons), we just let the memcached API
350+            library raise its own exception on bad keys. Refs #6447.
351+
352+            In order to be memcached-API-library agnostic, we only assert
353+            that a generic exception of some kind is raised.
354+
355+            """
356+            # memcached does not allow whitespace or control characters in keys
357+            self.assertRaises(Exception, self.cache.set, 'key with spaces', 'value')
358+            # memcached limits key length to 250
359+            self.assertRaises(Exception, self.cache.set, 'a' * 251, 'value')
360+
361+
362 class FileBasedCacheTests(unittest.TestCase, BaseCacheTests):
363     """
364     Specific test cases for the file-based cache.
365@@ -429,6 +473,22 @@
366     def test_cull(self):
367         self.perform_cull_test(50, 28)
368 
369+class CustomCacheKeyValidationTests(unittest.TestCase):
370+    """
371+    Tests for the ability to mixin a custom ``validate_key`` method to
372+    a custom cache backend that otherwise inherits from a builtin
373+    backend, and override the default key validation. Refs #6447.
374+
375+    """
376+    def test_custom_key_validation(self):
377+        cache = get_cache('regressiontests.cache.liberal_backend://')
378+
379+        # this key is both longer than 250 characters, and has spaces
380+        key = 'some key with spaces' * 15
381+        val = 'a value'
382+        cache.set(key, val)
383+        self.assertEqual(cache.get(key), val)
384+
385 class CacheUtils(unittest.TestCase):
386     """TestCase for django.utils.cache functions."""
387