From 300578fe808a6a58deb5cb58b63704e87032a24f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 16 Jan 2024 17:29:18 +0000 Subject: [PATCH 1/6] Add function that validates identifiers. --- src/textual/css/errors.py | 4 ++++ src/textual/css/parse.py | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/src/textual/css/errors.py b/src/textual/css/errors.py index 815b11394e..79873692ee 100644 --- a/src/textual/css/errors.py +++ b/src/textual/css/errors.py @@ -48,3 +48,7 @@ def __rich_console__( class StylesheetError(Exception): pass + + +class InvalidIDError(Exception): + """For when invalid identifiers are used.""" diff --git a/src/textual/css/parse.py b/src/textual/css/parse.py index b9e59d94ba..28aa3e26f5 100644 --- a/src/textual/css/parse.py +++ b/src/textual/css/parse.py @@ -1,13 +1,14 @@ from __future__ import annotations import dataclasses +import re from functools import lru_cache from typing import Iterable, Iterator, NoReturn from ..suggestions import get_suggestion from ._help_renderables import HelpText from ._styles_builder import DeclarationError, StylesBuilder -from .errors import UnresolvedVariableError +from .errors import InvalidIDError, UnresolvedVariableError from .model import ( CombinatorType, Declaration, @@ -17,7 +18,13 @@ SelectorType, ) from .styles import Styles -from .tokenize import Token, tokenize, tokenize_declarations, tokenize_values +from .tokenize import ( + IDENTIFIER, + Token, + tokenize, + tokenize_declarations, + tokenize_values, +) from .tokenizer import EOFError, ReferencedBy from .types import CSSLocation, Specificity3 @@ -52,6 +59,27 @@ def _add_specificity( return (a1 + a2, b1 + b2, c1 + c2) +_IDENTIFIER_REGEX = re.compile(f"^{IDENTIFIER}$") +"""Regular expression that matches strings that contain valid identifiers.""" + + +def validate_identifier(identifier: str) -> str: + """Determines whether the given identifier is valid or not. + + Args: + identifier: Identifier to validate. + + Returns: + The identifier, if it is valid. + + Raises: + InvalidIDError: If the identifier is invalid. + """ + if _IDENTIFIER_REGEX.match(identifier) is None: + raise InvalidIDError(f"{identifier!r} is not a valid identifier.") + return identifier + + @lru_cache(maxsize=1024) def parse_selectors(css_selectors: str) -> tuple[SelectorSet, ...]: if not css_selectors.strip(): From 5f0af2da473a50632e5da8453509edb332adb2aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 16 Jan 2024 17:30:14 +0000 Subject: [PATCH 2/6] Validate IDs when creating DOM nodes. --- src/textual/dom.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/textual/dom.py b/src/textual/dom.py index 461e9acea7..c5124ae9c8 100644 --- a/src/textual/dom.py +++ b/src/textual/dom.py @@ -38,7 +38,7 @@ from .css._error_tools import friendly_list from .css.constants import VALID_DISPLAY, VALID_VISIBILITY from .css.errors import DeclarationError, StyleValueError -from .css.parse import parse_declarations +from .css.parse import parse_declarations, validate_identifier from .css.styles import RenderStyles, Styles from .css.tokenize import IDENTIFIER from .message_pump import MessagePump @@ -170,7 +170,7 @@ def __init__( self._name = name self._id = None if id is not None: - self.id = id + self.id = validate_identifier(id) _classes = classes.split() if classes else [] check_identifiers("class name", *_classes) From 2d0591af0096b683f9624f0e9df06cb7d375822b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 16 Jan 2024 17:30:24 +0000 Subject: [PATCH 3/6] Add regression tests. --- CHANGELOG.md | 1 + tests/css/test_parse.py | 41 +++++++++++++++++++++++++++++++++++++++-- tests/test_dom.py | 23 ++++++++++++++++++++++- 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3f838be7d..f16ac4dbe3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `SelectionList` option IDs are usable as soon as the widget is instantiated https://github.com/Textualize/textual/issues/3903 - Fix issue with `Strip.crop` when crop window start aligned with strip end https://github.com/Textualize/textual/pull/3998 - Fixed Strip.crop_extend https://github.com/Textualize/textual/pull/4011 +- Widgets could be created with IDs that you couldn't query back https://github.com/Textualize/textual/issues/3954 ## [0.47.1] - 2023-01-05 diff --git a/tests/css/test_parse.py b/tests/css/test_parse.py index febe4f06bb..2d6aeef002 100644 --- a/tests/css/test_parse.py +++ b/tests/css/test_parse.py @@ -3,8 +3,8 @@ import pytest from textual.color import Color -from textual.css.errors import UnresolvedVariableError -from textual.css.parse import substitute_references +from textual.css.errors import InvalidIDError, UnresolvedVariableError +from textual.css.parse import substitute_references, validate_identifier from textual.css.scalar import Scalar, Unit from textual.css.stylesheet import Stylesheet, StylesheetParseError from textual.css.tokenize import tokenize @@ -1272,3 +1272,40 @@ def test_parse_bad_pseudo_selector_with_suggestion(): stylesheet.parse() assert error.value.start == (2, 7) + + +@pytest.mark.parametrize( + "identifier", + [ + "the", + "quick", + "brown", + "f0x", + "jump5", + "ov3r", + "_the__", + "l4zy_", + "_d0g", + ], +) +def test_identifier_validation_passes(identifier: str): + assert validate_identifier(identifier) == identifier + + +@pytest.mark.parametrize( + "identifier", + [ + " the", + "quick ", + "bro wn", + "f0x&", + "+jump5", + "0v3r", + "—the", + "läzy_", + "_d0g!", + ], +) +def test_identifier_validation_fails(identifier: str): + with pytest.raises(InvalidIDError): + validate_identifier(identifier) diff --git a/tests/test_dom.py b/tests/test_dom.py index 6f06fb8091..d4dc4336dd 100644 --- a/tests/test_dom.py +++ b/tests/test_dom.py @@ -1,6 +1,6 @@ import pytest -from textual.css.errors import StyleValueError +from textual.css.errors import InvalidIDError, StyleValueError from textual.dom import BadIdentifier, DOMNode @@ -259,3 +259,24 @@ def test_walk_children_with_self_breadth(search): ] assert children == ["f", "e", "d", "c", "b", "a"] + + +@pytest.mark.parametrize( + "identifier", + [ + " bad", + " terrible ", + "worse! ", + "&ersand", + "amper&sand", + "ampersand&", + "2_leading_digits", + "água", # water + "cão", # dog + "@'/.23", + ], +) +def test_id_validation(identifier: str): + """Regression test for https://github.com/Textualize/textual/issues/3954.""" + with pytest.raises(InvalidIDError): + DOMNode(id=identifier) From 27408a0ff74086ab28b3dbcaa0d3b829a467922c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 16 Jan 2024 17:40:21 +0000 Subject: [PATCH 4/6] Removes my duplicate work. This reverts commit 300578fe808a6a58deb5cb58b63704e87032a24f and a couple of other things that I really didn't need. --- src/textual/css/errors.py | 4 ---- src/textual/css/parse.py | 32 ++---------------------------- tests/css/test_parse.py | 41 ++------------------------------------- tests/test_dom.py | 2 +- 4 files changed, 5 insertions(+), 74 deletions(-) diff --git a/src/textual/css/errors.py b/src/textual/css/errors.py index 79873692ee..815b11394e 100644 --- a/src/textual/css/errors.py +++ b/src/textual/css/errors.py @@ -48,7 +48,3 @@ def __rich_console__( class StylesheetError(Exception): pass - - -class InvalidIDError(Exception): - """For when invalid identifiers are used.""" diff --git a/src/textual/css/parse.py b/src/textual/css/parse.py index 28aa3e26f5..b9e59d94ba 100644 --- a/src/textual/css/parse.py +++ b/src/textual/css/parse.py @@ -1,14 +1,13 @@ from __future__ import annotations import dataclasses -import re from functools import lru_cache from typing import Iterable, Iterator, NoReturn from ..suggestions import get_suggestion from ._help_renderables import HelpText from ._styles_builder import DeclarationError, StylesBuilder -from .errors import InvalidIDError, UnresolvedVariableError +from .errors import UnresolvedVariableError from .model import ( CombinatorType, Declaration, @@ -18,13 +17,7 @@ SelectorType, ) from .styles import Styles -from .tokenize import ( - IDENTIFIER, - Token, - tokenize, - tokenize_declarations, - tokenize_values, -) +from .tokenize import Token, tokenize, tokenize_declarations, tokenize_values from .tokenizer import EOFError, ReferencedBy from .types import CSSLocation, Specificity3 @@ -59,27 +52,6 @@ def _add_specificity( return (a1 + a2, b1 + b2, c1 + c2) -_IDENTIFIER_REGEX = re.compile(f"^{IDENTIFIER}$") -"""Regular expression that matches strings that contain valid identifiers.""" - - -def validate_identifier(identifier: str) -> str: - """Determines whether the given identifier is valid or not. - - Args: - identifier: Identifier to validate. - - Returns: - The identifier, if it is valid. - - Raises: - InvalidIDError: If the identifier is invalid. - """ - if _IDENTIFIER_REGEX.match(identifier) is None: - raise InvalidIDError(f"{identifier!r} is not a valid identifier.") - return identifier - - @lru_cache(maxsize=1024) def parse_selectors(css_selectors: str) -> tuple[SelectorSet, ...]: if not css_selectors.strip(): diff --git a/tests/css/test_parse.py b/tests/css/test_parse.py index 2d6aeef002..febe4f06bb 100644 --- a/tests/css/test_parse.py +++ b/tests/css/test_parse.py @@ -3,8 +3,8 @@ import pytest from textual.color import Color -from textual.css.errors import InvalidIDError, UnresolvedVariableError -from textual.css.parse import substitute_references, validate_identifier +from textual.css.errors import UnresolvedVariableError +from textual.css.parse import substitute_references from textual.css.scalar import Scalar, Unit from textual.css.stylesheet import Stylesheet, StylesheetParseError from textual.css.tokenize import tokenize @@ -1272,40 +1272,3 @@ def test_parse_bad_pseudo_selector_with_suggestion(): stylesheet.parse() assert error.value.start == (2, 7) - - -@pytest.mark.parametrize( - "identifier", - [ - "the", - "quick", - "brown", - "f0x", - "jump5", - "ov3r", - "_the__", - "l4zy_", - "_d0g", - ], -) -def test_identifier_validation_passes(identifier: str): - assert validate_identifier(identifier) == identifier - - -@pytest.mark.parametrize( - "identifier", - [ - " the", - "quick ", - "bro wn", - "f0x&", - "+jump5", - "0v3r", - "—the", - "läzy_", - "_d0g!", - ], -) -def test_identifier_validation_fails(identifier: str): - with pytest.raises(InvalidIDError): - validate_identifier(identifier) diff --git a/tests/test_dom.py b/tests/test_dom.py index d4dc4336dd..5026f0ab07 100644 --- a/tests/test_dom.py +++ b/tests/test_dom.py @@ -1,6 +1,6 @@ import pytest -from textual.css.errors import InvalidIDError, StyleValueError +from textual.css.errors import StyleValueError from textual.dom import BadIdentifier, DOMNode From 0db8e6d6c06a88f4d523f14ff83b776969513b3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 16 Jan 2024 17:50:47 +0000 Subject: [PATCH 5/6] Fix regular expression check for identifiers. --- CHANGELOG.md | 2 +- src/textual/dom.py | 6 +++--- tests/test_dom.py | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f16ac4dbe3..42dcff82e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `SelectionList` option IDs are usable as soon as the widget is instantiated https://github.com/Textualize/textual/issues/3903 - Fix issue with `Strip.crop` when crop window start aligned with strip end https://github.com/Textualize/textual/pull/3998 - Fixed Strip.crop_extend https://github.com/Textualize/textual/pull/4011 -- Widgets could be created with IDs that you couldn't query back https://github.com/Textualize/textual/issues/3954 +- ID and class validation was too lenient https://github.com/Textualize/textual/issues/3954 ## [0.47.1] - 2023-01-05 diff --git a/src/textual/dom.py b/src/textual/dom.py index c5124ae9c8..92a9e6e18c 100644 --- a/src/textual/dom.py +++ b/src/textual/dom.py @@ -38,7 +38,7 @@ from .css._error_tools import friendly_list from .css.constants import VALID_DISPLAY, VALID_VISIBILITY from .css.errors import DeclarationError, StyleValueError -from .css.parse import parse_declarations, validate_identifier +from .css.parse import parse_declarations from .css.styles import RenderStyles, Styles from .css.tokenize import IDENTIFIER from .message_pump import MessagePump @@ -62,7 +62,7 @@ from typing_extensions import Literal -_re_identifier = re.compile(IDENTIFIER) +_re_identifier = re.compile(f"^{IDENTIFIER}$") WalkMethod: TypeAlias = Literal["depth", "breadth"] @@ -170,7 +170,7 @@ def __init__( self._name = name self._id = None if id is not None: - self.id = validate_identifier(id) + self.id = id _classes = classes.split() if classes else [] check_identifiers("class name", *_classes) diff --git a/tests/test_dom.py b/tests/test_dom.py index 5026f0ab07..ea14a25bfe 100644 --- a/tests/test_dom.py +++ b/tests/test_dom.py @@ -277,6 +277,6 @@ def test_walk_children_with_self_breadth(search): ], ) def test_id_validation(identifier: str): - """Regression test for https://github.com/Textualize/textual/issues/3954.""" - with pytest.raises(InvalidIDError): + """Regression tests for https://github.com/Textualize/textual/issues/3954.""" + with pytest.raises(BadIdentifier): DOMNode(id=identifier) From 98e28f0dcd6debb058c36fe1f269139d93aa8294 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Wed, 17 Jan 2024 11:10:43 +0000 Subject: [PATCH 6/6] Use 're.fullmatch' instead of anchors. Relevant review comment: https://github.com/Textualize/textual/pull/4032#discussion_r1453831415. --- src/textual/dom.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/textual/dom.py b/src/textual/dom.py index 92a9e6e18c..2664881d91 100644 --- a/src/textual/dom.py +++ b/src/textual/dom.py @@ -62,7 +62,7 @@ from typing_extensions import Literal -_re_identifier = re.compile(f"^{IDENTIFIER}$") +_re_identifier = re.compile(IDENTIFIER) WalkMethod: TypeAlias = Literal["depth", "breadth"] @@ -80,7 +80,7 @@ def check_identifiers(description: str, *names: str) -> None: description: Description of where identifier is used for error message. *names: Identifiers to check. """ - match = _re_identifier.match + match = _re_identifier.fullmatch for name in names: if match(name) is None: raise BadIdentifier(