Pass XBlock parents down to their children when constructing them, for caching

common/lib/xmodule/xmodule/error_module.py

@@ -80,7 +80,18 @@ class ErrorDescriptor(ErrorFields, XModuleDescriptor):
         return u''
 
     @classmethod
-    def _construct(cls, system, contents, error_msg, location):
+    def _construct(cls, system, contents, error_msg, location, for_parent=None):
+        """
+        Build a new ErrorDescriptor. using ``system``.
+
+        Arguments:
+            system (:class:`DescriptorSystem`): The :class:`DescriptorSystem` used
+                to construct the XBlock that had an error.
+            contents (unicode): An encoding of the content of the xblock that had an error.
+            error_msg (unicode): A message describing the error.
+            location (:class:`UsageKey`): The usage key of the XBlock that had an error.
+            for_parent (:class:`XBlock`): Optional. The parent of this error block.
+        """
 
         if error_msg is None:
             # this string is not marked for translation because we don't have
@@ -110,6 +121,7 @@ class ErrorDescriptor(ErrorFields, XModuleDescriptor):
             # real scope keys
             ScopeIds(None, 'error', location, location),
             field_data,
+            for_parent=for_parent,
         )
 
     def get_context(self):
@@ -139,6 +151,7 @@ class ErrorDescriptor(ErrorFields, XModuleDescriptor):
             str(descriptor),
             error_msg,
             location=descriptor.location,
+            for_parent=descriptor.get_parent() if descriptor.has_cached_parent else None
         )
 
     @classmethod

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -223,7 +223,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         self.course_id = course_key
         self.cached_metadata = cached_metadata
 
-    def load_item(self, location):
+    def load_item(self, location, for_parent=None):  # pylint: disable=method-hidden
         """
         Return an XModule instance for the specified location
         """
@@ -292,7 +292,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
 
                 field_data = KvsFieldData(kvs)
                 scope_ids = ScopeIds(None, category, location, location)
-                module = self.construct_xblock_from_class(class_, scope_ids, field_data)
+                module = self.construct_xblock_from_class(class_, scope_ids, field_data, for_parent=for_parent)
                 if self.cached_metadata is not None:
                     # parent container pointers don't differentiate between draft and non-draft
                     # so when we do the lookup, we should do so with a non-draft location
@@ -883,7 +883,8 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         apply_cached_metadata=bool,
         using_descriptor_system="None|CachingDescriptorSystem"
     )
-    def _load_item(self, course_key, item, data_cache, apply_cached_metadata=True, using_descriptor_system=None):
+    def _load_item(self, course_key, item, data_cache,
+                   apply_cached_metadata=True, using_descriptor_system=None, for_parent=None):
         """
         Load an XModuleDescriptor from item, using the children stored in data_cache
 
@@ -898,6 +899,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
                 purposes.
             using_descriptor_system (CachingDescriptorSystem): The existing CachingDescriptorSystem
                 to add data to, and to load the XBlocks from.
+            for_parent (:class:`XBlock`): The parent of the XBlock being loaded.
         """
         course_key = self.fill_in_run(course_key)
         location = Location._from_deprecated_son(item['location'], course_key.run)
@@ -942,9 +944,9 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
             system.module_data.update(data_cache)
             system.cached_metadata.update(cached_metadata)
 
-        return system.load_item(location)
+        return system.load_item(location, for_parent=for_parent)
 
-    def _load_items(self, course_key, items, depth=0, using_descriptor_system=None):
+    def _load_items(self, course_key, items, depth=0, using_descriptor_system=None, for_parent=None):
         """
         Load a list of xmodules from the data in items, with children cached up
         to specified depth
@@ -960,7 +962,8 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
                 item,
                 data_cache,
                 using_descriptor_system=using_descriptor_system,
-                apply_cached_metadata=self._should_apply_cached_metadata(item, depth)
+                apply_cached_metadata=self._should_apply_cached_metadata(item, depth),
+                for_parent=for_parent,
             )
             for item in items
         ]
@@ -1078,7 +1081,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         except ItemNotFoundError:
             return False
 
-    def get_item(self, usage_key, depth=0, using_descriptor_system=None):
+    def get_item(self, usage_key, depth=0, using_descriptor_system=None, for_parent=None, **kwargs):
         """
         Returns an XModuleDescriptor instance for the item at location.
 
@@ -1101,7 +1104,8 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
             usage_key.course_key,
             [item],
             depth,
-            using_descriptor_system=using_descriptor_system
+            using_descriptor_system=using_descriptor_system,
+            for_parent=for_parent,
         )[0]
         return module
 
