reafactor: simplify XModule serialization/deserialization layer [BD-13]

cms/djangoapps/contentstore/course_info_model.py

@@ -24,7 +24,7 @@
 from xmodule.modulestore.django import modulestore  # lint-amnesty, pylint: disable=wrong-import-order
 from xmodule.modulestore.exceptions import ItemNotFoundError  # lint-amnesty, pylint: disable=wrong-import-order
 
-# # This should be in a class which inherits from XmlDescriptor
+# # This should be in a class which inherits from XModuleDescriptor
 log = logging.getLogger(__name__)
 
 

lms/djangoapps/courseware/tests/test_video_mongo.py

@@ -1839,9 +1839,10 @@ def test_import_val_data_internal(self):
             val_transcript_language_code=val_transcript_language_code,
             val_transcript_provider=val_transcript_provider
         )
+        xml_object = etree.fromstring(xml_data)
         id_generator = Mock()
         id_generator.target_course_id = "test_course_id"
-        video = self.descriptor.from_xml(xml_data, module_system, id_generator)
+        video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator)
 
         assert video.edx_video_id == 'test_edx_video_id'
         video_data = get_video_info(video.edx_video_id)
@@ -1887,13 +1888,14 @@ def test_import_no_video_id(self):
         Test that importing a video with no video id, creates a new external video.
         """
         xml_data = """<video><video_asset></video_asset></video>"""
+        xml_object = etree.fromstring(xml_data)
         module_system = DummySystem(load_error_modules=True)
         id_generator = Mock()
 
         # Verify edx_video_id is empty before.
         assert self.descriptor.edx_video_id == ''
 
-        video = self.descriptor.from_xml(xml_data, module_system, id_generator)
+        video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator)
 
         # Verify edx_video_id is populated after the import.
         assert video.edx_video_id != ''
@@ -1923,6 +1925,7 @@ def test_import_val_transcript(self):
             val_transcript_language_code=val_transcript_language_code,
             val_transcript_provider=val_transcript_provider
         )
+        xml_object = etree.fromstring(xml_data)
         module_system = DummySystem(load_error_modules=True)
         id_generator = Mock()
 
@@ -1940,7 +1943,7 @@ def test_import_val_transcript(self):
         # Verify edx_video_id is empty before.
         assert self.descriptor.edx_video_id == ''
 
-        video = self.descriptor.from_xml(xml_data, module_system, id_generator)
+        video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator)
 
         # Verify edx_video_id is populated after the import.
         assert video.edx_video_id != ''
@@ -2077,11 +2080,12 @@ def test_import_val_transcript_priority(self, sub_id, external_transcripts, val_
             xml_data += val_transcripts
 
         xml_data += '</video_asset></video>'
+        xml_object = etree.fromstring(xml_data)
 
         # Verify edx_video_id is empty before import.
         assert self.descriptor.edx_video_id == ''
 
-        video = self.descriptor.from_xml(xml_data, module_system, id_generator)
+        video = self.descriptor.parse_xml(xml_object, module_system, None, id_generator)
 
         # Verify edx_video_id is not empty after import.
         assert video.edx_video_id != ''
@@ -2107,8 +2111,9 @@ def test_import_val_data_invalid(self):
                 </video_asset>
             </video>
         """
+        xml_object = etree.fromstring(xml_data)
         with pytest.raises(ValCannotCreateError):
-            VideoBlock.from_xml(xml_data, module_system, id_generator=Mock())
+            VideoBlock.parse_xml(xml_object, module_system, None, id_generator=Mock())
         with pytest.raises(ValVideoNotFoundError):
             get_video_info("test_edx_video_id")
 

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

openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py

@@ -67,7 +67,7 @@ def get_block(self, usage_id, for_parent=None, block_type_overrides=None):  # py
                 # 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()

openedx/core/djangoapps/xblock/runtime/serializer.py

@@ -12,7 +12,7 @@
 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

openedx/core/djangoapps/xblock/runtime/shims.py

@@ -154,7 +154,7 @@ def render_template(self, template_name, dictionary, namespace='main'):
 
     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.

xmodule/course_module.py

@@ -6,7 +6,6 @@
 import json
 import logging
 from datetime import datetime, timedelta
-from io import BytesIO
 
 import dateutil.parser
 import requests
@@ -1128,18 +1127,11 @@ def read_grading_policy(cls, paths, system):
         return policy_str
 
     @classmethod
-    def from_xml(cls, xml_data, system, id_generator):
-        instance = super().from_xml(xml_data, system, id_generator)
-
-        # bleh, have to parse the XML here to just pull out the url_name attribute
-        # I don't think it's stored anywhere in the instance.
-        if isinstance(xml_data, str):
-            xml_data = xml_data.encode('ascii', 'ignore')
-        course_file = BytesIO(xml_data)
-        xml_obj = etree.parse(course_file, parser=edx_xml_parser).getroot()
+    def parse_xml(cls, node, runtime, keys, id_generator):
+        instance = super().parse_xml(node, runtime, keys, id_generator)
 
         policy_dir = None
-        url_name = xml_obj.get('url_name', xml_obj.get('slug'))
+        url_name = node.get('url_name')
         if url_name:
             policy_dir = 'policies/' + url_name
 
@@ -1149,9 +1141,9 @@ def from_xml(cls, xml_data, system, id_generator):
             paths = [policy_dir + '/grading_policy.json'] + paths
 
         try:
-            policy = json.loads(cls.read_grading_policy(paths, system))
+            policy = json.loads(cls.read_grading_policy(paths, runtime))
         except ValueError:
-            system.error_tracker("Unable to decode grading policy as json")
+            runtime.error_tracker("Unable to decode grading policy as json")
             policy = {}
 
         # now set the current instance. set_grading_policy() will apply some inheritance rules

xmodule/discussion_block.py

@@ -18,7 +18,7 @@
 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.
     """

xmodule/raw_module.py

@@ -4,7 +4,6 @@
 
 from lxml import etree
 from xblock.fields import Scope, String
-from xmodule.xml_module import XmlDescriptor  # pylint: disable=unused-import
 
 from .exceptions import SerializationError
 

xmodule/template_module.py

@@ -147,19 +147,18 @@ class TranslateCustomTagBlock(  # pylint: disable=abstract-method
     resources_dir = None
 
     @classmethod
-    def from_xml(cls, xml_data, system, id_generator):
+    def parse_xml(cls, node, runtime, _keys, _id_generator):
         """
         Transforms the xml_data from <$custom_tag attr="" attr=""/> to
         <customtag attr="" attr="" impl="$custom_tag"/>
         """
 
-        xml_object = etree.fromstring(xml_data)
-        system.error_tracker(Text('WARNING: the <{tag}> tag is deprecated.  '
-                             'Instead, use <customtag impl="{tag}" attr1="..." attr2="..."/>. ')
-                             .format(tag=xml_object.tag))
+        runtime.error_tracker(Text('WARNING: the <{tag}> tag is deprecated.  '
+                              'Instead, use <customtag impl="{tag}" attr1="..." attr2="..."/>. ')
+                              .format(tag=node.tag))
 
-        tag = xml_object.tag
-        xml_object.tag = 'customtag'
-        xml_object.attrib['impl'] = tag
+        tag = node.tag
+        node.tag = 'customtag'
+        node.attrib['impl'] = tag
 
-        return system.process_xml(etree.tostring(xml_object))
+        return runtime.process_xml(etree.tostring(node))

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 @@ def setUp(self):
         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 @@ def test_poll_export_with_unescaped_characters_xml(self):
             <answer id="less18">18</answer>
         </poll_question>
         '''
+        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.