From 046140785771e1abe0742e05851061606a6f61da Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 15 Jan 2013 11:45:08 -0500 Subject: [PATCH 1/4] Proposed changes to fix asset caching bug. --- cms/djangoapps/contentstore/views.py | 3 ++- common/lib/xmodule/xmodule/contentstore/content.py | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index b3c28f726d..003a82647e 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -753,7 +753,8 @@ def upload_asset(request, org, course, coursename): # nomenclature since we're using a FileSystem paradigm here. We're just imposing # the Location string formatting expectations to keep things a bit more consistent - filename = request.FILES['file'].name + # unicode needed for cache equivalency + filename = unicode(request.FILES['file'].name) mime_type = request.FILES['file'].content_type filedata = request.FILES['file'].read() diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index b84aa01c54..33af46c218 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -29,12 +29,14 @@ class StaticContent(object): @staticmethod def generate_thumbnail_name(original_name): - return ('{0}'+XASSET_THUMBNAIL_TAIL_NAME).format(os.path.splitext(original_name)[0]) + # unicode needed for cache equivalency + return unicode(('{0}'+XASSET_THUMBNAIL_TAIL_NAME).format(os.path.splitext(original_name)[0])) @staticmethod def compute_location(org, course, name, revision=None, is_thumbnail=False): name = name.replace('/', '_') - return Location([XASSET_LOCATION_TAG, org, course, 'asset' if not is_thumbnail else 'thumbnail', Location.clean(name), revision]) + # unicode needed for cache equivalency + return Location([unicode(XASSET_LOCATION_TAG), org, course, u'asset' if not is_thumbnail else u'thumbnail', Location.clean(name), revision]) def get_id(self): return StaticContent.get_id_from_location(self.location) From 9818664bb1262b2b7b9f6f088567db4f8001e4d1 Mon Sep 17 00:00:00 2001 From: cahrens Date: Tue, 15 Jan 2013 16:04:34 -0500 Subject: [PATCH 2/4] storing work --- .../contentstore/tests/test_core_caching.py | 18 ++++++++++++++++++ cms/djangoapps/contentstore/views.py | 3 +-- common/djangoapps/cache_toolbox/core.py | 18 +++++++++++++++--- .../xmodule/xmodule/contentstore/content.py | 6 ++---- rakefile | 2 +- 5 files changed, 37 insertions(+), 10 deletions(-) create mode 100644 cms/djangoapps/contentstore/tests/test_core_caching.py diff --git a/cms/djangoapps/contentstore/tests/test_core_caching.py b/cms/djangoapps/contentstore/tests/test_core_caching.py new file mode 100644 index 0000000000..8abb048aff --- /dev/null +++ b/cms/djangoapps/contentstore/tests/test_core_caching.py @@ -0,0 +1,18 @@ +from django.test.testcases import TestCase +from cache_toolbox.core import get_cached_content, set_cached_content +import mock + +class CachingTestCase(TestCase): +# Tests for https://edx.lighthouseapp.com/projects/102637/tickets/112-updating-asset-does-not-refresh-the-cached-copy + def test_put_and_get(self): + mockAsset = mock.Mock() + mockLocation = mock.Mock() + mockLocation.category = u'thumbnail' + mockLocation.name = u'monsters.jpg' + mockLocation.course = u'800' + mockLocation.tag = u'c4x' + mockLocation.org = u'mitX' + mockLocation.revision = None + mockAsset.location = mockLocation + set_cached_content(mockAsset) + cachedAsset = get_cached_content(mockLocation) diff --git a/cms/djangoapps/contentstore/views.py b/cms/djangoapps/contentstore/views.py index 003a82647e..b3c28f726d 100644 --- a/cms/djangoapps/contentstore/views.py +++ b/cms/djangoapps/contentstore/views.py @@ -753,8 +753,7 @@ def upload_asset(request, org, course, coursename): # nomenclature since we're using a FileSystem paradigm here. We're just imposing # the Location string formatting expectations to keep things a bit more consistent - # unicode needed for cache equivalency - filename = unicode(request.FILES['file'].name) + filename = request.FILES['file'].name mime_type = request.FILES['file'].content_type filedata = request.FILES['file'].read() diff --git a/common/djangoapps/cache_toolbox/core.py b/common/djangoapps/cache_toolbox/core.py index caba804750..426b522e07 100644 --- a/common/djangoapps/cache_toolbox/core.py +++ b/common/djangoapps/cache_toolbox/core.py @@ -108,11 +108,23 @@ def instance_key(model, instance_or_pk): getattr(instance_or_pk, 'pk', instance_or_pk), ) +import logging + def set_cached_content(content): - cache.set(content.get_id(), content) + logging.warn("set cached---------------------------------------") + logging.warn(str(content.location)) + cache.set(str(content.location), content) def get_cached_content(location): - return cache.get(StaticContent.get_id_from_location(location)) + logging.warn("get cached------------------------") + logging.warn(str(location)) + logging.warn(StaticContent.get_id_from_location(location)) + return cache.get(str(location)) def del_cached_content(location): - cache.delete(StaticContent.get_id_from_location(location)) + if cache.get(str(location)) is None: + logging.err('nothing in cache for: ' + str(location)) + logging.warn("deleted cache----------") + logging.warn(str(location)) + logging.warn(StaticContent.get_id_from_location(location)) + cache.delete(str(location)) diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index 33af46c218..b84aa01c54 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -29,14 +29,12 @@ class StaticContent(object): @staticmethod def generate_thumbnail_name(original_name): - # unicode needed for cache equivalency - return unicode(('{0}'+XASSET_THUMBNAIL_TAIL_NAME).format(os.path.splitext(original_name)[0])) + return ('{0}'+XASSET_THUMBNAIL_TAIL_NAME).format(os.path.splitext(original_name)[0]) @staticmethod def compute_location(org, course, name, revision=None, is_thumbnail=False): name = name.replace('/', '_') - # unicode needed for cache equivalency - return Location([unicode(XASSET_LOCATION_TAG), org, course, u'asset' if not is_thumbnail else u'thumbnail', Location.clean(name), revision]) + return Location([XASSET_LOCATION_TAG, org, course, 'asset' if not is_thumbnail else 'thumbnail', Location.clean(name), revision]) def get_id(self): return StaticContent.get_id_from_location(self.location) diff --git a/rakefile b/rakefile index e984b7cc5b..e2441aec12 100644 --- a/rakefile +++ b/rakefile @@ -120,7 +120,7 @@ default_options = { } task :predjango do - sh("find . -type f -name *.pyc -delete") + sh("find . -type f -name '*.pyc' -delete") sh('pip install -q --upgrade --no-deps -r local-requirements.txt') end From 07d6bee5a7361a441665bc42775be7bc365cb61d Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 16 Jan 2013 09:53:28 -0500 Subject: [PATCH 3/4] storing work --- .../contentstore/tests/test_core_caching.py | 43 +++++++++++++------ common/djangoapps/cache_toolbox/core.py | 12 ------ 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_core_caching.py b/cms/djangoapps/contentstore/tests/test_core_caching.py index 8abb048aff..3abeaec2fd 100644 --- a/cms/djangoapps/contentstore/tests/test_core_caching.py +++ b/cms/djangoapps/contentstore/tests/test_core_caching.py @@ -1,18 +1,35 @@ from django.test.testcases import TestCase -from cache_toolbox.core import get_cached_content, set_cached_content -import mock +from cache_toolbox.core import get_cached_content, set_cached_content, del_cached_content +from xmodule.modulestore import Location +from xmodule.contentstore.content import StaticContent + +class Content: + def __init__(self, location, content): + self.location = location + self.content = content + + def get_id(self): + return StaticContent.get_id_from_location(self.location) class CachingTestCase(TestCase): # Tests for https://edx.lighthouseapp.com/projects/102637/tickets/112-updating-asset-does-not-refresh-the-cached-copy + unicodeLocation = Location(u'c4x', u'mitX', u'800', u'thumbnail', u'monsters.jpg') + # Note that some of the parts are strings instead of unicode strings + nonUnicodeLocation = Location('c4x', u'mitX', u'800', 'thumbnail', 'monsters.jpg') + mockAsset = Content(unicodeLocation, 'my content') + def test_put_and_get(self): - mockAsset = mock.Mock() - mockLocation = mock.Mock() - mockLocation.category = u'thumbnail' - mockLocation.name = u'monsters.jpg' - mockLocation.course = u'800' - mockLocation.tag = u'c4x' - mockLocation.org = u'mitX' - mockLocation.revision = None - mockAsset.location = mockLocation - set_cached_content(mockAsset) - cachedAsset = get_cached_content(mockLocation) + set_cached_content(self.mockAsset) + self.assertEqual(self.mockAsset.content, get_cached_content(self.unicodeLocation).content, + 'should be stored in cache with unicodeLocation') + self.assertEqual(self.mockAsset.content, get_cached_content(Location('c4x', u'mitX', u'800', 'thumbnail', 'monsters.jpg')).content, + 'should be stored in cache with nonUnicodeLocation') + + def test_delete(self): + set_cached_content(self.mockAsset) + del_cached_content(self.nonUnicodeLocation) + self.assertEqual(None, get_cached_content(self.nonUnicodeLocation), 'should not be stored in cache with nonUnicodeLocation') + self.assertEqual(None, get_cached_content(self.unicodeLocation), 'should not be stored in cache with unicodeLocation') + + + diff --git a/common/djangoapps/cache_toolbox/core.py b/common/djangoapps/cache_toolbox/core.py index 426b522e07..3bd184bc2c 100644 --- a/common/djangoapps/cache_toolbox/core.py +++ b/common/djangoapps/cache_toolbox/core.py @@ -108,23 +108,11 @@ def instance_key(model, instance_or_pk): getattr(instance_or_pk, 'pk', instance_or_pk), ) -import logging - def set_cached_content(content): - logging.warn("set cached---------------------------------------") - logging.warn(str(content.location)) cache.set(str(content.location), content) def get_cached_content(location): - logging.warn("get cached------------------------") - logging.warn(str(location)) - logging.warn(StaticContent.get_id_from_location(location)) return cache.get(str(location)) def del_cached_content(location): - if cache.get(str(location)) is None: - logging.err('nothing in cache for: ' + str(location)) - logging.warn("deleted cache----------") - logging.warn(str(location)) - logging.warn(StaticContent.get_id_from_location(location)) cache.delete(str(location)) From 65d45d6dee7cf0d3e6e2107fa6bc436a3f660eaf Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 16 Jan 2013 09:57:21 -0500 Subject: [PATCH 4/4] Minor cleanup. --- cms/djangoapps/contentstore/tests/test_core_caching.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_core_caching.py b/cms/djangoapps/contentstore/tests/test_core_caching.py index 3abeaec2fd..0cb4a4930c 100644 --- a/cms/djangoapps/contentstore/tests/test_core_caching.py +++ b/cms/djangoapps/contentstore/tests/test_core_caching.py @@ -22,14 +22,17 @@ class CachingTestCase(TestCase): set_cached_content(self.mockAsset) self.assertEqual(self.mockAsset.content, get_cached_content(self.unicodeLocation).content, 'should be stored in cache with unicodeLocation') - self.assertEqual(self.mockAsset.content, get_cached_content(Location('c4x', u'mitX', u'800', 'thumbnail', 'monsters.jpg')).content, + self.assertEqual(self.mockAsset.content, get_cached_content(self.nonUnicodeLocation).content, 'should be stored in cache with nonUnicodeLocation') def test_delete(self): set_cached_content(self.mockAsset) del_cached_content(self.nonUnicodeLocation) - self.assertEqual(None, get_cached_content(self.nonUnicodeLocation), 'should not be stored in cache with nonUnicodeLocation') - self.assertEqual(None, get_cached_content(self.unicodeLocation), 'should not be stored in cache with unicodeLocation') + self.assertEqual(None, get_cached_content(self.unicodeLocation), + 'should not be stored in cache with unicodeLocation') + self.assertEqual(None, get_cached_content(self.nonUnicodeLocation), + 'should not be stored in cache with nonUnicodeLocation') +