fix: avoid item bank side effects during field data prefetch

lms/djangoapps/courseware/model_data.py

@@ -660,6 +660,27 @@ def _cache_key_for_kvs_key(self, key):
         return key.field_name
 
 
+def _children_for_field_data_cache(block, read_only=False):
+    """
+    Return child blocks whose field data should be prefetched for ``block``.
+
+    Dynamic blocks such as library_content expose a learner-specific subset via
+    ``get_child_blocks()``. Prefetching all modulestore children (``get_children()``)
+    loads user state for blocks that will never render for the current learner.
+    """
+    get_child_blocks = getattr(block, 'get_child_blocks', None)
+    get_child_blocks_for_prefetch = getattr(block, 'get_child_blocks_for_prefetch', None)
+    has_dynamic_children = getattr(block, 'has_dynamic_children', None)
+    if callable(has_dynamic_children) and has_dynamic_children():
+        if read_only and callable(get_child_blocks_for_prefetch):
+            return list(get_child_blocks_for_prefetch())
+
+        if callable(get_child_blocks):
+            return list(get_child_blocks())
+
+    return list(block.get_children()) + list(block.get_required_block_descriptors())
+
+
 class FieldDataCache:
     """
     A cache of django model objects needed to supply the data
@@ -731,7 +752,7 @@ def add_block_descendents(self, block, depth=None, block_filter=lambda block: Tr
                 should be cached
         """
 
