From 2924e564b099a1adb7e9928381fa52d1fb90d5ec Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 19 May 2023 13:55:25 -0700
Subject: [PATCH 01/16] fix: field data should be loaded on-demand, not pushed
 into block init

---
 .../split_mongo/caching_descriptor_system.py  | 148 +++++++++++-------
 1 file changed, 89 insertions(+), 59 deletions(-)

diff --git a/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
index 5cb0bb3571..fa3ace5a16 100644
--- a/xmodule/modulestore/split_mongo/caching_descriptor_system.py
+++ b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
@@ -13,6 +13,7 @@ from xmodule.error_block import ErrorBlock
 from xmodule.errortracker import exc_info_to_str
 from xmodule.library_tools import LibraryToolsService
 from xmodule.mako_block import MakoDescriptorSystem
+from xmodule.modulestore import BlockData
 from xmodule.modulestore.edit_info import EditInfoRuntimeMixin
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.modulestore.inheritance import InheritanceMixin, inheriting_field_data
@@ -73,6 +74,8 @@ 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
+        self.block_field_datas = {}
 
     @lazy
     def _parent_map(self):  # lint-amnesty, pylint: disable=missing-function-docstring
@@ -167,76 +170,19 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
             # most recent retrieval is most likely the right one for next caller (see comment above fn)
             self.course_entry = CourseEnvelope(course_entry_override.course_key, self.course_entry.structure)
 
-        definition_id = block_data.definition
-
-        # If no usage id is provided, generate an in-memory id
-        if block_key is None:
-            block_key = BlockKey(block_data.block_type, LocalId())
-
-        convert_fields = lambda field: self.modulestore.convert_references_to_keys(
-            course_key, class_, field, self.course_entry.structure['blocks'],
-        )
-
-        if definition_id is not None and not block_data.definition_loaded:
-            definition_loader = DefinitionLazyLoader(
-                self.modulestore,
-                course_key,
-                block_key.type,
-                definition_id,
-                convert_fields,
-            )
-        else:
-            definition_loader = None
-
-        # If no definition id is provide, generate an in-memory id
-        if definition_id is None:
-            definition_id = LocalId()
-
         # Construct the Block Usage Locator:
         block_locator = course_key.make_usage_key(
             block_type=block_key.type,
             block_id=block_key.id,
         )
 
-        converted_fields = convert_fields(block_data.fields)
-        converted_defaults = convert_fields(block_data.defaults)
-        if block_key in self._parent_map:
-            parent_key = self._parent_map[block_key]
-            parent = course_key.make_usage_key(parent_key.type, parent_key.id)
-        else:
-            parent = None
-
-        aside_fields = None
-
-        # for the situation if block_data has no asides attribute
-        # (in case it was taken from memcache)
-        try:
-            if block_data.asides:
-                aside_fields = {block_key.type: {}}
-                for aside in block_data.asides:
-                    aside_fields[block_key.type].update(aside['fields'])
-        except AttributeError:
-            pass
+        definition_id = block_data.definition or LocalId()
 
         try:
-            kvs = SplitMongoKVS(
-                definition_loader,
-                converted_fields,
-                converted_defaults,
-                parent=parent,
-                aside_fields=aside_fields,
-                field_decorator=kwargs.get('field_decorator')
-            )
-
-            if InheritanceMixin in self.modulestore.xblock_mixins:
-                field_data = inheriting_field_data(kvs)
-            else:
-                field_data = KvsFieldData(kvs)
-
             block = self.construct_xblock_from_class(
                 class_,
                 ScopeIds(None, block_key.type, definition_id, block_locator),
-                field_data,
+                field_data=None,
                 for_parent=kwargs.get('for_parent')
             )
         except Exception:  # pylint: disable=broad-except
@@ -271,6 +217,90 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
 
         return block
 
