Use request_cache for storing disabled xblock names.

TNL-5002

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -1170,7 +1170,7 @@ class ModuleStoreReadBase(BulkOperationsMixin, ModuleStoreRead):
         contentstore=None,
         doc_store_config=None,  # ignore if passed up
         metadata_inheritance_cache_subsystem=None, request_cache=None,
-        xblock_mixins=(), xblock_select=None, xblock_field_data_wrappers=(), disabled_xblock_types=(),  # pylint: disable=bad-continuation
+        xblock_mixins=(), xblock_select=None, xblock_field_data_wrappers=(), disabled_xblock_types=lambda: [],  # pylint: disable=bad-continuation
         # temporary parms to enable backward compatibility. remove once all envs migrated
         db=None, collection=None, host=None, port=None, tz_aware=True, user=None, password=None,
         # allow lower level init args to pass harmlessly

common/lib/xmodule/xmodule/modulestore/django.py

@@ -19,7 +19,6 @@ from django.conf import settings
 if not settings.configured:
     settings.configure()
 
-from django.db.models.signals import post_save
 from django.core.cache import caches, InvalidCacheBackendError
 import django.dispatch
 import django.utils
@@ -51,10 +50,8 @@ except ImportError:
 
 try:
     from xblock_django.api import disabled_xblocks
-    from xblock_django.models import XBlockConfiguration
 except ImportError:
     disabled_xblocks = None
-    XBlockConfiguration = None
 
 log = logging.getLogger(__name__)
 ASSET_IGNORE_REGEX = getattr(settings, "ASSET_IGNORE_REGEX", r"(^\._.*$)|(^\.DS_Store$)|(^.*~$)")
@@ -191,13 +188,26 @@ def create_modulestore_instance(
     if 'read_preference' in doc_store_config:
         doc_store_config['read_preference'] = getattr(ReadPreference, doc_store_config['read_preference'])
 
-    if disabled_xblocks:
-        disabled_xblock_types = [block.name for block in disabled_xblocks()]
-    else:
-        disabled_xblock_types = ()
-
     xblock_field_data_wrappers = [load_function(path) for path in settings.XBLOCK_FIELD_DATA_WRAPPERS]
 
+    def fetch_disabled_xblock_types():
+        """
+        Get the disabled xblock names, using the request_cache if possible to avoid hitting
+        a database every time the list is needed.
+        """
+        # If the import could not be loaded, return an empty list.
+        if disabled_xblocks is None:
+            return []
+
+        if request_cache:
+            if 'disabled_xblock_types' not in request_cache.data:
+                request_cache.data['disabled_xblock_types'] = [block.name for block in disabled_xblocks()]
+            return request_cache.data['disabled_xblock_types']
+        else:
+            disabled_xblock_types = [block.name for block in disabled_xblocks()]
+
+        return disabled_xblock_types
+
     return class_(
         contentstore=content_store,
         metadata_inheritance_cache_subsystem=metadata_inheritance_cache,
@@ -205,7 +215,7 @@ def create_modulestore_instance(
         xblock_mixins=getattr(settings, 'XBLOCK_MIXINS', ()),
         xblock_select=getattr(settings, 'XBLOCK_SELECT_FUNCTION', None),
         xblock_field_data_wrappers=xblock_field_data_wrappers,
-        disabled_xblock_types=disabled_xblock_types,
+        disabled_xblock_types=fetch_disabled_xblock_types,
         doc_store_config=doc_store_config,
         i18n_service=i18n_service or ModuleI18nService,
         fs_service=fs_service or xblock.reference.plugins.FSService(),
@@ -255,17 +265,6 @@ def clear_existing_modulestores():
     _MIXED_MODULESTORE = None
 
 
-if XBlockConfiguration and disabled_xblocks:
-    @django.dispatch.receiver(post_save, sender=XBlockConfiguration)
-    def reset_disabled_xblocks(sender, instance, **kwargs):  # pylint: disable=unused-argument
-        """
-        If XBlockConfiguation and disabled_xblocks are available, register a signal handler
-        to update disabled_xblocks on model changes.
-        """
-        disabled_xblock_types = [block.name for block in disabled_xblocks()]
-        modulestore().disabled_xblock_types = disabled_xblock_types
-
-
 class ModuleI18nService(object):
     """
     Implement the XBlock runtime "i18n" service.

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -185,25 +185,6 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
                     self.mappings[course_key] = store
             self.modulestores.append(store)
 
-    @property
-    def disabled_xblock_types(self):
-        """
-        Returns the list of disabled xblock types.
-        """
-        return None if not hasattr(self, "_disabled_xblock_types") else self._disabled_xblock_types
-
-    @disabled_xblock_types.setter
-    def disabled_xblock_types(self, value):
-        """
-        Sets the list of disabled xblock types, and propagates down
-        to child modulestores if available.
-        """
-        self._disabled_xblock_types = value  # pylint: disable=attribute-defined-outside-init
-
-        if hasattr(self, 'modulestores'):
-            for store in self.modulestores:
-                store.disabled_xblock_types = value
-
     def _clean_locator_for_mapping(self, locator):
         """
         In order for mapping to work, the locator must be minimal--no version, no branch--

common/lib/xmodule/xmodule/x_module.py

@@ -1371,7 +1371,7 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime):
     """
     # pylint: disable=bad-continuation
     def __init__(
-        self, load_item, resources_fs, error_tracker, get_policy=None, disabled_xblock_types=(), **kwargs
+        self, load_item, resources_fs, error_tracker, get_policy=None, disabled_xblock_types=lambda: [], **kwargs
     ):
         """
         load_item: Takes a Location and returns an XModuleDescriptor
@@ -1439,7 +1439,7 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime):
         """
         Returns a subclass of :class:`.XBlock` that corresponds to the specified `block_type`.
         """
-        if block_type in self.disabled_xblock_types:
+        if block_type in self.disabled_xblock_types():
             return self.default_class
         return super(DescriptorSystem, self).load_block_type(block_type)
 

lms/djangoapps/ccx/tests/test_field_override_performance.py

@@ -229,18 +229,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase):
         #     # of sql queries to default,
         #     # of mongo queries,
         # )
