diff --git a/xmodule/modulestore/inheritance.py b/xmodule/modulestore/inheritance.py index 5e24c39031d0..489365e80e0d 100644 --- a/xmodule/modulestore/inheritance.py +++ b/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__() diff --git a/xmodule/modulestore/xml.py b/xmodule/modulestore/xml.py index 38e3c22189e5..c4a4d66cbf64 100644 --- a/xmodule/modulestore/xml.py +++ b/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 diff --git a/xmodule/tests/test_import.py b/xmodule/tests/test_import.py index 3fb444a1c1ba..95feedeb9c76 100644 --- a/xmodule/tests/test_import.py +++ b/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): '''.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): '''.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 diff --git a/xmodule/xml_block.py b/xmodule/xml_block.py index cb7c096ce942..45b7589357c4 100644 --- a/xmodule/xml_block.py +++ b/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)