From ceadf69e438cc173ed4bc9dcf9610dfce3a4e5fa Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 31 Oct 2026 01:00:26 +0100 Subject: [PATCH] fix(i18n): hold every refusal reason by type, and refuse what the scan cannot read Review round 1 of the refusal contract: it claimed more than it held. - The other reason carriers get their Literal too (BlockedUrl, ClipError, FolderError, the playlist and sign-in refusal helpers), so the type checker refuses a reason passed by variable or an unknown literal, and the contract reads one shape. - A literal reason in a detail dict built anywhere counts as a reason of its code; a code the scan cannot read, a refused() whose reason it cannot read, and a new reason passed by variable each fail the contract until listed with what holds them. - A sign-in refusal key the backend no longer sends is caught too. - localizeDetail's numbers (download cap in GB, upload cap in bytes) and the shared-sentence fallbacks are tested in both languages. - One Literal parser in the contract; a stale comment fixed. --- backend/app/downloads/clip.py | 18 +- backend/app/downloads/folders.py | 13 +- backend/app/downloads/urlguard.py | 23 ++- backend/app/refusal.py | 3 +- backend/app/routes/auth.py | 11 +- backend/app/routes/playlists.py | 19 +- frontend/src/i18n/backendIds.contract.test.ts | 189 +++++++++++------- frontend/src/lib/api/refusalText.dom.test.ts | 19 ++ 8 files changed, 213 insertions(+), 82 deletions(-) diff --git a/backend/app/downloads/clip.py b/backend/app/downloads/clip.py index 9d3f57d4..af2cb46b 100644 --- a/backend/app/downloads/clip.py +++ b/backend/app/downloads/clip.py @@ -22,6 +22,7 @@ import json import math import re from pathlib import Path +from typing import Literal from app.downloads.edit import AAC_ARGS, EVEN_FRAME, INPUT_GUARD, X264_ARGS from app.plex.sync import HDR_TRANSFERS @@ -57,11 +58,26 @@ _CROP_ASPECTS = (0.25, 4.0) _EDGE_SLACK = 1e-3 +# Every reason a clip is refused; the UI names each one (`clip.errors.`, held by the frontend's +# backendIds contract, which reads this Literal). +ClipReason = Literal[ + "bad_source", + "bad_range", + "too_short", + "too_long", + "bad_audio", + "bad_crop", + "not_found", + "media_missing", + "past_end", +] + + class ClipError(Exception): """A clip request that cannot be cut; `reason` is an i18n-mappable key, `params` the numbers its message names (the cap itself, so the client never says a stale one).""" - def __init__(self, reason: str, **params: float): + def __init__(self, reason: ClipReason, **params: float): self.reason = reason self.params = params super().__init__(reason) diff --git a/backend/app/downloads/folders.py b/backend/app/downloads/folders.py index f38eb8ce..fffb75c0 100644 --- a/backend/app/downloads/folders.py +++ b/backend/app/downloads/folders.py @@ -24,6 +24,7 @@ kept it — and its `ready_at`, the moment it finished (`stamp_done`). from datetime import datetime, timedelta, timezone from enum import StrEnum +from typing import Literal from urllib.parse import urlsplit from sqlalchemy import select @@ -181,10 +182,20 @@ def stamp_done(db: Session, job: DownloadJob) -> None: job.expiry_warned = False +# Every folder change the rules refuse, answered as a generic refusal; the UI names each one +# (`errors.refused.`, held by the frontend's backendIds contract, which reads this Literal). +FolderReason = Literal[ + "file_folder_gone", + "file_folder_name_taken", + "file_folder_not_its_source", + "file_folder_not_own", +] + + class FolderError(Exception): """A folder change the rules refuse; `reason` is the refusal the route answers with.""" - def __init__(self, reason: str) -> None: + def __init__(self, reason: FolderReason) -> None: super().__init__(reason) self.reason = reason diff --git a/backend/app/downloads/urlguard.py b/backend/app/downloads/urlguard.py index 3ca5a946..575c7836 100644 --- a/backend/app/downloads/urlguard.py +++ b/backend/app/downloads/urlguard.py @@ -33,22 +33,31 @@ user-controlled destination to guard. import ipaddress import socket +from typing import Literal from urllib.parse import urlparse # Reason codes travel to the client inside the structured 422 detail, so they are part of the API -# vocabulary (frontend `localizeDetail`) — keep them stable. -REASON_SCHEME = "scheme" # not http(s) -REASON_NO_HOST = "no_host" # URL carries no hostname -REASON_MALFORMED = "malformed" # unparseable authority or port; urlparse raises late, so guard it -REASON_UNRESOLVABLE = "unresolvable" # DNS said nothing (honest: not the same as "blocked") -REASON_PRIVATE = "private" # resolves into a network the server must not fetch from +# vocabulary — keep them stable. The UI names each one (`downloads.errors.urlBlocked.`, held by +# the frontend's backendIds contract, which reads this Literal). +UrlBlockedReason = Literal["scheme", "no_host", "malformed", "unresolvable", "private"] +REASON_SCHEME: UrlBlockedReason = "scheme" # not http(s) +REASON_NO_HOST: UrlBlockedReason = "no_host" # URL carries no hostname +REASON_MALFORMED: UrlBlockedReason = ( + "malformed" # unparseable authority or port; urlparse raises late +) +REASON_UNRESOLVABLE: UrlBlockedReason = ( + "unresolvable" # DNS said nothing (not the same as "blocked") +) +REASON_PRIVATE: UrlBlockedReason = ( + "private" # resolves into a network the server must not fetch from +) class BlockedUrl(Exception): """A user-supplied URL that must not be fetched server-side. ``reason`` is one of the REASON_* codes above.""" - def __init__(self, reason: str): + def __init__(self, reason: UrlBlockedReason): super().__init__(reason) self.reason = reason diff --git a/backend/app/refusal.py b/backend/app/refusal.py index 8e6b9ffd..c092bb55 100644 --- a/backend/app/refusal.py +++ b/backend/app/refusal.py @@ -11,7 +11,8 @@ A test (`test_http_details.py`) reads every `HTTPException` in the app and fails and the frontend's contract test (`i18n/backendIds.contract.test.ts`) requires every `code` a detail carries to be in one record of `lib/api/refusalCodes.ts`, and every reason it can be sent with — a literal, or a `Literal` type when it travels in a variable — to have its sentence in both languages. -So the rule is enforced, not described. +A code or reason that scan cannot read fails it until it is listed with what holds it. So the rule is +enforced, not described. """ from fastapi import HTTPException diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index a36f4584..92c6a4ef 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -976,7 +976,16 @@ def _resend_verify_if_eligible(email: str) -> None: send_verification_link(db, user) -def _login_refused(status: int, reason: str, user: User | None = None, **extra) -> HTTPException: +# Every refusal a password sign-in answers with; the UI names each one (`welcome.auth.blocked.`, +# held by the frontend's backendIds contract, which reads this Literal). +LoginRefusal = Literal[ + "rate_limited", "invalid", "suspended", "email_unverified", "pending_approval" +] + + +def _login_refused( + status: int, reason: LoginRefusal, user: User | None = None, **extra +) -> HTTPException: """Every refusal password_login can answer with, in the codebase's structured `{code, reason}` shape so the SPA can say it in the user's own language (the old bare English strings were unreadable under a bilingual UI, and unusable as anything but a one-line error). diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index 076409a5..f27ea526 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -3,7 +3,7 @@ CRUD (create / rename / delete, add / remove / reorder items) plus YouTube two-w mirror the user's own YouTube playlists in (`/sync-youtube`) and push/revert local edits back out (`/{id}/push-plan`, `/{id}/revert-youtube`), behind the optional write scope.""" -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Literal from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel, Field @@ -53,7 +53,22 @@ def _check_pull_cooldown(user: User) -> None: raise HTTPException(status_code=429, detail={"code": "sync_cooldown"}) -def _refused(status: int, reason: str) -> HTTPException: +# Every playlist action the server refuses; the UI names each one (`playlists.errors.`, held by +# the frontend's backendIds contract, which reads this Literal). +PlaylistRefusal = Literal[ + "name_required", + "not_on_youtube", + "quota_exceeded", + "sync_all_busy", + "sync_busy", + "watch_later_not_pushable", + "watch_later_undeletable", + "youtube_editing_off", + "youtube_read_scope", +] + + +def _refused(status: int, reason: PlaylistRefusal) -> HTTPException: """A playlist action the server refuses, as a reason the UI translates (`playlists.errors.`) — a bare English detail reached the Hungarian UI verbatim.""" return HTTPException(status_code=status, detail={"code": "playlist", "reason": reason}) diff --git a/frontend/src/i18n/backendIds.contract.test.ts b/frontend/src/i18n/backendIds.contract.test.ts index 29d7b082..524ed7db 100644 --- a/frontend/src/i18n/backendIds.contract.test.ts +++ b/frontend/src/i18n/backendIds.contract.test.ts @@ -90,9 +90,9 @@ const runEnum = (name: string) => const runStatuses = runEnum("RunStatus"); const runTriggers = runEnum("RunTrigger"); const quotaActions = enumValues("quota.py"); -// The admin actions' refusal reasons → `errors.admin.`. The codes are read from -// `localizeDetail`'s own ADMIN_REFUSALS, so a code added there is scanned here too; a detail dict -// ruff wraps over several lines still matches. +// The admin actions' refusal reasons → `errors.admin.`. The codes are the refusal records +// under that prefix (lib/api/refusalCodes), so a code recorded there is scanned here too; a detail +// dict ruff wraps over several lines still matches. const adminCodes = Object.entries(REFUSAL_FAMILIES) .filter(([, f]) => f.prefix === "errors.admin") .map(([code]) => code); @@ -172,29 +172,38 @@ const goneWhats = [ ), ), ]; -// Every generic refusal → `errors.refused.`: each `refused(, "")` anywhere in -// the backend (not `_refused`, a router's own family), plus the values of the settings map, which -// `setting_refused` picks from at runtime, and every `FolderError` reason. -const appSources = (dir = ""): string[] => +// A backend `Name = Literal["a", "b"]` alias, by name: the reasons a typed parameter accepts, which +// basedpyright then holds at every call site — a reason passed by variable included. +const literalOf = (file: string, name: string) => + [ + ...( + backend(file).match(new RegExp(`^${name} = Literal\\[([^\\]]*)\\]`, "m"))?.[1] ?? "" + ).matchAll(/"(\w+)"/g), + ].map((m) => m[1]!); +// Every backend source, by its path under app/. +const appPaths = (dir = ""): string[] => readdirSync(new URL(`../../../backend/app/${dir}`, import.meta.url), { withFileTypes: true, }).flatMap((e) => e.isDirectory() - ? appSources(`${dir}${e.name}/`) + ? e.name === "__pycache__" + ? [] + : appPaths(`${dir}${e.name}/`) : e.name.endsWith(".py") - ? [backend(`${dir}${e.name}`)] + ? [`${dir}${e.name}`] : [], ); +const appSources = (): string[] => appPaths().map(backend); +// Every generic refusal → `errors.refused.`: each `refused(, "")` anywhere in +// the backend (not `_refused`, a router's own family), the values of the settings map, which +// `setting_refused` picks from at runtime, and the folder rules' `FolderReason`, answered with +// `refused(409, e.reason)`. const refusedReasons = [ ...new Set([ ...appSources().flatMap((src) => [...src.matchAll(/(? m[1]!), ), - // A folder change the rules refuse travels as `FolderError("")` and is answered with - // `refused(409, e.reason)`, which the pattern above cannot read. - ...appSources().flatMap((src) => - [...src.matchAll(/FolderError\("(\w+)"\)/g)].map((m) => m[1]!), - ), + ...literalOf("downloads/folders.py", "FolderReason"), ...[ ...(backend("refusal.py").split("_SETTING_REASONS = {")[1]?.split("}")[0] ?? "").matchAll( /:\s*"(\w+)"/g, @@ -202,61 +211,90 @@ const refusedReasons = [ ].map((m) => m[1]!), ]), ]; -// A refused YouTube action → `errors.youtube.`, the reasons read from their Literal. -const youtubeReasons = [ - ...( - backend("youtube/client.py").match(/^YouTubeRefusal = Literal\[([^\]]*)\]/m)?.[1] ?? "" - ).matchAll(/"(\w+)"/g), -].map((m) => m[1]!); -// A clip the server will not cut → `clip.errors.`: every `ClipError(""…)` in the backend -// (a reason may carry its numbers after it: `ClipError("too_long", limit_s=…)`). -const clipReasons = [ - ...new Set( - appSources().flatMap((src) => [...src.matchAll(/ClipError\("(\w+)"[,)]/g)].map((m) => m[1]!)), - ), -]; +// A `refused(…)` whose reason the scan above cannot read. Only two may: a folder rule's `e.reason` +// (the nearest `except` above it catches `FolderError`, typed `FolderReason`) and the settings map. +const opaqueRefused = appPaths().flatMap((f) => { + const src = backend(f); + return [...src.matchAll(/(? { + const arg = m[1]!.trim(); + if (f === "refusal.py" && arg.startsWith("_SETTING_REASONS[")) return false; + const caught = [...src.slice(0, m.index).matchAll(/except ([^\n:]*):/g)].pop()?.[1] ?? ""; + return !(arg === "e.reason" && /FolderError as e$/.test(caught)); + }) + .map((m) => `${f}: ${m[0]}`); +}); +const youtubeReasons = literalOf("youtube/client.py", "YouTubeRefusal"); +const clipReasons = literalOf("downloads/clip.py", "ClipReason"); -// Every refusal code the backend answers: each `"code": ""` of a detail anywhere in the app. A -// subtitle's status payload (`{"state": …, "code": …}`) is the player's own vocabulary, not a refusal. -const backendCodes = [ +// Every `"code": …` a backend dict carries, with the `"reason": …` that follows it. Two modules use a +// `code` key for something that is not a refusal; everything else is one. +const NOT_REFUSALS: Record = { + "plex/subtitles.py": + "the player's subtitle status vocabulary (`/status`), read by the player itself", + "plex/tmdb.py": "a production country's ISO code", +}; +const detailSites = appPaths() + .filter((f) => !(f in NOT_REFUSALS)) + .flatMap((f) => + [ + ...backend(f).matchAll( + /"code":\s*(?:"(\w+)"|([^\s,}]+))(?:,\s*"reason":\s*(?:"(\w+)"|([^\s,}]+)))?/g, + ), + ].map((m) => ({ f, code: m[1], codeExpr: m[2], reason: m[3], reasonExpr: m[4] })), + ); +const backendCodes = [...new Set(detailSites.flatMap((s) => (s.code ? [s.code] : [])))]; +// A code the scan cannot read (`"code": SOME_CONSTANT`) is a refusal no record is checked for. +const opaqueCodes = detailSites.filter((s) => !s.code).map((s) => `${s.f}: "code": ${s.codeExpr}`); +// A reason passed by variable, each pinned to what holds it, so a new one has to say how it is held. +const OPAQUE_REASONS: Record = { + "refusal.py: refused ← reason": "a literal at each refused() call, else opaqueRefused", + "routes/auth.py: login ← reason": "_login_refused's LoginRefusal", + "routes/downloads.py: url_blocked ← urlguard.REASON_MALFORMED": "UrlBlockedReason", + "routes/downloads.py: url_blocked ← e.reason": "BlockedUrl's UrlBlockedReason", + "routes/downloads.py: cookies ← e.reason": "InvalidCookies' CookieReason", + "routes/downloads.py: edit ← e.reason": "EditError's EditReason", + "routes/downloads.py: clip ← e.reason": "ClipError's ClipReason", + "routes/files.py: quota ← e.reason": "QuotaExceeded's QuotaReason", + "routes/playlists.py: playlist ← reason": "_refused's PlaylistRefusal", + "routes/playlists.py: playlist_move ← e.reason": "MoveRefused's MoveReason", + "routes/plex/playback.py: subtitle ← code": "REFUSAL_UNSHOWN: no screen reads it", + "routes/_common.py: not_found ← what": "opaqueWhats", + "routes/_common.py: youtube ← reason": "YouTubeRefusal", +}; +const opaqueReasons = [ ...new Set( - appSources().flatMap((src) => - src - .split("\n") - .filter((line) => !line.includes('"state":')) - .flatMap((line) => [...line.matchAll(/"code":\s*"(\w+)"/g)].map((m) => m[1]!)), - ), + detailSites + .filter((s) => s.code && s.reasonExpr) + .map((s) => `${s.f}: ${s.code} ← ${s.reasonExpr}`), ), ]; -const literalOf = (file: string, name: string) => - [ - ...( - backend(file).match(new RegExp(`^${name} = Literal\\[([^\\]]*)\\]`, "m"))?.[1] ?? "" - ).matchAll(/"(\w+)"/g), - ].map((m) => m[1]!); -const literalCalls = (file: string, re: RegExp) => [ - ...new Set([...backend(file).matchAll(re)].map((m) => m[1]!)), -]; -const detailReasons = (file: string, code: string) => - literalCalls(file, new RegExp(`"code":\\s*"${code}",\\s*"reason":\\s*"(\\w+)"`, "g")); -// What each refusal code can carry as its reason, read where it is decided: a Literal when the reason -// travels in a variable (`e.reason`), the call's own literal otherwise. -const reasonsOf: Record = { - refused: refusedReasons, - youtube: youtubeReasons, - not_found: goneWhats, - clip: clipReasons, - ...Object.fromEntries(adminCodes.map((c) => [c, detailReasons("routes/admin.py", c)])), - playlist: literalCalls("routes/playlists.py", /_refused\(\s*\d+,\s*"(\w+)"/g), - playlist_move: literalOf("reader/service.py", "MoveReason"), - edit: literalOf("downloads/service.py", "EditReason"), - cookies: literalOf("downloads/credentials.py", "CookieReason"), - url_blocked: literalCalls("downloads/urlguard.py", /^REASON_\w+ = "(\w+)"/gm), - login: literalCalls("routes/auth.py", /_login_refused\(\s*\d+,\s*"(\w+)"/g), - quota: literalOf("downloads/quota.py", "QuotaReason"), - reset: detailReasons("routes/auth.py", "reset"), - collection: detailReasons("routes/plex/collections.py", "collection"), -}; +// A reason written as a literal in a detail dict, wherever that dict is built. +const literalReasons = (code: string) => + detailSites.filter((s) => s.code === code && s.reason).map((s) => s.reason!); +// What each refusal code can carry as its reason: the Literal its typed parameter accepts, or the +// call's own literal — and every literal any detail dict anywhere gives it. +const reasonsOf: Record = Object.fromEntries( + Object.entries({ + refused: refusedReasons, + youtube: youtubeReasons, + not_found: goneWhats, + clip: clipReasons, + ...Object.fromEntries(adminCodes.map((c) => [c, []])), + playlist: literalOf("routes/playlists.py", "PlaylistRefusal"), + playlist_move: literalOf("reader/service.py", "MoveReason"), + edit: literalOf("downloads/service.py", "EditReason"), + cookies: literalOf("downloads/credentials.py", "CookieReason"), + url_blocked: literalOf("downloads/urlguard.py", "UrlBlockedReason"), + login: literalOf("routes/auth.py", "LoginRefusal"), + quota: literalOf("downloads/quota.py", "QuotaReason"), + reset: [], + collection: [], + } as Record).map(([code, rs]) => [ + code, + [...new Set([...rs, ...literalReasons(code)])], + ]), +); const recordedCodes = [ ...Object.keys(REFUSAL_FAMILIES), ...Object.keys(REFUSAL_BY_REASON), @@ -359,6 +397,14 @@ describe("backend ids have their names in both languages", () => { expect(recordedCodes.filter((c) => !backendCodes.includes(c))).toEqual([]); }); + it("every refusal code and reason is one the scans can read, or says what holds it", () => { + // The modules set aside as not refusals still exist and still use the key. + for (const f of Object.keys(NOT_REFUSALS)) expect(backend(f), f).toContain('"code":'); + expect(opaqueCodes).toEqual([]); + expect(opaqueRefused).toEqual([]); + expect(opaqueReasons.sort()).toEqual(Object.keys(OPAQUE_REASONS).sort()); + }); + it("read every refusal's reasons (a scan that matches nothing is green over nothing)", () => { const coded = [...Object.keys(REFUSAL_FAMILIES), ...Object.keys(REFUSAL_BY_REASON)]; expect(Object.keys(reasonsOf).sort()).toEqual(coded.sort()); @@ -542,16 +588,21 @@ describe("backend ids have their names in both languages", () => { }); it(`${lang}: no sentence for a refusal reason the backend cannot send`, () => { - // A family whose prefix is its own (the shared `errors.admin`, and the sign-in screens, which - // name more than the one-line refusal, are checked elsewhere or not at all). + // A family whose prefix is its own (the shared `errors.admin` is checked below). A suffixed + // family shares its prefix with more than its refusals — the sign-in screens' `gate`, + // `addressLabel` — so only an id with the refusal's own key counts as a reason there. for (const [code, f] of Object.entries(REFUSAL_FAMILIES)) { - if (f.prefix === "errors.admin" || "suffix" in f) continue; + if (f.prefix === "errors.admin") continue; + const suffix = "suffix" in f ? f.suffix : ""; const fallbackId = f.fallback.startsWith(`${f.prefix}.`) ? f.fallback.slice(f.prefix.length + 1) : null; expect( [...idsUnder(keys, f.prefix)].filter( - (r) => r !== fallbackId && !reasonsOf[code]!.includes(r), + (r) => + keys.has(`${f.prefix}.${r}${suffix}`) && + r !== fallbackId && + !reasonsOf[code]!.includes(r), ), code, ).toEqual([]); diff --git a/frontend/src/lib/api/refusalText.dom.test.ts b/frontend/src/lib/api/refusalText.dom.test.ts index 138228ac..0a22de0e 100644 --- a/frontend/src/lib/api/refusalText.dom.test.ts +++ b/frontend/src/lib/api/refusalText.dom.test.ts @@ -4,6 +4,7 @@ // one case per record shape, so moving the dispatch into data changed what no reader sees. import { afterEach, describe, expect, it } from "vitest"; import i18n from "../../i18n"; +import { formatBytes } from "../format"; import { localizeDetail } from "./refusalText"; afterEach(async () => { @@ -45,6 +46,24 @@ describe.each(["en", "hu"])("localizeDetail (%s)", (lng) => { ); }); + it("puts a download cap in GB and an upload cap in bytes, in the reader's number format", async () => { + expect(await say({ code: "quota", reason: "max_bytes", limit: 1.5 * 2 ** 30 })).toContain( + lng === "hu" ? "1,5" : "1.5", + ); + const upload = await say({ code: "upload_too_large", limit: 5_242_880 }); + expect(upload).toContain(formatBytes(5_242_880, lng)); + expect(upload).not.toBe(i18n.t("downloads.errors.upload.tooLarge", { size: "" })); + }); + + it("says a shared-sentence code's own fallback for a reason it does not know", async () => { + expect(await say({ code: "quota", reason: "nope" })).toBe( + i18n.t("downloads.errors.quota.generic"), + ); + expect(await say({ code: "collection", reason: "nope" })).toBe( + i18n.t("plex.collection.editFailed"), + ); + }); + it("says a shared-sentence code's fallback, and a fixed code's one sentence", async () => { expect(await say({ code: "reset", reason: "invalid" })).toBe( i18n.t("welcome.auth.deadLink.short"),