Skip to content

fix: do not remove comments from XML during course import #28557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nedbat merged 1 commit into from
Nov 16, 2021
Merged

fix: do not remove comments from XML during course import #28557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion cms/djangoapps/contentstore/views/tests/test_import_export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1256,7 +1256,25 @@ def assert_problem_definition(self, course_location, expected_problem_content):

'<problem>\n <pre>\n <code>x=10 print("hello \n")</code>\n </pre>\n '
'<multiplechoiceresponse/>\n</problem>\n'
]
],
[
'<!-- Comment outside of the root (will be deleted). -->'
'<problem>'
'<!-- Valid comment -->'
'<p>'
'"<!-- String with non-XML structure: >< -->"'
'Text'
'</p>'
'</problem>',

'<problem>\n '
'<!-- Valid comment -->\n '
'<p>'
'"<!-- String with non-XML structure: >< -->"'
'Text'
'</p>\n'
'</problem>\n'
],
)
@ddt.unpack
def test_problem_content_on_course_export_import(self, problem_data, expected_problem_content):
Expand Down
19 changes: 15 additions & 4 deletions common/lib/xmodule/xmodule/library_content_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import bleach
from lazy import lazy
from lxml import etree
from lxml.etree import XMLSyntaxError
from opaque_keys.edx.locator import LibraryLocator
from pkg_resources import resource_string
from web_fragments.fragment import Fragment
Expand Down Expand Up @@ -666,10 +667,20 @@ def get_content_titles(self):

@classmethod
def definition_from_xml(cls, xml_object, system):
children = [
system.process_xml(etree.tostring(child)).scope_ids.usage_id
for child in xml_object.getchildren()
]
children = []

for child in xml_object.getchildren():
try:
children.append(system.process_xml(etree.tostring(child)).scope_ids.usage_id)
except (XMLSyntaxError, AttributeError):
msg = (
"Unable to load child when parsing Library Content Block. "
"This can happen when a comment is manually added to the course export."
)
logger.error(msg)
Copy link
Contributor

@nedbat nedbat Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't an error, it would be better to do what XBlock does: explicitly check for a comment node and skip it. But this will rarely happen, so let's move ahead.

if system.error_tracker is not None:
system.error_tracker(msg)

definition = {
attr_name: json.loads(attr_value)
for attr_name, attr_value in xml_object.attrib.items()
Expand Down
3 changes: 1 addition & 2 deletions common/lib/xmodule/xmodule/modulestore/xml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@
from .exceptions import ItemNotFoundError
from .inheritance import InheritanceKeyValueStore, compute_inherited_metadata, inheriting_field_data

edx_xml_parser = etree.XMLParser(dtd_validation=False, load_dtd=False,
remove_comments=True, remove_blank_text=True)
edx_xml_parser = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True)

etree.set_default_parser(edx_xml_parser)

Expand Down
94 changes: 62 additions & 32 deletions common/lib/xmodule/xmodule/tests/test_library_content.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,18 +74,14 @@ class TestLibraryContentExportImport(LibraryContentTest):
"""
Export and import tests for LibraryContentBlock
"""
def setUp(self):
super().setUp()
Copy link
Contributor

@nedbat nedbat Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to define setUp if it only calls super().setUp(). The parent's setUp will be inherited.

Copy link
Member Author

@Agrendalath Agrendalath Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nedbat, it looks a bit confusing in the diff, but both super().setUp() and self.lc_block.refresh_children() have the same indentation level.


maxDiff = None

def test_xml_export_import_cycle(self):
"""
Test the export-import cycle.
"""
# Children will only set after calling this.
self.lc_block.refresh_children()
lc_block = self.store.get_item(self.lc_block.location)
self.lc_block = self.store.get_item(self.lc_block.location)

