From 8127d1911529d344c1f2a6d2e50353067dfc4821 Mon Sep 17 00:00:00 2001 From: Agrendalath Date: Tue, 15 Mar 2022 23:54:10 +0100 Subject: [PATCH] refactor: replace `XmlMixin` with `XmlParserMixin` Most of the methods in `XmlMixin` act as wrappers for the official API for serialization and deserialization (parse_xml() and add_xml_to_node()). `XmlParserMixin` contains the code which does the actual serialization and deserialization. --- .../core/djangoapps/olx_rest_api/adapters.py | 26 ++--- .../xblock/runtime/blockstore_runtime.py | 2 +- .../djangoapps/xblock/runtime/serializer.py | 10 +- .../core/djangoapps/xblock/runtime/shims.py | 2 +- xmodule/discussion_block.py | 4 +- xmodule/tests/test_poll.py | 11 +- xmodule/vertical_block.py | 4 +- xmodule/x_module.py | 2 +- xmodule/xml_module.py | 104 +----------------- 9 files changed, 34 insertions(+), 131 deletions(-) diff --git a/openedx/core/djangoapps/olx_rest_api/adapters.py b/openedx/core/djangoapps/olx_rest_api/adapters.py index a9943fd3ac..4db3cb03aa 100644 --- a/openedx/core/djangoapps/olx_rest_api/adapters.py +++ b/openedx/core/djangoapps/olx_rest_api/adapters.py @@ -1,22 +1,22 @@ """ Helpers required to adapt to differing APIs """ -from contextlib import contextmanager import logging import re +from contextlib import contextmanager -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import AssetKey, CourseKey from fs.memoryfs import MemoryFS from fs.wrapfs import WrapFS +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import AssetKey, CourseKey +from xmodule.assetstore.assetmgr import AssetManager +from xmodule.contentstore.content import StaticContent +from xmodule.exceptions import NotFoundError +from xmodule.modulestore.django import modulestore as store +from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.xml_module import XmlMixin from common.djangoapps.static_replace import replace_static_urls -from xmodule.contentstore.content import StaticContent # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.assetstore.assetmgr import AssetManager # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.django import modulestore as store # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.exceptions import NotFoundError # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.xml_module import XmlParserMixin # lint-amnesty, pylint: disable=wrong-import-order log = logging.getLogger(__name__) @@ -105,7 +105,7 @@ def override_export_fs(block): XmlSerializationMixin.add_xml_to_node() method. This method temporarily replaces a block's runtime's 'export_fs' system with an in-memory filesystem. - This method also abuses the XmlParserMixin.export_to_file() + This method also abuses the XmlMixin.export_to_file() API to prevent the XModule export code from exporting each block as two files (one .olx pointing to one .xml file). The export_to_file was meant to be used only by the @@ -120,10 +120,10 @@ def override_export_fs(block): if hasattr(block, 'export_to_file'): old_export_to_file = block.export_to_file block.export_to_file = lambda: False - old_global_export_to_file = XmlParserMixin.export_to_file - XmlParserMixin.export_to_file = lambda _: False # So this applies to child blocks that get loaded during export + old_global_export_to_file = XmlMixin.export_to_file + XmlMixin.export_to_file = lambda _: False # So this applies to child blocks that get loaded during export yield fs block.runtime.export_fs = old_export_fs if hasattr(block, 'export_to_file'): block.export_to_file = old_export_to_file - XmlParserMixin.export_to_file = old_global_export_to_file + XmlMixin.export_to_file = old_global_export_to_file diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py index f48ae7d0b8..25f3a78aa7 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py @@ -67,7 +67,7 @@ class BlockstoreXBlockRuntime(XBlockRuntime): # This is a (former) XModule with messy XML parsing code; let its parse_xml() method continue to work # as it currently does in the old runtime, but let this parse_xml_new_runtime() method parse the XML in # a simpler way that's free of tech debt, if defined. - # In particular, XmlParserMixin doesn't play well with this new runtime, so this is mostly about + # In particular, XmlMixin doesn't play well with this new runtime, so this is mostly about # bypassing that mixin's code. # When a former XModule no longer needs to support the old runtime, its parse_xml_new_runtime method # should be removed and its parse_xml() method should be simplified to just call the super().parse_xml() diff --git a/openedx/core/djangoapps/xblock/runtime/serializer.py b/openedx/core/djangoapps/xblock/runtime/serializer.py index 9694f6a6e5..3cf803b4db 100644 --- a/openedx/core/djangoapps/xblock/runtime/serializer.py +++ b/openedx/core/djangoapps/xblock/runtime/serializer.py @@ -12,7 +12,7 @@ from fs.wrapfs import WrapFS from lxml.etree import Element from lxml.etree import tostring as etree_tostring -from xmodule.xml_module import XmlParserMixin +from xmodule.xml_module import XmlMixin log = logging.getLogger(__name__) @@ -104,7 +104,7 @@ def override_export_fs(block): This method temporarily replaces a block's runtime's 'export_fs' system with an in-memory filesystem. This method also abuses the - XmlParserMixin.export_to_file() + XmlMixin.export_to_file() API to prevent the XModule export code from exporting each block as two files (one .olx pointing to one .xml file). The export_to_file was meant to be used only by the customtag XModule but it makes our lives here much @@ -119,8 +119,8 @@ def override_export_fs(block): if hasattr(block, 'export_to_file'): old_export_to_file = block.export_to_file block.export_to_file = lambda: False - old_global_export_to_file = XmlParserMixin.export_to_file - XmlParserMixin.export_to_file = lambda _: False # So this applies to child blocks that get loaded during export + old_global_export_to_file = XmlMixin.export_to_file + XmlMixin.export_to_file = lambda _: False # So this applies to child blocks that get loaded during export try: yield fs except: # lint-amnesty, pylint: disable=try-except-raise @@ -129,4 +129,4 @@ def override_export_fs(block): block.runtime.export_fs = old_export_fs if hasattr(block, 'export_to_file'): block.export_to_file = old_export_to_file - XmlParserMixin.export_to_file = old_global_export_to_file + XmlMixin.export_to_file = old_global_export_to_file diff --git a/openedx/core/djangoapps/xblock/runtime/shims.py b/openedx/core/djangoapps/xblock/runtime/shims.py index 5f9909c95d..e9b57f4058 100644 --- a/openedx/core/djangoapps/xblock/runtime/shims.py +++ b/openedx/core/djangoapps/xblock/runtime/shims.py @@ -154,7 +154,7 @@ class RuntimeShim: def process_xml(self, xml): """ - Code to handle parsing of child XML for old blocks that use XmlParserMixin. + Code to handle parsing of child XML for old blocks that use XmlMixin. """ # We can't parse XML in a vacuum - we need to know the parent block and/or the # OLX file that holds this XML in order to generate useful definition keys etc. diff --git a/xmodule/discussion_block.py b/xmodule/discussion_block.py index 3e0d4ed254..63106fa9f2 100644 --- a/xmodule/discussion_block.py +++ b/xmodule/discussion_block.py @@ -18,7 +18,7 @@ from xblockutils.studio_editable import StudioEditableXBlockMixin from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration, Provider from openedx.core.djangolib.markup import HTML, Text from openedx.core.lib.xblock_utils import get_css_dependencies, get_js_dependencies -from xmodule.xml_module import XmlParserMixin +from xmodule.xml_module import XmlMixin log = logging.getLogger(__name__) loader = ResourceLoader(__name__) # pylint: disable=invalid-name @@ -34,7 +34,7 @@ def _(text): @XBlock.needs('user') # pylint: disable=abstract-method @XBlock.needs('i18n') @XBlock.needs('mako') -class DiscussionXBlock(XBlock, StudioEditableXBlockMixin, XmlParserMixin): # lint-amnesty, pylint: disable=abstract-method +class DiscussionXBlock(XBlock, StudioEditableXBlockMixin, XmlMixin): # lint-amnesty, pylint: disable=abstract-method """ Provides a discussion forum that is inline with other content in the courseware. """ diff --git a/xmodule/tests/test_poll.py b/xmodule/tests/test_poll.py index ea0f6049db..a879de1214 100644 --- a/xmodule/tests/test_poll.py +++ b/xmodule/tests/test_poll.py @@ -2,14 +2,14 @@ import json import unittest - from unittest.mock import Mock from opaque_keys.edx.keys import CourseKey from xblock.field_data import DictFieldData from xblock.fields import ScopeIds -from xmodule.poll_module import PollBlock +from openedx.core.lib.safe_lxml import etree +from xmodule.poll_module import PollBlock from . import get_test_system from .test_import import DummySystem @@ -29,9 +29,9 @@ class PollBlockTest(unittest.TestCase): self.system = get_test_system(course_key) usage_key = course_key.make_usage_key(PollBlock.category, 'test_loc') # ScopeIds has 4 fields: user_id, block_type, def_id, usage_id - scope_ids = ScopeIds(1, PollBlock.category, usage_key, usage_key) + self.scope_ids = ScopeIds(1, PollBlock.category, usage_key, usage_key) self.xmodule = PollBlock( - self.system, DictFieldData(self.raw_field_data), scope_ids + self.system, DictFieldData(self.raw_field_data), self.scope_ids ) def ajax_request(self, dispatch, data): @@ -70,8 +70,9 @@ class PollBlockTest(unittest.TestCase): 18 ''' + node = etree.fromstring(sample_poll_xml) - output = PollBlock.from_xml(sample_poll_xml, module_system, id_generator) + output = PollBlock.parse_xml(node, module_system, self.scope_ids, id_generator) # Update the answer with invalid character. invalid_characters_poll_answer = output.answers[0] # Invalid less-than character. diff --git a/xmodule/vertical_block.py b/xmodule/vertical_block.py index 3efc1fdc15..cdd2fce32b 100644 --- a/xmodule/vertical_block.py +++ b/xmodule/vertical_block.py @@ -21,7 +21,7 @@ from xmodule.studio_editable import StudioEditableBlock from xmodule.util.misc import is_xblock_an_assignment from xmodule.util.xmodule_django import add_webpack_to_fragment from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW, XModuleFields -from xmodule.xml_module import XmlParserMixin +from xmodule.xml_module import XmlMixin log = logging.getLogger(__name__) @@ -55,7 +55,7 @@ class VerticalBlock( VerticalFields, XModuleFields, StudioEditableBlock, - XmlParserMixin, + XmlMixin, MakoTemplateBlockBase, XBlock ): diff --git a/xmodule/x_module.py b/xmodule/x_module.py index 0ae31ddd38..9563abd837 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -1169,7 +1169,7 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): raise NotImplementedError("edX Platform doesn't currently implement XBlock resource urls") def add_block_as_child_node(self, block, node): - child = etree.SubElement(node, "unknown") + child = etree.SubElement(node, block.category) child.set('url_name', block.url_name) block.add_xml_to_node(child) diff --git a/xmodule/xml_module.py b/xmodule/xml_module.py index 2f470dba36..c57308497b 100644 --- a/xmodule/xml_module.py +++ b/xmodule/xml_module.py @@ -7,7 +7,7 @@ import logging import os from lxml import etree -from lxml.etree import Element, ElementTree, XMLParser +from lxml.etree import ElementTree, XMLParser from xblock.core import XBlock, XML_NAMESPACES from xblock.fields import Dict, Scope, ScopeIds from xblock.runtime import KvsFieldData @@ -103,7 +103,7 @@ def deserialize_field(field, value): return value -class XmlParserMixin: +class XmlMixin: """ Class containing XML parsing functionality shared between XBlock and XModuleDescriptor. """ @@ -503,108 +503,10 @@ class XmlParserMixin: Return a list of all metadata fields that cannot be edited. """ non_editable_fields = super().non_editable_metadata_fields - non_editable_fields.append(XmlParserMixin.xml_attributes) + non_editable_fields.append(XmlMixin.xml_attributes) return non_editable_fields -class XmlMixin(XmlParserMixin): # lint-amnesty, pylint: disable=abstract-method - """ - Mixin class for standardized parsing of XModule xml. - """ - @classmethod - def from_xml(cls, xml_data, system, id_generator): - """ - Creates an instance of this descriptor from the supplied xml_data. - This may be overridden by subclasses. - - Args: - xml_data (str): A string of xml that will be translated into data and children - for this module - - system (:class:`.XMLParsingSystem): - - id_generator (:class:`xblock.runtime.IdGenerator`): Used to generate the - usage_ids and definition_ids when loading this xml - - """ - # Shim from from_xml to the parse_xml defined in XmlParserMixin. - # This only exists to satisfy subclasses that both: - # a) define from_xml themselves - # b) call super(..).from_xml(..) - return super().parse_xml( - etree.fromstring(xml_data), - system, - None, # This is ignored by XmlParserMixin - id_generator, - ) - - @classmethod - def parse_xml(cls, node, runtime, keys, id_generator): - """ - Interpret the parsed XML in `node`, creating an XModuleDescriptor. - """ - if cls.from_xml != XmlMixin.from_xml: - xml = etree.tostring(node).decode('utf-8') - block = cls.from_xml(xml, runtime, id_generator) - return block - else: - return super().parse_xml(node, runtime, keys, id_generator) - - @classmethod - def parse_xml_new_runtime(cls, node, runtime, keys): - """ - This XML lives within Blockstore and the new runtime doesn't need this - legacy XModule code. Use the "normal" XBlock parsing code. - """ - try: - return super().parse_xml_new_runtime(node, runtime, keys) - except AttributeError: - return super().parse_xml(node, runtime, keys, id_generator=None) - - def export_to_xml(self, resource_fs): # lint-amnesty, pylint: disable=unused-argument - """ - Returns an xml string representing this module, and all modules - underneath it. May also write required resources out to resource_fs. - - Assumes that modules have single parentage (that no module appears twice - in the same course), and that it is thus safe to nest modules as xml - children as appropriate. - - The returned XML should be able to be parsed back into an identical - XModuleDescriptor using the from_xml method with the same system, org, - and course - """ - # Shim from export_to_xml to the add_xml_to_node defined in XmlParserMixin. - # This only exists to satisfy subclasses that both: - # a) define export_to_xml themselves - # b) call super(..).export_to_xml(..) - node = Element(self.category) - super().add_xml_to_node(node) - return etree.tostring(node) - - def add_xml_to_node(self, node): - """ - Export this :class:`XModuleDescriptor` as XML, by setting attributes on the provided - `node`. - """ - if self.export_to_xml != XmlMixin.export_to_xml: # lint-amnesty, pylint: disable=comparison-with-callable - xml_string = self.export_to_xml(self.runtime.export_fs) - exported_node = etree.fromstring(xml_string) - node.tag = exported_node.tag - node.text = exported_node.text - node.tail = exported_node.tail - - for key, value in exported_node.items(): - if key == 'url_name' and value == 'course' and key in node.attrib: - # if url_name is set in ExportManager then do not override it here. - continue - node.set(key, value) - - node.extend(list(exported_node)) - else: - super().add_xml_to_node(node) - - @XBlock.needs("i18n") class XmlDescriptor(XmlMixin, XModuleMixin): # lint-amnesty, pylint: disable=abstract-method pass