From 61b77cc2dbaba5f881fb07fcbea0b58e72a27acc Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 12:33:39 -0400 Subject: [PATCH 1/5] Child-profile business layer (Feature 03, task 1) Business-layer functions over ChildrenDao for creating and editing child profiles, so the Feature 09 service gate never touches the DAO directly. - children.py: create_child (display name required and trimmed, whitespace-only rejected; duplicate display name allowed but logged as a warning, case-insensitive and whitespace-normalised on both sides), edit_child (_UNSET sentinel for leave-alone; returns the updated record; renaming onto a different child's name warns, own name does not), set_child_active (the only removal path), and list_children (display order). Explicit None for optional fields raises instead of silently no-oping: the DAO's update deliberately writes only provided fields and cannot express NULL, so a clear request must not masquerade as one. UTC second-precision stamps per the timestamp policy. - tests/test_children_registry.py: 27 tests covering the typed round-trips, name validation matrix, duplicate-name warning behaviour (create and rename, including the rename-to-self non-warning), partial edits, refusal of unsupported clears, deactivation round-trip and list ordering. --- custom_components/nestquest/children.py | 210 ++++++++++++++ tests/test_children_registry.py | 350 ++++++++++++++++++++++++ 2 files changed, 560 insertions(+) create mode 100644 custom_components/nestquest/children.py create mode 100644 tests/test_children_registry.py diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py new file mode 100644 index 0000000..44e2007 --- /dev/null +++ b/custom_components/nestquest/children.py @@ -0,0 +1,210 @@ +"""Business layer for the children registry (Feature 03). + +Sits between the Feature 09 service gate and the typed +:class:`~.dao_children.ChildrenDao`: callers get validation, name +policies and the duplicate-name warning here, and never talk to the +DAO or SQL directly. Children are never hard-deleted — deactivation +is a separate explicit operation (:func:`set_child_active`), so +completion history keeps its references (feature guardrail). + +Name policy: the display name is required and trimmed; an empty or +whitespace-only name is rejected with ValueError. A duplicate display +name is ALLOWED (two children may legitimately share a nickname) but +logged as a warning so an accidental double-add is visible in the HA +log. The duplicate check is case-insensitive and whitespace-normalised +on both sides, because "ada" and "Ada " are the same name to a parent. + +Colour and avatar are optional free-form strings, trimmed when present; +``sort_order`` is an int with the DAO/schema default of 0. Clearing an +optional field back to NULL is deliberately rejected: the DAO's update +writes only provided fields (it cannot express NULL), so the business +layer refuses an explicit ``None`` instead of silently ignoring it. +Timestamps follow the module timestamp policy: the business layer +stamps UTC ISO-8601 at second precision so every row it creates is +uniform. +""" +from __future__ import annotations + +import datetime +import logging + +from .dao_children import ChildRecord, ChildrenDao +from .db import NestQuestDatabase + +LOGGER = logging.getLogger(__name__) + +#: Sentinel distinguishing "argument omitted" from "explicit SQL NULL" +#: in :func:`edit_child`. +_UNSET = object() + + +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 (whitespace-only names are empty to a parent). + """ + 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_sort_order(value: object) -> int: + """Reject non-int sort orders (bools included) before the DAO.""" + if isinstance(value, bool) or not isinstance(value, int): + raise ValueError(f"sort_order must be an integer, got {value!r}") + return value + + +async def _warn_on_duplicate_name( + database: NestQuestDatabase, display_name: str +) -> None: + """Log a warning when ``display_name`` duplicates an existing child. + + Case-insensitive and whitespace-normalised on both sides. The + duplicate is still allowed — this only makes it visible; the log + line names both rows so an accidental double-add can be found. + """ + lowered = display_name.casefold() + for child in await ChildrenDao(database).list_all(): + if child.display_name.strip().casefold() == lowered: + LOGGER.warning( + "Child display name %r duplicates existing child %d " + "(%r); allowed, but check this was intentional", + display_name, + child.id, + child.display_name, + ) + return + + +async def create_child( + database: NestQuestDatabase, + display_name: str, + *, + colour: str | None = None, + avatar_ref: str | None = None, + sort_order: int = 0, +) -> ChildRecord: + """Create a child profile and return the stored record. + + The display name is required, trimmed, and rejected when empty. + A duplicate display name (case-insensitive) is allowed but logged + as a warning. The duplicate scan and the INSERT are deliberately + not one transaction: the warning is advisory, so serialising + creates to protect it would buy nothing. + """ + name = _validate_text(display_name, "display_name", required=True) + assert name is not None + colour_value = _validate_text(colour, "colour") + avatar_value = _validate_text(avatar_ref, "avatar_ref") + order = _validate_sort_order(sort_order) + await _warn_on_duplicate_name(database, name) + return await ChildrenDao(database).create( + name, + _now_stamp(), + colour=colour_value, + avatar_ref=avatar_value, + sort_order=order, + ) + + +async def edit_child( + database: NestQuestDatabase, + child_id: int, + *, + display_name: str | object = _UNSET, + colour: str | None | object = _UNSET, + avatar_ref: str | None | object = _UNSET, + sort_order: int | object = _UNSET, +) -> ChildRecord: + """Edit a child profile and return the updated record. + + Arguments default to the module sentinel ``_UNSET`` meaning "leave + this field alone". An explicitly passed ``None`` for an optional + field raises: the DAO's update writes only provided fields and + cannot express NULL, so a silent no-op would mask the caller's + intent (see the module docstring). A provided display name is + validated like create's and, when it duplicates a DIFFERENT child, + still allowed with a warning. Raises ValueError when the child + does not exist. + """ + dao = ChildrenDao(database) + existing = await dao.get(child_id) + if existing is None: + raise ValueError(f"child {child_id} does not exist") + updates: dict[str, object] = {} + if display_name is not _UNSET: + name = _validate_text(display_name, "display_name", required=True) + assert name is not None + if name.casefold() != existing.display_name.strip().casefold(): + await _warn_on_duplicate_name(database, name) + updates["display_name"] = name + if colour is not _UNSET: + value = _validate_text(colour, "colour") + if value is None: + raise ValueError( + "clearing colour is not supported; pass a replacement " + "value or omit the field" + ) + updates["colour"] = value + if avatar_ref is not _UNSET: + value = _validate_text(avatar_ref, "avatar_ref") + if value is None: + raise ValueError( + "clearing avatar_ref is not supported; pass a " + "replacement value or omit the field" + ) + updates["avatar_ref"] = value + if sort_order is not _UNSET: + updates["sort_order"] = _validate_sort_order(sort_order) + if updates: + await dao.update(child_id, **updates) + edited = await dao.get(child_id) + assert edited is not None + return edited + + +async def set_child_active( + database: NestQuestDatabase, child_id: int, is_active: bool +) -> ChildRecord: + """Deactivate or reactivate a child; returns the updated record. + + Deactivation hides the child from the panel without deleting their + history (feature guardrail); this is the only removal path. + Raises ValueError when the child does not exist. + """ + dao = ChildrenDao(database) + if await dao.get(child_id) is None: + raise ValueError(f"child {child_id} does not exist") + await dao.set_active(child_id, is_active) + updated = await dao.get(child_id) + assert updated is not None + return updated + + +async def list_children( + database: NestQuestDatabase, *, active_only: bool = False +) -> list[ChildRecord]: + """Return children in display order (sort_order, then id).""" + dao = ChildrenDao(database) + if active_only: + return await dao.list_active() + return await dao.list_all() diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py new file mode 100644 index 0000000..d88209d --- /dev/null +++ b/tests/test_children_registry.py @@ -0,0 +1,350 @@ +"""Tests for children.py: the Feature 03 child-profile business layer.""" +from __future__ import annotations + +import asyncio +import logging +import re +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from custom_components.nestquest.children import ( + create_child, + edit_child, + list_children, + set_child_active, +) +from custom_components.nestquest.dao_children import ChildRecord +from custom_components.nestquest.db import NestQuestDatabase +from custom_components.nestquest.migrations import apply_migrations + + +def _run(coro): + return asyncio.new_event_loop().run_until_complete(coro) + + +def _make_hass_mock(): + hass = MagicMock() + hass.async_add_executor_job = AsyncMock(side_effect=(lambda fn, *a: fn(*a))) + return hass + + +def _with_db(tmp_path, name): + def _run_test(body): + async def _main(): + database = NestQuestDatabase(_make_hass_mock()) + await database.open(tmp_path / name) + try: + await apply_migrations(database) + return await body(database) + finally: + await database.close() + + return _run(_main()) + + return _run_test + + +# --------------------------------------------------------------------------- +# create_child: required, trimmed display name +# --------------------------------------------------------------------------- + + +def test_create_child_returns_typed_record(tmp_path) -> None: + async def _body(database): + child = await create_child( + database, + " Declan ", + colour="#2E7D32", + avatar_ref="avatars/declan.png", + sort_order=2, + ) + assert isinstance(child, ChildRecord) + # The name is trimmed before storage. + assert child.display_name == "Declan" + assert child.colour == "#2E7D32" + assert child.avatar_ref == "avatars/declan.png" + assert child.sort_order == 2 + assert child.is_active is True + # The business layer stamps created_at in strict UTC shape. + assert re.fullmatch( + r"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\+00:00", child.created_at + ) + return child + + _with_db(tmp_path, "create-typed.db")(_body) + + +def test_create_child_defaults(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Chloe") + assert child.display_name == "Chloe" + assert child.colour is None + assert child.avatar_ref is None + assert child.sort_order == 0 + return child + + _with_db(tmp_path, "create-defaults.db")(_body) + + +@pytest.mark.parametrize("bad", ["", " ", "\t\n", None]) +def test_create_child_missing_name_rejected(tmp_path, bad) -> None: + async def _body(database): + with pytest.raises(ValueError, match="display_name is required"): + await create_child(database, bad) + assert await list_children(database) == [] + return None + + _with_db(tmp_path, "create-empty-name.db")(_body) + + +@pytest.mark.parametrize("bad", [42, ["Ada"], 3.5]) +def test_create_child_non_string_name_rejected(tmp_path, bad) -> None: + async def _body(database): + with pytest.raises(ValueError, match="must be a string"): + await create_child(database, bad) + return None + + _with_db(tmp_path, "create-bad-name-type.db")(_body) + + +def test_create_child_optional_text_fields_validated(tmp_path) -> None: + async def _body(database): + with pytest.raises(ValueError, match="colour must be a string"): + await create_child(database, "Ada", colour=7) + with pytest.raises(ValueError, match="avatar_ref must be a string"): + await create_child(database, "Ada", avatar_ref=1.5) + return None + + _with_db(tmp_path, "create-bad-optional.db")(_body) + + +@pytest.mark.parametrize("bad", ["2", 1.5, True, None]) +def test_create_child_sort_order_must_be_int(tmp_path, bad) -> None: + async def _body(database): + with pytest.raises(ValueError, match="sort_order must be an integer"): + await create_child(database, "Ada", sort_order=bad) + return None + + _with_db(tmp_path, "create-bad-order.db")(_body) + + +# --------------------------------------------------------------------------- +# create_child: duplicate display name allowed but warned +# --------------------------------------------------------------------------- + + +def test_create_child_duplicate_name_allowed_and_warned( + tmp_path, caplog +) -> None: + async def _body(database): + first = await create_child(database, "Ada") + with caplog.at_level( + logging.WARNING, logger="custom_components.nestquest.children" + ): + second = await create_child(database, " ADA ") + # Whitespace/case-normalised duplicate: allowed, both rows exist. + assert first.display_name == "Ada" + assert second.display_name == "ADA" + names = [c.display_name for c in await list_children(database)] + assert names == ["Ada", "ADA"] + warnings = [ + r for r in caplog.records if "duplicates existing child" in r.getMessage() + ] + assert len(warnings) == 1, ( + "exactly one duplicate warning for the second create" + ) + assert "Ada" in warnings[0].getMessage() + return second + + _with_db(tmp_path, "create-duplicate.db")(_body) + + +def test_create_child_distinct_name_logs_no_warning( + tmp_path, caplog +) -> None: + async def _body(database): + await create_child(database, "Ada") + caplog.clear() + await create_child(database, "Jordyn") + warnings = [ + r for r in caplog.records if "duplicates existing child" in r.getMessage() + ] + assert warnings == [] + return None + + _with_db(tmp_path, "create-no-duplicate.db")(_body) + + +# --------------------------------------------------------------------------- +# edit_child: returns the updated record +# --------------------------------------------------------------------------- + + +def test_edit_child_updates_fields_and_returns_record(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Ada", colour="#111111") + edited = await edit_child( + database, + child.id, + display_name=" Ada L. ", + colour="#222222", + avatar_ref="avatars/ada.png", + sort_order=3, + ) + assert isinstance(edited, ChildRecord) + assert edited.id == child.id + assert edited.display_name == "Ada L." + assert edited.colour == "#222222" + assert edited.avatar_ref == "avatars/ada.png" + assert edited.sort_order == 3 + # The stored row reflects every edit. + stored = await list_children(database) + assert len(stored) == 1 + assert stored[0] == edited + return edited + + _with_db(tmp_path, "edit-round-trip.db")(_body) + + +def test_edit_child_omitted_fields_unchanged(tmp_path) -> None: + async def _body(database): + child = await create_child( + database, "Ada", colour="#111111", sort_order=5 + ) + edited = await edit_child(database, child.id, sort_order=9) + # Only sort_order moved; colour and name untouched. + assert edited.sort_order == 9 + assert edited.colour == "#111111" + assert edited.display_name == "Ada" + return edited + + _with_db(tmp_path, "edit-partial.db")(_body) + + +def test_edit_child_no_fields_returns_current_record(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Ada", colour="#111111") + edited = await edit_child(database, child.id) + assert edited == child + return edited + + _with_db(tmp_path, "edit-noop.db")(_body) + + +def test_edit_child_missing_raises(tmp_path) -> None: + async def _body(database): + with pytest.raises(ValueError, match="does not exist"): + await edit_child(database, 999, display_name="X") + return None + + _with_db(tmp_path, "edit-missing.db")(_body) + + +def test_edit_child_name_validation(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Ada") + for bad in ("", " ", None, 42): + with pytest.raises(ValueError, match="display_name"): + await edit_child(database, child.id, display_name=bad) + # The failed edits left the record untouched. + assert (await list_children(database))[0].display_name == "Ada" + return None + + _with_db(tmp_path, "edit-name-validation.db")(_body) + + +def test_edit_child_sort_order_must_be_int(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Ada") + with pytest.raises(ValueError, match="sort_order must be an integer"): + await edit_child(database, child.id, sort_order="3") + return None + + _with_db(tmp_path, "edit-bad-order.db")(_body) + + +def test_edit_child_explicit_none_clear_rejected(tmp_path) -> None: + """Clearing is refused loudly, not silently ignored: the DAO cannot + express NULL writes, so an explicit None must never masquerade as a + no-op.""" + async def _body(database): + child = await create_child(database, "Ada", colour="#111111") + with pytest.raises(ValueError, match="clearing colour"): + await edit_child(database, child.id, colour=None) + with pytest.raises(ValueError, match="clearing avatar_ref"): + await edit_child(database, child.id, avatar_ref=None) + # The refused clears left the colour intact. + assert (await list_children(database))[0].colour == "#111111" + return None + + _with_db(tmp_path, "edit-clear-unsupported.db")(_body) + + +def test_edit_child_rename_to_duplicate_warns_but_different_name_not( + tmp_path, caplog +) -> None: + async def _body(database): + ada = await create_child(database, "Ada") + bo = await create_child(database, "Bo") + caplog.clear() + # Renaming Bo onto Ada's name warns (different child's name). + edited = await edit_child(database, bo.id, display_name="ada") + assert edited.display_name == "ada" + assert any( + "duplicates existing child" in r.getMessage() + for r in caplog.records + ) + caplog.clear() + # Renaming a child to their own name (casing aside) is not a + # duplicate: no warning. + await edit_child(database, ada.id, display_name="ADA") + assert not any( + "duplicates existing child" in r.getMessage() + for r in caplog.records + ) + return None + + _with_db(tmp_path, "edit-rename-duplicate.db")(_body) + + +# --------------------------------------------------------------------------- +# set_child_active / list_children +# --------------------------------------------------------------------------- + + +def test_set_child_active_round_trip(tmp_path) -> None: + async def _body(database): + child = await create_child(database, "Ada") + deactivated = await set_child_active(database, child.id, False) + assert deactivated.is_active is False + assert [c.id for c in await list_children(database)] == [child.id] + assert await list_children(database, active_only=True) == [] + reactivated = await set_child_active(database, child.id, True) + assert reactivated.is_active is True + assert len(await list_children(database, active_only=True)) == 1 + return reactivated + + _with_db(tmp_path, "set-active.db")(_body) + + +def test_set_child_active_missing_raises(tmp_path) -> None: + async def _body(database): + with pytest.raises(ValueError, match="does not exist"): + await set_child_active(database, 999, False) + return None + + _with_db(tmp_path, "set-active-missing.db")(_body) + + +def test_list_children_orders_by_sort_order_then_id(tmp_path) -> None: + async def _body(database): + second = await create_child(database, "Bo", sort_order=1) + first = await create_child(database, "Ada", sort_order=0) + third = await create_child(database, "Cleo", sort_order=1) + listed = await list_children(database) + # sort_order 0 first, then the two sort_order-1 rows by id. + assert [c.id for c in listed] == [first.id, second.id, third.id] + return listed + + _with_db(tmp_path, "list-order.db")(_body) From 712527d5baba9d0d24020a7847601c6b3df060c4 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 14:08:24 -0400 Subject: [PATCH 2/5] Review fix: duplicate-name scan excludes only the edited child Review round 1 (Codex gpt-5.6-terra) found: a case-only edit could suppress the duplicate-name warning even when a DIFFERENT child already carried the normalised name, because the edit path compared only against the child's own old name. The scan now always runs on the resulting state and excludes only the child being edited (exclude_child_id), so case-only edits and stay-duplicated renames warn like any other collision; renaming to your own name stays warning-free when no other child holds the name. Tests: new case-only-collision test, stay-duplicated rename assertion, and a split self-rename test; suite at 848 passed. --- custom_components/nestquest/children.py | 21 ++++++--- tests/test_children_registry.py | 58 +++++++++++++++++++++---- 2 files changed, 65 insertions(+), 14 deletions(-) diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index 44e2007..c0b6790 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -74,16 +74,25 @@ def _validate_sort_order(value: object) -> int: async def _warn_on_duplicate_name( - database: NestQuestDatabase, display_name: str + database: NestQuestDatabase, + display_name: str, + *, + exclude_child_id: int | None = None, ) -> None: """Log a warning when ``display_name`` duplicates an existing child. Case-insensitive and whitespace-normalised on both sides. The duplicate is still allowed — this only makes it visible; the log line names both rows so an accidental double-add can be found. + ``exclude_child_id`` removes the child being edited from the scan, + so an edit only warns when the result collides with a DIFFERENT + child's name — including case-only edits, where the self-comparison + would otherwise hide a real duplicate. """ lowered = display_name.casefold() for child in await ChildrenDao(database).list_all(): + if exclude_child_id is not None and child.id == exclude_child_id: + continue if child.display_name.strip().casefold() == lowered: LOGGER.warning( "Child display name %r duplicates existing child %d " @@ -142,9 +151,10 @@ async def edit_child( field raises: the DAO's update writes only provided fields and cannot express NULL, so a silent no-op would mask the caller's intent (see the module docstring). A provided display name is - validated like create's and, when it duplicates a DIFFERENT child, - still allowed with a warning. Raises ValueError when the child - does not exist. + validated like create's; the duplicate scan excludes only the child + being edited, so a case-only edit colliding with another child's + normalised name still warns. Raises ValueError when the child does + not exist. """ dao = ChildrenDao(database) existing = await dao.get(child_id) @@ -154,8 +164,7 @@ async def edit_child( if display_name is not _UNSET: name = _validate_text(display_name, "display_name", required=True) assert name is not None - if name.casefold() != existing.display_name.strip().casefold(): - await _warn_on_duplicate_name(database, name) + await _warn_on_duplicate_name(database, name, exclude_child_id=child_id) updates["display_name"] = name if colour is not _UNSET: value = _validate_text(colour, "colour") diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index d88209d..3aa874d 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -291,21 +291,63 @@ def test_edit_child_rename_to_duplicate_warns_but_different_name_not( # Renaming Bo onto Ada's name warns (different child's name). edited = await edit_child(database, bo.id, display_name="ada") assert edited.display_name == "ada" - assert any( - "duplicates existing child" in r.getMessage() - for r in caplog.records - ) + warnings = [ + r for r in caplog.records if "duplicates existing child" in r.getMessage() + ] + assert len(warnings) == 1 + assert "Ada" in warnings[0].getMessage() + return edited + + _with_db(tmp_path, "edit-rename-duplicate.db")(_body) + + +def test_edit_child_rename_to_own_name_casing_alone_warns_not( + tmp_path, caplog +) -> None: + """Renaming a child to their own name (casing aside) is not a + duplicate when NO OTHER child carries the normalised name.""" + async def _body(database): + ada = await create_child(database, "Ada") caplog.clear() - # Renaming a child to their own name (casing aside) is not a - # duplicate: no warning. - await edit_child(database, ada.id, display_name="ADA") + edited = await edit_child(database, ada.id, display_name="ADA") + assert edited.display_name == "ADA" assert not any( "duplicates existing child" in r.getMessage() for r in caplog.records ) + return edited + + _with_db(tmp_path, "edit-rename-self.db")(_body) + + +def test_edit_child_case_only_edit_colliding_with_other_child_warns( + tmp_path, caplog +) -> None: + """A case-only edit must not hide a real duplicate: with two Ada + profiles already present, editing either one's name to another + casing still warns (the scan excludes only the edited child).""" + async def _body(database): + first = await create_child(database, "Ada") + await create_child(database, "ada") + caplog.clear() + edited = await edit_child(database, first.id, display_name="ADA") + assert edited.display_name == "ADA" + warnings = [ + r for r in caplog.records if "duplicates existing child" in r.getMessage() + ] + assert len(warnings) == 1 + # Editing the name to a value that STAYS duplicated (no change, + # other child still carries it) warns too: the scan is on the + # resulting state, not on whether the word moved. + caplog.clear() + await edit_child(database, first.id, display_name=" ada ") + warnings = [ + r for r in caplog.records if "duplicates existing child" in r.getMessage() + ] + assert len(warnings) == 1 return None - _with_db(tmp_path, "edit-rename-duplicate.db")(_body) + _with_db(tmp_path, "edit-case-only-duplicate.db")(_body) # --------------------------------------------------------------------------- From 5578507ce898a28942e709a948247b151d43c6d0 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 14:13:48 -0400 Subject: [PATCH 3/5] Review fix: edit_child/set_child_active update+readback atomic Review round 2 (Codex gpt-5.6-terra) found both functions returned a readback that a concurrent writer could overwrite between the UPDATE and the SELECT, so the returned record could be the other caller's values. Both now run existence-check, duplicate scan, UPDATE and readback inside the DAOs' shared connection-scoped lock. Two gated concurrency tests prove the contract deterministically: task A paused at its UPDATE while task B queues, each returns its own values, final stored state is the last writer's. Suite: 850 passed. --- custom_components/nestquest/children.py | 86 +++++++++------- tests/test_children_registry.py | 125 ++++++++++++++++++++++++ 2 files changed, 176 insertions(+), 35 deletions(-) diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index c0b6790..e3439aa 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -28,7 +28,7 @@ from __future__ import annotations import datetime import logging -from .dao_children import ChildRecord, ChildrenDao +from .dao_children import ChildRecord, ChildrenDao, _connection_lock from .db import NestQuestDatabase LOGGER = logging.getLogger(__name__) @@ -155,38 +155,49 @@ async def edit_child( being edited, so a case-only edit colliding with another child's normalised name still warns. Raises ValueError when the child does not exist. + + Atomicity: the existence check, the duplicate scan, the UPDATE and + the readback all run inside the connection-scoped lock shared with + the DAOs, so another edit cannot slip between this call's UPDATE + and its readback — the returned record is exactly the one this + edit produced, not a later writer's. """ - dao = ChildrenDao(database) - existing = await dao.get(child_id) - if existing is None: - raise ValueError(f"child {child_id} does not exist") - updates: dict[str, object] = {} - if display_name is not _UNSET: - name = _validate_text(display_name, "display_name", required=True) - assert name is not None - await _warn_on_duplicate_name(database, name, exclude_child_id=child_id) - updates["display_name"] = name - if colour is not _UNSET: - value = _validate_text(colour, "colour") - if value is None: - raise ValueError( - "clearing colour is not supported; pass a replacement " - "value or omit the field" + async with _connection_lock(database): + dao = ChildrenDao(database) + existing = await dao.get(child_id) + if existing is None: + raise ValueError(f"child {child_id} does not exist") + updates: dict[str, object] = {} + if display_name is not _UNSET: + name = _validate_text( + display_name, "display_name", required=True ) - updates["colour"] = value - if avatar_ref is not _UNSET: - value = _validate_text(avatar_ref, "avatar_ref") - if value is None: - raise ValueError( - "clearing avatar_ref is not supported; pass a " - "replacement value or omit the field" + assert name is not None + await _warn_on_duplicate_name( + database, name, exclude_child_id=child_id ) - updates["avatar_ref"] = value - if sort_order is not _UNSET: - updates["sort_order"] = _validate_sort_order(sort_order) - if updates: - await dao.update(child_id, **updates) - edited = await dao.get(child_id) + updates["display_name"] = name + if colour is not _UNSET: + value = _validate_text(colour, "colour") + if value is None: + raise ValueError( + "clearing colour is not supported; pass a replacement " + "value or omit the field" + ) + updates["colour"] = value + if avatar_ref is not _UNSET: + value = _validate_text(avatar_ref, "avatar_ref") + if value is None: + raise ValueError( + "clearing avatar_ref is not supported; pass a " + "replacement value or omit the field" + ) + updates["avatar_ref"] = value + if sort_order is not _UNSET: + updates["sort_order"] = _validate_sort_order(sort_order) + if updates: + await dao.update(child_id, **updates) + edited = await dao.get(child_id) assert edited is not None return edited @@ -199,12 +210,17 @@ async def set_child_active( Deactivation hides the child from the panel without deleting their history (feature guardrail); this is the only removal path. Raises ValueError when the child does not exist. + + Atomicity: existence check, UPDATE and readback run inside the + connection-scoped lock, so a concurrent opposite transition cannot + make this call report the other caller's value. """ - dao = ChildrenDao(database) - if await dao.get(child_id) is None: - raise ValueError(f"child {child_id} does not exist") - await dao.set_active(child_id, is_active) - updated = await dao.get(child_id) + async with _connection_lock(database): + dao = ChildrenDao(database) + if await dao.get(child_id) is None: + raise ValueError(f"child {child_id} does not exist") + await dao.set_active(child_id, is_active) + updated = await dao.get(child_id) assert updated is not None return updated diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index 3aa874d..e2af6cf 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -379,6 +379,131 @@ def test_set_child_active_missing_raises(tmp_path) -> None: _with_db(tmp_path, "set-active-missing.db")(_body) +# --------------------------------------------------------------------------- +# Concurrency: edit/set_active are atomic under the connection lock +# --------------------------------------------------------------------------- + + +def _gated_hass(armed: dict, gate_open: asyncio.Event, a_started: asyncio.Event): + """A hass mock whose executor gates the FIRST `_execute` job after + arming — the mutate statement of whichever business call starts + first — proving the caller holds the connection lock at that + moment and the second caller must queue behind it.""" + from unittest.mock import MagicMock + + hass = MagicMock() + + async def _gated_executor(fn, *args): + if armed["active"]: + if getattr(fn, "__name__", "") == "_execute" and not armed["gated"]: + armed["gated"] = True + a_started.set() + await gate_open.wait() + return fn(*args) + + hass.async_add_executor_job = _gated_executor + return hass + + +def test_edit_child_concurrent_opposite_edits_return_own_values( + tmp_path, +) -> None: + """Two racing edits cannot cross-contaminate each other's readback. + + Deterministic gate: task A is paused at its UPDATE (it provably + holds the connection lock), task B is then created and must queue; + A's readback runs before B's edit, so A returns A's value and B + returns B's. Without the lock, A's readback would see B's write. + """ + async def _main(): + armed = {"active": False, "gated": False} + gate_open = asyncio.Event() + a_started = asyncio.Event() + hass = _gated_hass(armed, gate_open, a_started) + database = NestQuestDatabase(hass) + await database.open(tmp_path / "edit-race.db") + try: + await apply_migrations(database) + child = await create_child( + database, "Ada", colour="#111111", sort_order=0 + ) + + armed["active"] = True + task_a = asyncio.ensure_future( + edit_child(database, child.id, colour="#AAAAAA", sort_order=1) + ) + await a_started.wait() + armed["active"] = False + task_b = asyncio.ensure_future( + edit_child( + database, child.id, colour="#BBBBBB", sort_order=2 + ) + ) + await asyncio.sleep(0) + gate_open.set() + record_a, record_b = await asyncio.gather(task_a, task_b) + + # Each caller gets back exactly what it wrote. + assert record_a.colour == "#AAAAAA" + assert record_a.sort_order == 1 + assert record_b.colour == "#BBBBBB" + assert record_b.sort_order == 2 + # B landed last: the stored row is B's. + row = await database.fetch_one( + "SELECT colour, sort_order FROM children WHERE id = ?", + (child.id,), + ) + assert row == ("#BBBBBB", 2) + finally: + await database.close() + + _run(_main()) + + +def test_set_child_active_concurrent_opposite_transitions_return_own( + tmp_path, +) -> None: + """Racing activate/deactivate calls each report their own request. + + Same deterministic gate on task A's UPDATE; task B queues behind + the connection lock and lands the opposite transition after A. + """ + async def _main(): + armed = {"active": False, "gated": False} + gate_open = asyncio.Event() + a_started = asyncio.Event() + hass = _gated_hass(armed, gate_open, a_started) + database = NestQuestDatabase(hass) + await database.open(tmp_path / "active-race.db") + try: + await apply_migrations(database) + child = await create_child(database, "Ada") + + armed["active"] = True + task_a = asyncio.ensure_future( + set_child_active(database, child.id, False) + ) + await a_started.wait() + armed["active"] = False + task_b = asyncio.ensure_future( + set_child_active(database, child.id, True) + ) + await asyncio.sleep(0) + gate_open.set() + record_a, record_b = await asyncio.gather(task_a, task_b) + + assert record_a.is_active is False + assert record_b.is_active is True + row = await database.fetch_one( + "SELECT is_active FROM children WHERE id = ?", (child.id,) + ) + assert row == (1,) + finally: + await database.close() + + _run(_main()) + + def test_list_children_orders_by_sort_order_then_id(tmp_path) -> None: async def _body(database): second = await create_child(database, "Bo", sort_order=1) From 09363a57cf89332dcf40ef5b2072c829a874d84d Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 14:14:56 -0400 Subject: [PATCH 4/5] Review fix: race tests read via the DAO, not raw SQL probes The children/admin_users leak guard forbids children SQL outside the DAO and its sanctioned test files; the two gated concurrency tests probed the children table raw. They now assert via ChildrenDao.get. Suite: 850 passed. --- tests/test_children_registry.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index e2af6cf..9cd6779 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -14,7 +14,7 @@ from custom_components.nestquest.children import ( list_children, set_child_active, ) -from custom_components.nestquest.dao_children import ChildRecord +from custom_components.nestquest.dao_children import ChildRecord, ChildrenDao from custom_components.nestquest.db import NestQuestDatabase from custom_components.nestquest.migrations import apply_migrations @@ -448,12 +448,11 @@ def test_edit_child_concurrent_opposite_edits_return_own_values( assert record_a.sort_order == 1 assert record_b.colour == "#BBBBBB" assert record_b.sort_order == 2 - # B landed last: the stored row is B's. - row = await database.fetch_one( - "SELECT colour, sort_order FROM children WHERE id = ?", - (child.id,), - ) - assert row == ("#BBBBBB", 2) + # B landed last: the stored row is B's (read via the DAO — + # the children leak guard forbids raw probes here). + stored = await ChildrenDao(database).get(child.id) + assert stored is not None + assert (stored.colour, stored.sort_order) == ("#BBBBBB", 2) finally: await database.close() @@ -494,10 +493,9 @@ def test_set_child_active_concurrent_opposite_transitions_return_own( assert record_a.is_active is False assert record_b.is_active is True - row = await database.fetch_one( - "SELECT is_active FROM children WHERE id = ?", (child.id,) - ) - assert row == (1,) + stored = await ChildrenDao(database).get(child.id) + assert stored is not None + assert stored.is_active is True finally: await database.close() From d5d972b872e8b38e765a02c0a5ae9966fc3a6be4 Mon Sep 17 00:00:00 2001 From: Joshua King Date: Tue, 15 Sep 2026 14:18:33 -0400 Subject: [PATCH 5/5] Review fix: serialize create_child's duplicate scan with the insert Review round 3 (Codex gpt-5.6-terra) found two concurrent creates of the same normalised name could both pass the duplicate scan before either INSERT committed, inserting a duplicate with no warning. The scan and INSERT now run inside the shared connection-scoped lock, so the second create always sees the first's committed row and warns. A gated concurrent-create test proves exactly one warning fires; a sequential-duplicate warning assertion rode along. Suite: 851 passed. --- custom_components/nestquest/children.py | 26 ++++++----- tests/test_children_registry.py | 62 +++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 11 deletions(-) diff --git a/custom_components/nestquest/children.py b/custom_components/nestquest/children.py index e3439aa..4e1d56e 100644 --- a/custom_components/nestquest/children.py +++ b/custom_components/nestquest/children.py @@ -116,23 +116,27 @@ async def create_child( The display name is required, trimmed, and rejected when empty. A duplicate display name (case-insensitive) is allowed but logged - as a warning. The duplicate scan and the INSERT are deliberately - not one transaction: the warning is advisory, so serialising - creates to protect it would buy nothing. + as a warning. The scan and the INSERT run inside the + connection-scoped lock shared with the DAOs, so a concurrent + create of the same normalised name queues behind the first insert + and sees it — the duplicate still warns; it can never slip through + unlogged. """ name = _validate_text(display_name, "display_name", required=True) assert name is not None colour_value = _validate_text(colour, "colour") avatar_value = _validate_text(avatar_ref, "avatar_ref") order = _validate_sort_order(sort_order) - await _warn_on_duplicate_name(database, name) - return await ChildrenDao(database).create( - name, - _now_stamp(), - colour=colour_value, - avatar_ref=avatar_value, - sort_order=order, - ) + async with _connection_lock(database): + await _warn_on_duplicate_name(database, name) + created = await ChildrenDao(database).create( + name, + _now_stamp(), + colour=colour_value, + avatar_ref=avatar_value, + sort_order=order, + ) + return created async def edit_child( diff --git a/tests/test_children_registry.py b/tests/test_children_registry.py index 9cd6779..63f6d73 100644 --- a/tests/test_children_registry.py +++ b/tests/test_children_registry.py @@ -320,6 +320,68 @@ def test_edit_child_rename_to_own_name_casing_alone_warns_not( _with_db(tmp_path, "edit-rename-self.db")(_body) +def test_create_child_concurrent_same_name_still_warns(tmp_path) -> None: + """Racing creates of the same normalised name cannot slip through + unlogged: the scan and INSERT are serialized on the connection + lock, so the second create sees the first's committed row and + warns. Deterministic gate on task A's INSERT while task B queues + behind the lock. Without the lock, B's scan would run before A's + INSERT commits and no warning would fire.""" + async def _main(): + armed = {"active": False, "gated": False} + gate_open = asyncio.Event() + a_started = asyncio.Event() + hass = _gated_hass(armed, gate_open, a_started) + database = NestQuestDatabase(hass) + await database.open(tmp_path / "create-race.db") + try: + await apply_migrations(database) + logger = logging.getLogger( + "custom_components.nestquest.children" + ) + seen: list[logging.LogRecord] = [] + + class _Capture(logging.Handler): + def emit(self, record: logging.LogRecord) -> None: + seen.append(record) + + handler = _Capture() + logger.addHandler(handler) + try: + armed["active"] = True + task_a = asyncio.ensure_future( + create_child(database, "Ada") + ) + await a_started.wait() + armed["active"] = False + task_b = asyncio.ensure_future( + create_child(database, " ada ") + ) + await asyncio.sleep(0) + gate_open.set() + first, second = await asyncio.gather(task_a, task_b) + finally: + logger.removeHandler(handler) + + # Both creates land; the duplicate is allowed. + assert first.display_name == "Ada" + assert second.display_name == "ada" + names = [c.display_name for c in await list_children(database)] + assert names == ["Ada", "ada"] + # Exactly one warning: B's scan saw A's committed row. + warnings = [ + r for r in seen if "duplicates existing child" in r.getMessage() + ] + assert len(warnings) == 1, ( + "the queued duplicate create must warn exactly once" + ) + assert "Ada" in warnings[0].getMessage() + finally: + await database.close() + + _run(_main()) + + def test_edit_child_case_only_edit_colliding_with_other_child_warns( tmp_path, caplog ) -> None: