diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 34a8575ab7..0607cc0c06 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/django.py b/common/lib/xmodule/xmodule/modulestore/django.py index 798b5e110e..e6179f133a 100644 --- a/common/lib/xmodule/xmodule/modulestore/django.py +++ b/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. diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 1c7e799338..0f726bbaf0 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/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-- diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index d43f6f3e0b..601477563f 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/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) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 46cc461614..263715e5ae 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/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), } diff --git a/lms/djangoapps/courseware/tests/test_course_info.py b/lms/djangoapps/courseware/tests/test_course_info.py index ccb7ebacca..92b574e75a 100644 --- a/lms/djangoapps/courseware/tests/test_course_info.py +++ b/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) diff --git a/lms/djangoapps/courseware/tests/test_entrance_exam.py b/lms/djangoapps/courseware/tests/test_entrance_exam.py index 8d05bcc606..904b7aa16a 100644 --- a/lms/djangoapps/courseware/tests/test_entrance_exam.py +++ b/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) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 552297aa1e..f078157e47 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/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 diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 8a1f21e738..69cde1635c 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/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): diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index 03d823c8e4..3b3f3ebd6e 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/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 diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index 813066f3e9..de82c41f0b 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/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), diff --git a/lms/djangoapps/grades/tests/test_grades.py b/lms/djangoapps/grades/tests/test_grades.py index 2acbb82035..162c7f55f0 100644 --- a/lms/djangoapps/grades/tests/test_grades.py +++ b/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) diff --git a/openedx/core/djangoapps/bookmarks/tests/test_api.py b/openedx/core/djangoapps/bookmarks/tests/test_api.py index 6484047eba..2de3d68f05 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_api.py +++ b/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( diff --git a/openedx/core/djangoapps/bookmarks/tests/test_models.py b/openedx/core/djangoapps/bookmarks/tests/test_models.py index 2c3072ad5e..fd5b9b62bb 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_models.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_models.py @@ -48,9 +48,9 @@ class BookmarksTestsBase(ModuleStoreTestCase): self.admin = AdminFactory() self.user = UserFactory.create(password=self.TEST_PASSWORD) self.other_user = UserFactory.create(password=self.TEST_PASSWORD) - self.setup_test_data(self.STORE_TYPE) + self.setup_data(self.STORE_TYPE) - def setup_test_data(self, store_type=ModuleStoreEnum.Type.mongo): + def setup_data(self, store_type=ModuleStoreEnum.Type.mongo): """ Create courses and add some test blocks. """ with self.store.default_store(store_type): @@ -273,7 +273,7 @@ class BookmarkModelTests(BookmarksTestsBase): is needed to fetch the parent blocks. """ - self.setup_test_data(store_type) + self.setup_data(store_type) user = UserFactory.create() expected_path = [PathItem( diff --git a/openedx/core/djangoapps/bookmarks/tests/test_services.py b/openedx/core/djangoapps/bookmarks/tests/test_services.py index 4be7d32207..d06ba7059e 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_services.py +++ b/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): diff --git a/openedx/core/djangoapps/bookmarks/tests/test_tasks.py b/openedx/core/djangoapps/bookmarks/tests/test_tasks.py index c8a789d74a..e6c3c25c16 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_tasks.py +++ b/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) diff --git a/openedx/core/djangoapps/credit/tests/test_api.py b/openedx/core/djangoapps/credit/tests/test_api.py index 94d47fc891..ab82cc9272 100644 --- a/openedx/core/djangoapps/credit/tests/test_api.py +++ b/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,