fix: remove the need to patch the KVS during library import

xmodule/modulestore/inheritance.py

@@ -3,11 +3,13 @@
 """
 
 
+import warnings
 from django.utils import timezone
 from xblock.core import XBlockMixin
 from xblock.fields import Boolean, Dict, Float, Integer, List, Scope, String
 from xblock.runtime import KeyValueStore, KvsFieldData
 
+from xmodule.error_block import ErrorBlock
 from xmodule.fields import Date, Timedelta
 from xmodule.partitions.partitions import UserPartition
 
@@ -280,7 +282,10 @@ def compute_inherited_metadata(block):
     NOTE: This means that there is no such thing as lazy loading at the
     moment--this accesses all the children."""
     if block.has_children:
-        parent_metadata = block.xblock_kvs.inherited_settings.copy()
+        if isinstance(block.xblock_kvs, InheritanceKeyValueStore):
+            parent_metadata = block.xblock_kvs.inherited_settings.copy()
+        else:
+            parent_metadata = {}
         # add any of block's explicitly set fields to the inheriting list
         for field in InheritanceMixin.fields.values():  # lint-amnesty, pylint: disable=no-member
             if field.is_set_on(block):
@@ -301,10 +306,23 @@ def inherit_metadata(block, inherited_data):
     `inherited_data`: A dictionary mapping field names to the values that
         they should inherit
     """
-    try:
+    if isinstance(block, ErrorBlock):
+        return
+
+    block_type = block.scope_ids.block_type
+    if isinstance(block.xblock_kvs, InheritanceKeyValueStore):
+        # This XBlock's field_data is backed by InheritanceKeyValueStore, which supports pre-computed inherited fields
         block.xblock_kvs.inherited_settings = inherited_data
-    except AttributeError:  # the kvs doesn't have inherited_settings probably b/c it's an error block
-        pass
+    else:
+        # We cannot apply pre-computed field data to this XBlock during import, but inheritance should still work
+        # normally when it's used in Studio/LMS, which use a different runtime.
+        # Though if someone ever needs a hacky temporary fix here, it's possible here to force it with:
+        #     init_dict = {key: getattr(block, key) for key in block.fields.keys()}
+        #     block._field_data = InheritanceKeyValueStore(init_dict)
+        warnings.warn(
+            f'Cannot inherit metadata to {block_type} block with KVS {block.xblock_kvs}',
+            stacklevel=2,
+        )
 
 
 def own_metadata(block):
@@ -316,7 +334,17 @@ def own_metadata(block):
 
 
 class InheritingFieldData(KvsFieldData):
-    """A `FieldData` implementation that can inherit value from parents to children."""
+    """
+    A `FieldData` implementation that can inherit value from parents to children.
+
+    This wraps a KeyValueStore, and will work fine with any KVS implementation.
+    Sometimes this wraps a subclass of InheritanceKeyValueStore, but that's not
+    a requirement.
+
+    This class is the way that inheritance "normally" works in modulestore.
+    During XML import/export, however, a different mechanism is used:
+    InheritanceKeyValueStore.
+    """
 
     def __init__(self, inheritable_names, **kwargs):
         """
@@ -379,6 +407,15 @@ class InheritanceKeyValueStore(KeyValueStore):
     dict-based storage of fields and lookup of inherited values.
 
     Note: inherited_settings is a dict of key to json values (internal xblock field repr)
