Merge pull request #8980 from cpennington/cache-ccx-lookups

Cache ccx lookups

common/djangoapps/request_cache/__init__.py

+"""
+A cache that is cleared after every request.
+
+This module requires that :class:`request_cache.middleware.RequestCache`
+is installed in order to clear the cache after each request.
+"""
+
+
+from request_cache import middleware
+
+
+def get_cache(name):
+    """
+    Return the request cache named ``name``.
+
+    Arguments:
+        name (str): The name of the request cache to load
+
+    Returns: dict
+    """
+    return middleware.RequestCache.get_request_cache(name)
+
+
+def get_request():
+    """
+    Return the current request.
+    """
+    return middleware.RequestCache.get_current_request()

common/djangoapps/request_cache/middleware.py

 import threading
 
-_request_cache_threadlocal = threading.local()
-_request_cache_threadlocal.data = {}
-_request_cache_threadlocal.request = None
+
+class _RequestCache(threading.local):
+    """
+    A thread-local for storing the per-request cache.
+    """
+    def __init__(self):
+        super(_RequestCache, self).__init__()
+        self.data = {}
+        self.request = None
+
+
+REQUEST_CACHE = _RequestCache()
 
 
 class RequestCache(object):
     @classmethod
-    def get_request_cache(cls):
-        return _request_cache_threadlocal
+    def get_request_cache(cls, name=None):
+        """
+        This method is deprecated. Please use :func:`request_cache.get_cache`.
+        """
+        if name is None:
+            return REQUEST_CACHE
+        else:
+            return REQUEST_CACHE.data.setdefault(name, {})
 
     @classmethod
     def get_current_request(cls):
         """
-        Get a reference to the HttpRequest object, if we are presently
-        servicing one.
+        This method is deprecated. Please use :func:`request_cache.get_request`.
         """
-        return _request_cache_threadlocal.request
+        return REQUEST_CACHE.request
 
     @classmethod
     def clear_request_cache(cls):
         """
         Empty the request cache.
         """
-        _request_cache_threadlocal.data = {}
-        _request_cache_threadlocal.request = None
+        REQUEST_CACHE.data = {}
+        REQUEST_CACHE.request = None
 
     def process_request(self, request):
         self.clear_request_cache()
-        _request_cache_threadlocal.request = request
+        REQUEST_CACHE.request = request
         return None
 
     def process_response(self, request, response):

lms/djangoapps/ccx/overrides.py

@@ -7,6 +7,8 @@ import logging
 
 from django.db import transaction, IntegrityError
 
+import request_cache
+
 from courseware.field_overrides import FieldOverrideProvider  # pylint: disable=import-error
 from opaque_keys.edx.keys import CourseKey, UsageKey
 from ccx_keys.locator import CCXLocator, CCXBlockUsageLocator
@@ -69,7 +71,11 @@ def get_current_ccx(course_key):
     if not isinstance(course_key, CCXLocator):
         return None
 
-    return CustomCourseForEdX.objects.get(pk=course_key.ccx)
+    ccx_cache = request_cache.get_cache('ccx')
+    if course_key not in ccx_cache:
+        ccx_cache[course_key] = CustomCourseForEdX.objects.get(pk=course_key.ccx)
+
+    return ccx_cache[course_key]
 
 
 def get_override_for_ccx(ccx, block, name, default=None):