-        ('no_overrides', 1, True, False): (34, 6),
-        ('no_overrides', 2, True, False): (40, 6),
-        ('no_overrides', 3, True, False): (50, 6),
-        ('ccx', 1, True, False): (34, 6),
-        ('ccx', 2, True, False): (40, 6),
-        ('ccx', 3, True, False): (50, 6),
-        ('no_overrides', 1, False, False): (34, 6),
-        ('no_overrides', 2, False, False): (40, 6),
-        ('no_overrides', 3, False, False): (50, 6),
-        ('ccx', 1, False, False): (34, 6),
-        ('ccx', 2, False, False): (40, 6),
-        ('ccx', 3, False, False): (50, 6),
+        ('no_overrides', 1, True, False): (35, 6),
+        ('no_overrides', 2, True, False): (41, 6),
+        ('no_overrides', 3, True, False): (51, 6),
+        ('ccx', 1, True, False): (35, 6),
+        ('ccx', 2, True, False): (41, 6),
+        ('ccx', 3, True, False): (51, 6),
+        ('no_overrides', 1, False, False): (35, 6),
+        ('no_overrides', 2, False, False): (41, 6),
+        ('no_overrides', 3, False, False): (51, 6),
+        ('ccx', 1, False, False): (35, 6),
+        ('ccx', 2, False, False): (41, 6),
+        ('ccx', 3, False, False): (51, 6),
     }
 
 
@@ -252,19 +252,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase):
     __test__ = True
 
     TEST_DATA = {
-        ('no_overrides', 1, True, False): (34, 3),
-        ('no_overrides', 2, True, False): (40, 3),
-        ('no_overrides', 3, True, False): (50, 3),
-        ('ccx', 1, True, False): (34, 3),
-        ('ccx', 2, True, False): (40, 3),
-        ('ccx', 3, True, False): (50, 3),
-        ('ccx', 1, True, True): (35, 3),
-        ('ccx', 2, True, True): (41, 3),
-        ('ccx', 3, True, True): (51, 3),
-        ('no_overrides', 1, False, False): (34, 3),
-        ('no_overrides', 2, False, False): (40, 3),
-        ('no_overrides', 3, False, False): (50, 3),
-        ('ccx', 1, False, False): (34, 3),
-        ('ccx', 2, False, False): (40, 3),
-        ('ccx', 3, False, False): (50, 3),
+        ('no_overrides', 1, True, False): (35, 3),
+        ('no_overrides', 2, True, False): (41, 3),
+        ('no_overrides', 3, True, False): (51, 3),
+        ('ccx', 1, True, False): (35, 3),
+        ('ccx', 2, True, False): (41, 3),
+        ('ccx', 3, True, False): (51, 3),
+        ('ccx', 1, True, True): (36, 3),
+        ('ccx', 2, True, True): (42, 3),
+        ('ccx', 3, True, True): (52, 3),
+        ('no_overrides', 1, False, False): (35, 3),
+        ('no_overrides', 2, False, False): (41, 3),
+        ('no_overrides', 3, False, False): (51, 3),
+        ('ccx', 1, False, False): (35, 3),
+        ('ccx', 2, False, False): (41, 3),
+        ('ccx', 3, False, False): (51, 3),
     }

