From 709326acbcfed26f1795fbbbd184eff50bb09299 Mon Sep 17 00:00:00 2001 From: Muhammad Tayyab Tahir Qureshi Date: Mon, 24 Mar 2025 15:11:03 +0500 Subject: [PATCH 1/8] feat: add pointer tags parsing implementation in `XBlock`'s `parse_xml` method --- xblock/core.py | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/xblock/core.py b/xblock/core.py index 7ff362475..a52d8e4fe 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -32,12 +32,85 @@ ("block", "http://code.edx.org/xblock/block"), ]) +XML_PARSER = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True, encoding='utf-8') + # __all__ controls what classes end up in the docs. __all__ = ['XBlock', 'XBlockAside'] UNSET = object() +def name_to_pathname(name): + """ + Convert a location name for use in a path: replace ':' with '/'. + This allows users of the xml format to organize content into directories + """ + return name.replace(':', '/') + + +def is_pointer_tag(xml_obj): + """ + Check if xml_obj is a pointer tag: . + No children, one attribute named url_name, no text. + + Special case for course roots: the pointer is + + + xml_obj: an etree Element + + Returns a bool. + """ + if xml_obj.tag != "course": + expected_attr = {'url_name'} + else: + expected_attr = {'url_name', 'course', 'org'} + + actual_attr = set(xml_obj.attrib.keys()) + + has_text = xml_obj.text is not None and len(xml_obj.text.strip()) > 0 + + return len(xml_obj) == 0 and actual_attr == expected_attr and not has_text + + +def load_definition_xml(node, runtime, def_id): + """ + Loads definition_xml stored in a dedicated file + """ + url_name = node.get('url_name') + filepath = format_filepath(node.tag, name_to_pathname(url_name)) + definition_xml = load_file(filepath, runtime.resources_fs, def_id) + return definition_xml, filepath + + +def format_filepath(category, name): + return f'{category}/{name}.xml' + + +def load_file(filepath, fs, def_id): # pylint: disable=invalid-name + """ + Open the specified file in fs, and call cls.file_to_xml on it, + returning the lxml object. + + Add details and reraise on error. + """ + try: + with fs.open(filepath) as xml_file: + return file_to_xml(xml_file) + except Exception as err: # lint-amnesty, pylint: disable=broad-except + # Add info about where we are, but keep the traceback + raise Exception(f'Unable to load file contents at path {filepath} for item {def_id}: {err}') from err + + +def file_to_xml(file_object): + """ + Used when this module wants to parse a file object to xml + that will be converted to the definition. + + Returns an lxml Element + """ + return etree.parse(file_object, parser=XML_PARSER).getroot() + + class _AutoNamedFieldsMetaclass(type): """ Builds classes such that their Field attributes know their own names. @@ -746,6 +819,11 @@ def parse_xml(cls, node, runtime, keys): keys (:class:`.ScopeIds`): The keys identifying where this block will store its data. """ + if is_pointer_tag(node): + # new style: + # read the actual definition file--named using url_name.replace(':','/') + node, _ = load_definition_xml(node, runtime, keys.def_id) + block = runtime.construct_xblock_from_class(cls, keys) # The base implementation: child nodes become child blocks. From 781023dc0e74d32ec6dc72faa124fb3e7c97845e Mon Sep 17 00:00:00 2001 From: Muhammad Tayyab Tahir Qureshi Date: Tue, 25 Mar 2025 17:34:10 +0500 Subject: [PATCH 2/8] test: add test cases --- xblock/test/test_core.py | 65 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/xblock/test/test_core.py b/xblock/test/test_core.py index 8a888a210..11e3abd6b 100644 --- a/xblock/test/test_core.py +++ b/xblock/test/test_core.py @@ -13,10 +13,19 @@ import ddt import pytest +from lxml import etree from opaque_keys.edx.locator import LibraryUsageLocatorV2, LibraryLocatorV2 from webob import Response from xblock.core import Blocklike, XBlock +from xblock.core import ( + name_to_pathname, + is_pointer_tag, + load_definition_xml, + format_filepath, + file_to_xml, + XML_PARSER, +) from xblock.exceptions import ( XBlockSaveError, KeyValueMultiSaveError, @@ -1163,3 +1172,59 @@ def test_key_properties_when_usage_is_not_an_opaque_key(self): block = XBlock(Mock(spec=Runtime), scope_ids=scope_ids) self.assertEqual(block.usage_key, "myWeirdOldUsageId") self.assertIsNone(block.context_key) + + +class TestCoreFunctions(unittest.TestCase): + """ + Tests for core functions in XBlock. + """ + def test_name_to_pathname(self): + self.assertEqual(name_to_pathname("course:subcourse"), "course/subcourse") + self.assertEqual(name_to_pathname("module:lesson:part"), "module/lesson/part") + self.assertEqual(name_to_pathname("no_colon"), "no_colon") + + def test_is_pointer_tag(self): + # Case 1: Valid pointer tag + xml_obj = etree.Element("some_tag", url_name="test_url") + self.assertTrue(is_pointer_tag(xml_obj)) + + # Case 2: Valid course pointer tag + xml_obj = etree.Element("course", url_name="test_url", course="test_course", org="test_org") + self.assertTrue(is_pointer_tag(xml_obj)) + + # Case 3: Invalid case - extra attribute + xml_obj = etree.Element("some_tag", url_name="test_url", extra_attr="invalid") + self.assertFalse(is_pointer_tag(xml_obj)) + + # Case 4: Invalid case - has text + xml_obj = etree.Element("some_tag", url_name="test_url") + xml_obj.text = "invalid_text" + self.assertFalse(is_pointer_tag(xml_obj)) + + # Case 5: Invalid case - has children + xml_obj = etree.Element("some_tag", url_name="test_url") + _ = etree.SubElement(xml_obj, "child") + self.assertFalse(is_pointer_tag(xml_obj)) + + @patch("xblock.core.load_file") + def test_load_definition_xml(self, mock_load_file): + mock_load_file.return_value = "" + node = etree.Element("course", url_name="test_url") + runtime = Mock() + def_id = "mock_id" + + definition_xml, filepath = load_definition_xml(node, runtime, def_id) + self.assertEqual(filepath, "course/test_url.xml") + self.assertEqual(definition_xml, "") + mock_load_file.assert_called_once() + + def test_format_filepath(self): + self.assertEqual(format_filepath("course", "test_url"), "course/test_url.xml") + + @patch("xblock.core.etree.parse") + def test_file_to_xml(self, mock_etree_parse): + mock_etree_parse.return_value.getroot.return_value = "mock_xml_root" + file_object = Mock() + result = file_to_xml(file_object) + self.assertEqual(result, "mock_xml_root") + mock_etree_parse.assert_called_once_with(file_object, parser=XML_PARSER) From dec49e8d7e96bc39bd4b8b1924526df40cfbd76b Mon Sep 17 00:00:00 2001 From: Muhammad Tayyab Tahir Qureshi Date: Wed, 9 Apr 2025 13:37:23 +0500 Subject: [PATCH 3/8] refactor: move the pointer-tag parsing logic from `core.py` to `utils/helpers.py` --- xblock/core.py | 73 +----------------------------- xblock/test/test_core.py | 65 --------------------------- xblock/test/utils/test_helpers.py | 68 +++++++++++++++++++++++++++- xblock/utils/helpers.py | 75 +++++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 138 deletions(-) diff --git a/xblock/core.py b/xblock/core.py index a52d8e4fe..4a3910baf 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -23,6 +23,7 @@ from xblock.fields import Field, List, Reference, ReferenceList, Scope, String from xblock.internal import class_lazy from xblock.plugin import Plugin +from xblock.utils.helpers import is_pointer_tag, load_definition_xml from xblock.validation import Validation # OrderedDict is used so that namespace attributes are put in predictable order @@ -32,7 +33,6 @@ ("block", "http://code.edx.org/xblock/block"), ]) -XML_PARSER = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True, encoding='utf-8') # __all__ controls what classes end up in the docs. __all__ = ['XBlock', 'XBlockAside'] @@ -40,77 +40,6 @@ UNSET = object() -def name_to_pathname(name): - """ - Convert a location name for use in a path: replace ':' with '/'. - This allows users of the xml format to organize content into directories - """ - return name.replace(':', '/') - - -def is_pointer_tag(xml_obj): - """ - Check if xml_obj is a pointer tag: . - No children, one attribute named url_name, no text. - - Special case for course roots: the pointer is - - - xml_obj: an etree Element - - Returns a bool. - """ - if xml_obj.tag != "course": - expected_attr = {'url_name'} - else: - expected_attr = {'url_name', 'course', 'org'} - - actual_attr = set(xml_obj.attrib.keys()) - - has_text = xml_obj.text is not None and len(xml_obj.text.strip()) > 0 - - return len(xml_obj) == 0 and actual_attr == expected_attr and not has_text - - -def load_definition_xml(node, runtime, def_id): - """ - Loads definition_xml stored in a dedicated file - """ - url_name = node.get('url_name') - filepath = format_filepath(node.tag, name_to_pathname(url_name)) - definition_xml = load_file(filepath, runtime.resources_fs, def_id) - return definition_xml, filepath - - -def format_filepath(category, name): - return f'{category}/{name}.xml' - - -def load_file(filepath, fs, def_id): # pylint: disable=invalid-name - """ - Open the specified file in fs, and call cls.file_to_xml on it, - returning the lxml object. - - Add details and reraise on error. - """ - try: - with fs.open(filepath) as xml_file: - return file_to_xml(xml_file) - except Exception as err: # lint-amnesty, pylint: disable=broad-except - # Add info about where we are, but keep the traceback - raise Exception(f'Unable to load file contents at path {filepath} for item {def_id}: {err}') from err - - -def file_to_xml(file_object): - """ - Used when this module wants to parse a file object to xml - that will be converted to the definition. - - Returns an lxml Element - """ - return etree.parse(file_object, parser=XML_PARSER).getroot() - - class _AutoNamedFieldsMetaclass(type): """ Builds classes such that their Field attributes know their own names. diff --git a/xblock/test/test_core.py b/xblock/test/test_core.py index 11e3abd6b..8a888a210 100644 --- a/xblock/test/test_core.py +++ b/xblock/test/test_core.py @@ -13,19 +13,10 @@ import ddt import pytest -from lxml import etree from opaque_keys.edx.locator import LibraryUsageLocatorV2, LibraryLocatorV2 from webob import Response from xblock.core import Blocklike, XBlock -from xblock.core import ( - name_to_pathname, - is_pointer_tag, - load_definition_xml, - format_filepath, - file_to_xml, - XML_PARSER, -) from xblock.exceptions import ( XBlockSaveError, KeyValueMultiSaveError, @@ -1172,59 +1163,3 @@ def test_key_properties_when_usage_is_not_an_opaque_key(self): block = XBlock(Mock(spec=Runtime), scope_ids=scope_ids) self.assertEqual(block.usage_key, "myWeirdOldUsageId") self.assertIsNone(block.context_key) - - -class TestCoreFunctions(unittest.TestCase): - """ - Tests for core functions in XBlock. - """ - def test_name_to_pathname(self): - self.assertEqual(name_to_pathname("course:subcourse"), "course/subcourse") - self.assertEqual(name_to_pathname("module:lesson:part"), "module/lesson/part") - self.assertEqual(name_to_pathname("no_colon"), "no_colon") - - def test_is_pointer_tag(self): - # Case 1: Valid pointer tag - xml_obj = etree.Element("some_tag", url_name="test_url") - self.assertTrue(is_pointer_tag(xml_obj)) - - # Case 2: Valid course pointer tag - xml_obj = etree.Element("course", url_name="test_url", course="test_course", org="test_org") - self.assertTrue(is_pointer_tag(xml_obj)) - - # Case 3: Invalid case - extra attribute - xml_obj = etree.Element("some_tag", url_name="test_url", extra_attr="invalid") - self.assertFalse(is_pointer_tag(xml_obj)) - - # Case 4: Invalid case - has text - xml_obj = etree.Element("some_tag", url_name="test_url") - xml_obj.text = "invalid_text" - self.assertFalse(is_pointer_tag(xml_obj)) - - # Case 5: Invalid case - has children - xml_obj = etree.Element("some_tag", url_name="test_url") - _ = etree.SubElement(xml_obj, "child") - self.assertFalse(is_pointer_tag(xml_obj)) - - @patch("xblock.core.load_file") - def test_load_definition_xml(self, mock_load_file): - mock_load_file.return_value = "" - node = etree.Element("course", url_name="test_url") - runtime = Mock() - def_id = "mock_id" - - definition_xml, filepath = load_definition_xml(node, runtime, def_id) - self.assertEqual(filepath, "course/test_url.xml") - self.assertEqual(definition_xml, "") - mock_load_file.assert_called_once() - - def test_format_filepath(self): - self.assertEqual(format_filepath("course", "test_url"), "course/test_url.xml") - - @patch("xblock.core.etree.parse") - def test_file_to_xml(self, mock_etree_parse): - mock_etree_parse.return_value.getroot.return_value = "mock_xml_root" - file_object = Mock() - result = file_to_xml(file_object) - self.assertEqual(result, "mock_xml_root") - mock_etree_parse.assert_called_once_with(file_object, parser=XML_PARSER) diff --git a/xblock/test/utils/test_helpers.py b/xblock/test/utils/test_helpers.py index c76344e11..2586efe78 100644 --- a/xblock/test/utils/test_helpers.py +++ b/xblock/test/utils/test_helpers.py @@ -3,10 +3,20 @@ """ import unittest +from unittest.mock import patch, Mock +from lxml import etree from xblock.core import XBlock from xblock.test.toy_runtime import ToyRuntime -from xblock.utils.helpers import child_isinstance +from xblock.utils.helpers import ( + child_isinstance, + name_to_pathname, + is_pointer_tag, + load_definition_xml, + format_filepath, + file_to_xml, + XML_PARSER, +) # pylint: disable=unnecessary-pass @@ -78,3 +88,59 @@ def test_child_isinstance_descendants(self): self.assertTrue(child_isinstance(root, block.children[1], DogXBlock)) self.assertTrue(child_isinstance(root, block.children[1], GoldenRetrieverXBlock)) self.assertFalse(child_isinstance(root, block.children[1], CatXBlock)) + + +class TestPointerTagParsing(unittest.TestCase): + """ + Tests for core functions in XBlock. + """ + def test_name_to_pathname(self): + self.assertEqual(name_to_pathname("course:subcourse"), "course/subcourse") + self.assertEqual(name_to_pathname("module:lesson:part"), "module/lesson/part") + self.assertEqual(name_to_pathname("no_colon"), "no_colon") + + def test_is_pointer_tag(self): + # Case 1: Valid pointer tag + xml_obj = etree.Element("some_tag", url_name="test_url") + self.assertTrue(is_pointer_tag(xml_obj)) + + # Case 2: Valid course pointer tag + xml_obj = etree.Element("course", url_name="test_url", course="test_course", org="test_org") + self.assertTrue(is_pointer_tag(xml_obj)) + + # Case 3: Invalid case - extra attribute + xml_obj = etree.Element("some_tag", url_name="test_url", extra_attr="invalid") + self.assertFalse(is_pointer_tag(xml_obj)) + + # Case 4: Invalid case - has text + xml_obj = etree.Element("some_tag", url_name="test_url") + xml_obj.text = "invalid_text" + self.assertFalse(is_pointer_tag(xml_obj)) + + # Case 5: Invalid case - has children + xml_obj = etree.Element("some_tag", url_name="test_url") + _ = etree.SubElement(xml_obj, "child") + self.assertFalse(is_pointer_tag(xml_obj)) + + @patch("xblock.utils.helpers.load_file") + def test_load_definition_xml(self, mock_load_file): + mock_load_file.return_value = "" + node = etree.Element("course", url_name="test_url") + runtime = Mock() + def_id = "mock_id" + + definition_xml, filepath = load_definition_xml(node, runtime, def_id) + self.assertEqual(filepath, "course/test_url.xml") + self.assertEqual(definition_xml, "") + mock_load_file.assert_called_once() + + def test_format_filepath(self): + self.assertEqual(format_filepath("course", "test_url"), "course/test_url.xml") + + @patch("xblock.utils.helpers.etree.parse") + def test_file_to_xml(self, mock_etree_parse): + mock_etree_parse.return_value.getroot.return_value = "mock_xml_root" + file_object = Mock() + result = file_to_xml(file_object) + self.assertEqual(result, "mock_xml_root") + mock_etree_parse.assert_called_once_with(file_object, parser=XML_PARSER) diff --git a/xblock/utils/helpers.py b/xblock/utils/helpers.py index f897f5ee6..24f66992e 100644 --- a/xblock/utils/helpers.py +++ b/xblock/utils/helpers.py @@ -1,6 +1,10 @@ """ Useful helper methods """ +from lxml import etree + + +XML_PARSER = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True, encoding='utf-8') def child_isinstance(block, child_id, block_class_or_mixin): @@ -23,3 +27,74 @@ def child_isinstance(block, child_id, block_class_or_mixin): type_name = block.runtime.id_reader.get_block_type(def_id) child_class = block.runtime.load_block_type(type_name) return issubclass(child_class, block_class_or_mixin) + + +def name_to_pathname(name): + """ + Convert a location name for use in a path: replace ':' with '/'. + This allows users of the xml format to organize content into directories + """ + return name.replace(':', '/') + + +def is_pointer_tag(xml_obj): + """ + Check if xml_obj is a pointer tag: . + No children, one attribute named url_name, no text. + + Special case for course roots: the pointer is + + + xml_obj: an etree Element + + Returns a bool. + """ + if xml_obj.tag != "course": + expected_attr = {'url_name'} + else: + expected_attr = {'url_name', 'course', 'org'} + + actual_attr = set(xml_obj.attrib.keys()) + + has_text = xml_obj.text is not None and len(xml_obj.text.strip()) > 0 + + return len(xml_obj) == 0 and actual_attr == expected_attr and not has_text + + +def load_definition_xml(node, runtime, def_id): + """ + Loads definition_xml stored in a dedicated file + """ + url_name = node.get('url_name') + filepath = format_filepath(node.tag, name_to_pathname(url_name)) + definition_xml = load_file(filepath, runtime.resources_fs, def_id) + return definition_xml, filepath + + +def format_filepath(category, name): + return f'{category}/{name}.xml' + + +def load_file(filepath, fs, def_id): # pylint: disable=invalid-name + """ + Open the specified file in fs, and call cls.file_to_xml on it, + returning the lxml object. + + Add details and reraise on error. + """ + try: + with fs.open(filepath) as xml_file: + return file_to_xml(xml_file) + except Exception as err: # lint-amnesty + # Add info about where we are, but keep the traceback + raise Exception(f'Unable to load file contents at path {filepath} for item {def_id}: {err}') from err + + +def file_to_xml(file_object): + """ + Used when this module wants to parse a file object to xml + that will be converted to the definition. + + Returns an lxml Element + """ + return etree.parse(file_object, parser=XML_PARSER).getroot() From e42f36d087cd9aa9fb6e6a19c0dd2522b686bdc4 Mon Sep 17 00:00:00 2001 From: Muhammad Tayyab Tahir Qureshi Date: Thu, 10 Apr 2025 13:01:45 +0500 Subject: [PATCH 4/8] docs: update docstrings --- xblock/core.py | 2 -- xblock/utils/helpers.py | 15 ++++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/xblock/core.py b/xblock/core.py index 4a3910baf..598b99db2 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -749,8 +749,6 @@ def parse_xml(cls, node, runtime, keys): will store its data. """ if is_pointer_tag(node): - # new style: - # read the actual definition file--named using url_name.replace(':','/') node, _ = load_definition_xml(node, runtime, keys.def_id) block = runtime.construct_xblock_from_class(cls, keys) diff --git a/xblock/utils/helpers.py b/xblock/utils/helpers.py index 24f66992e..bb4083e9d 100644 --- a/xblock/utils/helpers.py +++ b/xblock/utils/helpers.py @@ -63,7 +63,17 @@ def is_pointer_tag(xml_obj): def load_definition_xml(node, runtime, def_id): """ - Loads definition_xml stored in a dedicated file + Parses and loads an XML definition file based on a given node, runtime + environment, and definition ID. + + Arguments: + node: XML element containing attributes for definition loading. + runtime: The runtime environment that provides resource access. + def_id: Unique identifier for the definition being loaded. + + Returns: + tuple: A tuple containing the loaded XML definition and the + corresponding file path. """ url_name = node.get('url_name') filepath = format_filepath(node.tag, name_to_pathname(url_name)) @@ -72,6 +82,9 @@ def load_definition_xml(node, runtime, def_id): def format_filepath(category, name): + """ + Construct a formatted filepath string based on the given category and name. + """ return f'{category}/{name}.xml' From d8f02a9ac1df87404346027189fdc658a17404f5 Mon Sep 17 00:00:00 2001 From: Muhammad Tayyab Tahir Qureshi Date: Thu, 10 Apr 2025 13:32:21 +0500 Subject: [PATCH 5/8] test: update `test_file_to_xml` --- xblock/test/utils/test_helpers.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/xblock/test/utils/test_helpers.py b/xblock/test/utils/test_helpers.py index 2586efe78..6fcc3193c 100644 --- a/xblock/test/utils/test_helpers.py +++ b/xblock/test/utils/test_helpers.py @@ -3,6 +3,7 @@ """ import unittest +from io import BytesIO from unittest.mock import patch, Mock from lxml import etree @@ -15,7 +16,6 @@ load_definition_xml, format_filepath, file_to_xml, - XML_PARSER, ) @@ -137,10 +137,15 @@ def test_load_definition_xml(self, mock_load_file): def test_format_filepath(self): self.assertEqual(format_filepath("course", "test_url"), "course/test_url.xml") - @patch("xblock.utils.helpers.etree.parse") - def test_file_to_xml(self, mock_etree_parse): - mock_etree_parse.return_value.getroot.return_value = "mock_xml_root" - file_object = Mock() - result = file_to_xml(file_object) - self.assertEqual(result, "mock_xml_root") - mock_etree_parse.assert_called_once_with(file_object, parser=XML_PARSER) + def test_file_to_xml(self): + """Test that `file_to_xml` correctly parses XML from a file object.""" + # Create a BytesIO object + file_obj = BytesIO(b"Value") + + # Parse the XML + result = file_to_xml(file_obj) + + # Verify the result + self.assertEqual(result.tag, 'root') + self.assertEqual(result[0].tag, 'child') + self.assertEqual(result[0].text, 'Value') From fa76371e3bb1c844dee522b154dd8e3fd9aac43d Mon Sep 17 00:00:00 2001 From: "M. Tayyab Tahir Qureshi" <109274085+ttqureshi@users.noreply.github.com> Date: Wed, 7 May 2025 19:00:29 +0500 Subject: [PATCH 6/8] chore: remove `lint-amnesty` Co-authored-by: Kyle McCormick --- xblock/utils/helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xblock/utils/helpers.py b/xblock/utils/helpers.py index bb4083e9d..3177a148c 100644 --- a/xblock/utils/helpers.py +++ b/xblock/utils/helpers.py @@ -98,7 +98,7 @@ def load_file(filepath, fs, def_id): # pylint: disable=invalid-name try: with fs.open(filepath) as xml_file: return file_to_xml(xml_file) - except Exception as err: # lint-amnesty + except Exception as err: # Add info about where we are, but keep the traceback raise Exception(f'Unable to load file contents at path {filepath} for item {def_id}: {err}') from err From 12e8032d2223bbdda5b12e07773ca66e9fea0c4c Mon Sep 17 00:00:00 2001 From: Muhammad Tayyab Tahir Qureshi Date: Thu, 8 May 2025 13:39:10 +0500 Subject: [PATCH 7/8] chore: move xml utility functions to `xblock/xml.py` --- xblock/core.py | 2 +- xblock/test/test_xml.py | 77 ++++++++++++++++++++++++++ xblock/test/utils/test_helpers.py | 73 +------------------------ xblock/utils/helpers.py | 88 ------------------------------ xblock/xml.py | 91 +++++++++++++++++++++++++++++++ 5 files changed, 170 insertions(+), 161 deletions(-) create mode 100644 xblock/test/test_xml.py create mode 100644 xblock/xml.py diff --git a/xblock/core.py b/xblock/core.py index 598b99db2..5c3f188bf 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -23,8 +23,8 @@ from xblock.fields import Field, List, Reference, ReferenceList, Scope, String from xblock.internal import class_lazy from xblock.plugin import Plugin -from xblock.utils.helpers import is_pointer_tag, load_definition_xml from xblock.validation import Validation +from xblock.xml import is_pointer_tag, load_definition_xml # OrderedDict is used so that namespace attributes are put in predictable order # This allows for simple string equality assertions in tests and have no other effects diff --git a/xblock/test/test_xml.py b/xblock/test/test_xml.py new file mode 100644 index 000000000..ca8764d4b --- /dev/null +++ b/xblock/test/test_xml.py @@ -0,0 +1,77 @@ +""" +Tests for xblock/xml.py +""" + +import unittest +from io import BytesIO +from unittest.mock import patch, Mock +from lxml import etree + +from xblock.xml import ( + name_to_pathname, + is_pointer_tag, + load_definition_xml, + format_filepath, + file_to_xml, +) + + +class TestPointerTagParsing(unittest.TestCase): + """ + Tests for core functions in XBlock. + """ + def test_name_to_pathname(self): + self.assertEqual(name_to_pathname("course:subcourse"), "course/subcourse") + self.assertEqual(name_to_pathname("module:lesson:part"), "module/lesson/part") + self.assertEqual(name_to_pathname("no_colon"), "no_colon") + + def test_is_pointer_tag(self): + # Case 1: Valid pointer tag + xml_obj = etree.Element("some_tag", url_name="test_url") + self.assertTrue(is_pointer_tag(xml_obj)) + + # Case 2: Valid course pointer tag + xml_obj = etree.Element("course", url_name="test_url", course="test_course", org="test_org") + self.assertTrue(is_pointer_tag(xml_obj)) + + # Case 3: Invalid case - extra attribute + xml_obj = etree.Element("some_tag", url_name="test_url", extra_attr="invalid") + self.assertFalse(is_pointer_tag(xml_obj)) + + # Case 4: Invalid case - has text + xml_obj = etree.Element("some_tag", url_name="test_url") + xml_obj.text = "invalid_text" + self.assertFalse(is_pointer_tag(xml_obj)) + + # Case 5: Invalid case - has children + xml_obj = etree.Element("some_tag", url_name="test_url") + _ = etree.SubElement(xml_obj, "child") + self.assertFalse(is_pointer_tag(xml_obj)) + + @patch("xblock.xml.load_file") + def test_load_definition_xml(self, mock_load_file): + mock_load_file.return_value = "" + node = etree.Element("course", url_name="test_url") + runtime = Mock() + def_id = "mock_id" + + definition_xml, filepath = load_definition_xml(node, runtime, def_id) + self.assertEqual(filepath, "course/test_url.xml") + self.assertEqual(definition_xml, "") + mock_load_file.assert_called_once() + + def test_format_filepath(self): + self.assertEqual(format_filepath("course", "test_url"), "course/test_url.xml") + + def test_file_to_xml(self): + """Test that `file_to_xml` correctly parses XML from a file object.""" + # Create a BytesIO object + file_obj = BytesIO(b"Value") + + # Parse the XML + result = file_to_xml(file_obj) + + # Verify the result + self.assertEqual(result.tag, 'root') + self.assertEqual(result[0].tag, 'child') + self.assertEqual(result[0].text, 'Value') diff --git a/xblock/test/utils/test_helpers.py b/xblock/test/utils/test_helpers.py index 6fcc3193c..c76344e11 100644 --- a/xblock/test/utils/test_helpers.py +++ b/xblock/test/utils/test_helpers.py @@ -3,20 +3,10 @@ """ import unittest -from io import BytesIO -from unittest.mock import patch, Mock -from lxml import etree from xblock.core import XBlock from xblock.test.toy_runtime import ToyRuntime -from xblock.utils.helpers import ( - child_isinstance, - name_to_pathname, - is_pointer_tag, - load_definition_xml, - format_filepath, - file_to_xml, -) +from xblock.utils.helpers import child_isinstance # pylint: disable=unnecessary-pass @@ -88,64 +78,3 @@ def test_child_isinstance_descendants(self): self.assertTrue(child_isinstance(root, block.children[1], DogXBlock)) self.assertTrue(child_isinstance(root, block.children[1], GoldenRetrieverXBlock)) self.assertFalse(child_isinstance(root, block.children[1], CatXBlock)) - - -class TestPointerTagParsing(unittest.TestCase): - """ - Tests for core functions in XBlock. - """ - def test_name_to_pathname(self): - self.assertEqual(name_to_pathname("course:subcourse"), "course/subcourse") - self.assertEqual(name_to_pathname("module:lesson:part"), "module/lesson/part") - self.assertEqual(name_to_pathname("no_colon"), "no_colon") - - def test_is_pointer_tag(self): - # Case 1: Valid pointer tag - xml_obj = etree.Element("some_tag", url_name="test_url") - self.assertTrue(is_pointer_tag(xml_obj)) - - # Case 2: Valid course pointer tag - xml_obj = etree.Element("course", url_name="test_url", course="test_course", org="test_org") - self.assertTrue(is_pointer_tag(xml_obj)) - - # Case 3: Invalid case - extra attribute - xml_obj = etree.Element("some_tag", url_name="test_url", extra_attr="invalid") - self.assertFalse(is_pointer_tag(xml_obj)) - - # Case 4: Invalid case - has text - xml_obj = etree.Element("some_tag", url_name="test_url") - xml_obj.text = "invalid_text" - self.assertFalse(is_pointer_tag(xml_obj)) - - # Case 5: Invalid case - has children - xml_obj = etree.Element("some_tag", url_name="test_url") - _ = etree.SubElement(xml_obj, "child") - self.assertFalse(is_pointer_tag(xml_obj)) - - @patch("xblock.utils.helpers.load_file") - def test_load_definition_xml(self, mock_load_file): - mock_load_file.return_value = "" - node = etree.Element("course", url_name="test_url") - runtime = Mock() - def_id = "mock_id" - - definition_xml, filepath = load_definition_xml(node, runtime, def_id) - self.assertEqual(filepath, "course/test_url.xml") - self.assertEqual(definition_xml, "") - mock_load_file.assert_called_once() - - def test_format_filepath(self): - self.assertEqual(format_filepath("course", "test_url"), "course/test_url.xml") - - def test_file_to_xml(self): - """Test that `file_to_xml` correctly parses XML from a file object.""" - # Create a BytesIO object - file_obj = BytesIO(b"Value") - - # Parse the XML - result = file_to_xml(file_obj) - - # Verify the result - self.assertEqual(result.tag, 'root') - self.assertEqual(result[0].tag, 'child') - self.assertEqual(result[0].text, 'Value') diff --git a/xblock/utils/helpers.py b/xblock/utils/helpers.py index 3177a148c..f897f5ee6 100644 --- a/xblock/utils/helpers.py +++ b/xblock/utils/helpers.py @@ -1,10 +1,6 @@ """ Useful helper methods """ -from lxml import etree - - -XML_PARSER = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True, encoding='utf-8') def child_isinstance(block, child_id, block_class_or_mixin): @@ -27,87 +23,3 @@ def child_isinstance(block, child_id, block_class_or_mixin): type_name = block.runtime.id_reader.get_block_type(def_id) child_class = block.runtime.load_block_type(type_name) return issubclass(child_class, block_class_or_mixin) - - -def name_to_pathname(name): - """ - Convert a location name for use in a path: replace ':' with '/'. - This allows users of the xml format to organize content into directories - """ - return name.replace(':', '/') - - -def is_pointer_tag(xml_obj): - """ - Check if xml_obj is a pointer tag: . - No children, one attribute named url_name, no text. - - Special case for course roots: the pointer is - - - xml_obj: an etree Element - - Returns a bool. - """ - if xml_obj.tag != "course": - expected_attr = {'url_name'} - else: - expected_attr = {'url_name', 'course', 'org'} - - actual_attr = set(xml_obj.attrib.keys()) - - has_text = xml_obj.text is not None and len(xml_obj.text.strip()) > 0 - - return len(xml_obj) == 0 and actual_attr == expected_attr and not has_text - - -def load_definition_xml(node, runtime, def_id): - """ - Parses and loads an XML definition file based on a given node, runtime - environment, and definition ID. - - Arguments: - node: XML element containing attributes for definition loading. - runtime: The runtime environment that provides resource access. - def_id: Unique identifier for the definition being loaded. - - Returns: - tuple: A tuple containing the loaded XML definition and the - corresponding file path. - """ - url_name = node.get('url_name') - filepath = format_filepath(node.tag, name_to_pathname(url_name)) - definition_xml = load_file(filepath, runtime.resources_fs, def_id) - return definition_xml, filepath - - -def format_filepath(category, name): - """ - Construct a formatted filepath string based on the given category and name. - """ - return f'{category}/{name}.xml' - - -def load_file(filepath, fs, def_id): # pylint: disable=invalid-name - """ - Open the specified file in fs, and call cls.file_to_xml on it, - returning the lxml object. - - Add details and reraise on error. - """ - try: - with fs.open(filepath) as xml_file: - return file_to_xml(xml_file) - except Exception as err: - # Add info about where we are, but keep the traceback - raise Exception(f'Unable to load file contents at path {filepath} for item {def_id}: {err}') from err - - -def file_to_xml(file_object): - """ - Used when this module wants to parse a file object to xml - that will be converted to the definition. - - Returns an lxml Element - """ - return etree.parse(file_object, parser=XML_PARSER).getroot() diff --git a/xblock/xml.py b/xblock/xml.py new file mode 100644 index 000000000..85661c154 --- /dev/null +++ b/xblock/xml.py @@ -0,0 +1,91 @@ +""" +XML utilities for XBlock, including parsing and loading XML definitions. +""" +from lxml import etree + + +XML_PARSER = etree.XMLParser(dtd_validation=False, load_dtd=False, remove_blank_text=True, encoding='utf-8') + + +def name_to_pathname(name): + """ + Convert a location name for use in a path: replace ':' with '/'. + This allows users of the xml format to organize content into directories + """ + return name.replace(':', '/') + + +def is_pointer_tag(xml_obj): + """ + Check if xml_obj is a pointer tag: . + No children, one attribute named url_name, no text. + + Special case for course roots: the pointer is + + + xml_obj: an etree Element + + Returns a bool. + """ + if xml_obj.tag != "course": + expected_attr = {'url_name'} + else: + expected_attr = {'url_name', 'course', 'org'} + + actual_attr = set(xml_obj.attrib.keys()) + + has_text = xml_obj.text is not None and len(xml_obj.text.strip()) > 0 + + return len(xml_obj) == 0 and actual_attr == expected_attr and not has_text + + +def load_definition_xml(node, runtime, def_id): + """ + Parses and loads an XML definition file based on a given node, runtime + environment, and definition ID. + + Arguments: + node: XML element containing attributes for definition loading. + runtime: The runtime environment that provides resource access. + def_id: Unique identifier for the definition being loaded. + + Returns: + tuple: A tuple containing the loaded XML definition and the + corresponding file path. + """ + url_name = node.get('url_name') + filepath = format_filepath(node.tag, name_to_pathname(url_name)) + definition_xml = load_file(filepath, runtime.resources_fs, def_id) + return definition_xml, filepath + + +def format_filepath(category, name): + """ + Construct a formatted filepath string based on the given category and name. + """ + return f'{category}/{name}.xml' + + +def load_file(filepath, fs, def_id): # pylint: disable=invalid-name + """ + Open the specified file in fs, and call cls.file_to_xml on it, + returning the lxml object. + + Add details and reraise on error. + """ + try: + with fs.open(filepath) as xml_file: + return file_to_xml(xml_file) + except Exception as err: + # Add info about where we are, but keep the traceback + raise Exception(f'Unable to load file contents at path {filepath} for item {def_id}: {err}') from err + + +def file_to_xml(file_object): + """ + Used when this module wants to parse a file object to xml + that will be converted to the definition. + + Returns an lxml Element + """ + return etree.parse(file_object, parser=XML_PARSER).getroot() From ea7eddd52f0498caf7bef16415f13ccf9556553f Mon Sep 17 00:00:00 2001 From: Muhammad Tayyab Tahir Qureshi Date: Mon, 12 May 2025 17:31:15 +0500 Subject: [PATCH 8/8] refactor: delegate `load_definition_xml` to `Runtime` --- xblock/core.py | 4 ++-- xblock/runtime.py | 19 +++++++++++++++++++ xblock/test/test_xml.py | 14 -------------- xblock/xml.py | 20 -------------------- 4 files changed, 21 insertions(+), 36 deletions(-) diff --git a/xblock/core.py b/xblock/core.py index 5c3f188bf..34306e1e1 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -24,7 +24,7 @@ from xblock.internal import class_lazy from xblock.plugin import Plugin from xblock.validation import Validation -from xblock.xml import is_pointer_tag, load_definition_xml +from xblock.xml import is_pointer_tag # OrderedDict is used so that namespace attributes are put in predictable order # This allows for simple string equality assertions in tests and have no other effects @@ -749,7 +749,7 @@ def parse_xml(cls, node, runtime, keys): will store its data. """ if is_pointer_tag(node): - node, _ = load_definition_xml(node, runtime, keys.def_id) + node, _ = runtime.load_definition_xml(node, keys.def_id) block = runtime.construct_xblock_from_class(cls, keys) diff --git a/xblock/runtime.py b/xblock/runtime.py index 8aa822dda..b3c0726a4 100644 --- a/xblock/runtime.py +++ b/xblock/runtime.py @@ -31,6 +31,7 @@ FieldDataDeprecationWarning, UserIdDeprecationWarning, ) +from xblock.xml import format_filepath, load_file, name_to_pathname log = logging.getLogger(__name__) @@ -792,6 +793,24 @@ def add_block_as_child_node(self, block, node): child = etree.SubElement(node, "unknown") block.add_xml_to_node(child) + def load_definition_xml(self, node, def_id): + """ + Load an XML definition for a block. + This method can be overridden in different runtime environments. + + Args: + node: XML element containing attributes for definition loading + def_id: Unique identifier for the definition being loaded + + Returns: + tuple: A tuple containing the loaded XML definition and the + corresponding file path or identifier + """ + url_name = node.get('url_name') + filepath = format_filepath(node.tag, name_to_pathname(url_name)) + definition_xml = load_file(filepath, self.resources_fs, def_id) # pylint: disable=no-member + return definition_xml, filepath + # Rendering def render(self, block, view_name, context=None): diff --git a/xblock/test/test_xml.py b/xblock/test/test_xml.py index ca8764d4b..763246d24 100644 --- a/xblock/test/test_xml.py +++ b/xblock/test/test_xml.py @@ -4,13 +4,11 @@ import unittest from io import BytesIO -from unittest.mock import patch, Mock from lxml import etree from xblock.xml import ( name_to_pathname, is_pointer_tag, - load_definition_xml, format_filepath, file_to_xml, ) @@ -48,18 +46,6 @@ def test_is_pointer_tag(self): _ = etree.SubElement(xml_obj, "child") self.assertFalse(is_pointer_tag(xml_obj)) - @patch("xblock.xml.load_file") - def test_load_definition_xml(self, mock_load_file): - mock_load_file.return_value = "" - node = etree.Element("course", url_name="test_url") - runtime = Mock() - def_id = "mock_id" - - definition_xml, filepath = load_definition_xml(node, runtime, def_id) - self.assertEqual(filepath, "course/test_url.xml") - self.assertEqual(definition_xml, "") - mock_load_file.assert_called_once() - def test_format_filepath(self): self.assertEqual(format_filepath("course", "test_url"), "course/test_url.xml") diff --git a/xblock/xml.py b/xblock/xml.py index 85661c154..8466a1d84 100644 --- a/xblock/xml.py +++ b/xblock/xml.py @@ -39,26 +39,6 @@ def is_pointer_tag(xml_obj): return len(xml_obj) == 0 and actual_attr == expected_attr and not has_text -def load_definition_xml(node, runtime, def_id): - """ - Parses and loads an XML definition file based on a given node, runtime - environment, and definition ID. - - Arguments: - node: XML element containing attributes for definition loading. - runtime: The runtime environment that provides resource access. - def_id: Unique identifier for the definition being loaded. - - Returns: - tuple: A tuple containing the loaded XML definition and the - corresponding file path. - """ - url_name = node.get('url_name') - filepath = format_filepath(node.tag, name_to_pathname(url_name)) - definition_xml = load_file(filepath, runtime.resources_fs, def_id) - return definition_xml, filepath - - def format_filepath(category, name): """ Construct a formatted filepath string based on the given category and name.