+    def service(self, block, service_name):
+        """
+        Return a service, or None.
+        Services are objects implementing arbitrary other interfaces.
+        """
+        # Implement field data service:
+        if service_name == "field-data":
+            if block.scope_ids not in self.block_field_datas:
+                try:
+                    self.block_field_datas[block.scope_ids] = self._init_field_data_for_block(block)
+                except:
+                    # Don't try again pointlessly every time another field is accessed
+                    self.block_field_datas[block.scope_ids] = None
+                    raise
+            return self.block_field_datas[block.scope_ids]
+        return super().service(block, service_name)
+
+    def _init_field_data_for_block(self, block):
+        """
+        Initialize the field-data service for the given block.
+        """
+        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 = BlockData()
+            definition_id = None  # Perhaps this block was newly created.
+        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'],
+        )
+
+        if definition_id is not None and not block_data.definition_loaded:
+            definition_loader = DefinitionLazyLoader(
+                self.modulestore,
+                course_key,
+                block_key.type,
+                definition_id,
+                convert_fields,
+            )
+        else:
+            definition_loader = None
+
+        # If no definition id is provide, generate an in-memory id
+        if definition_id is None:
+            definition_id = LocalId()
+
+        converted_fields = convert_fields(block_data.fields)
+        converted_defaults = convert_fields(block_data.defaults)
+        if block_key in self._parent_map:
+            parent_key = self._parent_map[block_key]
+            parent = course_key.make_usage_key(parent_key.type, parent_key.id)
+        else:
+            parent = None
+
+        aside_fields = None
+
+        # for the situation if block_data has no asides attribute
+        # (in case it was taken from memcache)
+        try:
+            if block_data.asides:
+                aside_fields = {block_key.type: {}}
+                for aside in block_data.asides:
+                    aside_fields[block_key.type].update(aside['fields'])
+        except AttributeError:
+            pass
+
+        kvs = SplitMongoKVS(
+            definition_loader,
+            converted_fields,
+            converted_defaults,
+            parent=parent,
+            aside_fields=aside_fields,
+            # field_decorator=kwargs.get('field_decorator') # FIXME: Need to get the field_decorator
+        )
+
+        if InheritanceMixin in self.modulestore.xblock_mixins:
+            field_data = inheriting_field_data(kvs)
+        else:
+            field_data = KvsFieldData(kvs)
+        return field_data
+
     def get_edited_by(self, xblock):
         """
         See :meth: cms.lib.xblock.runtime.EditInfoRuntimeMixin.get_edited_by

From f05f48a156e9e91b13f67212a7085b4bfc9dab77 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 19 May 2023 15:41:49 -0700
Subject: [PATCH 02/16] temp: continued

---
 .../modulestore/split_mongo/caching_descriptor_system.py | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
index fa3ace5a16..819cffddc5 100644
--- a/xmodule/modulestore/split_mongo/caching_descriptor_system.py
+++ b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
@@ -205,12 +205,6 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
         block.source_version = edit_info.source_version
         block.definition_locator = DefinitionLocator(block_key.type, definition_id)
 
-        for wrapper in self.modulestore.xblock_field_data_wrappers:
-            block._field_data = wrapper(block, block._field_data)  # pylint: disable=protected-access
-
-        # decache any pending field settings
-        block.save()
-
         # If this is an in-memory block, store it in this system
         if isinstance(block_locator.block_id, LocalId):
             self.local_modules[block_locator] = block
@@ -299,6 +293,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
             field_data = inheriting_field_data(kvs)
         else:
             field_data = KvsFieldData(kvs)
+
+        for wrapper in self.modulestore.xblock_field_data_wrappers:
+            field_data = wrapper(block, field_data)
         return field_data
 
     def get_edited_by(self, xblock):

From db490866e09dc02acc117f63966333a3346e144a Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Tue, 23 May 2023 10:29:35 -0700
Subject: [PATCH 03/16] temp: update test case, uses one less query since
 field-data is on demand

---
 .../content/course_overviews/tests/test_course_overviews.py     | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 b65ec15989..66e2936a40 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, 2, 2))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 5, 5), (ModuleStoreEnum.Type.split, 1, 1))
     @ddt.unpack
     def test_versioning(self, modulestore_type, min_mongo_calls, max_mongo_calls):
         """

From 1c09f6c3ef7821782616a522a5334146963c957c Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Tue, 23 May 2023 10:54:23 -0700
Subject: [PATCH 04/16] temp: clarify why we don't pass field_data to
 constructor

---
 xmodule/modulestore/split_mongo/caching_descriptor_system.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
index 819cffddc5..f6b5cdd227 100644
--- a/xmodule/modulestore/split_mongo/caching_descriptor_system.py
+++ b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
@@ -182,8 +182,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
             block = self.construct_xblock_from_class(
                 class_,
                 ScopeIds(None, block_key.type, definition_id, block_locator),
-                field_data=None,
                 for_parent=kwargs.get('for_parent')
+                # 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
             )
         except Exception:  # pylint: disable=broad-except
             log.warning("Failed to load descriptor", exc_info=True)

From 30e7b5b92b6605df55f35e4556f685094c80fa1d Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Tue, 23 May 2023 16:32:09 -0700
Subject: [PATCH 05/16] temp: continue

---
 .../split_mongo/caching_descriptor_system.py  | 21 +++++++++++++++----
 xmodule/x_module.py                           | 21 +++++++++----------
 2 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
index f6b5cdd227..eb6f43f266 100644
--- a/xmodule/modulestore/split_mongo/caching_descriptor_system.py
+++ b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
@@ -218,10 +218,15 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
         Services are objects implementing arbitrary other interfaces.
         """
         # Implement field data service:
-        if service_name == "field-data":
+        if service_name in ('field-data', 'field-data-unbound'):
+            if hasattr(block, "_bound_field_data") and service_name != "field-data-unbound":
+                # Return the user-specific wrapped field data that gets set onto the block during XModule.bind_for_user
+                # This complexity is due to XModule heritage. If we didn't load the block as one step, then sometimes
+                # "rebind" it to be user-specific as a later step, we could load the field data and overrides at once.
+                return block._bound_field_data  # pylint: disable=protected-access
             if block.scope_ids not in self.block_field_datas:
                 try:
-                    self.block_field_datas[block.scope_ids] = self._init_field_data_for_block(block)
+                    self._init_field_data_for_block(block)
                 except:
                     # Don't try again pointlessly every time another field is accessed
                     self.block_field_datas[block.scope_ids] = None
@@ -295,9 +300,17 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
         else:
             field_data = KvsFieldData(kvs)
 
+        # Save the field_data now, because some of the wrappers will try to read fields from the block while they
+        # determine if they want to wrap the field_data or not.
+        self.block_field_datas[block.scope_ids] = field_data
+        # Now add in any wrappers that want to be added:
         for wrapper in self.modulestore.xblock_field_data_wrappers:
-            field_data = wrapper(block, field_data)
-        return field_data
+            self.block_field_datas[block.scope_ids] = wrapper(block, self.block_field_datas[block.scope_ids])
+
+        # Note: user-specific wrappers like LmsFieldData or OverrideFieldData do not get set here. They get set later
+        # during bind_for_student(), which wraps the field-data and sets block._bound_field_data. Calling
+        # runtime.service(block, "field-data") or block._field_data (deprecated) will load block._bound_field_data if
+        # the block has been bound to a user, otherwise it returns the wrapped field data we just created above.
 
     def get_edited_by(self, xblock):
         """
