fix(e5): address code-review findings (TMDB guid media_type, search ingest guard, discover keying)

Follow-ups from the epic-end /code-review of the E5 branch:

- tmdb.resolve_recs + people._resolve_known_for: match recommendations/
  credits to the library bucketed by media_type. TMDB movie ids and tv ids
  are independent sequences, so a bare `tmdb:<id>` match could deep-link a
  movie rec to an unrelated show (or vice-versa) on a numeric collision.
  Regression test added (test_resolve_recs_matches_by_media_type_not_bare_id).
- routes/search: wrap the _ingest_candidates call so a DB/enrich failure
  degrades to a 422 with a message instead of a bare 500 hidden behind the
  FE's generic "search failed" (extends the B-core-1 rationale); rollback on
  failure. Stamp enriched_at with its own timestamp taken after the enrich,
  not the stub-insert placeholder.
- routes/plex/discover: key serialized cards back by rating_key (globally
  unique in Plex) instead of id(obj); fix the stale rows 3-tuple annotation.
This commit is contained in:
2026-08-21 03:16:24 +02:00
parent ff45db70ae
commit 2b5de89eda
5 changed files with 118 additions and 42 deletions
+21 -9
View File
@@ -340,25 +340,37 @@ def run_enrich(db: Session) -> dict:
def resolve_recs(db: Session, recs: list[dict] | None) -> list[dict]: def resolve_recs(db: Session, recs: list[dict] | None) -> list[dict]:
"""Tag each recommendation with whether it's in OUR library (matched by guid `tmdb:<id>`) and, if so, """Tag each recommendation with whether it's in OUR library (matched by guid `tmdb:<id>`) and, if so,
its rating_key — so the client can deep-link the ones we have and merely display the rest.""" its rating_key — so the client can deep-link the ones we have and merely display the rest.
TMDB movie ids and tv ids are independent sequences (id 603 can be BOTH a movie and a show), so the
match is bucketed by each rec's own media_type: a movie rec only matches a library movie, a tv rec
only a library show — otherwise a numeric-id collision would deep-link a rec to an unrelated title."""
if not recs: if not recs:
return [] return []
guids = [f"tmdb:{r['tmdb_id']}" for r in recs if r.get("tmdb_id")] movie_guids = [
in_lib: dict[str, str] = {} f"tmdb:{r['tmdb_id']}" for r in recs if r.get("tmdb_id") and r.get("media_type") != "tv"
if guids: ]
tv_guids = [
f"tmdb:{r['tmdb_id']}" for r in recs if r.get("tmdb_id") and r.get("media_type") == "tv"
]
movie_lib: dict[str, str] = {}
show_lib: dict[str, str] = {}
if movie_guids:
for guid, rk in db.execute( for guid, rk in db.execute(
select(PlexItem.guid, PlexItem.rating_key).where( select(PlexItem.guid, PlexItem.rating_key).where(
PlexItem.kind == "movie", PlexItem.guid.in_(guids) PlexItem.kind == "movie", PlexItem.guid.in_(movie_guids)
) )
): ):
in_lib[guid] = rk movie_lib.setdefault(guid, rk)
if tv_guids:
for guid, rk in db.execute( for guid, rk in db.execute(
select(PlexShow.guid, PlexShow.rating_key).where(PlexShow.guid.in_(guids)) select(PlexShow.guid, PlexShow.rating_key).where(PlexShow.guid.in_(tv_guids))
): ):
in_lib.setdefault(guid, rk) show_lib.setdefault(guid, rk)
out: list[dict] = [] out: list[dict] = []
for r in recs: for r in recs:
rk = in_lib.get(f"tmdb:{r.get('tmdb_id')}") g = f"tmdb:{r.get('tmdb_id')}"
rk = show_lib.get(g) if r.get("media_type") == "tv" else movie_lib.get(g)
out.append({**r, "in_library": rk is not None, "rating_key": rk}) out.append({**r, "in_library": rk is not None, "rating_key": rk})
return out return out
+18 -22
View File
@@ -90,21 +90,16 @@ def _recently_added(
] ]
tagged.sort(key=lambda t: (t[0] is not None, t[0]), reverse=True) tagged.sort(key=lambda t: (t[0] is not None, t[0]), reverse=True)
top = tagged[:RAIL_LIMIT] top = tagged[:RAIL_LIMIT]
mcards = { # Key the serialized cards back by rating_key (globally unique in Plex — a movie and a show never
id(m): c # share one), so the merge doesn't depend on object identity staying alive.
for m, c in zip( top_movies = [o for _, kind, o in top if kind == "movie"]
[t[2] for t in top if t[1] == "movie"], top_shows = [o for _, kind, o in top if kind == "show"]
_movie_cards(db, uid, [t[2] for t in top if t[1] == "movie"]), mcards = {m.rating_key: c for m, c in zip(top_movies, _movie_cards(db, uid, top_movies))}
) scards = {s.rating_key: c for s, c in zip(top_shows, _show_cards(db, uid, top_shows))}
} return [
scards = { mcards[o.rating_key] if kind == "movie" else scards[o.rating_key]
id(s): c for _, kind, o in top
for s, c in zip( ]
[t[2] for t in top if t[1] == "show"],
_show_cards(db, uid, [t[2] for t in top if t[1] == "show"]),
)
}
return [mcards[id(o)] if kind == "movie" else scards[id(o)] for _, kind, o in top]
def _continue_watching( def _continue_watching(
@@ -112,7 +107,7 @@ def _continue_watching(
) -> list[dict]: ) -> list[dict]:
"""Resume rail: in-progress movies + partially-watched shows, most-recently-progressed first. """Resume rail: in-progress movies + partially-watched shows, most-recently-progressed first.
A movie's resume time is its own `progress_updated_at`; a show's is the max across its episodes.""" A movie's resume time is its own `progress_updated_at`; a show's is the max across its episodes."""
rows: list[tuple[object, object, dict]] = [] # (resumed_at, sentinel-ordinal, card) rows: list[tuple[object, dict]] = [] # (resumed_at, card)
if movie_ids: if movie_ids:
mrows = db.execute( mrows = db.execute(
@@ -256,14 +251,15 @@ def _because_you_watched(
} }
ordered_movies = [movies[rk] for rk in rks if rk in movies] ordered_movies = [movies[rk] for rk in rks if rk in movies]
ordered_shows = [shows[rk] for rk in rks if rk in shows] ordered_shows = [shows[rk] for rk in rks if rk in shows]
mcards = {id(m): c for m, c in zip(ordered_movies, _movie_cards(db, uid, ordered_movies))} # rating_key is globally unique in Plex, so it keys the serialized cards back safely (no id()).
scards = {id(s): c for s, c in zip(ordered_shows, _show_cards(db, uid, ordered_shows))} mcards = {m.rating_key: c for m, c in zip(ordered_movies, _movie_cards(db, uid, ordered_movies))}
scards = {s.rating_key: c for s, c in zip(ordered_shows, _show_cards(db, uid, ordered_shows))}
items: list[dict] = [] items: list[dict] = []
for rk in rks: for rk in rks:
if rk in movies: if rk in mcards:
items.append(mcards[id(movies[rk])]) items.append(mcards[rk])
elif rk in shows: elif rk in scards:
items.append(scards[id(shows[rk])]) items.append(scards[rk])
if not items: if not items:
return None return None
return {"anchor": {"id": anchor.rating_key, "title": anchor.title}, "items": items} return {"anchor": {"id": anchor.rating_key, "title": anchor.title}, "items": items}
+19 -9
View File
@@ -36,24 +36,32 @@ def _resolve_known_for(
popularity order. Matched by the canonical guid `tmdb:<id>`; out-of-library credits are dropped.""" popularity order. Matched by the canonical guid `tmdb:<id>`; out-of-library credits are dropped."""
if not known_for: if not known_for:
return [] return []
guids = [f"tmdb:{k['tmdb_id']}" for k in known_for if k.get("tmdb_id")] # Bucket by media_type: TMDB movie ids and tv ids collide numerically, so a movie credit must only
if not guids: # match a library movie and a tv credit only a library show (mirrors tmdb.resolve_recs). Matching
# blind to media_type would render a tv credit as an unrelated movie whose tmdb id happens to match.
movie_guids = [
f"tmdb:{k['tmdb_id']}" for k in known_for if k.get("tmdb_id") and k.get("media_type") != "tv"
]
tv_guids = [
f"tmdb:{k['tmdb_id']}" for k in known_for if k.get("tmdb_id") and k.get("media_type") == "tv"
]
if not movie_guids and not tv_guids:
return [] return []
movies: dict[str, PlexItem] = {} movies: dict[str, PlexItem] = {}
if movie_lib_ids: if movie_lib_ids and movie_guids:
for m in db.execute( for m in db.execute(
select(PlexItem).where( select(PlexItem).where(
PlexItem.kind == "movie", PlexItem.kind == "movie",
PlexItem.library_id.in_(movie_lib_ids), PlexItem.library_id.in_(movie_lib_ids),
PlexItem.guid.in_(guids), PlexItem.guid.in_(movie_guids),
) )
).scalars(): ).scalars():
movies.setdefault(m.guid, m) movies.setdefault(m.guid, m)
shows: dict[str, PlexShow] = {} shows: dict[str, PlexShow] = {}
if show_lib_ids: if show_lib_ids and tv_guids:
for s in db.execute( for s in db.execute(
select(PlexShow).where( select(PlexShow).where(
PlexShow.library_id.in_(show_lib_ids), PlexShow.guid.in_(guids) PlexShow.library_id.in_(show_lib_ids), PlexShow.guid.in_(tv_guids)
) )
).scalars(): ).scalars():
shows.setdefault(s.guid, s) shows.setdefault(s.guid, s)
@@ -70,10 +78,12 @@ def _resolve_known_for(
out: list[dict] = [] out: list[dict] = []
for k in known_for: for k in known_for:
g = f"tmdb:{k.get('tmdb_id')}" g = f"tmdb:{k.get('tmdb_id')}"
if g in movies: # A credit resolves against its OWN media_type's bucket only.
if k.get("media_type") == "tv":
if g in shows:
out.append(show_card(shows[g], show_statuses.get(shows[g].id, "new")))
elif g in movies:
out.append(movie_card(movies[g], m_states.get(movies[g].id))) out.append(movie_card(movies[g], m_states.get(movies[g].id)))
elif g in shows:
out.append(show_card(shows[g], show_statuses.get(shows[g].id, "new")))
return out return out
+18 -2
View File
@@ -12,6 +12,7 @@ Ingested results are flagged (videos.via_search / channels.from_search) so the L
this search-discovered "noise" by default. Shorts and currently-live/upcoming videos are never this search-discovered "noise" by default. Shorts and currently-live/upcoming videos are never
ingested this way (Shorts are confirmed by the youtube.com/shorts probe; live/upcoming are ingested this way (Shorts are confirmed by the youtube.com/shorts probe; live/upcoming are
dropped from the search response up front).""" dropped from the search response up front)."""
import logging
from concurrent.futures import ThreadPoolExecutor from concurrent.futures import ThreadPoolExecutor
from datetime import datetime, timezone from datetime import datetime, timezone
@@ -43,6 +44,8 @@ from app.youtube import search_scrape
from app.youtube.client import YouTubeClient, YouTubeError from app.youtube.client import YouTubeClient, YouTubeError
from app.youtube.shorts import make_client, probe_is_short from app.youtube.shorts import make_client, probe_is_short
log = logging.getLogger("siftlode.search")
router = APIRouter(prefix="/api/search", tags=["search"]) router = APIRouter(prefix="/api/search", tags=["search"])
# search.list costs this many quota units per page. # search.list costs this many quota units per page.
@@ -162,12 +165,13 @@ def _ingest_candidates(
detail_by_id = {it["id"]: it for it in yt.get_videos(ids)} detail_by_id = {it["id"]: it for it in yt.get_videos(ids)}
except YouTubeError: except YouTubeError:
detail_by_id = {} detail_by_id = {}
enriched_at = datetime.now(timezone.utc) # when THIS enrich ran (distinct from the stub placeholder)
for v in videos: for v in videos:
item = detail_by_id.get(v.id) item = detail_by_id.get(v.id)
if item is None: if item is None:
continue continue
apply_video_details(v, item) apply_video_details(v, item)
v.enriched_at = now v.enriched_at = enriched_at
db.commit() db.commit()
# 4) Confirm/deny Shorts (no quota) so none enter via search. # 4) Confirm/deny Shorts (no quota) so none enter via search.
@@ -285,7 +289,19 @@ def search_youtube(
and it["channel_id"] and it["channel_id"]
and it["channel_id"] not in blocked and it["channel_id"] not in blocked
] ]
collected.extend(_ingest_candidates(db, yt, user, candidates)) # Ingest (DB writes + enrich) runs outside the page-fetch try above; guard it too so a
# write/shape failure degrades to a 422 with a message on this JSON endpoint instead of a
# bare 500 hidden behind the FE's generic "search failed" (same rationale as B-core-1).
try:
collected.extend(_ingest_candidates(db, yt, user, candidates))
except Exception as exc: # noqa: BLE001 — any ingest failure, not just YouTubeError
db.rollback()
log.exception("live search ingest failed for %r", term)
if collected:
break # keep what earlier pages already gathered
raise HTTPException(
status_code=422, detail=f"YouTube search failed: {exc}"
) from exc
next_cursor = page["next_page_token"] next_cursor = page["next_page_token"]
# The API source pages cost 100 units each — never auto-page; one page per request. # The API source pages cost 100 units each — never auto-page; one page per request.
if source == "api" or len(collected) >= limit or not next_cursor: if source == "api" or len(collected) >= limit or not next_cursor:
+42
View File
@@ -212,3 +212,45 @@ def test_enrich_batch_skips_shows_without_guid(db):
assert tmdb.enrich_batch(db, FakeTmdb(), 50) == 0 assert tmdb.enrich_batch(db, FakeTmdb(), 50) == 0
db.refresh(sh) db.refresh(sh)
assert sh.tmdb_enriched_at is None assert sh.tmdb_enriched_at is None
def test_resolve_recs_matches_by_media_type_not_bare_id(db):
# TMDB movie ids and tv ids are independent, so tmdb:500 can be BOTH a show (in our library) and a
# movie recommendation. The match must respect media_type — a bare-id match would deep-link the movie
# rec to the unrelated show (and vice-versa).
mlib = _lib(db, key="10", kind="movie")
slib = _lib(db, key="11", kind="show")
show500 = PlexShow(rating_key="s500", library_id=slib.id, title="Show 500", guid="tmdb:500")
movie900 = PlexItem(
rating_key="m900", library_id=mlib.id, kind="movie", title="Movie 900", guid="tmdb:900"
)
db.add_all([show500, movie900])
db.commit()
# A movie rec for id 500 must NOT match show s500; a tv rec for id 900 must NOT match movie m900.
miss = {
r["tmdb_id"]: r
for r in tmdb.resolve_recs(
db,
[
{"tmdb_id": 500, "title": "Movie 500", "media_type": "movie"},
{"tmdb_id": 900, "title": "Show 900", "media_type": "tv"},
],
)
}
assert miss[500]["in_library"] is False and miss[500]["rating_key"] is None
assert miss[900]["in_library"] is False and miss[900]["rating_key"] is None
# The right media_type DOES resolve to the right row.
hit = {
r["tmdb_id"]: r
for r in tmdb.resolve_recs(
db,
[
{"tmdb_id": 900, "media_type": "movie"},
{"tmdb_id": 500, "media_type": "tv"},
],
)
}
assert hit[900]["in_library"] is True and hit[900]["rating_key"] == "m900"
assert hit[500]["in_library"] is True and hit[500]["rating_key"] == "s500"