From c5533b0ac5132e97a6a7cc82bd68de441c51189b Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 26 Jul 2026 04:17:08 +0200 Subject: [PATCH 01/18] refactor(api): shared serializer + ownership + write-scope helpers (R6 S1a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Establish the R6 API-contract infra before the S1b payload:dict migration: - iso(dt) in utils.py replaces the ~16 hand-rolled `x.isoformat() if x else None` serializer idioms across the routes. - get_or_404 / owned_or_404 in routes/_common.py — one 404 + existence-leak policy; plex's _own_playlist_or_404 and saved_views' _own_view now delegate to it. - require_write_scope dependency in auth.py collapses the inline has_write_scope→403 checks (channels subscribe/unsubscribe, playlists push/push-plan) that had drifted into three different messages into one; the conditional delete_playlist guard stays inline (its message was already canonical). No response-shape change (iso == isoformat for a present value). Gate: ruff clean, pytest 163 green (+ test_utils for iso). --- backend/app/auth.py | 13 +++++++++ backend/app/routes/_common.py | 44 +++++++++++++++++++++++++++++ backend/app/routes/admin.py | 10 +++---- backend/app/routes/audit.py | 3 +- backend/app/routes/channels.py | 32 ++++++++------------- backend/app/routes/downloads.py | 7 +++-- backend/app/routes/feed.py | 5 ++-- backend/app/routes/messages.py | 5 ++-- backend/app/routes/notifications.py | 3 +- backend/app/routes/playlists.py | 19 ++++--------- backend/app/routes/plex.py | 13 +++------ backend/app/routes/saved_views.py | 6 ++-- backend/app/utils.py | 7 +++++ backend/tests/test_utils.py | 25 ++++++++++++++++ 14 files changed, 132 insertions(+), 60 deletions(-) create mode 100644 backend/app/routes/_common.py create mode 100644 backend/tests/test_utils.py diff --git a/backend/app/auth.py b/backend/app/auth.py index 024f62a..1723e25 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -1010,6 +1010,19 @@ def require_human(user: User = Depends(current_user)) -> User: return user +def require_write_scope(user: User = Depends(require_human)) -> User: + """Dependency: require the YouTube write scope (subscribe/unsubscribe, playlist export/edit). + Single source of truth for the write gate — replaces the inline `if not has_write_scope(user)` + checks that had drifted into three different 403 messages across channels/playlists. Layers on + `require_human` because writing to YouTube needs a real, non-demo identity.""" + if not has_write_scope(user): + raise HTTPException( + status_code=403, + detail="Enable YouTube editing in Settings first.", + ) + return user + + def admin_user(user: User = Depends(current_user)) -> User: """Dependency: require the signed-in user to be an admin. Single source of truth for the admin gate — every admin route/action depends on this rather than re-checking the role.""" diff --git a/backend/app/routes/_common.py b/backend/app/routes/_common.py new file mode 100644 index 0000000..9fa458a --- /dev/null +++ b/backend/app/routes/_common.py @@ -0,0 +1,44 @@ +"""Route-layer shared helpers for the API contract (R6 S1a). + +Small, dependency-light guards that several route modules had hand-rolled with subtle +drift. Keeping them here (imported by the route modules) means the "fetch a row or 404" +and "fetch a row this user owns or 404" idioms have one definition, one status code, and +one existence-leak policy. +""" +from typing import Any, TypeVar + +from fastapi import HTTPException +from sqlalchemy.orm import Session + +from app.db import Base +from app.models import User + +M = TypeVar("M", bound=Base) + + +def get_or_404(db: Session, model: type[M], id_: Any, *, detail: str = "Not found") -> M: + """Fetch a row by primary key or raise 404. Replaces the `db.get(Model, id); if None: 404` + idiom repeated across routes.""" + obj = db.get(model, id_) + if obj is None: + raise HTTPException(status_code=404, detail=detail) + return obj + + +def owned_or_404( + db: Session, + model: type[M], + id_: Any, + user: User, + *, + owner_attr: str = "user_id", + detail: str = "Not found", +) -> M: + """Fetch a row by primary key that `user` owns, or raise 404. A row owned by someone + else 404s (NOT 403) on purpose — a 403 would confirm the id exists to a stranger. + Generalizes the near-identical per-route ownership helpers (e.g. plex's + `_own_playlist_or_404`, the saved-views owner check).""" + obj = get_or_404(db, model, id_, detail=detail) + if getattr(obj, owner_attr) != user.id: + raise HTTPException(status_code=404, detail=detail) + return obj diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index c507c6c..c319756 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -27,7 +27,7 @@ from app.models import ( Video, VideoState, ) -from app.utils import valid_email +from app.utils import iso, valid_email router = APIRouter(prefix="/api/admin", tags=["admin"]) @@ -38,8 +38,8 @@ def _serialize(inv: Invite) -> dict: "email": inv.email, "status": inv.status, "note": inv.note, - "requested_at": inv.requested_at.isoformat() if inv.requested_at else None, - "decided_at": inv.decided_at.isoformat() if inv.decided_at else None, + "requested_at": iso(inv.requested_at), + "decided_at": iso(inv.decided_at), "decided_by": inv.decided_by, } @@ -147,7 +147,7 @@ def _serialize_user(u: User) -> dict: "is_demo": u.is_demo, "has_password": u.password_hash is not None, "has_google": u.google_sub is not None, - "created_at": u.created_at.isoformat() if u.created_at else None, + "created_at": iso(u.created_at), } @@ -280,7 +280,7 @@ def _serialize_demo(row: DemoWhitelist) -> dict: "email": row.email, "note": row.note, "added_by": row.added_by, - "created_at": row.created_at.isoformat() if row.created_at else None, + "created_at": iso(row.created_at), } diff --git a/backend/app/routes/audit.py b/backend/app/routes/audit.py index 206a32c..deffc69 100644 --- a/backend/app/routes/audit.py +++ b/backend/app/routes/audit.py @@ -10,6 +10,7 @@ from sqlalchemy.orm import Session from app import audit from app.audit import AuditAction from app.db import get_db +from app.utils import iso from app.models import AuditLog, User from app.routes.admin import admin_user @@ -23,7 +24,7 @@ MAX_ROWS = 2000 def _serialize(row: AuditLog, actors: dict[int, str]) -> dict: return { "id": row.id, - "created_at": row.created_at.isoformat() if row.created_at else None, + "created_at": iso(row.created_at), # actor_id NULL = the scheduler/background did it (client shows "System"); a set id that # can't be resolved would mean a deleted user, but SET NULL makes that NULL — so present. "actor": actors.get(row.actor_id) if row.actor_id is not None else None, diff --git a/backend/app/routes/channels.py b/backend/app/routes/channels.py index 6b3c4da..0f02eb2 100644 --- a/backend/app/routes/channels.py +++ b/backend/app/routes/channels.py @@ -9,8 +9,9 @@ from sqlalchemy import func, select, update from sqlalchemy.orm import Session from app import quota, sysconfig -from app.auth import current_user, has_write_scope, require_human +from app.auth import current_user, require_human, require_write_scope from app.db import get_db +from app.utils import iso from app.models import ( LIVE_OR_UPCOMING, BlockedChannel, @@ -67,7 +68,7 @@ def list_channels( ).all(): agg[cid] = { "stored": n, - "last_video_at": last_up.isoformat() if last_up else None, + "last_video_at": iso(last_up), "total_duration_seconds": int(total_dur or 0), "count_short": int(shorts), "count_live": int(live), @@ -232,7 +233,7 @@ def _channel_detail_dict( "subscriber_count": channel.subscriber_count, "video_count": channel.video_count, "total_view_count": channel.total_view_count, - "published_at": channel.published_at.isoformat() if channel.published_at else None, + "published_at": iso(channel.published_at), "external_links": channel.external_links or [], "country": channel.country, "default_language": channel.default_language, @@ -454,19 +455,14 @@ def reset_backfill( @router.post("/{channel_id}/subscribe") def subscribe( channel_id: str, - user: User = Depends(current_user), + user: User = Depends(require_write_scope), db: Session = Depends(get_db), ) -> dict: """Subscribe to a channel discovered from the user's playlists. Gated behind the - optional write scope (YouTube subscriptions.insert). Creates the local subscription - with YouTube's returned resource id and enriches the channel's metadata if it's still - a stub, so the new row renders properly in the manager. It does NOT pull the channel's - videos — the scheduler picks those up on its next run, like any subscribed channel.""" - if not has_write_scope(user): - raise HTTPException( - status_code=403, - detail="Enable playlist editing in Settings to subscribe on YouTube.", - ) + optional write scope (YouTube subscriptions.insert) via require_write_scope. Creates the + local subscription with YouTube's returned resource id and enriches the channel's metadata + if it's still a stub, so the new row renders properly in the manager. It does NOT pull the + channel's videos — the scheduler picks those up on its next run, like any subscribed channel.""" channel = db.get(Channel, channel_id) if channel is None: raise HTTPException(status_code=404, detail="Unknown channel") @@ -524,16 +520,12 @@ def subscribe( @router.delete("/{channel_id}/subscription") def unsubscribe( channel_id: str, - user: User = Depends(current_user), + user: User = Depends(require_write_scope), db: Session = Depends(get_db), ) -> dict: """Unsubscribe from this channel on YouTube and drop the local subscription. Gated - behind the optional write scope; read-only users get a 403 and should Hide instead.""" - if not has_write_scope(user): - raise HTTPException( - status_code=403, - detail="Enable playlist editing in Settings to unsubscribe on YouTube.", - ) + behind the optional write scope (require_write_scope); read-only users get a 403 and + should Hide instead.""" sub = _user_subscription(db, user, channel_id) if not sub.yt_subscription_id: raise HTTPException( diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index f3b4943..2d95a87 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -18,6 +18,7 @@ from app import sysconfig from app.auth import admin_user, require_human from app.config import settings from app.db import get_db +from app.utils import iso from app.downloads import edit as editmod from app.downloads import links as linksmod from app.downloads import formats, quota, service, storage @@ -80,7 +81,7 @@ def _serialize(job: DownloadJob, asset: MediaAsset | None) -> dict: "phase": job.phase, "error": job.error, "queue_pos": job.queue_pos, - "created_at": job.created_at.isoformat() if job.created_at else None, + "created_at": iso(job.created_at), "source_kind": job.source_kind, "source_ref": job.source_ref, "source_url": service.reference_url(job, asset), @@ -106,7 +107,7 @@ def _serialize(job: DownloadJob, asset: MediaAsset | None) -> dict: "status": asset.status, "title": asset.title, "uploader": asset.uploader, - "upload_date": asset.upload_date.isoformat() if asset.upload_date else None, + "upload_date": iso(asset.upload_date), "duration_s": asset.duration_s, "size_bytes": asset.size_bytes, "width": asset.width, @@ -114,7 +115,7 @@ def _serialize(job: DownloadJob, asset: MediaAsset | None) -> dict: "container": asset.container, "uploader_url": asset.uploader_url, "thumbnail_url": asset.thumbnail_url, - "expires_at": asset.expires_at.isoformat() if asset.expires_at else None, + "expires_at": iso(asset.expires_at), }, } diff --git a/backend/app/routes/feed.py b/backend/app/routes/feed.py index 443ee19..71b0dd5 100644 --- a/backend/app/routes/feed.py +++ b/backend/app/routes/feed.py @@ -13,6 +13,7 @@ from sqlalchemy.orm.exc import StaleDataError from app import quota from app.auth import current_user from app.db import get_db +from app.utils import iso from app.models import ( LIVE_OR_UPCOMING, BlockedChannel, @@ -97,7 +98,7 @@ def _serialize(row) -> dict: "channel_title": row.channel_title, "channel_thumbnail": row.channel_thumbnail, "channel_url": _channel_url(row.channel_id, row.channel_handle), - "published_at": row.published_at.isoformat() if row.published_at else None, + "published_at": iso(row.published_at), "thumbnail_url": row.thumbnail_url, "duration_seconds": row.duration_seconds, "view_count": row.view_count, @@ -732,7 +733,7 @@ def get_video_detail( "in_db": True, "channel_id": v.channel_id, "channel_title": v.channel.title if v.channel else None, - "published_at": v.published_at.isoformat() if v.published_at else None, + "published_at": iso(v.published_at), "view_count": v.view_count, "duration_seconds": v.duration_seconds, } diff --git a/backend/app/routes/messages.py b/backend/app/routes/messages.py index 7ac772e..b953690 100644 --- a/backend/app/routes/messages.py +++ b/backend/app/routes/messages.py @@ -27,6 +27,7 @@ from app.ratelimit import RateLimiter from app.realtime import manager from app.security import verify_password from app.userscope import is_messageable_user, messageable_clauses +from app.utils import iso router = APIRouter(prefix="/api/messages", tags=["messages"]) @@ -97,8 +98,8 @@ def _serialize_msg(m: Message) -> dict: "body": m.body, # only set for system messages "ciphertext": m.ciphertext, "iv": m.iv, - "read_at": m.read_at.isoformat() if m.read_at else None, - "created_at": m.created_at.isoformat() if m.created_at else None, + "read_at": iso(m.read_at), + "created_at": iso(m.created_at), } diff --git a/backend/app/routes/notifications.py b/backend/app/routes/notifications.py index 224a7f7..2ee03ea 100644 --- a/backend/app/routes/notifications.py +++ b/backend/app/routes/notifications.py @@ -11,6 +11,7 @@ from sqlalchemy.orm import Session from app.auth import current_user from app.db import get_db from app.models import Notification, User +from app.utils import iso from app.notifications import trim_read router = APIRouter(prefix="/api/me/notifications", tags=["notifications"]) @@ -25,7 +26,7 @@ def _serialize(n: Notification) -> dict: "data": n.data, "read": n.read, "dismissed": n.dismissed, - "created_at": n.created_at.isoformat() if n.created_at else None, + "created_at": iso(n.created_at), } diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index e37d625..4d8e02a 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -6,7 +6,7 @@ from sqlalchemy import func, select, and_ from sqlalchemy.orm import Session, aliased from app import quota -from app.auth import current_user, has_read_scope, has_write_scope +from app.auth import current_user, has_read_scope, has_write_scope, require_write_scope from app.db import get_db from app.models import Channel, Playlist, PlaylistItem, User, Video, VideoState from app.routes.feed import _serialize, feed_columns @@ -316,16 +316,13 @@ def _pushable(db: Session, user: User, playlist_id: int) -> Playlist: @router.get("/{playlist_id}/push-plan") def push_plan( playlist_id: int, - user: User = Depends(current_user), + user: User = Depends(require_write_scope), db: Session = Depends(get_db), ) -> dict: """Dry run: what 'Sync to YouTube' would do + the quota estimate, so the UI can show a - confirm. For a linked playlist this reads the live YouTube state (cheap, 1 unit/page).""" + confirm. For a linked playlist this reads the live YouTube state (cheap, 1 unit/page). + Write scope is required (require_write_scope) — syncing is a YouTube write.""" pl = _pushable(db, user, playlist_id) - if not has_write_scope(user): - raise HTTPException( - status_code=403, detail="Enable YouTube editing in Settings first." - ) try: with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH): plan = plan_push(db, user, pl) @@ -339,17 +336,13 @@ def push_plan( @router.post("/{playlist_id}/push") def push( playlist_id: int, - user: User = Depends(current_user), + user: User = Depends(require_write_scope), db: Session = Depends(get_db), ) -> dict: """Push a local playlist to YouTube (create it on first push, else reconcile to match local — local wins). Refuses if the estimated quota exceeds what's left today, so a - big playlist can't strand half-pushed.""" + big playlist can't strand half-pushed. Write scope required (require_write_scope).""" pl = _pushable(db, user, playlist_id) - if not has_write_scope(user): - raise HTTPException( - status_code=403, detail="Enable YouTube editing in Settings first." - ) try: with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH): # Read the live playlist ONCE (linked playlists only) and share it with both the diff --git a/backend/app/routes/plex.py b/backend/app/routes/plex.py index 2f82edf..97cc4f4 100644 --- a/backend/app/routes/plex.py +++ b/backend/app/routes/plex.py @@ -41,6 +41,8 @@ from app.models import ( PlexState, User, ) +from app.routes._common import owned_or_404 +from app.utils import iso from app.plex import paths as plex_paths from app.plex import people as plex_people from app.plex import stream as plex_stream @@ -141,11 +143,7 @@ def _link_dict(link: PlexLink | None) -> dict: "sync_enabled": bool(link and link.sync_enabled), "uses_admin": bool(link and link.uses_admin), "initial_import_done": bool(link and link.initial_import_done), - "last_watch_sync_at": ( - link.last_watch_sync_at.isoformat() - if link and link.last_watch_sync_at - else None - ), + "last_watch_sync_at": iso(link.last_watch_sync_at) if link else None, } @@ -912,10 +910,7 @@ def collection_set_editable( # --- Playlists (Siftlode-native, per-user, ordered — no Plex account needed; Plex sync is a later phase). def _own_playlist_or_404(db: Session, pid: int, user: User) -> PlexPlaylist: - pl = db.query(PlexPlaylist).filter_by(id=pid, user_id=user.id).first() - if pl is None: - raise HTTPException(status_code=404, detail="Playlist not found") - return pl + return owned_or_404(db, PlexPlaylist, pid, user, detail="Playlist not found") def _playlist_card(pl: PlexPlaylist, count: int, thumb_rk: str | None) -> dict: diff --git a/backend/app/routes/saved_views.py b/backend/app/routes/saved_views.py index dd8a41e..7a2f9fe 100644 --- a/backend/app/routes/saved_views.py +++ b/backend/app/routes/saved_views.py @@ -6,6 +6,7 @@ from sqlalchemy.orm import Session from app.auth import require_human from app.db import get_db from app.models import SavedView, User +from app.routes._common import owned_or_404 # Saved "smart views": per-user named snapshots of the feed's FeedFilters. Gated behind # require_human so the shared demo account can't pollute a communal list (mirrors how other @@ -71,10 +72,7 @@ def create_view( def _own_view(db: Session, user: User, view_id: int) -> SavedView: - view = db.get(SavedView, view_id) - if view is None or view.user_id != user.id: - raise HTTPException(status_code=404, detail="Unknown view") - return view + return owned_or_404(db, SavedView, view_id, user, detail="Unknown view") @router.patch("/{view_id}") diff --git a/backend/app/utils.py b/backend/app/utils.py index 70feb39..866e6e0 100644 --- a/backend/app/utils.py +++ b/backend/app/utils.py @@ -2,6 +2,13 @@ import re from datetime import datetime, timezone + +def iso(dt: datetime | None) -> str | None: + """Serialize a datetime for a JSON response: ISO-8601, or None when the column is NULL. + Single source of truth for the `x.isoformat() if x else None` idiom the routes had + hand-rolled ~17 times — so a future tz/format tweak lands in one place.""" + return dt.isoformat() if dt is not None else None + # Single source of truth for "looks like an email" — used by every endpoint that accepts an # address (auth register/reset/demo, admin invites + demo whitelist, the setup wizard). Keep # the strictness in one place so routes don't drift between this and a looser "@"-in check. diff --git a/backend/tests/test_utils.py b/backend/tests/test_utils.py new file mode 100644 index 0000000..9c6f5d3 --- /dev/null +++ b/backend/tests/test_utils.py @@ -0,0 +1,25 @@ +"""iso(): the single datetime→JSON serializer the routes share (R6 S1a).""" +from datetime import datetime, timezone + +from app.utils import iso + + +def test_none_maps_to_none(): + assert iso(None) is None + + +def test_aware_datetime_keeps_offset(): + dt = datetime(2026, 7, 26, 12, 30, 0, tzinfo=timezone.utc) + assert iso(dt) == "2026-07-26T12:30:00+00:00" + + +def test_naive_datetime_serializes_without_offset(): + dt = datetime(2026, 7, 26, 12, 30, 0) + assert iso(dt) == "2026-07-26T12:30:00" + + +def test_matches_datetime_isoformat_contract(): + # iso is exactly `dt.isoformat()` for a present value — guards the routes that used to + # hand-roll `x.isoformat() if x else None`. + dt = datetime(2020, 1, 2, 3, 4, 5, 678901, tzinfo=timezone.utc) + assert iso(dt) == dt.isoformat() From cf8bbad8a17862c96104b40b4221d4dced973059 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 26 Jul 2026 04:25:40 +0200 Subject: [PATCH 02/18] =?UTF-8?q?refactor(api):=20code-review=20fixes=20?= =?UTF-8?q?=E2=80=94=20widen=20iso()=20to=20date,=20fix=20import=20order?= =?UTF-8?q?=20(R6=20S1a)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /code-review high round 1 (5 findings): - iso(dt) now typed `date | None` — MediaAsset.upload_date is a plain `date`, not a datetime; the helper's signature was too narrow for the type-contract epic. +test. - restore alphabetical app.* import order in audit.py and plex.py (the new iso/_common imports had been dropped in mid-block). The other 3 findings are deliberate design choices, left as-is for UAT: push/push-plan now gate write-scope before the 404 (dependency ordering); the unified 403 message; and owned_or_404's owner_attr kept for S1b's DownloadShare.to_user_id-style owners. Gate: ruff clean, pytest 164 green. --- backend/app/routes/audit.py | 2 +- backend/app/routes/plex.py | 4 ++-- backend/app/utils.py | 11 ++++++----- backend/tests/test_utils.py | 9 +++++++-- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/backend/app/routes/audit.py b/backend/app/routes/audit.py index deffc69..9e7a4ef 100644 --- a/backend/app/routes/audit.py +++ b/backend/app/routes/audit.py @@ -10,9 +10,9 @@ from sqlalchemy.orm import Session from app import audit from app.audit import AuditAction from app.db import get_db -from app.utils import iso from app.models import AuditLog, User from app.routes.admin import admin_user +from app.utils import iso router = APIRouter(prefix="/api/admin/audit", tags=["admin"]) diff --git a/backend/app/routes/plex.py b/backend/app/routes/plex.py index 97cc4f4..ce0549f 100644 --- a/backend/app/routes/plex.py +++ b/backend/app/routes/plex.py @@ -41,16 +41,16 @@ from app.models import ( PlexState, User, ) -from app.routes._common import owned_or_404 -from app.utils import iso from app.plex import paths as plex_paths from app.plex import people as plex_people from app.plex import stream as plex_stream from app.plex import sync as plex_sync from app.plex import watch_sync as plex_watch from app.plex.client import PlexClient, PlexError, PlexNotConfigured +from app.routes._common import owned_or_404 from app.routes.admin import admin_user from app.routes.feed import _to_tsquery_str +from app.utils import iso log = logging.getLogger("siftlode.plex") diff --git a/backend/app/utils.py b/backend/app/utils.py index 866e6e0..d266187 100644 --- a/backend/app/utils.py +++ b/backend/app/utils.py @@ -1,12 +1,13 @@ """Small cross-cutting helpers shared across modules (kept dependency-light on purpose).""" import re -from datetime import datetime, timezone +from datetime import date, datetime, timezone -def iso(dt: datetime | None) -> str | None: - """Serialize a datetime for a JSON response: ISO-8601, or None when the column is NULL. - Single source of truth for the `x.isoformat() if x else None` idiom the routes had - hand-rolled ~17 times — so a future tz/format tweak lands in one place.""" +def iso(dt: date | None) -> str | None: + """Serialize a date/datetime for a JSON response: ISO-8601, or None when the column is + NULL. Single source of truth for the `x.isoformat() if x else None` idiom the routes had + hand-rolled ~17 times — so a future tz/format tweak lands in one place. Accepts `date` + too (datetime is a subclass) — e.g. MediaAsset.upload_date is a plain `date`.""" return dt.isoformat() if dt is not None else None # Single source of truth for "looks like an email" — used by every endpoint that accepts an diff --git a/backend/tests/test_utils.py b/backend/tests/test_utils.py index 9c6f5d3..3e93b7d 100644 --- a/backend/tests/test_utils.py +++ b/backend/tests/test_utils.py @@ -1,5 +1,5 @@ -"""iso(): the single datetime→JSON serializer the routes share (R6 S1a).""" -from datetime import datetime, timezone +"""iso(): the single date/datetime→JSON serializer the routes share (R6 S1a).""" +from datetime import date, datetime, timezone from app.utils import iso @@ -8,6 +8,11 @@ def test_none_maps_to_none(): assert iso(None) is None +def test_plain_date_serializes_as_iso_date(): + # MediaAsset.upload_date is a `date`, not a datetime — the contract must accept it. + assert iso(date(2026, 7, 26)) == "2026-07-26" + + def test_aware_datetime_keeps_offset(): dt = datetime(2026, 7, 26, 12, 30, 0, tzinfo=timezone.utc) assert iso(dt) == "2026-07-26T12:30:00+00:00" From 9fe050279ec219d72ad64c1efe534c2de1750a5d Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 26 Jul 2026 04:44:22 +0200 Subject: [PATCH 03/18] style(api): complete the S1a import-order fix (R6 S1a follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 review finding #4 named only audit.py/plex.py, but the same misplaced `from app.utils import iso` sat in channels/downloads/feed/notifications too — the round missed them (ruff enforces no import order here, so the gate was silent). Move them to alphabetical position for consistency. No behaviour change. --- backend/app/routes/channels.py | 2 +- backend/app/routes/downloads.py | 2 +- backend/app/routes/feed.py | 2 +- backend/app/routes/notifications.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/backend/app/routes/channels.py b/backend/app/routes/channels.py index 0f02eb2..32287ac 100644 --- a/backend/app/routes/channels.py +++ b/backend/app/routes/channels.py @@ -11,7 +11,6 @@ from sqlalchemy.orm import Session from app import quota, sysconfig from app.auth import current_user, require_human, require_write_scope from app.db import get_db -from app.utils import iso from app.models import ( LIVE_OR_UPCOMING, BlockedChannel, @@ -29,6 +28,7 @@ from app.routes.admin import admin_user from app.sync.explore import explore_ingest_page from app.sync.runner import run_recent_backfill from app.sync.subscriptions import apply_channel_details +from app.utils import iso from app.youtube.client import YouTubeClient, YouTubeError router = APIRouter(prefix="/api/channels", tags=["channels"]) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 2d95a87..10c45c1 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -18,7 +18,6 @@ from app import sysconfig from app.auth import admin_user, require_human from app.config import settings from app.db import get_db -from app.utils import iso from app.downloads import edit as editmod from app.downloads import links as linksmod from app.downloads import formats, quota, service, storage @@ -33,6 +32,7 @@ from app.models import ( ) from app.security import hash_password from app.userscope import is_messageable_user, messageable_clauses +from app.utils import iso router = APIRouter(prefix="/api/downloads", tags=["downloads"]) admin_router = APIRouter(prefix="/api/admin/downloads", tags=["admin-downloads"]) diff --git a/backend/app/routes/feed.py b/backend/app/routes/feed.py index 71b0dd5..8bfcc5f 100644 --- a/backend/app/routes/feed.py +++ b/backend/app/routes/feed.py @@ -13,7 +13,6 @@ from sqlalchemy.orm.exc import StaleDataError from app import quota from app.auth import current_user from app.db import get_db -from app.utils import iso from app.models import ( LIVE_OR_UPCOMING, BlockedChannel, @@ -30,6 +29,7 @@ from app.models import ( VideoState, ) from app.sync.videos import parse_iso8601_duration +from app.utils import iso from app.youtube.client import YouTubeClient, YouTubeError router = APIRouter(prefix="/api", tags=["feed"]) diff --git a/backend/app/routes/notifications.py b/backend/app/routes/notifications.py index 2ee03ea..132e478 100644 --- a/backend/app/routes/notifications.py +++ b/backend/app/routes/notifications.py @@ -11,8 +11,8 @@ from sqlalchemy.orm import Session from app.auth import current_user from app.db import get_db from app.models import Notification, User -from app.utils import iso from app.notifications import trim_read +from app.utils import iso router = APIRouter(prefix="/api/me/notifications", tags=["notifications"]) From d982e8b9ae56b599e8d031baabd8b278bcd05ec0 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 26 Jul 2026 04:46:40 +0200 Subject: [PATCH 04/18] refactor(api): Pydantic request models for channels routes (R6 S1b) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit update_channel: payload:dict → ChannelUpdateIn (priority/hidden/deep_requested, all optional for PATCH semantics). Kills the `int(payload["priority"])` 500 the prod report hit — a non-numeric/garbage value now returns 422, not 500. attach_tag: payload:dict → AttachTagIn(tag_id:int) — a non-int tag_id was a 500 in `db.get(Tag, ...)`; the personal-tag ownership check reuses owned_or_404. --- backend/app/routes/channels.py | 45 ++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 16 deletions(-) diff --git a/backend/app/routes/channels.py b/backend/app/routes/channels.py index 32287ac..dd04589 100644 --- a/backend/app/routes/channels.py +++ b/backend/app/routes/channels.py @@ -5,6 +5,7 @@ import logging from datetime import datetime, timezone from fastapi import APIRouter, Depends, HTTPException +from pydantic import BaseModel from sqlalchemy import func, select, update from sqlalchemy.orm import Session @@ -24,6 +25,7 @@ from app.models import ( User, Video, ) +from app.routes._common import owned_or_404 from app.routes.admin import admin_user from app.sync.explore import explore_ingest_page from app.sync.runner import run_recent_backfill @@ -378,28 +380,37 @@ def _is_blocked(db: Session, user: User, channel_id: str) -> bool: ) +class ChannelUpdateIn(BaseModel): + # PATCH semantics: a field left unset (None) is untouched. Every field is optional so the + # UI can send just the one control it changed. int/bool coercion + rejection of garbage + # (a non-numeric priority, a non-bool flag) is now Pydantic's job → 422, not a 500. + priority: int | None = None + hidden: bool | None = None + deep_requested: bool | None = None + + @router.patch("/{channel_id}") def update_channel( channel_id: str, - payload: dict, + body: ChannelUpdateIn, user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: sub = _user_subscription(db, user, channel_id) channel = db.get(Channel, channel_id) deep_turned_on = False - if "priority" in payload: - sub.priority = int(payload["priority"]) - if "hidden" in payload: - sub.hidden = bool(payload["hidden"]) + if body.priority is not None: + sub.priority = body.priority + if body.hidden is not None: + sub.hidden = body.hidden # The demo account is SHARED, so it must not persist deep_requested (a quota-relevant flag) onto # the shared subscription — skip the write entirely for demo, which also keeps deep_turned_on # False so the immediate backfill below can never fire for it. - if "deep_requested" in payload and not user.is_demo: + if body.deep_requested is not None and not user.is_demo: # Opt this channel into (or out of) full-history backfill. The deep scheduler # picks the flag up on its next run; turning it off won't undo already-fetched # videos, it just stops further deep paging if nobody else still wants it. - want = bool(payload["deep_requested"]) + want = body.deep_requested deep_turned_on = want and not sub.deep_requested sub.deep_requested = want db.commit() @@ -601,23 +612,25 @@ def unblock_channel( return {"unblocked": channel_id} +class AttachTagIn(BaseModel): + tag_id: int + + @router.post("/{channel_id}/tags") def attach_tag( channel_id: str, - payload: dict, + body: AttachTagIn, user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: _user_subscription(db, user, channel_id) - tag_id = payload.get("tag_id") - tag = db.get(Tag, tag_id) if tag_id is not None else None - # Only the user's own tags may be attached as personal links. - if tag is None or tag.user_id != user.id: - raise HTTPException(status_code=404, detail="Unknown tag") + # Only the user's own tags may be attached as personal links (owned_or_404 → 404 if the tag + # is missing OR belongs to someone else, without leaking which). + owned_or_404(db, Tag, body.tag_id, user, detail="Unknown tag") exists = db.execute( select(ChannelTag).where( ChannelTag.channel_id == channel_id, - ChannelTag.tag_id == tag_id, + ChannelTag.tag_id == body.tag_id, ChannelTag.user_id == user.id, ) ).scalar_one_or_none() @@ -625,13 +638,13 @@ def attach_tag( db.add( ChannelTag( channel_id=channel_id, - tag_id=tag_id, + tag_id=body.tag_id, user_id=user.id, source="user", ) ) db.commit() - return {"channel_id": channel_id, "tag_id": tag_id} + return {"channel_id": channel_id, "tag_id": body.tag_id} @router.delete("/{channel_id}/tags/{tag_id}") From a622470a28bf35d905dad1f68edbe540bc1f1607 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 26 Jul 2026 04:51:25 +0200 Subject: [PATCH 05/18] refactor(api): Pydantic request models for small route modules (R6 S1b) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit payload:dict → typed request models across the single/double-endpoint modules: - tags: TagCreateIn / TagUpdateIn (color:null clears → reads model_fields_set); _own_tag now delegates to owned_or_404. - config: ConfigSetIn(value:Any) — keeps the "Missing 'value'" 400 via model_fields_set. - me: SwitchAccountIn(user_id:int). - scheduler: JobIntervalIn / MaintenanceSettingsIn — the manual int()+400 becomes a clean 422 on a non-number; the range checks stay 400. - search: ClearSearchIn(video_ids:list[str]). - public: WatchUnlockIn(password:str). Gate: ruff clean, pytest 164 green. --- backend/app/routes/config.py | 15 ++++++++--- backend/app/routes/me.py | 9 +++++-- backend/app/routes/public.py | 9 +++++-- backend/app/routes/scheduler.py | 23 +++++++++-------- backend/app/routes/search.py | 9 +++++-- backend/app/routes/tags.py | 44 ++++++++++++++++++++++----------- 6 files changed, 75 insertions(+), 34 deletions(-) diff --git a/backend/app/routes/config.py b/backend/app/routes/config.py index 9e90a10..9513162 100644 --- a/backend/app/routes/config.py +++ b/backend/app/routes/config.py @@ -1,8 +1,10 @@ """Admin Configuration page API: read/edit the DB-overridable operational settings defined in app.sysconfig. Secret values (e.g. SMTP password) are write-only — never returned.""" import re +from typing import Any from fastapi import APIRouter, Depends, HTTPException +from pydantic import BaseModel from sqlalchemy.orm import Session from app import audit @@ -30,17 +32,24 @@ def get_config(_: User = Depends(admin_user), db: Session = Depends(get_db)) -> return sysconfig.describe(db) +class ConfigSetIn(BaseModel): + # A config value is arbitrarily typed (str/int/bool/…), validated downstream by + # sysconfig.set_value. `value` defaults to a sentinel so a MISSING value stays a 400 + # ("Missing 'value'") while an explicit `null` is a real, settable value. + value: Any = None + + @router.patch("/{key}") def set_config( key: str, - payload: dict, + body: ConfigSetIn, admin: User = Depends(admin_user), db: Session = Depends(get_db), ) -> dict: s = sysconfig.spec(key) if s is None: raise HTTPException(status_code=404, detail="Unknown config key") - if "value" not in payload: + if "value" not in body.model_fields_set: raise HTTPException(status_code=400, detail="Missing 'value'") if s.secret and not sysconfig.secrets_manageable(): raise HTTPException( @@ -49,7 +58,7 @@ def set_config( ) old = None if s.secret else sysconfig.get(db, key) try: - sysconfig.set_value(db, key, payload["value"]) + sysconfig.set_value(db, key, body.value) except (TypeError, ValueError) as exc: raise HTTPException(status_code=400, detail=str(exc) or "Invalid value") # Never log a secret's value — only that it was changed, by whom. Non-secret values are logged diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index d21e9db..5d683d5 100644 --- a/backend/app/routes/me.py +++ b/backend/app/routes/me.py @@ -1,4 +1,5 @@ from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request +from pydantic import BaseModel from sqlalchemy import func, select from sqlalchemy.orm import Session @@ -45,9 +46,13 @@ def my_accounts( return out +class SwitchAccountIn(BaseModel): + user_id: int + + @router.post("/switch") def switch_account( - payload: dict, + body: SwitchAccountIn, request: Request, user: User = Depends(current_user), db: Session = Depends(get_db), @@ -55,7 +60,7 @@ def switch_account( """Switch the active account to another one already authenticated in this browser. No Google round-trip — but only accounts present in the session list (proof they signed in here) are allowed, and access is re-checked in case the invite was revoked since.""" - target = payload.get("user_id") + target = body.user_id if target not in (request.session.get("account_ids") or []): raise HTTPException(status_code=403, detail="Not an account you've signed into here.") u = db.get(User, target) diff --git a/backend/app/routes/public.py b/backend/app/routes/public.py index e7c3d0c..4d39459 100644 --- a/backend/app/routes/public.py +++ b/backend/app/routes/public.py @@ -8,6 +8,7 @@ a short-lived signed grant that the media URL carries (a `