@@ -1293,6 +1297,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
             # so we use the location for both.
             ScopeIds(None, block_type, location, location),
             dbmodel,
+            for_parent=kwargs.get('for_parent'),
         )
         if fields is not None:
             for key, value in fields.iteritems():
@@ -1341,11 +1346,16 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
             block_id: a unique identifier for the new item. If not supplied,
                 a new identifier will be generated
         """
-        xblock = self.create_item(user_id, parent_usage_key.course_key, block_type, block_id=block_id, **kwargs)
         # attach to parent if given
-        if 'detached' not in xblock._class_tags:
+        parent = None
+
+        if parent_usage_key is not None:
             parent = self.get_item(parent_usage_key)
+            kwargs.setdefault('for_parent', parent)
+
+        xblock = self.create_item(user_id, parent_usage_key.course_key, block_type, block_id=block_id, **kwargs)
 
+        if parent is not None and 'detached' not in xblock._class_tags:
             # Originally added to support entrance exams (settings.FEATURES.get('ENTRANCE_EXAMS'))
             if kwargs.get('position') is None:
                 parent.children.append(xblock.location)

common/lib/xmodule/xmodule/modulestore/mongo/draft.py

@@ -82,12 +82,14 @@ class DraftModuleStore(MongoModuleStore):
         """
         def get_published():
             return wrap_draft(super(DraftModuleStore, self).get_item(
-                usage_key, depth=depth, using_descriptor_system=using_descriptor_system
+                usage_key, depth=depth, using_descriptor_system=using_descriptor_system,
+                for_parent=kwargs.get('for_parent'),
             ))
 
         def get_draft():
             return wrap_draft(super(DraftModuleStore, self).get_item(
-                as_draft(usage_key), depth=depth, using_descriptor_system=using_descriptor_system
+                as_draft(usage_key), depth=depth, using_descriptor_system=using_descriptor_system,
+                for_parent=kwargs.get('for_parent')
             ))
 
         # return the published version if ModuleStoreEnum.RevisionOption.published_only is requested

common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py

@@ -227,6 +227,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
                 class_,
                 ScopeIds(None, block_key.type, definition_id, block_locator),
                 field_data,
+                for_parent=kwargs.get('for_parent')
             )
         except Exception:  # pylint: disable=broad-except
             log.warning("Failed to load descriptor", exc_info=True)

common/lib/xmodule/xmodule/modulestore/tests/factories.py

@@ -351,6 +351,8 @@ class StackTraceCounter(object):
             args: The positional arguments to capture at this stack frame
             kwargs: The keyword arguments to capture at this stack frame
         """
+        # pylint: disable=broad-except
+
         stack = traceback.extract_stack()[:-2]
 
         if self._top_of_stack in stack:
@@ -419,6 +421,7 @@ class StackTraceCounter(object):
         """
         stacks = StackTraceCounter(stack_depth, include_arguments)
 
+        # pylint: disable=missing-docstring
         @functools.wraps(func)
         def capture(*args, **kwargs):
             stacks.capture_stack(args, kwargs)

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -250,9 +250,9 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
         # TODO (vshnayder): we are somewhat architecturally confused in the loading code:
         # load_item should actually be get_instance, because it expects the course-specific
         # policy to be loaded.  For now, just add the course_id here...
-        def load_item(usage_key):
+        def load_item(usage_key, for_parent=None):
             """Return the XBlock for the specified location"""
-            return xmlstore.get_item(usage_key)
+            return xmlstore.get_item(usage_key, for_parent=for_parent)
 
         resources_fs = OSFS(xmlstore.data_dir / course_dir)
 

common/lib/xmodule/xmodule/tests/test_conditional.py

@@ -79,10 +79,14 @@ class ConditionalFactory(object):
         child_descriptor.render = lambda view, context=None: descriptor_system.render(child_descriptor, view, context)
         child_descriptor.location = source_location.replace(category='html', name='child')
 
-        descriptor_system.load_item = {
-            child_descriptor.location: child_descriptor,
-            source_location: source_descriptor
-        }.get
+        def load_item(usage_id, for_parent=None):  # pylint: disable=unused-argument
+            """Test-only implementation of load_item that simply returns static xblocks."""
+            return {
+                child_descriptor.location: child_descriptor,
+                source_location: source_descriptor
+            }.get(usage_id)
+
+        descriptor_system.load_item = load_item
 
         system.descriptor_runtime = descriptor_system
 

common/lib/xmodule/xmodule/tests/test_error_module.py

@@ -9,7 +9,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location
 from xmodule.x_module import XModuleDescriptor, XModule, STUDENT_VIEW
 from mock import MagicMock, Mock, patch
 from xblock.runtime import Runtime, IdReader