-        def get_child_blocks(block, depth, block_filter):
+        def collect_descendant_blocks(block, depth, block_filter):
             """
             Return a list of all child blocks down to the specified depth
             that match the block filter. Includes `block`
@@ -749,13 +770,15 @@ def get_child_blocks(block, depth, block_filter):
             if depth is None or depth > 0:
                 new_depth = depth - 1 if depth is not None else depth
 
-                for child in block.get_children() + block.get_required_block_descriptors():
-                    blocks.extend(get_child_blocks(child, new_depth, block_filter))
+                for child in _children_for_field_data_cache(block, read_only=self.read_only):
+                    if child is None:
+                        continue
+                    blocks.extend(collect_descendant_blocks(child, new_depth, block_filter))
 
             return blocks
 
         with modulestore().bulk_operations(block.location.course_key):
-            blocks = get_child_blocks(block, depth, block_filter)
+            blocks = collect_descendant_blocks(block, depth, block_filter)
 
         self.add_blocks_to_cache(blocks)
 

lms/djangoapps/courseware/tests/test_model_data.py

@@ -13,7 +13,13 @@
 from xblock.fields import BlockScope, Scope, ScopeIds
 
 from common.djangoapps.student.tests.factories import UserFactory
-from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataCache, InvalidScopeError
+from lms.djangoapps.courseware.model_data import (
+    DjangoKeyValueStore,
+    FieldDataCache,
+    InvalidScopeError,
+    UserStateCache,
+    _children_for_field_data_cache,
+)
 from lms.djangoapps.courseware.models import (
     StudentModule,
     XModuleStudentInfoField,
@@ -444,3 +450,118 @@ class TestStudentInfoStorage(OtherUserFailureTestMixin, StorageTestBase, TestCas
     storage_class = XModuleStudentInfoField
     other_key_factory = partial(DjangoKeyValueStore.Key, Scope.user_info, 2, 'mock_problem')  # user_id=2, not 1
     existing_field_name = "existing_field"
+
+
+class TestFieldDataCacheDynamicChildren(TestCase):
+    """Tests for dynamic-child handling in FieldDataCache descendant prefetch."""
+
+    def test_children_for_field_data_cache_uses_get_child_blocks(self):
+        """
+        Dynamic blocks should only expose learner-selected children for prefetch.
+        """
+        selected_child = Mock(name='selected_child')
+        dynamic_block = Mock(name='dynamic_block')
+        dynamic_block.has_dynamic_children.return_value = True
+        dynamic_block.get_child_blocks.return_value = [selected_child]
+        dynamic_block.get_children.side_effect = AssertionError(
+            'get_children should not be called for dynamic blocks'
+        )
+
+        assert _children_for_field_data_cache(dynamic_block) == [selected_child]
+        dynamic_block.get_child_blocks.assert_called_once_with()
+        dynamic_block.get_children.assert_not_called()
+
+    def test_children_for_field_data_cache_uses_prefetch_api_in_read_only_mode(self):
+        """
+        Read-only prefetch should use a side-effect-free dynamic-child API when available.
+        """
+        selected_child = Mock(name='selected_child')
+        dynamic_block = Mock(name='dynamic_block')
+        dynamic_block.has_dynamic_children.return_value = True
+        dynamic_block.get_child_blocks_for_prefetch.return_value = [selected_child]
+        dynamic_block.get_child_blocks.side_effect = AssertionError(
+            'get_child_blocks should not be called for read-only prefetch'
+        )
+        dynamic_block.get_children.side_effect = AssertionError(
+            'get_children should not be called for dynamic blocks'
+        )
+
+        assert _children_for_field_data_cache(dynamic_block, read_only=True) == [selected_child]
+        dynamic_block.get_child_blocks_for_prefetch.assert_called_once_with()
+        dynamic_block.get_child_blocks.assert_not_called()
+        dynamic_block.get_children.assert_not_called()
+
+    def test_children_for_field_data_cache_uses_get_children_for_static_blocks(self):
+        """
+        Static blocks should continue to prefetch all modulestore children.
+        """
+        static_child = Mock(name='static_child')
+        static_block = Mock(name='static_block')
+        static_block.has_dynamic_children.return_value = False
+        static_block.get_children.return_value = [static_child]
+        static_block.get_required_block_descriptors.return_value = []
+
+        assert _children_for_field_data_cache(static_block) == [static_child]
+        static_block.get_children.assert_called_once_with()
+        static_block.get_required_block_descriptors.assert_called_once_with()
+
+    @patch('lms.djangoapps.courseware.model_data.modulestore')
+    def test_add_block_descendents_prefetches_only_selected_dynamic_children(self, mock_modulestore):
+        """
+        add_block_descendents should not walk unselected modulestore children.
+        """
+        mock_modulestore.return_value.bulk_operations.return_value.__enter__ = Mock(return_value=None)
+        mock_modulestore.return_value.bulk_operations.return_value.__exit__ = Mock(return_value=False)
+
+        user_state_field = mock_field(Scope.user_state, 'state')
+
+        def configure_static_block(block):
+            block.get_children.return_value = []
+            block.get_required_block_descriptors.return_value = []
+            block.has_dynamic_children.return_value = False
+            block.fields.values.return_value = [user_state_field]
+            block.has_score = False
+            block.location = LOCATION('usage_id')
+
+        unselected_children = [Mock(name=f'unselected_{index}') for index in range(3)]
+        selected_children = [Mock(name='selected_0'), Mock(name='selected_1')]
+        for child in selected_children + unselected_children:
+            configure_static_block(child)
+
+        library_content = Mock(name='library_content')
+        library_content.has_dynamic_children.return_value = True
+        library_content.get_child_blocks.return_value = selected_children
+        library_content.get_children.return_value = unselected_children + selected_children
+        library_content.get_required_block_descriptors.return_value = []
+        library_content.fields.values.return_value = [user_state_field]
+        library_content.has_score = False
+        library_content.location = Mock(course_key=COURSE_KEY)
+
+        vertical = Mock(name='vertical')
+        vertical.has_dynamic_children.return_value = False
+        vertical.get_children.return_value = [library_content]
+        vertical.get_required_block_descriptors.return_value = []
+        vertical.fields.values.return_value = [user_state_field]
+        vertical.has_score = False
+        vertical.location = Mock(course_key=COURSE_KEY)
+
+        user = UserFactory.create(username='dynamic_children_user')
+        field_data_cache = FieldDataCache([], COURSE_KEY, user)
+
+        cached_blocks = []
+
+        def capture_cache_fields(fields, blocks, aside_types):  # lint-amnesty, pylint: disable=unused-argument
+            cached_blocks.extend(blocks)
+
+        with patch.object(UserStateCache, 'cache_fields', side_effect=capture_cache_fields):
+            field_data_cache.add_block_descendents(vertical)
+
+        cached_block_names = {block._mock_name for block in cached_blocks}  # pylint: disable=protected-access
+        assert 'vertical' in cached_block_names
+        assert 'library_content' in cached_block_names
+        assert 'selected_0' in cached_block_names
+        assert 'selected_1' in cached_block_names
+        assert 'unselected_0' not in cached_block_names
+        assert 'unselected_1' not in cached_block_names
+        assert 'unselected_2' not in cached_block_names
+        library_content.get_child_blocks.assert_called_once_with()

xmodule/item_bank_block.py

@@ -238,7 +238,7 @@ def publish_selected_children_events(cls, block_keys, format_block_keys, publish
                 added=format_block_keys(block_keys['added'])
             )
 
-    def selected_children(self):
+    def selected_children(self, read_only=False):
         """
         Returns a [] of block_ids indicating which of the possible children
         have been selected to display to the current user.
