Merge pull request #11839 from edx/PERF-268

Make CDNifying of course over image URLs only happen for relative URLs.

openedx/core/djangoapps/content/course_overviews/models.py

@@ -3,7 +3,7 @@ Declaration of CourseOverview model
 """
 import json
 import logging
-from urlparse import urlunparse
+from urlparse import urlparse, urlunparse
 
 from django.db import models, transaction
 from django.db.models.fields import BooleanField, DateTimeField, DecimalField, TextField, FloatField, IntegerField
@@ -549,9 +549,9 @@ class CourseOverview(TimeStampedModel):
             urls['small'] = self.image_set.small_url or raw_image_url
             urls['large'] = self.image_set.large_url or raw_image_url
 
-        return self._apply_cdn(urls)
+        return self.apply_cdn_to_urls(urls)
 
-    def _apply_cdn(self, image_urls):
+    def apply_cdn_to_urls(self, image_urls):
         """
         Given a dict of resolutions -> urls, return a copy with CDN applied.
 
@@ -568,10 +568,32 @@ class CourseOverview(TimeStampedModel):
         base_url = cdn_config.base_url
 
         return {
-            resolution: urlunparse((None, base_url, url, None, None, None))
+            resolution: self._apply_cdn_to_url(url, base_url)
             for resolution, url in image_urls.items()
         }
 
+    def _apply_cdn_to_url(self, url, base_url):
+        """
+        Applies a new CDN/base URL to the given URL.
+
+        If a URL is absolute, we skip switching the host since it could
+        be a hostname that isn't behind our CDN, and we could unintentionally
+        break the URL overall.
+        """
+
+        # The URL can't be empty.
+        if not url:
+            return url
+
+        _, netloc, path, params, query, fragment = urlparse(url)
+
+        # If this is an absolute URL, just return it as is.  It could be a domain
+        # that isn't ours, and thus CDNing it would actually break it.
+        if netloc:
+            return url
+
+        return urlunparse((None, base_url, path, params, query, fragment))
+
     def __unicode__(self):
         """Represent ourselves with the course key."""
         return unicode(self.id)

openedx/core/djangoapps/content/course_overviews/tests.py

@@ -669,6 +669,31 @@ class CourseOverviewImageSetTestCase(ModuleStoreTestCase):
             for url in overview.image_urls.values():
                 self.assertTrue(url.startswith(expected_cdn_url))
 
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_cdn_with_external_image(self, modulestore_type):
+        """
+        Test that we return CDN prefixed URLs unless they're absolute.
+        """
+        with self.store.default_store(modulestore_type):
+            course = CourseFactory.create(default_store=modulestore_type)
+            overview = CourseOverview.get_from_id(course.id)
+
+            # Now enable the CDN...
+            AssetBaseUrlConfig.objects.create(enabled=True, base_url='fakecdn.edx.org')
+            expected_cdn_url = "//fakecdn.edx.org"
+
+            start_urls = {
+                'raw': 'http://google.com/image.png',
+                'small': '/static/overview.png',
+                'large': ''
+            }
+
+            modified_urls = overview.apply_cdn_to_urls(start_urls)
+            self.assertEqual(modified_urls['raw'], start_urls['raw'])
+            self.assertNotEqual(modified_urls['small'], start_urls['small'])
+            self.assertTrue(modified_urls['small'].startswith(expected_cdn_url))
+            self.assertEqual(modified_urls['large'], start_urls['large'])
+
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
     def test_error_generating_thumbnails(self, modulestore_type):
         """