Merge pull request #5212 from cpennington/split/import-export-performance

Improve performance of split-mongo import/export performance

cms/djangoapps/contentstore/tests/test_import.py

@@ -56,7 +56,7 @@ class ContentStoreImportTest(ModuleStoreTestCase):
             target_course_id=target_course_id,
             create_new_course_if_not_present=create_new_course_if_not_present,
         )
-        course_id = SlashSeparatedCourseKey('edX', 'test_import_course', '2012_Fall')
+        course_id = module_store.make_course_key('edX', 'test_import_course', '2012_Fall')
         course = module_store.get_course(course_id)
         self.assertIsNotNone(course)
 

common/lib/xmodule/.coveragerc

@@ -5,6 +5,9 @@ source = common/lib/xmodule
 
 [report]
 ignore_errors = True
+exclude_lines =
+    pragma: no cover
+    raise NotImplementedError
 
 [html]
 title = XModule Python Test Coverage Report

common/lib/xmodule/xmodule/contentstore/mongo.py

@@ -167,7 +167,7 @@ class MongoContentStore(ContentStore):
                     policy.setdefault(asset['asset_key'].name, {})[attr] = value
 
         with open(assets_policy_file, 'w') as f:
-            json.dump(policy, f)
+            json.dump(policy, f, sort_keys=True, indent=4)
 
     def get_all_content_thumbnails_for_course(self, course_key):
         return self._get_all_content_for_course(course_key, get_thumbnails=True)[0]

common/lib/xmodule/xmodule/fields.py

@@ -2,7 +2,7 @@ import time
 import logging
 import re
 
-from xblock.fields import Field
+from xblock.fields import JSONField
 import datetime
 import dateutil.parser
 
@@ -11,7 +11,7 @@ from pytz import UTC
 log = logging.getLogger(__name__)
 
 
-class Date(Field):
+class Date(JSONField):
     '''
     Date fields know how to parse and produce json (iso) compatible formats. Converts to tz aware datetimes.
     '''
@@ -85,7 +85,7 @@ class Date(Field):
 TIMEDELTA_REGEX = re.compile(r'^((?P<days>\d+?) day(?:s?))?(\s)?((?P<hours>\d+?) hour(?:s?))?(\s)?((?P<minutes>\d+?) minute(?:s)?)?(\s)?((?P<seconds>\d+?) second(?:s)?)?$')
 
 
-class Timedelta(Field):
+class Timedelta(JSONField):
     # Timedeltas are immutable, see http://docs.python.org/2/library/datetime.html#available-types
     MUTABLE = False
 
@@ -101,6 +101,10 @@ class Timedelta(Field):
         """
         if time_str is None:
             return None
+
+        if isinstance(time_str, datetime.timedelta):
+            return time_str
+
         parts = TIMEDELTA_REGEX.match(time_str)
         if not parts:
             return
@@ -112,6 +116,9 @@ class Timedelta(Field):
         return datetime.timedelta(**time_params)
 
     def to_json(self, value):
+        if value is None:
+            return None
+
         values = []
         for attr in ('days', 'hours', 'minutes', 'seconds'):
             cur_value = getattr(value, attr, 0)
@@ -129,7 +136,7 @@ class Timedelta(Field):
         return self.from_json(value)
 
 
-class RelativeTime(Field):
+class RelativeTime(JSONField):
     """
     Field for start_time and end_time video module properties.
 
@@ -182,6 +189,9 @@ class RelativeTime(Field):
         if not value:
             return datetime.timedelta(seconds=0)
 
+        if isinstance(value, datetime.timedelta):
+            return value
+
         # We've seen serialized versions of float in this field
         if isinstance(value, float):
             return datetime.timedelta(seconds=value)

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -634,7 +634,7 @@ class ModuleStoreReadBase(BulkOperationsMixin, ModuleStoreRead):
         '''
         Set up the error-tracking logic.
         '''
-        super(ModuleStoreReadBase, self).__init__()
+        super(ModuleStoreReadBase, self).__init__(**kwargs)
         self._course_errors = defaultdict(make_error_tracker)  # location -> ErrorLog
         # TODO move the inheritance_cache_subsystem to classes which use it
         self.metadata_inheritance_cache_subsystem = metadata_inheritance_cache_subsystem

common/lib/xmodule/xmodule/modulestore/django.py

@@ -18,6 +18,8 @@ import threading
 
 from xmodule.util.django import get_current_request_hostname
 import xmodule.modulestore  # pylint: disable=unused-import
+from xmodule.modulestore.mixed import MixedModuleStore
+from xmodule.modulestore.draft_and_published import BranchSettingMixin
 from xmodule.contentstore.django import contentstore
 import xblock.reference.plugins
 