lms/djangoapps/courseware/tests/test_course_info.py

@@ -317,7 +317,7 @@ class SelfPacedCourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTest
         self.assertEqual(resp.status_code, 200)
 
     def test_num_queries_instructor_paced(self):
-        self.fetch_course_info_with_queries(self.instructor_paced_course, 22, 4)
+        self.fetch_course_info_with_queries(self.instructor_paced_course, 23, 4)
 
     def test_num_queries_self_paced(self):
-        self.fetch_course_info_with_queries(self.self_paced_course, 22, 4)
+        self.fetch_course_info_with_queries(self.self_paced_course, 23, 4)

lms/djangoapps/courseware/tests/test_entrance_exam.py

@@ -293,7 +293,9 @@ class EntranceExamTestCases(LoginEnrollmentTestCase, ModuleStoreTestCase, Milest
         """
         test entrance exam score. we will hit the method get_entrance_exam_score to verify exam score.
         """
-        with self.assertNumQueries(1):
+        # One query is for getting the list of disabled XBlocks (which is
+        # then stored in the request).
+        with self.assertNumQueries(2):
             exam_score = get_entrance_exam_score(self.request, self.course)
         self.assertEqual(exam_score, 0)
 

lms/djangoapps/courseware/tests/test_module_render.py

@@ -19,7 +19,6 @@ from mock import MagicMock, patch, Mock
 from opaque_keys.edx.keys import UsageKey, CourseKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from pyquery import PyQuery
-from courseware.module_render import hash_resource
 from xblock.field_data import FieldData
 from xblock.runtime import Runtime
 from xblock.fields import ScopeIds
@@ -2225,9 +2224,7 @@ class TestDisabledXBlockTypes(ModuleStoreTestCase):
     # pylint: disable=no-member
     def setUp(self):
         super(TestDisabledXBlockTypes, self).setUp()
-
-        for store in self.store.modulestores:
-            store.disabled_xblock_types = ('video',)
+        XBlockConfiguration(name='video', enabled=False).save()
 
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
     def test_get_item(self, default_ms):
@@ -2240,15 +2237,27 @@ class TestDisabledXBlockTypes(ModuleStoreTestCase):
         """Tests that the list of disabled xblocks can dynamically update."""
         with self.store.default_store(default_ms):
             course = CourseFactory()
-            self._verify_descriptor('problem', course, 'CapaDescriptorWithMixins')
+            item_usage_id = self._verify_descriptor('problem', course, 'CapaDescriptorWithMixins')
             XBlockConfiguration(name='problem', enabled=False).save()
-            self._verify_descriptor('problem', course, 'RawDescriptorWithMixins')
 
-    def _verify_descriptor(self, category, course, descriptor):
+            # First verify that the cached value is used until there is a new request cache.
+            self._verify_descriptor('problem', course, 'CapaDescriptorWithMixins', item_usage_id)
+
+            # Now simulate a new request cache.
+            self.store.request_cache.data = {}
+            self._verify_descriptor('problem', course, 'RawDescriptorWithMixins', item_usage_id)
+
+    def _verify_descriptor(self, category, course, descriptor, item_id=None):
         """
         Helper method that gets an item with the specified category from the
         modulestore and verifies that it has the expected descriptor name.
+
+        Returns the item's usage_id.
         """
-        item = ItemFactory(category=category, parent=course)
-        item = self.store.get_item(item.scope_ids.usage_id)
+        if not item_id:
+            item = ItemFactory(category=category, parent=course)
+            item_id = item.scope_ids.usage_id
+
+        item = self.store.get_item(item_id)
         self.assertEqual(item.__class__.__name__, descriptor)
+        return item_id

lms/djangoapps/courseware/tests/test_views.py

@@ -1346,7 +1346,7 @@ class ProgressPageTests(ModuleStoreTestCase):
         self.assertContains(resp, u"Download Your Certificate")
 
     @ddt.data(
-        *itertools.product(((47, 4, True), (47, 4, False)), (True, False))
+        *itertools.product(((48, 4, True), (48, 4, False)), (True, False))
     )
     @ddt.unpack
     def test_query_counts(self, (sql_calls, mongo_calls, self_paced), self_paced_enabled):

lms/djangoapps/django_comment_client/base/tests.py

@@ -372,8 +372,8 @@ class ViewsQueryCountTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet
         return inner
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 3, 4, 31),
-        (ModuleStoreEnum.Type.split, 3, 13, 31),
+        (ModuleStoreEnum.Type.mongo, 3, 4, 32),
+        (ModuleStoreEnum.Type.split, 3, 13, 32),
     )
     @ddt.unpack
     @count_queries