diff --git a/xmodule/x_module.py b/xmodule/x_module.py
index 5478ccc45d..f61dc227ad 100644
--- a/xmodule/x_module.py
+++ b/xmodule/x_module.py
@@ -10,7 +10,6 @@ from functools import partial
 import yaml
 
 from django.conf import settings
-from lazy import lazy
 from lxml import etree
 from opaque_keys.edx.asides import AsideDefinitionKeyV2, AsideUsageKeyV2
 from opaque_keys.edx.keys import UsageKey
@@ -417,13 +416,12 @@ class XModuleMixin(XModuleFields, XBlock):
         self.save()
         return self._field_data._kvs  # pylint: disable=protected-access
 
-    @lazy
     def _unwrapped_field_data(self):
         """
-        This property hold the value _field_data here before we wrap it in
-        the LmsFieldData or OverrideFieldData classes.
+        This property gets the field-data service before we wrap it in user-specifc wrappers during bind_for_student,
+        e.g. the LmsFieldData or OverrideFieldData classes.
         """
-        return self._field_data
+        return self.runtime.service(self, 'field-data-unbound')
 
     def add_aside(self, aside):
         """
@@ -653,12 +651,13 @@ class XModuleMixin(XModuleFields, XBlock):
 
         if wrappers is None:
             wrappers = []
-
-        wrapped_field_data = self._unwrapped_field_data
-        for wrapper in wrappers:
-            wrapped_field_data = wrapper(wrapped_field_data)
-
-        self._field_data = wrapped_field_data
+        if wrappers:
+            # Put user-specific wrappers around the field-data service for this block.
+            # Note that these are different from modulestore.xblock_field_data_wrappers, which are not user-specific.
+            wrapped_field_data = self.runtime.service(self, 'field-data-unbound')
+            for wrapper in wrappers:
+                wrapped_field_data = wrapper(wrapped_field_data)
+            self._bound_field_data = wrapped_field_data
 
     @property
     def non_editable_metadata_fields(self):

From a6accf701b89fecc65994b9e3c14d072fbd0b112 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Tue, 23 May 2023 18:12:42 -0700
Subject: [PATCH 06/16] temp: update query counts

---
 lms/djangoapps/grades/tests/test_tasks.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py
index 4c4f60d731..475cd6032b 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, 2, 41, True),
-        (ModuleStoreEnum.Type.split, 2, 41, False),
+        (ModuleStoreEnum.Type.split, 1, 41, True),
+        (ModuleStoreEnum.Type.split, 1, 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, 2, 41),
+        (ModuleStoreEnum.Type.split, 1, 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, 2, 41),
+        (ModuleStoreEnum.Type.split, 1, 41),
     )
     @ddt.unpack
     def test_persistent_grades_on_course(self, default_store, num_mongo_queries, num_sql_queries):

From f1ff90dc3e7e5943f6f62984db494e0201c6b5a9 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Tue, 23 May 2023 18:15:14 -0700
Subject: [PATCH 07/16] fix: failing test, not really sure why this worked
 before without modulestore.update_item()

---
 lms/djangoapps/gating/tests/test_integration.py | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/lms/djangoapps/gating/tests/test_integration.py b/lms/djangoapps/gating/tests/test_integration.py
index ffa9ef243f..3b399e1b89 100644
--- a/lms/djangoapps/gating/tests/test_integration.py
+++ b/lms/djangoapps/gating/tests/test_integration.py
@@ -52,11 +52,9 @@ class TestGatedContent(MilestonesTestCaseMixin, SharedModuleStoreTestCase):
             org='edX',
             number='EDX101',
             run='EDX101_RUN1',
-            display_name='edX 101'
-        )
-        with modulestore().bulk_operations(course.id):
-            course.enable_subsection_gating = True
-            grading_policy = {
+            display_name='edX 101',
+            enable_subsection_gating=True,
+            grading_policy={
                 "GRADER": [{
                     "type": "Homework",
                     "min_count": 3,
@@ -64,10 +62,9 @@ class TestGatedContent(MilestonesTestCaseMixin, SharedModuleStoreTestCase):
                     "short_label": "HW",
                     "weight": 1.0
                 }]
-            }
-            course.grading_policy = grading_policy
-            course.save()
-
+            },
+        )
+        with modulestore().bulk_operations(course.id):
             # create chapter
             cls.chapter1 = BlockFactory.create(
                 parent=course,

From b3c5faa8840cc1cc27ddd5dc9c240ee901008fe7 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Tue, 23 May 2023 18:28:27 -0700
Subject: [PATCH 08/16] temp: update query counts

---
 .../instructor/tests/views/test_instructor_dashboard.py         | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py
index bca16977a9..f27963f068 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(6):
+        with check_mongo_calls(5):
             response = self.client.get(url)
             assert response.status_code == 200

From 15b511ffd14912a3cccb402ee0376c08e92dd018 Mon Sep 17 00:00:00 2001
From: Agrendalath <piotr@surowiec.it>
Date: Thu, 25 May 2023 20:11:02 +0200
Subject: [PATCH 09/16] test: fix the number of Mongo calls

---
 xmodule/modulestore/tests/test_mongo_call_count.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xmodule/modulestore/tests/test_mongo_call_count.py b/xmodule/modulestore/tests/test_mongo_call_count.py
index 01425d0e2d..9fe3d5d697 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, 2),
+        (MIXED_SPLIT_MODULESTORE_BUILDER, None, True, False, 1),
         (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, False, 2),
-        (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, False, 2),
+        (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, False, 1),
     )
     @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, 3),
+        (MIXED_SPLIT_MODULESTORE_BUILDER, 2),
     )
     @ddt.unpack
     def test_lazy_when_course_previously_cached(self, store_builder, num_mongo_calls):

From 9f5f3108a0afc47f80d8879aa42da2bdcf7aa291 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Thu, 25 May 2023 16:33:40 -0700
Subject: [PATCH 10/16] fix: cleanups and bug fix from review

---
 .../split_mongo/caching_descriptor_system.py  | 40 +++++++++----------
 xmodule/x_module.py                           |  2 -
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
index eb6f43f266..ef5bfd24c8 100644
--- a/xmodule/modulestore/split_mongo/caching_descriptor_system.py
+++ b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
@@ -162,7 +162,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
     # is the intended one when not given a course_entry_override; thus, the caching of the last branch/course id.
     def xblock_from_json(self, class_, course_key, block_key, block_data, course_entry_override=None, **kwargs):
         """
