Merge pull request #8594 from cpennington/pass-xblock-parents

Pass xblock parents

common/lib/xmodule/xmodule/course_module.py

@@ -919,7 +919,7 @@ class CourseModule(CourseFields, SequenceModule):  # pylint: disable=abstract-me
     """
 
 
-class CourseDescriptor(CourseFields, LicenseMixin, SequenceDescriptor):
+class CourseDescriptor(CourseFields, SequenceDescriptor, LicenseMixin):
     """
     The descriptor for the course XModule
     """

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

@@ -2,12 +2,15 @@
 Factories for use in tests of XBlocks.
 """
 
+import functools
 import inspect
 import pprint
+import pymongo.message
 import threading
-from uuid import uuid4
+import traceback
+from collections import defaultdict
 from decorator import contextmanager
-import pymongo.message
+from uuid import uuid4
 
 from factory import Factory, Sequence, lazy_attribute_sequence, lazy_attribute
 from factory.containers import CyclicDefinitionError
@@ -320,47 +323,160 @@ def check_number_of_calls(object_with_method, method_name, maximum_calls, minimu
     return check_sum_of_calls(object_with_method, [method_name], maximum_calls, minimum_calls)
 
 
+class StackTraceCounter(object):
+    """
+    A class that counts unique stack traces underneath a particular stack frame.
+    """
+    def __init__(self, stack_depth, include_arguments=True):
+        """
+        Arguments:
+            stack_depth (int): The number of stack frames above this constructor to capture.
+            include_arguments (bool): Whether to store the arguments that are passed
+                when capturing a stack trace.
+        """
+        self.include_arguments = include_arguments
+        self._top_of_stack = traceback.extract_stack(limit=stack_depth)[0]
+
+        if self.include_arguments:
+            self._stacks = defaultdict(lambda: defaultdict(int))
+        else:
+            self._stacks = defaultdict(int)
+
+    def capture_stack(self, args, kwargs):
+        """
+        Record the stack frames starting at the caller of this method, and
+        ending at the top of the stack as defined by the ``stack_depth``.
+
+        Arguments:
+            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:
+            stack = stack[stack.index(self._top_of_stack):]
+
+        if self.include_arguments:
+            safe_args = []
+            for arg in args:
+                try:
+                    safe_args.append(repr(arg))
+                except Exception as exc:
+                    safe_args.append('<un-repr-able value: {}'.format(exc))
+
+            safe_kwargs = {}
+            for key, kwarg in kwargs.items():
+                try:
+                    safe_kwargs[key] = repr(kwarg)
+                except Exception as exc:
+                    safe_kwargs[key] = '<un-repr-able value: {}'.format(exc)
+
+            self._stacks[tuple(stack)][tuple(safe_args), tuple(safe_kwargs.items())] += 1
+        else:
+            self._stacks[tuple(stack)] += 1
+
+    @property
+    def total_calls(self):
+        """
+        Return the total number of stacks recorded.
+        """
+        return sum(self.stack_calls(stack) for stack in self._stacks)
+
+    def stack_calls(self, stack):
+        """
+        Return the number of calls to the supplied ``stack``.
+        """
+        if self.include_arguments:
+            return sum(self._stacks[stack].values())
+        else:
+            return self._stacks[stack]
+
+    def __iter__(self):
+        """
+        Iterate over all unique captured stacks.
+        """
+        return iter(sorted(self._stacks.keys(), key=lambda stack: (self.stack_calls(stack), stack), reverse=True))
+
+    def __getitem__(self, stack):
+        """
+        Return the set of captured calls with the supplied stack.
+        """
+        return self._stacks[stack]
+
+    @classmethod
+    def capture_call(cls, func, stack_depth, include_arguments=True):
+        """
+        A decorator that wraps ``func``, and captures each call to ``func``,
+        recording the stack trace, and optionally the arguments that the function
+        is called with.
+
+        Arguments:
+            func: the function to wrap
+            stack_depth: how far up the stack to truncate the stored stack traces (
+                this is counted from the call to ``capture_call``, rather than calls
+                to the captured function).
+
+        """
+        stacks = StackTraceCounter(stack_depth, include_arguments)
+
+        # pylint: disable=missing-docstring
+        @functools.wraps(func)
+        def capture(*args, **kwargs):
+            stacks.capture_stack(args, kwargs)
+            return func(*args, **kwargs)
+
+        capture.stack_counter = stacks
+
+        return capture
+
+
 @contextmanager
-def check_sum_of_calls(object_, methods, maximum_calls, minimum_calls=1):
+def check_sum_of_calls(object_, methods, maximum_calls, minimum_calls=1, include_arguments=True):
     """
     Instruments the given methods on the given object to verify that the total sum of calls made to the
     methods falls between minumum_calls and maximum_calls.
     """
 
     mocks = {
-        method: Mock(wraps=getattr(object_, method))
+        method: StackTraceCounter.capture_call(
+            getattr(object_, method),
+            stack_depth=7,
+            include_arguments=include_arguments
+        )
         for method in methods
     }
 
-    if inspect.isclass(object_):
-        # If the object that we're intercepting methods on is a class, rather than a module,
-        # then we need to set the method to a real function, so that self gets passed to it,
-        # and then explicitly pass that self into the call to the mock
-        # pylint: disable=unnecessary-lambda,cell-var-from-loop
-        mock_kwargs = {
-            method: lambda self, *args, **kwargs: mocks[method](self, *args, **kwargs)
-            for method in methods
-        }
-    else:
-        mock_kwargs = mocks
-
-    with patch.multiple(object_, **mock_kwargs):
+    with patch.multiple(object_, **mocks):
         yield
 
-    call_count = sum(mock.call_count for mock in mocks.values())
+    call_count = sum(capture_fn.stack_counter.total_calls for capture_fn in mocks.values())
 
     # Assertion errors don't handle multi-line values, so pretty-print to std-out instead
     if not minimum_calls <= call_count <= maximum_calls:
-        calls = {
-            method_name: mock.call_args_list
-            for method_name, mock in mocks.items()
-        }
-        print "Expected between {} and {} calls, {} were made. Calls: {}".format(
+        messages = ["Expected between {} and {} calls, {} were made.\n\n".format(
             minimum_calls,
             maximum_calls,
             call_count,
-            pprint.pformat(calls),
-        )
+        )]
+        for method_name, capture_fn in mocks.items():
+            stack_counter = capture_fn.stack_counter
+            messages.append("{!r} was called {} times:\n".format(
+                method_name,
+                stack_counter.total_calls
+            ))
+            for stack in stack_counter:
+                messages.append("  called {} times:\n\n".format(stack_counter.stack_calls(stack)))
+                messages.append("    " + "    ".join(traceback.format_list(stack)))
+                messages.append("\n\n")
+                if include_arguments:
+                    for (args, kwargs), count in stack_counter[stack].items():
+                        messages.append("      called {} times with:\n".format(count))
+                        messages.append("      args: {}\n".format(args))
+                        messages.append("      kwargs: {}\n\n".format(dict(kwargs)))
+
+        print "".join(messages)
 
     # verify the counter actually worked by ensuring we have counted greater than (or equal to) the minimum calls
     assert_greater_equal(call_count, minimum_calls)

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

@@ -60,7 +60,7 @@ DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor'
 RENDER_TEMPLATE = lambda t_n, d, ctx=None, nsp='main': ''
 
 
-class ReferenceTestXBlock(XBlock, XModuleMixin):
+class ReferenceTestXBlock(XModuleMixin):
     """
     Test xblock type to test the reference field types
     """

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

@@ -101,7 +101,7 @@ def render_to_template_mock(*args):
     pass
 
 
-class StubXBlock(XBlock, XModuleMixin, InheritanceMixin):
+class StubXBlock(XModuleMixin, InheritanceMixin):
     """
     Stub XBlock used for testing.
     """

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/video_module/video_module.py

@@ -86,7 +86,7 @@ log = logging.getLogger(__name__)
 _ = lambda text: text
 
 
-class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers, XModule):
+class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers, XModule, LicenseMixin):
     """
     XML source example:
         <video show_captions="true"
@@ -332,7 +332,7 @@ class VideoModule(VideoFields, VideoTranscriptsMixin, VideoStudentViewHandlers,
 @XBlock.wants("request_cache")
 @XBlock.wants("settings")
 class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandlers,
-                      TabsEditingDescriptor, EmptyDataRawDescriptor):
+                      TabsEditingDescriptor, EmptyDataRawDescriptor, LicenseMixin):
     """
     Descriptor for `VideoModule`.
     """