@@ -78,35 +84,40 @@ def get_override_for_ccx(ccx, block, name, default=None):
     specify the block and the name of the field.  If the field is not
     overridden for the given ccx, returns `default`.
     """
-    if not hasattr(block, '_ccx_overrides'):
-        block._ccx_overrides = {}  # pylint: disable=protected-access
-    overrides = block._ccx_overrides.get(ccx.id)  # pylint: disable=protected-access
-    if overrides is None:
-        overrides = _get_overrides_for_ccx(ccx, block)
-        block._ccx_overrides[ccx.id] = overrides  # pylint: disable=protected-access
-    return overrides.get(name, default)
+    overrides = _get_overrides_for_ccx(ccx)
+
+    if isinstance(block.location, CCXBlockUsageLocator):
+        non_ccx_key = block.location.to_block_locator()
+    else:
+        non_ccx_key = block.location
+
+    block_overrides = overrides.get(non_ccx_key, {})
+    if name in block_overrides:
+        return block.fields[name].from_json(block_overrides[name])
+    else:
+        return default
 
 
-def _get_overrides_for_ccx(ccx, block):
+def _get_overrides_for_ccx(ccx):
     """
     Returns a dictionary mapping field name to overriden value for any
     overrides set on this block for this CCX.
     """
-    overrides = {}
-    # block as passed in may have a location specific to a CCX, we must strip
-    # that for this query
-    location = block.location
-    if isinstance(block.location, CCXBlockUsageLocator):
-        location = block.location.to_block_locator()
-    query = CcxFieldOverride.objects.filter(
-        ccx=ccx,
-        location=location
-    )
-    for override in query:
-        field = block.fields[override.field]
-        value = field.from_json(json.loads(override.value))
-        overrides[override.field] = value
-    return overrides
+    overrides_cache = request_cache.get_cache('ccx-overrides')
+
+    if ccx not in overrides_cache:
+        overrides = {}
+        query = CcxFieldOverride.objects.filter(
+            ccx=ccx,
+        )
+
+        for override in query:
+            block_overrides = overrides.setdefault(override.location, {})
+            block_overrides[override.field] = json.loads(override.value)
+
+        overrides_cache[ccx] = overrides
+
+    return overrides_cache[ccx]
 
 
 @transaction.commit_on_success
@@ -117,23 +128,25 @@ def override_field_for_ccx(ccx, block, name, value):
     value to set for the given field.
     """
     field = block.fields[name]
-    value = json.dumps(field.to_json(value))
+    value_json = field.to_json(value)
+    serialized_value = json.dumps(value_json)
     try:
         override = CcxFieldOverride.objects.create(
             ccx=ccx,
             location=block.location,
             field=name,
-            value=value)
+            value=serialized_value
+        )
     except IntegrityError:
         transaction.commit()
         override = CcxFieldOverride.objects.get(
             ccx=ccx,
             location=block.location,
-            field=name)
-        override.value = value
+            field=name
+        )
+        override.value = serialized_value
     override.save()
-    if hasattr(block, '_ccx_overrides'):
-        del block._ccx_overrides[ccx.id]  # pylint: disable=protected-access
+    _get_overrides_for_ccx(ccx).setdefault(block.location, {})[name] = value_json
 
 
 def clear_override_for_ccx(ccx, block, name):
@@ -149,8 +162,7 @@ def clear_override_for_ccx(ccx, block, name):
             location=block.location,
             field=name).delete()
 
-        if hasattr(block, '_ccx_overrides'):
-            del block._ccx_overrides[ccx.id]  # pylint: disable=protected-access
+        _get_overrides_for_ccx(ccx).setdefault(block.location, {}).pop(name)
 
     except CcxFieldOverride.DoesNotExist:
         pass

lms/djangoapps/ccx/tests/factories.py

 """
 Dummy factories for tests
 """
+from factory import SubFactory
 from factory.django import DjangoModelFactory
+from student.tests.factories import UserFactory
 from ccx.models import CustomCourseForEdX  # pylint: disable=import-error
 from ccx.models import CcxMembership  # pylint: disable=import-error
 from ccx.models import CcxFutureMembership  # pylint: disable=import-error
@@ -11,6 +13,7 @@ class CcxFactory(DjangoModelFactory):  # pylint: disable=missing-docstring
     FACTORY_FOR = CustomCourseForEdX
     display_name = "Test CCX"
     id = None  # pylint: disable=redefined-builtin, invalid-name
+    coach = SubFactory(UserFactory)
 
 
 class CcxMembershipFactory(DjangoModelFactory):  # pylint: disable=missing-docstring

lms/djangoapps/ccx/tests/test_field_override_performance.py

@@ -5,6 +5,7 @@ Performance tests for field overrides.
 import ddt
 import itertools
 import mock
+from nose.plugins.skip import SkipTest
 
 from courseware.views import progress  # pylint: disable=import-error
 from courseware.field_overrides import OverrideFieldData
@@ -25,6 +26,8 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, \
     TEST_DATA_SPLIT_MODULESTORE, TEST_DATA_MONGO_MODULESTORE
 from xmodule.modulestore.tests.factories import check_mongo_calls, CourseFactory, check_sum_of_calls
 from xmodule.modulestore.tests.utils import ProceduralCourseTestMixin