-        Load and return block info.
+        Instantiate an XBlock of the specified class, using the provided data.
         """
         if course_entry_override is None:
             course_entry_override = self.course_entry
@@ -170,12 +170,14 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
             # most recent retrieval is most likely the right one for next caller (see comment above fn)
             self.course_entry = CourseEnvelope(course_entry_override.course_key, self.course_entry.structure)
 
-        # Construct the Block Usage Locator:
-        block_locator = course_key.make_usage_key(
-            block_type=block_key.type,
-            block_id=block_key.id,
-        )
+        # If no usage id is provided, generate an in-memory id
+        if block_key is None:
+            block_key = BlockKey(block_data.block_type, LocalId())
 
+        # Construct the Block Usage Locator:
+        block_locator = course_key.make_usage_key(block_type=block_key.type, block_id=block_key.id)
+
+        # If no definition id is provided, generate an in-memory id
         definition_id = block_data.definition or LocalId()
 
         try:
@@ -251,21 +253,6 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
             course_key, class_, field, self.course_entry.structure['blocks'],
         )
 
-        if definition_id is not None and not block_data.definition_loaded:
-            definition_loader = DefinitionLazyLoader(
-                self.modulestore,
-                course_key,
-                block_key.type,
-                definition_id,
-                convert_fields,
-            )
-        else:
-            definition_loader = None
-
-        # If no definition id is provide, generate an in-memory id
-        if definition_id is None:
-            definition_id = LocalId()
-
         converted_fields = convert_fields(block_data.fields)
         converted_defaults = convert_fields(block_data.defaults)
         if block_key in self._parent_map:
@@ -286,6 +273,17 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
         except AttributeError:
             pass
 
+        if definition_id is not None and not block_data.definition_loaded:
+            definition_loader = DefinitionLazyLoader(
+                self.modulestore,
+                course_key,
+                block_key.type,
+                definition_id,
+                convert_fields,
+            )
+        else:
+            definition_loader = None
+
         kvs = SplitMongoKVS(
             definition_loader,
             converted_fields,
diff --git a/xmodule/x_module.py b/xmodule/x_module.py
index f61dc227ad..f3c37034cf 100644
--- a/xmodule/x_module.py
+++ b/xmodule/x_module.py
@@ -649,8 +649,6 @@ class XModuleMixin(XModuleFields, XBlock):
                 if field in self._dirty_fields:
                     del self._dirty_fields[field]
 
-        if wrappers is None:
-            wrappers = []
         if wrappers:
             # Put user-specific wrappers around the field-data service for this block.
             # Note that these are different from modulestore.xblock_field_data_wrappers, which are not user-specific.

From f13263df4f0e9aff48fbbb7ca04583619317b947 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Thu, 25 May 2023 20:26:32 -0700
Subject: [PATCH 11/16] fix: remove the need to patch the KVS during library
 import

---
 xmodule/modulestore/inheritance.py | 47 ++++++++++++++++++++++++++----
 xmodule/modulestore/xml.py         | 38 ++++++++++++------------
 xmodule/tests/test_import.py       |  8 -----
 xmodule/xml_block.py               |  3 ++
 4 files changed, 65 insertions(+), 31 deletions(-)

diff --git a/xmodule/modulestore/inheritance.py b/xmodule/modulestore/inheritance.py
index 5e24c39031..489365e80e 100644
--- a/xmodule/modulestore/inheritance.py
+++ b/xmodule/modulestore/inheritance.py
@@ -3,11 +3,13 @@ Support for inheritance of fields down an XBlock hierarchy.
 """
 
 
