From f15533dc8fc71b22f4dd7e1d9f3a27da046e61bc Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Thu, 7 Jul 2016 11:26:27 -0400 Subject: [PATCH] [PERF-346] Add a second version component to versioned course asset URLs This version component reflects the "version" of the StaticContent objects which we cache server-side. If the layout of those objects changes between releases, errors occur when loading them from cache. By using a separate version value, which can be incremented on its own after a change has been made to the StaticContent class, we can avoid loading older cached content and in turn take advantage of these changes faster, without needing to intervene operationally. --- common/djangoapps/contentserver/__init__.py | 2 - common/djangoapps/contentserver/caching.py | 8 ++-- .../contentserver/test/test_contentserver.py | 37 ++++++++++++++++++- .../test/test_static_replace.py | 14 ++++--- .../xmodule/xmodule/contentstore/content.py | 9 +++-- 5 files changed, 54 insertions(+), 16 deletions(-) diff --git a/common/djangoapps/contentserver/__init__.py b/common/djangoapps/contentserver/__init__.py index a1038dee36..63136758e8 100644 --- a/common/djangoapps/contentserver/__init__.py +++ b/common/djangoapps/contentserver/__init__.py @@ -1,5 +1,3 @@ """ Serves course assets to end users. """ - -CONTENTSERVER_VERSION = 1 diff --git a/common/djangoapps/contentserver/caching.py b/common/djangoapps/contentserver/caching.py index 7a33032b01..ba55a38812 100644 --- a/common/djangoapps/contentserver/caching.py +++ b/common/djangoapps/contentserver/caching.py @@ -4,7 +4,7 @@ Helper functions for caching course assets. from django.core.cache import caches from django.core.cache.backends.base import InvalidCacheBackendError from opaque_keys import InvalidKeyError -from . import CONTENTSERVER_VERSION +from xmodule.contentstore.content import STATIC_CONTENT_VERSION # See if there's a "course_assets" cache configured, and if not, fallback to the default cache. CONTENT_CACHE = caches['default'] @@ -18,14 +18,14 @@ def set_cached_content(content): """ Stores the given piece of content in the cache, using its location as the key. """ - CONTENT_CACHE.set(unicode(content.location).encode("utf-8"), content, version=CONTENTSERVER_VERSION) + CONTENT_CACHE.set(unicode(content.location).encode("utf-8"), content, version=STATIC_CONTENT_VERSION) def get_cached_content(location): """ Retrieves the given piece of content by its location if cached. """ - return CONTENT_CACHE.get(unicode(location).encode("utf-8"), version=CONTENTSERVER_VERSION) + return CONTENT_CACHE.get(unicode(location).encode("utf-8"), version=STATIC_CONTENT_VERSION) def del_cached_content(location): @@ -46,4 +46,4 @@ def del_cached_content(location): # although deprecated keys allowed run=None, new keys don't if there is no version. pass - CONTENT_CACHE.delete_many(locations, version=CONTENTSERVER_VERSION) + CONTENT_CACHE.delete_many(locations, version=STATIC_CONTENT_VERSION) diff --git a/common/djangoapps/contentserver/test/test_contentserver.py b/common/djangoapps/contentserver/test/test_contentserver.py index 52bdf83d2d..10d0c7a421 100644 --- a/common/djangoapps/contentserver/test/test_contentserver.py +++ b/common/djangoapps/contentserver/test/test_contentserver.py @@ -16,7 +16,7 @@ from django.test.utils import override_settings from mock import patch from xmodule.contentstore.django import contentstore -from xmodule.contentstore.content import StaticContent +from xmodule.contentstore.content import StaticContent, VERSIONED_ASSETS_PREFIX from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.xml_importer import import_course_from_xml @@ -51,6 +51,20 @@ def get_versioned_asset_url(asset_path): return asset_path +def get_old_style_versioned_asset_url(asset_path): + """ + Creates an old-style versioned asset URL. + """ + try: + locator = StaticContent.get_location_from_path(asset_path) + content = AssetManager.find(locator, as_stream=True) + return u'{}/{}{}'.format(VERSIONED_ASSETS_PREFIX, content.content_digest, asset_path) + except (InvalidKeyError, ItemNotFoundError): + pass + + return asset_path + + @ddt.ddt @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) class ContentStoreToyCourseTest(SharedModuleStoreTestCase): @@ -76,12 +90,14 @@ class ContentStoreToyCourseTest(SharedModuleStoreTestCase): cls.locked_asset = cls.course_key.make_asset_key('asset', 'sample_static.html') cls.url_locked = unicode(cls.locked_asset) cls.url_locked_versioned = get_versioned_asset_url(cls.url_locked) + cls.url_locked_versioned_old_style = get_old_style_versioned_asset_url(cls.url_locked) cls.contentstore.set_attr(cls.locked_asset, 'locked', True) # An unlocked asset cls.unlocked_asset = cls.course_key.make_asset_key('asset', 'another_static.txt') cls.url_unlocked = unicode(cls.unlocked_asset) cls.url_unlocked_versioned = get_versioned_asset_url(cls.url_unlocked) + cls.url_unlocked_versioned_old_style = get_old_style_versioned_asset_url(cls.url_unlocked) cls.length_unlocked = cls.contentstore.get_attr(cls.unlocked_asset, 'length') def setUp(self): @@ -110,6 +126,14 @@ class ContentStoreToyCourseTest(SharedModuleStoreTestCase): resp = self.client.get(self.url_unlocked_versioned) self.assertEqual(resp.status_code, 200) + def test_unlocked_versioned_asset_old_style(self): + """ + Test that unlocked assets that are versioned (old-style) are being served. + """ + self.client.logout() + resp = self.client.get(self.url_unlocked_versioned_old_style) + self.assertEqual(resp.status_code, 200) + def test_unlocked_versioned_asset_with_nonexistent_version(self): """ Test that unlocked assets that are versioned, but have a nonexistent version, @@ -133,6 +157,17 @@ class ContentStoreToyCourseTest(SharedModuleStoreTestCase): resp = self.client.get(self.url_locked_versioned) self.assertEqual(resp.status_code, 200) + def test_locked_versioned_old_styleasset(self): + """ + Test that locked assets that are versioned (old-style) are being served. + """ + CourseEnrollment.enroll(self.non_staff_usr, self.course_key) + self.assertTrue(CourseEnrollment.is_enrolled(self.non_staff_usr, self.course_key)) + + self.client.login(username=self.non_staff_usr, password='test') + resp = self.client.get(self.url_locked_versioned_old_style) + self.assertEqual(resp.status_code, 200) + def test_locked_asset_not_logged_in(self): """ Test that locked assets behave appropriately in case the user is not diff --git a/common/djangoapps/static_replace/test/test_static_replace.py b/common/djangoapps/static_replace/test/test_static_replace.py index 04a562a771..906ee28c78 100644 --- a/common/djangoapps/static_replace/test/test_static_replace.py +++ b/common/djangoapps/static_replace/test/test_static_replace.py @@ -525,9 +525,9 @@ class CanonicalContentTest(SharedModuleStoreTestCase): # - finally shove back in our regex patterns digest = CanonicalContentTest.get_content_digest_for_asset_path(prefix, start) if digest: - adjusted_asset_key = u'assets/courseware/MARK/asset-v1:a+b+{}+type@asset+block'.format(prefix) - adjusted_th_key = u'assets/courseware/MARK/asset-v1:a+b+{}+type@thumbnail+block'.format(prefix) - encoded_asset_key = u'/assets/courseware/MARK/asset-v1:a+b+{}+type@asset+block@'.format(prefix) + adjusted_asset_key = u'assets/courseware/VMARK/HMARK/asset-v1:a+b+{}+type@asset+block'.format(prefix) + adjusted_th_key = u'assets/courseware/VMARK/HMARK/asset-v1:a+b+{}+type@thumbnail+block'.format(prefix) + encoded_asset_key = u'/assets/courseware/VMARK/HMARK/asset-v1:a+b+{}+type@asset+block@'.format(prefix) encoded_asset_key = urlquote(encoded_asset_key) expected = expected.format( @@ -544,7 +544,8 @@ class CanonicalContentTest(SharedModuleStoreTestCase): ) expected = encode_unicode_characters_in_url(expected) - expected = expected.replace('MARK', '[a-f0-9]{32}') + expected = expected.replace('VMARK', r'v[\d]') + expected = expected.replace('HMARK', '[a-f0-9]{32}') expected = expected.replace('+', r'\+').replace('?', r'\?') with check_mongo_calls(mongo_calls): @@ -728,7 +729,7 @@ class CanonicalContentTest(SharedModuleStoreTestCase): # - finally shove back in our regex patterns digest = CanonicalContentTest.get_content_digest_for_asset_path(prefix, start) if digest: - adjusted_c4x_block = 'assets/courseware/MARK/c4x/a/b/asset' + adjusted_c4x_block = 'assets/courseware/VMARK/HMARK/c4x/a/b/asset' encoded_c4x_block = urlquote('/' + adjusted_c4x_block + '/') expected = expected.format( @@ -741,7 +742,8 @@ class CanonicalContentTest(SharedModuleStoreTestCase): ) expected = encode_unicode_characters_in_url(expected) - expected = expected.replace('MARK', '[a-f0-9]{32}') + expected = expected.replace('VMARK', r'v[\d]') + expected = expected.replace('HMARK', '[a-f0-9]{32}') expected = expected.replace('+', r'\+').replace('?', r'\?') with check_mongo_calls(mongo_calls): diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index c98663e91c..99f0f3e922 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -3,12 +3,13 @@ import uuid from xmodule.assetstore.assetmgr import AssetManager +STATIC_CONTENT_VERSION = 1 XASSET_LOCATION_TAG = 'c4x' XASSET_SRCREF_PREFIX = 'xasset:' XASSET_THUMBNAIL_TAIL_NAME = '.jpg' STREAM_DATA_CHUNK_SIZE = 1024 VERSIONED_ASSETS_PREFIX = '/assets/courseware' -VERSIONED_ASSETS_PATTERN = r'/assets/courseware/([a-f0-9]{32})' +VERSIONED_ASSETS_PATTERN = r'/assets/courseware/(v[\d]/)?([a-f0-9]{32})' import os import logging @@ -163,7 +164,7 @@ class StaticContent(object): if StaticContent.is_versioned_asset_path(asset_path): result = re.match(VERSIONED_ASSETS_PATTERN, asset_path) if result is not None: - asset_digest = result.groups()[0] + asset_digest = result.groups()[1] asset_path = re.sub(VERSIONED_ASSETS_PATTERN, '', asset_path) return (asset_digest, asset_path) @@ -178,7 +179,9 @@ class StaticContent(object): if StaticContent.is_versioned_asset_path(path): return path - return VERSIONED_ASSETS_PREFIX + '/' + version + path + structure_version = 'v{}'.format(STATIC_CONTENT_VERSION) + + return u'{}/{}/{}{}'.format(VERSIONED_ASSETS_PREFIX, structure_version, version, path) @staticmethod def get_asset_key_from_path(course_key, path):