+from ccx_keys.locator import CCXLocator
+from ccx.tests.factories import CcxFactory, CcxMembershipFactory
 
 
 @attr('shard_1')
@@ -58,6 +61,7 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin,
         self.request = self.request_factory.get("foo")
         self.request.user = self.student
         self.course = None
+        self.ccx = None
 
         MakoMiddleware().process_request(self.request)
 
@@ -113,17 +117,42 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin,
             self.course.id
         )
 
-    def grade_course(self, course):
+        if enable_ccx:
+            self.ccx = CcxFactory.create()
+            CcxMembershipFactory.create(
+                student=self.student,
+                ccx=self.ccx
+            )
+
+    def grade_course(self, course, view_as_ccx):
         """
         Renders the progress page for the given course.
         """
+        course_key = course.id
+        if view_as_ccx:
+            course_key = CCXLocator.from_course_locator(course_key, self.ccx.id)
+
         return progress(
             self.request,
-            course_id=course.id.to_deprecated_string(),
+            course_id=unicode(course_key),
             student_id=self.student.id
         )
 
-    def instrument_course_progress_render(self, course_width, enable_ccx, queries, reads, xblocks):
+    def assertMongoCallCount(self, calls):
+        """
+        Assert that mongodb is queried ``calls`` times in the surrounded
+        context.
+        """
+        return check_mongo_calls(calls)
+
+    def assertXBlockInstantiations(self, instantiations):
+        """
+        Assert that exactly ``instantiations`` XBlocks are instantiated in
+        the surrounded context.
+        """
+        return check_sum_of_calls(XBlock, ['__init__'], instantiations, instantiations, include_arguments=False)
+
+    def instrument_course_progress_render(self, course_width, enable_ccx, view_as_ccx, queries, reads, xblocks):
         """
         Renders the progress page, instrumenting Mongo reads and SQL queries.
         """
@@ -146,16 +175,16 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin,
             OverrideFieldData.provider_classes = None
 
             with self.assertNumQueries(queries):
-                with check_mongo_calls(reads):
-                    with check_sum_of_calls(XBlock, ['__init__'], xblocks, xblocks, include_arguments=False):
-                        self.grade_course(self.course)
+                with self.assertMongoCallCount(reads):
+                    with self.assertXBlockInstantiations(xblocks):
+                        self.grade_course(self.course, view_as_ccx)
 
-    @ddt.data(*itertools.product(('no_overrides', 'ccx'), range(1, 4), (True, False)))
+    @ddt.data(*itertools.product(('no_overrides', 'ccx'), range(1, 4), (True, False), (True, False)))
     @ddt.unpack
     @override_settings(
         FIELD_OVERRIDE_PROVIDERS=(),
     )
-    def test_field_overrides(self, overrides, course_width, enable_ccx):
+    def test_field_overrides(self, overrides, course_width, enable_ccx, view_as_ccx):
         """
         Test without any field overrides.
         """
@@ -163,9 +192,18 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin,
             'no_overrides': (),
             'ccx': ('ccx.overrides.CustomCoursesForEdxOverrideProvider',)
         }
+        if overrides == 'no_overrides' and view_as_ccx:
+            raise SkipTest("Can't view a ccx course if field overrides are disabled.")
+
+        if not enable_ccx and view_as_ccx:
+            raise SkipTest("Can't view a ccx course if ccx is disabled on the course")
+
+        if self.MODULESTORE == TEST_DATA_MONGO_MODULESTORE and view_as_ccx:
+            raise SkipTest("Can't use a MongoModulestore test as a CCX course")
+
         with self.settings(FIELD_OVERRIDE_PROVIDERS=providers[overrides]):
-            queries, reads, xblocks = self.TEST_DATA[(overrides, course_width, enable_ccx)]
-            self.instrument_course_progress_render(course_width, enable_ccx, queries, reads, xblocks)
+            queries, reads, xblocks = self.TEST_DATA[(overrides, course_width, enable_ccx, view_as_ccx)]
+            self.instrument_course_progress_render(course_width, enable_ccx, view_as_ccx, queries, reads, xblocks)
 
 
 class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase):
