Skip to content
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.

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

feat: check for duplicate input names #155

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
41 changes: 41 additions & 0 deletions questionpy_sdk/webserver/question_ui/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from questionpy_common.api.attempt import DisplayRole
from questionpy_sdk.webserver.question_ui.errors import (
ConversionError,
DuplicateNameError,
ExpectedAncestorError,
InvalidAttributeValueError,
InvalidCleanOptionError,
Expand Down Expand Up @@ -594,6 +595,7 @@ def collect(self) -> RenderErrorCollection:
self._validate_shuffle_contents_and_shuffled_index()
self._validate_format_floats()
self._look_for_unknown_qpy_elements_and_attributes()
self._check_input_names()

return self.errors

Expand Down Expand Up @@ -740,3 +742,42 @@ def _look_for_unknown_qpy_elements_and_attributes(self) -> None:
if unknown_attributes:
unknown_attribute_error = UnknownAttributeError(element=element, attributes=unknown_attributes)
self.errors.insert(unknown_attribute_error)

def _check_input_names(self) -> None:
"""Check if there are only valid input names.

Duplicate input names are only allowed for `checkbox` and `radio` elements, but only when their types match and
they have different values.
"""
# Maps element name to the reference element and the values.
name_map: dict[str, tuple[etree._Element, set[str]]] = {}

for current_element in _assert_element_list(
self._xpath("(//xhtml:button | //xhtml:input | //xhtml:select | //xhtml:textarea)[@name]")
):
# Get name, type, and value of the current element.
name = str(current_element.attrib["name"])
current_type = current_element.get("type", "text")
current_value = current_element.get("value", "on")

if name not in name_map:
# This name has not been used yet by other elements.
name_map[name] = (current_element, {current_value})
continue

# Get type and values of the other element(s) with the same name.
other_element, values = name_map[name]
other_type = other_element.get("type", "text")

if current_type not in {"checkbox", "radio"}:
Copy link
Member

Choose a reason for hiding this comment

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

So oder so nicht merkbar, aber aus akademischer Sicht: Für dieses Pattern ein Set zu verwenden hat ziemlich sicher mehr Overhead als du durch den O(1) lookup gewinnst. Für das Set müssen "checkbox" und "radio" z.B. erst gehasht werden.

# Duplicate names are not allowed for other elements.
error = DuplicateNameError(element=current_element, name=name, other_element=other_element)
self.errors.insert(error)
continue

# Check that the types match and the value is unique.
if other_type != current_type or current_value in values:
error = DuplicateNameError(element=current_element, name=name, other_element=other_element)
self.errors.insert(error)
else:
values.add(current_value)
38 changes: 27 additions & 11 deletions questionpy_sdk/webserver/question_ui/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,16 @@ def _format_human_readable_list(values: Collection[str], opening: str, closing:
return opening + f"{closing}, {opening}".join(values) + f"{closing} and {last_value}"


def _element_representation(element: etree._Element) -> str:
# Return the whole element if it is a PI.
if isinstance(element, etree._ProcessingInstruction):
return str(element)

# Create the prefix of an element. We do not want to keep 'html' as a prefix.
prefix = f"{element.prefix}:" if element.prefix and element.prefix != "html" else ""
return prefix + etree.QName(element).localname


@dataclass(frozen=True)
class RenderError(ABC):
"""Represents a generic error which occurred during rendering."""
Expand Down Expand Up @@ -76,7 +86,7 @@ class RenderElementError(RenderError, ABC):

def _message(self, *, as_html: bool) -> str:
(opening, closing) = ("<code>", "</code>") if as_html else ("'", "'")
template_kwargs = {"element": f"{opening}{self.element_representation}{closing}"}
template_kwargs = {"element": f"{opening}{_element_representation(self.element)}{closing}"}

for key, values in self.template_kwargs.items():
collection = {values} if isinstance(values, str) else values
Expand All @@ -92,16 +102,6 @@ def message(self) -> str:
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

@property
def line(self) -> int | None:
"""Original line number as found by the parser or None if unknown."""
Expand Down Expand Up @@ -234,6 +234,22 @@ def __init__(self, element: etree._Element, attributes: Collection[str]):
)


@dataclass(frozen=True)
class DuplicateNameError(RenderElementError):
"""Invalid duplicate input name."""

def __init__(self, element: etree._Element, name: str, other_element: etree._Element):
super().__init__(
element=element,
template="{element} should not have the same name ({name}) like {other_element} at line {line}.",
template_kwargs={
"name": name,
"other_element": _element_representation(other_element),
"line": str(other_element.sourceline or "?"),
},
)


@dataclass(frozen=True)
class XMLSyntaxError(RenderError):
"""Syntax error while parsing the XML."""
Expand Down
2 changes: 2 additions & 0 deletions tests/questionpy_sdk/webserver/test_data/faulty.xhtml
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,7 @@
<div>Empty placeholder.<?p ?></div>
<qpy:shuffled-index/> <!-- Not inside a qpy:shuffle-contents element. -->
<div qpy:unknown-attribute="">Unknown attribute.</div>
<input type="checkbox" name="duplicate"/>
<input type="radio" name="duplicate"/>
<span no-attribute-value>Missing attribute value.</span>
</div>
4 changes: 2 additions & 2 deletions tests/questionpy_sdk/webserver/test_data/input-values.xhtml
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@

<input type="hidden" name="my_hidden" value="original"/>

<input name="my_button" type="button" value="value1"/>
<input name="my_button" type="button" value="value2"/>
<input name="button_1" type="button" value="value1"/>
<input name="button_2" type="button" value="value2"/>

<textarea name="my_textarea">original</textarea>
</div>
14 changes: 9 additions & 5 deletions tests/questionpy_sdk/webserver/test_question_ui.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
)
from questionpy_sdk.webserver.question_ui.errors import (
ConversionError,
DuplicateNameError,
ExpectedAncestorError,
InvalidAttributeValueError,
InvalidCleanOptionError,
Expand Down Expand Up @@ -210,8 +211,8 @@ def test_should_set_input_values(renderer: QuestionUIRenderer) -> None:

<input class="form-control qpy-input" type="hidden" name="my_hidden" value="new"/>

<input class="btn btn-primary qpy-input" name="my_button" type="button" value="value1"/>
<input class="btn btn-primary qpy-input" name="my_button" type="button" value="value2"/>
<input class="btn btn-primary qpy-input" name="button_1" type="button" value="value1"/>
<input class="btn btn-primary qpy-input" name="button_2" type="button" value="value2"/>

<textarea class="form-control qpy-input" name="my_textarea">new</textarea>
</div>
Expand Down Expand Up @@ -242,8 +243,8 @@ def test_should_disable_inputs(renderer: QuestionUIRenderer) -> None:

<input class="form-control qpy-input" type="hidden" name="my_hidden" value="original" disabled="disabled"/>

<input class="btn btn-primary qpy-input" name="my_button" type="button" value="value1" disabled="disabled"/>
<input class="btn btn-primary qpy-input" name="my_button" type="button" value="value2" disabled="disabled"/>
<input class="btn btn-primary qpy-input" name="button_1" type="button" value="value1" disabled="disabled"/>
<input class="btn btn-primary qpy-input" name="button_2" type="button" value="value2" disabled="disabled"/>

<textarea class="form-control qpy-input" name="my_textarea" disabled="disabled">original</textarea>
</div>
Expand Down Expand Up @@ -389,14 +390,16 @@ def test_errors_should_be_collected(renderer: QuestionUIRenderer) -> None:
<div>Missing placeholder.</div>
<div>Empty placeholder.</div>
<div>Unknown attribute.</div>
<input type="checkbox" name="duplicate" class="qpy-input"></input>
<input type="radio" name="duplicate" class="qpy-input"></input>
<span>Missing attribute value.</span>
</div>
""" # noqa: E501
html, errors = renderer.render()

expected_errors: list[tuple[type[RenderError], int]] = [
# Even though the syntax error occurs after all the other errors, it should be listed first.
(XMLSyntaxError, 17),
(XMLSyntaxError, 19),
(InvalidAttributeValueError, 2),
(UnknownElementError, 3),
(InvalidAttributeValueError, 4),
Expand All @@ -410,6 +413,7 @@ def test_errors_should_be_collected(renderer: QuestionUIRenderer) -> None:
(PlaceholderReferenceError, 14),
(ExpectedAncestorError, 15),
(UnknownAttributeError, 16),
(DuplicateNameError, 18),
]

assert len(errors) == len(expected_errors)
Expand Down