Merge pull request #730 from Textualize/validate-id

validate identifiers

sandbox/will/spacing.py

@@ -9,7 +9,7 @@ class Clickable(Static):
 
 class SpacingApp(App):
     def compose(self):
-        yield Clickable()
+        yield Static(id="2332")
 
 
 app = SpacingApp(css_path="spacing.css")

src/textual/css/tokenize.py

@@ -26,6 +26,8 @@ TOKEN = "[a-zA-Z][a-zA-Z0-9_-]*"
 STRING = r"\".*?\""
 VARIABLE_REF = r"\$[a-zA-Z0-9_\-]+"
 
+IDENTIFIER = r"[a-zA-Z_\-][a-zA-Z0-9_\-]*"
+
 # Values permitted in variable and rule declarations.
 DECLARATION_VALUES = {
     "scalar": SCALAR,
@@ -44,8 +46,8 @@ DECLARATION_VALUES = {
 expect_root_scope = Expect(
     whitespace=r"\s+",
     comment_start=COMMENT_START,
-    selector_start_id=r"\#[a-zA-Z_\-][a-zA-Z0-9_\-]*",
-    selector_start_class=r"\.[a-zA-Z_\-][a-zA-Z0-9_\-]*",
+    selector_start_id=r"\#" + IDENTIFIER,
+    selector_start_class=r"\." + IDENTIFIER,
     selector_start_universal=r"\*",
     selector_start=r"[a-zA-Z_\-]+",
     variable_name=rf"{VARIABLE_REF}:",

src/textual/dom.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 from inspect import getfile
+import re
 from typing import (
     cast,
     ClassVar,
@@ -27,6 +28,7 @@ from .css.constants import VALID_DISPLAY, VALID_VISIBILITY
 from .css.errors import StyleValueError, DeclarationError
 from .css.parse import parse_declarations
 from .css.styles import Styles, RenderStyles
+from .css.tokenize import IDENTIFIER
 from .css.query import NoMatchingNodesError
 from .message_pump import MessagePump
 from .timer import Timer
@@ -38,6 +40,32 @@ if TYPE_CHECKING:
     from .widget import Widget
 
 
+_re_identifier = re.compile(IDENTIFIER)
+
+
+class BadIdentifier(Exception):
+    """raised by check_identifiers."""
+
+
+def check_identifiers(description: str, *names: str) -> None:
+    """Validate identifier and raise an error if it fails.
+
+    Args:
+        description (str): Description of where identifier is used for error message.
+        names (list[str]): Identifiers to check.
+
+    Returns:
+        bool: True if the name is valid.
+    """
+    match = _re_identifier.match
+    for name in names:
+        if match(name) is None:
+            raise BadIdentifier(
+                f"{name!r} is an invalid {description}; "
+                "identifiers must contain only letters, numbers, underscores, or hyphens, and must not begin with a number."
+            )
+
+
 class DOMError(Exception):
     pass
 
@@ -75,9 +103,16 @@ class DOMNode(MessagePump):
         id: str | None = None,
         classes: str | None = None,
     ) -> None:
+        self._classes = set()
         self._name = name
-        self._id = id
-        self._classes: set[str] = set() if classes is None else set(classes.split())
+        self._id = None
+        if id is not None:
+            self.id = id
+
+        _classes = classes.split() if classes else []
+        check_identifiers("class name", *_classes)
+        self._classes.update(_classes)
+
         self.children = NodeList()
         self._css_styles: Styles = Styles(self)
         self._inline_styles: Styles = Styles(self)
@@ -248,6 +283,8 @@ class DOMNode(MessagePump):
             ValueError: If the ID has already been set.
 
         """
+        check_identifiers("id", new_id)
+
         if self._id is not None:
             raise ValueError(
                 f"Node 'id' attribute may not be changed once set (current id={self._id!r})"
@@ -723,6 +760,7 @@ class DOMNode(MessagePump):
             *class_names (str): CSS class names to add.
 
         """
+        check_identifiers("class name", *class_names)
         old_classes = self._classes.copy()
         self._classes.update(class_names)
         if old_classes == self._classes:
@@ -739,6 +777,7 @@ class DOMNode(MessagePump):
             *class_names (str): CSS class names to remove.
 
         """
+        check_identifiers("class name", *class_names)
         old_classes = self._classes.copy()
         self._classes.difference_update(class_names)
         if old_classes == self._classes:
@@ -755,6 +794,7 @@ class DOMNode(MessagePump):
             *class_names (str): CSS class names to toggle.
 
         """
+        check_identifiers("class name", *class_names)
         old_classes = self._classes.copy()
         self._classes.symmetric_difference_update(class_names)
         if old_classes == self._classes:

tests/test_dom.py

@@ -2,7 +2,7 @@ import pytest
 
 from textual.css.errors import StyleValueError
 from textual.css.query import NoMatchingNodesError
-from textual.dom import DOMNode
+from textual.dom import DOMNode, BadIdentifier
 
 
 def test_display_default():
@@ -55,3 +55,23 @@ def test_get_child_no_matching_child(parent):
 def test_get_child_only_immediate_descendents(parent):
     with pytest.raises(NoMatchingNodesError):
         parent.get_child(id="grandchild1")
+
+
+def test_validate():
+    with pytest.raises(BadIdentifier):
+        DOMNode(id="23")
+    with pytest.raises(BadIdentifier):
+        DOMNode(id=".3")
+    with pytest.raises(BadIdentifier):
+        DOMNode(classes="+2323")
+    with pytest.raises(BadIdentifier):
+        DOMNode(classes="foo 22")
+
+    node = DOMNode()
+    node.add_class("foo")
+    with pytest.raises(BadIdentifier):
+        node.add_class("1")
+    with pytest.raises(BadIdentifier):
+        node.remove_class("1")
+    with pytest.raises(BadIdentifier):
+        node.toggle_class("1")