+
+    Using this KVS is an alternative to using InheritingFieldData(). That one works with any KVS, like
+    DictKeyValueStore, and doesn't require any special behavior. On the other hand, this InheritanceKeyValueStore only
+    does inheritance properly if you first use compute_inherited_metadata() to walk the tree of XBlocks and pre-compute
+    the inherited metadata for the whole tree, storing it in the inherited_settings field of each instance of this KVS.
+
+    🟥 Warning: Unlike the base class, this KVS makes the assumption that you're using a completely separate KVS
+       instance for every XBlock, so that we only have to look at the "field_name" part of the key. You cannot use this
+       as a drop-in replacement for DictKeyValueStore for this reason.
     """
     def __init__(self, initial_values=None, inherited_settings=None):
         super().__init__()

xmodule/modulestore/xml.py

@@ -7,13 +7,13 @@
 import logging
 import os
 import re
+import warnings
 import sys
 from collections import defaultdict
 from contextlib import contextmanager
 from importlib import import_module
 
 from fs.osfs import OSFS
-from lazy import lazy
 from lxml import etree
 from opaque_keys.edx.keys import CourseKey
 from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryLocator
@@ -37,7 +37,7 @@
 )
 
 from .exceptions import ItemNotFoundError
-from .inheritance import InheritanceKeyValueStore, compute_inherited_metadata, inheriting_field_data
+from .inheritance import compute_inherited_metadata, inheriting_field_data
 
 edx_xml_parser = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True)
 
@@ -239,6 +239,24 @@ def load_item(usage_key, for_parent=None):
             **kwargs
         )
 
+    # pylint: disable=keyword-arg-before-vararg
+    def construct_xblock_from_class(self, cls, scope_ids, field_data=None, *args, **kwargs):
+        """
+        Construct a new xblock of type cls, mixing in the mixins
+        defined for this application.
+        """
+        if field_data:
+            # Currently, *some* XBlocks (those with XmlMixin) use XmlMixin.parse_xml() which instantiates
+            # its own key value store for field data. That is something which should be left to the runtime, for
+            # consistent behavior across all XBlocks, not controlled by individual XBlock implementations.
+            # We cannot just ignore field_data here though, because parse_xml may have pre-loaded data into it that we
+            # would otherwise lose.
+            warnings.warn(
+                'XBlocks should not instantiate their own field_data store during parse_xml()',
+                DeprecationWarning, stacklevel=2,
+            )
+        return super().construct_xblock_from_class(cls, scope_ids, field_data, *args, **kwargs)
+
     # id_generator is ignored, because each ImportSystem is already local to
     # a course, and has it's own id_generator already in place
     def add_node_as_child(self, block, node, id_generator):  # lint-amnesty, pylint: disable=signature-differs
@@ -899,26 +917,10 @@ def get_id(org, library, url_name):  # lint-amnesty, pylint: disable=arguments-d
         """
         return LibraryLocator(org=org, library=library)
 
-    @staticmethod
-    def patch_block_kvs(library_block):
-        """
-        Metadata inheritance can be done purely through XBlocks, but in the import phase
-        a root block with an InheritanceKeyValueStore is assumed to be at the top of the hierarchy.
-        This should change in the future, but as XBlocks don't have this KVS, we have to patch it
-        here manually.
-        """
-        init_dict = {key: getattr(library_block, key) for key in library_block.fields.keys()}
-        # if set, invalidate '_unwrapped_field_data' so it will be reset
-        # the next time it will be called
-        lazy.invalidate(library_block, '_unwrapped_field_data')
-        # pylint: disable=protected-access
-        library_block._field_data = inheriting_field_data(InheritanceKeyValueStore(init_dict))
-
     def content_importers(self, system, course_block, course_dir, url_name):
         """
         Handle Metadata inheritance for Libraries.
         """
-        self.patch_block_kvs(course_block)
         compute_inherited_metadata(course_block)
 
     def get_library(self, library_id, depth=0, **kwargs):  # pylint: disable=unused-argument

xmodule/tests/test_import.py

@@ -276,12 +276,6 @@ def test_library_metadata_import_export(self):
         )
         block = system.process_xml(start_xml)
 
-        # pylint: disable=protected-access
-        original_unwrapped = block._unwrapped_field_data
-        LibraryXMLModuleStore.patch_block_kvs(block)
-        # '_unwrapped_field_data' is reset in `patch_block_kvs`
-        # pylint: disable=protected-access
-        assert original_unwrapped is not block._unwrapped_field_data
         compute_inherited_metadata(block)
         # Check the course block, since it has inheritance
         block = block.get_children()[0]
@@ -322,7 +316,6 @@ def test_library_metadata_no_inheritance(self):
             </course>
         </library>'''.format(org=ORG, course=COURSE, url_name=url_name)
         block = system.process_xml(start_xml)
-        LibraryXMLModuleStore.patch_block_kvs(block)
         compute_inherited_metadata(block)
         # Run the checks on the course node instead.
         block = block.get_children()[0]
@@ -390,7 +383,6 @@ def test_library_metadata_override_default(self):
             </course>
         </library>'''.format(due=course_due, org=ORG, course=COURSE, url_name=url_name)
         block = system.process_xml(start_xml)
-        LibraryXMLModuleStore.patch_block_kvs(block)
         # Chapter is two levels down here.
         child = block.get_children()[0].get_children()[0]
         # pylint: disable=protected-access

xmodule/xml_block.py

@@ -360,6 +360,9 @@ def parse_xml(cls, node, runtime, _keys, id_generator):
         field_data['children'] = children
 
         field_data['xml_attributes']['filename'] = definition.get('filename', ['', None])  # for git link
+        # TODO: we shouldn't be instantiating our own field data instance here, but rather just call to
+        # runtime.construct_xblock_from_class() and then set fields on the returned block.
+        # See the base XBlock class (XmlSerializationMixin.parse_xml) for how it should be done.
         kvs = InheritanceKeyValueStore(initial_values=field_data)
         field_data = KvsFieldData(kvs)