common/lib/xmodule/xmodule/video_module/video_xfields.py

@@ -12,7 +12,7 @@ from xmodule.mixin import LicenseMixin
 _ = lambda text: text
 
 
-class VideoFields(LicenseMixin):
+class VideoFields(object):
     """Fields for `VideoModule` and `VideoDescriptor`."""
     display_name = String(
         help=_("The name students see. This name appears in the course ribbon and as a header for the video."),

common/lib/xmodule/xmodule/x_module.py

@@ -20,7 +20,7 @@ from lazy import lazy
 
 from xblock.core import XBlock, XBlockAside
 from xblock.fields import (
-    Scope, Integer, Float, List, XBlockMixin,
+    Scope, Integer, Float, List,
     String, Dict, ScopeIds, Reference, ReferenceList,
     ReferenceValueDict, UserScope
 )
@@ -265,7 +265,7 @@ class XModuleFields(object):
     )
 
 
-class XModuleMixin(XModuleFields, XBlockMixin):
+class XModuleMixin(XModuleFields, XBlock):
     """
     Fields and methods used by XModules internally.
 
@@ -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():
@@ -737,7 +742,7 @@ module_runtime_attr = partial(ProxyAttribute, 'xmodule_runtime')  # pylint: disa
 
 
 @XBlock.needs("i18n")
-class XModule(XModuleMixin, HTMLSnippet, XBlock):  # pylint: disable=abstract-method
+class XModule(HTMLSnippet, XModuleMixin):  # pylint: disable=abstract-method
     """ Implements a generic learning module.
 
         Subclasses must at a minimum provide a definition for get_html in order
@@ -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
@@ -932,7 +949,7 @@ class ResourceTemplates(object):
 
 
 @XBlock.needs("i18n")
-class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
+class XModuleDescriptor(HTMLSnippet, ResourceTemplates, XModuleMixin):
     """
     An XModuleDescriptor is a specification for an element of a course. This
     could be a problem, an organizational element (a group of content), or a
@@ -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

@@ -143,7 +143,7 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin,
 
             with self.assertNumQueries(queries):
                 with check_mongo_calls(reads):
-                    with check_sum_of_calls(XBlock, ['__init__'], xblocks):
+                    with check_sum_of_calls(XBlock, ['__init__'], xblocks, xblocks, include_arguments=False):
                         self.grade_course(self.course)
 
     @ddt.data(*itertools.product(('no_overrides', 'ccx'), range(1, 4), (True, False)))
@@ -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/edx/XBlock.git@74fdc5a361f48e5596acf3846ca3790a33a05253#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