Merge pull request #1957 from edx/fix/will/bug_392

Added unit tests for safe_key() to resolve bug 392.

common/djangoapps/util/memcache.py

@@ -8,15 +8,42 @@ import urllib
 
 
 def fasthash(string):
-    m = hashlib.new("md4")
-    m.update(string)
-    return m.hexdigest()
+    """
+    Hashes `string` into a string representation of a 128-bit digest.
+    """
+    md4 = hashlib.new("md4")
+    md4.update(string)
+    return md4.hexdigest()
+
+
+def cleaned_string(val):
+    """
+    Converts `val` to unicode and URL-encodes special characters
+    (including quotes and spaces)
+    """
+    return urllib.quote_plus(smart_str(val))
 
 
 def safe_key(key, key_prefix, version):
-    safe_key = urllib.quote_plus(smart_str(key))
+    """
+    Given a `key`, `key_prefix`, and `version`,
+    return a key that is safe to use with memcache.
 
-    if len(safe_key) > 250:
-        safe_key = fasthash(safe_key)
+    `key`, `key_prefix`, and `version` can be numbers, strings, or unicode.
+    """
 
-    return ":".join([key_prefix, str(version), safe_key])
+    # Clean for whitespace and control characters, which
+    # cause memcache to raise an exception
+    key = cleaned_string(key)
+    key_prefix = cleaned_string(key_prefix)
+    version = cleaned_string(version)
+
+    # Attempt to combine the prefix, version, and key
+    combined = ":".join([key_prefix, version, key])
+
+    # If the total length is too long for memcache, hash it
+    if len(combined) > 250:
+        combined = fasthash(combined)
+
+    # Return the result
+    return combined

common/djangoapps/util/tests/__init__.py

@@ -0,0 +1 @@
+

common/djangoapps/util/tests/test_memcache.py

@@ -0,0 +1,124 @@
+"""
+Tests for memcache in util app
+"""
+
+from django.test import TestCase
+from django.core.cache import get_cache
+from django.conf import settings
+from util.memcache import safe_key
+
+
+class MemcacheTest(TestCase):
+    """
+    Test memcache key cleanup
+    """
+
+    # Test whitespace, control characters, and some non-ASCII UTF-16
+    UNICODE_CHAR_CODES = ([c for c in range(0, 30)] + [127] +
+                          [129, 500, 2 ** 8 - 1, 2 ** 8 + 1, 2 ** 16 - 1])
+
+    def setUp(self):
+        self.cache = get_cache('default')
+
+    def test_safe_key(self):
+        key = safe_key('test', 'prefix', 'version')
+        self.assertEqual(key, 'prefix:version:test')
+
+    def test_numeric_inputs(self):
+
+        # Numeric key
+        self.assertEqual(safe_key(1, 'prefix', 'version'), 'prefix:version:1')
+
+        # Numeric prefix
+        self.assertEqual(safe_key('test', 5, 'version'), '5:version:test')
+
+        # Numeric version
+        self.assertEqual(safe_key('test', 'prefix', 5), 'prefix:5:test')
+
+    def test_safe_key_long(self):
+
+        # Choose lengths close to memcached's cutoff (250)
+        for length in [248, 249, 250, 251, 252]:
+
+            # Generate a key of that length
+            key = 'a' * length
+
+            # Make the key safe
+            key = safe_key(key, '', '')
+
+            # The key should now be valid
+            self.assertTrue(self._is_valid_key(key),
+                            msg="Failed for key length {0}".format(length))
+
+    def test_long_key_prefix_version(self):
+
+        # Long key
+        key = safe_key('a' * 300, 'prefix', 'version')
+        self.assertTrue(self._is_valid_key(key))
+
+        # Long prefix
+        key = safe_key('key', 'a' * 300, 'version')
+        self.assertTrue(self._is_valid_key(key))
+
+        # Long version
+        key = safe_key('key', 'prefix', 'a' * 300)
+        self.assertTrue(self._is_valid_key(key))
+
+    def test_safe_key_unicode(self):
+
+        for unicode_char in self.UNICODE_CHAR_CODES:
+
+            # Generate a key with that character
+            key = unichr(unicode_char)
+
+            # Make the key safe
+            key = safe_key(key, '', '')
+
+            # The key should now be valid
+            self.assertTrue(self._is_valid_key(key),
+                            msg="Failed for unicode character {0}".format(unicode_char))
+
+    def test_safe_key_prefix_unicode(self):
+
+        for unicode_char in self.UNICODE_CHAR_CODES:
+
+            # Generate a prefix with that character
+            prefix = unichr(unicode_char)
+
+            # Make the key safe
+            key = safe_key('test', prefix, '')
+
+            # The key should now be valid
+            self.assertTrue(self._is_valid_key(key),
+                            msg="Failed for unicode character {0}".format(unicode_char))
+
+    def test_safe_key_version_unicode(self):
+
+        for unicode_char in self.UNICODE_CHAR_CODES:
+
+            # Generate a version with that character
+            version = unichr(unicode_char)
+
+            # Make the key safe
+            key = safe_key('test', '', version)
+
+            # The key should now be valid
+            self.assertTrue(self._is_valid_key(key),
+                            msg="Failed for unicode character {0}".format(unicode_char))
+
+    def _is_valid_key(self, key):
+        """
+        Test that a key is memcache-compatible.
+        Based on Django's validator in core.cache.backends.base
+        """
+
+        # Check the length
+        if len(key) > 250:
+            return False
+
+        # Check that there are no spaces or control characters
+        for char in key:
+            if ord(char) < 33 or ord(char) == 127:
+                return False
+
+        return True

common/djangoapps/util/tests/test_zendesk.py

@@ -1,4 +1,4 @@
-"""Tests for the util package"""
+"""Tests for the Zendesk"""
 
 from django.conf import settings
 from django.contrib.auth.models import AnonymousUser