From 77343df0d927e64c810cad82fcce7d8c6c2fa2be Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Thu, 17 Dec 2015 15:24:26 -0500 Subject: [PATCH] [PERF-224] Support to serve static assets from a CDN. A base URL can now be configured which is, potentially, prepended to an asset URL. This allows a CDN, or caching server, to front static asset requests, taking load off of the contentstore and speeding up page load times. Asset URL generation respects locked vs unlocked assets, and will not generate links to locked assets that would traverse a CDN (even though the authorization component of the contentserver middleware wouldn't allow those links to work anyways). --- common/djangoapps/static_replace/__init__.py | 4 +- common/djangoapps/static_replace/admin.py | 30 ++ common/djangoapps/static_replace/models.py | 29 ++ .../test/test_static_replace.py | 293 +++++++++++++++++- .../xmodule/xmodule/contentstore/content.py | 94 ++++-- .../lib/xmodule/xmodule/tests/test_content.py | 10 +- .../courseware/tests/test_module_render.py | 2 +- 7 files changed, 428 insertions(+), 34 deletions(-) create mode 100644 common/djangoapps/static_replace/admin.py create mode 100644 common/djangoapps/static_replace/models.py diff --git a/common/djangoapps/static_replace/__init__.py b/common/djangoapps/static_replace/__init__.py index b22a1058e1..64145302ff 100644 --- a/common/djangoapps/static_replace/__init__.py +++ b/common/djangoapps/static_replace/__init__.py @@ -5,6 +5,7 @@ from django.contrib.staticfiles.storage import staticfiles_storage from django.contrib.staticfiles import finders from django.conf import settings +from static_replace.models import AssetBaseUrlConfig from xmodule.modulestore.django import modulestore from xmodule.modulestore import ModuleStoreEnum from xmodule.contentstore.content import StaticContent @@ -180,7 +181,8 @@ def replace_static_urls(text, data_directory=None, course_id=None, static_asset_ else: # if not, then assume it's courseware specific content and then look in the # Mongo-backed database - url = StaticContent.convert_legacy_static_url_with_course_id(rest, course_id) + base_url = AssetBaseUrlConfig.get_base_url() + url = StaticContent.get_canonicalized_asset_path(course_id, rest, base_url) if AssetLocator.CANONICAL_NAMESPACE in url: url = url.replace('block@', 'block/', 1) diff --git a/common/djangoapps/static_replace/admin.py b/common/djangoapps/static_replace/admin.py new file mode 100644 index 0000000000..d475d61b2f --- /dev/null +++ b/common/djangoapps/static_replace/admin.py @@ -0,0 +1,30 @@ +""" +Django admin page for AssetBaseUrlConfig, which allows you to set the base URL +that gets prepended to asset URLs in order to serve them from, say, a CDN. +""" +from django.contrib import admin + +from config_models.admin import ConfigurationModelAdmin +from .models import AssetBaseUrlConfig + + +class AssetBaseUrlConfigAdmin(ConfigurationModelAdmin): + """ + Basic configuration for asset base URL. + """ + list_display = [ + 'base_url' + ] + + def get_list_display(self, request): + """ + Restore default list_display behavior. + + ConfigurationModelAdmin overrides this, but in a way that doesn't + respect the ordering. This lets us customize it the usual Django admin + way. + """ + return self.list_display + + +admin.site.register(AssetBaseUrlConfig, AssetBaseUrlConfigAdmin) diff --git a/common/djangoapps/static_replace/models.py b/common/djangoapps/static_replace/models.py new file mode 100644 index 0000000000..7521d885bb --- /dev/null +++ b/common/djangoapps/static_replace/models.py @@ -0,0 +1,29 @@ +""" +Models for static_replace +""" + +from django.db.models.fields import TextField +from config_models.models import ConfigurationModel + + +class AssetBaseUrlConfig(ConfigurationModel): + """Configuration for the base URL used for static assets.""" + + class Meta(object): + app_label = 'static_replace' + + base_url = TextField( + blank=True, + help_text="The alternative hostname to serve static assets from. Should be in the form of hostname[:port]." + ) + + @classmethod + def get_base_url(cls): + """Gets the base URL to use for serving static assets, if present""" + return cls.current().base_url + + def __repr__(self): + return ''.format(self.get_base_url()) + + def __unicode__(self): + return unicode(repr(self)) diff --git a/common/djangoapps/static_replace/test/test_static_replace.py b/common/djangoapps/static_replace/test/test_static_replace.py index 0d86acfdc7..56f90bd0be 100644 --- a/common/djangoapps/static_replace/test/test_static_replace.py +++ b/common/djangoapps/static_replace/test/test_static_replace.py @@ -1,4 +1,11 @@ +"""Tests for static_replace""" + +from urllib import quote_plus + +import ddt import re +from PIL import Image +from cStringIO import StringIO from nose.tools import assert_equals, assert_true, assert_false # pylint: disable=no-name-in-module from static_replace import ( @@ -11,7 +18,12 @@ from static_replace import ( from mock import patch, Mock from opaque_keys.edx.locations import SlashSeparatedCourseKey +from xmodule.contentstore.content import StaticContent +from xmodule.contentstore.django import contentstore +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.mongo import MongoModuleStore +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls from xmodule.modulestore.xml import XMLModuleStore DATA_DIRECTORY = 'data_dir' @@ -85,21 +97,23 @@ def test_storage_url_not_exists(mock_storage): @patch('static_replace.StaticContent', autospec=True) @patch('static_replace.modulestore', autospec=True) -def test_mongo_filestore(mock_modulestore, mock_static_content): +@patch('static_replace.AssetBaseUrlConfig.get_base_url') +def test_mongo_filestore(mock_get_base_url, mock_modulestore, mock_static_content): mock_modulestore.return_value = Mock(MongoModuleStore) - mock_static_content.convert_legacy_static_url_with_course_id.return_value = "c4x://mock_url" + mock_static_content.get_canonicalized_asset_path.return_value = "c4x://mock_url" + mock_get_base_url.return_value = u'' # No namespace => no change to path assert_equals('"/static/data_dir/file.png"', replace_static_urls(STATIC_SOURCE, DATA_DIRECTORY)) # Namespace => content url assert_equals( - '"' + mock_static_content.convert_legacy_static_url_with_course_id.return_value + '"', + '"' + mock_static_content.get_canonicalized_asset_path.return_value + '"', replace_static_urls(STATIC_SOURCE, DATA_DIRECTORY, course_id=COURSE_KEY) ) - mock_static_content.convert_legacy_static_url_with_course_id.assert_called_once_with('file.png', COURSE_KEY) + mock_static_content.get_canonicalized_asset_path.assert_called_once_with(COURSE_KEY, 'file.png', u'') @patch('static_replace.settings', autospec=True) @@ -161,3 +175,274 @@ def test_regex(): for s in no: print 'Should not match: {0!r}'.format(s) assert_false(re.match(regex, s)) + + +@ddt.ddt +class CanonicalContentTest(SharedModuleStoreTestCase): + """ + Tests the generation of canonical asset URLs for different types + of assets: c4x-style, opaque key style, locked, unlocked, CDN + set, CDN not set, etc. + """ + + def setUp(self): + super(CanonicalContentTest, self).setUp() + + @classmethod + def setUpClass(cls): + cls.courses = {} + + super(CanonicalContentTest, cls).setUpClass() + + names_and_prefixes = [(ModuleStoreEnum.Type.split, 'split'), (ModuleStoreEnum.Type.mongo, 'old')] + for store, prefix in names_and_prefixes: + with cls.store.default_store(store): + cls.courses[prefix] = CourseFactory.create(org='a', course='b', run=prefix) + + # Create an unlocked image. + unlock_content = cls.create_image(prefix, (32, 32), 'blue', '{}_unlock.png') + + # Create a locked image. + lock_content = cls.create_image(prefix, (32, 32), 'green', '{}_lock.png', locked=True) + + # Create a thumbnail of the images. + contentstore().generate_thumbnail(unlock_content, dimensions=(16, 16)) + contentstore().generate_thumbnail(lock_content, dimensions=(16, 16)) + + # Create an unlocked image in a subdirectory. + cls.create_image(prefix, (1, 1), 'red', 'special/{}_unlock.png') + + # Create a locked image in a subdirectory. + cls.create_image(prefix, (1, 1), 'yellow', 'special/{}_lock.png', locked=True) + + # Create an unlocked image with funky characters in the name. + cls.create_image(prefix, (1, 1), 'black', 'weird {}_unlock.png') + + @classmethod + def create_image(cls, prefix, dimensions, color, name, locked=False): + """ + Creates an image. + + Args: + prefix: the prefix to use e.g. split vs mongo + dimensions: tuple of (width, height) + color: the background color of the image + name: the name of the image; can be a format string + locked: whether or not the asset should be locked + + Returns: + StaticContent: the StaticContent object for the created image + """ + new_image = Image.new('RGB', dimensions, color) + new_buf = StringIO() + new_image.save(new_buf, format='png') + new_buf.seek(0) + new_name = name.format(prefix) + new_key = StaticContent.compute_location(cls.courses[prefix].id, new_name) + new_content = StaticContent(new_key, new_name, 'image/png', new_buf.getvalue(), locked=locked) + contentstore().save(new_content) + + return new_content + + @ddt.data( + # No leading slash. + (u'', u'{prefix}_unlock.png', u'/{asset_key}@{prefix}_unlock.png', 1), + (u'', u'{prefix}_lock.png', u'/{asset_key}@{prefix}_lock.png', 1), + (u'', u'weird {prefix}_unlock.png', u'/{asset_key}@weird_{prefix}_unlock.png', 1), + (u'dev', u'{prefix}_unlock.png', u'//dev/{asset_key}@{prefix}_unlock.png', 1), + (u'dev', u'{prefix}_lock.png', u'/{asset_key}@{prefix}_lock.png', 1), + (u'dev', u'weird {prefix}_unlock.png', u'//dev/{asset_key}@weird_{prefix}_unlock.png', 1), + # No leading slash with subdirectory. This ensures we properly substitute slashes. + (u'', u'special/{prefix}_unlock.png', u'/{asset_key}@special_{prefix}_unlock.png', 1), + (u'', u'special/{prefix}_lock.png', u'/{asset_key}@special_{prefix}_lock.png', 1), + (u'dev', u'special/{prefix}_unlock.png', u'//dev/{asset_key}@special_{prefix}_unlock.png', 1), + (u'dev', u'special/{prefix}_lock.png', u'/{asset_key}@special_{prefix}_lock.png', 1), + # Leading slash. + (u'', u'/{prefix}_unlock.png', u'/{asset_key}@{prefix}_unlock.png', 1), + (u'', u'/{prefix}_lock.png', u'/{asset_key}@{prefix}_lock.png', 1), + (u'dev', u'/{prefix}_unlock.png', u'//dev/{asset_key}@{prefix}_unlock.png', 1), + (u'dev', u'/{prefix}_lock.png', u'/{asset_key}@{prefix}_lock.png', 1), + # Leading slash with subdirectory. This ensures we properly substitute slashes. + (u'', u'/special/{prefix}_unlock.png', u'/{asset_key}@special_{prefix}_unlock.png', 1), + (u'', u'/special/{prefix}_lock.png', u'/{asset_key}@special_{prefix}_lock.png', 1), + (u'dev', u'/special/{prefix}_unlock.png', u'//dev/{asset_key}@special_{prefix}_unlock.png', 1), + (u'dev', u'/special/{prefix}_lock.png', u'/{asset_key}@special_{prefix}_lock.png', 1), + # Static path. + (u'', u'/static/{prefix}_unlock.png', u'/{asset_key}@{prefix}_unlock.png', 1), + (u'', u'/static/{prefix}_lock.png', u'/{asset_key}@{prefix}_lock.png', 1), + (u'', u'/static/weird {prefix}_unlock.png', u'/{asset_key}@weird_{prefix}_unlock.png', 1), + (u'dev', u'/static/{prefix}_unlock.png', u'//dev/{asset_key}@{prefix}_unlock.png', 1), + (u'dev', u'/static/{prefix}_lock.png', u'/{asset_key}@{prefix}_lock.png', 1), + (u'dev', u'/static/weird {prefix}_unlock.png', u'//dev/{asset_key}@weird_{prefix}_unlock.png', 1), + # Static path with subdirectory. This ensures we properly substitute slashes. + (u'', u'/static/special/{prefix}_unlock.png', u'/{asset_key}@special_{prefix}_unlock.png', 1), + (u'', u'/static/special/{prefix}_lock.png', u'/{asset_key}@special_{prefix}_lock.png', 1), + (u'dev', u'/static/special/{prefix}_unlock.png', u'//dev/{asset_key}@special_{prefix}_unlock.png', 1), + (u'dev', u'/static/special/{prefix}_lock.png', u'/{asset_key}@special_{prefix}_lock.png', 1), + # Static path with query parameter. + ( + u'', + u'/static/{prefix}_unlock.png?foo=/static/{prefix}_lock.png', + u'/{asset_key}@{prefix}_unlock.png?foo={encoded_asset_key}{prefix}_lock.png', + 2 + ), + ( + u'', + u'/static/{prefix}_lock.png?foo=/static/{prefix}_unlock.png', + u'/{asset_key}@{prefix}_lock.png?foo={encoded_asset_key}{prefix}_unlock.png', + 2 + ), + ( + u'dev', + u'/static/{prefix}_unlock.png?foo=/static/{prefix}_lock.png', + u'//dev/{asset_key}@{prefix}_unlock.png?foo={encoded_asset_key}{prefix}_lock.png', + 2 + ), + ( + u'dev', + u'/static/{prefix}_lock.png?foo=/static/{prefix}_unlock.png', + u'/{asset_key}@{prefix}_lock.png?foo={encoded_base_url}{encoded_asset_key}{prefix}_unlock.png', + 2 + ), + # Already asset key. + (u'', u'/{asset_key}@{prefix}_unlock.png', u'/{asset_key}@{prefix}_unlock.png', 1), + (u'', u'/{asset_key}@{prefix}_lock.png', u'/{asset_key}@{prefix}_lock.png', 1), + (u'dev', u'/{asset_key}@{prefix}_unlock.png', u'//dev/{asset_key}@{prefix}_unlock.png', 1), + (u'dev', u'/{asset_key}@{prefix}_lock.png', u'/{asset_key}@{prefix}_lock.png', 1), + # Old, c4x-style path. + (u'', u'/{c4x}/{prefix}_unlock.png', u'/{c4x}/{prefix}_unlock.png', 1), + (u'', u'/{c4x}/{prefix}_lock.png', u'/{c4x}/{prefix}_lock.png', 1), + (u'', u'/{c4x}/weird_{prefix}_lock.png', u'/{c4x}/weird_{prefix}_lock.png', 1), + (u'dev', u'/{c4x}/{prefix}_unlock.png', u'/{c4x}/{prefix}_unlock.png', 1), + (u'dev', u'/{c4x}/{prefix}_lock.png', u'/{c4x}/{prefix}_lock.png', 1), + (u'dev', u'/{c4x}/weird_{prefix}_unlock.png', u'/{c4x}/weird_{prefix}_unlock.png', 1), + # Thumbnails. + (u'', u'/{th_key}@{prefix}_unlock-{th_ext}', u'/{th_key}@{prefix}_unlock-{th_ext}', 1), + (u'', u'/{th_key}@{prefix}_lock-{th_ext}', u'/{th_key}@{prefix}_lock-{th_ext}', 1), + (u'dev', u'/{th_key}@{prefix}_unlock-{th_ext}', u'//dev/{th_key}@{prefix}_unlock-{th_ext}', 1), + (u'dev', u'/{th_key}@{prefix}_lock-{th_ext}', u'//dev/{th_key}@{prefix}_lock-{th_ext}', 1), + ) + @ddt.unpack + def test_canonical_asset_path_with_new_style_assets(self, base_url, start, expected, mongo_calls): + prefix = 'split' + encoded_base_url = quote_plus('//' + base_url) + c4x = 'c4x/a/b/asset' + asset_key = 'asset-v1:a+b+{}+type@asset+block'.format(prefix) + encoded_asset_key = quote_plus('/asset-v1:a+b+{}+type@asset+block@'.format(prefix)) + th_key = 'asset-v1:a+b+{}+type@thumbnail+block'.format(prefix) + th_ext = 'png-16x16.jpg' + + start = start.format( + prefix=prefix, + c4x=c4x, + asset_key=asset_key, + encoded_base_url=encoded_base_url, + encoded_asset_key=encoded_asset_key, + th_key=th_key, + th_ext=th_ext + ) + expected = expected.format( + prefix=prefix, + c4x=c4x, + asset_key=asset_key, + encoded_base_url=encoded_base_url, + encoded_asset_key=encoded_asset_key, + th_key=th_key, + th_ext=th_ext + ) + + with check_mongo_calls(mongo_calls): + asset_path = StaticContent.get_canonicalized_asset_path(self.courses[prefix].id, start, base_url) + self.assertEqual(asset_path, expected) + + @ddt.data( + # No leading slash. + (u'', u'{prefix}_unlock.png', u'/{c4x}/{prefix}_unlock.png', 1), + (u'', u'{prefix}_lock.png', u'/{c4x}/{prefix}_lock.png', 1), + (u'', u'weird {prefix}_unlock.png', u'/{c4x}/weird_{prefix}_unlock.png', 1), + (u'dev', u'{prefix}_unlock.png', u'//dev/{c4x}/{prefix}_unlock.png', 1), + (u'dev', u'{prefix}_lock.png', u'/{c4x}/{prefix}_lock.png', 1), + (u'dev', u'weird {prefix}_unlock.png', u'//dev/{c4x}/weird_{prefix}_unlock.png', 1), + # No leading slash with subdirectory. This ensures we probably substitute slashes. + (u'', u'special/{prefix}_unlock.png', u'/{c4x}/special_{prefix}_unlock.png', 1), + (u'', u'special/{prefix}_lock.png', u'/{c4x}/special_{prefix}_lock.png', 1), + (u'dev', u'special/{prefix}_unlock.png', u'//dev/{c4x}/special_{prefix}_unlock.png', 1), + (u'dev', u'special/{prefix}_lock.png', u'/{c4x}/special_{prefix}_lock.png', 1), + # Leading slash. + (u'', u'/{prefix}_unlock.png', u'/{c4x}/{prefix}_unlock.png', 1), + (u'', u'/{prefix}_lock.png', u'/{c4x}/{prefix}_lock.png', 1), + (u'dev', u'/{prefix}_unlock.png', u'//dev/{c4x}/{prefix}_unlock.png', 1), + (u'dev', u'/{prefix}_lock.png', u'/{c4x}/{prefix}_lock.png', 1), + # Leading slash with subdirectory. This ensures we properly substitute slashes. + (u'', u'/special/{prefix}_unlock.png', u'/{c4x}/special_{prefix}_unlock.png', 1), + (u'', u'/special/{prefix}_lock.png', u'/{c4x}/special_{prefix}_lock.png', 1), + (u'dev', u'/special/{prefix}_unlock.png', u'//dev/{c4x}/special_{prefix}_unlock.png', 1), + (u'dev', u'/special/{prefix}_lock.png', u'/{c4x}/special_{prefix}_lock.png', 1), + # Static path. + (u'', u'/static/{prefix}_unlock.png', u'/{c4x}/{prefix}_unlock.png', 1), + (u'', u'/static/{prefix}_lock.png', u'/{c4x}/{prefix}_lock.png', 1), + (u'', u'/static/weird {prefix}_unlock.png', u'/{c4x}/weird_{prefix}_unlock.png', 1), + (u'dev', u'/static/{prefix}_unlock.png', u'//dev/{c4x}/{prefix}_unlock.png', 1), + (u'dev', u'/static/{prefix}_lock.png', u'/{c4x}/{prefix}_lock.png', 1), + (u'dev', u'/static/weird {prefix}_unlock.png', u'//dev/{c4x}/weird_{prefix}_unlock.png', 1), + # Static path with subdirectory. This ensures we properly substitute slashes. + (u'', u'/static/special/{prefix}_unlock.png', u'/{c4x}/special_{prefix}_unlock.png', 1), + (u'', u'/static/special/{prefix}_lock.png', u'/{c4x}/special_{prefix}_lock.png', 1), + (u'dev', u'/static/special/{prefix}_unlock.png', u'//dev/{c4x}/special_{prefix}_unlock.png', 1), + (u'dev', u'/static/special/{prefix}_lock.png', u'/{c4x}/special_{prefix}_lock.png', 1), + # Static path with query parameter. + ( + u'', + u'/static/{prefix}_unlock.png?foo=/static/{prefix}_lock.png', + u'/{c4x}/{prefix}_unlock.png?foo={encoded_c4x}{prefix}_lock.png', + 2 + ), + ( + u'', + u'/static/{prefix}_lock.png?foo=/static/{prefix}_unlock.png', + u'/{c4x}/{prefix}_lock.png?foo={encoded_c4x}{prefix}_unlock.png', + 2 + ), + ( + u'dev', + u'/static/{prefix}_unlock.png?foo=/static/{prefix}_lock.png', + u'//dev/{c4x}/{prefix}_unlock.png?foo={encoded_c4x}{prefix}_lock.png', + 2 + ), + ( + u'dev', + u'/static/{prefix}_lock.png?foo=/static/{prefix}_unlock.png', + u'/{c4x}/{prefix}_lock.png?foo={encoded_base_url}{encoded_c4x}{prefix}_unlock.png', + 2 + ), + # Old, c4x-style path. + (u'', u'/{c4x}/{prefix}_unlock.png', u'/{c4x}/{prefix}_unlock.png', 1), + (u'', u'/{c4x}/{prefix}_lock.png', u'/{c4x}/{prefix}_lock.png', 1), + (u'', u'/{c4x}/weird_{prefix}_unlock.png', u'/{c4x}/weird_{prefix}_unlock.png', 1), + (u'dev', u'/{c4x}/{prefix}_unlock.png', u'//dev/{c4x}/{prefix}_unlock.png', 1), + (u'dev', u'/{c4x}/{prefix}_lock.png', u'/{c4x}/{prefix}_lock.png', 1), + (u'dev', u'/{c4x}/weird_{prefix}_unlock.png', u'//dev/{c4x}/weird_{prefix}_unlock.png', 1), + ) + @ddt.unpack + def test_canonical_asset_path_with_c4x_style_assets(self, base_url, start, expected, mongo_calls): + prefix = 'old' + c4x_block = 'c4x/a/b/asset' + encoded_c4x_block = quote_plus('/' + c4x_block + '/') + encoded_base_url = quote_plus('//' + base_url) + + start = start.format( + prefix=prefix, + encoded_base_url=encoded_base_url, + c4x=c4x_block, + encoded_c4x=encoded_c4x_block + ) + expected = expected.format( + prefix=prefix, + encoded_base_url=encoded_base_url, + c4x=c4x_block, + encoded_c4x=encoded_c4x_block + ) + + with check_mongo_calls(mongo_calls): + asset_path = StaticContent.get_canonicalized_asset_path(self.courses[prefix].id, start, base_url) + self.assertEqual(asset_path, expected) diff --git a/common/lib/xmodule/xmodule/contentstore/content.py b/common/lib/xmodule/xmodule/contentstore/content.py index 9337b176e8..ddf92eea35 100644 --- a/common/lib/xmodule/xmodule/contentstore/content.py +++ b/common/lib/xmodule/xmodule/contentstore/content.py @@ -1,5 +1,8 @@ import re import uuid + +from xmodule.assetstore.assetmgr import AssetManager + XASSET_LOCATION_TAG = 'c4x' XASSET_SRCREF_PREFIX = 'xasset:' @@ -16,6 +19,8 @@ from urllib import urlencode from opaque_keys.edx.locator import AssetLocator from opaque_keys.edx.keys import CourseKey, AssetKey from opaque_keys import InvalidKeyError +from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.exceptions import NotFoundError from PIL import Image @@ -137,32 +142,79 @@ class StaticContent(object): return AssetKey.from_string(path[1:]) @staticmethod - def convert_legacy_static_url_with_course_id(path, course_id): + def get_asset_key_from_path(course_key, path): """ - Returns a path to a piece of static content when we are provided with a filepath and - a course_id - """ - # Generate url of urlparse.path component - scheme, netloc, orig_path, params, query, fragment = urlparse(path) - loc = StaticContent.compute_location(course_id, orig_path) - loc_url = StaticContent.serialize_asset_key_with_slash(loc) + Parses a path, extracting an asset key or creating one. - # parse the query params for "^/static/" and replace with the location url - orig_query = parse_qsl(query) - new_query_list = [] - for query_name, query_value in orig_query: + Args: + course_key: key to the course which owns this asset + path: the path to said content + + Returns: + AssetKey: the asset key that represents the path + """ + + # Clean up the path, removing any static prefix and any leading slash. + if path.startswith('/static/'): + path = path[len('/static/'):] + + path = path.lstrip('/') + + try: + return AssetKey.from_string(path) + except InvalidKeyError: + # If we couldn't parse the path, just let compute_location figure it out. + # It's most likely a path like /image.png or something. + return StaticContent.compute_location(course_key, path) + + @staticmethod + def get_canonicalized_asset_path(course_key, path, base_url): + """ + Returns a fully-qualified path to a piece of static content. + + If a static asset CDN is configured, this path will include it. + Otherwise, the path will simply be relative. + + Args: + course_key: key to the course which owns this asset + path: the path to said content + + Returns: + string: fully-qualified path to asset + """ + + # Break down the input path. + _, _, relative_path, params, query_string, fragment = urlparse(path) + + # Convert our path to an asset key if it isn't one already. + asset_key = StaticContent.get_asset_key_from_path(course_key, relative_path) + + # Check the status of the asset to see if this can be served via CDN aka publicly. + serve_from_cdn = False + try: + content = AssetManager.find(asset_key, as_stream=True) + is_locked = getattr(content, "locked", True) + serve_from_cdn = not is_locked + except (ItemNotFoundError, NotFoundError): + # If we can't find the item, just treat it as if it's locked. + serve_from_cdn = False + + # Update any query parameter values that have asset paths in them. This is for assets that + # require their own after-the-fact values, like a Flash file that needs the path of a config + # file passed to it e.g. /static/visualization.swf?configFile=/static/visualization.xml + query_params = parse_qsl(query_string) + updated_query_params = [] + for query_name, query_value in query_params: if query_value.startswith("/static/"): - new_query = StaticContent.compute_location( - course_id, - query_value[len('/static/'):], - ) - new_query_url = StaticContent.serialize_asset_key_with_slash(new_query) - new_query_list.append((query_name, new_query_url)) + new_query_value = StaticContent.get_canonicalized_asset_path(course_key, query_value, base_url) + updated_query_params.append((query_name, new_query_value)) else: - new_query_list.append((query_name, query_value)) + updated_query_params.append((query_name, query_value)) - # Reconstruct with new path - return urlunparse((scheme, netloc, loc_url, params, urlencode(new_query_list), fragment)) + serialized_asset_key = StaticContent.serialize_asset_key_with_slash(asset_key) + base_url = base_url if serve_from_cdn else '' + + return urlunparse((None, base_url, serialized_asset_key, params, urlencode(updated_query_params), fragment)) def stream_data(self): yield self._data diff --git a/common/lib/xmodule/xmodule/tests/test_content.py b/common/lib/xmodule/xmodule/tests/test_content.py index 637bb86d23..4d151e20e3 100644 --- a/common/lib/xmodule/xmodule/tests/test_content.py +++ b/common/lib/xmodule/xmodule/tests/test_content.py @@ -4,6 +4,7 @@ import os import unittest import ddt from path import Path as path + from xmodule.contentstore.content import StaticContent, StaticContentStream from xmodule.contentstore.content import ContentStore from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation @@ -94,11 +95,6 @@ class ContentTest(unittest.TestCase): content = StaticContent('loc', 'name', 'content_type', 'data') self.assertIsNone(content.thumbnail_location) - def test_static_url_generation_from_courseid(self): - course_key = SlashSeparatedCourseKey('foo', 'bar', 'bz') - url = StaticContent.convert_legacy_static_url_with_course_id('images_course_image.jpg', course_key) - self.assertEqual(url, '/c4x/foo/bar/asset/images_course_image.jpg') - @ddt.data( (u"monsters__.jpg", u"monsters__.jpg"), (u"monsters__.png", u"monsters__-png.jpg"), @@ -122,9 +118,9 @@ class ContentTest(unittest.TestCase): self.assertEqual(AssetLocation(u'mitX', u'400', u'ignore', u'asset', u'subs__1eo_jXvZnE_.srt.sjson', None), asset_location) def test_get_location_from_path(self): - asset_location = StaticContent.get_location_from_path(u'/c4x/foo/bar/asset/images_course_image.jpg') + asset_location = StaticContent.get_location_from_path(u'/c4x/a/b/asset/images_course_image.jpg') self.assertEqual( - AssetLocation(u'foo', u'bar', None, u'asset', u'images_course_image.jpg', None), + AssetLocation(u'a', u'b', None, u'asset', u'images_course_image.jpg', None), asset_location ) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 9970580737..8618242a6d 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1104,7 +1104,7 @@ class TestHtmlModifiers(ModuleStoreTestCase): result_fragment = module.render(STUDENT_VIEW) self.assertIn( - '/c4x/{org}/{course}/asset/_file.jpg'.format( + '/c4x/{org}/{course}/asset/file.jpg'.format( org=self.course.location.org, course=self.course.location.course, ),