@@ -66,6 +68,12 @@ def create_modulestore_instance(engine, content_store, doc_store_config, options
     except InvalidCacheBackendError:
         metadata_inheritance_cache = get_cache('default')
 
+    if issubclass(class_, MixedModuleStore):
+        _options['create_modulestore_instance'] = create_modulestore_instance
+
+    if issubclass(class_, BranchSettingMixin):
+        _options['branch_setting_func'] = _get_modulestore_branch_setting
+
     return class_(
         contentstore=content_store,
         metadata_inheritance_cache_subsystem=metadata_inheritance_cache,
@@ -75,8 +83,6 @@ def create_modulestore_instance(engine, content_store, doc_store_config, options
         doc_store_config=doc_store_config,
         i18n_service=i18n_service or ModuleI18nService(),
         fs_service=fs_service or xblock.reference.plugins.FSService(),
-        branch_setting_func=_get_modulestore_branch_setting,
-        create_modulestore_instance=create_modulestore_instance,
         **_options
     )
 

common/lib/xmodule/xmodule/modulestore/draft_and_published.py

@@ -25,11 +25,11 @@ class BranchSettingMixin(object):
         :param branch_setting_func: a function that returns the default branch setting for this object.
             If not specified, ModuleStoreEnum.Branch.published_only is used as the default setting.
         """
-        super(BranchSettingMixin, self).__init__(*args, **kwargs)
         self.default_branch_setting_func = kwargs.pop(
             'branch_setting_func',
             lambda: ModuleStoreEnum.Branch.published_only
         )
+        super(BranchSettingMixin, self).__init__(*args, **kwargs)
 
         # cache the branch setting on a local thread to support a multi-threaded environment
         self.thread_cache = threading.local()
@@ -69,9 +69,6 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin):
     """
     __metaclass__ = ABCMeta
 
-    def __init__(self, *args, **kwargs):
-        super(ModuleStoreDraftAndPublished, self).__init__(*args, **kwargs)
-
     @abstractmethod
     def delete_item(self, location, user_id, revision=None, **kwargs):
         raise NotImplementedError
@@ -116,7 +113,7 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin):
         raise NotImplementedError
 
 
-class   UnsupportedRevisionError(ValueError):
+class UnsupportedRevisionError(ValueError):
     """
     This error is raised if a method is called with an unsupported revision parameter.
     """

common/lib/xmodule/xmodule/modulestore/exceptions.py

@@ -74,7 +74,9 @@ class DuplicateCourseError(Exception):
         """
         existing_entry will have the who, when, and other properties of the existing entry
         """
-        super(DuplicateCourseError, self).__init__()
+        super(DuplicateCourseError, self).__init__(
+            u'Cannot create course {}, which duplicates {}'.format(course_id, existing_entry)
+        )
         self.course_id = course_id
         self.existing_entry = existing_entry
 
@@ -84,9 +86,6 @@ class InvalidBranchSetting(Exception):
     Raised when the process' branch setting did not match the required setting for the attempted operation on a store.
     """
     def __init__(self, expected_setting, actual_setting):
-        super(InvalidBranchSetting, self).__init__()
+        super(InvalidBranchSetting, self).__init__(u"Invalid branch: expected {} but got {}".format(expected_setting, actual_setting))
         self.expected_setting = expected_setting
         self.actual_setting = actual_setting
-
-    def __unicode__(self, *args, **kwargs):
-        return u"Invalid branch: expected {} but got {}".format(self.expected_setting, self.actual_setting)

common/lib/xmodule/xmodule/modulestore/inheritance.py

@@ -214,11 +214,19 @@ class InheritingFieldData(KvsFieldData):
         """
         The default for an inheritable name is found on a parent.
         """
-        if name in self.inheritable_names and block.parent is not None:
-            parent = block.get_parent()
-            if parent:
-                return getattr(parent, name)
-        super(InheritingFieldData, self).default(block, name)
+        if name in self.inheritable_names:
+            # Walk up the content tree to find the first ancestor
+            # that this field is set on. Use the field from the current
+            # block so that if it has a different default than the root
+            # node of the tree, the block's default will be used.
+            field = block.fields[name]
+            ancestor = block.get_parent()
+            while ancestor is not None:
+                if field.is_set_on(ancestor):
+                    return field.read_json(ancestor)
+                else:
+                    ancestor = ancestor.get_parent()
+        return super(InheritingFieldData, self).default(block, name)
 
 
 def inheriting_field_data(kvs):

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

@@ -20,3 +20,5 @@ class BlockKey(namedtuple('BlockKey', 'type id')):
     def from_usage_key(cls, usage_key):
         return cls(usage_key.block_type, usage_key.block_id)
 
+
+CourseEnvelope = namedtuple('CourseEnvelope', 'course_key structure')

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

 import sys
 import logging
 from contracts import contract, new_contract
+from lazy import lazy
 from xblock.runtime import KvsFieldData
 from xblock.fields import ScopeIds
 from opaque_keys.edx.locator import BlockUsageLocator, LocalId, CourseLocator, DefinitionLocator
@@ -12,12 +13,14 @@ from .split_mongo_kvs import SplitMongoKVS
 from fs.osfs import OSFS
 from .definition_lazy_loader import DefinitionLazyLoader
 from xmodule.modulestore.edit_info import EditInfoRuntimeMixin
-from xmodule.modulestore.split_mongo import BlockKey
+from xmodule.modulestore.inheritance import inheriting_field_data, InheritanceMixin
+from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope
 
 log = logging.getLogger(__name__)
 
 new_contract('BlockUsageLocator', BlockUsageLocator)
 new_contract('BlockKey', BlockKey)
+new_contract('CourseEnvelope', CourseEnvelope)
 
 
 class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
@@ -27,6 +30,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
 
     Computes the settings (nee 'metadata') inheritance upon creation.
     """
+    @contract(course_entry=CourseEnvelope)
     def __init__(self, modulestore, course_entry, default_class, module_data, lazy, **kwargs):
         """
         Computes the settings inheritance and sets up the cache.
@@ -34,8 +38,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         modulestore: the module store that can be used to retrieve additional
         modules
 
-        course_entry: the originally fetched enveloped course_structure w/ branch and course id info
-        plus a dictionary of cached inherited_settings indexed by (block_type, block_id) tuple.
+        course_entry: the originally fetched enveloped course_structure w/ branch and course id info.
         Callers to _load_item provide an override but that function ignores the provided structure and
         only looks at the branch and course id
 
@@ -43,10 +46,10 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
             underlying modulestore
         """
         # needed by capa_problem (as runtime.filestore via this.resources_fs)
-        if 'course' in course_entry:
-            root = modulestore.fs_root / course_entry['org'] / course_entry['course'] / course_entry['run']
+        if course_entry.course_key.course:
+            root = modulestore.fs_root / course_entry.course_key.org / course_entry.course_key.course / course_entry.course_key.run
         else:
-            root = modulestore.fs_root / course_entry['structure']['_id']
+            root = modulestore.fs_root / course_entry.structure['_id']
         root.makedirs_p()  # create directory if it doesn't exist
 
         super(CachingDescriptorSystem, self).__init__(
@@ -59,16 +62,19 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         self.course_entry = course_entry
         self.lazy = lazy
         self.module_data = module_data
-        # Compute inheritance
-        modulestore.inherit_settings(
-            course_entry['structure'].get('blocks', {}),
-            course_entry['structure'].get('root'),
-            course_entry.setdefault('inherited_settings', {}),
-        )
         self.default_class = default_class
         self.local_modules = {}
 
-    @contract(usage_key="BlockUsageLocator | BlockKey")
+    @lazy
+    @contract(returns="dict(BlockKey: BlockKey)")
+    def _parent_map(self):
+        parent_map = {}
+        for block_key, block in self.course_entry.structure['blocks'].iteritems():
+            for child in block['fields'].get('children', []):
+                parent_map[child] = block_key
+        return parent_map
+
+    @contract(usage_key="BlockUsageLocator | BlockKey", course_entry_override="CourseEnvelope | None")
     def _load_item(self, usage_key, course_entry_override=None, **kwargs):
         # usage_key is either a UsageKey or just the block_key. if a usage_key,
         if isinstance(usage_key, BlockUsageLocator):
@@ -88,20 +94,17 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
             block_key = usage_key
 
             course_info = course_entry_override or self.course_entry
-            course_key = CourseLocator(
-                version_guid=course_info['structure']['_id'],
-                org=course_info.get('org'),
-                course=course_info.get('course'),
-                run=course_info.get('run'),
-                branch=course_info.get('branch'),
-            )
+            course_key = course_info.course_key
+
+        if course_entry_override:
+            structure_id = course_entry_override.structure.get('_id')
+        else:
+            structure_id = self.course_entry.structure.get('_id')
 
         json_data = self.get_module_data(block_key, course_key)
 
         class_ = self.load_block_type(json_data.get('block_type'))
-        # pass None for inherited_settings to signal that it should get the settings from cache
-        new_item = self.xblock_from_json(class_, course_key, block_key, json_data, None, course_entry_override, **kwargs)
-        return new_item
+        return self.xblock_from_json(class_, course_key, block_key, json_data, course_entry_override, **kwargs)
 
     @contract(block_key=BlockKey, course_key=CourseLocator)
     def get_module_data(self, block_key, course_key):
@@ -134,36 +137,29 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
     # is the intended one when not given a course_entry_override; thus, the caching of the last branch/course id.
     @contract(block_key="BlockKey | None")
     def xblock_from_json(
-        self, class_, course_key, block_key, json_data, inherited_settings, course_entry_override=None, **kwargs
+        self, class_, course_key, block_key, json_data, course_entry_override=None, **kwargs
     ):
         if course_entry_override is None:
             course_entry_override = self.course_entry
         else:
             # most recent retrieval is most likely the right one for next caller (see comment above fn)
-            self.course_entry['branch'] = course_entry_override['branch']
-            self.course_entry['org'] = course_entry_override['org']
-            self.course_entry['course'] = course_entry_override['course']
-            self.course_entry['run'] = course_entry_override['run']
+            self.course_entry = CourseEnvelope(course_entry_override.course_key, self.course_entry.structure)
 
         definition_id = json_data.get('definition')
 
         # If no usage id is provided, generate an in-memory id
         if block_key is None:
             block_key = BlockKey(json_data['block_type'], LocalId())
-        else:
-            if inherited_settings is None:
-                # see if there's a value in course_entry
-                if block_key in self.course_entry['inherited_settings']:
-                    inherited_settings = self.course_entry['inherited_settings'][block_key]
-            elif block_key not in self.course_entry['inherited_settings']:
-                self.course_entry['inherited_settings'][block_key] = inherited_settings
 
         if definition_id is not None and not json_data.get('definition_loaded', False):
             definition_loader = DefinitionLazyLoader(
-                self.modulestore, block_key.type, definition_id,
+                self.modulestore,
+                course_key,
+                block_key.type,
+                definition_id,
                 lambda fields: self.modulestore.convert_references_to_keys(
                     course_key, self.load_block_type(block_key.type),
-                    fields, self.course_entry['structure']['blocks'],
+                    fields, self.course_entry.structure['blocks'],
                 )
             )
         else:
@@ -180,15 +176,24 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         )
 
         converted_fields = self.modulestore.convert_references_to_keys(
-            block_locator.course_key, class_, json_data.get('fields', {}), self.course_entry['structure']['blocks'],
+            block_locator.course_key, class_, json_data.get('fields', {}), self.course_entry.structure['blocks'],
         )
+        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
         kvs = SplitMongoKVS(
             definition_loader,
             converted_fields,
-            inherited_settings,
-            **kwargs
+            parent=parent,
+            field_decorator=kwargs.get('field_decorator')
         )
-        field_data = KvsFieldData(kvs)
+
+        if InheritanceMixin in self.modulestore.xblock_mixins:
+            field_data = inheriting_field_data(kvs)
+        else:
+            field_data = KvsFieldData(kvs)
 
         try:
             module = self.construct_xblock_from_class(
@@ -201,8 +206,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
             return ErrorDescriptor.from_json(
                 json_data,
                 self,
-                BlockUsageLocator(
-                    CourseLocator(version_guid=course_entry_override['structure']['_id']),
+                course_entry_override.course_key.make_usage_key(
                     block_type='error',
                     block_id=block_key.id
                 ),

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

@@ -8,13 +8,14 @@ class DefinitionLazyLoader(object):
     object doesn't force access during init but waits until client wants the
     definition. Only works if the modulestore is a split mongo store.
     """
-    def __init__(self, modulestore, block_type, definition_id, field_converter):
+    def __init__(self, modulestore, course_key, block_type, definition_id, field_converter):
         """
         Simple placeholder for yet-to-be-fetched data
         :param modulestore: the pymongo db connection with the definitions
         :param definition_locator: the id of the record in the above to fetch
         """
         self.modulestore = modulestore
+        self.course_key = course_key
         self.definition_locator = DefinitionLocator(block_type, definition_id)
         self.field_converter = field_converter
 
@@ -23,4 +24,4 @@ class DefinitionLazyLoader(object):
         Fetch the definition. Note, the caller should replace this lazy
         loader pointer with the result so as not to fetch more than once
         """
-        return self.modulestore.db_connection.get_definition(self.definition_locator.definition_id)
+        return self.modulestore.get_definition(self.course_key, self.definition_locator.definition_id)

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

@@ -3,7 +3,14 @@ Segregation of pymongo functions from the data modeling mechanisms for split mod
 """
 import re
 import pymongo
+import time
+
+# Import this just to export it
+from pymongo.errors import DuplicateKeyError  # pylint: disable=unused-import
+
 from contracts import check
+from functools import wraps
+from pymongo.errors import AutoReconnect
 from xmodule.exceptions import HeartbeatFailure
 from xmodule.modulestore.split_mongo import BlockKey
 from datetime import tzinfo
@@ -62,6 +69,32 @@ def structure_to_mongo(structure):
     return new_structure
 
 
+def autoretry_read(wait=0.1, retries=5):
+    """
+    Automatically retry a read-only method in the case of a pymongo
+    AutoReconnect exception.
+
+    See http://emptysqua.re/blog/save-the-monkey-reliably-writing-to-mongodb/
+    for a discussion of this technique.
+    """
+    def decorate(fn):
+        @wraps(fn)
+        def wrapper(*args, **kwargs):
+            for attempt in xrange(retries):
+                try:
+                    return fn(*args, **kwargs)
+                    break
+                except AutoReconnect:
+                    # Reraise if we failed on our last attempt
+                    if attempt == retries - 1:
+                        raise
+
+                    if wait:
+                        time.sleep(wait)
+        return wrapper
+    return decorate
+
+
 class MongoConnection(object):
     """
     Segregation of pymongo functions from the data modeling mechanisms for split modulestore.
@@ -106,12 +139,14 @@ class MongoConnection(object):
         else:
             raise HeartbeatFailure("Can't connect to {}".format(self.database.name))
 
+    @autoretry_read()
     def get_structure(self, key):
         """
         Get the structure from the persistence mechanism whose id is the given key
         """
         return structure_from_mongo(self.structures.find_one({'_id': key}))
 
+    @autoretry_read()
     def find_structures_by_id(self, ids):
         """
         Return all structures that specified in ``ids``.
@@ -121,6 +156,7 @@ class MongoConnection(object):
         """
         return [structure_from_mongo(structure) for structure in self.structures.find({'_id': {'$in': ids}})]
 
+    @autoretry_read()
     def find_structures_derived_from(self, ids):
         """
         Return all structures that were immediately derived from a structure listed in ``ids``.
@@ -130,6 +166,7 @@ class MongoConnection(object):
         """
         return [structure_from_mongo(structure) for structure in self.structures.find({'previous_version': {'$in': ids}})]
 
+    @autoretry_read()
     def find_ancestor_structures(self, original_version, block_key):
         """
         Find all structures that originated from ``original_version`` that contain ``block_key``.
@@ -149,12 +186,13 @@ class MongoConnection(object):
             }
         })]
 
-    def upsert_structure(self, structure):
+    def insert_structure(self, structure):
         """
-        Update the db record for structure, creating that record if it doesn't already exist
+        Insert a new structure into the database.
         """
-        self.structures.update({'_id': structure['_id']}, structure_to_mongo(structure), upsert=True)
+        self.structures.insert(structure_to_mongo(structure))
 
+    @autoretry_read()
     def get_course_index(self, key, ignore_case=False):
         """
         Get the course_index from the persistence mechanism whose id is the given key
@@ -171,6 +209,7 @@ class MongoConnection(object):
             }
         return self.course_index.find_one(query)
 
+    @autoretry_read()
     def find_matching_course_indexes(self, branch=None, search_targets=None):
         """
         Find the course_index matching particular conditions.
@@ -229,18 +268,19 @@ class MongoConnection(object):
             'run': course_index['run'],
         })
 
+    @autoretry_read()
     def get_definition(self, key):
         """
         Get the definition from the persistence mechanism whose id is the given key
         """
         return self.definitions.find_one({'_id': key})
 
-    def find_matching_definitions(self, query):
+    @autoretry_read()
+    def get_definitions(self, definitions):
         """
-        Find the definitions matching the query. Right now the query must be a legal mongo query
-        :param query: a mongo-style query of {key: [value|{$in ..}|..], ..}
+        Retrieve all definitions listed in `definitions`.
         """
-        return self.definitions.find(query)
+        return self.definitions.find({'$in': {'_id': definitions}})
 
     def insert_definition(self, definition):
         """

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

@@ -75,9 +75,10 @@ from xmodule.modulestore import (
 
 from ..exceptions import ItemNotFoundError
 from .caching_descriptor_system import CachingDescriptorSystem
-from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection, BlockKey
+from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection, DuplicateKeyError
+from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope
 from xmodule.error_module import ErrorDescriptor
-from _collections import defaultdict
+from collections import defaultdict
 from types import NoneType
 
 
@@ -116,6 +117,8 @@ class SplitBulkWriteRecord(BulkOpsRecord):
         self.index = None
         self.structures = {}
         self.structures_in_db = set()
+        self.definitions = {}
+        self.definitions_in_db = set()
 
     # TODO: This needs to track which branches have actually been modified/versioned,
     # so that copying one branch to another doesn't update the original branch.
@@ -223,7 +226,22 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
         """
         # If the content is dirty, then update the database
         for _id in bulk_write_record.structures.viewkeys() - bulk_write_record.structures_in_db:
-            self.db_connection.upsert_structure(bulk_write_record.structures[_id])
+            try:
+                self.db_connection.insert_structure(bulk_write_record.structures[_id])
+            except DuplicateKeyError:
+                # We may not have looked up this structure inside this bulk operation, and thus
+                # didn't realize that it was already in the database. That's OK, the store is
+                # append only, so if it's already been written, we can just keep going.
+                log.debug("Attempted to insert duplicate structure %s", _id)
+
+        for _id in bulk_write_record.definitions.viewkeys() - bulk_write_record.definitions_in_db:
+            try:
+                self.db_connection.insert_definition(bulk_write_record.definitions[_id])
+            except DuplicateKeyError:
+                # We may not have looked up this definition inside this bulk operation, and thus
+                # didn't realize that it was already in the database. That's OK, the store is
+                # append only, so if it's already been written, we can just keep going.
+                log.debug("Attempted to insert duplicate definition %s", _id)
 
         if bulk_write_record.index is not None and bulk_write_record.index != bulk_write_record.initial_index:
             if bulk_write_record.initial_index is None:
@@ -289,7 +307,71 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
         if bulk_write_record.active:
             bulk_write_record.structures[structure['_id']] = structure
         else:
-            self.db_connection.upsert_structure(structure)
+            self.db_connection.insert_structure(structure)
+
+    def get_definition(self, course_key, definition_guid):
+        """
+        Retrieve a single definition by id, respecting the active bulk operation
+        on course_key.
+
+        Args:
+            course_key (:class:`.CourseKey`): The course being operated on
+            definition_guid (str or ObjectID): The id of the definition to load
+        """
+        bulk_write_record = self._get_bulk_ops_record(course_key)
+        if bulk_write_record.active:
+            definition = bulk_write_record.definitions.get(definition_guid)
+
+            # The definition hasn't been loaded from the db yet, so load it
+            if definition is None:
+                definition = self.db_connection.get_definition(definition_guid)
+                bulk_write_record.definitions[definition_guid] = definition
+                if definition is not None:
+                    bulk_write_record.definitions_in_db.add(definition_guid)
+
+            return definition
+        else:
+            # cast string to ObjectId if necessary
+            definition_guid = course_key.as_object_id(definition_guid)
+            return self.db_connection.get_definition(definition_guid)
+
+    def get_definitions(self, course_key, ids):
+        """
+        Return all definitions that specified in ``ids``.
+
+        If a definition with the same id is in both the cache and the database,
+        the cached version will be preferred.
+
+        Arguments:
+            course_key (:class:`.CourseKey`): The course that these definitions are being loaded
+                for (to respect bulk operations).
+            ids (list): A list of definition ids
+        """
+        definitions = []
+        ids = set(ids)
+
+        bulk_write_record = self._get_bulk_ops_record(course_key)
+        if bulk_write_record.active:
+            for definition in bulk_write_record.definitions.values():
+                definition_id = definition.get('_id')
+                if definition_id in ids:
+                    ids.remove(definition_id)
+                    definitions.append(definition)
+
+        definitions.extend(self.db_connection.get_definitions(list(ids)))
+        return definitions
+
+
+    def update_definition(self, course_key, definition):
+        """
+        Update a definition, respecting the current bulk operation status
+        (no data will be written to the database if a bulk operation is active.)
+        """
+        bulk_write_record = self._get_bulk_ops_record(course_key)
+        if bulk_write_record.active:
+            bulk_write_record.definitions[definition['_id']] = definition
+        else:
+            self.db_connection.insert_definition(definition)
 
     def version_structure(self, course_key, structure, user_id):
         """
@@ -521,7 +603,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             new_module_data = {}
             for block_id in base_block_ids:
                 new_module_data = self.descendants(
-                    system.course_entry['structure']['blocks'],
+                    system.course_entry.structure['blocks'],
                     block_id,
                     depth,
                     new_module_data
@@ -529,9 +611,13 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
 
             if not lazy:
                 # Load all descendants by id
-                descendent_definitions = self.db_connection.find_matching_definitions({
-                    '_id': {'$in': [block['definition']
-                                    for block in new_module_data.itervalues()]}})
+                descendent_definitions = self.get_definitions(
+                    course_key,
+                    [
+                        block['definition']
+                        for block in new_module_data.itervalues()
+                    ]
+                )
                 # turn into a map
                 definitions = {definition['_id']: definition
                                for definition in descendent_definitions}
@@ -541,7 +627,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                         converted_fields = self.convert_references_to_keys(
                             course_key, system.load_block_type(block['block_type']),
                             definitions[block['definition']].get('fields'),
-                            system.course_entry['structure']['blocks'],
+                            system.course_entry.structure['blocks'],
                         )
                         block['fields'].update(converted_fields)
                         block['definition_loaded'] = True
@@ -549,24 +635,18 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             system.module_data.update(new_module_data)
             return system.module_data
 
+    @contract(course_entry=CourseEnvelope, block_keys="list(BlockKey)", depth="int | None")
     def _load_items(self, course_entry, block_keys, depth=0, lazy=True, **kwargs):
         '''
         Load & cache the given blocks from the course. Prefetch down to the
         given depth. Load the definitions into each block if lazy is False;
         otherwise, use the lazy definition placeholder.
         '''
-        runtime = self._get_cache(course_entry['structure']['_id'])
+        runtime = self._get_cache(course_entry.structure['_id'])
         if runtime is None:
             runtime = self.create_runtime(course_entry, lazy)
-            self._add_cache(course_entry['structure']['_id'], runtime)
-            course_key = CourseLocator(
-                version_guid=course_entry['structure']['_id'],
-                org=course_entry.get('org'),
-                course=course_entry.get('course'),
-                run=course_entry.get('run'),
-                branch=course_entry.get('branch'),
-            )
-            self.cache_items(runtime, block_keys, course_key, depth, lazy)
+            self._add_cache(course_entry.structure['_id'], runtime)
+            self.cache_items(runtime, block_keys, course_entry.course_key, depth, lazy)
         return [runtime.load_item(block_key, course_entry, **kwargs) for block_key in block_keys]
 
     def _get_cache(self, course_version_guid):
@@ -650,14 +730,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         # 'run', and 'branch' are not intrinsic to structure
         # and the one assoc'd w/ it by another fetch may not be the one relevant to this fetch; so,
         # add it in the envelope for the structure.
-        envelope = {
-            'org': course_key.org,
-            'course': course_key.course,
-            'run': course_key.run,
-            'branch': course_key.branch,
-            'structure': entry,
-        }
-        return envelope
+        return CourseEnvelope(course_key.replace(version_guid=version_guid), entry)
 
     def get_courses(self, branch, **kwargs):
         '''
@@ -690,13 +763,15 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         result = []
         for entry in matching_structures:
             course_info = id_version_map[entry['_id']]
-            envelope = {
-                'org': course_info['org'],
-                'course': course_info['course'],
-                'run': course_info['run'],
-                'branch': branch,
-                'structure': entry,
-            }
+            envelope = CourseEnvelope(
+                CourseLocator(
+                    org=course_info['org'],
+                    course=course_info['course'],
+                    run=course_info['run'],
+                    branch=branch,
+                ),
+                entry
+            )
             root = entry['root']
             course_list = self._load_items(envelope, [root], 0, lazy=True, **kwargs)
             if not isinstance(course_list[0], ErrorDescriptor):
@@ -716,12 +791,12 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         '''
         Gets the course descriptor for the course identified by the locator
         '''
-        if not isinstance(course_id, CourseLocator):
+        if not isinstance(course_id, CourseLocator) or course_id.deprecated:
             # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore.
             raise ItemNotFoundError(course_id)
 
         course_entry = self._lookup_course(course_id)
-        root = course_entry['structure']['root']
+        root = course_entry.structure['root']
         result = self._load_items(course_entry, [root], depth, lazy=True, **kwargs)
         return result[0]
 
@@ -734,7 +809,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         Note: we return the course_id instead of a boolean here since the found course may have
            a different id than the given course_id when ignore_case is True.
         '''
-        if not isinstance(course_id, CourseLocator):
+        if not isinstance(course_id, CourseLocator) or course_id.deprecated:
             # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore.
             return False
 
@@ -747,10 +822,14 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         the course or the block w/in the course do not exist for the given version.
         raises InsufficientSpecificationError if the usage_key does not id a block
         """
+        if not isinstance(usage_key, BlockUsageLocator) or usage_key.deprecated:
+            # The supplied UsageKey is of the wrong type, so it can't possibly be stored in this modulestore.
+            return False
+
         if usage_key.block_id is None:
             raise InsufficientSpecificationError(usage_key)
         try:
-            course_structure = self._lookup_course(usage_key.course_key)['structure']
+            course_structure = self._lookup_course(usage_key.course_key).structure
         except ItemNotFoundError:
             # this error only occurs if the course does not exist
             return False
@@ -766,7 +845,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             descendants.
         raises InsufficientSpecificationError or ItemNotFoundError
         """
-        if not isinstance(usage_key, BlockUsageLocator):
+        if not isinstance(usage_key, BlockUsageLocator) or usage_key.deprecated:
             # The supplied UsageKey is of the wrong type, so it can't possibly be stored in this modulestore.
             raise ItemNotFoundError(usage_key)
 
@@ -799,6 +878,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                 For split,
                 you can search by ``edited_by``, ``edited_on`` providing a function testing limits.
         """
+        if not isinstance(course_locator, CourseLocator) or course_locator.deprecated:
+            # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore.
+            return []
+
         course = self._lookup_course(course_locator)
         items = []
         qualifiers = qualifiers.copy() if qualifiers else {}  # copy the qualifiers (destructively manipulated here)
@@ -813,7 +896,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                 self._block_matches(block_json.get('fields', {}), settings)
             ):
                 if content:
-                    definition_block = self.db_connection.get_definition(block_json['definition'])
+                    definition_block = self.get_definition(course_locator, block_json['definition'])
                     return self._block_matches(definition_block.get('fields', {}), content)
                 else:
                     return True
@@ -824,7 +907,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             # odd case where we don't search just confirm
             block_name = qualifiers.pop('name')
             block_ids = []
-            for block_id, block in course['structure']['blocks'].iteritems():
+            for block_id, block in course.structure['blocks'].iteritems():
                 if block_name == block_id.id and _block_matches_all(block):
                     block_ids.append(block_id)
 
@@ -836,7 +919,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         # don't expect caller to know that children are in fields
         if 'children' in qualifiers:
             settings['children'] = qualifiers.pop('children')
-        for block_id, value in course['structure']['blocks'].iteritems():
+        for block_id, value in course.structure['blocks'].iteritems():
             if _block_matches_all(value):
                 items.append(block_id)
 
@@ -853,8 +936,12 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
 
         :param locator: BlockUsageLocator restricting search scope
         '''
+        if not isinstance(locator, BlockUsageLocator) or locator.deprecated:
+            # The supplied locator is of the wrong type, so it can't possibly be stored in this modulestore.
+            raise ItemNotFoundError(locator)
+
         course = self._lookup_course(locator.course_key)
-        parent_id = self._get_parent_from_structure(BlockKey.from_usage_key(locator), course['structure'])
+        parent_id = self._get_parent_from_structure(BlockKey.from_usage_key(locator), course.structure)
         if parent_id is None:
             return None
         return BlockUsageLocator.make_relative(
@@ -867,11 +954,15 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         """
         Return an array of all of the orphans in the course.
         """
+        if not isinstance(course_key, CourseLocator) or course_key.deprecated:
+            # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore.
+            raise ItemNotFoundError(course_key)
+
         detached_categories = [name for name, __ in XBlock.load_tagged_classes("detached")]
         course = self._lookup_course(course_key)
-        items = set(course['structure']['blocks'].keys())
-        items.remove(course['structure']['root'])
-        blocks = course['structure']['blocks']
+        items = set(course.structure['blocks'].keys())
+        items.remove(course.structure['root'])
+        blocks = course.structure['blocks']
         for block_id, block_data in blocks.iteritems():
             items.difference_update(BlockKey(*child) for child in block_data.get('fields', {}).get('children', []))
             if block_data['block_type'] in detached_categories:
@@ -895,6 +986,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             'edited_on': when the course was originally created
         }
         """
+        if not isinstance(course_key, CourseLocator) or course_key.deprecated:
+            # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore.
+            raise ItemNotFoundError(course_key)
+
         if not (course_key.course and course_key.run and course_key.org):
             return None
         index = self.get_course_index(course_key)
@@ -912,7 +1007,11 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             'edited_on': when the change was made
         }
         """
-        course = self._lookup_course(course_key)['structure']
+        if not isinstance(course_key, CourseLocator) or course_key.deprecated:
+            # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore.
+            raise ItemNotFoundError(course_key)
+
+        course = self._lookup_course(course_key).structure
         return {
             'original_version': course['original_version'],
             'previous_version': course['previous_version'],
@@ -930,6 +1029,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             'edited_on': when the change was made
         }
         """
+        if not isinstance(definition_locator, DefinitionLocator) or definition_locator.deprecated:
+            # The supplied locator is of the wrong type, so it can't possibly be stored in this modulestore.
+            raise ItemNotFoundError(definition_locator)
+
         definition = self.db_connection.get_definition(definition_locator.definition_id)
         if definition is None:
             return None
@@ -942,11 +1045,15 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         next versions, these do include those created for other courses.
         :param course_locator:
         '''
+        if not isinstance(course_locator, CourseLocator) or course_locator.deprecated:
+            # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore.
+            raise ItemNotFoundError(course_locator)
+
         if version_history_depth < 1:
             return None
         if course_locator.version_guid is None:
             course = self._lookup_course(course_locator)
-            version_guid = course['structure']['_id']
+            version_guid = course.structure['_id']
             course_locator = course_locator.for_version(version_guid)
         else:
             version_guid = course_locator.version_guid
@@ -977,7 +1084,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
 
         '''
         # course_agnostic means we don't care if the head and version don't align, trust the version
-        course_struct = self._lookup_course(block_locator.course_key.course_agnostic())['structure']
+        course_struct = self._lookup_course(block_locator.course_key.course_agnostic()).structure
         block_key = BlockKey.from_usage_key(block_locator)
         all_versions_with_block = self.find_ancestor_structures(
             original_version=course_struct['original_version'],
@@ -1026,7 +1133,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         # TODO implement
         raise NotImplementedError()
 
-    def create_definition_from_data(self, new_def_data, category, user_id):
+    def create_definition_from_data(self, course_key, new_def_data, category, user_id):
         """
         Pull the definition fields out of descriptor and save to the db as a new definition
         w/o a predecessor and return the new id.
@@ -1047,11 +1154,11 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             },
             'schema_version': self.SCHEMA_VERSION,
         }
-        self.db_connection.insert_definition(document)
+        self.update_definition(course_key, document)
         definition_locator = DefinitionLocator(category, new_id)
         return definition_locator
 
-    def update_definition_from_data(self, definition_locator, new_def_data, user_id):
+    def update_definition_from_data(self, course_key, definition_locator, new_def_data, user_id):
         """
         See if new_def_data differs from the persisted version. If so, update
         the persisted version and return the new id.
@@ -1068,22 +1175,23 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
 
         # if this looks in cache rather than fresh fetches, then it will probably not detect
         # actual change b/c the descriptor and cache probably point to the same objects
-        old_definition = self.db_connection.get_definition(definition_locator.definition_id)
+        old_definition = self.get_definition(course_key, definition_locator.definition_id)
         if old_definition is None:
             raise ItemNotFoundError(definition_locator)
 
         new_def_data = self._serialize_fields(old_definition['block_type'], new_def_data)
         if needs_saved():
-            # new id to create new version
-            old_definition['_id'] = ObjectId()
-            old_definition['fields'] = new_def_data
-            old_definition['edit_info']['edited_by'] = user_id
-            old_definition['edit_info']['edited_on'] = datetime.datetime.now(UTC)
+            # Do a deep copy so that we don't corrupt the cached version of the definition
+            new_definition = copy.deepcopy(old_definition)
+            new_definition['_id'] = ObjectId()
+            new_definition['fields'] = new_def_data
+            new_definition['edit_info']['edited_by'] = user_id
+            new_definition['edit_info']['edited_on'] = datetime.datetime.now(UTC)
             # previous version id
-            old_definition['edit_info']['previous_version'] = definition_locator.definition_id
-            old_definition['schema_version'] = self.SCHEMA_VERSION
-            self.db_connection.insert_definition(old_definition)
-            return DefinitionLocator(old_definition['block_type'], old_definition['_id']), True
+            new_definition['edit_info']['previous_version'] = definition_locator.definition_id
+            new_definition['schema_version'] = self.SCHEMA_VERSION
+            self.update_definition(course_key, new_definition)
+            return DefinitionLocator(new_definition['block_type'], new_definition['_id']), True
         else:
             return definition_locator, False
 
@@ -1164,15 +1272,15 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
 
             # find course_index entry if applicable and structures entry
             index_entry = self._get_index_if_valid(course_key, force)
-            structure = self._lookup_course(course_key)['structure']
+            structure = self._lookup_course(course_key).structure
 
             partitioned_fields = self.partition_fields_by_scope(block_type, fields)
             new_def_data = partitioned_fields.get(Scope.content, {})
             # persist the definition if persisted != passed
             if (definition_locator is None or isinstance(definition_locator.definition_id, LocalId)):
-                definition_locator = self.create_definition_from_data(new_def_data, block_type, user_id)
+                definition_locator = self.create_definition_from_data(course_key, new_def_data, block_type, user_id)
             elif new_def_data is not None:
-                definition_locator, _ = self.update_definition_from_data(definition_locator, new_def_data, user_id)
+                definition_locator, _ = self.update_definition_from_data(course_key, definition_locator, new_def_data, user_id)
 
             # copy the structure and modify the new one
             new_structure = self.version_structure(course_key, structure, user_id)
@@ -1243,7 +1351,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                 **kwargs)
 
             # don't version the structure as create_item handled that already.
-            new_structure = self._lookup_course(xblock.location.course_key)['structure']
+            new_structure = self._lookup_course(xblock.location.course_key).structure
 
             # add new block as child and update parent's version
             block_id = BlockKey.from_usage_key(parent_usage_key)
@@ -1365,7 +1473,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                 },
                 'schema_version': self.SCHEMA_VERSION,
             }
-            self.db_connection.insert_definition(definition_entry)
+            self.update_definition(locator, definition_entry)
 
             draft_structure = self._new_structure(
                 user_id,
@@ -1386,21 +1494,21 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         elif block_fields or definition_fields:  # pointing to existing course w/ some overrides
             # just get the draft_version structure
             draft_version = CourseLocator(version_guid=versions_dict[master_branch])
-            draft_structure = self._lookup_course(draft_version)['structure']
+            draft_structure = self._lookup_course(draft_version).structure
             draft_structure = self.version_structure(locator, draft_structure, user_id)
             new_id = draft_structure['_id']
             root_block = draft_structure['blocks'][draft_structure['root']]
             if block_fields is not None:
                 root_block['fields'].update(self._serialize_fields(root_category, block_fields))
             if definition_fields is not None:
-                definition = self.db_connection.get_definition(root_block['definition'])
+                definition = copy.deepcopy(self.get_definition(locator, root_block['definition']))
                 definition['fields'].update(definition_fields)
                 definition['edit_info']['previous_version'] = definition['_id']
                 definition['edit_info']['edited_by'] = user_id
                 definition['edit_info']['edited_on'] = datetime.datetime.now(UTC)
                 definition['_id'] = ObjectId()
                 definition['schema_version'] = self.SCHEMA_VERSION
-                self.db_connection.insert_definition(definition)
+                self.update_definition(locator, definition)
                 root_block['definition'] = definition['_id']
                 root_block['edit_info']['edited_on'] = datetime.datetime.now(UTC)
                 root_block['edit_info']['edited_by'] = user_id
@@ -1411,7 +1519,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         else:  # Pointing to an existing course structure
             new_id = versions_dict[master_branch]
             draft_version = CourseLocator(version_guid=new_id)
-            draft_structure = self._lookup_course(draft_version)['structure']
+            draft_structure = self._lookup_course(draft_version).structure
 
         locator = locator.replace(version_guid=new_id)
         with self.bulk_operations(locator):
@@ -1472,7 +1580,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                     user_id, course_key, block_key.type, fields=fields, force=force
                 )
 
-            original_structure = self._lookup_course(course_key)['structure']
+            original_structure = self._lookup_course(course_key).structure
             index_entry = self._get_index_if_valid(course_key, force)
 
             original_entry = self._get_block_from_structure(original_structure, block_key)
@@ -1493,7 +1601,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                 definition_locator = DefinitionLocator(original_entry['block_type'], original_entry['definition'])
             if definition_fields:
                 definition_locator, is_updated = self.update_definition_from_data(
-                    definition_locator, definition_fields, user_id
+                    course_key, definition_locator, definition_fields, user_id
                 )
 
             # check metadata
@@ -1582,7 +1690,6 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             course_key,
             BlockKey(block_type, block_id) if block_id else None,
             json_data,
-            inherited_settings,
             **kwargs
         )
         for field_name, value in (fields or {}).iteritems():
@@ -1615,10 +1722,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         course_key = xblock.location.course_key
         with self.bulk_operations(course_key):
             index_entry = self._get_index_if_valid(course_key, force)
-            structure = self._lookup_course(course_key)['structure']
+            structure = self._lookup_course(course_key).structure
             new_structure = self.version_structure(course_key, structure, user_id)
             new_id = new_structure['_id']
-            is_updated = self._persist_subdag(xblock, user_id, new_structure['blocks'], new_id)
+            is_updated = self._persist_subdag(course_key, xblock, user_id, new_structure['blocks'], new_id)
 
             if is_updated:
                 self.update_structure(course_key, new_structure)
@@ -1632,18 +1739,20 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             else:
                 return xblock
 
-    def _persist_subdag(self, xblock, user_id, structure_blocks, new_id):
+    def _persist_subdag(self, course_key, xblock, user_id, structure_blocks, new_id):
         # persist the definition if persisted != passed
         partitioned_fields = self.partition_xblock_fields_by_scope(xblock)
         new_def_data = self._serialize_fields(xblock.category, partitioned_fields[Scope.content])
         is_updated = False
         if xblock.definition_locator is None or isinstance(xblock.definition_locator.definition_id, LocalId):
             xblock.definition_locator = self.create_definition_from_data(
-                new_def_data, xblock.category, user_id)
+                course_key, new_def_data, xblock.category, user_id
+            )
             is_updated = True
         elif new_def_data:
             xblock.definition_locator, is_updated = self.update_definition_from_data(
-                xblock.definition_locator, new_def_data, user_id)
+                course_key, xblock.definition_locator, new_def_data, user_id
+            )
 
         if isinstance(xblock.scope_ids.usage_id.block_id, LocalId):
             # generate an id
