From 1b0b365fa6816b32b3f625709bcb21b8a5f6235f Mon Sep 17 00:00:00 2001 From: Will Daly Date: Mon, 6 May 2013 17:08:46 -0400 Subject: [PATCH 1/2] Added unit tests for safe_key() to resolve bug 392. Updated safe_key() so that it: (a) avoids creating keys that are too long for memcache, and (b) handles unicode in keys, prefixes, and versions Added __init__.py, which should have been in the last commit Pep8/Pylint fixes --- common/djangoapps/util/memcache.py | 43 +++++-- common/djangoapps/util/tests/__init__.py | 1 + common/djangoapps/util/tests/test_memcache.py | 114 ++++++++++++++++++ .../util/{tests.py => tests/test_zendesk.py} | 2 +- 4 files changed, 152 insertions(+), 8 deletions(-) create mode 100644 common/djangoapps/util/tests/__init__.py create mode 100644 common/djangoapps/util/tests/test_memcache.py rename common/djangoapps/util/{tests.py => tests/test_zendesk.py} (99%) diff --git a/common/djangoapps/util/memcache.py b/common/djangoapps/util/memcache.py index 540cf96539..db921d9845 100644 --- a/common/djangoapps/util/memcache.py +++ b/common/djangoapps/util/memcache.py @@ -8,15 +8,44 @@ 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 the key + # and combine the parts again + if len(combined) > 250: + key = fasthash(key) + combined = ":".join([key_prefix, version, key]) + + # Return the result + return combined diff --git a/common/djangoapps/util/tests/__init__.py b/common/djangoapps/util/tests/__init__.py new file mode 100644 index 0000000000..8b13789179 --- /dev/null +++ b/common/djangoapps/util/tests/__init__.py @@ -0,0 +1 @@ + diff --git a/common/djangoapps/util/tests/test_memcache.py b/common/djangoapps/util/tests/test_memcache.py new file mode 100644 index 0000000000..85b60c75f1 --- /dev/null +++ b/common/djangoapps/util/tests/test_memcache.py @@ -0,0 +1,114 @@ +""" +Tests for memcache in util app +""" + +from django.test import TestCase +from django.core.cache import get_cache +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): + + key = safe_key('a' * 300, 'prefix', 'version') + self.assertEqual(key[0:15], 'prefix:version:') + + 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 diff --git a/common/djangoapps/util/tests.py b/common/djangoapps/util/tests/test_zendesk.py similarity index 99% rename from common/djangoapps/util/tests.py rename to common/djangoapps/util/tests/test_zendesk.py index d829676eaf..51d06a92ed 100644 --- a/common/djangoapps/util/tests.py +++ b/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 From aaa383b8ca714b0f84db959339fe9a1496e76d78 Mon Sep 17 00:00:00 2001 From: Will Daly Date: Tue, 7 May 2013 15:48:51 -0400 Subject: [PATCH 2/2] safe_key() now hashes the prefix/version as well, just in case these are configured to be too long in the settings. --- common/djangoapps/util/memcache.py | 6 ++---- common/djangoapps/util/tests/test_memcache.py | 12 +++++++++++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/common/djangoapps/util/memcache.py b/common/djangoapps/util/memcache.py index db921d9845..ee450e68cb 100644 --- a/common/djangoapps/util/memcache.py +++ b/common/djangoapps/util/memcache.py @@ -41,11 +41,9 @@ def safe_key(key, key_prefix, 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 the key - # and combine the parts again + # If the total length is too long for memcache, hash it if len(combined) > 250: - key = fasthash(key) - combined = ":".join([key_prefix, version, key]) + combined = fasthash(combined) # Return the result return combined diff --git a/common/djangoapps/util/tests/test_memcache.py b/common/djangoapps/util/tests/test_memcache.py index 85b60c75f1..de8d352c38 100644 --- a/common/djangoapps/util/tests/test_memcache.py +++ b/common/djangoapps/util/tests/test_memcache.py @@ -4,6 +4,7 @@ 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 @@ -51,8 +52,17 @@ class MemcacheTest(TestCase): def test_long_key_prefix_version(self): + # Long key key = safe_key('a' * 300, 'prefix', 'version') - self.assertEqual(key[0:15], '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):