expected_olx = (
self.expected_olx = (
'<library_content display_name="{block.display_name}" max_count="{block.max_count}"'
' source_library_id="{block.source_library_id}" source_library_version="{block.source_library_version}">\n'
' <html url_name="{block.children[0].block_id}"/>\n'
Expand All @@ -94,44 +90,78 @@ def test_xml_export_import_cycle(self):
' <html url_name="{block.children[3].block_id}"/>\n'
'</library_content>\n'
).format(
block=lc_block,
block=self.lc_block,
)

export_fs = MemoryFS()
# Set the virtual FS to export the olx to.
lc_block.runtime._descriptor_system.export_fs = export_fs # pylint: disable=protected-access
self.export_fs = MemoryFS()
self.lc_block.runtime._descriptor_system.export_fs = self.export_fs # pylint: disable=protected-access

# Prepare runtime for the import.
self.runtime = TestImportSystem(load_error_modules=True, course_id=self.lc_block.location.course_key)
self.runtime.resources_fs = self.export_fs
self.id_generator = Mock()

# Export the olx.
node = etree.Element("unknown_root")
lc_block.add_xml_to_node(node)
self.lc_block.add_xml_to_node(node)

def _verify_xblock_properties(self, imported_lc_block):
"""
Check the new XBlock has the same properties as the old one.
"""
assert imported_lc_block.display_name == self.lc_block.display_name
assert imported_lc_block.source_library_id == self.lc_block.source_library_id
assert imported_lc_block.source_library_version == self.lc_block.source_library_version
assert imported_lc_block.mode == self.lc_block.mode
assert imported_lc_block.max_count == self.lc_block.max_count
assert imported_lc_block.capa_type == self.lc_block.capa_type
assert len(imported_lc_block.children) == len(self.lc_block.children)
assert imported_lc_block.children == self.lc_block.children

# Read it back
with export_fs.open('{dir}/{file_name}.xml'.format(
dir=lc_block.scope_ids.usage_id.block_type,
file_name=lc_block.scope_ids.usage_id.block_id
def test_xml_export_import_cycle(self):
"""
Test the export-import cycle.
"""
# Read back the olx.
with self.export_fs.open('{dir}/{file_name}.xml'.format(
dir=self.lc_block.scope_ids.usage_id.block_type,
file_name=self.lc_block.scope_ids.usage_id.block_id
)) as f:
exported_olx = f.read()

# And compare.
assert exported_olx == expected_olx

runtime = TestImportSystem(load_error_modules=True, course_id=lc_block.location.course_key)
runtime.resources_fs = export_fs
assert exported_olx == self.expected_olx

# Now import it.
olx_element = etree.fromstring(exported_olx)
id_generator = Mock()
imported_lc_block = LibraryContentBlock.parse_xml(olx_element, runtime, None, id_generator)

# Check the new XBlock has the same properties as the old one.
assert imported_lc_block.display_name == lc_block.display_name
assert imported_lc_block.source_library_id == lc_block.source_library_id
assert imported_lc_block.source_library_version == lc_block.source_library_version
assert imported_lc_block.mode == lc_block.mode
assert imported_lc_block.max_count == lc_block.max_count
assert imported_lc_block.capa_type == lc_block.capa_type
assert len(imported_lc_block.children) == 4
assert imported_lc_block.children == lc_block.children
imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None, self.id_generator)

self._verify_xblock_properties(imported_lc_block)

def test_xml_import_with_comments(self):
"""
Test that XML comments within LibraryContentBlock are ignored during the import.
"""
olx_with_comments = (
'<!-- Comment -->\n'
'<library_content display_name="{block.display_name}" max_count="{block.max_count}"'
' source_library_id="{block.source_library_id}" source_library_version="{block.source_library_version}">\n'
'<!-- Comment -->\n'
' <html url_name="{block.children[0].block_id}"/>\n'
' <html url_name="{block.children[1].block_id}"/>\n'
' <html url_name="{block.children[2].block_id}"/>\n'
' <html url_name="{block.children[3].block_id}"/>\n'
'</library_content>\n'
).format(
block=self.lc_block,
)

# Import the olx.
olx_element = etree.fromstring(olx_with_comments)
imported_lc_block = LibraryContentBlock.parse_xml(olx_element, self.runtime, None, self.id_generator)

self._verify_xblock_properties(imported_lc_block)


class LibraryContentBlockTestMixin:
Expand Down
4 changes: 1 addition & 3 deletions common/lib/xmodule/xmodule/xml_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@
log = logging.getLogger(__name__)

# assume all XML files are persisted as utf-8.
EDX_XML_PARSER = XMLParser(dtd_validation=False, load_dtd=False,
remove_comments=True, remove_blank_text=True,
encoding='utf-8')
EDX_XML_PARSER = XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True, encoding='utf-8')


def name_to_pathname(name):
Expand Down