fix(db): bind datetimes, not strings, when filtering timestamptz columns
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / TypeScript typecheck (push) Successful in 10s
CI & Build / integration (push) Successful in 16s
CI & Build / Python tests (push) Successful in 42s
CI & Build / Build & push image (push) Successful in 26s

Closes #1727 and #2257 — the same bug in two services written months apart.

AppLog.created_at is `timestamp with time zone`. asyncpg binds a Python str as
VARCHAR and Postgres has no `timestamptz >= text` operator, so both of these
raised when Postgres planned the query:

  notifications.check_due_tasks   AppLog.created_at >= today.isoformat()
  logging.get_logs                AppLog.created_at >= <raw request.args str>

#1727 was the worse of the two because a per-user `except Exception` swallowed
it: reminder emails silently never sent, and the only outward trace was an
hourly traceback in the Postgres log. It has been open since 2026-07-19 with the
diagnosis written and the fix never applied. #2257 has no swallowing handler, so
it merely breaks the admin log viewer's date filters outright.

notifications: `utc_day_start(day)` returns midnight UTC as an AWARE datetime.
Deliberately not the bare `date` the original diagnosis suggested — comparing
timestamptz to date does work via an implicit cast, but Postgres resolves that
cast in the SESSION's TimeZone, so the dedup window would drift with a server
setting nobody remembers is load-bearing.

logging: `parse_filter_datetime()` converts the query-string value to an aware
UTC datetime; unparseable input returns None so the filter is skipped rather
than 500ing the viewer. It also fixes a bug the naive fix would have introduced
— `date_to=2026-07-30` parses to midnight, so `<=` would exclude the entire day
the user asked for. Date-only upper bounds now run to 23:59:59.999999, while a
value carrying an explicit time is left as given.