+import warnings
 from django.utils import timezone
 from xblock.core import XBlockMixin
 from xblock.fields import Boolean, Dict, Float, Integer, List, Scope, String
 from xblock.runtime import KeyValueStore, KvsFieldData
 
+from xmodule.error_block import ErrorBlock
 from xmodule.fields import Date, Timedelta
 from xmodule.partitions.partitions import UserPartition
 
@@ -280,7 +282,10 @@ def compute_inherited_metadata(block):
     NOTE: This means that there is no such thing as lazy loading at the
     moment--this accesses all the children."""
     if block.has_children:
-        parent_metadata = block.xblock_kvs.inherited_settings.copy()
+        if isinstance(block.xblock_kvs, InheritanceKeyValueStore):
+            parent_metadata = block.xblock_kvs.inherited_settings.copy()
+        else:
+            parent_metadata = {}
         # add any of block's explicitly set fields to the inheriting list
         for field in InheritanceMixin.fields.values():  # lint-amnesty, pylint: disable=no-member
             if field.is_set_on(block):
@@ -301,10 +306,23 @@ def inherit_metadata(block, inherited_data):
     `inherited_data`: A dictionary mapping field names to the values that
         they should inherit
     """
-    try:
+    if isinstance(block, ErrorBlock):
+        return
+
+    block_type = block.scope_ids.block_type
+    if isinstance(block.xblock_kvs, InheritanceKeyValueStore):
+        # This XBlock's field_data is backed by InheritanceKeyValueStore, which supports pre-computed inherited fields
         block.xblock_kvs.inherited_settings = inherited_data
-    except AttributeError:  # the kvs doesn't have inherited_settings probably b/c it's an error block
-        pass
+    else:
+        # We cannot apply pre-computed field data to this XBlock during import, but inheritance should still work
+        # normally when it's used in Studio/LMS, which use a different runtime.
+        # Though if someone ever needs a hacky temporary fix here, it's possible here to force it with:
+        #     init_dict = {key: getattr(block, key) for key in block.fields.keys()}
+        #     block._field_data = InheritanceKeyValueStore(init_dict)
+        warnings.warn(
+            f'Cannot inherit metadata to {block_type} block with KVS {block.xblock_kvs}',
+            stacklevel=2,
+        )
 
 
 def own_metadata(block):
@@ -316,7 +334,17 @@ def own_metadata(block):
 
 
 class InheritingFieldData(KvsFieldData):
-    """A `FieldData` implementation that can inherit value from parents to children."""
+    """
+    A `FieldData` implementation that can inherit value from parents to children.
+
+    This wraps a KeyValueStore, and will work fine with any KVS implementation.
+    Sometimes this wraps a subclass of InheritanceKeyValueStore, but that's not
+    a requirement.
+
+    This class is the way that inheritance "normally" works in modulestore.
+    During XML import/export, however, a different mechanism is used:
+    InheritanceKeyValueStore.
+    """
 
     def __init__(self, inheritable_names, **kwargs):
         """
@@ -379,6 +407,15 @@ class InheritanceKeyValueStore(KeyValueStore):
     dict-based storage of fields and lookup of inherited values.
 
     Note: inherited_settings is a dict of key to json values (internal xblock field repr)
+
+    Using this KVS is an alternative to using InheritingFieldData(). That one works with any KVS, like
+    DictKeyValueStore, and doesn't require any special behavior. On the other hand, this InheritanceKeyValueStore only
+    does inheritance properly if you first use compute_inherited_metadata() to walk the tree of XBlocks and pre-compute
+    the inherited metadata for the whole tree, storing it in the inherited_settings field of each instance of this KVS.
+
+    🟥 Warning: Unlike the base class, this KVS makes the assumption that you're using a completely separate KVS
+       instance for every XBlock, so that we only have to look at the "field_name" part of the key. You cannot use this
+       as a drop-in replacement for DictKeyValueStore for this reason.
     """
     def __init__(self, initial_values=None, inherited_settings=None):
         super().__init__()
diff --git a/xmodule/modulestore/xml.py b/xmodule/modulestore/xml.py
index 38e3c22189..c4a4d66cbf 100644
--- a/xmodule/modulestore/xml.py
+++ b/xmodule/modulestore/xml.py
@@ -7,13 +7,13 @@ import json
 import logging
 import os
 import re
+import warnings
 import sys
 from collections import defaultdict
 from contextlib import contextmanager
 from importlib import import_module
 
 from fs.osfs import OSFS
-from lazy import lazy
 from lxml import etree
 from opaque_keys.edx.keys import CourseKey
 from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryLocator
@@ -37,7 +37,7 @@ from xmodule.x_module import (  # lint-amnesty, pylint: disable=unused-import
 )
 
 from .exceptions import ItemNotFoundError
-from .inheritance import InheritanceKeyValueStore, compute_inherited_metadata, inheriting_field_data
+from .inheritance import compute_inherited_metadata, inheriting_field_data
 
 edx_xml_parser = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True)
 
@@ -239,6 +239,24 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):  # lint-amnesty, pyl
             **kwargs
         )
 
