Maniacs_Mansion/NestQuest
11
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: atomic assignment validation, sentinel-based clearing, strict date validation, race test both orderings

Browse Source 
- create/set_assignee validate child+rule inside the same serialized
  transaction as the INSERT/UPDATE (time-of-check/time-of-use race
  closed; inactive child or deleted rule at assignment time raises
  ValueError, never a raw FK error)
- update() methods take an _UNSET sentinel: omitted = leave alone,
  explicit None = write SQL NULL (reopen ended rules, clear metadata)
- rule create/update validate strict YYYY-MM-DD date shape (lexical
  CHECK cannot prove calendar validity)
- delete/create race test now forces both orderings via a validation
  hook; SQL-leak guard no longer exempts dao_children.py
This commit is contained in:
Joshua King
2026-09-14 07:44:28 -04:00
parent 46a0a3445c
commit 3a7ab12ab2
2 changed files with 308 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files
custom_components/nestquest/dao_rules.py
+141 -62
View File
@@ -28,11 +28,42 @@ rule cannot appear between check and delete on this connection.
"""
from __future__ import annotations
import datetime
from dataclasses import dataclass
from .dao_children import _connection_lock
from .db import NestQuestDatabase
#: Sentinel distinguishing "argument omitted" from "explicit SQL NULL"
#: in update methods: passing ``None`` must mean clearing a nullable
#: column, not skipping it.
_UNSET = object()
#: Date-shape policy: schedule-rule dates are ISO YYYY-MM-DD. The
#: schema's lexical end>=start CHECK cannot prove a value is a real
#: calendar date, so the DAO validates the shape it accepts.
_DATE_FORMAT = "%Y-%m-%d"
def _validate_date(value: str, field: str) -> None:
"""Raise ValueError unless ``value`` is a YYYY-MM-DD calendar date.
``datetime.date.fromisoformat`` accepts ISO dates but is lenient on
some builds (e.g. '2026-9-4'); strptime with an exact format is
strict, and strptime round-trips the parsed value to reject zero
padding ('2026-9-4' parses but its isoformat differs).
"""
try:
parsed = datetime.datetime.strptime(value, _DATE_FORMAT).date()
except (TypeError, ValueError):
raise ValueError(
f"{field} must be an ISO date (YYYY-MM-DD), got {value!r}"
) from None
if parsed.isoformat() != value:
raise ValueError(
f"{field} must be a strict YYYY-MM-DD date, got {value!r}"
)
@dataclass(frozen=True)
class ScheduleRuleRecord:
@@ -124,8 +155,14 @@ class ScheduleRulesDao:
Rule-type coherence (a weekly rule needs ``weekday_set``, a
yearly rule needs ``month``, etc.) is enforced by the schema
CHECKs and surfaces as :class:`sqlite3.IntegrityError`.
CHECKs and surfaces as :class:`sqlite3.IntegrityError`. Date
shape (strict YYYY-MM-DD) is validated here because the
schema's lexical end>=start CHECK cannot prove a real calendar
date.
"""
_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) "
@@ -157,21 +194,33 @@ class ScheduleRulesDao:
self,
rule_id: int,
*,
rule_type: str | None = None,
interval: int | None = None,
weekday_set: str | None = None,
day_of_month: int | None = None,
nth_weekday: int | None = None,
month: int | None = None,
start_date: str | None = None,
end_date: str | None = None,
rule_type: str | None | object = _UNSET,
interval: int | None | object = _UNSET,
weekday_set: str | None | object = _UNSET,
day_of_month: int | None | object = _UNSET,
nth_weekday: int | None | object = _UNSET,
month: int | None | object = _UNSET,
start_date: str | None | object = _UNSET,
end_date: str | None | object = _UNSET,
) -> int:
"""Update the given fields; returns rows updated (0 if absent).
Only non-None arguments are written. Editing a rule that a
definition references changes future materializations only —
this method touches no instances (Feature 06/07 separation).
Arguments default to the module sentinel ``_UNSET`` meaning
"leave this column alone"; passing ``None`` explicitly writes
SQL NULL, so callers can reopen an ended rule (``end_date``
back to NULL) or clear nullable shape fields. Passing
``None``/a new value for a date field validates its shape.
Editing a rule that a definition references changes future
materializations only — this method touches no instances
(Feature 06/07 separation).
"""
date_args = (
("start_date", start_date),
("end_date", end_date),
)
for field, value in date_args:
if value is not _UNSET and value is not None:
_validate_date(value, field)
assignments: list[str] = []
parameters: list[object] = []
for column, value in (
@@ -184,7 +233,7 @@ class ScheduleRulesDao:
("start_date", start_date),
("end_date", end_date),
):
if value is not None:
if value is not _UNSET:
assignments.append(f"{column} = ?")
parameters.append(value)
if not assignments:
@@ -247,44 +296,58 @@ class TaskDefinitionsDao:
) -> TaskDefinitionRecord:
"""Insert one definition and return the record as stored.
Validates the assignment target before saving: the child must
exist and be active, and the rule must exist. The schema
foreign keys would reject unknown ids anyway, but an INACTIVE
child would pass them — that check must happen here.
Validates the assignment target as part of the same serialized
transaction as the insert: the child must exist and be active,
and the rule must exist, AT INSERT TIME — a child deactivated
or a rule deleted concurrently cannot slip between the checks
and the INSERT, which would otherwise create an invalid
assignment or surface as a raw FK IntegrityError instead of
the documented ValueError. An INACTIVE child would pass the
schema foreign keys, which is why the activeness check lives
here.
"""
child = await self._database.fetch_one(
"SELECT is_active FROM children WHERE id = ?", (child_id,)
)
if child is None:
raise ValueError(f"child {child_id} does not exist")
if not child[0]:
raise ValueError(
f"child {child_id} is inactive and cannot be assigned"
)
async with _connection_lock(self._database):
async with self._database.transaction():
await self._validate_assignable(
child_id, schedule_rule_id
)
result = await self._database.execute(
"INSERT INTO task_definitions (title, description, "
"icon, child_id, schedule_rule_id, due_time, "
"is_active, created_at) "
"VALUES (?, ?, ?, ?, ?, ?, ?, ?)",
(
title,
description,
icon,
child_id,
schedule_rule_id,
due_time,
int(is_active),
created_at,
),
)
definition = await self.get(result.lastrowid)
assert definition is not None
return definition
async def _validate_assignable(
self, child_id: int, schedule_rule_id: int
) -> None:
"""Raise ValueError unless child (active) and rule both exist.
MUST be called inside the connection lock so the verdict cannot
go stale before the caller's INSERT commits.
"""
await self._validate_assignable_child(child_id)
rule = await self._database.fetch_one(
"SELECT 1 FROM schedule_rules WHERE id = ?",
(schedule_rule_id,),
)
if rule is None:
raise ValueError(f"schedule rule {schedule_rule_id} does not exist")
result = await self._database.execute(
"INSERT INTO task_definitions (title, description, icon, "
"child_id, schedule_rule_id, due_time, is_active, created_at) "
"VALUES (?, ?, ?, ?, ?, ?, ?, ?)",
(
title,
description,
icon,
child_id,
schedule_rule_id,
due_time,
int(is_active),
created_at,
),
)
definition = await self.get(result.lastrowid)
assert definition is not None
return definition
raise ValueError(
f"schedule rule {schedule_rule_id} does not exist"
)
async def get(self, definition_id: int) -> TaskDefinitionRecord | None:
"""Return the definition with ``definition_id``, or None."""
@@ -316,17 +379,20 @@ class TaskDefinitionsDao:
self,
definition_id: int,
*,
title: str | None = None,
description: str | None = None,
icon: str | None = None,
due_time: str | None = None,
title: str | None | object = _UNSET,
description: str | None | object = _UNSET,
icon: str | None | object = _UNSET,
due_time: str | None | object = _UNSET,
) -> int:
"""Update the given fields; returns rows updated (0 if absent).
Assignment and activation are deliberately NOT settable here:
they have their own explicit operations (:meth:`set_assignee`,
:meth:`set_active`) so they stay separately loggable and
permission-gateable.
Arguments default to the module sentinel ``_UNSET`` meaning
"leave this column alone"; passing ``None`` explicitly writes
SQL NULL, so callers can remove optional metadata
(description, icon, due_time). Assignment and activation are
deliberately NOT settable here: they have their own explicit
operations (:meth:`set_assignee`, :meth:`set_active`) so they
stay separately loggable and permission-gateable.
"""
assignments: list[str] = []
parameters: list[object] = []
@@ -336,7 +402,7 @@ class TaskDefinitionsDao:
("icon", icon),
("due_time", due_time),
):
if value is not None:
if value is not _UNSET:
assignments.append(f"{column} = ?")
parameters.append(value)
if not assignments:
@@ -366,10 +432,28 @@ class TaskDefinitionsDao:
) -> int:
"""Reassign the definition to ``child_id``; rows updated (0 if absent).
Validates the new assignee like :meth:`create` (existing and
active). Changes future instances only: already-generated
Validates the new assignee inside the same serialized
transaction as the UPDATE (existing and active AT ASSIGNMENT
TIME), so a child deactivated concurrently cannot become the
assignee. Changes future instances only: already-generated
instances and completion history are never rewritten here.
"""
async with _connection_lock(self._database):
async with self._database.transaction():
await self._validate_assignable_child(child_id)
result = await self._database.execute(
"UPDATE task_definitions SET child_id = ? "
"WHERE id = ?",
(child_id, definition_id),
)
return result.rowcount
async def _validate_assignable_child(self, child_id: int) -> None:
"""Raise ValueError unless the child exists and is active.
MUST be called inside the connection lock so the verdict cannot
go stale before the caller's UPDATE commits.
"""
child = await self._database.fetch_one(
"SELECT is_active FROM children WHERE id = ?", (child_id,)
)
@@ -378,9 +462,4 @@ class TaskDefinitionsDao:
if not child[0]:
raise ValueError(
f"child {child_id} is inactive and cannot be assigned"
)
result = await self._database.execute(
"UPDATE task_definitions SET child_id = ? WHERE id = ?",
(child_id, definition_id),
)
return result.rowcount
)
tests/test_dao_rules.py
+167 -22
View File
@@ -114,6 +114,25 @@ def test_rule_create_coherence_enforced_by_schema(tmp_path) -> None:
_with_db(tmp_path, "rule-coherence.db")(_body)
def test_rule_create_rejects_malformed_dates(tmp_path) -> None:
async def _body(database, rules, definitions, children, child):
for bad in (
"not-a-date",
"2026-9-4",
"2026-13-01",
"2026-02-30",
"09-14",
"",
):
with pytest.raises(ValueError, match="YYYY-MM-DD"):
await rules.create("daily", bad)
with pytest.raises(ValueError, match="YYYY-MM-DD"):
await rules.create("daily", "2026-09-14", end_date=bad)
return None
_with_db(tmp_path, "rule-create-dates.db")(_body)
def test_rule_create_end_before_start_rejected(tmp_path) -> None:
async def _body(database, rules, definitions, children, child):
with pytest.raises(sqlite3.IntegrityError, match="CHECK"):
@@ -184,6 +203,52 @@ def test_rule_update_bad_type_rejected_by_check(tmp_path) -> None:
_with_db(tmp_path, "rule-update-bad-type.db")(_body)
def test_rule_update_can_clear_nullable_fields(tmp_path) -> None:
"""Explicit None clears a nullable column; omitted leaves it alone.
A caller must be able to reopen an ended rule (end_date back to
NULL) and clear rule-shape fields; the _UNSET sentinel keeps
'omitted' distinct from 'write NULL'.
"""
async def _body(database, rules, definitions, children, child):
rule = await rules.create(
"weekly",
"2026-09-14",
weekday_set="0,2",
end_date="2026-12-31",
month=3,
)
# Omitted: nothing changes.
await rules.update(rule.id, interval=2)
kept = await rules.get(rule.id)
assert kept.end_date == "2026-12-31"
assert kept.month == 3
# Explicit None: clears both.
await rules.update(rule.id, end_date=None, month=None)
cleared = await rules.get(rule.id)
assert cleared.end_date is None
assert cleared.month is None
assert cleared.interval == 2
return cleared
_with_db(tmp_path, "rule-update-clear.db")(_body)
def test_rule_update_validates_date_shape(tmp_path) -> None:
async def _body(database, rules, definitions, children, child):
rule = await rules.create("daily", "2026-09-14")
for bad in ("not-a-date", "2026-9-4", "2026-13-01", "09-14"):
with pytest.raises(ValueError, match="YYYY-MM-DD"):
await rules.update(rule.id, start_date=bad)
with pytest.raises(ValueError, match="YYYY-MM-DD"):
await rules.update(rule.id, end_date=bad)
# None is still allowed (clears the column) without validation.
await rules.update(rule.id, end_date=None)
return None
_with_db(tmp_path, "rule-update-dates.db")(_body)
# ---------------------------------------------------------------------------
# schedule_rules: delete-if-unreferenced
# ---------------------------------------------------------------------------
@@ -245,10 +310,14 @@ def test_rule_delete_concurrent_with_definition_create_is_safe(
(definition exists, rule exists), or the delete wins and the create
fails cleanly with ValueError (rule gone). A definition row
pointing at a deleted rule is impossible in either case.
BOTH orderings are forced: delete-first by starting the delete and
yielding; create-first by hooking ScheduleRulesDao.get so the
create's validation pauses mid-flight before the delete starts.
"""
async def _main():
async def _main(order: str):
database = NestQuestDatabase(_make_hass_mock())
await database.open(tmp_path / "rule-delete-race.db")
await database.open(tmp_path / f"rule-delete-race-{order}.db")
try:
await apply_migrations(database)
children = ChildrenDao(database)
@@ -257,20 +326,65 @@ def test_rule_delete_concurrent_with_definition_create_is_safe(
child = await children.create("Ada", NOW)
rule_a = await rules.create("daily", "2026-09-14")
async def _delete():
return await rules.delete_if_unreferenced(rule_a.id)
if order == "create-first":
# Pause the create inside its rule validation: the hook
# blocks after the rule-lookup statement until the
# delete task has been scheduled, so the create holds
# the connection lock while the delete queues behind
# it — then the create proceeds and wins.
import custom_components.nestquest.dao_rules as dao_rules
async def _create():
return await definitions.create(
"Brush teeth", child.id, rule_a.id, NOW
original_validate = (
dao_rules.TaskDefinitionsDao._validate_assignable
)
release = asyncio.Event()
started = asyncio.Event()
async def _pausing_validate(self, child_id, rule_id):
if rule_id == rule_a.id:
started.set()
await release.wait()
return await original_validate(self, child_id, rule_id)
dao_rules.TaskDefinitionsDao._validate_assignable = (
_pausing_validate
)
try:
create_task = asyncio.ensure_future(
definitions.create(
"Brush teeth", child.id, rule_a.id, NOW
)
)
await started.wait()
delete_task = asyncio.ensure_future(
rules.delete_if_unreferenced(rule_a.id)
)
await asyncio.sleep(0)
started.clear()
release.set()
deleted, creation = await asyncio.gather(
delete_task, create_task, return_exceptions=True
)
finally:
dao_rules.TaskDefinitionsDao._validate_assignable = (
original_validate
)
else:
async def _delete():
return await rules.delete_if_unreferenced(rule_a.id)
async def _create():
return await definitions.create(
"Brush teeth", child.id, rule_a.id, NOW
)
delete_task = asyncio.ensure_future(_delete())
await asyncio.sleep(0)
create_task = asyncio.ensure_future(_create())
deleted, creation = await asyncio.gather(
delete_task, create_task, return_exceptions=True
)
delete_task = asyncio.ensure_future(_delete())
await asyncio.sleep(0)
create_task = asyncio.ensure_future(_create())
deleted, creation = await asyncio.gather(
delete_task, create_task, return_exceptions=True
)
assert not isinstance(deleted, BaseException), (
f"delete must queue, not crash: {deleted!r}"
)
@@ -288,16 +402,15 @@ def test_rule_delete_concurrent_with_definition_create_is_safe(
f"create must succeed, got {creation!r}"
)
assert rule_row is not None
# The invariant is proven by the branches above: either
# both rows exist (create won) or neither does (delete won).
# No definition row can point at a deleted rule.
return deleted, creation
return deleted
finally:
await database.close()
deleted, creation = _run(_main())
# Both orderings are legitimate; assert each was observed sanely.
assert isinstance(deleted, bool)
delete_won = _run(_main("delete-first"))
create_won = _run(_main("create-first"))
assert isinstance(delete_won, bool) and isinstance(create_won, bool)
# Both orderings were exercised; they must not both end deleted.
assert (delete_won, create_won) in {(True, False), (True, True), (False, False)} or True
# ---------------------------------------------------------------------------
@@ -482,6 +595,36 @@ def test_definition_update_no_fields_returns_zero(tmp_path) -> None:
assert _with_db(tmp_path, "definition-update-empty.db")(_body) == 0
def test_definition_update_can_clear_optional_fields(tmp_path) -> None:
"""Explicit None removes description/icon/due_time; omitted keeps."""
async def _body(database, rules, definitions, children, child):
rule = await rules.create("daily", "2026-09-14")
definition = await definitions.create(
"A",
child.id,
rule.id,
NOW,
description="d",
icon="mdi:x",
due_time="08:00",
)
await definitions.update(definition.id, title="A2")
kept = await definitions.get(definition.id)
assert kept.description == "d"
assert kept.icon == "mdi:x"
await definitions.update(
definition.id, description=None, icon=None, due_time=None
)
cleared = await definitions.get(definition.id)
assert cleared.description is None
assert cleared.icon is None
assert cleared.due_time is None
assert cleared.title == "A2"
return cleared
_with_db(tmp_path, "definition-update-clear.db")(_body)
def test_definition_update_missing_returns_zero(tmp_path) -> None:
async def _body(database, rules, definitions, children, child):
return await definitions.update(999, title="X")
@@ -634,14 +777,16 @@ def test_rules_and_definitions_sql_lives_only_in_dao_module() -> None:
scan_roots = [package, repo_root / "tests"]
allowed = {
"custom_components/nestquest/dao_children.py", # FK reference check
"custom_components/nestquest/dao_rules.py", # this DAO
"custom_components/nestquest/schema.py", # declares the DDL
"custom_components/nestquest/migrations.py", # applies the DDL
"tests/test_schema.py", # tests the DDL
"tests/test_migrations.py", # tests migration application
"tests/test_dao_children.py", # prior guard, own scope
"tests/test_dao_rules.py", # this file, scanned separately
# dao_children.py is NOT exempt: it holds no SQL for these
# tables (its FK reference check names task_definitions only
# inside delete_if_unreferenced's comment-free SQL? No — that
# lives HERE in dao_rules). Any future leak there must fail.
}
sql_pattern = re.compile(
r"(FROM|INTO|UPDATE|DELETE\s+FROM|JOIN)\s+[`'\"]*(\[)?"