From 08fa20ee857b9e76448b9c436094b42a1ed02666 Mon Sep 17 00:00:00 2001 From: Jan Britz Date: Thu, 22 Aug 2024 16:25:44 +0200 Subject: [PATCH] feat: more error handling --- .../templates/formulation.xhtml.j2 | 22 +-- .../webserver/question_ui/__init__.py | 163 ++++++++++++++---- .../webserver/question_ui/errors.py | 151 ++++++++++++---- questionpy_sdk/webserver/static/styles.css | 25 ++- .../webserver/templates/attempt.html.jinja2 | 2 + ruff_defaults.toml | 1 + .../webserver/test_data/faulty.xhtml | 11 +- .../webserver/test_question_ui.py | 44 +++-- 8 files changed, 317 insertions(+), 102 deletions(-) diff --git a/examples/minimal/python/local/minimal_example/templates/formulation.xhtml.j2 b/examples/minimal/python/local/minimal_example/templates/formulation.xhtml.j2 index e2601137..7cc63b76 100644 --- a/examples/minimal/python/local/minimal_example/templates/formulation.xhtml.j2 +++ b/examples/minimal/python/local/minimal_example/templates/formulation.xhtml.j2 @@ -1,21 +1,13 @@
-
-
+ Unknown feedback type. + +
Missing placeholder.
+
Unknown roles.
+ Unknown value. +
- -
diff --git a/questionpy_sdk/webserver/question_ui/__init__.py b/questionpy_sdk/webserver/question_ui/__init__.py index 12dd346c..efb23648 100644 --- a/questionpy_sdk/webserver/question_ui/__init__.py +++ b/questionpy_sdk/webserver/question_ui/__init__.py @@ -14,8 +14,12 @@ from pydantic import BaseModel from questionpy_sdk.webserver.question_ui.errors import ( + ConversionError, InvalidAttributeValueError, + InvalidCleanOptionError, + PlaceholderReferenceError, RenderErrorCollection, + UnknownElementError, XMLSyntaxError, ) @@ -69,7 +73,7 @@ def _set_element_value(element: etree._Element, value: str, name: str, xpath: et element.set("value", value) -def _replace_shuffled_indices(element: etree._Element, index: int) -> None: +def _replace_shuffled_indices(element: etree._Element, index: int, error_collection: RenderErrorCollection) -> None: for index_element in _assert_element_list( element.xpath(".//qpy:shuffled-index", namespaces={"qpy": QuestionUIRenderer.QPY_NAMESPACE}) ): @@ -86,7 +90,9 @@ def _replace_shuffled_indices(element: etree._Element, index: int) -> None: elif format_style == "III": index_str = _int_to_roman(index).upper() else: - index_str = str(index) + error_collection.insert(InvalidAttributeValueError(index_element, "format", format_style)) + _remove_element(index_element) + continue # Replace the index element with the new index string new_text_node = etree.Element("span") # Using span to replace the custom element @@ -148,6 +154,12 @@ def _remove_preserving_tail(node: etree._Element) -> None: _require_parent(node).remove(node) +def _remove_element(node: etree._Element) -> None: + parent = node.getparent() + if parent is not None: + parent.remove(node) + + class QuestionMetadata: def __init__(self) -> None: self.correct_response: dict[str, str] = {} @@ -236,6 +248,39 @@ def _replace_qpy_urls(self, xml: str) -> str: """Replace QPY-URLs to package files with SDK-URLs.""" return re.sub(r"qpy://(static|static-private)/((?:[a-z_][a-z0-9_]{0,126}/){2})", r"/worker/\2file/\1/", xml) + def _validate_placeholder(self, p_instruction: etree._Element) -> tuple[str, str] | None: + """Collects potential render errors for the placeholder PIs. + + Returns: + If no error occurred, a tuple consisting of the placeholder key and the cleaning option. + value. Else, None. + """ + parsing_error = False + if not p_instruction.text: + # TODO: Show an error message? + return None + + parts = p_instruction.text.strip().split(maxsplit=1) + max_parts = 2 + key = parts[0] + clean_option = parts[1].lower() if len(parts) == max_parts else "clean" + expected = ("plain", "clean", "noclean") + if clean_option not in expected: + option_error = InvalidCleanOptionError(element=p_instruction, option=clean_option, expected=expected) + self._errors.insert(option_error) + parsing_error = True + + if key not in self._placeholders: + reference_error = PlaceholderReferenceError( + element=p_instruction, placeholder=key, available=self._placeholders + ) + self._errors.insert(reference_error) + parsing_error = True + + if parsing_error: + return None + return key, clean_option + def _resolve_placeholders(self) -> None: """Replace placeholder PIs such as `` with the appropriate value from `self.placeholders`. @@ -243,20 +288,12 @@ def _resolve_placeholders(self) -> None: last. """ for p_instruction in _assert_element_list(self._xpath("//processing-instruction('p')")): - if not p_instruction.text: - continue - parts = p_instruction.text.strip().split() - key = parts[0] - clean_option = parts[1].lower() if len(parts) > 1 else "clean" - - parent = p_instruction.getparent() - if parent is None: - continue - - if key not in self._placeholders: - parent.remove(p_instruction) + data = self._validate_placeholder(p_instruction) + if data is None: + _remove_element(p_instruction) continue + key, clean_option = data raw_value = self._placeholders[key] if clean_option == "plain": @@ -288,9 +325,7 @@ def _hide_unwanted_feedback(self) -> None: (feedback_type == "general" and self._options.general_feedback) or (feedback_type == "specific" and self._options.feedback) ): - parent = element.getparent() - if parent is not None: - parent.remove(element) + _remove_element(element) expected = ("general", "specific") if feedback_type not in expected: @@ -307,6 +342,18 @@ def _hide_if_role(self) -> None: for element in _assert_element_list(self._xpath("//*[@qpy:if-role]")): if attr := element.get(f"{{{self.QPY_NAMESPACE}}}if-role"): allowed_roles = [role.upper() for role in re.split(r"[\s|]+", attr)] + expected = list(QuestionDisplayRole) + if unexpected := [role for role in allowed_roles if role not in expected]: + error = InvalidAttributeValueError( + element=element, + attribute="qpy:if-role", + value=unexpected, + expected=expected, + ) + self._errors.insert(error) + _remove_element(element) + continue + has_role = any(role in allowed_roles and role in self._options.roles for role in QuestionDisplayRole) if not has_role and (parent := element.getparent()) is not None: @@ -394,16 +441,16 @@ def _shuffle_contents(self) -> None: # Reinsert shuffled elements, preserving non-element nodes for i, child in enumerate(child_elements): - _replace_shuffled_indices(child, i + 1) + _replace_shuffled_indices(child, i + 1, self._errors) # Move each child element back to its parent at the correct position element.append(child) def _clean_up(self) -> None: """Removes remaining QuestionPy elements and attributes as well as comments and xmlns declarations.""" for element in _assert_element_list(self._xpath("//qpy:*")): - parent = element.getparent() - if parent is not None: - parent.remove(element) + error = UnknownElementError(element=element) + self._errors.insert(error) + _remove_element(element) # Remove attributes in the QuestionPy namespace for element in _assert_element_list(self._xpath("//*")): @@ -413,9 +460,7 @@ def _clean_up(self) -> None: # Remove comments for comment in _assert_element_list(self._xpath("//comment()")): - parent = comment.getparent() - if parent is not None: - parent.remove(comment) + _remove_element(comment) # Remove namespaces from all elements. (QPy elements should all have been consumed previously anyhow.) for element in _assert_element_list(self._xpath("//*")): @@ -458,6 +503,63 @@ def _add_styles(self) -> None: for element in _assert_element_list(self._xpath("//xhtml:input[@type = 'checkbox' or @type = 'radio']")): self._add_class_names(element, "qpy-input") + def _validate_format_float_element(self, element: etree._Element) -> tuple[float, int | None, str] | None: + """Collects potential render errors for the `qpy:format-float` element. + + Returns: + If no error occurred, a tuple consisting of the float value, the precision, and the thousands separator + value. Else, None. + """ + parsing_error = False + + if element.text is None: + # TODO: Show an error message? + return None + + # As PHP parses floats and ints differently then Python, we make the format more strict. + # E.g. parsing '20_000' or '1d1' results in: + # Python -> 20000 Error + # PHP -> 20 1 + if re.match(r"^\s*((\d+\.?\d*)|(\d*\.\d+)|(\d+e\d+))\s*$", element.text) is None: + float_error = ConversionError(element=element, value=element.text, to_type=float) + self._errors.insert(float_error) + parsing_error = True + + precision_text: str | None = element.get("precision") + precision = None + if precision_text is not None: + if not precision_text or (precision_text[0] == "-" and precision_text[1:].isnumeric()): + # Empty or negative value. + precision_error = InvalidAttributeValueError( + element=element, attribute="precision", value=precision_text + ) + self._errors.insert(precision_error) + parsing_error = True + elif precision_text.isnumeric(): + # We disallow the usage of underscores to separate numeric literals, see above for an explanation. + precision = int(precision_text) + else: + conversion_error = ConversionError( + element=element, value=precision_text, to_type=int, attribute="precision" + ) + self._errors.insert(conversion_error) + parsing_error = True + else: + precision = None + + thousands_sep_attr = element.get("thousands-separator", "no") + expected = ("yes", "no") + if thousands_sep_attr not in expected: + thousands_sep_error = InvalidAttributeValueError( + element=element, attribute="thousands-separator", value=thousands_sep_attr, expected=expected + ) + self._errors.insert(thousands_sep_error) + parsing_error = True + + if parsing_error: + return None + return float(element.text), precision, thousands_sep_attr + def _format_floats(self) -> None: """Handles `qpy:format-float`. @@ -467,19 +569,20 @@ def _format_floats(self) -> None: decimal_sep = "." # Placeholder for decimal separator for element in _assert_element_list(self._xpath("//qpy:format-float")): - if element.text is None: + data = self._validate_format_float_element(element) + if data is None: + _remove_element(element) continue - float_val = float(element.text) - precision = int(element.get("precision", -1)) + float_val, precision, thousands_sep_attr = data + strip_zeroes = "strip-zeros" in element.attrib - formatted_str = f"{float_val:.{precision}f}" if precision >= 0 else str(float_val) + formatted_str = f"{float_val:.{precision}f}" if precision is not None else str(float_val) if strip_zeroes: formatted_str = formatted_str.rstrip("0").rstrip(decimal_sep) if "." in formatted_str else formatted_str - thousands_sep_attr = element.get("thousands-separator", "no") if thousands_sep_attr == "yes": parts = formatted_str.split(decimal_sep) integral_part = parts[0] @@ -495,7 +598,7 @@ def _format_floats(self) -> None: parent = element.getparent() new_text.tail = element.tail - if parent: + if parent is not None: parent.insert(parent.index(element), new_text) parent.remove(element) diff --git a/questionpy_sdk/webserver/question_ui/errors.py b/questionpy_sdk/webserver/question_ui/errors.py index d0f9d165..83ff44bd 100644 --- a/questionpy_sdk/webserver/question_ui/errors.py +++ b/questionpy_sdk/webserver/question_ui/errors.py @@ -5,8 +5,8 @@ import logging from abc import ABC, abstractmethod from bisect import insort -from collections.abc import Iterable, Iterator, Sized -from dataclasses import dataclass +from collections.abc import Collection, Iterable, Iterator, Mapping, Sized +from dataclasses import dataclass, field from operator import attrgetter from typing import TypeAlias @@ -15,10 +15,23 @@ _log = logging.getLogger(__name__) +def _comma_separated_values(values: Collection[str], opening: str, closing: str) -> str: + *values, last_value = values + last_value = f"{opening}{last_value}{closing}" + if not values: + return last_value + + return opening + f"{closing}, {opening}".join(values) + f"{closing} and {last_value}" + + @dataclass(frozen=True) class RenderError(ABC): """Represents a generic error which occurred during rendering.""" + @property + def type(self) -> str: + return self.__class__.__name__ + @property @abstractmethod def line(self) -> int | None: @@ -41,10 +54,36 @@ def html_message(self) -> str: @dataclass(frozen=True) class RenderElementError(RenderError, ABC): + """Represents a generic element error which occurred during rendering.""" + element: etree._Element + template: str + kwargs: Mapping[str, str | Collection[str]] = field(default_factory=dict) + + def _message(self, *, as_html: bool) -> str: + (opening, closing) = ("", "") if as_html else ("'", "'") + kwargs = {"element": f"{opening}{self.element_representation}{closing}"} + + for key, values in self.kwargs.items(): + collection = {values} if isinstance(values, str) else values + kwargs[key] = _comma_separated_values(collection, opening, closing) + + return self.template.format(**kwargs) + + @property + def message(self) -> str: + return self._message(as_html=False) + + @property + def html_message(self) -> str: + return self._message(as_html=True) @property def element_representation(self) -> str: + # Return the whole element if it is a PI. + if isinstance(self.element, etree._ProcessingInstruction): + return str(self.element) + # Create the prefix of an element. We do not want to keep 'html' as a prefix. prefix = f"{self.element.prefix}:" if self.element.prefix and self.element.prefix != "html" else "" return prefix + etree.QName(self.element).localname @@ -57,36 +96,84 @@ def line(self) -> int | None: @dataclass(frozen=True) class InvalidAttributeValueError(RenderElementError): - """Invalid attribute value.""" + """Invalid attribute value(s).""" + + def __init__( + self, + element: etree._Element, + attribute: str, + value: str | Collection[str], + expected: Collection[str] | None = None, + ): + kwargs = {"value": value, "attribute": attribute} + expected_str = "" + if expected: + kwargs["expected"] = expected + expected_str = " Expected values are {expected}." + + s = "" if isinstance(value, str) or len(value) <= 1 else "" + super().__init__( + element=element, + template=f"Invalid value{s} {{value}} for attribute {{attribute}} on element {{element}}.{expected_str}", + kwargs=kwargs, + ) - attribute: str - value: str - expected: Iterable[str] | None = None - def _message(self, *, as_html: bool) -> str: - if as_html: - (opening, closing) = ("", "") - value = html.escape(self.value) - else: - (opening, closing) = ("'", "'") - value = self.value - - expected = "" - if self.expected: - expected = f" Expected one of [{opening}" + f"{closing}, {opening}".join(self.expected) + f"{closing}]." - - return ( - f"Invalid value {opening}{value}{closing} for attribute {opening}{self.attribute}{closing} " - f"on element {opening}{self.element_representation}{closing}.{expected}" +@dataclass(frozen=True) +class ConversionError(RenderElementError): + """Could not convert a value to another type.""" + + def __init__(self, element: etree._Element, value: str, to_type: type, attribute: str | None = None): + kwargs = {"value": value, "type": to_type.__name__} + + in_attribute = "" + if attribute: + kwargs["attribute"] = attribute + in_attribute = " in attribute {attribute}" + + template = f"Unable to convert {{value}} to {{type}}{in_attribute} at element {{element}}." + super().__init__(element=element, template=template, kwargs=kwargs) + + +@dataclass(frozen=True) +class PlaceholderReferenceError(RenderElementError): + """A placeholder was referenced which was not provided.""" + + def __init__(self, element: etree._Element, placeholder: str, available: Collection[str]): + provided = "No placeholders were provided." + if len(available) == 1: + provided = "There is only one provided placeholder: {available}." + elif len(available) > 1: + provided = "These are the provided placeholders: {available}." + + super().__init__( + element=element, + template=f"Referenced placeholder {{placeholder}} was not found. {provided}", + kwargs={"placeholder": placeholder, "available": available}, ) - @property - def message(self) -> str: - return self._message(as_html=False) - @property - def html_message(self) -> str: - return self._message(as_html=True) +@dataclass(frozen=True) +class InvalidCleanOptionError(RenderElementError): + """Invalid clean option.""" + + def __init__(self, element: etree._Element, option: str, expected: Collection[str]): + super().__init__( + element=element, + template="Invalid cleaning option {option}. Available options are {expected}.", + kwargs={"option": option, "expected": expected}, + ) + + +@dataclass(frozen=True) +class UnknownElementError(RenderElementError): + """Unknown element with qpy-namespace.""" + + def __init__(self, element: etree._Element): + super().__init__( + element=element, + template="Unknown element {element}.", + ) @dataclass(frozen=True) @@ -106,11 +193,11 @@ def order(self) -> int: @property def message(self) -> str: - return f"Syntax error: {self.error.msg}" + return f"{self.error.msg}" @property def html_message(self) -> str: - return f"Invalid syntax: {html.escape(self.error.msg)}" + return f"{html.escape(self.error.msg)}" class RenderErrorCollection(Iterable, Sized): @@ -143,5 +230,7 @@ def log_render_errors(render_errors: RenderErrorCollections) -> None: errors_string = "" for error in errors: line = f"Line {error.line}: " if error.line else "" - errors_string += f"\n\t- {line}{error.message}" - _log.warning(f"{len(errors)} error(s) occurred while rendering {section}:{errors_string}") + errors_string += f"\n\t- {line}{error.type} - {error.message}" + error_count = len(errors) + s = "s" if error_count > 1 else "" + _log.warning(f"{error_count} error{s} occurred while rendering {section}:{errors_string}") diff --git a/questionpy_sdk/webserver/static/styles.css b/questionpy_sdk/webserver/static/styles.css index 517673a9..e3e6f556 100644 --- a/questionpy_sdk/webserver/static/styles.css +++ b/questionpy_sdk/webserver/static/styles.css @@ -260,23 +260,36 @@ fieldset { text-decoration: underline; } -.container-render-errors table tr td:first-child, -.container-render-errors table tr th:first-child:not([scope="rowgroup"]) { - border-left: 0; - padding-right: 0.5rem; -} - .container-render-errors table tr td:first-child { vertical-align: top; text-align: right; font-variant-numeric: lining-nums tabular-nums; } +.container-render-errors table tr td:first-child, +.container-render-errors table tr th:first-child:not([scope="rowgroup"]) { + border-left: 0; + padding-right: 0.5rem; +} + .container-render-errors table tr td:last-child, .container-render-errors table tr th:last-child:not([scope="rowgroup"]) { border-right: 0; padding-left: 0.5rem; +} + +.container-render-errors table tr td:not(:first-child):not(:last-child), +.container-render-errors table tr th:not(:first-child):not(:last-child):not([scope="rowgroup"]) { + padding: 0 0.5rem; + vertical-align: top; +} +/* If the screen is to small, hide every column expect for the first and last (-> line and error message). */ +@media only screen and (max-width: 60rem) { + .container-render-errors table tr td:not(:first-child):not(:last-child), + .container-render-errors table tr th:not(:first-child):not(:last-child):not([scope="rowgroup"]) { + display: none; + } } .container-render-errors table tbody th { diff --git a/questionpy_sdk/webserver/templates/attempt.html.jinja2 b/questionpy_sdk/webserver/templates/attempt.html.jinja2 index c72a3918..dbff95df 100644 --- a/questionpy_sdk/webserver/templates/attempt.html.jinja2 +++ b/questionpy_sdk/webserver/templates/attempt.html.jinja2 @@ -22,11 +22,13 @@ Line + Type Error-Message {% for error in errors %} {{ error.line or '' }} + {{ error.type }} {{ error.html_message|safe }} {% endfor %} diff --git a/ruff_defaults.toml b/ruff_defaults.toml index 4046cd80..cc9831c6 100644 --- a/ruff_defaults.toml +++ b/ruff_defaults.toml @@ -106,6 +106,7 @@ pydocstyle = { convention = "google" } [lint.per-file-ignores] "**/scripts/*" = ["INP001", "T201"] "**/tests/**/*" = ["PLC1901", "PLC2701", "PLR2004", "S", "TID252", "FBT"] +"**/question_ui/errors.py" = ["RUF027"] # Allow f-string without an `f` prefix for our custom error formatter. [lint.flake8-builtins] # help: (guess it's ok since built-in `help()` is for interactive use and no collisions are expected) diff --git a/tests/questionpy_sdk/webserver/test_data/faulty.xhtml b/tests/questionpy_sdk/webserver/test_data/faulty.xhtml index f2bd1372..16f4d511 100644 --- a/tests/questionpy_sdk/webserver/test_data/faulty.xhtml +++ b/tests/questionpy_sdk/webserver/test_data/faulty.xhtml @@ -1,5 +1,14 @@
Unknown feedback type. Missing attribute value. - Unknown feedback type. + +
Missing placeholder.
+
Unknown roles.
+ Unknown value. +
+ +
diff --git a/tests/questionpy_sdk/webserver/test_question_ui.py b/tests/questionpy_sdk/webserver/test_question_ui.py index 0050727d..fe882446 100644 --- a/tests/questionpy_sdk/webserver/test_question_ui.py +++ b/tests/questionpy_sdk/webserver/test_question_ui.py @@ -1,9 +1,8 @@ # This file is part of the QuestionPy SDK. (https://questionpy.org) # The QuestionPy SDK is free software released under terms of the MIT license. See LICENSE.md. # (c) Technische Universität Berlin, innoCampus -import re from importlib import resources -from typing import TYPE_CHECKING, Any +from typing import Any import pytest @@ -16,11 +15,15 @@ QuestionUIRenderer, XMLSyntaxError, ) +from questionpy_sdk.webserver.question_ui.errors import ( + ConversionError, + InvalidCleanOptionError, + PlaceholderReferenceError, + RenderError, + UnknownElementError, +) from tests.questionpy_sdk.webserver.conftest import assert_html_is_equal -if TYPE_CHECKING: - from questionpy_sdk.webserver.question_ui.errors import RenderError - @pytest.fixture def xml_content(request: pytest.FixtureRequest) -> str | None: @@ -320,7 +323,6 @@ def test_should_replace_shuffled_index(renderer: QuestionUIRenderer) -> None: @pytest.mark.render_params( xml="""
- Text Content Normal Content @@ -362,26 +364,30 @@ def test_errors_should_be_collected(renderer: QuestionUIRenderer) -> None: expected = """
Missing attribute value. +
Missing placeholder.
+
""" html, errors = renderer.render() - assert len(errors) == 3 - - invalid_attribute_error_message = re.compile( - r"Invalid value .+unknown.+ for attribute .+qpy:feedback.+ on element " r".+span.+. Expected one of \[.+]." - ) - expected_errors: list[tuple[type[RenderError], re.Pattern, int]] = [ - # Even though the syntax error occurs after an invalid attribute value error, it should be listed first. - (XMLSyntaxError, re.compile(".+"), 3), - (InvalidAttributeValueError, invalid_attribute_error_message, 2), - (InvalidAttributeValueError, invalid_attribute_error_message, 4), + assert len(errors) == 10 + + expected_errors: list[tuple[type[RenderError], int]] = [ + # Even though the syntax error occurs after all the other errors, it should be listed first. + (XMLSyntaxError, 3), + (InvalidAttributeValueError, 2), + (UnknownElementError, 4), + (InvalidCleanOptionError, 5), + (PlaceholderReferenceError, 5), + (InvalidAttributeValueError, 6), + (ConversionError, 7), + (ConversionError, 7), + (InvalidAttributeValueError, 7), + (InvalidAttributeValueError, 11), ] for actual_error, expected_error in zip(errors, expected_errors, strict=True): - error_type, message, line = expected_error + error_type, line = expected_error assert actual_error.line == line assert isinstance(actual_error, error_type) - assert message.match(actual_error.message) is not None - assert message.match(actual_error.html_message) is not None assert_html_is_equal(html, expected)