-from xblock.field_data import FieldData
+from xblock.field_data import DictFieldData
 from xblock.fields import ScopeIds
 from xblock.test.tools import unabc
 
@@ -43,10 +43,11 @@ class TestErrorModule(SetupTestErrorModules):
         self.assertIn(repr(self.valid_xml), context_repr)
 
     def test_error_module_from_descriptor(self):
-        descriptor = MagicMock([XModuleDescriptor],
-                               runtime=self.system,
-                               location=self.location,
-                               _field_data=self.valid_xml)
+        descriptor = MagicMock(
+            spec=XModuleDescriptor,
+            runtime=self.system,
+            location=self.location,
+        )
 
         error_descriptor = ErrorDescriptor.from_descriptor(
             descriptor, self.error_msg)
@@ -81,10 +82,11 @@ class TestNonStaffErrorModule(SetupTestErrorModules):
         self.assertNotIn(repr(self.valid_xml), context_repr)
 
     def test_error_module_from_descriptor(self):
-        descriptor = MagicMock([XModuleDescriptor],
-                               runtime=self.system,
-                               location=self.location,
-                               _field_data=self.valid_xml)
+        descriptor = MagicMock(
+            spec=XModuleDescriptor,
+            runtime=self.system,
+            location=self.location,
+        )
 
         error_descriptor = NonStaffErrorDescriptor.from_descriptor(
             descriptor, self.error_msg)
@@ -122,7 +124,7 @@ class TestErrorModuleConstruction(unittest.TestCase):
     def setUp(self):
         # pylint: disable=abstract-class-instantiated
         super(TestErrorModuleConstruction, self).setUp()
-        field_data = Mock(spec=FieldData)
+        field_data = DictFieldData({})
         self.descriptor = BrokenDescriptor(
             TestRuntime(Mock(spec=IdReader), field_data),
             field_data,

common/lib/xmodule/xmodule/tests/xml/__init__.py

@@ -49,7 +49,7 @@ class InMemorySystem(XMLParsingSystem, MakoDescriptorSystem):  # pylint: disable
         self._descriptors[descriptor.location.to_deprecated_string()] = descriptor
         return descriptor
 
-    def load_item(self, location):  # pylint: disable=method-hidden
+    def load_item(self, location, for_parent=None):  # pylint: disable=method-hidden, unused-argument
         """Return the descriptor loaded for `location`"""
         return self._descriptors[location.to_deprecated_string()]
 

common/lib/xmodule/xmodule/x_module.py

@@ -295,7 +295,6 @@ class XModuleMixin(XModuleFields, XBlockMixin):
 
     def __init__(self, *args, **kwargs):
         self.xmodule_runtime = None
-        self._child_instances = None
 
         super(XModuleMixin, self).__init__(*args, **kwargs)
 
@@ -424,39 +423,45 @@ class XModuleMixin(XModuleFields, XBlockMixin):
         else:
             return [self.display_name_with_default]
 
-    def get_children(self, usage_key_filter=None):
+    def get_children(self, usage_id_filter=None, usage_key_filter=None):  # pylint: disable=arguments-differ
         """Returns a list of XBlock instances for the children of
         this module"""
 
-        if not self.has_children:
-            return []
+        # Be backwards compatible with callers using usage_key_filter
+        if usage_id_filter is None and usage_key_filter is not None:
+            usage_id_filter = usage_key_filter
 
-        if self._child_instances is None:
-            self._child_instances = []  # pylint: disable=attribute-defined-outside-init
-            for child_loc in self.children:
-                # Skip if it doesn't satisfy the filter function
-                if usage_key_filter and not usage_key_filter(child_loc):
-                    continue
-                try:
-                    child = self.runtime.get_block(child_loc)
-                    if child is None:
-                        continue
+        return [
+            child
+            for child
+            in super(XModuleMixin, self).get_children(usage_id_filter)
+            if child is not None
+        ]
 
-                    child.runtime.export_fs = self.runtime.export_fs
-                except ItemNotFoundError:
-                    log.warning(u'Unable to load item {loc}, skipping'.format(loc=child_loc))
-                    dog_stats_api.increment(
-                        "xmodule.item_not_found_error",
-                        tags=[
-                            u"course_id:{}".format(child_loc.course_key),
-                            u"block_type:{}".format(child_loc.block_type),
-                            u"parent_block_type:{}".format(self.location.block_type),
-                        ]
-                    )
-                    continue
-                self._child_instances.append(child)
+    def get_child(self, usage_id):
+        """
+        Return the child XBlock identified by ``usage_id``, or ``None`` if there
+        is an error while retrieving the block.
+        """
+        try:
+            child = super(XModuleMixin, self).get_child(usage_id)
+        except ItemNotFoundError:
+            log.warning(u'Unable to load item %s, skipping', usage_id)
+            dog_stats_api.increment(
+                "xmodule.item_not_found_error",
+                tags=[
+                    u"course_id:{}".format(usage_id.course_key),
+                    u"block_type:{}".format(usage_id.block_type),
+                    u"parent_block_type:{}".format(self.location.block_type),
+                ]
+            )
+            return None
+
+        if child is None:
+            return None
 
-        return self._child_instances
+        child.runtime.export_fs = self.runtime.export_fs
+        return child
 
     def get_required_module_descriptors(self):
         """Returns a list of XModuleDescriptor instances upon which this module depends, but are
@@ -573,7 +578,7 @@ class XModuleMixin(XModuleFields, XBlockMixin):
         self.scope_ids = self.scope_ids._replace(user_id=user_id)
 
         # Clear out any cached instantiated children.
-        self._child_instances = None
+        self.clear_child_cache()
 
         # Clear out any cached field data scoped to the old user.
         for field in self.fields.values():
@@ -767,7 +772,6 @@ class XModule(XModuleMixin, HTMLSnippet, XBlock):  # pylint: disable=abstract-me
 
         # Set the descriptor first so that we can proxy to it
         self.descriptor = descriptor
-        self._loaded_children = None
         self._runtime = None
         super(XModule, self).__init__(*args, **kwargs)
         self.runtime.xmodule_instance = self
@@ -820,6 +824,19 @@ class XModule(XModuleMixin, HTMLSnippet, XBlock):  # pylint: disable=abstract-me
         response_data = self.handle_ajax(suffix, request_post)
         return Response(response_data, content_type='application/json')
 
+    def get_child(self, usage_id):
+        if usage_id in self._child_cache:
+            return self._child_cache[usage_id]
+
+        # Take advantage of the children cache that the descriptor might have
+        child_descriptor = self.descriptor.get_child(usage_id)
+        child_block = None
+        if child_descriptor is not None:
+            child_block = self.system.get_module(child_descriptor)
+
+        self._child_cache[usage_id] = child_block
+        return child_block
+
     def get_child_descriptors(self):
         """
         Returns the descriptors of the child modules
@@ -1103,6 +1120,7 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
                     descriptor=self,
                     scope_ids=self.scope_ids,
                     field_data=self._field_data,
+                    for_parent=self.get_parent() if self.has_cached_parent else None
                 )
                 self.xmodule_runtime.xmodule_instance.save()
             except Exception:  # pylint: disable=broad-except
