From e9a8067fda85c3b57bf00706759f419a3ba8a2a7 Mon Sep 17 00:00:00 2001 From: Jan Britz Date: Wed, 4 Dec 2024 18:29:26 +0100 Subject: [PATCH 1/3] feat: more error handling `ExpectedAncestorError`, `InvalidTextPlacementError`, and `UnknownAttributeError` --- .../webserver/question_ui/__init__.py | 60 ++++++++++++++----- .../webserver/question_ui/errors.py | 40 ++++++++++++- .../webserver/test_data/faulty.xhtml | 3 + .../webserver/test_question_ui.py | 24 +++++--- 4 files changed, 103 insertions(+), 24 deletions(-) diff --git a/questionpy_sdk/webserver/question_ui/__init__.py b/questionpy_sdk/webserver/question_ui/__init__.py index b311f3ba..335917af 100644 --- a/questionpy_sdk/webserver/question_ui/__init__.py +++ b/questionpy_sdk/webserver/question_ui/__init__.py @@ -15,10 +15,13 @@ from questionpy_sdk.webserver.question_ui.errors import ( ConversionError, + ExpectedAncestorError, InvalidAttributeValueError, InvalidCleanOptionError, + InvalidTextPlacementError, PlaceholderReferenceError, RenderErrorCollection, + UnknownAttributeError, UnknownElementError, XMLSyntaxError, ) @@ -582,9 +585,9 @@ def collect(self) -> RenderErrorCollection: self._validate_placeholders() self._validate_feedback() self._validate_if_role() - self._validate_shuffle_contents() + self._validate_shuffle_contents_and_shuffled_index() self._validate_format_floats() - self._look_for_unknown_qpy_elements() + self._look_for_unknown_qpy_elements_and_attributes() return self.errors @@ -639,11 +642,8 @@ def _validate_if_role(self) -> None: ) self.errors.insert(error) - def _validate_shuffle_contents(self) -> None: + def _validate_shuffle_contents_and_shuffled_index(self) -> None: """Validates elements marked with `qpy:shuffle-contents`.""" - # TODO: - check if shuffle-contents has children - # - check if shuffled-index elements have a parent element marked with qpy:shuffle-contests - for element in _assert_element_list(self._xpath("//*[@qpy:shuffle-contents]")): child_elements = [child for child in element if isinstance(child, etree._Element)] for child in child_elements: @@ -652,7 +652,24 @@ def _validate_shuffle_contents(self) -> None: ): format_style = index_element.get("format", "123") if format_style not in {"123", "abc", "ABC", "iii", "III"}: - self.errors.insert(InvalidAttributeValueError(index_element, "format", format_style)) + attribute_error = InvalidAttributeValueError( + element=index_element, attribute="format", value=format_style + ) + self.errors.insert(attribute_error) + + # Gather every qpy:shuffle-contents with direct text nodes. + for element in _assert_element_list( + self._xpath("//*[@qpy:shuffle-contents and text()[normalize-space()] != '']") + ): + placement_error = InvalidTextPlacementError(element=element, attribute="qpy:shuffle-contents") + self.errors.insert(placement_error) + + # Gather every qpy:shuffled-index without qpy:shuffle-contents ancestor. + for element in _assert_element_list( + self._xpath("//qpy:shuffled-index[not(ancestor::*[@qpy:shuffle-contents])]") + ): + ancestor_error = ExpectedAncestorError(element=element, expected_ancestor_attribute="qpy:shuffle-contents") + self.errors.insert(ancestor_error) def _validate_format_floats(self) -> None: """Validates the `qpy:format-float` element.""" @@ -692,13 +709,24 @@ def _validate_format_floats(self) -> None: ) self.errors.insert(thousands_sep_error) - def _look_for_unknown_qpy_elements(self) -> None: - """Checks if there are any unknown qpy-elements. - - TODO: also look for unknown qpy-attributes - """ + def _look_for_unknown_qpy_elements_and_attributes(self) -> None: + """Checks if there are any unknown qpy-elements or -attributes.""" + # Gather unknown elements. known_elements = ["shuffled-index", "format-float"] - xpath = " and ".join([f"name() != 'qpy:{element}'" for element in known_elements]) - for element in _assert_element_list(self._xpath(f"//*[starts-with(name(), 'qpy:') and {xpath}]")): - error = UnknownElementError(element=element) - self.errors.insert(error) + xpath_elements = " and ".join(f"name() != 'qpy:{element}'" for element in known_elements) + for element in _assert_element_list(self._xpath(f"//*[starts-with(name(), 'qpy:') and {xpath_elements}]")): + unknown_element_error = UnknownElementError(element=element) + self.errors.insert(unknown_element_error) + + # Gather unknown attributes. + known_attrs = ["feedback", "if-role", "shuffle-contents", "correct-response"] + xpath_attrs = " and ".join(f"name() != 'qpy:{attr}'" for attr in known_attrs) + for element in _assert_element_list(self._xpath(f"//*[@*[starts-with(name(), 'qpy:') and {xpath_attrs}]]")): + unknown_attributes: list[str] = [ + attr.replace(f"{{{_QPY_NAMESPACE}}}", "qpy:") + for attr in map(str, element.attrib) + if attr.startswith(f"{{{_QPY_NAMESPACE}}}") and attr.split("}")[1] not in known_attrs + ] + if unknown_attributes: + unknown_attribute_error = UnknownAttributeError(element=element, attributes=unknown_attributes) + self.errors.insert(unknown_attribute_error) diff --git a/questionpy_sdk/webserver/question_ui/errors.py b/questionpy_sdk/webserver/question_ui/errors.py index 6e0f2968..7f4e0f6c 100644 --- a/questionpy_sdk/webserver/question_ui/errors.py +++ b/questionpy_sdk/webserver/question_ui/errors.py @@ -180,6 +180,31 @@ def __init__(self, element: etree._Element, option: str, expected: Collection[st ) +@dataclass(frozen=True) +class InvalidTextPlacementError(RenderElementError): + """Invalid text placement.""" + + def __init__(self, element: etree._Element, attribute: str): + super().__init__( + element=element, + template="Avoid placing text directly inside {element} with the {attribute} attribute. Use child elements " + "for text instead.", + template_kwargs={"attribute": attribute}, + ) + + +@dataclass(frozen=True) +class ExpectedAncestorError(RenderElementError): + """Invalid element placement.""" + + def __init__(self, element: etree._Element, expected_ancestor_attribute: str): + super().__init__( + element=element, + template="{element} must be placed inside an element with the {expected_ancestor_attribute} attribute.", + template_kwargs={"expected_ancestor_attribute": expected_ancestor_attribute}, + ) + + @dataclass(frozen=True) class UnknownElementError(RenderElementError): """Unknown element with qpy-namespace.""" @@ -191,6 +216,19 @@ def __init__(self, element: etree._Element): ) +@dataclass(frozen=True) +class UnknownAttributeError(RenderElementError): + """Unknown attribute with qpy-namespace.""" + + def __init__(self, element: etree._Element, attributes: Collection[str]): + s = "" if len(attributes) == 1 else "s" + super().__init__( + element=element, + template=f"Unknown attribute{s} {{attributes}} on element {{element}}.", + template_kwargs={"attributes": attributes}, + ) + + @dataclass(frozen=True) class XMLSyntaxError(RenderError): """Syntax error while parsing the XML.""" @@ -247,5 +285,5 @@ def log_render_errors(render_errors: RenderErrorCollections) -> None: line = f"Line {error.line}: " if error.line else "" errors_string += f"\n\t- {line}{error.type} - {error.message}" error_count = len(errors) - s = "s" if error_count > 1 else "" + s = "" if error_count == 1 else "s" _log.warning(f"{error_count} error{s} occurred while rendering {section}:{errors_string}") diff --git a/tests/questionpy_sdk/webserver/test_data/faulty.xhtml b/tests/questionpy_sdk/webserver/test_data/faulty.xhtml index 9c5e981e..99a7ebbf 100644 --- a/tests/questionpy_sdk/webserver/test_data/faulty.xhtml +++ b/tests/questionpy_sdk/webserver/test_data/faulty.xhtml @@ -8,8 +8,11 @@ Invalid shuffle format. . A + Invalid text placement.
Missing placeholder.
Empty placeholder.
+ +
Unknown attribute.
Missing attribute value. diff --git a/tests/questionpy_sdk/webserver/test_question_ui.py b/tests/questionpy_sdk/webserver/test_question_ui.py index cea57767..02a0d098 100644 --- a/tests/questionpy_sdk/webserver/test_question_ui.py +++ b/tests/questionpy_sdk/webserver/test_question_ui.py @@ -7,20 +7,23 @@ import pytest from questionpy_sdk.webserver.question_ui import ( - InvalidAttributeValueError, QuestionDisplayOptions, QuestionDisplayRole, QuestionFormulationUIRenderer, QuestionMetadata, QuestionUIRenderer, - XMLSyntaxError, ) from questionpy_sdk.webserver.question_ui.errors import ( ConversionError, + ExpectedAncestorError, + InvalidAttributeValueError, InvalidCleanOptionError, + InvalidTextPlacementError, PlaceholderReferenceError, RenderError, + UnknownAttributeError, UnknownElementError, + XMLSyntaxError, ) from tests.questionpy_sdk.webserver.conftest import assert_html_is_equal @@ -336,7 +339,7 @@ def test_clean_up(renderer: QuestionUIRenderer) -> None: """ html, errors = renderer.render() - assert len(errors) == 0 + assert len(errors) == 1 # Undefined attribute. assert_html_is_equal(html, expected) @@ -363,9 +366,13 @@ def test_errors_should_be_collected(renderer: QuestionUIRenderer) -> None: expected = """
<qpy:format-float xmlns:qpy="http://questionpy.org/ns/question" xmlns="http://www.w3.org/1999/xhtml" thousands-separator="maybe" precision="invalid">Unknown value.</qpy:format-float> -
+
+ + Invalid text placement. +
Missing placeholder.
Empty placeholder.
+
Unknown attribute.
Missing attribute value.
""" # noqa: E501 @@ -373,17 +380,20 @@ def test_errors_should_be_collected(renderer: QuestionUIRenderer) -> None: expected_errors: list[tuple[type[RenderError], int]] = [ # Even though the syntax error occurs after all the other errors, it should be listed first. - (XMLSyntaxError, 14), + (XMLSyntaxError, 17), (InvalidAttributeValueError, 2), (UnknownElementError, 3), (InvalidAttributeValueError, 4), (ConversionError, 5), (ConversionError, 5), (InvalidAttributeValueError, 5), + (InvalidTextPlacementError, 6), (InvalidAttributeValueError, 9), - (InvalidCleanOptionError, 12), - (PlaceholderReferenceError, 12), + (InvalidCleanOptionError, 13), (PlaceholderReferenceError, 13), + (PlaceholderReferenceError, 14), + (ExpectedAncestorError, 15), + (UnknownAttributeError, 16), ] assert len(errors) == len(expected_errors) From c8bd3b2fce86806905ca4377563b2efee7e0c5ed Mon Sep 17 00:00:00 2001 From: Jan Britz Date: Thu, 5 Dec 2024 13:29:06 +0100 Subject: [PATCH 2/3] feat: also catch PIs as direct children of `shuffle-contents` --- questionpy_sdk/webserver/question_ui/__init__.py | 8 ++++---- questionpy_sdk/webserver/question_ui/errors.py | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/questionpy_sdk/webserver/question_ui/__init__.py b/questionpy_sdk/webserver/question_ui/__init__.py index 335917af..74d31418 100644 --- a/questionpy_sdk/webserver/question_ui/__init__.py +++ b/questionpy_sdk/webserver/question_ui/__init__.py @@ -18,7 +18,7 @@ ExpectedAncestorError, InvalidAttributeValueError, InvalidCleanOptionError, - InvalidTextPlacementError, + InvalidContentError, PlaceholderReferenceError, RenderErrorCollection, UnknownAttributeError, @@ -657,11 +657,11 @@ def _validate_shuffle_contents_and_shuffled_index(self) -> None: ) self.errors.insert(attribute_error) - # Gather every qpy:shuffle-contents with direct text nodes. + # Gather every qpy:shuffle-contents with direct text nodes or processing instructions. for element in _assert_element_list( - self._xpath("//*[@qpy:shuffle-contents and text()[normalize-space()] != '']") + self._xpath("//*[@qpy:shuffle-contents and (text()[normalize-space()] != '' or processing-instruction())]") ): - placement_error = InvalidTextPlacementError(element=element, attribute="qpy:shuffle-contents") + placement_error = InvalidContentError(element=element, attribute="qpy:shuffle-contents") self.errors.insert(placement_error) # Gather every qpy:shuffled-index without qpy:shuffle-contents ancestor. diff --git a/questionpy_sdk/webserver/question_ui/errors.py b/questionpy_sdk/webserver/question_ui/errors.py index 7f4e0f6c..3c57bade 100644 --- a/questionpy_sdk/webserver/question_ui/errors.py +++ b/questionpy_sdk/webserver/question_ui/errors.py @@ -181,14 +181,14 @@ def __init__(self, element: etree._Element, option: str, expected: Collection[st @dataclass(frozen=True) -class InvalidTextPlacementError(RenderElementError): - """Invalid text placement.""" +class InvalidContentError(RenderElementError): + """Invalid content placement.""" def __init__(self, element: etree._Element, attribute: str): super().__init__( element=element, - template="Avoid placing text directly inside {element} with the {attribute} attribute. Use child elements " - "for text instead.", + template="Avoid placing text or processing instructions directly inside {element} with the {attribute} " + "attribute. Wrap the content in an element instead.", template_kwargs={"attribute": attribute}, ) From f752d9438dfcfcbf44862f8165b89f8606a90444 Mon Sep 17 00:00:00 2001 From: Jan Britz Date: Thu, 5 Dec 2024 14:26:07 +0100 Subject: [PATCH 3/3] fix: make code more robust --- questionpy_sdk/webserver/question_ui/__init__.py | 12 ++++++++---- questionpy_sdk/webserver/question_ui/errors.py | 2 +- tests/questionpy_sdk/webserver/test_question_ui.py | 4 ++-- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/questionpy_sdk/webserver/question_ui/__init__.py b/questionpy_sdk/webserver/question_ui/__init__.py index 74d31418..c01c2508 100644 --- a/questionpy_sdk/webserver/question_ui/__init__.py +++ b/questionpy_sdk/webserver/question_ui/__init__.py @@ -713,15 +713,19 @@ def _look_for_unknown_qpy_elements_and_attributes(self) -> None: """Checks if there are any unknown qpy-elements or -attributes.""" # Gather unknown elements. known_elements = ["shuffled-index", "format-float"] - xpath_elements = " and ".join(f"name() != 'qpy:{element}'" for element in known_elements) - for element in _assert_element_list(self._xpath(f"//*[starts-with(name(), 'qpy:') and {xpath_elements}]")): + xpath_elements = " and ".join(f"local-name() != '{element}'" for element in known_elements) + xpath_query = f"//qpy:*[{xpath_elements}]" + + for element in _assert_element_list(self._xpath(xpath_query)): unknown_element_error = UnknownElementError(element=element) self.errors.insert(unknown_element_error) # Gather unknown attributes. known_attrs = ["feedback", "if-role", "shuffle-contents", "correct-response"] - xpath_attrs = " and ".join(f"name() != 'qpy:{attr}'" for attr in known_attrs) - for element in _assert_element_list(self._xpath(f"//*[@*[starts-with(name(), 'qpy:') and {xpath_attrs}]]")): + xpath_attrs = " and ".join(f"local-name() != '{attr}'" for attr in known_attrs) + xpath_query = f"//*[@qpy:*[{xpath_attrs}]]" + + for element in _assert_element_list(self._xpath(xpath_query)): unknown_attributes: list[str] = [ attr.replace(f"{{{_QPY_NAMESPACE}}}", "qpy:") for attr in map(str, element.attrib) diff --git a/questionpy_sdk/webserver/question_ui/errors.py b/questionpy_sdk/webserver/question_ui/errors.py index 3c57bade..f91ea887 100644 --- a/questionpy_sdk/webserver/question_ui/errors.py +++ b/questionpy_sdk/webserver/question_ui/errors.py @@ -188,7 +188,7 @@ def __init__(self, element: etree._Element, attribute: str): super().__init__( element=element, template="Avoid placing text or processing instructions directly inside {element} with the {attribute} " - "attribute. Wrap the content in an element instead.", + "attribute. Wrap the content in an element instead.", template_kwargs={"attribute": attribute}, ) diff --git a/tests/questionpy_sdk/webserver/test_question_ui.py b/tests/questionpy_sdk/webserver/test_question_ui.py index 02a0d098..41b1b3f8 100644 --- a/tests/questionpy_sdk/webserver/test_question_ui.py +++ b/tests/questionpy_sdk/webserver/test_question_ui.py @@ -18,7 +18,7 @@ ExpectedAncestorError, InvalidAttributeValueError, InvalidCleanOptionError, - InvalidTextPlacementError, + InvalidContentError, PlaceholderReferenceError, RenderError, UnknownAttributeError, @@ -387,7 +387,7 @@ def test_errors_should_be_collected(renderer: QuestionUIRenderer) -> None: (ConversionError, 5), (ConversionError, 5), (InvalidAttributeValueError, 5), - (InvalidTextPlacementError, 6), + (InvalidContentError, 6), (InvalidAttributeValueError, 9), (InvalidCleanOptionError, 13), (PlaceholderReferenceError, 13),