@@ -176,19 +214,25 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase):
     __test__ = True
 
     TEST_DATA = {
-        # (providers, course_width, enable_ccx): # of sql queries, # of mongo queries, # of xblocks
-        ('no_overrides', 1, True): (23, 7, 14),
-        ('no_overrides', 2, True): (68, 7, 85),
-        ('no_overrides', 3, True): (263, 7, 336),
-        ('ccx', 1, True): (23, 7, 14),
-        ('ccx', 2, True): (68, 7, 85),
-        ('ccx', 3, True): (263, 7, 336),
-        ('no_overrides', 1, False): (23, 7, 14),
-        ('no_overrides', 2, False): (68, 7, 85),
-        ('no_overrides', 3, False): (263, 7, 336),
-        ('ccx', 1, False): (23, 7, 14),
-        ('ccx', 2, False): (68, 7, 85),
-        ('ccx', 3, False): (263, 7, 336),
+        # (providers, course_width, enable_ccx, view_as_ccx): # of sql queries, # of mongo queries, # of xblocks
+        ('no_overrides', 1, True, False): (23, 7, 14),
+        ('no_overrides', 2, True, False): (68, 7, 85),
+        ('no_overrides', 3, True, False): (263, 7, 336),
+        ('ccx', 1, True, False): (23, 7, 14),
+        ('ccx', 2, True, False): (68, 7, 85),
+        ('ccx', 3, True, False): (263, 7, 336),
+        ('ccx', 1, True, True): (23, 7, 14),
+        ('ccx', 2, True, True): (68, 7, 85),
+        ('ccx', 3, True, True): (263, 7, 336),
+        ('no_overrides', 1, False, False): (23, 7, 14),
+        ('no_overrides', 2, False, False): (68, 7, 85),
+        ('no_overrides', 3, False, False): (263, 7, 336),
+        ('ccx', 1, False, False): (23, 7, 14),
+        ('ccx', 2, False, False): (68, 7, 85),
+        ('ccx', 3, False, False): (263, 7, 336),
+        ('ccx', 1, False, True): (23, 7, 14),
+        ('ccx', 2, False, True): (68, 7, 85),
+        ('ccx', 3, False, True): (263, 7, 336),
     }
 
 
@@ -200,16 +244,22 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase):
     __test__ = True
 
     TEST_DATA = {
-        ('no_overrides', 1, True): (23, 4, 9),
-        ('no_overrides', 2, True): (68, 19, 54),
-        ('no_overrides', 3, True): (263, 84, 215),
-        ('ccx', 1, True): (23, 4, 9),
-        ('ccx', 2, True): (68, 19, 54),
-        ('ccx', 3, True): (263, 84, 215),
-        ('no_overrides', 1, False): (23, 4, 9),
-        ('no_overrides', 2, False): (68, 19, 54),
-        ('no_overrides', 3, False): (263, 84, 215),
-        ('ccx', 1, False): (23, 4, 9),
-        ('ccx', 2, False): (68, 19, 54),
-        ('ccx', 3, False): (263, 84, 215),
+        ('no_overrides', 1, True, False): (23, 4, 9),
+        ('no_overrides', 2, True, False): (68, 19, 54),
+        ('no_overrides', 3, True, False): (263, 84, 215),
+        ('ccx', 1, True, False): (23, 4, 9),
+        ('ccx', 2, True, False): (68, 19, 54),
+        ('ccx', 3, True, False): (263, 84, 215),
+        ('ccx', 1, True, True): (25, 4, 13),
+        ('ccx', 2, True, True): (70, 19, 84),
+        ('ccx', 3, True, True): (265, 84, 335),
+        ('no_overrides', 1, False, False): (23, 4, 9),
+        ('no_overrides', 2, False, False): (68, 19, 54),
+        ('no_overrides', 3, False, False): (263, 84, 215),
+        ('ccx', 1, False, False): (23, 4, 9),
+        ('ccx', 2, False, False): (68, 19, 54),
+        ('ccx', 3, False, False): (263, 84, 215),
+        ('ccx', 1, False, True): (23, 4, 9),
+        ('ccx', 2, False, True): (68, 19, 54),
+        ('ccx', 3, False, True): (263, 84, 215),
     }

lms/djangoapps/ccx/tests/test_overrides.py

@@ -9,6 +9,7 @@ from nose.plugins.attrib import attr
 
 from courseware.field_overrides import OverrideFieldData  # pylint: disable=import-error
 from django.test.utils import override_settings
