From 7cb03237d8f5cf8e9fac0ac6c81f0ac5e64f81b9 Mon Sep 17 00:00:00 2001 From: Joshua Date: Thu, 17 Sep 2026 08:22:51 -0400 Subject: [PATCH 1/4] Implement create_quest_definition business layer (Feature 06, task 2) --- custom_components/nestquest/dao_rules.py | 122 +++++- .../nestquest/quest_definitions.py | 202 ++++++++++ tests/test_quest_definitions.py | 353 ++++++++++++++++++ 3 files changed, 663 insertions(+), 14 deletions(-) create mode 100644 custom_components/nestquest/quest_definitions.py create mode 100644 tests/test_quest_definitions.py diff --git a/custom_components/nestquest/dao_rules.py b/custom_components/nestquest/dao_rules.py index d994791..ae5dc5a 100644 --- a/custom_components/nestquest/dao_rules.py +++ b/custom_components/nestquest/dao_rules.py @@ -400,6 +400,37 @@ def _single_weekday_from_csv(value: str | None) -> int: ) from None +async def _insert_rule_row( + database: NestQuestDatabase, storage: ScheduleRuleStorage +) -> int: + """Insert one ``schedule_rules`` row and return its new id. + + Runs inside the caller's transaction (no lock or transaction of its + own) so a multi-table write can persist the rule together with the + rows that reference it. The caller is responsible for validating + the storage fields — :meth:`ScheduleRulesDao.create` validates the + date shape, and :meth:`QuestDefinitionsDao.create_with_rule_and_windows` + receives storage already mapped from a validated + :class:`~.recurrence.ScheduleRule`. + """ + result = await database.execute( + "INSERT INTO schedule_rules (rule_type, interval, weekday_set, " + "day_of_month, nth_weekday, month, start_date, end_date) " + "VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + ( + storage.rule_type, + storage.interval, + storage.weekday_set, + storage.day_of_month, + storage.nth_weekday, + storage.month, + storage.start_date, + storage.end_date, + ), + ) + return result.lastrowid + + class ScheduleRulesDao: """Typed async access to the ``schedule_rules`` table.""" @@ -430,22 +461,20 @@ class ScheduleRulesDao: _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) " - "VALUES (?, ?, ?, ?, ?, ?, ?, ?)", - ( - rule_type, - interval, - weekday_set, - day_of_month, - nth_weekday, - month, - start_date, - end_date, + rule_id = await _insert_rule_row( + self._database, + ScheduleRuleStorage( + rule_type=rule_type, + interval=interval, + weekday_set=weekday_set, + day_of_month=day_of_month, + nth_weekday=nth_weekday, + month=month, + start_date=start_date, + end_date=end_date, ), ) - rule = await self.get(result.lastrowid) + rule = await self.get(rule_id) assert rule is not None return rule @@ -605,6 +634,71 @@ class QuestDefinitionsDao: assert definition is not None return definition + async def create_with_rule_and_windows( + self, + title: str, + schedule_rule: ScheduleRuleStorage, + created_at: str, + assignee_child_ids: list[int], + windows: list[tuple[str, str | None]], + *, + description: str | None = None, + icon: str | None = None, + ) -> QuestDefinitionRecord: + """Insert a rule, definition, assignees and windows atomically. + + The whole write — the schedule rule row, the definition row, + every assignee link and every window declaration — runs inside + ONE transaction under the connection lock, so a validation or + write failure rolls the lot back together (all-or-nothing): the + caller can never observe a definition with a missing rule, + assignee or window, and a rejected create leaves no partial + rows. + + The rule arrives as pre-validated storage fields (mapped from a + :class:`~.recurrence.ScheduleRule` via + :func:`schedule_rule_to_storage`); its dates are re-validated + here so a hand-built storage can never persist a malformed date. + The definition-level ``due_time`` column is deliberately left + NULL — per-window due times supersede it (D-008). Assignees are + validated as existing-and-active at insert time, and windows are + validated for name and strict HH:MM due time, matching + :meth:`upsert_window`. + """ + _validate_date(schedule_rule.start_date, "start_date") + if schedule_rule.end_date is not None: + _validate_date(schedule_rule.end_date, "end_date") + async with _connection_lock(self._database): + async with self._database.transaction(): + for child_id in assignee_child_ids: + await self._validate_assignable_child(child_id) + rule_id = await _insert_rule_row(self._database, schedule_rule) + result = await self._database.execute( + "INSERT INTO quest_definitions (title, description, " + "icon, schedule_rule_id, due_time, is_active, " + "created_at) VALUES (?, ?, ?, ?, ?, ?, ?)", + (title, description, icon, rule_id, None, 1, created_at), + ) + definition_id = result.lastrowid + for child_id in assignee_child_ids: + await self._database.execute( + "INSERT INTO quest_definition_assignees " + "(definition_id, child_id) VALUES (?, ?)", + (definition_id, child_id), + ) + for window, due_time in windows: + _validate_window(window) + if due_time is not None: + _validate_time(due_time, "due_time") + await self._database.execute( + "INSERT INTO quest_definition_windows " + "(definition_id, window, due_time) VALUES (?, ?, ?)", + (definition_id, window, due_time), + ) + definition = await self.get(definition_id) + assert definition is not None + return definition + async def _validate_rule_exists(self, schedule_rule_id: int) -> None: """Raise ValueError unless the rule exists. diff --git a/custom_components/nestquest/quest_definitions.py b/custom_components/nestquest/quest_definitions.py new file mode 100644 index 0000000..3cf8b36 --- /dev/null +++ b/custom_components/nestquest/quest_definitions.py @@ -0,0 +1,202 @@ +"""Business layer for creating quest definitions (Feature 06). + +Sits between the Feature 09 service gate and the typed +:class:`~.dao_rules.QuestDefinitionsDao`: callers get validation and the +all-or-nothing create here, and never touch the DAO SQL directly. +:func:`create_quest_definition` is the create path; there is no +permission check here (that is Feature 09). + +Validation policy mirrors :mod:`.children`: + +- ``title`` is required and trimmed; empty or whitespace-only raises + ValueError. +- ``rule`` must be an already-validated :class:`~.recurrence.ScheduleRule` + (its constructor is total — rule validation is NOT re-implemented + here). +- ``assignee_child_ids`` is non-empty; every id is a plain int (bools + and floats rejected, since SQLite would bind them onto a real child) + and must reference an existing ACTIVE child (the DAO enforces the + active/exists check inside the same transaction). +- ``windows`` is non-empty; each entry is a window name from + :data:`~.const.QUEST_WINDOWS` — either a bare name (no due time) or a + ``(name, due_time)`` pair whose due time is a strict 24-hour HH:MM + string. + +The rule, definition, assignees and windows are persisted in ONE +transaction (the DAO's :meth:`~.dao_rules.QuestDefinitionsDao.create_with_rule_and_windows`), +so a rejected or failed create persists nothing. The definition-level +``due_time`` column is superseded by per-window due times (D-008) and is +never set here. ``created_at`` is stamped UTC ISO-8601 at second +precision, matching :mod:`.children`. +""" +from __future__ import annotations + +import datetime +from dataclasses import dataclass + +from .dao_children import ChildRecord +from .dao_rules import ( + QuestDefinitionRecord, + QuestDefinitionWindowRecord, + QuestDefinitionsDao, + _validate_time, + _validate_window, + schedule_rule_from_storage, + schedule_rule_to_storage, +) +from .db import NestQuestDatabase +from .recurrence import ScheduleRule + + +@dataclass(frozen=True) +class CreatedQuestDefinition: + """The persisted result of :func:`create_quest_definition`. + + ``definition`` is the stored row; ``rule`` is the schedule rule + decoded back through the storage mapping; ``assignees`` and + ``windows`` are the definition's linked child records and window + declarations, read through the DAO. + """ + + definition: QuestDefinitionRecord + rule: ScheduleRule + assignees: list[ChildRecord] + windows: list[QuestDefinitionWindowRecord] + + +def _now_stamp() -> str: + """Return the module's strict UTC second-precision stamp.""" + return datetime.datetime.now(datetime.timezone.utc).isoformat( + timespec="seconds" + ) + + +def _validate_text( + value: object, field: str, *, required: bool = False +) -> str | None: + """Validate a free-text field and return it trimmed. + + ``None`` raises when ``required`` and otherwise returns None. A + non-string value is rejected; an empty result is rejected for + required fields. + """ + if value is None: + if required: + raise ValueError(f"{field} is required") + return None + if not isinstance(value, str): + raise ValueError(f"{field} must be a string, got {value!r}") + trimmed = value.strip() + if required and not trimmed: + raise ValueError(f"{field} is required") + return trimmed + + +def _validate_assignee_ids(value: object) -> list[int]: + """Return the assignee ids as plain ints, or raise naming the field. + + The list must be non-empty; each entry must be a real int (bools and + floats rejected) so SQLite cannot bind ``True`` onto child 1 or a + float onto a neighbouring profile. + """ + if not isinstance(value, (list, tuple)): + raise ValueError( + "assignee_child_ids must be a non-empty list of child ids" + ) + if not value: + raise ValueError("assignee_child_ids must not be empty") + ids: list[int] = [] + for entry in value: + if isinstance(entry, bool) or not isinstance(entry, int): + raise ValueError( + "assignee_child_ids must contain integer child ids, " + f"got {entry!r}" + ) + ids.append(entry) + return ids + + +def _normalize_windows( + value: object, +) -> list[tuple[str, str | None]]: + """Normalize the windows argument to ``(name, due_time)`` pairs. + + Each entry is a bare window name (due time None) or a + ``(name, due_time)`` pair. The name must be a + :data:`~.const.QUEST_WINDOWS` spelling and a present due time must be + a strict 24-hour HH:MM, both validated up front so a malformed + window is rejected before any write. + """ + if not isinstance(value, (list, tuple)): + raise ValueError( + "windows must be a non-empty list of window names or " + "(name, due_time) pairs" + ) + if not value: + raise ValueError("windows must not be empty") + normalized: list[tuple[str, str | None]] = [] + for index, entry in enumerate(value): + if isinstance(entry, str): + name: object = entry + due_time: object = None + elif isinstance(entry, (tuple, list)) and len(entry) == 2: + name, due_time = entry[0], entry[1] + else: + raise ValueError( + f"windows[{index}] must be a window name or a " + f"(name, due_time) pair, got {entry!r}" + ) + _validate_window(name) + if due_time is not None: + _validate_time(due_time, "due_time") + normalized.append((name, due_time)) + return normalized + + +async def create_quest_definition( + database: NestQuestDatabase, + title: str, + rule: ScheduleRule, + assignee_child_ids: list[int], + windows: list[str | tuple[str, str | None]], + *, + description: str | None = None, + icon: str | None = None, +) -> CreatedQuestDefinition: + """Create a quest definition (rule + assignees + windows) atomically. + + Validates the arguments up front, then persists the schedule rule, + the definition, its assignees and its windows in one transaction via + the DAO. Returns the stored definition together with its decoded + rule, assignees and windows. Raises ValueError on any rejected + argument; a rejected or failed create persists nothing. + """ + name = _validate_text(title, "title", required=True) + assert name is not None + if not isinstance(rule, ScheduleRule): + raise ValueError(f"rule must be a ScheduleRule, got {rule!r}") + assignee_ids = _validate_assignee_ids(assignee_child_ids) + window_specs = _normalize_windows(windows) + description_value = _validate_text(description, "description") + icon_value = _validate_text(icon, "icon") + + storage = schedule_rule_to_storage(rule) + dao = QuestDefinitionsDao(database) + definition = await dao.create_with_rule_and_windows( + name, + storage, + _now_stamp(), + assignee_ids, + window_specs, + description=description_value, + icon=icon_value, + ) + decoded_rule = schedule_rule_from_storage(storage) + assignees = await dao.list_assignees(definition.id) + window_records = await dao.list_windows(definition.id) + return CreatedQuestDefinition( + definition=definition, + rule=decoded_rule, + assignees=assignees, + windows=window_records, + ) diff --git a/tests/test_quest_definitions.py b/tests/test_quest_definitions.py new file mode 100644 index 0000000..35e2d01 --- /dev/null +++ b/tests/test_quest_definitions.py @@ -0,0 +1,353 @@ +"""Tests for quest_definitions.py: the Feature 06 create business layer.""" +from __future__ import annotations + +import asyncio +import re + +import pytest + +from custom_components.nestquest.dao_children import ChildrenDao +from custom_components.nestquest.dao_rules import ( + QuestDefinitionsDao, + ScheduleRulesDao, +) +from custom_components.nestquest.db import NestQuestDatabase +from custom_components.nestquest.migrations import apply_migrations +from custom_components.nestquest.quest_definitions import ( + CreatedQuestDefinition, + create_quest_definition, +) +from custom_components.nestquest.recurrence import RuleType, ScheduleRule + + +def _run(coro): + return asyncio.new_event_loop().run_until_complete(coro) + + +def _make_hass_mock(): + from unittest.mock import AsyncMock, MagicMock + + hass = MagicMock() + hass.async_add_executor_job = AsyncMock(side_effect=(lambda fn, *a: fn(*a))) + return hass + + +NOW = "2026-09-14T12:00:00+00:00" + + +async def _prepare(path) -> tuple: + database = NestQuestDatabase(_make_hass_mock()) + await database.open(path) + await apply_migrations(database) + children = ChildrenDao(database) + rules = ScheduleRulesDao(database) + definitions = QuestDefinitionsDao(database) + child = await children.create("Ada", NOW) + return database, children, rules, definitions, child + + +def _with_db(tmp_path, name): + """Run an async body against a prepared database, closing it after.""" + + def _run_test(body): + async def _main(): + database, children, rules, definitions, child = await _prepare( + tmp_path / name + ) + try: + return await body(database, children, rules, definitions, child) + finally: + await database.close() + + return _run(_main()) + + return _run_test + + +def _daily_rule() -> ScheduleRule: + return ScheduleRule(rule_type=RuleType.DAILY, start_date="2026-09-14") + + +# --------------------------------------------------------------------------- +# create_quest_definition: successful creates +# --------------------------------------------------------------------------- + + +def test_create_one_child_one_window(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + rule = _daily_rule() + result = await create_quest_definition( + database, + " Brush teeth ", + rule, + [child.id], + ["morning"], + description=" Twice daily ", + icon=" mdi:tooth ", + ) + assert isinstance(result, CreatedQuestDefinition) + definition = result.definition + assert definition.title == "Brush teeth" + assert definition.description == "Twice daily" + assert definition.icon == "mdi:tooth" + assert definition.is_active is True + # D-008: the definition-level due_time column is never set here. + assert definition.due_time is None + # Timestamp is stamped UTC at second precision by the business layer. + assert re.fullmatch( + r"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\+00:00", + definition.created_at, + ) + # The rule round-trips through the storage mapping. + assert result.rule == rule + assert definition.schedule_rule_id is not None + rule_row = await rules.get(definition.schedule_rule_id) + assert rule_row is not None + assert rule_row.rule_type == "daily" + # Assignees and windows are read back through the DAO. + assert [c.id for c in result.assignees] == [child.id] + assert [w.window for w in result.windows] == ["morning"] + assert result.windows[0].due_time is None + return result + + _with_db(tmp_path, "create-one.db")(_body) + + +def test_create_several_children_several_windows_with_due_times( + tmp_path, +) -> None: + async def _body(database, children, rules, definitions, child): + bo = await children.create("Bo", NOW) + cleo = await children.create("Cleo", NOW) + rule = ScheduleRule( + rule_type=RuleType.WEEKLY, + weekday_set={0, 2, 4}, + start_date="2026-09-14", + ) + result = await create_quest_definition( + database, + "Chore", + rule, + [child.id, bo.id, cleo.id], + [("morning", "09:00"), ("evening", "19:30")], + ) + assert sorted(c.id for c in result.assignees) == sorted( + [child.id, bo.id, cleo.id] + ) + by_name = {w.window: w.due_time for w in result.windows} + assert by_name == {"morning": "09:00", "evening": "19:30"} + assert result.rule == rule + stored = await definitions.get(result.definition.id) + assert stored is not None + assert stored.due_time is None + return result + + _with_db(tmp_path, "create-many.db")(_body) + + +# --------------------------------------------------------------------------- +# create_quest_definition: argument rejections +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize("bad", ["", " ", "\t\n", None]) +def test_create_rejects_empty_title(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises(ValueError, match="title is required"): + await create_quest_definition( + database, bad, _daily_rule(), [child.id], ["morning"] + ) + return None + + _with_db(tmp_path, "create-bad-title.db")(_body) + + +def test_create_rejects_non_string_title(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises(ValueError, match="title must be a string"): + await create_quest_definition( + database, 42, _daily_rule(), [child.id], ["morning"] + ) + return None + + _with_db(tmp_path, "create-bad-title-type.db")(_body) + + +@pytest.mark.parametrize("bad", [None, "daily", 42, {"rule_type": "daily"}]) +def test_create_rejects_non_schedule_rule(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises(ValueError, match="rule must be a ScheduleRule"): + await create_quest_definition( + database, "Brush teeth", bad, [child.id], ["morning"] + ) + return None + + _with_db(tmp_path, "create-bad-rule.db")(_body) + + +@pytest.mark.parametrize("bad", [[], ()]) +def test_create_rejects_empty_assignees(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises( + ValueError, match="assignee_child_ids must not be empty" + ): + await create_quest_definition( + database, "Brush teeth", _daily_rule(), bad, ["morning"] + ) + return None + + _with_db(tmp_path, "create-empty-assignees.db")(_body) + + +@pytest.mark.parametrize("bad", [True, False, 1.5, "1", None]) +def test_create_rejects_non_int_assignee(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises( + ValueError, match="assignee_child_ids must contain integer" + ): + await create_quest_definition( + database, "Brush teeth", _daily_rule(), [bad], ["morning"] + ) + return None + + _with_db(tmp_path, "create-bad-assignee.db")(_body) + + +def test_create_rejects_inactive_child(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + inactive = await children.create("Bo", NOW) + await children.set_active(inactive.id, False) + with pytest.raises(ValueError, match="inactive"): + await create_quest_definition( + database, + "Brush teeth", + _daily_rule(), + [inactive.id], + ["morning"], + ) + return None + + _with_db(tmp_path, "create-inactive-child.db")(_body) + + +def test_create_rejects_unknown_child(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises(ValueError, match="does not exist"): + await create_quest_definition( + database, "Brush teeth", _daily_rule(), [999], ["morning"] + ) + return None + + _with_db(tmp_path, "create-unknown-child.db")(_body) + + +@pytest.mark.parametrize("bad", [[], ()]) +def test_create_rejects_empty_windows(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises(ValueError, match="windows must not be empty"): + await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], bad + ) + return None + + _with_db(tmp_path, "create-empty-windows.db")(_body) + + +@pytest.mark.parametrize("bad", [["noon"], [("MORNING", None)], [("", None)]]) +def test_create_rejects_invalid_window(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises(ValueError, match="window must be one of"): + await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], bad + ) + return None + + _with_db(tmp_path, "create-bad-window.db")(_body) + + +@pytest.mark.parametrize("bad", ["9:30", "25:00", "09:60", "0900"]) +def test_create_rejects_malformed_due_time(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises(ValueError, match="HH:MM"): + await create_quest_definition( + database, + "Brush teeth", + _daily_rule(), + [child.id], + [("morning", bad)], + ) + return None + + _with_db(tmp_path, "create-bad-due-time.db")(_body) + + +# --------------------------------------------------------------------------- +# create_quest_definition: a rejected create persists nothing +# --------------------------------------------------------------------------- + + +async def _assert_only_baseline_rows( + rules, definitions, child, baseline_definition +) -> None: + """Assert the baseline rule/definition/assignee/window rows survive + unchanged and no second row of any of them appeared. + + A rejected create must leave each table's row count exactly as it + was: still one rule, one definition, one assignee, one window — + checked through the DAO (id 2 must be absent everywhere, id 1 + intact), since the leak guard forbids raw SQL in this test file. + """ + assert await rules.get(1) is not None + assert await rules.get(2) is None + assert await definitions.get(1) == baseline_definition + assert await definitions.get(2) is None + assert [c.id for c in await definitions.list_assignees(1)] == [child.id] + assert await definitions.list_assignees(2) == [] + assert [w.window for w in await definitions.list_windows(1)] == [ + "morning" + ] + assert await definitions.list_windows(2) == [] + + +def test_rejected_create_writes_nothing(tmp_path) -> None: + """A rejected create leaves every quest row count unchanged. + + Two rejection paths are forced: an in-transaction rejection (an + inactive assignee fails validation inside the transaction and must + roll the rule/definition back together) and a pre-write rejection + (an invalid window is caught in the business layer before any write). + Both must leave the baseline single-rule/single-definition state + untouched, with no second row in any of the four tables. + """ + async def _body(database, children, rules, definitions, child): + baseline_rule = await rules.create("daily", "2026-09-14") + baseline_definition = await definitions.create( + "Baseline", baseline_rule.id, NOW, assignee_child_ids=[child.id] + ) + await definitions.upsert_window(baseline_definition.id, "morning") + inactive = await children.create("Bo", NOW) + await children.set_active(inactive.id, False) + rule = _daily_rule() + + # In-transaction rejection: the inactive child fails validation + # inside the transaction, rolling the rule and definition back. + with pytest.raises(ValueError, match="inactive"): + await create_quest_definition( + database, "Brush teeth", rule, [inactive.id], ["morning"] + ) + await _assert_only_baseline_rows( + rules, definitions, child, baseline_definition + ) + + # Pre-write rejection: an invalid window is caught up front, so + # no transaction ever opens. + with pytest.raises(ValueError, match="window must be one of"): + await create_quest_definition( + database, "Brush teeth", rule, [child.id], [("noon", None)] + ) + await _assert_only_baseline_rows( + rules, definitions, child, baseline_definition + ) + return None + + _with_db(tmp_path, "create-rejected-nothing.db")(_body) -- 2.55.0 From 71fbbf20e46575a5e0d88bdce40e7dbbe1395d8d Mon Sep 17 00:00:00 2001 From: Joshua Date: Thu, 17 Sep 2026 08:30:46 -0400 Subject: [PATCH 2/4] fix: reject duplicates, return consistent snapshot, test post-write rollback (NQ-67-01..03) --- custom_components/nestquest/dao_rules.py | 31 +++++++- .../nestquest/quest_definitions.py | 34 +++++--- tests/test_quest_definitions.py | 78 +++++++++++++++++++ 3 files changed, 131 insertions(+), 12 deletions(-) diff --git a/custom_components/nestquest/dao_rules.py b/custom_components/nestquest/dao_rules.py index ae5dc5a..672c064 100644 --- a/custom_components/nestquest/dao_rules.py +++ b/custom_components/nestquest/dao_rules.py @@ -163,6 +163,22 @@ class QuestDefinitionWindowRecord: due_time: str | None +@dataclass(frozen=True) +class QuestDefinitionSnapshot: + """A definition plus its assignees and windows, read atomically. + + Returned by :meth:`QuestDefinitionsDao.create_with_rule_and_windows` + with the assignees and windows read inside the SAME transaction that + inserted them, so the snapshot is a consistent creation-time view — + a concurrent assignee or window mutation cannot change it between + the write and the read. + """ + + definition: QuestDefinitionRecord + assignees: list[ChildRecord] + windows: list[QuestDefinitionWindowRecord] + + _RULE_COLUMNS = ( "id, rule_type, interval, weekday_set, day_of_month, nth_weekday, " "month, start_date, end_date" @@ -644,7 +660,7 @@ class QuestDefinitionsDao: *, description: str | None = None, icon: str | None = None, - ) -> QuestDefinitionRecord: + ) -> QuestDefinitionSnapshot: """Insert a rule, definition, assignees and windows atomically. The whole write — the schedule rule row, the definition row, @@ -655,6 +671,11 @@ class QuestDefinitionsDao: assignee or window, and a rejected create leaves no partial rows. + Returns a :class:`QuestDefinitionSnapshot` whose assignees and + windows are read back inside the same transaction, so the + returned view cannot be raced by a concurrent assignee/window + mutation after the write. + The rule arrives as pre-validated storage fields (mapped from a :class:`~.recurrence.ScheduleRule` via :func:`schedule_rule_to_storage`); its dates are re-validated @@ -696,8 +717,14 @@ class QuestDefinitionsDao: (definition_id, window, due_time), ) definition = await self.get(definition_id) + assignees = await self.list_assignees(definition_id) + window_records = await self.list_windows(definition_id) assert definition is not None - return definition + return QuestDefinitionSnapshot( + definition=definition, + assignees=assignees, + windows=window_records, + ) async def _validate_rule_exists(self, schedule_rule_id: int) -> None: """Raise ValueError unless the rule exists. diff --git a/custom_components/nestquest/quest_definitions.py b/custom_components/nestquest/quest_definitions.py index 3cf8b36..1efce70 100644 --- a/custom_components/nestquest/quest_definitions.py +++ b/custom_components/nestquest/quest_definitions.py @@ -16,11 +16,15 @@ Validation policy mirrors :mod:`.children`: - ``assignee_child_ids`` is non-empty; every id is a plain int (bools and floats rejected, since SQLite would bind them onto a real child) and must reference an existing ACTIVE child (the DAO enforces the - active/exists check inside the same transaction). + active/exists check inside the same transaction). Duplicate ids are + rejected here, before the composite primary key could surface them as + an :class:`sqlite3.IntegrityError`. - ``windows`` is non-empty; each entry is a window name from :data:`~.const.QUEST_WINDOWS` — either a bare name (no due time) or a ``(name, due_time)`` pair whose due time is a strict 24-hour HH:MM - string. + string. Duplicate window names are rejected here, before the + composite primary key could surface them as an + :class:`sqlite3.IntegrityError`. The rule, definition, assignees and windows are persisted in ONE transaction (the DAO's :meth:`~.dao_rules.QuestDefinitionsDao.create_with_rule_and_windows`), @@ -113,6 +117,10 @@ def _validate_assignee_ids(value: object) -> list[int]: f"got {entry!r}" ) ids.append(entry) + if len(set(ids)) != len(ids): + raise ValueError( + "assignee_child_ids must not contain duplicate child ids" + ) return ids @@ -135,6 +143,7 @@ def _normalize_windows( if not value: raise ValueError("windows must not be empty") normalized: list[tuple[str, str | None]] = [] + seen: set[str] = set() for index, entry in enumerate(value): if isinstance(entry, str): name: object = entry @@ -149,6 +158,11 @@ def _normalize_windows( _validate_window(name) if due_time is not None: _validate_time(due_time, "due_time") + if name in seen: + raise ValueError( + f"windows must not contain duplicate window {name!r}" + ) + seen.add(name) normalized.append((name, due_time)) return normalized @@ -168,8 +182,10 @@ async def create_quest_definition( Validates the arguments up front, then persists the schedule rule, the definition, its assignees and its windows in one transaction via the DAO. Returns the stored definition together with its decoded - rule, assignees and windows. Raises ValueError on any rejected - argument; a rejected or failed create persists nothing. + rule, assignees and windows — all read back inside that same + transaction, so the returned bundle is a consistent creation + snapshot. Raises ValueError on any rejected argument; a rejected or + failed create persists nothing. """ name = _validate_text(title, "title", required=True) assert name is not None @@ -182,7 +198,7 @@ async def create_quest_definition( storage = schedule_rule_to_storage(rule) dao = QuestDefinitionsDao(database) - definition = await dao.create_with_rule_and_windows( + snapshot = await dao.create_with_rule_and_windows( name, storage, _now_stamp(), @@ -192,11 +208,9 @@ async def create_quest_definition( icon=icon_value, ) decoded_rule = schedule_rule_from_storage(storage) - assignees = await dao.list_assignees(definition.id) - window_records = await dao.list_windows(definition.id) return CreatedQuestDefinition( - definition=definition, + definition=snapshot.definition, rule=decoded_rule, - assignees=assignees, - windows=window_records, + assignees=snapshot.assignees, + windows=snapshot.windows, ) diff --git a/tests/test_quest_definitions.py b/tests/test_quest_definitions.py index 35e2d01..3b1c7c6 100644 --- a/tests/test_quest_definitions.py +++ b/tests/test_quest_definitions.py @@ -10,6 +10,7 @@ from custom_components.nestquest.dao_children import ChildrenDao from custom_components.nestquest.dao_rules import ( QuestDefinitionsDao, ScheduleRulesDao, + schedule_rule_to_storage, ) from custom_components.nestquest.db import NestQuestDatabase from custom_components.nestquest.migrations import apply_migrations @@ -213,6 +214,23 @@ def test_create_rejects_non_int_assignee(tmp_path, bad) -> None: _with_db(tmp_path, "create-bad-assignee.db")(_body) +def test_create_rejects_duplicate_assignees(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises( + ValueError, match="assignee_child_ids must not contain duplicate" + ): + await create_quest_definition( + database, + "Brush teeth", + _daily_rule(), + [child.id, child.id], + ["morning"], + ) + return None + + _with_db(tmp_path, "create-duplicate-assignees.db")(_body) + + def test_create_rejects_inactive_child(tmp_path) -> None: async def _body(database, children, rules, definitions, child): inactive = await children.create("Bo", NOW) @@ -265,6 +283,33 @@ def test_create_rejects_invalid_window(tmp_path, bad) -> None: _with_db(tmp_path, "create-bad-window.db")(_body) +def test_create_rejects_duplicate_windows(tmp_path) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises( + ValueError, match="windows must not contain duplicate" + ): + await create_quest_definition( + database, + "Brush teeth", + _daily_rule(), + [child.id], + ["morning", "morning"], + ) + with pytest.raises( + ValueError, match="windows must not contain duplicate" + ): + await create_quest_definition( + database, + "Brush teeth", + _daily_rule(), + [child.id], + [("morning", "09:00"), ("morning", "10:00")], + ) + return None + + _with_db(tmp_path, "create-duplicate-windows.db")(_body) + + @pytest.mark.parametrize("bad", ["9:30", "25:00", "09:60", "0900"]) def test_create_rejects_malformed_due_time(tmp_path, bad) -> None: async def _body(database, children, rules, definitions, child): @@ -351,3 +396,36 @@ def test_rejected_create_writes_nothing(tmp_path) -> None: return None _with_db(tmp_path, "create-rejected-nothing.db")(_body) + + +def test_post_write_failure_rolls_back_everything(tmp_path) -> None: + """A failure after some writes rolls all four tables back together. + + Deterministic post-write trigger: the DAO method inserts the rule, + the definition and the assignee rows, THEN validates the window name + and raises ValueError for a window outside const.QUEST_WINDOWS. + That failure lands after three writes inside the transaction, so the + rule, definition and assignee rows must all roll back — no partial + rows survive in any of the four tables. + """ + async def _body(database, children, rules, definitions, child): + storage = schedule_rule_to_storage(_daily_rule()) + dao = QuestDefinitionsDao(database) + with pytest.raises(ValueError, match="window must be one of"): + await dao.create_with_rule_and_windows( + "Brush teeth", + storage, + NOW, + [child.id], + [("noon", None)], + ) + # All four tables are empty: the earlier rule, definition and + # assignee writes were rolled back, and no window row was added. + assert await rules.get(1) is None + assert await definitions.get(1) is None + assert await definitions.list_active() == [] + assert await definitions.list_assignees(1) == [] + assert await definitions.list_windows(1) == [] + return None + + _with_db(tmp_path, "post-write-rollback.db")(_body) -- 2.55.0 From f478528c67c55bfdbaf3c7c06c2dbb4be45a30fe Mon Sep 17 00:00:00 2001 From: Joshua Date: Thu, 17 Sep 2026 08:35:55 -0400 Subject: [PATCH 3/4] fix: require list for assignee_child_ids and windows (NQ67-001) --- .../nestquest/quest_definitions.py | 44 ++++++++++--------- tests/test_quest_definitions.py | 32 +++++++++++++- 2 files changed, 53 insertions(+), 23 deletions(-) diff --git a/custom_components/nestquest/quest_definitions.py b/custom_components/nestquest/quest_definitions.py index 1efce70..1b5fa2b 100644 --- a/custom_components/nestquest/quest_definitions.py +++ b/custom_components/nestquest/quest_definitions.py @@ -13,17 +13,17 @@ Validation policy mirrors :mod:`.children`: - ``rule`` must be an already-validated :class:`~.recurrence.ScheduleRule` (its constructor is total — rule validation is NOT re-implemented here). -- ``assignee_child_ids`` is non-empty; every id is a plain int (bools - and floats rejected, since SQLite would bind them onto a real child) - and must reference an existing ACTIVE child (the DAO enforces the - active/exists check inside the same transaction). Duplicate ids are - rejected here, before the composite primary key could surface them as - an :class:`sqlite3.IntegrityError`. -- ``windows`` is non-empty; each entry is a window name from - :data:`~.const.QUEST_WINDOWS` — either a bare name (no due time) or a - ``(name, due_time)`` pair whose due time is a strict 24-hour HH:MM - string. Duplicate window names are rejected here, before the - composite primary key could surface them as an +- ``assignee_child_ids`` must be a non-empty list (strictly a ``list``); + every id is a plain int (bools and floats rejected, since SQLite would + bind them onto a real child) and must reference an existing ACTIVE + child (the DAO enforces the active/exists check inside the same + transaction). Duplicate ids are rejected here, before the composite + primary key could surface them as an :class:`sqlite3.IntegrityError`. +- ``windows`` must be a non-empty list (strictly a ``list``); each entry + is a window name from :data:`~.const.QUEST_WINDOWS` — either a bare + name (no due time) or a ``(name, due_time)`` pair whose due time is a + strict 24-hour HH:MM string. Duplicate window names are rejected + here, before the composite primary key could surface them as an :class:`sqlite3.IntegrityError`. The rule, definition, assignees and windows are persisted in ONE @@ -99,13 +99,14 @@ def _validate_text( def _validate_assignee_ids(value: object) -> list[int]: """Return the assignee ids as plain ints, or raise naming the field. - The list must be non-empty; each entry must be a real int (bools and - floats rejected) so SQLite cannot bind ``True`` onto child 1 or a - float onto a neighbouring profile. + The argument must be a non-empty list — strictly a ``list``: tuples + and other sequences are rejected. Each entry must be a real int + (bools and floats rejected) so SQLite cannot bind ``True`` onto + child 1 or a float onto a neighbouring profile. """ - if not isinstance(value, (list, tuple)): + if type(value) is not list: raise ValueError( - "assignee_child_ids must be a non-empty list of child ids" + f"assignee_child_ids must be a list, got {type(value).__name__}" ) if not value: raise ValueError("assignee_child_ids must not be empty") @@ -129,16 +130,17 @@ def _normalize_windows( ) -> list[tuple[str, str | None]]: """Normalize the windows argument to ``(name, due_time)`` pairs. - Each entry is a bare window name (due time None) or a - ``(name, due_time)`` pair. The name must be a + The outer argument must be a non-empty list — strictly a ``list``: + tuples and other sequences are rejected, while each individual entry + may still be a ``(window, due_time)`` tuple. A bare window name + (due time None) is also accepted. The name must be a :data:`~.const.QUEST_WINDOWS` spelling and a present due time must be a strict 24-hour HH:MM, both validated up front so a malformed window is rejected before any write. """ - if not isinstance(value, (list, tuple)): + if type(value) is not list: raise ValueError( - "windows must be a non-empty list of window names or " - "(name, due_time) pairs" + f"windows must be a list, got {type(value).__name__}" ) if not value: raise ValueError("windows must not be empty") diff --git a/tests/test_quest_definitions.py b/tests/test_quest_definitions.py index 3b1c7c6..2560dff 100644 --- a/tests/test_quest_definitions.py +++ b/tests/test_quest_definitions.py @@ -186,7 +186,7 @@ def test_create_rejects_non_schedule_rule(tmp_path, bad) -> None: _with_db(tmp_path, "create-bad-rule.db")(_body) -@pytest.mark.parametrize("bad", [[], ()]) +@pytest.mark.parametrize("bad", [[]]) def test_create_rejects_empty_assignees(tmp_path, bad) -> None: async def _body(database, children, rules, definitions, child): with pytest.raises( @@ -200,6 +200,20 @@ def test_create_rejects_empty_assignees(tmp_path, bad) -> None: _with_db(tmp_path, "create-empty-assignees.db")(_body) +@pytest.mark.parametrize("bad", [(1,), (1, 2), "1", {1}]) +def test_create_rejects_non_list_assignees(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises( + ValueError, match="assignee_child_ids must be a list" + ): + await create_quest_definition( + database, "Brush teeth", _daily_rule(), bad, ["morning"] + ) + return None + + _with_db(tmp_path, "create-non-list-assignees.db")(_body) + + @pytest.mark.parametrize("bad", [True, False, 1.5, "1", None]) def test_create_rejects_non_int_assignee(tmp_path, bad) -> None: async def _body(database, children, rules, definitions, child): @@ -259,7 +273,7 @@ def test_create_rejects_unknown_child(tmp_path) -> None: _with_db(tmp_path, "create-unknown-child.db")(_body) -@pytest.mark.parametrize("bad", [[], ()]) +@pytest.mark.parametrize("bad", [[]]) def test_create_rejects_empty_windows(tmp_path, bad) -> None: async def _body(database, children, rules, definitions, child): with pytest.raises(ValueError, match="windows must not be empty"): @@ -271,6 +285,20 @@ def test_create_rejects_empty_windows(tmp_path, bad) -> None: _with_db(tmp_path, "create-empty-windows.db")(_body) +@pytest.mark.parametrize( + "bad", [("morning",), ("morning", "afternoon"), "morning", {"morning"}] +) +def test_create_rejects_non_list_windows(tmp_path, bad) -> None: + async def _body(database, children, rules, definitions, child): + with pytest.raises(ValueError, match="windows must be a list"): + await create_quest_definition( + database, "Brush teeth", _daily_rule(), [child.id], bad + ) + return None + + _with_db(tmp_path, "create-non-list-windows.db")(_body) + + @pytest.mark.parametrize("bad", [["noon"], [("MORNING", None)], [("", None)]]) def test_create_rejects_invalid_window(tmp_path, bad) -> None: async def _body(database, children, rules, definitions, child): -- 2.55.0 From 624cb7a53e68b801818af0e48d3dbaa29dc2a674 Mon Sep 17 00:00:00 2001 From: Joshua Date: Thu, 17 Sep 2026 08:39:47 -0400 Subject: [PATCH 4/4] fix: name assignee_child_ids in child validation errors (NQ67-ERR-001) --- .../nestquest/quest_definitions.py | 28 +++++++++++++------ tests/test_quest_definitions.py | 8 ++++-- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/custom_components/nestquest/quest_definitions.py b/custom_components/nestquest/quest_definitions.py index 1b5fa2b..28283e1 100644 --- a/custom_components/nestquest/quest_definitions.py +++ b/custom_components/nestquest/quest_definitions.py @@ -200,15 +200,25 @@ async def create_quest_definition( storage = schedule_rule_to_storage(rule) dao = QuestDefinitionsDao(database) - snapshot = await dao.create_with_rule_and_windows( - name, - storage, - _now_stamp(), - assignee_ids, - window_specs, - description=description_value, - icon=icon_value, - ) + try: + snapshot = await dao.create_with_rule_and_windows( + name, + storage, + _now_stamp(), + assignee_ids, + window_specs, + description=description_value, + icon=icon_value, + ) + except ValueError as error: + # The DAO's assignee check names only the child; re-raise so the + # public contract names the field the caller actually passed. + message = str(error) + if message.startswith("child "): + raise ValueError( + f"assignee_child_ids: {message}" + ) from error + raise decoded_rule = schedule_rule_from_storage(storage) return CreatedQuestDefinition( definition=snapshot.definition, diff --git a/tests/test_quest_definitions.py b/tests/test_quest_definitions.py index 2560dff..1f99808 100644 --- a/tests/test_quest_definitions.py +++ b/tests/test_quest_definitions.py @@ -249,7 +249,9 @@ def test_create_rejects_inactive_child(tmp_path) -> None: async def _body(database, children, rules, definitions, child): inactive = await children.create("Bo", NOW) await children.set_active(inactive.id, False) - with pytest.raises(ValueError, match="inactive"): + with pytest.raises( + ValueError, match="assignee_child_ids.*inactive" + ): await create_quest_definition( database, "Brush teeth", @@ -264,7 +266,9 @@ def test_create_rejects_inactive_child(tmp_path) -> None: def test_create_rejects_unknown_child(tmp_path) -> None: async def _body(database, children, rules, definitions, child): - with pytest.raises(ValueError, match="does not exist"): + with pytest.raises( + ValueError, match="assignee_child_ids.*does not exist" + ): await create_quest_definition( database, "Brush teeth", _daily_rule(), [999], ["morning"] ) -- 2.55.0