diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index 315fdf8..f4d8c3e 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -73,6 +73,18 @@ def _validate_sort_order(value: object) -> int: return value +def _validate_child_id(value: object) -> int: + """Reject non-int child ids (bools included) before any lookup. + + SQLite binds Python bools as integers, so ``True`` would silently + address child 1 and a float would round — malformed service input + must never mutate the wrong profile. + """ + if isinstance(value, bool) or not isinstance(value, int): + raise ValueError(f"child_id must be an integer, got {value!r}") + return value + + async def _warn_on_duplicate_name( database: NestQuestDatabase, display_name: str, @@ -166,6 +178,7 @@ async def edit_child( and its readback — the returned record is exactly the one this edit produced, not a later writer's. """ + _validate_child_id(child_id) async with _connection_lock(database): dao = ChildrenDao(database) existing = await dao.get(child_id) @@ -222,6 +235,7 @@ async def set_child_active( connection-scoped lock, so a concurrent opposite transition cannot make this call report the other caller's value. """ + _validate_child_id(child_id) async with _connection_lock(database): dao = ChildrenDao(database) if await dao.get(child_id) is None: @@ -236,6 +250,26 @@ async def set_child_active( return updated +async def reorder_children( + database: NestQuestDatabase, ordered_ids: list[int] +) -> None: + """Rewrite the children's panel display order in one transaction. + + ``ordered_ids`` must be a complete permutation of the children + table — every child id exactly once. A partial list (missing an + existing child), an unknown id, or a duplicate is rejected with + ValueError before any write, so the current order survives a + rejected call. The DAO enforces this inside its own transaction + (check and write are atomic under the connection lock); this + wrapper additionally rejects non-integer ids up front, since + ``bool`` and floats would silently compare against stored integer + ids at the SQL layer. + """ + for child_id in ordered_ids: + _validate_child_id(child_id) + await ChildrenDao(database).reorder(ordered_ids) + + async def list_children( database: NestQuestDatabase, *, active_only: bool = False ) -> list[ChildRecord]: diff --git a/custom_components/nestquest/dao_children.py b/custom_components/nestquest/dao_children.py index 88eb62c..57c42a8 100644 --- a/custom_components/nestquest/dao_children.py +++ b/custom_components/nestquest/dao_children.py @@ -191,13 +191,20 @@ class ChildrenDao: async def reorder(self, ordered_ids: list[int]) -> None: """Persist a new sort order: ``ordered_ids[i]`` gets sort_order i. + The list must be a COMPLETE permutation of the children table: + every existing child id appears exactly once. Partial lists + (missing an existing child) and unknown ids (naming a child + that does not exist) are rejected with ValueError BEFORE any + write, leaving the current order untouched — a silent partial + reorder would leave unlisted children with stale positions and + hide a caller bug. Duplicate ids are rejected up front: they + cannot all hold distinct positions, and silently assigning the + last occurrence's index would hide a caller bug. + Runs inside one transaction; positions are staged at offset -len (all negative, so any permutation of the same ids never collides mid-reorder under the wrapper's single-connection - serialization), then finalized 0..n-1. Duplicate ids are - rejected up front: they cannot all hold distinct positions, and - silently assigning the last occurrence's index would hide a - caller bug. + serialization), then finalized 0..n-1. Concurrent reorder() calls on one connection are serialized by a connection-scoped asyncio lock (shared across every ChildrenDao @@ -205,10 +212,10 @@ class ChildrenDao: transaction() rejects a second concurrent transaction with RuntimeError, so racing callers would crash instead of queueing. The lock makes the second call wait, then run - against the first call's final state. + against the first call's final state — which also keeps the + completeness check honest: the re-read of the table happens + inside the same lock and transaction as the writes. """ - if not ordered_ids: - return if len(set(ordered_ids)) != len(ordered_ids): raise ValueError( "reorder() received duplicate child ids; every id must " @@ -216,6 +223,29 @@ class ChildrenDao: ) async with _connection_lock(self._database): async with self._database.transaction(): + rows = await self._database.fetch_all( + f"SELECT id FROM children" + ) + existing = {row[0] for row in rows} + listed = set(ordered_ids) + missing = sorted(existing - listed) + unknown = sorted(listed - existing) + if missing or unknown: + details = [] + if missing: + details.append( + f"missing child ids {missing} (a partial " + "reorder would leave them with stale " + "positions)" + ) + if unknown: + details.append( + f"unknown child ids {unknown}" + ) + raise ValueError( + "reorder() must list every child exactly once; " + + "; ".join(details) + ) offset = len(ordered_ids) await self._database.execute_many( "UPDATE children SET sort_order = ? WHERE id = ?", diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index e3d28f6..6b84a18 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -12,6 +12,7 @@ from custom_components.nestquest.children import ( create_child, edit_child, list_children, + reorder_children, set_child_active, ) from custom_components.nestquest.dao_children import ChildRecord, ChildrenDao @@ -241,6 +242,33 @@ def test_edit_child_missing_raises(tmp_path) -> None: _with_db(tmp_path, "edit-missing.db")(_body) +@pytest.mark.parametrize("bad", [True, False, 1.0, "1", None]) +def test_edit_child_non_int_id_rejected(tmp_path, bad) -> None: + """SQLite binds bools as integers: True must not address child 1, + and floats must not round onto a real profile.""" + async def _body(database): + child = await create_child(database, "Ada") + with pytest.raises(ValueError, match="child_id must be an integer"): + await edit_child(database, bad, display_name="X") + # The refused edit left the profile untouched. + assert (await list_children(database))[0].display_name == "Ada" + return None + + _with_db(tmp_path, "edit-bad-id.db")(_body) + + +@pytest.mark.parametrize("bad", [True, False, 1.0, "1", None]) +def test_set_child_active_non_int_id_rejected(tmp_path, bad) -> None: + async def _body(database): + child = await create_child(database, "Ada") + with pytest.raises(ValueError, match="child_id must be an integer"): + await set_child_active(database, bad, False) + assert (await list_children(database))[0].is_active is True + return None + + _with_db(tmp_path, "set-active-bad-id.db")(_body) + + def test_edit_child_name_validation(tmp_path) -> None: async def _body(database): child = await create_child(database, "Ada") @@ -441,6 +469,114 @@ def test_set_child_active_missing_raises(tmp_path) -> None: _with_db(tmp_path, "set-active-missing.db")(_body) +# --------------------------------------------------------------------------- +# reorder_children: complete permutation or nothing changes +# --------------------------------------------------------------------------- + + +def test_reorder_children_persists_display_order(tmp_path) -> None: + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + second = await create_child(database, "Bo", sort_order=1) + third = await create_child(database, "Cleo", sort_order=2) + await reorder_children(database, [third.id, first.id, second.id]) + # The active list returns children in the new sort order. + listed = await list_children(database, active_only=True) + assert [c.id for c in listed] == [third.id, first.id, second.id] + assert [c.sort_order for c in listed] == [0, 1, 2] + return listed + + _with_db(tmp_path, "reorder-round-trip.db")(_body) + + +def test_reorder_children_partial_list_rejected_order_unchanged( + tmp_path, +) -> None: + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + second = await create_child(database, "Bo", sort_order=1) + third = await create_child(database, "Cleo", sort_order=2) + with pytest.raises(ValueError, match="missing child ids"): + await reorder_children(database, [third.id, first.id]) + listed = await list_children(database) + # Nothing changed: rejection happens before any write. + assert [c.id for c in listed] == [first.id, second.id, third.id] + assert [c.sort_order for c in listed] == [0, 1, 2] + return listed + + _with_db(tmp_path, "reorder-partial.db")(_body) + + +def test_reorder_children_unknown_id_rejected_order_unchanged( + tmp_path, +) -> None: + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + second = await create_child(database, "Bo", sort_order=1) + with pytest.raises(ValueError, match="unknown child ids"): + await reorder_children(database, [second.id, first.id, 999]) + listed = await list_children(database) + assert [c.id for c in listed] == [first.id, second.id] + assert [c.sort_order for c in listed] == [0, 1] + return listed + + _with_db(tmp_path, "reorder-unknown.db")(_body) + + +def test_reorder_children_empty_list_rejected_when_children_exist( + tmp_path, +) -> None: + """An empty list is the maximally partial reorder: rejected while + any child exists (against an empty table it is a complete + permutation and stays a harmless no-op).""" + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + with pytest.raises(ValueError, match="missing child ids"): + await reorder_children(database, []) + assert [c.sort_order for c in await list_children(database)] == [0] + return None + + _with_db(tmp_path, "reorder-empty.db")(_body) + + +def test_reorder_children_duplicate_and_non_int_rejected(tmp_path) -> None: + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + second = await create_child(database, "Bo", sort_order=1) + with pytest.raises(ValueError, match="duplicate child ids"): + await reorder_children(database, [first.id, first.id, second.id]) + with pytest.raises(ValueError, match="child_id must be an integer"): + await reorder_children(database, [first.id, second.id, True]) + with pytest.raises(ValueError, match="child_id must be an integer"): + await reorder_children(database, [first.id, second.id, "3"]) + listed = await list_children(database) + assert [c.sort_order for c in listed] == [0, 1] + return listed + + _with_db(tmp_path, "reorder-bad-inputs.db")(_body) + + +def test_reorder_children_inactive_children_included(tmp_path) -> None: + """Inactive children keep their display position: the permutation + covers the whole table, not just the active list.""" + async def _body(database): + first = await create_child(database, "Ada", sort_order=0) + second = await create_child(database, "Bo", sort_order=1) + await set_child_active(database, second.id, False) + # Both ids are required even though one is inactive. + await reorder_children(database, [second.id, first.id]) + listed = await list_children(database) + assert [c.id for c in listed] == [second.id, first.id] + assert [c.sort_order for c in listed] == [0, 1] + # The active list skips the inactive child but keeps position. + assert [c.id for c in await list_children(database, active_only=True)] == [ + first.id + ] + return listed + + _with_db(tmp_path, "reorder-inactive.db")(_body) + + @pytest.mark.parametrize("bad", ["1", "0", 1, 0, 1.0, None, "true"]) def test_set_child_active_rejects_non_bool(tmp_path, bad) -> None: """The DAO's int() would silently coerce "1"/0/1.0 into a state the diff --git a/tests/test_dao_children.py b/tests/test_dao_children.py index e3d78b0..893b855 100644 --- a/tests/test_dao_children.py +++ b/tests/test_dao_children.py @@ -227,24 +227,34 @@ def test_reorder_is_transactional_and_idempotent(tmp_path) -> None: _run(database.close()) -def test_reorder_empty_list_is_noop(tmp_path) -> None: +def test_reorder_empty_list_is_rejected(tmp_path) -> None: + """An empty list is the maximally partial reorder: rejected while + any child exists (Feature 03 reorder contract).""" + import pytest as pytest_module + database = _open_db(tmp_path / "reorder-empty.db") try: dao, _ = _daos(database) _make_child(dao, "Ada") - _run(dao.reorder([])) + with pytest_module.raises(ValueError, match="missing child ids"): + _run(dao.reorder([])) children = _run(dao.list_all()) assert children[0].sort_order == 0 finally: _run(database.close()) -def test_reorder_unknown_id_leaves_it_untouched(tmp_path) -> None: +def test_reorder_unknown_id_is_rejected(tmp_path) -> None: + """An unknown id is rejected before any write; the order is + untouched (Feature 03 reorder contract).""" + import pytest as pytest_module + database = _open_db(tmp_path / "reorder-unknown.db") try: dao, _ = _daos(database) a = _make_child(dao, "Ada") - _run(dao.reorder([a.id, 999])) + with pytest_module.raises(ValueError, match="unknown child ids"): + _run(dao.reorder([a.id, 999])) children = _run(dao.list_all()) assert children[0].sort_order == 0 finally: