From f00ec03c78f7e856349c6d6fc17820f568189756 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Mon, 14 Sep 2026 07:14:30 -0400 Subject: [PATCH] fix: address review findings - atomic admin add, reorder validation and serialization, honest rollback test, stronger SQL-leak guard - AdminUsersDao.add: single INSERT ON CONFLICT DO NOTHING (rowcount-derived result), race-safe; concurrent-add test with 5 racing coroutines - ChildrenDao.reorder: rejects duplicate ids (ValueError); per-instance asyncio lock serializes concurrent reorders (the wrapper rejects concurrent transactions); rollback test now forces a real finalize-batch failure and asserts original order survives - SQL-leak guard: scans package + tests with FROM/INTO/UPDATE/DELETE FROM/ JOIN pattern; narrowly justified exclusions (schema DDL, migrations, their tests); this test file verified to contain no direct table SQL --- custom_components/nestquest/dao_children.py | 74 +++++--- tests/test_dao_children.py | 183 +++++++++++++++++--- 2 files changed, 206 insertions(+), 51 deletions(-) diff --git a/custom_components/nestquest/dao_children.py b/custom_components/nestquest/dao_children.py index 7e94a51..699056b 100644 --- a/custom_components/nestquest/dao_children.py +++ b/custom_components/nestquest/dao_children.py @@ -16,6 +16,7 @@ the admin remove/set paths). """ from __future__ import annotations +import asyncio from dataclasses import dataclass from .db import NestQuestDatabase @@ -70,6 +71,10 @@ class ChildrenDao: def __init__(self, database) -> None: self._database = database + #: Serializes whole reorder() runs on this DAO instance: the + #: wrapper rejects concurrent transactions, so racing reorders + #: must queue here instead of colliding. + self._reorder_lock = asyncio.Lock() async def create( self, @@ -173,26 +178,42 @@ class ChildrenDao: 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. + 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. + + Concurrent reorder() calls on one connection are serialized by + a per-instance asyncio lock: the wrapper's 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. """ if not ordered_ids: return - async with self._database.transaction(): - offset = len(ordered_ids) - await self._database.execute_many( - "UPDATE children SET sort_order = ? WHERE id = ?", - [ - (-(offset + index), child_id) - for index, child_id in enumerate(ordered_ids) - ], - ) - await self._database.execute_many( - "UPDATE children SET sort_order = ? WHERE id = ?", - [ - (index, child_id) - for index, child_id in enumerate(ordered_ids) - ], + if len(set(ordered_ids)) != len(ordered_ids): + raise ValueError( + "reorder() received duplicate child ids; every id must " + "appear at most once" ) + async with self._reorder_lock: + async with self._database.transaction(): + offset = len(ordered_ids) + await self._database.execute_many( + "UPDATE children SET sort_order = ? WHERE id = ?", + [ + (-(offset + index), child_id) + for index, child_id in enumerate(ordered_ids) + ], + ) + await self._database.execute_many( + "UPDATE children SET sort_order = ? WHERE id = ?", + [ + (index, child_id) + for index, child_id in enumerate(ordered_ids) + ], + ) class AdminUsersDao: @@ -209,20 +230,19 @@ class AdminUsersDao: async def add(self, ha_user_id: str, added_at: str) -> bool: """Add a user to the allowlist; True if inserted, False if present. - Idempotent by design: allowlist management UIs re-adding an - existing admin must not raise. + Idempotent under concurrency: a single INSERT ... ON CONFLICT DO + NOTHING decides and inserts in one statement (the wrapper + serializes statements, not check-then-insert sequences, so a + SELECT/INSERT pair would let two concurrent adds both see no + row and the second one raise IntegrityError). rowcount reports + whether the insert actually happened. """ - existing = await self._database.fetch_one( - "SELECT 1 FROM admin_users WHERE ha_user_id = ?", - (ha_user_id,), - ) - if existing is not None: - return False - await self._database.execute( - "INSERT INTO admin_users (ha_user_id, added_at) VALUES (?, ?)", + result = await self._database.execute( + "INSERT INTO admin_users (ha_user_id, added_at) VALUES (?, ?) " + "ON CONFLICT (ha_user_id) DO NOTHING", (ha_user_id, added_at), ) - return True + return result.rowcount > 0 async def remove(self, ha_user_id: str) -> bool: """Remove a user from the allowlist; True if a row was removed.""" diff --git a/tests/test_dao_children.py b/tests/test_dao_children.py index 7c4273b..ed1e07c 100644 --- a/tests/test_dao_children.py +++ b/tests/test_dao_children.py @@ -251,24 +251,81 @@ def test_reorder_unknown_id_leaves_it_untouched(tmp_path) -> None: _run(database.close()) -def test_reorder_survives_partial_state_on_rollback(tmp_path) -> None: +def test_reorder_rolls_back_to_original_order_on_failure(tmp_path) -> None: + """A failing finalize batch rolls the whole reorder back. + + The two-phase staging is only meaningful if a crash between the + batches leaves the original order intact: monkeypatch execute_many + so the finalize (non-negative) batch raises, then assert every + sort_order is unchanged. + """ database = _open_db(tmp_path / "reorder-rollback.db") try: dao, _ = _daos(database) - a = _make_child(dao, "Ada") - b = _make_child(dao, "Bo") - _run(dao.reorder([b.id, a.id])) - # After a successful reorder no staged negative offsets remain. - rows = _run( - database.fetch_all( - "SELECT display_name, sort_order FROM children" - ) - ) - assert all(row[1] >= 0 for row in rows) + a = _make_child(dao, "Ada", sort_order=0) + b = _make_child(dao, "Bo", sort_order=1) + + original_execute_many = database.execute_many + + async def _failing_execute_many(sql, parameters): + # The staging batch writes negative positions; the finalize + # batch writes 0..n-1. Fail the finalize batch. + if all(position >= 0 for position, _ in parameters): + raise sqlite3.OperationalError("boom") + return await original_execute_many(sql, parameters) + + database.execute_many = _failing_execute_many + with pytest.raises(sqlite3.OperationalError, match="boom"): + _run(dao.reorder([b.id, a.id])) + children = _run(dao.list_all()) + assert [(c.display_name, c.sort_order) for c in children] == [ + ("Ada", 0), + ("Bo", 1), + ], "failed reorder must leave original sort orders intact" finally: _run(database.close()) +def test_reorder_rejects_duplicate_ids(tmp_path) -> None: + database = _open_db(tmp_path / "reorder-duplicate.db") + try: + dao, _ = _daos(database) + a = _make_child(dao, "Ada") + with pytest.raises(ValueError, match="duplicate"): + _run(dao.reorder([a.id, a.id])) + fetched = _run(dao.get(a.id)) + assert fetched is not None and fetched.sort_order == 0 + finally: + _run(database.close()) + + +def test_reorder_concurrent_calls_serialize(tmp_path) -> None: + """Two concurrent reorders queue on the transaction, both succeed. + + The second call sees the first call's final positions as its + starting state; both end with a consistent 0..n-1 assignment. + """ + async def _main() -> None: + database = NestQuestDatabase(_make_hass_mock()) + await database.open(tmp_path / "reorder-concurrent.db") + try: + await apply_migrations(database) + dao, _ = _daos(database) + a = await dao.create("Ada", NOW) + b = await dao.create("Bo", NOW) + task_1 = asyncio.ensure_future(dao.reorder([b.id, a.id])) + await asyncio.sleep(0) + task_2 = asyncio.ensure_future(dao.reorder([a.id, b.id])) + await asyncio.gather(task_1, task_2) + children = await dao.list_all() + orders = {c.display_name: c.sort_order for c in children} + assert sorted(orders.values()) == [0, 1] + finally: + await database.close() + + _run(_main()) + + # --------------------------------------------------------------------------- # admin_users: add, remove, list, exists # --------------------------------------------------------------------------- @@ -298,6 +355,35 @@ def test_admin_add_is_idempotent(tmp_path) -> None: _run(database.close()) +def test_admin_add_concurrent_calls_are_idempotent(tmp_path) -> None: + """Concurrent add() calls of the same user: one inserts, rest False. + + The single-statement ON CONFLICT DO NOTHING makes the decision and + the write atomic per statement, so racing coroutines cannot both + 'see no row' and one of them raise IntegrityError. + """ + async def _main() -> None: + database = NestQuestDatabase(_make_hass_mock()) + await database.open(tmp_path / "admin-race.db") + try: + await apply_migrations(database) + _, dao = _daos(database) + tasks = [ + asyncio.ensure_future(dao.add("user-1", NOW)) + for _ in range(5) + ] + await asyncio.sleep(0) + results = list(await asyncio.gather(*tasks)) + assert sorted(results) == [False, False, False, False, True] + records = await dao.list() + assert len(records) == 1 + assert records[0].added_at == NOW + finally: + await database.close() + + _run(_main()) + + def test_admin_add_preserves_original_added_at(tmp_path) -> None: database = _open_db(tmp_path / "admin-added-at.db") try: @@ -358,28 +444,77 @@ def test_admin_records_are_typed_not_raw_rows(tmp_path) -> None: # --------------------------------------------------------------------------- -# No SQL for these tables outside the module +# No SQL for these tables outside the DAO module # --------------------------------------------------------------------------- def test_children_and_admin_sql_lives_only_in_dao_module() -> None: + """Guardrail: children/admin_users SQL may appear only in the DAO + (and the schema DDL declarations). Scans the integration package + AND the repo's test files, matching FROM/INTO/UPDATE/DELETE FROM/ + JOIN against either table — including DELETE FROM children, the + hard-delete path the guardrail forbids. + + Exclusions are narrowly justified: schema.py declares the tables' + DDL; migrations.py applies that DDL; test_schema.py and + test_migrations.py test exactly those DDL/migration layers. None + of them touch the tables' data paths — that is the DAO's job. + """ import re from pathlib import Path package = Path( - __import__("custom_components.nestquest", fromlist=["__file__"]).__file__ + __import__( + "custom_components.nestquest", fromlist=["__file__"] + ).__file__ ).parent - offenders = [] - pattern = re.compile( - r"(FROM|INTO|UPDATE)\s+children\b|(FROM|INTO|DELETE FROM)\s+admin_users\b", + repo_root = package.parent.parent + scan_roots = [package, repo_root / "tests"] + + allowed = { + "dao_children.py", # the DAO itself + "schema.py", # declares the DDL + "migrations.py", # applies the DDL + "test_schema.py", # tests the DDL + "test_migrations.py", # tests migration application of the DDL + "test_dao_children.py", # this file, scanned separately below + } + sql_pattern = re.compile( + r"(FROM|INTO|UPDATE|DELETE\s+FROM|JOIN)\s+" + r'["\`]*(children|admin_users)["\`]*\b', re.IGNORECASE, ) - for py in package.glob("*.py"): - if py.name == "dao_children.py" or py.name == "schema.py": - continue - text = py.read_text() - if pattern.search(text): - offenders.append(py.name) + offenders: list[str] = [] + for root in scan_roots: + for py in sorted(root.glob("*.py")): + if py.name in allowed: + continue + if sql_pattern.search(py.read_text()): + offenders.append(py.name) assert offenders == [], ( - f"SQL for children/admin_users leaked into: {offenders}" - ) \ No newline at end of file + f"SQL touching children/admin_users leaked into: {offenders}" + ) + + +def test_dao_test_file_uses_dao_not_raw_table_sql() -> None: + """This test module must exercise the DAO, not raw SQL, for these + tables — except inside this guard itself. Any SQL statement naming + children or admin_users in a string literal elsewhere here is a + violation. + """ + import re + from pathlib import Path + + text = Path(__file__).read_text() + # Cut everything from the guard function to the end of file: the + # guard's own patterns mention the keywords in regex strings, not + # executable SQL. + head = text[: text.index("def test_children_and_admin_sql_lives")] + sql_pattern = re.compile( + r"(SELECT\s[^\"']*?FROM|INSERT\s+INTO|UPDATE|DELETE\s+FROM)\s+" + r'["\`]*(children|admin_users)\b', + re.IGNORECASE, + ) + assert sql_pattern.search(head) is None, ( + "tests must go through the DAO, not raw SQL, for these tables" + )