The guard is the point. This class is invisible to ordinary testing: the failure
happens when Postgres plans the query, not when Python builds it, so no unit
test that doesn't execute SQL can see it. tests/test_timestamp_filters.py fails
CI on two shapes —

  1. a local bound to .isoformat() compared against a *_at column   (#1727)
  2. a str-ANNOTATED PARAMETER compared against a *_at column       (#2257)

Shape 2 is the one that matters. Nothing in logging.py looks date-ish, so a
guard built only from #1727's shape finds nothing there — which is exactly how
the second instance survived. Verified by replaying both checks against the
pre-fix files out of git: shape 1 catches `today_str`, shape 2 catches
`date_from`/`date_to`, and the current tree is clean.

Found by grepping for siblings after fixing #1727 — the third instance today of
"the second place nobody checked", after #2245.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs
This commit is contained in:
2026-07-30 12:53:57 -04:00
co-authored by Claude Opus 5
parent 6ca215d2b6
commit 293a14361a
3 changed files with 207 additions and 10 deletions
+145
View File
@@ -0,0 +1,145 @@
"""Timestamp filters must bind datetimes, never strings (#1727, #2254).
`AppLog.created_at` is `timestamp with time zone`. asyncpg binds a Python `str`
as VARCHAR, and Postgres has no `timestamptz >= text` operator — so comparing
the column against `date.today().isoformat()` or a raw `request.args` string
raises at query time, not at import time and not in any unit test that doesn't
touch a database.
That is what made this class of bug survive: it is invisible to every test that
doesn't execute the SQL, and in the notifications case the exception was
swallowed by a per-user `except Exception`, so the only outward symptom was
reminder emails silently never sending.
Found twice in the same afternoon, in two services written months apart, so the
last test here guards the CLASS rather than the two instances.
"""
import ast
import re
from datetime import date, datetime, timezone
from pathlib import Path
SRC = Path(__file__).resolve().parents[1] / "src" / "scribe"
# --- #1727: the reminder dedup window ----------------------------------------
def test_utc_day_start_is_an_aware_datetime_not_a_string():
from scribe.services.notifications import utc_day_start
start = utc_day_start(date(2026, 7, 30))
assert isinstance(start, datetime)
assert not isinstance(start, str)
# Aware, and pinned to UTC rather than whatever the DB session's TimeZone
# happens to be — a bare `date` would resolve midnight server-side.
assert start.tzinfo is not None
assert start.utcoffset() == timezone.utc.utcoffset(None)
assert (start.year, start.month, start.day) == (2026, 7, 30)
assert (start.hour, start.minute, start.second, start.microsecond) == (0, 0, 0, 0)
# --- #2254: the admin log viewer's date filters ------------------------------
def test_filter_datetime_parses_dates_and_datetimes_to_aware_utc():
from scribe.services.logging import parse_filter_datetime
d = parse_filter_datetime("2026-07-30")
assert isinstance(d, datetime) and d.tzinfo is not None
assert (d.hour, d.minute) == (0, 0)
# An explicit time is honoured, not overwritten.
dt = parse_filter_datetime("2026-07-30T14:35:00")
assert (dt.hour, dt.minute) == (14, 35)
# Already-aware input keeps its offset rather than being stamped UTC.
aware = parse_filter_datetime("2026-07-30T14:35:00+02:00")
assert aware.utcoffset().total_seconds() == 2 * 3600
def test_filter_datetime_upper_bound_covers_the_whole_requested_day():
"""`date_to=2026-07-30` must INCLUDE that day. Parsed as bare midnight it
would exclude every event on the one day the user asked for."""
from scribe.services.logging import parse_filter_datetime
end = parse_filter_datetime("2026-07-30", end_of_day=True)
assert (end.hour, end.minute, end.second) == (23, 59, 59)
assert end.microsecond == 999999
# But a value that already carries a time is left alone — the caller meant it.
exact = parse_filter_datetime("2026-07-30T09:00:00", end_of_day=True)
assert (exact.hour, exact.minute, exact.second) == (9, 0, 0)
def test_filter_datetime_rejects_garbage_instead_of_raising():
"""A malformed filter should narrow nothing, not 500 the log viewer."""
from scribe.services.logging import parse_filter_datetime
for junk in ("", " ", None, "not-a-date", "2026-13-45", "yesterday"):
assert parse_filter_datetime(junk) is None
# --- the class-level guard ---------------------------------------------------
def _timestamp_comparison_offenders() -> list[str]:
"""Every place a `*_at` column is compared against a known-string name.
TWO shapes, because the two real bugs had different ones and a guard built
from only the first would have missed the second:
1. a local bound to `.isoformat()` — #1727, `today_str = today.isoformat()`
2. a parameter annotated `str` — #2254, `date_from: str | None`, straight
off `request.args`, never touching
isoformat at all
Shape 2 is the nastier one: nothing in the file looks date-ish, so grepping
for isoformat finds nothing. It was caught by reading, not by tooling —
which is exactly why it belongs in tooling now.
Deliberately narrow on both. A blanket "no strings near SQL" rule would fire
on every to_dict() in the codebase and get muted, and a muted guard is worse
than none.
"""
offenders: list[str] = []
files = sorted((SRC / "services").rglob("*.py")) + sorted((SRC / "routes").rglob("*.py"))
for path in files:
src = path.read_text()
rel = path.relative_to(SRC)
suspect_names: dict[str, str] = {}
# Shape 1 — locals bound to an isoformat() result.
for name in re.findall(r"^\s*(\w+)\s*=\s*[\w.()]+\.isoformat\(\)", src, re.M):
suspect_names[name] = "an isoformat() string"
# Shape 2 — parameters annotated as str (incl. `str | None`, Optional[str]).
try:
tree = ast.parse(src)
except SyntaxError: # pragma: no cover - not our concern here
tree = None
if tree is not None:
for node in ast.walk(tree):
if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
continue
args = node.args
for arg in [*args.posonlyargs, *args.args, *args.kwonlyargs]:
if arg.annotation is None:
continue
ann = ast.unparse(arg.annotation)
if re.fullmatch(r"(?:Optional\[)?str\]?(?:\s*\|\s*None)?", ann):
suspect_names[arg.arg] = f"a str-annotated parameter ({ann})"
for name, why in suspect_names.items():
if re.search(rf"\w*_at\s*(?:>=|<=|>|<)\s*{re.escape(name)}\b", src):
offenders.append(f"{rel}: timestamp column compared against `{name}` — {why}")
return offenders
def test_no_service_compares_a_timestamp_column_against_a_string():
offenders = _timestamp_comparison_offenders()
assert not offenders, (
"asyncpg binds a str as VARCHAR and Postgres has no `timestamptz >= text` "
"operator, so this raises at query time — invisible to any test that "
"doesn't execute the SQL:\n " + "\n ".join(offenders)
)