From f478528c67c55bfdbaf3c7c06c2dbb4be45a30fe Mon Sep 17 00:00:00 2001 From: Joshua Date: Thu, 17 Sep 2026 08:35:55 -0400 Subject: [PATCH] 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):