+from request_cache.middleware import RequestCache
 from student.tests.factories import AdminFactory  # pylint: disable=import-error
 from xmodule.modulestore.tests.django_utils import (
     ModuleStoreTestCase,
@@ -66,6 +67,8 @@ class TestFieldOverrides(ModuleStoreTestCase):
         get_ccx.return_value = ccx
         self.addCleanup(patch.stop)
 
+        self.addCleanup(RequestCache.clear_request_cache)
+
         # Apparently the test harness doesn't use LmsFieldStorage, and I'm not
         # sure if there's a way to poke the test harness to do so.  So, we'll
         # just inject the override field storage in this brute force manner.
@@ -97,7 +100,7 @@ class TestFieldOverrides(ModuleStoreTestCase):
         """
         ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC)
         chapter = self.ccx.course.get_children()[0]
-        with self.assertNumQueries(4):
+        with self.assertNumQueries(3):
             override_field_for_ccx(self.ccx, chapter, 'start', ccx_start)
             dummy = chapter.start
 
@@ -107,7 +110,7 @@ class TestFieldOverrides(ModuleStoreTestCase):
         """
         ccx_start = datetime.datetime(2014, 12, 25, 00, 00, tzinfo=pytz.UTC)
         chapter = self.ccx.course.get_children()[0]
-        with self.assertNumQueries(4):
+        with self.assertNumQueries(3):
             override_field_for_ccx(self.ccx, chapter, 'start', ccx_start)
             dummy1 = chapter.start
             dummy2 = chapter.start

lms/djangoapps/ccx/tests/test_views.py

@@ -21,6 +21,7 @@ from django.test.utils import override_settings
 from django.test import RequestFactory
 from edxmako.shortcuts import render_to_response  # pylint: disable=import-error
 from student.models import CourseEnrollment
+from request_cache.middleware import RequestCache
 from student.roles import CourseCcxCoachRole  # pylint: disable=import-error
 from student.tests.factories import (  # pylint: disable=import-error
     AdminFactory,
@@ -503,26 +504,6 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase):
         role.add_users(coach)
         ccx = CcxFactory(course_id=course.id, coach=self.coach)
 
-        # Apparently the test harness doesn't use LmsFieldStorage, and I'm not
-        # sure if there's a way to poke the test harness to do so.  So, we'll
-        # just inject the override field storage in this brute force manner.
-        OverrideFieldData.provider_classes = None
-        # pylint: disable=protected-access
-        for block in iter_blocks(course):
-            block._field_data = OverrideFieldData.wrap(coach, course, block._field_data)
-            new_cache = {'tabs': [], 'discussion_topics': []}
-            if 'grading_policy' in block._field_data_cache:
-                new_cache['grading_policy'] = block._field_data_cache['grading_policy']
-            block._field_data_cache = new_cache
-
-        def cleanup_provider_classes():
-            """
-            After everything is done, clean up by un-doing the change to the
-            OverrideFieldData object that is done during the wrap method.
-            """
-            OverrideFieldData.provider_classes = None
-        self.addCleanup(cleanup_provider_classes)
-
         # override course grading policy and make last section invisible to students
         override_field_for_ccx(ccx, course, 'grading_policy', {
             'GRADER': [
@@ -561,9 +542,13 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase):
 
         self.client.login(username=coach.username, password="test")
 
+        self.addCleanup(RequestCache.clear_request_cache)
+
     @patch('ccx.views.render_to_response', intercept_renderer)
     def test_gradebook(self):
         self.course.enable_ccx = True
+        RequestCache.clear_request_cache()
+
         url = reverse(
             'ccx_gradebook',
             kwargs={'course_id': self.ccx_key}
@@ -580,6 +565,8 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase):
 
     def test_grades_csv(self):
         self.course.enable_ccx = True
+        RequestCache.clear_request_cache()
+
         url = reverse(
             'ccx_grades_csv',
             kwargs={'course_id': self.ccx_key}
@@ -591,11 +578,11 @@ class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase):
             response.content.strip().split('\n')
         )
         data = dict(zip(headers, row))
+        self.assertTrue('HW 04' not in data)
         self.assertEqual(data['HW 01'], '0.75')
         self.assertEqual(data['HW 02'], '0.5')
         self.assertEqual(data['HW 03'], '0.25')
         self.assertEqual(data['HW Avg'], '0.5')
-        self.assertTrue('HW 04' not in data)
 
     @patch('courseware.views.render_to_response', intercept_renderer)
     def test_student_progress(self):