From faaa30c02d4189f8214462d6b95ad3383a232d64 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Wed, 22 Jun 2016 06:17:19 -0400 Subject: [PATCH] [PERF-341] Fix up to not rewrite XBlock resource URLs. For XBlocks that used their static.public resources in the rendered output -- for example, a link to a bundled image -- those URLs would be treated as course assets using the '/static/' prefix trick, and thus, rewritten. These rewritten URLs don't work because they aren't course assets. When course content authors are creating their courses, we provide them a special shorthand way of writing URLs that reference assets they have uploaded to the course. If they uploaded a file called my-lil-pony.mp4, and they wanted to provide a link to it when users view the course, they would use /static/my-lil-pony.mp4. This special prefix -- /static/ -- signals to the static_replace middleware that it's a course asset and we should write /static/my-lil-pony.mp4 to a URL that properly references the asset based on the course ID, and things like the configured asset CDN, etc. Thus, the URL /static/my-lil-pony.mp4 gets turned into something like: /assets/courseware//asset-v1:edX+Demo+2016T1+type@asset+block/my-lil-pony.mp4 when viewed in the courseware. Now, we also serve actual static assets from a prefix of /static/. This is stuff like our JavaScript and CSS, and the JS/CSS/etc of XBlocks/XModules. These paths look like: /static/js/lms-main_vendor.46d6a8c02600.js or /static/xblock/resources/xmodule.vertical_block/public/js/vertical_student_view.43727a907769.js Normally, these paths are caught by nginx, before they reach the LMS, and are served straight from the filesystem. However, if you were to have one of these paths in your course content, the static_replace middleware would see the /static/ at the front and immediately think it's a course asset, and would dutifully rewrite the URL to something like: /assets/courseware//asset-v1:edX+Demo+2016T1+type@asset+block/static_xblock_resources_xmodule.vertical_block_public_js_vertical_student_view.43727a907769.js which is not a course asset, and so it will always fail to load. Long story short, I changed the static_replace middleware to specifically check to see if the path being matched starts with /static/xblock/, and if so, it keeps the original instead of rewriting it. --- common/djangoapps/static_replace/__init__.py | 8 ++++++++ .../static_replace/test/test_static_replace.py | 15 +++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/common/djangoapps/static_replace/__init__.py b/common/djangoapps/static_replace/__init__.py index d98bea1b90..e2a8fe5069 100644 --- a/common/djangoapps/static_replace/__init__.py +++ b/common/djangoapps/static_replace/__init__.py @@ -13,6 +13,7 @@ from xmodule.contentstore.content import StaticContent from opaque_keys.edx.locator import AssetLocator log = logging.getLogger(__name__) +XBLOCK_STATIC_RESOURCE_PREFIX = '/static/xblock' def _url_replace_regex(prefix): @@ -109,6 +110,13 @@ def process_static_urls(text, replacement_function, data_dir=None): prefix = match.group('prefix') quote = match.group('quote') rest = match.group('rest') + + # Don't rewrite XBlock resource links. Probably wasn't a good idea that /static + # works for actual static assets and for magical course asset URLs.... + full_url = prefix + rest + if full_url.startswith(XBLOCK_STATIC_RESOURCE_PREFIX): + return original + return replacement_function(original, prefix, quote, rest) return re.sub( diff --git a/common/djangoapps/static_replace/test/test_static_replace.py b/common/djangoapps/static_replace/test/test_static_replace.py index b0a74e3315..04a562a771 100644 --- a/common/djangoapps/static_replace/test/test_static_replace.py +++ b/common/djangoapps/static_replace/test/test_static_replace.py @@ -196,6 +196,21 @@ def test_regex(): assert_false(re.match(regex, s)) +@patch('static_replace.staticfiles_storage', autospec=True) +@patch('static_replace.modulestore', autospec=True) +def test_static_url_with_xblock_resource(mock_modulestore, mock_storage): + """ + Make sure that for URLs with XBlock resource URL, which start with /static/, + we don't rewrite them. + """ + mock_storage.exists.return_value = False + mock_modulestore.return_value = Mock(MongoModuleStore) + + pre_text = 'EMBED src ="/static/xblock/resources/babys_first.lil_xblock/public/images/pacifier.png"' + post_text = pre_text + assert_equals(post_text, replace_static_urls(pre_text, DATA_DIRECTORY, COURSE_KEY)) + + @ddt.ddt class CanonicalContentTest(SharedModuleStoreTestCase): """