From 2b5de89eda0b2ed50222eee07bd417ed03703746 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 21 Aug 2026 03:16:24 +0200 Subject: [PATCH] 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:` 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. --- backend/app/plex/tmdb.py | 30 ++++++++++++++------- backend/app/routes/plex/discover.py | 40 +++++++++++++-------------- backend/app/routes/plex/people.py | 28 ++++++++++++------- backend/app/routes/search.py | 20 ++++++++++++-- backend/tests/test_plex_tmdb.py | 42 +++++++++++++++++++++++++++++ 5 files changed, 118 insertions(+), 42 deletions(-) diff --git a/backend/app/plex/tmdb.py b/backend/app/plex/tmdb.py index 91da0294..d53277f0 100644 --- a/backend/app/plex/tmdb.py +++ b/backend/app/plex/tmdb.py @@ -340,25 +340,37 @@ def run_enrich(db: Session) -> 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:`) 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: return [] - guids = [f"tmdb:{r['tmdb_id']}" for r in recs if r.get("tmdb_id")] - in_lib: dict[str, str] = {} - if guids: + movie_guids = [ + f"tmdb:{r['tmdb_id']}" for r in recs if r.get("tmdb_id") and r.get("media_type") != "tv" + ] + 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( 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( - 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] = [] 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}) return out diff --git a/backend/app/routes/plex/discover.py b/backend/app/routes/plex/discover.py index be50e250..d048ce9b 100644 --- a/backend/app/routes/plex/discover.py +++ b/backend/app/routes/plex/discover.py @@ -90,21 +90,16 @@ def _recently_added( ] tagged.sort(key=lambda t: (t[0] is not None, t[0]), reverse=True) top = tagged[:RAIL_LIMIT] - mcards = { - id(m): c - for m, c in zip( - [t[2] for t in top if t[1] == "movie"], - _movie_cards(db, uid, [t[2] for t in top if t[1] == "movie"]), - ) - } - scards = { - id(s): c - 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] + # Key the serialized cards back by rating_key (globally unique in Plex — a movie and a show never + # share one), so the merge doesn't depend on object identity staying alive. + top_movies = [o for _, kind, o in top if kind == "movie"] + top_shows = [o for _, kind, o in top if kind == "show"] + 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 [ + mcards[o.rating_key] if kind == "movie" else scards[o.rating_key] + for _, kind, o in top + ] def _continue_watching( @@ -112,7 +107,7 @@ def _continue_watching( ) -> list[dict]: """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.""" - rows: list[tuple[object, object, dict]] = [] # (resumed_at, sentinel-ordinal, card) + rows: list[tuple[object, dict]] = [] # (resumed_at, card) if movie_ids: mrows = db.execute( @@ -256,14 +251,15 @@ def _because_you_watched( } ordered_movies = [movies[rk] for rk in rks if rk in movies] 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))} - scards = {id(s): c for s, c in zip(ordered_shows, _show_cards(db, uid, ordered_shows))} + # rating_key is globally unique in Plex, so it keys the serialized cards back safely (no id()). + 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] = [] for rk in rks: - if rk in movies: - items.append(mcards[id(movies[rk])]) - elif rk in shows: - items.append(scards[id(shows[rk])]) + if rk in mcards: + items.append(mcards[rk]) + elif rk in scards: + items.append(scards[rk]) if not items: return None return {"anchor": {"id": anchor.rating_key, "title": anchor.title}, "items": items} diff --git a/backend/app/routes/plex/people.py b/backend/app/routes/plex/people.py index 203d4130..43df9c4f 100644 --- a/backend/app/routes/plex/people.py +++ b/backend/app/routes/plex/people.py @@ -36,24 +36,32 @@ def _resolve_known_for( popularity order. Matched by the canonical guid `tmdb:`; out-of-library credits are dropped.""" if not known_for: return [] - guids = [f"tmdb:{k['tmdb_id']}" for k in known_for if k.get("tmdb_id")] - if not guids: + # Bucket by media_type: TMDB movie ids and tv ids collide numerically, so a movie credit must only + # 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 [] movies: dict[str, PlexItem] = {} - if movie_lib_ids: + if movie_lib_ids and movie_guids: for m in db.execute( select(PlexItem).where( PlexItem.kind == "movie", PlexItem.library_id.in_(movie_lib_ids), - PlexItem.guid.in_(guids), + PlexItem.guid.in_(movie_guids), ) ).scalars(): movies.setdefault(m.guid, m) shows: dict[str, PlexShow] = {} - if show_lib_ids: + if show_lib_ids and tv_guids: for s in db.execute( 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(): shows.setdefault(s.guid, s) @@ -70,10 +78,12 @@ def _resolve_known_for( out: list[dict] = [] for k in known_for: 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))) - elif g in shows: - out.append(show_card(shows[g], show_statuses.get(shows[g].id, "new"))) return out diff --git a/backend/app/routes/search.py b/backend/app/routes/search.py index ac96fdfa..97bdcdd6 100644 --- a/backend/app/routes/search.py +++ b/backend/app/routes/search.py @@ -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 ingested this way (Shorts are confirmed by the youtube.com/shorts probe; live/upcoming are dropped from the search response up front).""" +import logging from concurrent.futures import ThreadPoolExecutor 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.shorts import make_client, probe_is_short +log = logging.getLogger("siftlode.search") + router = APIRouter(prefix="/api/search", tags=["search"]) # 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)} except YouTubeError: detail_by_id = {} + enriched_at = datetime.now(timezone.utc) # when THIS enrich ran (distinct from the stub placeholder) for v in videos: item = detail_by_id.get(v.id) if item is None: continue apply_video_details(v, item) - v.enriched_at = now + v.enriched_at = enriched_at db.commit() # 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"] 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"] # 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: diff --git a/backend/tests/test_plex_tmdb.py b/backend/tests/test_plex_tmdb.py index 853f7b89..1f654efb 100644 --- a/backend/tests/test_plex_tmdb.py +++ b/backend/tests/test_plex_tmdb.py @@ -212,3 +212,45 @@ def test_enrich_batch_skips_shows_without_guid(db): assert tmdb.enrich_batch(db, FakeTmdb(), 50) == 0 db.refresh(sh) 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"