From 24c47d12bd7ba36ffd439682c3da6685c31bcf07 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 4 Oct 2026 22:12:02 +0200 Subject: [PATCH] refactor(backend): select() and an inline now everywhere, held by a test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLAUDE.md's canonical table names SQLAlchemy 2.0 select() and an inline datetime.now(timezone.utc), and both had drifted: five db.query(...) calls remained in routes/auth.py and routes/me.py (seven before the demo went), and four private now helpers stood in sync/videos.py, utils.py (imported as _now by explore and maintenance), scheduler.py (_now_iso) and the e2e seed. All are the canonical form now. Ruff cannot ban a method call on an instance or a function by name, so tests/test_canonical_patterns.py reads every app module's AST and fails on a .query(...) call or a now/_now/now_utc helper — seen red on a planted file. --- backend/app/routes/auth.py | 16 +++++-- backend/app/routes/me.py | 2 +- backend/app/scheduler.py | 12 ++--- backend/app/sync/explore.py | 9 ++-- backend/app/sync/maintenance.py | 7 ++- backend/app/sync/videos.py | 24 +++++----- backend/app/utils.py | 7 +-- backend/tests/test_canonical_patterns.py | 52 +++++++++++++++++++++ backend/tests/test_maintenance_lifecycle.py | 27 ++++++----- scripts/e2e_seed.py | 14 +++--- 10 files changed, 111 insertions(+), 59 deletions(-) create mode 100644 backend/tests/test_canonical_patterns.py diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 3fbdefd8..db7ec50f 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -399,7 +399,7 @@ async def callback( return RedirectResponse(url=_access_notice_url("requested", email, req.invite)) return RedirectResponse(url=_access_notice_url("denied")) - user = db.query(User).filter(User.google_sub == sub).one_or_none() + user = db.execute(select(User).where(User.google_sub == sub)).scalar_one_or_none() if user is None: # No account carries this Google identity yet. If one already exists for this MAILBOX — # e.g. an email+password registration — adopt the Google identity onto it instead of @@ -412,7 +412,11 @@ async def callback( # (ids 13 and 14). Exact spelling still wins when both exist, so an account that already # matches literally is never passed over for a canonical sibling. canon = canonical_email(email) - candidates = db.query(User).filter(User.email_canonical == canon).order_by(User.id).all() + candidates = ( + db.execute(select(User).where(User.email_canonical == canon).order_by(User.id)) + .scalars() + .all() + ) existing = next((u for u in candidates if u.email == email), None) or next( iter(candidates), None ) @@ -516,7 +520,9 @@ def _sync_google_email(db: Session, user: User, email: str) -> None: return if canonical_email(user.email) == canonical_email(email): return # same mailbox, their spelling — leave it alone - clash = db.query(User).filter(User.email == email, User.id != user.id).one_or_none() + clash = db.execute( + select(User).where(User.email == email, User.id != user.id) + ).scalar_one_or_none() if clash is None: user.email = email @@ -575,7 +581,9 @@ def _complete_link( user = db.get(User, link_uid) if user is None: return RedirectResponse(url="/?link=error") - other = db.query(User).filter(User.google_sub == sub, User.id != user.id).one_or_none() + other = db.execute( + select(User).where(User.google_sub == sub, User.id != user.id) + ).scalar_one_or_none() if other is not None: return RedirectResponse(url="/?link=conflict") if user.google_sub is not None and user.google_sub != sub: diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index 61d4fa6b..c1b0611e 100644 --- a/backend/app/routes/me.py +++ b/backend/app/routes/me.py @@ -40,7 +40,7 @@ def my_accounts( """The accounts that have authenticated in this browser session (switchable without a new Google sign-in). The active one is flagged.""" ids = request.session.get("account_ids") or [user.id] - rows = {u.id: u for u in db.query(User).filter(User.id.in_(ids)).all()} + rows = {u.id: u for u in db.execute(select(User).where(User.id.in_(ids))).scalars()} out = [] for uid in ids: # preserve recency order from the session u = rows.get(uid) diff --git a/backend/app/scheduler.py b/backend/app/scheduler.py index 9e4cd474..d790bd9f 100644 --- a/backend/app/scheduler.py +++ b/backend/app/scheduler.py @@ -106,10 +106,6 @@ def apply_interval(db, job_id: str, minutes: int) -> int: return minutes -def _now_iso() -> str: - return datetime.now(timezone.utc).isoformat() - - def _record(name: str, **fields) -> None: with _activity_lock: _activity.setdefault(name, {}).update(fields) @@ -187,7 +183,7 @@ def _job(name: str, fn) -> None: db = SessionLocal() actor_id = _manual_actor.get() # set only for a manual "run now" trigger _record( - name, last_started=_now_iso(), last_error=None, progress=None + name, last_started=datetime.now(timezone.utc).isoformat(), last_error=None, progress=None ) # running already claimed def sink(current, total, phase): @@ -199,7 +195,7 @@ def _job(name: str, fn) -> None: _record( name, status="skipped", - last_finished=_now_iso(), + last_finished=datetime.now(timezone.utc).isoformat(), last_result="paused", result=_PAUSED, progress=None, @@ -216,7 +212,7 @@ def _job(name: str, fn) -> None: _record( name, status="ok", - last_finished=_now_iso(), + last_finished=datetime.now(timezone.utc).isoformat(), last_result=summary, result=data, progress=None, @@ -236,7 +232,7 @@ def _job(name: str, fn) -> None: _record( name, status="error", - last_finished=_now_iso(), + last_finished=datetime.now(timezone.utc).isoformat(), last_error=err, result=failure, progress=None, diff --git a/backend/app/sync/explore.py b/backend/app/sync/explore.py index f9bcc0b6..1a4172c8 100644 --- a/backend/app/sync/explore.py +++ b/backend/app/sync/explore.py @@ -8,7 +8,7 @@ stay private to users who explored (or subscribe to) the channel — the feed qu otherwise the cleanup job hard-deletes the channel and its untouched ephemeral videos after a grace period (mirrors the maintenance job's grace-then-delete safety).""" -from datetime import timedelta +from datetime import datetime, timedelta, timezone from sqlalchemy import delete, or_, select from sqlalchemy.orm import Session @@ -24,7 +24,6 @@ from app.models import ( VideoState, ) from app.sync.videos import insert_stubs, stub_from_playlist_item, apply_video_details -from app.utils import now_utc as _now from app.youtube.client import YouTubeClient, YouTubeError, is_playlist_gone log = logs.get_logger(__name__) @@ -59,7 +58,7 @@ def _enrich_new(db: Session, yt: YouTubeClient, video_ids: list[str]) -> int: if not videos: return 0 items = {it["id"]: it for it in yt.get_videos([v.id for v in videos])} - now = _now() + now = datetime.now(timezone.utc) for video in videos: item = items.get(video.id) if item is None: @@ -115,7 +114,7 @@ def purge_unkept_explored(db: Session) -> dict: or surfaced by the user's own search (SearchFind) — these survive even if the channel goes. A channel row is dropped only once it holds no videos at all. 0 quota.""" grace_days = sysconfig.get_int(db, "explore_grace_days") - cutoff = _now() - timedelta(days=grace_days) + cutoff = datetime.now(timezone.utc) - timedelta(days=grace_days) expired = db.execute( delete(ExploredChannel).where(ExploredChannel.created_at < cutoff) @@ -166,7 +165,7 @@ def purge_unkept_search(db: Session, grace_days: int | None = None) -> dict: SearchFinds. `grace_days=0` purges immediately (the admin "purge now" path); None uses config. 0 quota.""" grace = sysconfig.get_int(db, "search_grace_days") if grace_days is None else grace_days - cutoff = _now() - timedelta(days=grace) + cutoff = datetime.now(timezone.utc) - timedelta(days=grace) state_exists = select(VideoState.id).where(VideoState.video_id == Video.id).exists() pl_exists = select(PlaylistItem.id).where(PlaylistItem.video_id == Video.id).exists() sub_exists = select(Subscription.id).where(Subscription.channel_id == Video.channel_id).exists() diff --git a/backend/app/sync/maintenance.py b/backend/app/sync/maintenance.py index caebde45..d0a2a247 100644 --- a/backend/app/sync/maintenance.py +++ b/backend/app/sync/maintenance.py @@ -20,7 +20,7 @@ Quota: phase 1 costs ~(flagged / 50) units, phase 2 ~(batch / 50). Both respect backfill reserve as the other jobs so interactive syncs never starve. """ -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone from sqlalchemy import or_, select from sqlalchemy.orm import Session @@ -31,7 +31,6 @@ from app.models import Playlist, PlaylistItem, Video from app.notifications import create_notification from app.sync.runner import get_service_user from app.sync.videos import apply_video_details, parse_dt -from app.utils import now_utc as _now from app.youtube.client import YouTubeClient log = logs.get_logger(__name__) @@ -60,7 +59,7 @@ def _is_abandoned_upcoming(item: dict) -> bool: scheduled = parse_dt(live.get("scheduledStartTime")) if scheduled is None: return False - return _now() - scheduled > timedelta(days=2) + return datetime.now(timezone.utc) - scheduled > timedelta(days=2) def _evaluate(video: Video, item: dict | None) -> str | None: @@ -225,7 +224,7 @@ def run_maintenance(db: Session) -> dict: return {"reason": "no service user"} # One `now` for the whole run: each phase taking its own let a video phase 1 had just recovered # (stamped with phase 1's) pass phase 2's "not checked this run" test and be fetched twice. - now = _now() + now = datetime.now(timezone.utc) with YouTubeClient(db, user) as yt: phase1 = _recheck_flagged(db, yt, now) phase2 = ( diff --git a/backend/app/sync/videos.py b/backend/app/sync/videos.py index d725df3f..80dc772f 100644 --- a/backend/app/sync/videos.py +++ b/backend/app/sync/videos.py @@ -22,10 +22,6 @@ log = logs.get_logger(__name__) _DURATION_RE = re.compile(r"P(?:(\d+)D)?T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+)S)?", re.IGNORECASE) -def _now() -> datetime: - return datetime.now(timezone.utc) - - def parse_iso8601_duration(value: str | None) -> int | None: if not value: return None @@ -80,7 +76,7 @@ def apply_rss_feed(db: Session, channel: Channel, entries: list[dict]) -> int: time. DB-only — must run on the session's own thread; the network fetch (fetch_channel_feed) is kept separate so callers like run_rss_poll can parallelise it.""" inserted = insert_stubs(db, entries) - channel.last_rss_at = _now() + channel.last_rss_at = datetime.now(timezone.utc) db.add(channel) db.commit() return inserted @@ -96,7 +92,9 @@ def stub_from_playlist_item(item: dict, channel_id: str) -> dict | None: # published_at is NOT NULL (migration 0068); fall back to now() for the rare item that carries no # date at all, so the bulk stub insert never writes a NULL. The enrich pass replaces it if the # video's own snippet later yields a real publish date. - published = parse_dt(content.get("videoPublishedAt") or snippet.get("publishedAt")) or _now() + published = parse_dt( + content.get("videoPublishedAt") or snippet.get("publishedAt") + ) or datetime.now(timezone.utc) return { "id": video_id, "channel_id": channel_id, @@ -124,12 +122,14 @@ def backfill_channel_recent(db: Session, yt: YouTubeClient, channel: Channel) -> """First pass: newest videos up to the configured count or age cutoff. Records a resume cursor for the later deep backfill.""" if not channel.uploads_playlist_id: - channel.recent_synced_at = _now() + channel.recent_synced_at = datetime.now(timezone.utc) db.add(channel) db.commit() return 0 - cutoff = _now() - timedelta(days=sysconfig.get_int(db, "backfill_recent_max_days")) + cutoff = datetime.now(timezone.utc) - timedelta( + days=sysconfig.get_int(db, "backfill_recent_max_days") + ) collected: list[dict] = [] page_token: str | None = None next_cursor: str | None = None @@ -164,7 +164,7 @@ def backfill_channel_recent(db: Session, yt: YouTubeClient, channel: Channel) -> break next_cursor = page_token # whole page consumed, more pages remain - channel.recent_synced_at = _now() + channel.recent_synced_at = datetime.now(timezone.utc) channel.backfill_cursor = next_cursor channel.backfill_done = done inserted = insert_stubs(db, collected) @@ -264,7 +264,7 @@ def apply_video_details(video: Video, item: dict) -> None: if snippet_published: video.published_at = snippet_published elif not video.published_at: - video.published_at = _now() + video.published_at = datetime.now(timezone.utc) video.thumbnail_url = best_thumbnail(snippet.get("thumbnails")) or video.thumbnail_url video.duration_seconds = parse_iso8601_duration(content.get("duration")) video.view_count = int(stats["viewCount"]) if stats.get("viewCount") else None @@ -296,7 +296,7 @@ def _apply_video_batch( if not videos: return 0 items = {it["id"]: it for it in yt.get_videos([v.id for v in videos])} - now = _now() + now = datetime.now(timezone.utc) applied = 0 for video in videos: item = items.get(video.id) @@ -312,7 +312,7 @@ def _apply_video_batch( def _mark_enriched(video: Video) -> None: # Unavailable (deleted/private): stamp so we don't keep retrying. - video.enriched_at = _now() + video.enriched_at = datetime.now(timezone.utc) def enrich_pending(db: Session, yt: YouTubeClient, limit: int | None = None) -> int: diff --git a/backend/app/utils.py b/backend/app/utils.py index 114c57d6..2493cafb 100644 --- a/backend/app/utils.py +++ b/backend/app/utils.py @@ -1,7 +1,7 @@ """Small cross-cutting helpers shared across modules (kept dependency-light on purpose).""" import re -from datetime import date, datetime, timezone +from datetime import date def iso(dt: date | None) -> str | None: @@ -55,11 +55,6 @@ def canonical_email(value: str | None) -> str: return f"{local}@gmail.com" if local else email -def now_utc() -> datetime: - """Current time as a timezone-aware UTC datetime.""" - return datetime.now(timezone.utc) - - # The shortest last word that is prefix-matched. A 1–2 letter prefix matches a large share of the # library and ts_rank then scores every match: "m" took ~5–8 s on prod against ~100 ms for a whole # word (E31 S4e). A shorter last word is matched whole instead ("tv", "4k" still find themselves). diff --git a/backend/tests/test_canonical_patterns.py b/backend/tests/test_canonical_patterns.py new file mode 100644 index 00000000..7c347b12 --- /dev/null +++ b/backend/tests/test_canonical_patterns.py @@ -0,0 +1,52 @@ +"""Two rows of CLAUDE.md's canonical-patterns table, held in code (E35 S4). + +Both had drifted while the table said otherwise: seven `db.query(...)` calls stood beside the +`select()` the table names, and four modules carried a private `_now` / `now_utc` helper (nine uses) +where the table says inline `datetime.now(timezone.utc)`. A rule only prose holds drifts; ruff cannot +ban a method call on an instance or a function's name, so this reads the AST of every app module. +Pure-logic: no DB. +""" + +import ast +import pathlib +import re + +import app + +APP = pathlib.Path(app.__file__).resolve().parent +NOW_HELPER = re.compile(r"_?now(_\w+)?|now_utc|utcnow") + + +def _modules() -> list[tuple[str, ast.Module]]: + return [ + (p.relative_to(APP.parent).as_posix(), ast.parse(p.read_text(encoding="utf-8"))) + for p in sorted(APP.rglob("*.py")) + ] + + +def test_queries_use_select_not_the_legacy_query_api(): + found = [ + f"{path}:{node.lineno}" + for path, tree in _modules() + for node in ast.walk(tree) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "query" + ] + assert found == [], "use db.execute(select(...)) — SQLAlchemy 2.0 — not db.query(...)" + + +def test_the_current_time_is_inline_not_a_helper(): + found = [ + f"{path}:{node.lineno} {node.name}" + for path, tree in _modules() + for node in ast.walk(tree) + if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef) + and NOW_HELPER.fullmatch(node.name) + ] + assert found == [], "write datetime.now(timezone.utc) inline, not a now()/_now() helper" + + +def test_the_scan_sees_the_app(): + # a scan that reads nothing is green over nothing + assert len(_modules()) > 50 diff --git a/backend/tests/test_maintenance_lifecycle.py b/backend/tests/test_maintenance_lifecycle.py index 5973065e..a8698771 100644 --- a/backend/tests/test_maintenance_lifecycle.py +++ b/backend/tests/test_maintenance_lifecycle.py @@ -7,14 +7,13 @@ trigger, and an upcoming premiere that never went live is. YouTube is a fake tha videos.list from a dict. """ -from datetime import timedelta +from datetime import datetime, timedelta, timezone import pytest from sqlalchemy import select from app.models import Channel, Notification, Playlist, PlaylistItem, Video from app.sync import maintenance -from app.utils import now_utc class _YT: @@ -41,11 +40,14 @@ def channel(db): def test_a_flagged_video_that_answers_again_is_unhidden(db, channel): v = Video( - id="v1", channel_id=channel.id, unavailable_since=now_utc(), unavailable_reason="removed" + id="v1", + channel_id=channel.id, + unavailable_since=datetime.now(timezone.utc), + unavailable_reason="removed", ) db.add(v) db.commit() - out = maintenance._recheck_flagged(db, _YT({"v1": _item("v1")}), now_utc()) + out = maintenance._recheck_flagged(db, _YT({"v1": _item("v1")}), datetime.now(timezone.utc)) db.refresh(v) assert v.unavailable_since is None and v.unavailable_reason is None assert out == {"checked": 1, "recovered": 1, "deleted": 0, "notified": 0} @@ -55,12 +57,13 @@ def test_a_video_still_missing_inside_its_grace_period_is_kept_hidden(db, channe v = Video( id="v1", channel_id=channel.id, - unavailable_since=now_utc() - timedelta(days=maintenance.DEFAULT_GRACE_DAYS - 1), + unavailable_since=datetime.now(timezone.utc) + - timedelta(days=maintenance.DEFAULT_GRACE_DAYS - 1), unavailable_reason="removed", ) db.add(v) db.commit() - out = maintenance._recheck_flagged(db, _YT({}), now_utc()) + out = maintenance._recheck_flagged(db, _YT({}), datetime.now(timezone.utc)) assert db.get(Video, "v1") is not None assert out["deleted"] == 0 @@ -69,7 +72,7 @@ def test_past_the_grace_period_it_is_deleted_and_each_user_who_kept_it_told_once db, channel, make_user ): a, b = make_user(), make_user() - gone_since = now_utc() - timedelta(days=maintenance.DEFAULT_GRACE_DAYS + 1) + gone_since = datetime.now(timezone.utc) - timedelta(days=maintenance.DEFAULT_GRACE_DAYS + 1) for vid in ("v1", "v2"): db.add( Video( @@ -92,7 +95,7 @@ def test_past_the_grace_period_it_is_deleted_and_each_user_who_kept_it_told_once ] ) db.commit() - out = maintenance._recheck_flagged(db, _YT({}), now_utc()) + out = maintenance._recheck_flagged(db, _YT({}), datetime.now(timezone.utc)) db.expire_all() assert db.get(Video, "v1") is None and db.get(Video, "v2") is None assert db.execute(select(PlaylistItem)).scalars().all() == [] # the cascade @@ -118,7 +121,7 @@ def test_rolling_revalidation_flags_the_missing_and_keeps_a_live_stream(db, chan ) db.commit() yt = _YT({"ok": _item("ok"), "live": _item("live", liveBroadcastContent="live")}) - out = maintenance._revalidate_rolling(db, yt, now_utc()) + out = maintenance._revalidate_rolling(db, yt, datetime.now(timezone.utc)) db.expire_all() assert db.get(Video, "gone").unavailable_reason == "removed" assert db.get(Video, "ok").unavailable_since is None @@ -150,7 +153,7 @@ def test_a_run_asks_youtube_about_each_video_once(db, channel, monkeypatch, make Video( id="v1", channel_id=channel.id, - unavailable_since=now_utc(), + unavailable_since=datetime.now(timezone.utc), unavailable_reason="removed", ), Video(id="v2", channel_id=channel.id), @@ -163,8 +166,8 @@ def test_a_run_asks_youtube_about_each_video_once(db, channel, monkeypatch, make def test_an_upcoming_premiere_counts_as_abandoned_only_when_it_never_went_live(): - past = (now_utc() - timedelta(days=3)).isoformat() - soon = (now_utc() - timedelta(days=1)).isoformat() + past = (datetime.now(timezone.utc) - timedelta(days=3)).isoformat() + soon = (datetime.now(timezone.utc) - timedelta(days=1)).isoformat() def item(**live): return {"snippet": {"liveBroadcastContent": "upcoming"}, "liveStreamingDetails": live} diff --git a/scripts/e2e_seed.py b/scripts/e2e_seed.py index 1cb55b2e..3ef64bbd 100644 --- a/scripts/e2e_seed.py +++ b/scripts/e2e_seed.py @@ -45,10 +45,6 @@ if not PASSWORD: N_SUBS = 3 -def _now() -> datetime: - return datetime.now(timezone.utc) - - def main() -> None: db = SessionLocal() try: @@ -99,7 +95,11 @@ def main() -> None: db.execute(delete(Subscription).where(Subscription.user_id == user.id)) for cid in channel_ids: - db.add(Subscription(user_id=user.id, channel_id=cid, subscribed_at=_now())) + db.add( + Subscription( + user_id=user.id, channel_id=cid, subscribed_at=datetime.now(timezone.utc) + ) + ) db.commit() # 3. Deterministic watch state (reset each run): @@ -133,7 +133,7 @@ def main() -> None: video_id=vid.id, status="new", position_seconds=42, - progress_updated_at=_now(), + progress_updated_at=datetime.now(timezone.utc), ) ) in_progress = vid.id @@ -146,7 +146,7 @@ def main() -> None: user_id=user.id, video_id=wv.id, status="watched", - watched_at=_now(), + watched_at=datetime.now(timezone.utc), ) ) watched = wv.id