@@ -1665,7 +1774,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
             for child in xblock.children:
                 if isinstance(child.block_id, LocalId):
                     child_block = xblock.system.get_block(child)
-                    is_updated = self._persist_subdag(child_block, user_id, structure_blocks, new_id) or is_updated
+                    is_updated = self._persist_subdag(course_key, child_block, user_id, structure_blocks, new_id) or is_updated
                     children.append(BlockKey.from_usage_key(child_block.location))
                 else:
                     children.append(BlockKey.from_usage_key(child))
@@ -1749,65 +1858,66 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         """
         # get the destination's index, and source and destination structures.
         with self.bulk_operations(source_course):
-            with self.bulk_operations(destination_course):
-                source_structure = self._lookup_course(source_course)['structure']
-                index_entry = self.get_course_index(destination_course)
-                if index_entry is None:
-                    # brand new course
-                    raise ItemNotFoundError(destination_course)
-                if destination_course.branch not in index_entry['versions']:
-                    # must be copying the dag root if there's no current dag
-                    root_block_key = source_structure['root']
-                    if not any(root_block_key == BlockKey.from_usage_key(subtree) for subtree in subtree_list):
-                        raise ItemNotFoundError(u'Must publish course root {}'.format(root_block_key))
-                    root_source = source_structure['blocks'][root_block_key]
-                    # create branch
-                    destination_structure = self._new_structure(
-                        user_id, root_block_key,
-                        # leave off the fields b/c the children must be filtered
-                        definition_id=root_source['definition'],
-                    )
-                else:
-                    destination_structure = self._lookup_course(destination_course)['structure']
-                    destination_structure = self.version_structure(destination_course, destination_structure, user_id)
-
-                if blacklist != EXCLUDE_ALL:
-                    blacklist = [BlockKey.from_usage_key(shunned) for shunned in blacklist or []]
-                # iterate over subtree list filtering out blacklist.
-                orphans = set()
-                destination_blocks = destination_structure['blocks']
-                for subtree_root in subtree_list:
-                    if BlockKey.from_usage_key(subtree_root) != source_structure['root']:
-                        # find the parents and put root in the right sequence
-                        parent = self._get_parent_from_structure(BlockKey.from_usage_key(subtree_root), source_structure)
-                        if parent is not None:  # may be a detached category xblock
-                            if not parent in destination_blocks:
-                                raise ItemNotFoundError(parent)
-                            orphans.update(
-                                self._sync_children(
-                                    source_structure['blocks'][parent],
-                                    destination_blocks[parent],
-                                    BlockKey.from_usage_key(subtree_root)
-                                )
+            source_structure = self._lookup_course(source_course).structure
+
+        with self.bulk_operations(destination_course):
+            index_entry = self.get_course_index(destination_course)
+            if index_entry is None:
+                # brand new course
+                raise ItemNotFoundError(destination_course)
+            if destination_course.branch not in index_entry['versions']:
+                # must be copying the dag root if there's no current dag
+                root_block_key = source_structure['root']
+                if not any(root_block_key == BlockKey.from_usage_key(subtree) for subtree in subtree_list):
+                    raise ItemNotFoundError(u'Must publish course root {}'.format(root_block_key))
+                root_source = source_structure['blocks'][root_block_key]
+                # create branch
+                destination_structure = self._new_structure(
+                    user_id, root_block_key,
+                    # leave off the fields b/c the children must be filtered
+                    definition_id=root_source['definition'],
+                )
+            else:
+                destination_structure = self._lookup_course(destination_course).structure
+                destination_structure = self.version_structure(destination_course, destination_structure, user_id)
+
+            if blacklist != EXCLUDE_ALL:
+                blacklist = [BlockKey.from_usage_key(shunned) for shunned in blacklist or []]
+            # iterate over subtree list filtering out blacklist.
+            orphans = set()
+            destination_blocks = destination_structure['blocks']
+            for subtree_root in subtree_list:
+                if BlockKey.from_usage_key(subtree_root) != source_structure['root']:
+                    # find the parents and put root in the right sequence
+                    parent = self._get_parent_from_structure(BlockKey.from_usage_key(subtree_root), source_structure)
+                    if parent is not None:  # may be a detached category xblock
+                        if not parent in destination_blocks:
+                            raise ItemNotFoundError(parent)
+                        orphans.update(
+                            self._sync_children(
+                                source_structure['blocks'][parent],
+                                destination_blocks[parent],
+                                BlockKey.from_usage_key(subtree_root)
                             )
-                    # update/create the subtree and its children in destination (skipping blacklist)
-                    orphans.update(
-                        self._copy_subdag(
-                            user_id, destination_structure['_id'],
-                            BlockKey.from_usage_key(subtree_root),
-                            source_structure['blocks'],
-                            destination_blocks,
-                            blacklist
                         )
+                # update/create the subtree and its children in destination (skipping blacklist)
+                orphans.update(
+                    self._copy_subdag(
+                        user_id, destination_structure['_id'],
+                        BlockKey.from_usage_key(subtree_root),
+                        source_structure['blocks'],
+                        destination_blocks,
+                        blacklist
                     )
-                # remove any remaining orphans
-                for orphan in orphans:
-                    # orphans will include moved as well as deleted xblocks. Only delete the deleted ones.
-                    self._delete_if_true_orphan(orphan, destination_structure)
+                )
+            # remove any remaining orphans
+            for orphan in orphans:
+                # orphans will include moved as well as deleted xblocks. Only delete the deleted ones.
+                self._delete_if_true_orphan(orphan, destination_structure)
 
-                # update the db
-                self.update_structure(destination_course, destination_structure)
-                self._update_head(destination_course, index_entry, destination_course.branch, destination_structure['_id'])
+            # update the db
+            self.update_structure(destination_course, destination_structure)
+            self._update_head(destination_course, index_entry, destination_course.branch, destination_structure['_id'])
 
     def delete_item(self, usage_locator, user_id, force=False):
         """
