From d0c353609db6070aaf6c42f9d0b4aa1468e6c103 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 1 Feb 2019 18:19:28 -0500 Subject: [PATCH] Don't break exports for uninstalled XBlock content When an unknown content type is encountered, it's imported as a RawDescriptor, which will preserve the OLX and export it back out. But if we import a course while an XBlock is installed and then export it after that XBlock is removed, we export RawDescriptors that never got to save the original OLX and have a blank "data" field. Attempting to export this used to fail and break export altogether. We now test that the export continues to complete, and just skips over anything it can't serialize out. Note that this will stil export pointers in the export, so if you uninstalled a "AmazingBlock" and exported, you might see something like the following in a vertical's XML:: However there would be no corresponding file at: /amazing/2edebb68d5734395a06b8a62b9bb677e.xml In fact, there would be no /amazing directory at all in the export. The better long term solution is probably to leave the pointer as-is and export some generic file that can't be mistaken for OLX (say a JSON file) that represents the raw key-value data we have in Modulstore for the now unknown XBlock type. However, this commit at least keeps export from crashing out entirely. --- .../views/tests/test_import_export.py | 98 ++++++++++++++++--- common/lib/xmodule/xmodule/raw_module.py | 19 ++++ common/lib/xmodule/xmodule/xml_module.py | 6 ++ 3 files changed, 110 insertions(+), 13 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index 4ba3a0ab77..e2162d7840 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/views/tests/test_import_export.py @@ -5,7 +5,9 @@ import copy import json import logging import os +import re import shutil +import StringIO import tarfile import tempfile from uuid import uuid4 @@ -554,6 +556,8 @@ class ExportTestCase(CourseTestCase): def test_export_async(self): """ Get tar.gz file, using asynchronous background task + + Return a TarFile of the successful export. """ resp = self.client.post(self.url) self.assertEquals(resp.status_code, 200) @@ -566,30 +570,98 @@ class ExportTestCase(CourseTestCase): resp = self.client.get(output_url) self._verify_export_succeeded(resp) + buff = StringIO.StringIO(b"".join(resp.streaming_content)) + return tarfile.open(fileobj=buff) + def _verify_export_succeeded(self, resp): """ Export success helper method. """ self.assertEquals(resp.status_code, 200) self.assertTrue(resp.get('Content-Disposition').startswith('attachment')) - def test_export_failure_top_level(self): + def test_unknown_xblock_top_level(self): """ - Export failure. + Export unknown XBlock type (i.e. we uninstalled the XBlock), top level. """ - fake_xblock = ItemFactory.create(parent_location=self.course.location, category='aawefawef') + fake_xblock = ItemFactory.create( + parent_location=self.course.location, + category='not_a_real_block_type' + ) self.store.publish(fake_xblock.location, self.user.id) - self._verify_export_failure(u'/container/{}'.format(self.course.location)) - def test_export_failure_subsection_level(self): - """ - Slightly different export failure. - """ - vertical = ItemFactory.create(parent_location=self.course.location, category='vertical', display_name='foo') - ItemFactory.create( - parent_location=vertical.location, - category='aawefawef' + # Now check the resulting export + tar_ball = self.test_export_async() + course_file_path = next( + path for path in tar_ball.getnames() + if re.match(r'\w+/course/\w+.xml', path) + ) + course_file = tar_ball.extractfile(course_file_path) + course_xml = lxml.etree.parse(course_file) + course_elem = course_xml.getroot() + + # The course run file still has a child pointer to the unknown type and + # creates the pointer tag... + self.assertEqual(course_elem.tag, 'course') + unknown_elem = course_elem[0] + self.assertEqual(unknown_elem.tag, 'not_a_real_block_type') + # Non empty url_name attribute (the generated ID) + self.assertTrue(unknown_elem.attrib['url_name']) + + # But there should be no file exported for our fake block type. Without + # the XBlock installed, we don't know how to serialize it properly. + assert not any( + '/not_a_real_block_type/' in path + for path in tar_ball.getnames() ) - self._verify_export_failure(u'/container/{}'.format(vertical.location)) + def test_unknown_xblock_subsection_level(self): + """ + Export unknown XBlock type deeper in the course. + """ + vertical = ItemFactory.create( + parent_location=self.course.location, + category='vertical', + display_name='sample_vertical', + ) + fake_xblock = ItemFactory.create( + parent_location=vertical.location, + category='not_a_real_block_type', + ) + self.store.publish(fake_xblock.location, self.user.id) + + # Now check the resulting export + tar_ball = self.test_export_async() + course_file_path = next( + path for path in tar_ball.getnames() + if re.match(r'\w+/course/\w+.xml', path) + ) + course_file = tar_ball.extractfile(course_file_path) + course_xml = lxml.etree.parse(course_file) + course_elem = course_xml.getroot() + + # The course run file should have a vertical that points to the + # non-existant block. + self.assertEqual(course_elem.tag, 'course') + self.assertEqual(course_elem[0].tag, 'vertical') # This is just a reference + + vert_file_path = next( + path for path in tar_ball.getnames() + if re.match(r'\w+/vertical/\w+.xml', path) + ) + vert_file = tar_ball.extractfile(vert_file_path) + vert_xml = lxml.etree.parse(vert_file) + vert_elem = vert_xml.getroot() + self.assertEqual(vert_elem.tag, 'vertical') + self.assertEqual(len(vert_elem), 1) + unknown_elem = vert_elem[0] + self.assertEqual(unknown_elem.tag, 'not_a_real_block_type') + # Non empty url_name attribute (the generated ID) + self.assertTrue(unknown_elem.attrib['url_name']) + + # There should be no file exported for our fake block type + assert not any( + '/not_a_real_block_type/' in path + for path in tar_ball.getnames() + ) def _verify_export_failure(self, expected_text): """ Export failure helper method. """ diff --git a/common/lib/xmodule/xmodule/raw_module.py b/common/lib/xmodule/xmodule/raw_module.py index b0e38b63f1..48bb05c524 100644 --- a/common/lib/xmodule/xmodule/raw_module.py +++ b/common/lib/xmodule/xmodule/raw_module.py @@ -24,6 +24,25 @@ class RawDescriptor(XmlDescriptor, XMLEditingDescriptor): return {'data': etree.tostring(xml_object, pretty_print=True, encoding='unicode')}, [] def definition_to_xml(self, resource_fs): + """ + Return an Element if we've kept the import OLX, or None otherwise. + """ + # If there's no self.data, it means that an XBlock/XModule originally + # existed for this data at the time of import/editing, but was later + # uninstalled. RawDescriptor therefore never got to preserve the + # original OLX that came in, and we have no idea how it should be + # serialized for export. It's possible that we could do some smarter + # fallback here and attempt to extract the data, but it's reasonable + # and simpler to just skip this node altogether. + if not self.data: + log.warning( + "Could not serialize %s: No XBlock installed for '%s' tag.", + self.location, + self.location.block_type, + ) + return None + + # Normal case: Just echo back the original OLX we saved. try: return etree.fromstring(self.data) except etree.XMLSyntaxError as err: diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 6035ca4ef4..faf33b4807 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -429,6 +429,12 @@ class XmlParserMixin(object): """ # Get the definition xml_object = self.definition_to_xml(self.runtime.export_fs) + + # If xml_object is None, we don't know how to serialize this node, but + # we shouldn't crash out the whole export for it. + if xml_object is None: + return + for aside in self.runtime.get_asides(self): if aside.needs_serialization(): aside_node = etree.Element("unknown_root", nsmap=XML_NAMESPACES)