@@ -254,19 +254,20 @@ def selected_children(self):
             max_count = len(self.children)
 
         block_keys = self.make_selection(self.selected, self.children, max_count)  # pylint: disable=no-member
+        selected = block_keys['selected']
 
-        self.publish_selected_children_events(
-            block_keys,
-            self.format_block_keys_for_analytics,
-            self._publish_event,
-        )
+        if not read_only:
+            self.publish_selected_children_events(
+                block_keys,
+                self.format_block_keys_for_analytics,
+                self._publish_event,
+            )
 
-        if any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')):
+        if not read_only and any(block_keys[changed] for changed in ('invalid', 'overlimit', 'added')):
             # Save our selections to the user state, to ensure consistency:
-            selected = block_keys['selected']
             self.selected = selected  # TODO: this doesn't save from the LMS "Progress" page.
 
-        return self.selected
+        return selected if read_only else self.selected
 
     def format_block_keys_for_analytics(self, block_keys: list[tuple[str, str]]) -> list[dict]:
         """
@@ -303,14 +304,20 @@ def reset_selected_children(self, _, __):
         self.selected = []
         return Response(json.dumps(self.student_view({}).content))
 
-    def _get_selected_child_blocks(self):
+    def _get_selected_child_blocks(self, read_only=False):
         """
         Generator returning XBlock instances of the children selected for the
         current user.
         """
-        for block_type, block_id in self.selected_children():
+        for block_type, block_id in self.selected_children(read_only=read_only):
             yield self.runtime.get_block(self.context_key.make_usage_key(block_type, block_id))
 
+    def get_child_blocks_for_prefetch(self):
+        """
+        Return the learner-selected child blocks without publishing analytics or mutating user state.
+        """
+        return list(self._get_selected_child_blocks(read_only=True))
+
     def student_view(self, context):  # lint-amnesty, pylint: disable=missing-function-docstring
         fragment = Fragment()
         contents = []

xmodule/tests/test_item_bank.py

@@ -232,6 +232,18 @@ def test_children_seen_by_a_user(self):
         # Check that get_content_titles() doesn't return titles for hidden/unused children
         assert len(self.item_bank.get_content_titles()) == 1
 
+    def test_prefetch_child_blocks_does_not_publish_or_persist_selection(self):
+        """
+        Read-only child selection should avoid analytics events and user-state writes.
+        """
+        self._bind_course_block(self.item_bank)
+
+        selected_children = self.item_bank.get_child_blocks_for_prefetch()
+
+        assert len(selected_children) == 1
+        self.publisher.assert_not_called()
+        assert self.item_bank.selected == []
+
     def test_overlimit_blocks_chosen_randomly(self):
         """
         Tests that blocks to remove from selected children are chosen