+    # pylint: disable=keyword-arg-before-vararg
+    def construct_xblock_from_class(self, cls, scope_ids, field_data=None, *args, **kwargs):
+        """
+        Construct a new xblock of type cls, mixing in the mixins
+        defined for this application.
+        """
+        if field_data:
+            # Currently, *some* XBlocks (those with XmlMixin) use XmlMixin.parse_xml() which instantiates
+            # its own key value store for field data. That is something which should be left to the runtime, for
+            # consistent behavior across all XBlocks, not controlled by individual XBlock implementations.
+            # We cannot just ignore field_data here though, because parse_xml may have pre-loaded data into it that we
+            # would otherwise lose.
+            warnings.warn(
+                'XBlocks should not instantiate their own field_data store during parse_xml()',
+                DeprecationWarning, stacklevel=2,
+            )
+        return super().construct_xblock_from_class(cls, scope_ids, field_data, *args, **kwargs)
+
     # id_generator is ignored, because each ImportSystem is already local to
     # a course, and has it's own id_generator already in place
     def add_node_as_child(self, block, node, id_generator):  # lint-amnesty, pylint: disable=signature-differs
@@ -899,26 +917,10 @@ class LibraryXMLModuleStore(XMLModuleStore):
         """
         return LibraryLocator(org=org, library=library)
 
-    @staticmethod
-    def patch_block_kvs(library_block):
-        """
-        Metadata inheritance can be done purely through XBlocks, but in the import phase
-        a root block with an InheritanceKeyValueStore is assumed to be at the top of the hierarchy.
-        This should change in the future, but as XBlocks don't have this KVS, we have to patch it
-        here manually.
-        """
-        init_dict = {key: getattr(library_block, key) for key in library_block.fields.keys()}
-        # if set, invalidate '_unwrapped_field_data' so it will be reset
-        # the next time it will be called
-        lazy.invalidate(library_block, '_unwrapped_field_data')
-        # pylint: disable=protected-access
-        library_block._field_data = inheriting_field_data(InheritanceKeyValueStore(init_dict))
-
     def content_importers(self, system, course_block, course_dir, url_name):
         """
         Handle Metadata inheritance for Libraries.
         """
-        self.patch_block_kvs(course_block)
         compute_inherited_metadata(course_block)
 
     def get_library(self, library_id, depth=0, **kwargs):  # pylint: disable=unused-argument
diff --git a/xmodule/tests/test_import.py b/xmodule/tests/test_import.py
index 3fb444a1c1..95feedeb9c 100644
--- a/xmodule/tests/test_import.py
+++ b/xmodule/tests/test_import.py
@@ -276,12 +276,6 @@ class ImportTestCase(BaseCourseTestCase):  # lint-amnesty, pylint: disable=missi
         )
         block = system.process_xml(start_xml)
 
-        # pylint: disable=protected-access
-        original_unwrapped = block._unwrapped_field_data
-        LibraryXMLModuleStore.patch_block_kvs(block)
-        # '_unwrapped_field_data' is reset in `patch_block_kvs`
-        # pylint: disable=protected-access
-        assert original_unwrapped is not block._unwrapped_field_data
         compute_inherited_metadata(block)
         # Check the course block, since it has inheritance
         block = block.get_children()[0]
@@ -322,7 +316,6 @@ class ImportTestCase(BaseCourseTestCase):  # lint-amnesty, pylint: disable=missi
             </course>
         </library>'''.format(org=ORG, course=COURSE, url_name=url_name)
         block = system.process_xml(start_xml)
-        LibraryXMLModuleStore.patch_block_kvs(block)
         compute_inherited_metadata(block)
         # Run the checks on the course node instead.
         block = block.get_children()[0]
@@ -390,7 +383,6 @@ class ImportTestCase(BaseCourseTestCase):  # lint-amnesty, pylint: disable=missi
             </course>
         </library>'''.format(due=course_due, org=ORG, course=COURSE, url_name=url_name)
         block = system.process_xml(start_xml)
-        LibraryXMLModuleStore.patch_block_kvs(block)
         # Chapter is two levels down here.
         child = block.get_children()[0].get_children()[0]
         # pylint: disable=protected-access
diff --git a/xmodule/xml_block.py b/xmodule/xml_block.py
index cb7c096ce9..45b7589357 100644
--- a/xmodule/xml_block.py
+++ b/xmodule/xml_block.py
@@ -360,6 +360,9 @@ class XmlMixin:
         field_data['children'] = children
 
         field_data['xml_attributes']['filename'] = definition.get('filename', ['', None])  # for git link
+        # TODO: we shouldn't be instantiating our own field data instance here, but rather just call to
+        # runtime.construct_xblock_from_class() and then set fields on the returned block.
+        # See the base XBlock class (XmlSerializationMixin.parse_xml) for how it should be done.
         kvs = InheritanceKeyValueStore(initial_values=field_data)
         field_data = KvsFieldData(kvs)
 

From 018f8f2c7c82fd972ff0cb8a5f5903f2078ad827 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 26 May 2023 11:16:13 -0700
Subject: [PATCH 12/16] fix: Apparently block.scope_ids gets mutated a lot and
 isn't a good key

-> bind_for_student() and friends change the user_id
-> DraftVersioningModuleStore.update_item() mutates .location which mutates scope_ids.def_id and scope_ids.usage_id in a way that seems totally wrong
---
 .../split_mongo/caching_descriptor_system.py        | 13 +++++++------
 xmodule/x_module.py                                 |  2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
index ef5bfd24c8..68286be73d 100644
--- a/xmodule/modulestore/split_mongo/caching_descriptor_system.py
+++ b/xmodule/modulestore/split_mongo/caching_descriptor_system.py
@@ -2,6 +2,7 @@
 
 import logging
 import sys
+import weakref
 
 from fs.osfs import OSFS
 from lazy import lazy
@@ -75,7 +76,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
         self.local_modules = {}
         self._services['library_tools'] = LibraryToolsService(modulestore, user_id=None)
         # Cache of block field datas, keyed by their ScopeId tuples
-        self.block_field_datas = {}
+        self.block_field_datas = weakref.WeakKeyDictionary()
 
     @lazy
     def _parent_map(self):  # lint-amnesty, pylint: disable=missing-function-docstring
@@ -226,14 +227,14 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
                 # This complexity is due to XModule heritage. If we didn't load the block as one step, then sometimes
                 # "rebind" it to be user-specific as a later step, we could load the field data and overrides at once.
                 return block._bound_field_data  # pylint: disable=protected-access
-            if block.scope_ids not in self.block_field_datas:
+            if block not in self.block_field_datas:
                 try:
                     self._init_field_data_for_block(block)
                 except:
                     # Don't try again pointlessly every time another field is accessed
-                    self.block_field_datas[block.scope_ids] = None
+                    self.block_field_datas[block] = None
                     raise
-            return self.block_field_datas[block.scope_ids]
+            return self.block_field_datas[block]
         return super().service(block, service_name)
 
     def _init_field_data_for_block(self, block):
@@ -300,10 +301,10 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
 
         # Save the field_data now, because some of the wrappers will try to read fields from the block while they
         # determine if they want to wrap the field_data or not.
-        self.block_field_datas[block.scope_ids] = field_data
+        self.block_field_datas[block] = field_data
         # Now add in any wrappers that want to be added:
         for wrapper in self.modulestore.xblock_field_data_wrappers:
-            self.block_field_datas[block.scope_ids] = wrapper(block, self.block_field_datas[block.scope_ids])
+            self.block_field_datas[block] = wrapper(block, self.block_field_datas[block])
 
         # Note: user-specific wrappers like LmsFieldData or OverrideFieldData do not get set here. They get set later
         # during bind_for_student(), which wraps the field-data and sets block._bound_field_data. Calling
diff --git a/xmodule/x_module.py b/xmodule/x_module.py
index f3c37034cf..ea3eac4f8a 100644
--- a/xmodule/x_module.py
+++ b/xmodule/x_module.py
@@ -366,7 +366,7 @@ class XModuleMixin(XModuleFields, XBlock):
     def location(self, value):
         assert isinstance(value, UsageKey)
         self.scope_ids = self.scope_ids._replace(
-            def_id=value,
+            def_id=value,  # FIXME: this seems wrong... a UsageKey is not a definition key.
             usage_id=value,
         )
 

From fbaa2e5a68374502d10de95faf60cdb009f83715 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Sat, 27 May 2023 11:57:18 -0700
Subject: [PATCH 13/16] 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

From 6dd9d2e068da846d529e7b5e99d1dbf4f60f126e Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Mon, 29 May 2023 13:50:47 -0700
Subject: [PATCH 14/16] fix: some tests still use old mongo and weren't passing

---
 xmodule/modulestore/mongo/base.py | 17 +++++++++++++++++
 xmodule/x_module.py               |  4 ++++
 2 files changed, 21 insertions(+)

diff --git a/xmodule/modulestore/mongo/base.py b/xmodule/modulestore/mongo/base.py
index fd434adc0e..9a04a8099d 100644
--- a/xmodule/modulestore/mongo/base.py
+++ b/xmodule/modulestore/mongo/base.py
@@ -177,6 +177,11 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
             [str(key) for key in self.module_data.keys()],
             self.default_class,
         ))
+    
+    # This CachingDescriptorSystem runtime sets block._field_data on each block via construct_xblock_from_class(),
+    # rather than the newer approach of providing a "field-data" service via runtime.service(). As a result, during
+    # bind_for_student() we can't just set ._bound_field_data; we must overwrite block._field_data.
+    uses_deprecated_field_data = True
 
     def __init__(self, modulestore, course_key, module_data, default_class, **kwargs):
         """
@@ -303,6 +308,18 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
                     error_msg=exc_info_to_str(sys.exc_info())
                 )
 
+    def service(self, block, service_name):
+        """
+        Return a service, or None.
+        Services are objects implementing arbitrary other interfaces.
+        """
+        # A very minimal shim for compatibility with the new API for how we access field data in split mongo:
+        if service_name == 'field-data-unbound':
+            return block._field_data  # pylint: disable=protected-access
+        elif service_name == 'field-data':
+            return block._bound_field_data if hasattr(block, "_bound_field_data") else block._field_data
+        return super().service(block, service_name)
+
     def _convert_reference_to_key(self, ref_string):
         """
         Convert a single serialized UsageKey string in a ReferenceField into a UsageKey.
diff --git a/xmodule/x_module.py b/xmodule/x_module.py
index 8ce4739363..74fa31918e 100644
--- a/xmodule/x_module.py
+++ b/xmodule/x_module.py
@@ -669,6 +669,10 @@ class XModuleMixin(XModuleFields, XBlock):
             for wrapper in wrappers:
                 wrapped_field_data = wrapper(wrapped_field_data)
             self._bound_field_data = wrapped_field_data
+            if getattr(self.runtime, "uses_deprecated_field_data", False):
+                # This approach is deprecated but old mongo's CachingDescriptorSystem still requires it.
+                # For Split mongo's CachingDescriptor system, don't set ._field_data this way.
+                self._field_data = wrapped_field_data
 
     @property
     def non_editable_metadata_fields(self):

From dac1c5abe1193ce51297d6aa6db32742b2f6cedc Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Mon, 29 May 2023 14:47:21 -0700
Subject: [PATCH 15/16] refactor: we don't need _unwrapped_field_data any more

---
 lms/djangoapps/courseware/tests/test_block_render.py | 10 +++++-----
 xmodule/modulestore/mongo/base.py                    | 11 ++++++-----
 xmodule/x_module.py                                  |  7 -------
 3 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py
index 55896d4fa9..0504772aa5 100644
--- a/lms/djangoapps/courseware/tests/test_block_render.py
+++ b/lms/djangoapps/courseware/tests/test_block_render.py
@@ -487,11 +487,10 @@ class BlockRenderTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase):
             self.mock_user, request, block, field_data_cache, course.id, course=course
         )
 
-        # check that _unwrapped_field_data is the same as the original
+        # check that block.runtime.service(block, 'field-data-unbound') is the same as the original
         # _field_data, but now _field_data as been reset.
-        # pylint: disable=protected-access
-        assert block._unwrapped_field_data is original_field_data  # lint-amnesty, pylint: disable=no-member
-        assert block._unwrapped_field_data is not block._field_data  # lint-amnesty, pylint: disable=no-member
+        assert block.runtime.service(block, 'field-data-unbound') is original_field_data
+        assert block.runtime.service(block, 'field-data-unbound') is not block._field_data  # pylint: disable=protected-access, line-too-long
 
         # now bind this block to a few other students
         for user in [UserFactory(), UserFactory(), self.mock_user]:
@@ -513,7 +512,8 @@ class BlockRenderTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase):
 
         # the OverrideFieldData should point to the date FieldData
         assert isinstance(block._field_data._authored_data._source.fallback, DateLookupFieldData)    # lint-amnesty, pylint: disable=no-member, line-too-long
-        assert block._field_data._authored_data._source.fallback._defaults is block._unwrapped_field_data    # lint-amnesty, pylint: disable=no-member, line-too-long
+        assert block._field_data._authored_data._source.fallback._defaults \
+            is block.runtime.service(block, 'field-data-unbound')
 
     def test_hash_resource(self):
         """
