From fbaa2e5a68374502d10de95faf60cdb009f83715 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 27 May 2023 11:57:18 -0700 Subject: [PATCH] fix: make the initialization more like how it was in the past --- lms/djangoapps/grades/tests/test_tasks.py | 8 ++++---- .../tests/views/test_instructor_dashboard.py | 2 +- .../tests/test_course_overviews.py | 2 +- .../split_mongo/caching_descriptor_system.py | 20 +++++++++++-------- .../tests/test_mongo_call_count.py | 6 +++--- xmodule/x_module.py | 13 ++++++++++++ 6 files changed, 34 insertions(+), 17 deletions(-) diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 475cd6032b..4c4f60d731 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -153,8 +153,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest assert mock_block_structure_create.call_count == 1 @ddt.data( - (ModuleStoreEnum.Type.split, 1, 41, True), - (ModuleStoreEnum.Type.split, 1, 41, False), + (ModuleStoreEnum.Type.split, 2, 41, True), + (ModuleStoreEnum.Type.split, 2, 41, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -165,7 +165,7 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.split, 1, 41), + (ModuleStoreEnum.Type.split, 2, 41), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -210,7 +210,7 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest ) @ddt.data( - (ModuleStoreEnum.Type.split, 1, 41), + (ModuleStoreEnum.Type.split, 2, 41), ) @ddt.unpack def test_persistent_grades_on_course(self, default_store, num_mongo_queries, num_sql_queries): diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index f27963f068..bca16977a9 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -683,6 +683,6 @@ class TestInstructorDashboardPerformance(ModuleStoreTestCase, LoginEnrollmentTes # check MongoDB calls count url = reverse('spoc_gradebook', kwargs={'course_id': self.course.id}) - with check_mongo_calls(5): + with check_mongo_calls(6): response = self.client.get(url) assert response.status_code == 200 diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py index 66e2936a40..b65ec15989 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py @@ -379,7 +379,7 @@ class CourseOverviewTestCase(CatalogIntegrationMixin, ModuleStoreTestCase, Cache course_overview = CourseOverview._create_or_update(course) # pylint: disable=protected-access assert course_overview.lowest_passing_grade is None - @ddt.data((ModuleStoreEnum.Type.mongo, 5, 5), (ModuleStoreEnum.Type.split, 1, 1)) + @ddt.data((ModuleStoreEnum.Type.mongo, 5, 5), (ModuleStoreEnum.Type.split, 2, 2)) @ddt.unpack def test_versioning(self, modulestore_type, min_mongo_calls, max_mongo_calls): """ diff --git a/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/xmodule/modulestore/split_mongo/caching_descriptor_system.py index 68286be73d..b3234df5a2 100644 --- a/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -75,7 +75,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # li self.default_class = default_class self.local_modules = {} self._services['library_tools'] = LibraryToolsService(modulestore, user_id=None) - # Cache of block field datas, keyed by their ScopeId tuples + # Cache of block field datas, keyed by the XBlock instance (since the ScopeId changes!) self.block_field_datas = weakref.WeakKeyDictionary() @lazy @@ -185,7 +185,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # li block = self.construct_xblock_from_class( class_, ScopeIds(None, block_key.type, definition_id, block_locator), - for_parent=kwargs.get('for_parent') + for_parent=kwargs.get('for_parent'), + # Pass this tuple on for use by _init_field_data_for_block() when field data is initialized. + cds_init_args=(block_data, definition_id, kwargs.get("field_decorator")), # Passing field_data here is deprecated, so we don't. Get it via block.service(block, "field-data"). # https://github.com/openedx/XBlock/blob/e89cbc5/xblock/mixins.py#L200-L207 ) @@ -244,11 +246,13 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # li block_key = BlockKey.from_usage_key(block.scope_ids.usage_id) course_key = block.scope_ids.usage_id.context_key try: - block_data = self.get_module_data(block_key, course_key) - definition_id = block_data.definition - except ItemNotFoundError: + (block_data, definition_id, field_decorator) = block.get_cds_init_args() + except KeyError: + # This block was not instantiate via xblock_from_json()/modulestore but rather via the "direct" XBlock API + # such as using construct_xblock_from_class(). It probably doesn't yet exist in modulestore. block_data = BlockData() - definition_id = None # Perhaps this block was newly created. + definition_id = LocalId() + field_decorator = None class_ = self.load_block_type(block.scope_ids.block_type) convert_fields = lambda field: self.modulestore.convert_references_to_keys( course_key, class_, field, self.course_entry.structure['blocks'], @@ -274,7 +278,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # li except AttributeError: pass - if definition_id is not None and not block_data.definition_loaded: + if not isinstance(definition_id, LocalId) and not block_data.definition_loaded: definition_loader = DefinitionLazyLoader( self.modulestore, course_key, @@ -291,7 +295,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # li converted_defaults, parent=parent, aside_fields=aside_fields, - # field_decorator=kwargs.get('field_decorator') # FIXME: Need to get the field_decorator + field_decorator=field_decorator, ) if InheritanceMixin in self.modulestore.xblock_mixins: diff --git a/xmodule/modulestore/tests/test_mongo_call_count.py b/xmodule/modulestore/tests/test_mongo_call_count.py index 9fe3d5d697..01425d0e2d 100644 --- a/xmodule/modulestore/tests/test_mongo_call_count.py +++ b/xmodule/modulestore/tests/test_mongo_call_count.py @@ -159,9 +159,9 @@ class CountMongoCallsCourseTraversal(TestCase): (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, True, 37), (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, True, 37), (MIXED_SPLIT_MODULESTORE_BUILDER, None, False, False, 2), - (MIXED_SPLIT_MODULESTORE_BUILDER, None, True, False, 1), + (MIXED_SPLIT_MODULESTORE_BUILDER, None, True, False, 2), (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, False, 2), - (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, False, 1), + (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, False, 2), ) @ddt.unpack def test_number_mongo_calls(self, store_builder, depth, lazy, access_all_block_fields, num_mongo_calls): @@ -178,7 +178,7 @@ class CountMongoCallsCourseTraversal(TestCase): @ddt.data( (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 324), - (MIXED_SPLIT_MODULESTORE_BUILDER, 2), + (MIXED_SPLIT_MODULESTORE_BUILDER, 3), ) @ddt.unpack def test_lazy_when_course_previously_cached(self, store_builder, num_mongo_calls): diff --git a/xmodule/x_module.py b/xmodule/x_module.py index ea3eac4f8a..8ce4739363 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -313,9 +313,22 @@ class XModuleMixin(XModuleFields, XBlock): def __init__(self, *args, **kwargs): self._asides = [] + # Initialization data used by CachingDescriptorSystem to defer FieldData initialization + self._cds_init_args = kwargs.pop("cds_init_args", None) super().__init__(*args, **kwargs) + def get_cds_init_args(self): + """ Get initialization data used by CachingDescriptorSystem to defer FieldData initialization """ + if self._cds_init_args is None: + raise KeyError("cds_init_args was not provided for this XBlock") + if self._cds_init_args is False: + raise RuntimeError("Tried to get CachingDescriptorSystem cds_init_args twice for the same XBlock.") + args = self._cds_init_args + # Free the memory and set this False to flag any double-access bugs. This only needs to be read once. + self._cds_init_args = False + return args + @property def runtime(self): return self._runtime