Switch inheritance in split-mongo over to using InheritingFieldData.

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/xmodule/fields.py

@@ -116,6 +116,9 @@ class Timedelta(JSONField):
         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)

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/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,6 +13,7 @@ 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.inheritance import inheriting_field_data, InheritanceMixin
 from xmodule.modulestore.split_mongo import BlockKey
 
 log = logging.getLogger(__name__)
@@ -34,8 +36,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
 
@@ -59,15 +60,18 @@ 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 = {}
 
+    @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")
     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,
@@ -96,12 +100,15 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
                 branch=course_info.get('branch'),
             )
 
+        if course_entry_override:
+            structure_id = course_entry_override.get('_id')
+        else:
+            structure_id = self.course_entry.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,7 +141,7 @@ 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
@@ -150,13 +157,6 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         # 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(
@@ -182,13 +182,22 @@ 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'],
         )
+        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(

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

@@ -1582,7 +1582,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():

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

@@ -323,6 +323,7 @@ class CrossStoreXMLRoundtrip(CourseComparisonTest):
 
                         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

@@ -33,11 +33,11 @@ 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)
@@ -1047,7 +1047,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')
@@ -1082,7 +1082,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):

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 (
@@ -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/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
@@ -227,6 +229,26 @@ class BulkAssertionTest(unittest.TestCase):
     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.
@@ -256,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
@@ -313,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)
@@ -333,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)