diff --git a/xmodule/modulestore/mongo/base.py b/xmodule/modulestore/mongo/base.py
index 9a04a8099d..469ef608a5 100644
--- a/xmodule/modulestore/mongo/base.py
+++ b/xmodule/modulestore/mongo/base.py
@@ -170,6 +170,12 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
     A system that has a cache of block json that it will use to load blocks
     from, with a backup of calling to the underlying modulestore for more data
     """
+
+    # This CachingDescriptorSystem runtime sets block._field_data on each block via construct_xblock_from_class(),
+    # rather than the newer approach of providing a "field-data" service via runtime.service(). As a result, during
+    # bind_for_student() we can't just set ._bound_field_data; we must overwrite block._field_data.
+    uses_deprecated_field_data = True
+
     def __repr__(self):
         return "CachingDescriptorSystem{!r}".format((
             self.modulestore,
@@ -177,11 +183,6 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):  # li
             [str(key) for key in self.module_data.keys()],
             self.default_class,
         ))
-    
-    # This CachingDescriptorSystem runtime sets block._field_data on each block via construct_xblock_from_class(),
-    # rather than the newer approach of providing a "field-data" service via runtime.service(). As a result, during
-    # bind_for_student() we can't just set ._bound_field_data; we must overwrite block._field_data.
-    uses_deprecated_field_data = True
 
     def __init__(self, modulestore, course_key, module_data, default_class, **kwargs):
         """
diff --git a/xmodule/x_module.py b/xmodule/x_module.py
index 74fa31918e..da0eed90e9 100644
--- a/xmodule/x_module.py
+++ b/xmodule/x_module.py
@@ -429,13 +429,6 @@ class XModuleMixin(XModuleFields, XBlock):
         self.save()
         return self._field_data._kvs  # pylint: disable=protected-access
 
-    def _unwrapped_field_data(self):
-        """
-        This property gets the field-data service before we wrap it in user-specifc wrappers during bind_for_student,
-        e.g. the LmsFieldData or OverrideFieldData classes.
-        """
-        return self.runtime.service(self, 'field-data-unbound')
-
     def add_aside(self, aside):
         """
         save connected asides

From d44ca41410f4b2706dd143a91e9f1db68f6045ea Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Wed, 31 May 2023 16:14:05 -0700
Subject: [PATCH 16/16] docs: clarify comment that I had added earlier

---
 xmodule/x_module.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xmodule/x_module.py b/xmodule/x_module.py
index da0eed90e9..f3e7395fa3 100644
--- a/xmodule/x_module.py
+++ b/xmodule/x_module.py
@@ -379,7 +379,7 @@ class XModuleMixin(XModuleFields, XBlock):
     def location(self, value):
         assert isinstance(value, UsageKey)
         self.scope_ids = self.scope_ids._replace(
-            def_id=value,  # FIXME: this seems wrong... a UsageKey is not a definition key.
+            def_id=value,  # Note: assigning a UsageKey as def_id is OK in old mongo / import system but wrong in split
             usage_id=value,
         )