@@ -381,8 +381,8 @@ class ViewsQueryCountTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet
         self.create_thread_helper(mock_request)
 
     @ddt.data(
-        (ModuleStoreEnum.Type.mongo, 3, 3, 25),
-        (ModuleStoreEnum.Type.split, 3, 10, 25),
+        (ModuleStoreEnum.Type.mongo, 3, 3, 26),
+        (ModuleStoreEnum.Type.split, 3, 10, 26),
     )
     @ddt.unpack
     @count_queries

lms/djangoapps/django_comment_client/forum/tests.py

@@ -341,9 +341,14 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
     MODULESTORE = TEST_DATA_MONGO_MODULESTORE
 
     @ddt.data(
-        # old mongo with cache
-        (ModuleStoreEnum.Type.mongo, 1, 6, 4, 17, 8),
-        (ModuleStoreEnum.Type.mongo, 50, 6, 4, 17, 8),
+        # Old mongo with cache. There is an additional SQL query for old mongo
+        # because the first time that disabled_xblocks is queried is in call_single_thread,
+        # vs. the creation of the course (CourseFactory.create). The creation of the
+        # course is outside the context manager that is verifying the number of queries,
+        # and with split mongo, that method ends up querying disabled_xblocks (which is then
+        # cached and hence not queried as part of call_single_thread).
+        (ModuleStoreEnum.Type.mongo, 1, 6, 4, 18, 8),
+        (ModuleStoreEnum.Type.mongo, 50, 6, 4, 18, 8),
         # split mongo: 3 queries, regardless of thread response size.
         (ModuleStoreEnum.Type.split, 1, 3, 3, 17, 8),
         (ModuleStoreEnum.Type.split, 50, 3, 3, 17, 8),

lms/djangoapps/grades/tests/test_grades.py

@@ -380,7 +380,9 @@ class TestGetModuleScore(LoginEnrollmentTestCase, SharedModuleStoreTestCase):
         """
         Test test_get_module_score
         """
-        with self.assertNumQueries(1):
+        # One query is for getting the list of disabled XBlocks (which is
+        # then stored in the request).
+        with self.assertNumQueries(2):
             score = get_module_score(self.request.user, self.course, self.seq1)
         self.assertEqual(score, 0)
 

openedx/core/djangoapps/bookmarks/tests/test_api.py

@@ -123,7 +123,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase):
         """
         self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 2)
 
-        with self.assertNumQueries(9):
+        with self.assertNumQueries(10):
             bookmark_data = api.create_bookmark(user=self.user, usage_key=self.vertical_2.location)
 
         self.assert_bookmark_event_emitted(
@@ -144,7 +144,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase):
         """
         self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 2)
 
-        with self.assertNumQueries(9):
+        with self.assertNumQueries(10):
             bookmark_data = api.create_bookmark(user=self.user, usage_key=self.vertical_2.location)
 
         self.assert_bookmark_event_emitted(

openedx/core/djangoapps/bookmarks/tests/test_services.py

@@ -70,7 +70,7 @@ class BookmarksServiceTests(BookmarksTestsBase):
                 self.bookmark_service.set_bookmarked(usage_key=UsageKey.from_string("i4x://ed/ed/ed/interactive"))
             )
 
-        with self.assertNumQueries(9):
+        with self.assertNumQueries(10):
             self.assertTrue(self.bookmark_service.set_bookmarked(usage_key=self.vertical_2.location))
 
     def test_unset_bookmarked(self):

openedx/core/djangoapps/bookmarks/tests/test_tasks.py

@@ -4,6 +4,8 @@ Tests for tasks.
 import ddt
 from nose.plugins.attrib import attr
 
+from django.conf import settings
+
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.tests.factories import check_mongo_calls, ItemFactory
 
@@ -152,6 +154,12 @@ class XBlockCacheTaskTests(BookmarksTestsBase):
         """
         course = getattr(self, course_attr)
 
+        if settings.ROOT_URLCONF == 'lms.urls':
+            # When the tests run under LMS, there is a certificates course_published
+            # signal handler that runs and causes the number of queries to be one more
+            # (due to the check for disabled_xblocks in django.py).
+            expected_sql_queries += 1
+
         with self.assertNumQueries(expected_sql_queries):
             _update_xblocks_cache(course.id)
 

openedx/core/djangoapps/credit/tests/test_api.py

@@ -557,7 +557,7 @@ class CreditRequirementApiTests(CreditApiTestBase):
         self.assertFalse(api.is_user_eligible_for_credit("bob", self.course_key))
 
         # Satisfy the other requirement
-        with self.assertNumQueries(20):
+        with self.assertNumQueries(21):
             api.set_credit_requirement_status(
                 "bob",
                 self.course_key,