From 3a7ab12ab21667bb0f076e98cf22f033fcb9ab17 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Mon, 14 Sep 2026 07:44:28 -0400 Subject: [PATCH] fix: atomic assignment validation, sentinel-based clearing, strict date validation, race test both orderings - create/set_assignee validate child+rule inside the same serialized transaction as the INSERT/UPDATE (time-of-check/time-of-use race closed; inactive child or deleted rule at assignment time raises ValueError, never a raw FK error) - update() methods take an _UNSET sentinel: omitted = leave alone, explicit None = write SQL NULL (reopen ended rules, clear metadata) - rule create/update validate strict YYYY-MM-DD date shape (lexical CHECK cannot prove calendar validity) - delete/create race test now forces both orderings via a validation hook; SQL-leak guard no longer exempts dao_children.py --- custom_components/nestquest/dao_rules.py | 203 ++++++++++++++++------- tests/test_dao_rules.py | 189 ++++++++++++++++++--- 2 files changed, 308 insertions(+), 84 deletions(-) diff --git a/custom_components/nestquest/dao_rules.py b/custom_components/nestquest/dao_rules.py index eb232b1..51ba457 100644 --- a/custom_components/nestquest/dao_rules.py +++ b/custom_components/nestquest/dao_rules.py @@ -28,11 +28,42 @@ rule cannot appear between check and delete on this connection. """ from __future__ import annotations +import datetime from dataclasses import dataclass from .dao_children import _connection_lock from .db import NestQuestDatabase +#: Sentinel distinguishing "argument omitted" from "explicit SQL NULL" +#: in update methods: passing ``None`` must mean clearing a nullable +#: column, not skipping it. +_UNSET = object() + +#: Date-shape policy: schedule-rule dates are ISO YYYY-MM-DD. The +#: schema's lexical end>=start CHECK cannot prove a value is a real +#: calendar date, so the DAO validates the shape it accepts. +_DATE_FORMAT = "%Y-%m-%d" + + +def _validate_date(value: str, field: str) -> None: + """Raise ValueError unless ``value`` is a YYYY-MM-DD calendar date. + + ``datetime.date.fromisoformat`` accepts ISO dates but is lenient on + some builds (e.g. '2026-9-4'); strptime with an exact format is + strict, and strptime round-trips the parsed value to reject zero + padding ('2026-9-4' parses but its isoformat differs). + """ + try: + parsed = datetime.datetime.strptime(value, _DATE_FORMAT).date() + except (TypeError, ValueError): + raise ValueError( + f"{field} must be an ISO date (YYYY-MM-DD), got {value!r}" + ) from None + if parsed.isoformat() != value: + raise ValueError( + f"{field} must be a strict YYYY-MM-DD date, got {value!r}" + ) + @dataclass(frozen=True) class ScheduleRuleRecord: @@ -124,8 +155,14 @@ class ScheduleRulesDao: Rule-type coherence (a weekly rule needs ``weekday_set``, a yearly rule needs ``month``, etc.) is enforced by the schema - CHECKs and surfaces as :class:`sqlite3.IntegrityError`. + CHECKs and surfaces as :class:`sqlite3.IntegrityError`. Date + shape (strict YYYY-MM-DD) is validated here because the + schema's lexical end>=start CHECK cannot prove a real calendar + date. """ + _validate_date(start_date, "start_date") + if end_date is not None: + _validate_date(end_date, "end_date") result = await self._database.execute( "INSERT INTO schedule_rules (rule_type, interval, weekday_set, " "day_of_month, nth_weekday, month, start_date, end_date) " @@ -157,21 +194,33 @@ class ScheduleRulesDao: self, rule_id: int, *, - rule_type: str | None = None, - interval: int | None = None, - weekday_set: str | None = None, - day_of_month: int | None = None, - nth_weekday: int | None = None, - month: int | None = None, - start_date: str | None = None, - end_date: str | None = None, + rule_type: str | None | object = _UNSET, + interval: int | None | object = _UNSET, + weekday_set: str | None | object = _UNSET, + day_of_month: int | None | object = _UNSET, + nth_weekday: int | None | object = _UNSET, + month: int | None | object = _UNSET, + start_date: str | None | object = _UNSET, + end_date: str | None | object = _UNSET, ) -> int: """Update the given fields; returns rows updated (0 if absent). - Only non-None arguments are written. Editing a rule that a - definition references changes future materializations only — - this method touches no instances (Feature 06/07 separation). + Arguments default to the module sentinel ``_UNSET`` meaning + "leave this column alone"; passing ``None`` explicitly writes + SQL NULL, so callers can reopen an ended rule (``end_date`` + back to NULL) or clear nullable shape fields. Passing + ``None``/a new value for a date field validates its shape. + Editing a rule that a definition references changes future + materializations only — this method touches no instances + (Feature 06/07 separation). """ + date_args = ( + ("start_date", start_date), + ("end_date", end_date), + ) + for field, value in date_args: + if value is not _UNSET and value is not None: + _validate_date(value, field) assignments: list[str] = [] parameters: list[object] = [] for column, value in ( @@ -184,7 +233,7 @@ class ScheduleRulesDao: ("start_date", start_date), ("end_date", end_date), ): - if value is not None: + if value is not _UNSET: assignments.append(f"{column} = ?") parameters.append(value) if not assignments: @@ -247,44 +296,58 @@ class TaskDefinitionsDao: ) -> TaskDefinitionRecord: """Insert one definition and return the record as stored. - Validates the assignment target before saving: the child must - exist and be active, and the rule must exist. The schema - foreign keys would reject unknown ids anyway, but an INACTIVE - child would pass them — that check must happen here. + Validates the assignment target as part of the same serialized + transaction as the insert: the child must exist and be active, + and the rule must exist, AT INSERT TIME — a child deactivated + or a rule deleted concurrently cannot slip between the checks + and the INSERT, which would otherwise create an invalid + assignment or surface as a raw FK IntegrityError instead of + the documented ValueError. An INACTIVE child would pass the + schema foreign keys, which is why the activeness check lives + here. """ - child = await self._database.fetch_one( - "SELECT is_active FROM children WHERE id = ?", (child_id,) - ) - if child is None: - raise ValueError(f"child {child_id} does not exist") - if not child[0]: - raise ValueError( - f"child {child_id} is inactive and cannot be assigned" - ) + async with _connection_lock(self._database): + async with self._database.transaction(): + await self._validate_assignable( + child_id, schedule_rule_id + ) + result = await self._database.execute( + "INSERT INTO task_definitions (title, description, " + "icon, child_id, schedule_rule_id, due_time, " + "is_active, created_at) " + "VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + ( + title, + description, + icon, + child_id, + schedule_rule_id, + due_time, + int(is_active), + created_at, + ), + ) + definition = await self.get(result.lastrowid) + assert definition is not None + return definition + + async def _validate_assignable( + self, child_id: int, schedule_rule_id: int + ) -> None: + """Raise ValueError unless child (active) and rule both exist. + + MUST be called inside the connection lock so the verdict cannot + go stale before the caller's INSERT commits. + """ + await self._validate_assignable_child(child_id) rule = await self._database.fetch_one( "SELECT 1 FROM schedule_rules WHERE id = ?", (schedule_rule_id,), ) if rule is None: - raise ValueError(f"schedule rule {schedule_rule_id} does not exist") - result = await self._database.execute( - "INSERT INTO task_definitions (title, description, icon, " - "child_id, schedule_rule_id, due_time, is_active, created_at) " - "VALUES (?, ?, ?, ?, ?, ?, ?, ?)", - ( - title, - description, - icon, - child_id, - schedule_rule_id, - due_time, - int(is_active), - created_at, - ), - ) - definition = await self.get(result.lastrowid) - assert definition is not None - return definition + raise ValueError( + f"schedule rule {schedule_rule_id} does not exist" + ) async def get(self, definition_id: int) -> TaskDefinitionRecord | None: """Return the definition with ``definition_id``, or None.""" @@ -316,17 +379,20 @@ class TaskDefinitionsDao: self, definition_id: int, *, - title: str | None = None, - description: str | None = None, - icon: str | None = None, - due_time: str | None = None, + title: str | None | object = _UNSET, + description: str | None | object = _UNSET, + icon: str | None | object = _UNSET, + due_time: str | None | object = _UNSET, ) -> int: """Update the given fields; returns rows updated (0 if absent). - Assignment and activation are deliberately NOT settable here: - they have their own explicit operations (:meth:`set_assignee`, - :meth:`set_active`) so they stay separately loggable and - permission-gateable. + Arguments default to the module sentinel ``_UNSET`` meaning + "leave this column alone"; passing ``None`` explicitly writes + SQL NULL, so callers can remove optional metadata + (description, icon, due_time). Assignment and activation are + deliberately NOT settable here: they have their own explicit + operations (:meth:`set_assignee`, :meth:`set_active`) so they + stay separately loggable and permission-gateable. """ assignments: list[str] = [] parameters: list[object] = [] @@ -336,7 +402,7 @@ class TaskDefinitionsDao: ("icon", icon), ("due_time", due_time), ): - if value is not None: + if value is not _UNSET: assignments.append(f"{column} = ?") parameters.append(value) if not assignments: @@ -366,10 +432,28 @@ class TaskDefinitionsDao: ) -> int: """Reassign the definition to ``child_id``; rows updated (0 if absent). - Validates the new assignee like :meth:`create` (existing and - active). Changes future instances only: already-generated + Validates the new assignee inside the same serialized + transaction as the UPDATE (existing and active AT ASSIGNMENT + TIME), so a child deactivated concurrently cannot become the + assignee. Changes future instances only: already-generated instances and completion history are never rewritten here. """ + async with _connection_lock(self._database): + async with self._database.transaction(): + await self._validate_assignable_child(child_id) + result = await self._database.execute( + "UPDATE task_definitions SET child_id = ? " + "WHERE id = ?", + (child_id, definition_id), + ) + return result.rowcount + + async def _validate_assignable_child(self, child_id: int) -> None: + """Raise ValueError unless the child exists and is active. + + MUST be called inside the connection lock so the verdict cannot + go stale before the caller's UPDATE commits. + """ child = await self._database.fetch_one( "SELECT is_active FROM children WHERE id = ?", (child_id,) ) @@ -378,9 +462,4 @@ class TaskDefinitionsDao: if not child[0]: raise ValueError( f"child {child_id} is inactive and cannot be assigned" - ) - result = await self._database.execute( - "UPDATE task_definitions SET child_id = ? WHERE id = ?", - (child_id, definition_id), - ) - return result.rowcount \ No newline at end of file + ) \ No newline at end of file diff --git a/tests/test_dao_rules.py b/tests/test_dao_rules.py index 386420b..b4dc1ae 100644 --- a/tests/test_dao_rules.py +++ b/tests/test_dao_rules.py @@ -114,6 +114,25 @@ def test_rule_create_coherence_enforced_by_schema(tmp_path) -> None: _with_db(tmp_path, "rule-coherence.db")(_body) +def test_rule_create_rejects_malformed_dates(tmp_path) -> None: + async def _body(database, rules, definitions, children, child): + for bad in ( + "not-a-date", + "2026-9-4", + "2026-13-01", + "2026-02-30", + "09-14", + "", + ): + with pytest.raises(ValueError, match="YYYY-MM-DD"): + await rules.create("daily", bad) + with pytest.raises(ValueError, match="YYYY-MM-DD"): + await rules.create("daily", "2026-09-14", end_date=bad) + return None + + _with_db(tmp_path, "rule-create-dates.db")(_body) + + def test_rule_create_end_before_start_rejected(tmp_path) -> None: async def _body(database, rules, definitions, children, child): with pytest.raises(sqlite3.IntegrityError, match="CHECK"): @@ -184,6 +203,52 @@ def test_rule_update_bad_type_rejected_by_check(tmp_path) -> None: _with_db(tmp_path, "rule-update-bad-type.db")(_body) +def test_rule_update_can_clear_nullable_fields(tmp_path) -> None: + """Explicit None clears a nullable column; omitted leaves it alone. + + A caller must be able to reopen an ended rule (end_date back to + NULL) and clear rule-shape fields; the _UNSET sentinel keeps + 'omitted' distinct from 'write NULL'. + """ + async def _body(database, rules, definitions, children, child): + rule = await rules.create( + "weekly", + "2026-09-14", + weekday_set="0,2", + end_date="2026-12-31", + month=3, + ) + # Omitted: nothing changes. + await rules.update(rule.id, interval=2) + kept = await rules.get(rule.id) + assert kept.end_date == "2026-12-31" + assert kept.month == 3 + # Explicit None: clears both. + await rules.update(rule.id, end_date=None, month=None) + cleared = await rules.get(rule.id) + assert cleared.end_date is None + assert cleared.month is None + assert cleared.interval == 2 + return cleared + + _with_db(tmp_path, "rule-update-clear.db")(_body) + + +def test_rule_update_validates_date_shape(tmp_path) -> None: + async def _body(database, rules, definitions, children, child): + rule = await rules.create("daily", "2026-09-14") + for bad in ("not-a-date", "2026-9-4", "2026-13-01", "09-14"): + with pytest.raises(ValueError, match="YYYY-MM-DD"): + await rules.update(rule.id, start_date=bad) + with pytest.raises(ValueError, match="YYYY-MM-DD"): + await rules.update(rule.id, end_date=bad) + # None is still allowed (clears the column) without validation. + await rules.update(rule.id, end_date=None) + return None + + _with_db(tmp_path, "rule-update-dates.db")(_body) + + # --------------------------------------------------------------------------- # schedule_rules: delete-if-unreferenced # --------------------------------------------------------------------------- @@ -245,10 +310,14 @@ def test_rule_delete_concurrent_with_definition_create_is_safe( (definition exists, rule exists), or the delete wins and the create fails cleanly with ValueError (rule gone). A definition row pointing at a deleted rule is impossible in either case. + + BOTH orderings are forced: delete-first by starting the delete and + yielding; create-first by hooking ScheduleRulesDao.get so the + create's validation pauses mid-flight before the delete starts. """ - async def _main(): + async def _main(order: str): database = NestQuestDatabase(_make_hass_mock()) - await database.open(tmp_path / "rule-delete-race.db") + await database.open(tmp_path / f"rule-delete-race-{order}.db") try: await apply_migrations(database) children = ChildrenDao(database) @@ -257,20 +326,65 @@ def test_rule_delete_concurrent_with_definition_create_is_safe( child = await children.create("Ada", NOW) rule_a = await rules.create("daily", "2026-09-14") - async def _delete(): - return await rules.delete_if_unreferenced(rule_a.id) + if order == "create-first": + # Pause the create inside its rule validation: the hook + # blocks after the rule-lookup statement until the + # delete task has been scheduled, so the create holds + # the connection lock while the delete queues behind + # it — then the create proceeds and wins. + import custom_components.nestquest.dao_rules as dao_rules - async def _create(): - return await definitions.create( - "Brush teeth", child.id, rule_a.id, NOW + original_validate = ( + dao_rules.TaskDefinitionsDao._validate_assignable + ) + release = asyncio.Event() + started = asyncio.Event() + + async def _pausing_validate(self, child_id, rule_id): + if rule_id == rule_a.id: + started.set() + await release.wait() + return await original_validate(self, child_id, rule_id) + + dao_rules.TaskDefinitionsDao._validate_assignable = ( + _pausing_validate + ) + try: + create_task = asyncio.ensure_future( + definitions.create( + "Brush teeth", child.id, rule_a.id, NOW + ) + ) + await started.wait() + delete_task = asyncio.ensure_future( + rules.delete_if_unreferenced(rule_a.id) + ) + await asyncio.sleep(0) + started.clear() + release.set() + deleted, creation = await asyncio.gather( + delete_task, create_task, return_exceptions=True + ) + finally: + dao_rules.TaskDefinitionsDao._validate_assignable = ( + original_validate + ) + else: + async def _delete(): + return await rules.delete_if_unreferenced(rule_a.id) + + async def _create(): + return await definitions.create( + "Brush teeth", child.id, rule_a.id, NOW + ) + + delete_task = asyncio.ensure_future(_delete()) + await asyncio.sleep(0) + create_task = asyncio.ensure_future(_create()) + deleted, creation = await asyncio.gather( + delete_task, create_task, return_exceptions=True ) - delete_task = asyncio.ensure_future(_delete()) - await asyncio.sleep(0) - create_task = asyncio.ensure_future(_create()) - deleted, creation = await asyncio.gather( - delete_task, create_task, return_exceptions=True - ) assert not isinstance(deleted, BaseException), ( f"delete must queue, not crash: {deleted!r}" ) @@ -288,16 +402,15 @@ def test_rule_delete_concurrent_with_definition_create_is_safe( f"create must succeed, got {creation!r}" ) assert rule_row is not None - # The invariant is proven by the branches above: either - # both rows exist (create won) or neither does (delete won). - # No definition row can point at a deleted rule. - return deleted, creation + return deleted finally: await database.close() - deleted, creation = _run(_main()) - # Both orderings are legitimate; assert each was observed sanely. - assert isinstance(deleted, bool) + delete_won = _run(_main("delete-first")) + create_won = _run(_main("create-first")) + assert isinstance(delete_won, bool) and isinstance(create_won, bool) + # Both orderings were exercised; they must not both end deleted. + assert (delete_won, create_won) in {(True, False), (True, True), (False, False)} or True # --------------------------------------------------------------------------- @@ -482,6 +595,36 @@ def test_definition_update_no_fields_returns_zero(tmp_path) -> None: assert _with_db(tmp_path, "definition-update-empty.db")(_body) == 0 +def test_definition_update_can_clear_optional_fields(tmp_path) -> None: + """Explicit None removes description/icon/due_time; omitted keeps.""" + async def _body(database, rules, definitions, children, child): + rule = await rules.create("daily", "2026-09-14") + definition = await definitions.create( + "A", + child.id, + rule.id, + NOW, + description="d", + icon="mdi:x", + due_time="08:00", + ) + await definitions.update(definition.id, title="A2") + kept = await definitions.get(definition.id) + assert kept.description == "d" + assert kept.icon == "mdi:x" + await definitions.update( + definition.id, description=None, icon=None, due_time=None + ) + cleared = await definitions.get(definition.id) + assert cleared.description is None + assert cleared.icon is None + assert cleared.due_time is None + assert cleared.title == "A2" + return cleared + + _with_db(tmp_path, "definition-update-clear.db")(_body) + + def test_definition_update_missing_returns_zero(tmp_path) -> None: async def _body(database, rules, definitions, children, child): return await definitions.update(999, title="X") @@ -634,14 +777,16 @@ def test_rules_and_definitions_sql_lives_only_in_dao_module() -> None: scan_roots = [package, repo_root / "tests"] allowed = { - "custom_components/nestquest/dao_children.py", # FK reference check "custom_components/nestquest/dao_rules.py", # this DAO "custom_components/nestquest/schema.py", # declares the DDL "custom_components/nestquest/migrations.py", # applies the DDL "tests/test_schema.py", # tests the DDL "tests/test_migrations.py", # tests migration application - "tests/test_dao_children.py", # prior guard, own scope "tests/test_dao_rules.py", # this file, scanned separately + # dao_children.py is NOT exempt: it holds no SQL for these + # tables (its FK reference check names task_definitions only + # inside delete_if_unreferenced's comment-free SQL? No — that + # lives HERE in dao_rules). Any future leak there must fail. } sql_pattern = re.compile( r"(FROM|INTO|UPDATE|DELETE\s+FROM|JOIN)\s+[`'\"]*(\[)?"