Review fix: reject datetime anchors, freeze the pattern mapping
Review round 1 (Codex gpt-5.6-terra, 2 MEDIUM): datetime.datetime subclasses datetime.date and slipped past the anchor check, storing a time component that would TypeError every later calendar comparison — rejected explicitly now (both model classes). The frozen schedule's pattern dict let callers inject unvalidated weekdays or drop required weeks after construction; the validated mapping is now exposed as a read-only MappingProxyType over frozenset weeks. Tests: datetime rejection matrix and immutability assertions. Suite: 974 passed.
This commit is contained in:
@@ -43,6 +43,7 @@ from __future__ import annotations
|
||||
|
||||
import datetime
|
||||
from dataclasses import dataclass
|
||||
from types import MappingProxyType
|
||||
|
||||
#: The schema's documented cap for cycle_length_weeks (schema.py keeps
|
||||
#: the pattern human-readable and the anchor arithmetic small).
|
||||
@@ -52,7 +53,18 @@ _DATE_FORMAT = "%Y-%m-%d"
|
||||
|
||||
|
||||
def _parse_date(value: object, field_name: str) -> datetime.date:
|
||||
"""Accept a datetime.date or a strict YYYY-MM-DD string."""
|
||||
"""Accept a datetime.date or a strict YYYY-MM-DD string.
|
||||
|
||||
``datetime.datetime`` is rejected explicitly: it SUBCLASSES
|
||||
datetime.date, so a bare isinstance check would store a value with
|
||||
a time component and every later calendar-date comparison (e.g.
|
||||
``PresenceOverride.covers``) would raise TypeError.
|
||||
"""
|
||||
if isinstance(value, datetime.datetime):
|
||||
raise ValueError(
|
||||
f"{field_name} must be a plain calendar date, not a "
|
||||
f"datetime with a time component: {value!r}"
|
||||
)
|
||||
if isinstance(value, datetime.date):
|
||||
return value
|
||||
if not isinstance(value, str):
|
||||
@@ -149,7 +161,7 @@ class PresenceSchedule:
|
||||
child_id: int
|
||||
cycle_length_weeks: int
|
||||
anchor_date: datetime.date
|
||||
pattern: dict[int, frozenset[int]]
|
||||
pattern: MappingProxyType # week index -> frozenset of weekdays
|
||||
|
||||
def __init__(
|
||||
self,
|
||||
@@ -181,7 +193,12 @@ class PresenceSchedule:
|
||||
f"pattern must cover week indices 0..{cycle - 1}; "
|
||||
f"missing {missing}"
|
||||
)
|
||||
object.__setattr__(self, "pattern", normalised)
|
||||
# Frozen must mean frozen: a bare dict would let callers inject
|
||||
# unvalidated weekdays (or drop required weeks) AFTER
|
||||
# construction, bypassing total validation. The mapping
|
||||
# proxy exposes the validated contents read-only; the
|
||||
# per-week sets are already frozensets.
|
||||
object.__setattr__(self, "pattern", MappingProxyType(normalised))
|
||||
|
||||
def encode(self) -> str:
|
||||
"""Serialize to the database's pipe-separated CSV shape.
|
||||
|
||||
@@ -227,3 +227,36 @@ def test_override_rejects_non_int_child_id(bad) -> None:
|
||||
def test_override_rejects_non_strict_dates(start, end) -> None:
|
||||
with pytest.raises(ValueError, match="date"):
|
||||
PresenceOverride(1, start, end, True)
|
||||
|
||||
|
||||
def test_schedule_rejects_datetime_anchor() -> None:
|
||||
"""datetime.datetime subclasses date: a bare isinstance check would
|
||||
store a time component and every date comparison would TypeError."""
|
||||
with pytest.raises(ValueError, match="plain calendar date"):
|
||||
PresenceSchedule(1, 1, datetime.datetime(2026, 1, 5, 9, 30), {0: {0}})
|
||||
|
||||
|
||||
def test_override_rejects_datetime_bounds() -> None:
|
||||
with pytest.raises(ValueError, match="plain calendar date"):
|
||||
PresenceOverride(
|
||||
1, datetime.datetime(2026, 7, 20), "2026-07-20", True
|
||||
)
|
||||
|
||||
|
||||
def test_schedule_pattern_is_immutable_after_construction() -> None:
|
||||
"""The frozen schedule must not leak a mutable dict: injecting
|
||||
unvalidated weekdays or dropping required weeks after construction
|
||||
would bypass total validation."""
|
||||
schedule = PresenceSchedule(1, 2, ANCHOR, {0: {0, 2}, 1: {1}})
|
||||
with pytest.raises(TypeError):
|
||||
schedule.pattern[0] = {9}
|
||||
with pytest.raises(TypeError):
|
||||
del schedule.pattern[1]
|
||||
# mappingproxy exposes no mutators at all.
|
||||
assert not any(
|
||||
hasattr(schedule.pattern, name)
|
||||
for name in ("clear", "pop", "popitem", "setdefault", "update")
|
||||
)
|
||||
# The validated contents are still readable and encode correctly.
|
||||
assert schedule.pattern[0] == frozenset({0, 2})
|
||||
assert schedule.encode() == "0,2|1"
|
||||
|
||||
Reference in New Issue
Block a user