diff --git a/xmodule/modulestore/tests/test_xml.py b/xmodule/modulestore/tests/test_xml.py index 5627ea451b2f..206c2ebc9ef6 100644 --- a/xmodule/modulestore/tests/test_xml.py +++ b/xmodule/modulestore/tests/test_xml.py @@ -159,3 +159,71 @@ def test_tilde_files_ignored(self, _fake_glob): # noqa: PT019 about_block = modulestore.get_item(about_location) assert 'GREEN' in about_block.data assert 'RED' not in about_block.data + + +class TestLoadPointerNodeHelper(TestCase): + """ + Unit tests for ``XMLParsingModuleStoreRuntime._load_pointer_node``. + + Regression for bug #36390: external XBlocks (which do not inherit + ``XmlMixin``) must be able to import pointer-tag OLX like built-in + blocks. The runtime-level helper is the new chokepoint. + """ + + def _make_runtime_with_fs(self, fs): + """Instantiate just the helper without building a full modulestore.""" + from xmodule.modulestore.xml import XMLParsingModuleStoreRuntime + runtime = XMLParsingModuleStoreRuntime.__new__(XMLParsingModuleStoreRuntime) + runtime.resources_fs = fs + return runtime + + def test_unit_load_pointer_node_reads_referenced_file(self): + """Pointer node resolves by reading ``/.xml``.""" + from fs.memoryfs import MemoryFS + from lxml import etree as _etree + + fs = MemoryFS() + fs.makedirs("ext_block") + with fs.open("ext_block/hello.xml", "w") as fp: + fp.write('PAYLOAD') + + runtime = self._make_runtime_with_fs(fs) + pointer = _etree.fromstring('') + resolved = runtime._load_pointer_node(pointer) # pylint: disable=protected-access + assert resolved.tag == "ext_block" + assert (resolved.text or "").strip() == "PAYLOAD" + assert resolved.get("url_name") == "hello" + + def test_unit_load_pointer_node_preserves_url_name_if_missing(self): + """When the inner file omits url_name, we copy it from the pointer.""" + from fs.memoryfs import MemoryFS + from lxml import etree as _etree + + fs = MemoryFS() + fs.makedirs("ext_block") + with fs.open("ext_block/foo.xml", "w") as fp: + fp.write('BODY') + + runtime = self._make_runtime_with_fs(fs) + pointer = _etree.fromstring('') + resolved = runtime._load_pointer_node(pointer) # pylint: disable=protected-access + assert resolved.get("url_name") == "foo" + + def test_bug_36390_regression_load_pointer_node_honors_url_name(self): + """ + Regression for bug #36390: an inner file that already declares + ``url_name`` must not be clobbered by the pointer's url_name. + """ + from fs.memoryfs import MemoryFS + from lxml import etree as _etree + + fs = MemoryFS() + fs.makedirs("ext_block") + with fs.open("ext_block/bar.xml", "w") as fp: + fp.write('BODY') + + runtime = self._make_runtime_with_fs(fs) + pointer = _etree.fromstring('') + resolved = runtime._load_pointer_node(pointer) # pylint: disable=protected-access + # The inner file's url_name wins (it was explicitly set). + assert resolved.get("url_name") == "inner-name" diff --git a/xmodule/modulestore/xml.py b/xmodule/modulestore/xml.py index ef73c6bb0171..486ee74e464e 100644 --- a/xmodule/modulestore/xml.py +++ b/xmodule/modulestore/xml.py @@ -92,6 +92,15 @@ def xblock_from_node(self, node, parent_id, id_generator=None): keys = ScopeIds(None, block_type, def_id, usage_id) block_class = self.mixologist.mix(self.load_block_type(block_type)) + # Resolve pointer-tag syntax at the runtime level so external XBlocks + # (which do not inherit XmlMixin) can be loaded from a separate file + # just like built-in blocks. Classes that already inherit XmlMixin + # handle pointer tags in their own parse_xml, so we skip to avoid a + # double file read. See bug #36390. + from xmodule.xml_block import XmlMixin, is_pointer_tag # noqa: PLC0415 + if is_pointer_tag(node) and not issubclass(block_class, XmlMixin): + node = self._load_pointer_node(node) + aside_children = self.parse_asides(node, def_id, usage_id, id_generator) asides_tags = [x.tag for x in aside_children] @@ -107,6 +116,27 @@ def xblock_from_node(self, node, parent_id, id_generator=None): return block + def _load_pointer_node(self, node): + """ + Resolve a pointer-tag XML node (e.g. ````) + by loading the referenced file from ``resources_fs``. Returns the + resolved ``lxml.etree`` element. + + Used so external XBlocks — which do not inherit ``XmlMixin`` — can be + imported from pointer-tag OLX at the runtime level. See bug #36390. + """ + from lxml import etree as _etree # noqa: PLC0415 + from xmodule.xml_block import EDX_XML_PARSER, name_to_pathname # noqa: PLC0415 + + url_name = node.get("url_name") + filepath = f"{node.tag}/{name_to_pathname(url_name)}.xml" + with self.resources_fs.open(filepath) as xml_file: + resolved = _etree.parse(xml_file, parser=EDX_XML_PARSER).getroot() + # Preserve the url_name from the pointer tag — inner files may omit it. + if "url_name" not in resolved.attrib: + resolved.set("url_name", url_name) + return resolved + def parse_asides(self, node, def_id, usage_id, id_generator): """pull the asides out of the xml payload and instantiate them""" aside_children = [] diff --git a/xmodule/x_module.py b/xmodule/x_module.py index f1d574155e80..2c6377e66ea3 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -1550,10 +1550,33 @@ def resource_url(self, resource): raise NotImplementedError("edX Platform doesn't currently implement XBlock resource urls") def add_block_as_child_node(self, block, node): - """Append the block’s XML to the given parent XML node.""" + """Append the block's XML to the given parent XML node. + + For blocks that do not inherit ``XmlMixin`` (i.e. external XBlocks), + also write the block's definition to a separate file and leave a + pointer tag in the parent, matching the export format used by + built-in blocks. See bug #36390. + """ + from xmodule.xml_block import XmlMixin, name_to_pathname # noqa: PLC0415 child = etree.SubElement(node, block.category) child.set("url_name", block.url_name) - block.add_xml_to_node(child) + if not isinstance(block, XmlMixin) and hasattr(self, "export_fs"): + # External block: dispatch into a scratch element, then serialize + # that element into its own file, leaving only a pointer tag here. + scratch = etree.Element(block.category) + block.add_xml_to_node(scratch) + url_path = name_to_pathname(block.url_name) + filepath = f"{block.category}/{url_path}.xml" + dirname = os.path.dirname(filepath) + if dirname: + self.export_fs.makedirs(dirname, recreate=True) + with self.export_fs.open(filepath, "wb") as fileobj: + etree.ElementTree(scratch).write( + fileobj, pretty_print=True, encoding="utf-8", + ) + # Leave the child node as a pure pointer; url_name is already set. + else: + block.add_xml_to_node(child) def publish(self, block, event_type, event_data): """