fix(api): mutations answer ok; route helpers flush and leave the commit to the handler
Two rows of the canonical-patterns table had drifted, found while closing the roadmap audit:
- 63 mutation responses carried no `ok` — `{"unsubscribed": true}`, `{"status": "ok"}`,
`{"added": 3, …}` and so on. Each now leads with `"ok": true` (additive: every field a client
reads is still there).
- Twelve route helpers committed on their own. Six were just the end of their handler's work and now
flush: the playlist item add/remove and Watch later get-or-create, the Plex series/season bulk
mark, and the invite decision + account activation — which an approval used to land in separate
transactions. The other six commit on purpose (a token durable before its link is mailed, a
single-use token burned as its own unit, the Google callback's tail, a background registration
with its own session, the staged search ingest around YouTube calls), each listed with its reason.
`tests/test_route_conventions.py` holds both rules in code; CLAUDE.md's two rows point at it.
This commit is contained in:
@@ -447,7 +447,7 @@ def test_register_route_creates_the_account_end_to_end(client, db, monkeypatch):
|
||||
|
||||
r = client.post("/auth/register", json={"email": "newbie@example.com", "password": _PW})
|
||||
|
||||
assert r.status_code == 200 and r.json() == {"status": "ok"}
|
||||
assert r.status_code == 200 and r.json() == {"ok": True, "status": "ok"}
|
||||
db.expire_all()
|
||||
user = db.execute(_select(User).where(User.email == "newbie@example.com")).scalar_one_or_none()
|
||||
assert user is not None, "POST /auth/register answered ok but created nothing"
|
||||
|
||||
@@ -79,7 +79,7 @@ def test_the_demo_entry_does_not_stamp_the_shared_account(client, db):
|
||||
|
||||
r = client.post("/auth/demo", json={"email": "visitor@example.com"})
|
||||
|
||||
assert r.json() == {"authenticated": True}
|
||||
assert r.json() == {"ok": True, "authenticated": True}
|
||||
demo = auth_routes.get_or_create_demo_user(db)
|
||||
db.refresh(demo)
|
||||
assert demo.last_login_at is None
|
||||
|
||||
@@ -76,5 +76,5 @@ def test_the_finish_supersedes_a_resume_push_still_in_flight(db, as_user, make_u
|
||||
r = as_user(user).post(
|
||||
"/api/plex/item/m1/progress", json={"position_seconds": 2995, "duration_seconds": 3000}
|
||||
)
|
||||
assert r.json() == {"status": "watched", "position_seconds": 0}
|
||||
assert r.json() == {"ok": True, "status": "watched", "position_seconds": 0}
|
||||
assert playback._resume_push_superseded(user.id, item.id, earlier) is True
|
||||
|
||||
@@ -436,7 +436,7 @@ class TestPlaylistAndCollectionEndpoints:
|
||||
{"item_rating_keys": ["m1", "m2", "m3"]},
|
||||
)
|
||||
assert added.status_code == 200, added.text
|
||||
assert added.json() == {"added": 3, "item_count": 3}
|
||||
assert added.json() == {"ok": True, "added": 3, "item_count": 3}
|
||||
|
||||
removed = _request(
|
||||
db,
|
||||
@@ -448,7 +448,7 @@ class TestPlaylistAndCollectionEndpoints:
|
||||
{"item_rating_keys": ["m2"]},
|
||||
)
|
||||
assert removed.status_code == 200, removed.text
|
||||
assert removed.json() == {"removed": 1, "item_count": 2}
|
||||
assert removed.json() == {"ok": True, "removed": 1, "item_count": 2}
|
||||
|
||||
detail = _get(db, user, monkeypatch, override_dep, f"/api/plex/playlists/{pid}").json()
|
||||
assert [it["id"] for it in detail["items"]] == [
|
||||
|
||||
@@ -0,0 +1,68 @@
|
||||
"""Two rows of CLAUDE.md's canonical-patterns table, held in code rather than prose.
|
||||
|
||||
A rule stated only in a comment drifts: this one had — 63 mutation responses without `ok`, and
|
||||
twelve route helpers that committed on their own, among them an approval whose decision, activation
|
||||
and token landed in three separate transactions. The debt burn of 2026-09 fixed the drift; these keep
|
||||
it fixed.
|
||||
"""
|
||||
|
||||
import ast
|
||||
from pathlib import Path
|
||||
|
||||
ROUTES = Path(__file__).parent.parent / "app" / "routes"
|
||||
MUTATING = {"post", "put", "patch", "delete"}
|
||||
|
||||
# A route helper that commits where it stands, and why the commit has to be there rather than in
|
||||
# the handler. Anything else flushes and leaves the commit to its handler.
|
||||
COMMITTING_HELPERS = {
|
||||
"auth.py:_issue_token": "the token must be durable before the link that carries it is mailed",
|
||||
"auth.py:_consume_token": "burning a single-use token is its own unit: a later failure must "
|
||||
"not hand the token back",
|
||||
"auth.py:_complete_link": "the tail of the Google callback route, which answers with its result",
|
||||
"auth.py:_register_account": "a background task with its own session, i.e. a job wrapper",
|
||||
"search.py:_ingest_candidates": "stages around YouTube calls: the stubs must survive an enrich "
|
||||
"that fails",
|
||||
"search.py:_classify_shorts": "the last stage of that ingest, after its probes",
|
||||
}
|
||||
|
||||
|
||||
def _modules():
|
||||
for path in sorted(ROUTES.rglob("*.py")):
|
||||
yield path.relative_to(ROUTES).as_posix(), ast.parse(path.read_text(encoding="utf-8"))
|
||||
|
||||
|
||||
def _is_mutation_route(fn: ast.FunctionDef) -> bool:
|
||||
return any(
|
||||
isinstance(d, ast.Call) and getattr(d.func, "attr", "") in MUTATING
|
||||
for d in fn.decorator_list
|
||||
)
|
||||
|
||||
|
||||
def test_every_mutation_answers_ok():
|
||||
"""`{"ok": true, …}` (or `{"deleted": id}`) for every mutation that answers with a literal —
|
||||
one field a client can read the same way everywhere. A serializer's return is exempt: it
|
||||
describes a resource, not an acknowledgement."""
|
||||
missing = []
|
||||
for name, tree in _modules():
|
||||
for fn in ast.walk(tree):
|
||||
if not (isinstance(fn, ast.FunctionDef) and _is_mutation_route(fn)):
|
||||
continue
|
||||
for node in ast.walk(fn):
|
||||
if isinstance(node, ast.Return) and isinstance(node.value, ast.Dict):
|
||||
keys = {k.value for k in node.value.keys if isinstance(k, ast.Constant)}
|
||||
if not keys & {"ok", "deleted"}:
|
||||
missing.append(f"{name}:{node.lineno} {fn.name}")
|
||||
assert missing == []
|
||||
|
||||
|
||||
def test_route_helpers_flush_and_leave_the_commit_to_the_handler():
|
||||
committing = set()
|
||||
for name, tree in _modules():
|
||||
for fn in ast.walk(tree):
|
||||
if isinstance(fn, ast.FunctionDef) and fn.name.startswith("_"):
|
||||
if any(
|
||||
isinstance(n, ast.Call) and getattr(n.func, "attr", "") == "commit"
|
||||
for n in ast.walk(fn)
|
||||
):
|
||||
committing.add(f"{name}:{fn.name}")
|
||||
assert committing == set(COMMITTING_HELPERS)
|
||||
@@ -220,7 +220,7 @@ def test_registering_again_re_sends_the_link_instead_of_sending_nothing(db, clie
|
||||
# The old behaviour was to return in silence: same cheerful "check your inbox", no mail. That is
|
||||
# the most likely explanation for the 2026-09-03 confirmation mail that "never arrived" — none
|
||||
# was ever sent. The response stays uniform; only the dead end is gone.
|
||||
assert r.status_code == 200 and r.json() == {"status": "ok"}
|
||||
assert r.status_code == 200 and r.json() == {"ok": True, "status": "ok"}
|
||||
assert len(sent) == 2
|
||||
assert sent[1][0][0] == "twice@example.com"
|
||||
|
||||
|
||||
Reference in New Issue
Block a user