@@ -1340,9 +1358,9 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime):  # p
         else:
             self.get_policy = lambda u: {}
 
-    def get_block(self, usage_id):
+    def get_block(self, usage_id, for_parent=None):
         """See documentation for `xblock.runtime:Runtime.get_block`"""
-        return self.load_item(usage_id)
+        return self.load_item(usage_id, for_parent=for_parent)
 
     def get_field_provenance(self, xblock, field):
         """
@@ -1681,8 +1699,8 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime):  # pylin
         assert self.xmodule_instance is not None
         return self.handler_url(self.xmodule_instance, 'xmodule_handler', '', '').rstrip('/?')
 
-    def get_block(self, block_id):
-        return self.get_module(self.descriptor_runtime.get_block(block_id))
+    def get_block(self, block_id, for_parent=None):
+        return self.get_module(self.descriptor_runtime.get_block(block_id, for_parent=for_parent))
 
     def resource_url(self, resource):
         raise NotImplementedError("edX Platform doesn't currently implement XBlock resource urls")

lms/djangoapps/ccx/tests/test_field_override_performance.py

@@ -173,18 +173,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase):
 
     TEST_DATA = {
         # (providers, course_width, enable_ccx): # of sql queries, # of mongo queries, # of xblocks
-        ('no_overrides', 1, True): (26, 7, 19),
-        ('no_overrides', 2, True): (134, 7, 131),
-        ('no_overrides', 3, True): (594, 7, 537),
-        ('ccx', 1, True): (26, 7, 47),
-        ('ccx', 2, True): (134, 7, 455),
-        ('ccx', 3, True): (594, 7, 2037),
-        ('no_overrides', 1, False): (26, 7, 19),
-        ('no_overrides', 2, False): (134, 7, 131),
-        ('no_overrides', 3, False): (594, 7, 537),
-        ('ccx', 1, False): (26, 7, 19),
-        ('ccx', 2, False): (134, 7, 131),
-        ('ccx', 3, False): (594, 7, 537),
+        ('no_overrides', 1, True): (26, 7, 14),
+        ('no_overrides', 2, True): (134, 7, 85),
+        ('no_overrides', 3, True): (594, 7, 336),
+        ('ccx', 1, True): (26, 7, 14),
+        ('ccx', 2, True): (134, 7, 85),
+        ('ccx', 3, True): (594, 7, 336),
+        ('no_overrides', 1, False): (26, 7, 14),
+        ('no_overrides', 2, False): (134, 7, 85),
+        ('no_overrides', 3, False): (594, 7, 336),
+        ('ccx', 1, False): (26, 7, 14),
+        ('ccx', 2, False): (134, 7, 85),
+        ('ccx', 3, False): (594, 7, 336),
     }
 
 

lms/djangoapps/course_structure_api/v0/views.py

@@ -560,7 +560,12 @@ class CourseBlocksAndNavigation(ListAPIView):
         )
 
         # verify the user has access to this block
-        if not has_access(request_info.request.user, 'load', block_info.block, course_key=request_info.course.id):
+        if (block_info.block is None or not has_access(
+                request_info.request.user,
+                'load',
+                block_info.block,
+                course_key=request_info.course.id
+        )):
             return
 
         # add the block's value to the result

lms/djangoapps/courseware/tests/test_module_render.py

@@ -610,11 +610,11 @@ class TestTOC(ModuleStoreTestCase):
     # Split makes 6 queries to load the course to depth 2:
     #     - load the structure
     #     - load 5 definitions
-    # Split makes 6 queries to render the toc:
+    # Split makes 5 queries to render the toc:
     #     - it loads the active version at the start of the bulk operation
-    #     - it loads 5 definitions, because it instantiates the a CourseModule and 4 VideoModules
+    #     - it loads 4 definitions, because it instantiates 4 VideoModules
     #       each of which access a Scope.content field in __init__
-    @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 6, 0, 6))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 6, 0, 5))
     @ddt.unpack
     def test_toc_toy_from_chapter(self, default_ms, setup_finds, setup_sends, toc_finds):
         with self.store.default_store(default_ms):
@@ -634,9 +634,10 @@ class TestTOC(ModuleStoreTestCase):
                             'format': '', 'due': None, 'active': False}],
                           'url_name': 'secret:magic', 'display_name': 'secret:magic'}])
 
+            course = self.store.get_course(self.toy_course.id, depth=2)
             with check_mongo_calls(toc_finds):
                 actual = render.toc_for_course(
-                    self.request, self.toy_course, self.chapter, None, self.field_data_cache
+                    self.request, course, self.chapter, None, self.field_data_cache
                 )
         for toc_section in expected:
             self.assertIn(toc_section, actual)
@@ -648,11 +649,11 @@ class TestTOC(ModuleStoreTestCase):
     # Split makes 6 queries to load the course to depth 2:
     #     - load the structure
     #     - load 5 definitions
-    # Split makes 2 queries to render the toc:
+    # Split makes 5 queries to render the toc:
     #     - it loads the active version at the start of the bulk operation
-    #     - it loads 5 definitions, because it instantiates the a CourseModule and 4 VideoModules
+    #     - it loads 4 definitions, because it instantiates 4 VideoModules
     #       each of which access a Scope.content field in __init__
-    @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 6, 0, 6))
+    @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 6, 0, 5))
     @ddt.unpack
     def test_toc_toy_from_section(self, default_ms, setup_finds, setup_sends, toc_finds):
         with self.store.default_store(default_ms):

requirements/edx/github.txt

@@ -32,7 +32,7 @@ git+https://github.com/hmarr/django-debug-toolbar-mongo.git@b0686a76f1ce3532088c
 -e git+https://github.com/jazkarta/ccx-keys.git@e6b03704b1bb97c1d2f31301ecb4e3a687c536ea#egg=ccx-keys
 
 # Our libraries:
--e git+https://github.com/edx/XBlock.git@e1831fa86bff778ffe1308e00d8ed51b26f7c047#egg=XBlock
+-e git+https://github.com/cpennington/XBlock.git@64f6bdaf6c987cdea4798d8b1a1fa8e471c20a21#egg=XBlock
 -e git+https://github.com/edx/codejail.git@6b17c33a89bef0ac510926b1d7fea2748b73aadd#egg=codejail
 -e git+https://github.com/edx/js-test-tool.git@v0.1.6#egg=js_test_tool
 -e git+https://github.com/edx/event-tracking.git@0.2.0#egg=event-tracking