@@ -1824,12 +1934,12 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         change to this item, it raises a VersionConflictError unless force is True. In the force case, it forks
         the course but leaves the head pointer where it is (this change will not be in the course head).
         """
-        if not isinstance(usage_locator, BlockUsageLocator):
+        if not isinstance(usage_locator, BlockUsageLocator) or usage_locator.deprecated:
             # The supplied UsageKey is of the wrong type, so it can't possibly be stored in this modulestore.
             raise ItemNotFoundError(usage_locator)
 
         with self.bulk_operations(usage_locator.course_key):
-            original_structure = self._lookup_course(usage_locator.course_key)['structure']
+            original_structure = self._lookup_course(usage_locator.course_key).structure
             block_key = BlockKey.from_usage_key(usage_locator)
             if original_structure['root'] == block_key:
                 raise ValueError("Cannot delete the root of a course")
@@ -1973,7 +2083,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
 
         :param course_locator: the course to clean
         """
-        original_structure = self._lookup_course(course_locator)['structure']
+        original_structure = self._lookup_course(course_locator).structure
         for block in original_structure['blocks'].itervalues():
             if 'fields' in block and 'children' in block['fields']:
                 block['fields']["children"] = [

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

@@ -242,7 +242,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         :return: True if the draft and published versions differ
         """
         def get_course(branch_name):
-            return self._lookup_course(xblock.location.course_key.for_branch(branch_name))['structure']
+            return self._lookup_course(xblock.location.course_key.for_branch(branch_name)).structure
 
         def get_block(course_structure, block_key):
             return self._get_block_from_structure(course_structure, block_key)
@@ -318,7 +318,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
             # get head version of Published branch
             published_course_structure = self._lookup_course(
                 location.course_key.for_branch(ModuleStoreEnum.BranchName.published)
-            )['structure']
+            ).structure
             published_block = self._get_block_from_structure(
                 published_course_structure,
                 BlockKey.from_usage_key(location)
@@ -327,7 +327,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
                 raise InvalidVersionError(location)
 
             # create a new versioned draft structure
-            draft_course_structure = self._lookup_course(draft_course_key)['structure']
+            draft_course_structure = self._lookup_course(draft_course_key).structure
             new_structure = self.version_structure(draft_course_key, draft_course_structure, user_id)
 
             # remove the block and its descendants from the new structure
@@ -394,7 +394,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         pass
 
     def _get_head(self, xblock, branch):
-        course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch))['structure']
+        course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch)).structure
         return self._get_block_from_structure(course_structure, BlockKey.from_usage_key(xblock.location))
 
     def _get_version(self, block):

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

 import copy
+from contracts import contract, new_contract
 from xblock.fields import Scope
 from collections import namedtuple
 from xblock.exceptions import InvalidScopeError
 from .definition_lazy_loader import DefinitionLazyLoader
 from xmodule.modulestore.inheritance import InheritanceKeyValueStore
+from opaque_keys.edx.locator import BlockUsageLocator
 
 # id is a BlockUsageLocator, def_id is the definition's guid
 SplitMongoKVSid = namedtuple('SplitMongoKVSid', 'id, def_id')
+new_contract('BlockUsageLocator', BlockUsageLocator)
 
 
 class SplitMongoKVS(InheritanceKeyValueStore):
@@ -15,22 +18,25 @@ class SplitMongoKVS(InheritanceKeyValueStore):
     known to the MongoModuleStore (data, children, and metadata)
     """
 
-    def __init__(self, definition, initial_values, inherited_settings, **kwargs):
+    @contract(parent="BlockUsageLocator | None")
+    def __init__(self, definition, initial_values, parent, field_decorator=None):
         """
 
         :param definition: either a lazyloader or definition id for the definition
         :param initial_values: a dictionary of the locally set values
-        :param inherited_settings: the json value of each inheritable field from above this.
-            Note, local fields may override and disagree w/ this b/c this says what the value
-            should be if the field is undefined.
         """
         # deepcopy so that manipulations of fields does not pollute the source
-        super(SplitMongoKVS, self).__init__(copy.deepcopy(initial_values), inherited_settings)
+        super(SplitMongoKVS, self).__init__(copy.deepcopy(initial_values))
         self._definition = definition  # either a DefinitionLazyLoader or the db id of the definition.
         # if the db id, then the definition is presumed to be loaded into _fields
 
         # a decorator function for field values (to be called when a field is accessed)
-        self.field_decorator = kwargs.get('field_decorator', lambda x: x)
+        if field_decorator is None:
+            self.field_decorator = lambda x: x
+        else:
+            self.field_decorator = field_decorator
+
+        self.parent = parent
 
 
     def get(self, key):
@@ -38,8 +44,7 @@ class SplitMongoKVS(InheritanceKeyValueStore):
         if key.field_name not in self._fields:
             # parent undefined in editing runtime (I think)
             if key.scope == Scope.parent:
-                # see STUD-624. Right now copies MongoKeyValueStore.get's behavior of returning None
-                return None
+                return self.parent
             if key.scope == Scope.children:
                 # didn't find children in _fields; so, see if there's a default
                 raise KeyError()

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

@@ -28,6 +28,8 @@ from xmodule.modulestore.xml_importer import import_from_xml
 from xmodule.modulestore.xml_exporter import export_to_xml
 from xmodule.modulestore.split_mongo.split_draft import DraftVersioningModuleStore
 from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST
+from xmodule.modulestore.inheritance import InheritanceMixin
+from xmodule.x_module import XModuleMixin
 
 
 COMMON_DOCSTORE_CONFIG = {
@@ -36,6 +38,9 @@ COMMON_DOCSTORE_CONFIG = {
 }
 
 
+XBLOCK_MIXINS = (InheritanceMixin, XModuleMixin)
+
+
 class MemoryCache(object):
     """
     This fits the metadata_inheritance_cache_subsystem interface used by
@@ -95,6 +100,7 @@ class MongoModulestoreBuilder(object):
             render_template=repr,
             branch_setting_func=lambda: ModuleStoreEnum.Branch.draft_preferred,
             metadata_inheritance_cache_subsystem=MemoryCache(),
+            xblock_mixins=XBLOCK_MIXINS,
         )
         modulestore.ensure_indexes()
 
@@ -139,6 +145,7 @@ class VersioningModulestoreBuilder(object):
             doc_store_config,
             fs_root,
             render_template=repr,
+            xblock_mixins=XBLOCK_MIXINS,
         )
         modulestore.ensure_indexes()
 
@@ -189,7 +196,13 @@ class MixedModulestoreBuilder(object):
             # Generate a fake list of stores to give the already generated stores appropriate names
             stores = [{'NAME': name, 'ENGINE': 'This space deliberately left blank'} for name in names]
 
-            modulestore = MixedModuleStore(contentstore, self.mappings, stores, create_modulestore_instance=create_modulestore_instance)
+            modulestore = MixedModuleStore(
+                contentstore,
+                self.mappings,
+                stores,
+                create_modulestore_instance=create_modulestore_instance,
+                xblock_mixins=XBLOCK_MIXINS,
+            )
 
             yield modulestore
 
@@ -269,8 +282,8 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest):
                 with dest_content_builder.build() as dest_content:
                     # Construct the modulestore for storing the second import (using the second contentstore)
                     with dest_builder.build(dest_content) as dest_store:
-                        source_course_key = source_store.make_course_key('source', 'course', 'key')
-                        dest_course_key = dest_store.make_course_key('dest', 'course', 'key')
+                        source_course_key = source_store.make_course_key('a', 'course', 'course')
+                        dest_course_key = dest_store.make_course_key('a', 'course', 'course')
 
                         import_from_xml(
                             source_store,
@@ -287,20 +300,30 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest):
                             source_content,
                             source_course_key,
                             self.export_dir,
-                            'exported_course',
+                            'exported_source_course',
                         )
 
                         import_from_xml(
                             dest_store,
                             'test_user',
                             self.export_dir,
+                            course_dirs=['exported_source_course'],
                             static_content_store=dest_content,
                             target_course_id=dest_course_key,
                             create_new_course_if_not_present=True,
                         )
 
+                        export_to_xml(
+                            dest_store,
+                            dest_content,
+                            dest_course_key,
+                            self.export_dir,
+                            'exported_dest_course',
+                        )
+
                         self.exclude_field(None, 'wiki_slug')
                         self.exclude_field(None, 'xml_attributes')
+                        self.exclude_field(None, 'parent')
                         self.ignore_asset_key('_id')
                         self.ignore_asset_key('uploadDate')
                         self.ignore_asset_key('content_son')

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

@@ -18,6 +18,7 @@ from uuid import uuid4
 # TODO remove this import and the configuration -- xmodule should not depend on django!
 from django.conf import settings
 from xmodule.modulestore.edit_info import EditInfoMixin
+from xmodule.modulestore.inheritance import InheritanceMixin
 
 if not settings.configured:
     settings.configure()
@@ -26,17 +27,17 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator
 from xmodule.exceptions import InvalidVersionError
 from xmodule.modulestore import ModuleStoreEnum
-from xmodule.modulestore.draft_and_published import UnsupportedRevisionError
+from xmodule.modulestore.draft_and_published import UnsupportedRevisionError, ModuleStoreDraftAndPublished
 from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError, ReferentialIntegrityError, NoPathToItem
 from xmodule.modulestore.mixed import MixedModuleStore
 from xmodule.modulestore.search import path_to_location
 from xmodule.modulestore.tests.factories import check_mongo_calls
 from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST
-from xmodule.tests import DATA_DIR
+from xmodule.tests import DATA_DIR, CourseComparisonTest
 
 
 @ddt.ddt
-class TestMixedModuleStore(unittest.TestCase):
+class TestMixedModuleStore(CourseComparisonTest):
     """
     Quasi-superclass which tests Location based apps against both split and mongo dbs (Locator and
     Location-based dbs)
@@ -58,7 +59,7 @@ class TestMixedModuleStore(unittest.TestCase):
         'default_class': DEFAULT_CLASS,
         'fs_root': DATA_DIR,
         'render_template': RENDER_TEMPLATE,
-        'xblock_mixins': (EditInfoMixin,)
+        'xblock_mixins': (EditInfoMixin, InheritanceMixin),
     }
     DOC_STORE_CONFIG = {
         'host': HOST,
@@ -244,7 +245,8 @@ class TestMixedModuleStore(unittest.TestCase):
             for course_id, course_key in self.course_locations.iteritems()  # pylint: disable=maybe-no-member
         }
 
-        self.fake_location = self.course_locations[self.MONGO_COURSEID].course_key.make_usage_key('vertical', 'fake')
+        mongo_course_key = self.course_locations[self.MONGO_COURSEID].course_key
+        self.fake_location = self.store.make_course_key(mongo_course_key.org, mongo_course_key.course, mongo_course_key.run).make_usage_key('vertical', 'fake')
 
         self.xml_chapter_location = self.course_locations[self.XML_COURSEID1].replace(
             category='chapter', name='Overview'
@@ -1046,7 +1048,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self.store.revert_to_published(self.vertical_x1a, self.user_id)
         reverted_parent = self.store.get_item(self.vertical_x1a)
         self.assertEqual(vertical_children_num, len(published_parent.children))
-        self.assertEqual(reverted_parent, published_parent)
+        self.assertBlocksEqualByFields(reverted_parent, published_parent)
         self.assertFalse(self._has_changes(self.vertical_x1a))
 
     @ddt.data('draft', 'split')
@@ -1081,7 +1083,8 @@ class TestMixedModuleStore(unittest.TestCase):
         orig_vertical = self.store.get_item(self.vertical_x1a)
         self.store.revert_to_published(self.vertical_x1a, self.user_id)
         reverted_vertical = self.store.get_item(self.vertical_x1a)
-        self.assertEqual(orig_vertical, reverted_vertical)
+
+        self.assertBlocksEqualByFields(orig_vertical, reverted_vertical)
 
     @ddt.data('draft', 'split')
     def test_revert_to_published_no_published(self, default_ms):
@@ -1787,9 +1790,11 @@ def create_modulestore_instance(engine, contentstore, doc_store_config, options,
     """
     class_ = load_function(engine)
 
+    if issubclass(class_, ModuleStoreDraftAndPublished):
+        options['branch_setting_func'] = lambda: ModuleStoreEnum.Branch.draft_preferred
+
     return class_(
         doc_store_config=doc_store_config,
         contentstore=contentstore,
-        branch_setting_func=lambda: ModuleStoreEnum.Branch.draft_preferred,
         **options
     )

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

@@ -10,6 +10,7 @@ from contracts import contract
 from importlib import import_module
 from path import path
 
+from xblock.fields import Reference, ReferenceList, ReferenceValueDict
 from xmodule.course_module import CourseDescriptor
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.exceptions import (
@@ -1592,7 +1593,7 @@ class TestInheritance(SplitModuleTest):
 
         # unset on parent, retrieve child, verify unset
         chapter = modulestore().get_item(chapter.location.version_agnostic())
-        chapter.fields['visible_to_staff_only'].delete_from(chapter)
+        del chapter.visible_to_staff_only
         modulestore().update_item(chapter, self.user_id)
 
         problem = modulestore().get_item(problem.location.version_agnostic())
@@ -1756,12 +1757,26 @@ class TestPublish(SplitModuleTest):
             for field in source.fields.values():
                 if field.name == 'children':
                     self._compare_children(field.read_from(source), field.read_from(pub_copy), unexpected_blocks)
+                elif isinstance(field, (Reference, ReferenceList, ReferenceValueDict)):
+                    self.assertReferenceEqual(field.read_from(source), field.read_from(pub_copy))
                 else:
                     self.assertEqual(field.read_from(source), field.read_from(pub_copy))
         for unexp in unexpected_blocks:
             with self.assertRaises(ItemNotFoundError):
                 modulestore().get_item(dest_course_loc.make_usage_key(unexp.type, unexp.id))
 
+    def assertReferenceEqual(self, expected, actual):
+        if isinstance(expected, BlockUsageLocator):
+            expected = BlockKey.from_usage_key(expected)
+            actual = BlockKey.from_usage_key(actual)
+        elif isinstance(expected, list):
+            expected = [BlockKey.from_usage_key(key) for key in expected]
+            actual = [BlockKey.from_usage_key(key) for key in actual]
+        elif isinstance(expected, dict):
+            expected = {key: BlockKey.from_usage_key(val) for (key, val) in expected}
+            actual = {key: BlockKey.from_usage_key(val) for (key, val) in actual}
+        self.assertEqual(expected, actual)
+
     @contract(
         source_children="list(BlockUsageLocator)",
         dest_children="list(BlockUsageLocator)",

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

@@ -21,6 +21,7 @@ class TestBulkWriteMixin(unittest.TestCase):
         self.course_key = CourseLocator('org', 'course', 'run-a', branch='test')
         self.course_key_b = CourseLocator('org', 'course', 'run-b', branch='test')
         self.structure = {'this': 'is', 'a': 'structure', '_id': ObjectId()}
+        self.definition = {'this': 'is', 'a': 'definition', '_id': ObjectId()}
         self.index_entry = {'this': 'is', 'an': 'index'}
 
     def assertConnCalls(self, *calls):
@@ -63,9 +64,23 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin):
         # call through to the db_connection. It should also clear the
         # system cache
         self.bulk.update_structure(self.course_key, self.structure)
-        self.assertConnCalls(call.upsert_structure(self.structure))
+        self.assertConnCalls(call.insert_structure(self.structure))
         self.clear_cache.assert_called_once_with(self.structure['_id'])
 
+    @ddt.data('deadbeef1234' * 2, u'deadbeef1234' * 2, ObjectId())
+    def test_no_bulk_read_definition(self, version_guid):
+        # Reading a definition when no bulk operation is active should just call
+        # through to the db_connection
+        result = self.bulk.get_definition(self.course_key, version_guid)
+        self.assertConnCalls(call.get_definition(self.course_key.as_object_id(version_guid)))
+        self.assertEqual(result, self.conn.get_definition.return_value)
+
+    def test_no_bulk_write_definition(self):
+        # Writing a definition when no bulk operation is active should just
+        # call through to the db_connection.
+        self.bulk.update_definition(self.course_key, self.definition)
+        self.assertConnCalls(call.insert_definition(self.definition))
+
     @ddt.data(True, False)
     def test_no_bulk_read_index(self, ignore_case):
         # Reading a course index when no bulk operation is active should just call
@@ -113,7 +128,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin):
         self.bulk.update_structure(self.course_key, self.structure)
         self.assertConnCalls()
         self.bulk._end_bulk_operation(self.course_key)
-        self.assertConnCalls(call.upsert_structure(self.structure))
+        self.assertConnCalls(call.insert_structure(self.structure))
 
     def test_write_multiple_structures_on_close(self):
         self.conn.get_course_index.return_value = None
@@ -125,7 +140,69 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin):
         self.assertConnCalls()
         self.bulk._end_bulk_operation(self.course_key)
         self.assertItemsEqual(
-            [call.upsert_structure(self.structure), call.upsert_structure(other_structure)],
+            [call.insert_structure(self.structure), call.insert_structure(other_structure)],
+            self.conn.mock_calls
+        )
+
+    def test_write_index_and_definition_on_close(self):
+        original_index = {'versions': {}}
+        self.conn.get_course_index.return_value = copy.deepcopy(original_index)
+        self.bulk._begin_bulk_operation(self.course_key)
+        self.conn.reset_mock()
+        self.bulk.update_definition(self.course_key, self.definition)
+        self.bulk.insert_course_index(self.course_key, {'versions': {self.course_key.branch: self.definition['_id']}})
+        self.assertConnCalls()
+        self.bulk._end_bulk_operation(self.course_key)
+        self.assertConnCalls(
+            call.insert_definition(self.definition),
+            call.update_course_index(
+                {'versions': {self.course_key.branch: self.definition['_id']}},
+                from_index=original_index
+            )
+        )
+
+    def test_write_index_and_multiple_definitions_on_close(self):
+        original_index = {'versions': {'a': ObjectId(), 'b': ObjectId()}}
+        self.conn.get_course_index.return_value = copy.deepcopy(original_index)
+        self.bulk._begin_bulk_operation(self.course_key)
+        self.conn.reset_mock()
+        self.bulk.update_definition(self.course_key.replace(branch='a'), self.definition)
+        other_definition = {'another': 'definition', '_id': ObjectId()}
+        self.bulk.update_definition(self.course_key.replace(branch='b'), other_definition)
+        self.bulk.insert_course_index(self.course_key, {'versions': {'a': self.definition['_id'], 'b': other_definition['_id']}})
+        self.bulk._end_bulk_operation(self.course_key)
+        self.assertItemsEqual(
+            [
+                call.insert_definition(self.definition),
+                call.insert_definition(other_definition),
+                call.update_course_index(
+                    {'versions': {'a': self.definition['_id'], 'b': other_definition['_id']}},
+                    from_index=original_index
+                )
+            ],
+            self.conn.mock_calls
+        )
+
+    def test_write_definition_on_close(self):
+        self.conn.get_course_index.return_value = None
+        self.bulk._begin_bulk_operation(self.course_key)
+        self.conn.reset_mock()
+        self.bulk.update_definition(self.course_key, self.definition)
+        self.assertConnCalls()
+        self.bulk._end_bulk_operation(self.course_key)
+        self.assertConnCalls(call.insert_definition(self.definition))
+
+    def test_write_multiple_definitions_on_close(self):
+        self.conn.get_course_index.return_value = None
+        self.bulk._begin_bulk_operation(self.course_key)
+        self.conn.reset_mock()
+        self.bulk.update_definition(self.course_key.replace(branch='a'), self.definition)
+        other_definition = {'another': 'definition', '_id': ObjectId()}
+        self.bulk.update_definition(self.course_key.replace(branch='b'), other_definition)
+        self.assertConnCalls()
+        self.bulk._end_bulk_operation(self.course_key)
+        self.assertItemsEqual(
+            [call.insert_definition(self.definition), call.insert_definition(other_definition)],
             self.conn.mock_calls
         )
 
@@ -139,7 +216,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin):
         self.assertConnCalls()
         self.bulk._end_bulk_operation(self.course_key)
         self.assertConnCalls(
-            call.upsert_structure(self.structure),
+            call.insert_structure(self.structure),
             call.update_course_index(
                 {'versions': {self.course_key.branch: self.structure['_id']}},
                 from_index=original_index
@@ -158,8 +235,8 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin):
         self.bulk._end_bulk_operation(self.course_key)
         self.assertItemsEqual(
             [
-                call.upsert_structure(self.structure),
-                call.upsert_structure(other_structure),
+                call.insert_structure(self.structure),
+                call.insert_structure(other_structure),
                 call.update_course_index(
                     {'versions': {'a': self.structure['_id'], 'b': other_structure['_id']}},
                     from_index=original_index
@@ -181,6 +258,7 @@ class TestBulkWriteMixinClosed(TestBulkWriteMixin):
         get_result = self.bulk.get_structure(self.course_key, version_result['_id'])
         self.assertEquals(version_result, get_result)
 
+
 class TestBulkWriteMixinClosedAfterPrevTransaction(TestBulkWriteMixinClosed, TestBulkWriteMixinPreviousTransaction):
     """
     Test that operations on with a closed transaction aren't affected by a previously executed transaction
@@ -307,6 +385,36 @@ class TestBulkWriteMixinFindMethods(TestBulkWriteMixin):
             else:
                 self.assertNotIn(db_structure(_id), results)
 
+    @ddt.data(
+        ([], [], []),
+        ([1, 2, 3], [1, 2], [1, 2]),
+        ([1, 2, 3], [1], [1, 2]),
+        ([1, 2, 3], [], [1, 2]),
+    )
+    @ddt.unpack
+    def test_get_definitions(self, search_ids, active_ids, db_ids):
+        db_definition = lambda _id: {'db': 'definition', '_id': _id}
+        active_definition = lambda _id: {'active': 'definition', '_id': _id}
+
+        db_definitions = [db_definition(_id) for _id in db_ids if _id not in active_ids]
+        self.bulk._begin_bulk_operation(self.course_key)
+        for n, _id in enumerate(active_ids):
+            self.bulk.update_definition(self.course_key, active_definition(_id))
+
+        self.conn.get_definitions.return_value = db_definitions
+        results = self.bulk.get_definitions(self.course_key, search_ids)
+        self.conn.get_definitions.assert_called_once_with(list(set(search_ids) - set(active_ids)))
+        for _id in active_ids:
+            if _id in search_ids:
+                self.assertIn(active_definition(_id), results)
+            else:
+                self.assertNotIn(active_definition(_id), results)
+        for _id in db_ids:
+            if _id in search_ids and _id not in active_ids:
+                self.assertIn(db_definition(_id), results)
+            else:
+                self.assertNotIn(db_definition(_id), results)
+
     def test_no_bulk_find_structures_derived_from(self):
         ids = [Mock(name='id')]
         self.conn.find_structures_derived_from.return_value = [MagicMock(name='result')]
@@ -456,6 +564,45 @@ class TestBulkWriteMixinOpen(TestBulkWriteMixin):
         self.assertEquals(self.conn.get_structure.call_count, 1)
         self.assertEqual(result, self.structure)
 
+    @ddt.data('deadbeef1234' * 2, u'deadbeef1234' * 2, ObjectId())
+    def test_read_definition_without_write_from_db(self, version_guid):
+        # Reading a definition before it's been written (while in bulk operation mode)
+        # returns the definition from the database
+        result = self.bulk.get_definition(self.course_key, version_guid)
+        self.assertEquals(self.conn.get_definition.call_count, 1)
+        self.assertEqual(result, self.conn.get_definition.return_value)
+        self.assertCacheNotCleared()
+
+    @ddt.data('deadbeef1234' * 2, u'deadbeef1234' * 2, ObjectId())
+    def test_read_definition_without_write_only_reads_once(self, version_guid):
+        # Reading the same definition multiple times shouldn't hit the database
+        # more than once
+        for _ in xrange(2):
+            result = self.bulk.get_definition(self.course_key, version_guid)
+            self.assertEquals(self.conn.get_definition.call_count, 1)
+            self.assertEqual(result, self.conn.get_definition.return_value)
+            self.assertCacheNotCleared()
+
+    @ddt.data('deadbeef1234' * 2, u'deadbeef1234' * 2, ObjectId())
+    def test_read_definition_after_write_no_db(self, version_guid):
+        # Reading a definition that's already been written shouldn't hit the db at all
+        self.definition['_id'] = version_guid
+        self.bulk.update_definition(self.course_key, self.definition)
+        result = self.bulk.get_definition(self.course_key, version_guid)
+        self.assertEquals(self.conn.get_definition.call_count, 0)
+        self.assertEqual(result, self.definition)
+
+    @ddt.data('deadbeef1234' * 2, u'deadbeef1234' * 2, ObjectId())
+    def test_read_definition_after_write_after_read(self, version_guid):
+        # Reading a definition that's been updated after being pulled from the db should
+        # still get the updated value
+        self.definition['_id'] = version_guid
+        self.bulk.get_definition(self.course_key, version_guid)
+        self.bulk.update_definition(self.course_key, self.definition)
+        result = self.bulk.get_definition(self.course_key, version_guid)
+        self.assertEquals(self.conn.get_definition.call_count, 1)
+        self.assertEqual(result, self.definition)
+
     @ddt.data(True, False)
     def test_read_index_without_write_from_db(self, ignore_case):
         # Reading the index without writing to it should pull from the database
@@ -521,7 +668,7 @@ class TestBulkWriteMixinOpen(TestBulkWriteMixin):
         index_copy['versions']['draft'] = index['versions']['published']
         self.bulk.update_course_index(self.course_key, index_copy)
         self.bulk._end_bulk_operation(self.course_key)
-        self.conn.upsert_structure.assert_called_once_with(published_structure)
+        self.conn.insert_structure.assert_called_once_with(published_structure)
         self.conn.update_course_index.assert_called_once_with(index_copy, from_index=self.conn.get_course_index.return_value)
         self.conn.get_course_index.assert_called_once_with(self.course_key)
 

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

@@ -370,7 +370,7 @@ class XMLModuleStore(ModuleStoreReadBase):
     """
     def __init__(
         self, data_dir, default_class=None, course_dirs=None, course_ids=None,
-        load_error_modules=True, i18n_service=None, pyfs_service=None, **kwargs
+        load_error_modules=True, i18n_service=None, fs_service=None, **kwargs
     ):
         """
         Initialize an XMLModuleStore from data_dir
@@ -409,7 +409,7 @@ class XMLModuleStore(ModuleStoreReadBase):
         self.field_data = inheriting_field_data(kvs=DictKeyValueStore())
 
         self.i18n_service = i18n_service
-        self.pyfs_service = pyfs_service
+        self.fs_service = fs_service
 
         # If we are specifically asked for missing courses, that should
         # be an error.  If we are asked for "all" courses, find the ones
@@ -555,8 +555,8 @@ class XMLModuleStore(ModuleStoreReadBase):
             if self.i18n_service:
                 services['i18n'] = self.i18n_service
 
-            if self.pyfs_service:
-                services['fs'] = self.pyfs_service
+            if self.fs_service:
+                services['fs'] = self.fs_service
 
             system = ImportSystem(
                 xmlstore=self,

common/lib/xmodule/xmodule/modulestore/xml_exporter.py

@@ -37,106 +37,108 @@ def export_to_xml(modulestore, contentstore, course_key, root_dir, course_dir):
     `course_dir`: The name of the directory inside `root_dir` to write the course content to
     """
 
-    course = modulestore.get_course(course_key, depth=None)  # None means infinite
-    fsm = OSFS(root_dir)
-    export_fs = course.runtime.export_fs = fsm.makeopendir(course_dir)
-
-    root = lxml.etree.Element('unknown')
-
-    # export only the published content
-    with modulestore.branch_setting(ModuleStoreEnum.Branch.published_only, course_key):
-        # change all of the references inside the course to use the xml expected key type w/o version & branch
-        xml_centric_course_key = CourseLocator(course_key.org, course_key.course, course_key.run, deprecated=True)
-        adapt_references(course, xml_centric_course_key, export_fs)
-
-        course.add_xml_to_node(root)
-
-    with export_fs.open('course.xml', 'w') as course_xml:
-        lxml.etree.ElementTree(root).write(course_xml)
-
-    # export the static assets
-    policies_dir = export_fs.makeopendir('policies')
-    if contentstore:
-        contentstore.export_all_for_course(
-            course_key,
-            root_dir + '/' + course_dir + '/static/',
-            root_dir + '/' + course_dir + '/policies/assets.json',
-        )
-
-        # If we are using the default course image, export it to the
-        # legacy location to support backwards compatibility.
-        if course.course_image == course.fields['course_image'].default:
-            try:
-                course_image = contentstore.find(
-                    StaticContent.compute_location(
-                        course.id,
-                        course.course_image
-                    ),
-                )
-            except NotFoundError:
-                pass
-            else:
-                output_dir = root_dir + '/' + course_dir + '/static/images/'
-                if not os.path.isdir(output_dir):
-                    os.makedirs(output_dir)
-                with OSFS(output_dir).open('course_image.jpg', 'wb') as course_image_file:
-                    course_image_file.write(course_image.data)
-
-    # export the static tabs
-    export_extra_content(export_fs, modulestore, xml_centric_course_key, 'static_tab', 'tabs', '.html')
-
-    # export the custom tags
-    export_extra_content(export_fs, modulestore, xml_centric_course_key, 'custom_tag_template', 'custom_tags')
-
-    # export the course updates
-    export_extra_content(export_fs, modulestore, xml_centric_course_key, 'course_info', 'info', '.html')
-
-    # export the 'about' data (e.g. overview, etc.)
-    export_extra_content(export_fs, modulestore, xml_centric_course_key, 'about', 'about', '.html')
-
-    # export the grading policy
-    course_run_policy_dir = policies_dir.makeopendir(course.location.name)
-    with course_run_policy_dir.open('grading_policy.json', 'w') as grading_policy:
-        grading_policy.write(dumps(course.grading_policy, cls=EdxJSONEncoder))
-
-    # export all of the course metadata in policy.json
-    with course_run_policy_dir.open('policy.json', 'w') as course_policy:
-        policy = {'course/' + course.location.name: own_metadata(course)}
-        course_policy.write(dumps(policy, cls=EdxJSONEncoder))
-
-    #### DRAFTS ####
-    # xml backed courses don't support drafts!
-    if course.runtime.modulestore.get_modulestore_type() != ModuleStoreEnum.Type.xml:
-        # NOTE: this code assumes that verticals are the top most draftable container
-        # should we change the application, then this assumption will no longer be valid
-        # NOTE: we need to explicitly implement the logic for setting the vertical's parent
-        # and index here since the XML modulestore cannot load draft modules
-        with modulestore.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key):
-            draft_verticals = modulestore.get_items(
+    with modulestore.bulk_operations(course_key):
+
+        course = modulestore.get_course(course_key, depth=None)  # None means infinite
+        fsm = OSFS(root_dir)
+        export_fs = course.runtime.export_fs = fsm.makeopendir(course_dir)
+
+        root = lxml.etree.Element('unknown')
+
+        # export only the published content
+        with modulestore.branch_setting(ModuleStoreEnum.Branch.published_only, course_key):
+            # change all of the references inside the course to use the xml expected key type w/o version & branch
+            xml_centric_course_key = CourseLocator(course_key.org, course_key.course, course_key.run, deprecated=True)
+            adapt_references(course, xml_centric_course_key, export_fs)
+
+            course.add_xml_to_node(root)
+
+        with export_fs.open('course.xml', 'w') as course_xml:
+            lxml.etree.ElementTree(root).write(course_xml)
+
+        # export the static assets
+        policies_dir = export_fs.makeopendir('policies')
+        if contentstore:
+            contentstore.export_all_for_course(
                 course_key,
-                qualifiers={'category': 'vertical'},
-                revision=ModuleStoreEnum.RevisionOption.draft_only
+                root_dir + '/' + course_dir + '/static/',
+                root_dir + '/' + course_dir + '/policies/assets.json',
             )
 
-            if len(draft_verticals) > 0:
-                draft_course_dir = export_fs.makeopendir(DRAFT_DIR)
-                for draft_vertical in draft_verticals:
-                    parent_loc = modulestore.get_parent_location(
-                        draft_vertical.location,
-                        revision=ModuleStoreEnum.RevisionOption.draft_preferred
+            # If we are using the default course image, export it to the
+            # legacy location to support backwards compatibility.
+            if course.course_image == course.fields['course_image'].default:
+                try:
+                    course_image = contentstore.find(
+                        StaticContent.compute_location(
+                            course.id,
+                            course.course_image
+                        ),
                     )
-                    # Don't try to export orphaned items.
-                    if parent_loc is not None:
-                        logging.debug('parent_loc = {0}'.format(parent_loc))
-                        if parent_loc.category in DIRECT_ONLY_CATEGORIES:
-                            draft_vertical.xml_attributes['parent_sequential_url'] = parent_loc.to_deprecated_string()
-                            sequential = modulestore.get_item(parent_loc)
-                            index = sequential.children.index(draft_vertical.location)
-                            draft_vertical.xml_attributes['index_in_children_list'] = str(index)
-                        draft_vertical.runtime.export_fs = draft_course_dir
-                        adapt_references(draft_vertical, xml_centric_course_key, draft_course_dir)
-                        node = lxml.etree.Element('unknown')
-                        draft_vertical.add_xml_to_node(node)
+                except NotFoundError:
+                    pass
+                else:
+                    output_dir = root_dir + '/' + course_dir + '/static/images/'
+                    if not os.path.isdir(output_dir):
+                        os.makedirs(output_dir)
+                    with OSFS(output_dir).open('course_image.jpg', 'wb') as course_image_file:
+                        course_image_file.write(course_image.data)
+
+        # export the static tabs
+        export_extra_content(export_fs, modulestore, course_key, xml_centric_course_key, 'static_tab', 'tabs', '.html')
+
+        # export the custom tags
+        export_extra_content(export_fs, modulestore, course_key, xml_centric_course_key, 'custom_tag_template', 'custom_tags')
+
+        # export the course updates
+        export_extra_content(export_fs, modulestore, course_key, xml_centric_course_key, 'course_info', 'info', '.html')
+
+        # export the 'about' data (e.g. overview, etc.)
+        export_extra_content(export_fs, modulestore, course_key, xml_centric_course_key, 'about', 'about', '.html')
+
+        # export the grading policy
+        course_run_policy_dir = policies_dir.makeopendir(course.location.name)
+        with course_run_policy_dir.open('grading_policy.json', 'w') as grading_policy:
+            grading_policy.write(dumps(course.grading_policy, cls=EdxJSONEncoder, sort_keys=True, indent=4))
+
+        # export all of the course metadata in policy.json
+        with course_run_policy_dir.open('policy.json', 'w') as course_policy:
+            policy = {'course/' + course.location.name: own_metadata(course)}
+            course_policy.write(dumps(policy, cls=EdxJSONEncoder, sort_keys=True, indent=4))
+
+        #### DRAFTS ####
+        # xml backed courses don't support drafts!
+        if course.runtime.modulestore.get_modulestore_type() != ModuleStoreEnum.Type.xml:
+            # NOTE: this code assumes that verticals are the top most draftable container
+            # should we change the application, then this assumption will no longer be valid
+            # NOTE: we need to explicitly implement the logic for setting the vertical's parent
+            # and index here since the XML modulestore cannot load draft modules
+            with modulestore.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key):
+                draft_verticals = modulestore.get_items(
+                    course_key,
+                    qualifiers={'category': 'vertical'},
+                    revision=ModuleStoreEnum.RevisionOption.draft_only
+                )
+
+                if len(draft_verticals) > 0:
+                    draft_course_dir = export_fs.makeopendir(DRAFT_DIR)
+                    for draft_vertical in draft_verticals:
+                        parent_loc = modulestore.get_parent_location(
+                            draft_vertical.location,
+                            revision=ModuleStoreEnum.RevisionOption.draft_preferred
+                        )
+                        # Don't try to export orphaned items.
+                        if parent_loc is not None:
+                            logging.debug('parent_loc = {0}'.format(parent_loc))
+                            if parent_loc.category in DIRECT_ONLY_CATEGORIES:
+                                draft_vertical.xml_attributes['parent_sequential_url'] = parent_loc.to_deprecated_string()
+                                sequential = modulestore.get_item(parent_loc)
+                                index = sequential.children.index(draft_vertical.location)
+                                draft_vertical.xml_attributes['index_in_children_list'] = str(index)
+                            draft_vertical.runtime.export_fs = draft_course_dir
+                            adapt_references(draft_vertical, xml_centric_course_key, draft_course_dir)
+                            node = lxml.etree.Element('unknown')
+                            draft_vertical.add_xml_to_node(node)
 
 
 def adapt_references(subtree, destination_course_key, export_fs):
@@ -178,16 +180,16 @@ def _export_field_content(xblock_item, item_dir):
                 # filename format: {dirname}.{field_name}.json
                 with item_dir.open('{0}.{1}.{2}'.format(xblock_item.location.name, field_name, 'json'),
                                    'w') as field_content_file:
-                    field_content_file.write(dumps(module_data.get(field_name, {}), cls=EdxJSONEncoder))
+                    field_content_file.write(dumps(module_data.get(field_name, {}), cls=EdxJSONEncoder, sort_keys=True, indent=4))
 
 
-def export_extra_content(export_fs, modulestore, course_key, category_type, dirname, file_suffix=''):
-    items = modulestore.get_items(course_key, qualifiers={'category': category_type})
+def export_extra_content(export_fs, modulestore, source_course_key, dest_course_key, category_type, dirname, file_suffix=''):
+    items = modulestore.get_items(source_course_key, qualifiers={'category': category_type})
 
     if len(items) > 0:
         item_dir = export_fs.makeopendir(dirname)
         for item in items:
-            adapt_references(item, course_key, export_fs)
+            adapt_references(item, dest_course_key, export_fs)
             with item_dir.open(item.location.name + file_suffix, 'w') as item_file:
                 item_file.write(item.data.encode('utf8'))
 

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -42,6 +42,7 @@ from xmodule.modulestore.django import ASSET_IGNORE_REGEX
 from xmodule.modulestore.exceptions import DuplicateCourseError
 from xmodule.modulestore.mongo.base import MongoRevisionKey
 from xmodule.modulestore import ModuleStoreEnum
+from xmodule.modulestore.exceptions import ItemNotFoundError
 
 log = logging.getLogger(__name__)
 
@@ -588,6 +589,7 @@ def _import_course_draft(
 
                             # IMPORTANT: Be sure to update the sequential in the NEW namespace
                             seq_location = seq_location.map_into_course(target_course_id)
+
                             sequential = store.get_item(seq_location, depth=0)
 
                             non_draft_location = module.location.map_into_course(target_course_id)

common/lib/xmodule/xmodule/partitions/partitions.py

@@ -42,6 +42,9 @@ class Group(namedtuple("Group", "id name")):
 
         Raises TypeError if the value doesn't have the right keys.
         """
+        if isinstance(value, Group):
+            return value
+
         for key in ('id', 'name', 'version'):
             if key not in value:
                 raise TypeError("Group dict {0} missing value key '{1}'".format(
@@ -96,6 +99,9 @@ class UserPartition(namedtuple("UserPartition", "id name description groups")):
 
         Raises TypeError if the value doesn't have the right keys.
         """
+        if isinstance(value, UserPartition):
+            return value
+
         for key in ('id', 'name', 'description', 'version', 'groups'):
             if key not in value:
                 raise TypeError("UserPartition dict {0} missing value key '{1}'"

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

@@ -13,7 +13,9 @@ import pprint
 import unittest
 
 from contextlib import contextmanager
+from lazy import lazy
 from mock import Mock
+from operator import attrgetter
 from path import path
 
 from xblock.field_data import DictFieldData
@@ -193,7 +195,7 @@ class BulkAssertionManager(object):
             self._equal_actual.append((description, actual))
 
     def run_assertions(self):
-        self._test_case.assertEqual(self._equal_expected, self._equal_actual)
+        super(BulkAssertionTest, self._test_case).assertEqual(self._equal_expected, self._equal_actual)
 
 
 class BulkAssertionTest(unittest.TestCase):
@@ -224,8 +226,29 @@ class BulkAssertionTest(unittest.TestCase):
             self._manager.assertEqual(expected, actual, message)
         else:
             super(BulkAssertionTest, self).assertEqual(expected, actual, message)
+    assertEquals = assertEqual
 
 
+class LazyFormat(object):
+    """
+    An stringy object that delays formatting until it's put into a string context.
+    """
+    __slots__ = ('template', 'args', 'kwargs', '_message')
+
+    def __init__(self, template, *args, **kwargs):
+        self.template = template
+        self.args = args
+        self.kwargs = kwargs
+        self._message = None
+
+    def __unicode__(self):
+        if self._message is None:
+            self._message = self.template.format(*self.args, **self.kwargs)
+        return self._message
+
+    def __repr__(self):
+        return unicode(self)
+
 class CourseComparisonTest(BulkAssertionTest):
     """
     Mixin that has methods for comparing courses for equality.
@@ -255,6 +278,65 @@ class CourseComparisonTest(BulkAssertionTest):
         """
         self.ignored_asset_keys.add(key_name)
 
+    def assertReferenceRelativelyEqual(self, reference_field, expected_block, actual_block):
+        """
+        Assert that the supplied reference field is identical on the expected_block and actual_block,
+        assoming that the references are only relative (that is, comparing only on block_type and block_id,
+        not course_key).
+        """
+        def extract_key(usage_key):
+            if usage_key is None:
+                return None
+            else:
+                return (usage_key.block_type, usage_key.block_id)
+        expected = reference_field.read_from(expected_block)
+        actual = reference_field.read_from(actual_block)
+        if isinstance(reference_field, Reference):
+            expected = extract_key(expected)
+            actual = extract_key(actual)
+        elif isinstance(reference_field, ReferenceList):
+            expected = [extract_key(key) for key in expected]
+            actual = [extract_key(key) for key in actual]
+        elif isinstance(reference_field, ReferenceValueDict):
+            expected = {key: extract_key(val) for (key, val) in expected.iteritems()}
+            actual = {key: extract_key(val) for (key, val) in actual.iteritems()}
+        self.assertEqual(
+            expected,
+            actual,
+            LazyFormat(
+                "Field {} doesn't match between usages {} and {}: {!r} != {!r}",
+                reference_field.name,
+                expected_block.scope_ids.usage_id,
+                actual_block.scope_ids.usage_id,
+                expected,
+                actual
+            )
+        )
+
+    def assertBlocksEqualByFields(self, expected_block, actual_block):
+        self.assertEqual(expected_block.fields, actual_block.fields)
+        for field in expected_block.fields.values():
+            self.assertFieldEqual(field, expected_block, actual_block)
+
+    def assertFieldEqual(self, field, expected_block, actual_block):
+        if isinstance(field, (Reference, ReferenceList, ReferenceValueDict)):
+            self.assertReferenceRelativelyEqual(field, expected_block, actual_block)
+        else:
+            expected = field.read_from(expected_block)
+            actual = field.read_from(actual_block)
+            self.assertEqual(
+                expected,
+                actual,
+                LazyFormat(
+                    "Field {} doesn't match between usages {} and {}: {!r} != {!r}",
+                    field.name,
+                    expected_block.scope_ids.usage_id,
+                    actual_block.scope_ids.usage_id,
+                    expected,
+                    actual
+                )
+            )
+
     def assertCoursesEqual(self, expected_store, expected_course_key, actual_store, actual_course_key):
         """
         Assert that the courses identified by ``expected_course_key`` in ``expected_store`` and
@@ -312,11 +394,7 @@ class CourseComparisonTest(BulkAssertionTest):
                     actual_item = actual_item_map.get(map_key(actual_item_location))
 
                 # Formatting the message slows down tests of large courses significantly, so only do it if it would be used
-                if actual_item is None:
-                    msg = u'cannot find {} in {}'.format(map_key(actual_item_location), actual_item_map)
-                else:
-                    msg = None
-                self.assertIsNotNone(actual_item, msg)
+                self.assertIsNotNone(actual_item, LazyFormat(u'cannot find {} in {}', map_key(actual_item_location), actual_item_map))
 
                 # compare fields
                 self.assertEqual(expected_item.fields, actual_item.fields)
@@ -332,20 +410,7 @@ class CourseComparisonTest(BulkAssertionTest):
                     if field_name == 'children':
                         continue
 
-                    exp_value = map_references(field.read_from(expected_item), field, actual_course_key)
-                    actual_value = field.read_from(actual_item)
-                    # Formatting the message slows down tests of large courses significantly, so only do it if it would be used
-                    if exp_value != actual_value:
-                        msg = "Field {!r} doesn't match between usages {} and {}: {!r} != {!r}".format(
-                            field_name,
-                            expected_item.scope_ids.usage_id,
-                            actual_item.scope_ids.usage_id,
-                            exp_value,
-                            actual_value,
-                        )
-                    else:
-                        msg = None
-                    self.assertEqual(exp_value, actual_value, msg)
+                    self.assertFieldEqual(field, expected_item, actual_item)
 
                 # compare children
                 self.assertEqual(expected_item.has_children, actual_item.has_children)

common/lib/xmodule/xmodule/tests/test_bulk_assertions.py

+import ddt
+from xmodule.tests import BulkAssertionTest
+
+@ddt.ddt
+class TestBulkAssertionTestCase(BulkAssertionTest):
+
+    @ddt.data(
+        ('assertTrue', True),
+        ('assertFalse', False),
+        ('assertIs', 1, 1),
+        ('assertIsNot', 1, 2),
+        ('assertIsNone', None),
+        ('assertIsNotNone', 1),
+        ('assertIn', 1, (1, 2, 3)),
+        ('assertNotIn', 5, (1, 2, 3)),
+        ('assertIsInstance', 1, int),
+        ('assertNotIsInstance', '1', int),
+        ('assertRaises', KeyError, {}.__getitem__, '1'),
+    )
+    @ddt.unpack
+    def test_passing_asserts_passthrough(self, assertion, *args):
+        getattr(self, assertion)(*args)
+
+
+    @ddt.data(
+        ('assertTrue', False),
+        ('assertFalse', True),
+        ('assertIs', 1, 2),
+        ('assertIsNot', 1, 1),
+        ('assertIsNone', 1),
+        ('assertIsNotNone', None),
+        ('assertIn', 5, (1, 2, 3)),
+        ('assertNotIn', 1, (1, 2, 3)),
+        ('assertIsInstance', '1', int),
+        ('assertNotIsInstance', 1, int),
+        ('assertRaises', ValueError, lambda: None),
+    )
+    @ddt.unpack
+    def test_failing_asserts_passthrough(self, assertion, *args):
+        # Use super(BulkAssertionTest) to make sure we get un-adulturated assertions
+        with super(BulkAssertionTest, self).assertRaises(AssertionError):
+            getattr(self, assertion)(*args)
+
+    def test_no_bulk_assert_equals(self):
+        # Use super(BulkAssertionTest) to make sure we get un-adulturated assertions
+        with super(BulkAssertionTest, self).assertRaises(AssertionError):
+            self.assertEquals(1, 2)
+
+    @ddt.data(
+        'assertEqual', 'assertEquals'
+    )
+    def test_bulk_assert_equals(self, asserterFn):
+        asserter = getattr(self, asserterFn)
+        contextmanager = self.bulk_assertions()
+
+        contextmanager.__enter__()
+        super(BulkAssertionTest, self).assertIsNotNone(self._manager)
+        asserter(1, 2)
+        asserter(3, 4)
+
+        # Use super(BulkAssertionTest) to make sure we get un-adulturated assertions
+        with super(BulkAssertionTest, self).assertRaises(AssertionError):
+            contextmanager.__exit__(None, None, None)
+
+    @ddt.data(
+        'assertEqual', 'assertEquals'
+    )
+    def test_bulk_assert_closed(self, asserterFn):
+        asserter = getattr(self, asserterFn)
+
+        with self.bulk_assertions():
+            asserter(1, 1)
+            asserter(2, 2)
+
+        # Use super(BulkAssertionTest) to make sure we get un-adulturated assertions
+        with super(BulkAssertionTest, self).assertRaises(AssertionError):
+            asserter(1, 2)

common/test/acceptance/pages/studio/course_page.py

@@ -2,6 +2,8 @@
 Base class for pages specific to a course in Studio.
 """
 
+import os
+from opaque_keys.edx.locator import CourseLocator
 from bok_choy.page_object import PageObject
 from . import BASE_URL
 
@@ -34,5 +36,12 @@ class CoursePage(PageObject):
         """
         Construct a URL to the page within the course.
         """
-        course_key = "{course_org}/{course_num}/{course_run}".format(**self.course_info)
-        return "/".join([BASE_URL, self.url_path, course_key])
+        # TODO - is there a better way to make this agnostic to the underlying default module store?
+        default_store = os.environ.get('DEFAULT_STORE', 'draft')
+        course_key = CourseLocator(
+            self.course_info['course_org'],
+            self.course_info['course_num'],
+            self.course_info['course_run'],
+            deprecated=(default_store == 'draft')
+        )
+        return "/".join([BASE_URL, self.url_path, unicode(course_key)])

lms/djangoapps/courseware/module_render.py

@@ -115,35 +115,36 @@ def toc_for_course(user, request, course, active_chapter, active_section, field_
     field_data_cache must include data from the course module and 2 levels of its descendents
     '''
 
-    course_module = get_module_for_descriptor(user, request, course, field_data_cache, course.id)
-    if course_module is None:
-        return None
+    with modulestore().bulk_operations(course.id):
+        course_module = get_module_for_descriptor(user, request, course, field_data_cache, course.id)
+        if course_module is None:
+            return None
 
-    chapters = list()
-    for chapter in course_module.get_display_items():
-        if chapter.hide_from_toc:
-            continue
-
-        sections = list()
-        for section in chapter.get_display_items():
-
-            active = (chapter.url_name == active_chapter and
-                      section.url_name == active_section)
-
-            if not section.hide_from_toc:
-                sections.append({'display_name': section.display_name_with_default,
-                                 'url_name': section.url_name,
-                                 'format': section.format if section.format is not None else '',
-                                 'due': get_extended_due_date(section),
-                                 'active': active,
-                                 'graded': section.graded,
-                                 })
-
-        chapters.append({'display_name': chapter.display_name_with_default,
-                         'url_name': chapter.url_name,
-                         'sections': sections,
-                         'active': chapter.url_name == active_chapter})
-    return chapters
+        chapters = list()
+        for chapter in course_module.get_display_items():
+            if chapter.hide_from_toc:
+                continue
+
+            sections = list()
+            for section in chapter.get_display_items():
+
+                active = (chapter.url_name == active_chapter and
+                          section.url_name == active_section)
+
+                if not section.hide_from_toc:
+                    sections.append({'display_name': section.display_name_with_default,
+                                     'url_name': section.url_name,
+                                     'format': section.format if section.format is not None else '',
+                                     'due': get_extended_due_date(section),
+                                     'active': active,
+                                     'graded': section.graded,
+                                     })
+
+            chapters.append({'display_name': chapter.display_name_with_default,
+                             'url_name': chapter.url_name,
+                             'sections': sections,
+                             'active': chapter.url_name == active_chapter})
+        return chapters
 
 
 def get_module(user, request, usage_key, field_data_cache,

lms/djangoapps/courseware/tests/test_module_render.py

@@ -326,19 +326,29 @@ class TestTOC(ModuleStoreTestCase):
         self.request = factory.get(chapter_url)
         self.request.user = UserFactory()
         self.modulestore = self.store._get_modulestore_for_courseid(self.course_key)
-        with check_mongo_calls(num_finds, num_sends):
-            self.toy_course = self.store.get_course(self.toy_loc, depth=2)
-            self.field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
-                self.toy_loc, self.request.user, self.toy_course, depth=2
-            )
+        with self.modulestore.bulk_operations(self.course_key):
+            with check_mongo_calls(num_finds, num_sends):
+                self.toy_course = self.store.get_course(self.toy_loc, depth=2)
+                self.field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
+                    self.toy_loc, self.request.user, self.toy_course, depth=2
+                )
 
-    # TODO: LMS-11220: Document why split find count is 9
-    # TODO: LMS-11220: Document why mongo find count is 4
-    @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0), (ModuleStoreEnum.Type.split, 9, 0))
+    # Mongo makes 3 queries to load the course to depth 2:
+    #     - 1 for the course
+    #     - 1 for its children
+    #     - 1 for its grandchildren
+    # 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:
+    #     - it loads the active version at the start of the bulk operation
+    #     - it loads the course definition for inheritance, because it's outside
+    #     the bulk-operation marker that loaded the course descriptor
+    @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 6, 0, 2))
     @ddt.unpack
-    def test_toc_toy_from_chapter(self, default_ms, num_finds, num_sends):
+    def test_toc_toy_from_chapter(self, default_ms, setup_finds, setup_sends, toc_finds):
         with self.store.default_store(default_ms):
-            self.setup_modulestore(default_ms, num_finds, num_sends)
+            self.setup_modulestore(default_ms, setup_finds, setup_sends)
             expected = ([{'active': True, 'sections':
                           [{'url_name': 'Toy_Videos', 'display_name': u'Toy Videos', 'graded': True,
                             'format': u'Lecture Sequence', 'due': None, 'active': False},
@@ -354,20 +364,29 @@ class TestTOC(ModuleStoreTestCase):
                             'format': '', 'due': None, 'active': False}],
                           'url_name': 'secret:magic', 'display_name': 'secret:magic'}])
 
-            with check_mongo_calls(0, 0):
+            with check_mongo_calls(toc_finds, 0):
                 actual = render.toc_for_course(
                     self.request.user, self.request, self.toy_course, self.chapter, None, self.field_data_cache
                 )
         for toc_section in expected:
             self.assertIn(toc_section, actual)
 
-    # TODO: LMS-11220: Document why split find count is 9
-    # TODO: LMS-11220: Document why mongo find count is 4
-    @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0), (ModuleStoreEnum.Type.split, 9, 0))
+    # Mongo makes 3 queries to load the course to depth 2:
+    #     - 1 for the course
+    #     - 1 for its children
+    #     - 1 for its grandchildren
+    # 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:
+    #     - it loads the active version at the start of the bulk operation
+    #     - it loads the course definition for inheritance, because it's outside
+    #     the bulk-operation marker that loaded the course descriptor
+    @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 6, 0, 2))
     @ddt.unpack
-    def test_toc_toy_from_section(self, default_ms, num_finds, num_sends):
+    def test_toc_toy_from_section(self, default_ms, setup_finds, setup_sends, toc_finds):
         with self.store.default_store(default_ms):
-            self.setup_modulestore(default_ms, num_finds, num_sends)
+            self.setup_modulestore(default_ms, setup_finds, setup_sends)
             section = 'Welcome'
             expected = ([{'active': True, 'sections':
                           [{'url_name': 'Toy_Videos', 'display_name': u'Toy Videos', 'graded': True,
@@ -384,7 +403,8 @@ class TestTOC(ModuleStoreTestCase):
                             'format': '', 'due': None, 'active': False}],
                           'url_name': 'secret:magic', 'display_name': 'secret:magic'}])
 
-            actual = render.toc_for_course(self.request.user, self.request, self.toy_course, self.chapter, section, self.field_data_cache)
+            with check_mongo_calls(toc_finds, 0):
+                actual = render.toc_for_course(self.request.user, self.request, self.toy_course, self.chapter, section, self.field_data_cache)
             for toc_section in expected:
                 self.assertIn(toc_section, actual)
 

requirements/edx/base.txt

@@ -131,7 +131,7 @@ rednose==0.3
 selenium==2.42.1
 splinter==0.5.4
 testtools==0.9.34
-PyContracts==1.6.4
+PyContracts==1.6.5
 
 # Used for Segment.io analytics
 analytics-python==0.4.4

requirements/edx/github.txt

@@ -30,7 +30,7 @@
 -e git+https://github.com/edx-solutions/django-splash.git@7579d052afcf474ece1239153cffe1c89935bc4f#egg=django-splash
 -e git+https://github.com/edx/acid-block.git@459aff7b63db8f2c5decd1755706c1a64fb4ebb1#egg=acid-xblock
 -e git+https://github.com/edx/edx-ora2.git@release-2014-09-18T16.00#egg=edx-ora2
--e git+https://github.com/edx/opaque-keys.git@d45d0bd8d64c69531be69178b9505b5d38806ce0#egg=opaque-keys
+-e git+https://github.com/edx/opaque-keys.git@295d93170b2f6e57e3a2b9ba0a52087a4e8712c5#egg=opaque-keys
 -e git+https://github.com/edx/ease.git@97de68448e5495385ba043d3091f570a699d5b5f#egg=ease
 -e git+https://github.com/edx/i18n-tools.git@56f048af9b6868613c14aeae760548834c495011#egg=i18n-tools
 -e git+https://github.com/edx/edx-oauth2-provider.git@0.2.2#egg=oauth2-provider