From e008c482848a1dafe8aac02fcb4eb4474af62e2e Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 16 Aug 2026 07:06:19 +0200 Subject: [PATCH] fix(plex): address S1 code-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Restore synchronize_session=False on the remove-bulk Core delete (the 2.0 default is 'auto'; keep the pre-conversion contract explicit). - _episode_nav: list(...scalars()) instead of [r[0] for r in ...all()], matching the house style used across the conversion. - Extract app/plex/lookups.by_rating_key() and migrate the get-by-rating_key pattern at 13 sites (imgcache x4, _shared.item_or_404, playback show/season, browse x6). - Coverage: add TestFacetsWithData (the converted wsq aggregation with real data — the highest-risk untested builder), TestPlaylistAndCollectionEndpoints (playlist CRUD + collections list), and an item-state playback test. Full suite 256 passed, 2 skipped; ruff clean. --- backend/app/plex/imgcache.py | 18 +-- backend/app/plex/lookups.py | 11 ++ backend/app/routes/plex/_shared.py | 6 +- backend/app/routes/plex/browse.py | 38 ++---- backend/app/routes/plex/playback.py | 9 +- backend/tests/test_plex_query_conversion.py | 134 +++++++++++++++++++- 6 files changed, 166 insertions(+), 50 deletions(-) create mode 100644 backend/app/plex/lookups.py diff --git a/backend/app/plex/imgcache.py b/backend/app/plex/imgcache.py index 215e18ea..01a4a50d 100644 --- a/backend/app/plex/imgcache.py +++ b/backend/app/plex/imgcache.py @@ -9,11 +9,11 @@ The cache directory (``cache.CACHE_DIR``) is shared with the subtitle cache (see """ from fastapi import Response -from sqlalchemy import select from sqlalchemy.orm import Session from app.models import PlexCollection, PlexItem, PlexSeason, PlexShow from app.plex.cache import CACHE_DIR +from app.plex.lookups import by_rating_key _EXT_CT = [("jpg", "image/jpeg"), ("png", "image/png"), ("webp", "image/webp")] _CT_EXT = {ct: ext for ext, ct in _EXT_CT} @@ -25,24 +25,16 @@ SIZES = {"thumb": (400, 600), "art": (1280, 720)} def key_for(db: Session, rating_key: str, variant: str) -> str | None: """The stored Plex image path for a known rating_key (item/show/season). Only mirrored keys are proxyable — this is not an open image proxy.""" - it = db.execute( - select(PlexItem).where(PlexItem.rating_key == rating_key) - ).scalars().first() + it = by_rating_key(db, PlexItem, rating_key) if it is not None: return it.art_key if variant == "art" else it.thumb_key - sh = db.execute( - select(PlexShow).where(PlexShow.rating_key == rating_key) - ).scalars().first() + sh = by_rating_key(db, PlexShow, rating_key) if sh is not None: return sh.art_key if variant == "art" else sh.thumb_key - se = db.execute( - select(PlexSeason).where(PlexSeason.rating_key == rating_key) - ).scalars().first() + se = by_rating_key(db, PlexSeason, rating_key) if se is not None: return se.thumb_key - col = db.execute( - select(PlexCollection).where(PlexCollection.rating_key == rating_key) - ).scalars().first() + col = by_rating_key(db, PlexCollection, rating_key) if col is not None: return col.thumb_key return None diff --git a/backend/app/plex/lookups.py b/backend/app/plex/lookups.py new file mode 100644 index 00000000..fceba665 --- /dev/null +++ b/backend/app/plex/lookups.py @@ -0,0 +1,11 @@ +"""Small shared Plex row lookups, so the get-by-rating_key pattern lives in one place.""" +from sqlalchemy import select +from sqlalchemy.orm import Session + + +def by_rating_key(db: Session, model, rating_key: str): + """The mirrored row for a Plex rating_key, or None. `model` is any of the rating_key-keyed Plex + tables (PlexItem / PlexShow / PlexSeason / PlexCollection).""" + return db.execute( + select(model).where(model.rating_key == str(rating_key)) + ).scalars().first() diff --git a/backend/app/routes/plex/_shared.py b/backend/app/routes/plex/_shared.py index a35e216e..a5ecd2b4 100644 --- a/backend/app/routes/plex/_shared.py +++ b/backend/app/routes/plex/_shared.py @@ -3,20 +3,18 @@ import logging from fastapi import HTTPException -from sqlalchemy import select from sqlalchemy.orm import Session from app.models import ( PlexItem, ) +from app.plex.lookups import by_rating_key log = logging.getLogger("siftlode.plex") def item_or_404(db: Session, rating_key: str) -> PlexItem: - it = db.execute( - select(PlexItem).where(PlexItem.rating_key == str(rating_key)) - ).scalars().first() + it = by_rating_key(db, PlexItem, rating_key) if it is None: raise HTTPException(status_code=404, detail="Unknown Plex item") return it diff --git a/backend/app/routes/plex/browse.py b/backend/app/routes/plex/browse.py index 99a3f20f..1c13380a 100644 --- a/backend/app/routes/plex/browse.py +++ b/backend/app/routes/plex/browse.py @@ -43,6 +43,7 @@ from app.plex import people as plex_people from app.plex import rollup, subtitles from app.plex import sync as plex_sync from app.plex.client import PlexClient, PlexError, PlexNotConfigured +from app.plex.lookups import by_rating_key from app.routes._common import owned_or_404 from app.routes.admin import admin_user from app.routes.feed import to_tsquery_str @@ -738,9 +739,7 @@ def collections( def _editable_collection_or_403(db: Session, rating_key: str) -> PlexCollection: - col = db.execute( - select(PlexCollection).where(PlexCollection.rating_key == str(rating_key)) - ).scalars().first() + col = by_rating_key(db, PlexCollection, rating_key) if col is None: raise HTTPException(status_code=404, detail="Unknown collection") if not _collection_editable(col): @@ -882,9 +881,7 @@ def collection_set_editable( ) -> dict: """Mark an existing Plex collection as 'mine to edit' (or unmark). Smart / auto-list collections (IMDb/TMDb/…) can never be made editable — they're regenerated outside Plex.""" - col = db.execute( - select(PlexCollection).where(PlexCollection.rating_key == str(rating_key)) - ).scalars().first() + col = by_rating_key(db, PlexCollection, rating_key) if col is None: raise HTTPException(status_code=404, detail="Unknown collection") want = body.editable @@ -1008,9 +1005,7 @@ def create_playlist( count = 0 thumb_rk = None if seed: - it = db.execute( - select(PlexItem).where(PlexItem.rating_key == seed) - ).scalars().first() + it = by_rating_key(db, PlexItem, seed) if it is not None: db.add(PlexPlaylistItem(playlist_id=pl.id, item_id=it.id, position=0)) count, thumb_rk = 1, it.rating_key @@ -1105,9 +1100,7 @@ def playlist_add_item( db: Session = Depends(get_db), ) -> dict: pl = _own_playlist_or_404(db, pid, user) - it = db.execute( - select(PlexItem).where(PlexItem.rating_key == body.item_rating_key) - ).scalars().first() + it = by_rating_key(db, PlexItem, body.item_rating_key) if it is None: raise HTTPException(status_code=404, detail="Unknown item") exists = db.execute( @@ -1137,9 +1130,7 @@ def playlist_remove_item( db: Session = Depends(get_db), ) -> dict: pl = _own_playlist_or_404(db, pid, user) - it = db.execute( - select(PlexItem).where(PlexItem.rating_key == str(item_rating_key)) - ).scalars().first() + it = by_rating_key(db, PlexItem, item_rating_key) if it is not None: db.execute( delete(PlexPlaylistItem).where( @@ -1237,7 +1228,8 @@ def playlist_remove_bulk( removed = db.execute( delete(PlexPlaylistItem).where( PlexPlaylistItem.playlist_id == pid, PlexPlaylistItem.item_id.in_(ids) - ) + ), + execution_options={"synchronize_session": False}, ).rowcount if removed: pl.updated_at = datetime.now(timezone.utc) @@ -1332,9 +1324,7 @@ def show_detail( db: Session = Depends(get_db), ) -> dict: """A show's seasons + episodes (the drill-down view) with per-user watch state.""" - sh = db.execute( - select(PlexShow).where(PlexShow.rating_key == str(rating_key)) - ).scalars().first() + sh = by_rating_key(db, PlexShow, rating_key) if sh is None: raise HTTPException(status_code=404, detail="Unknown Plex show") seasons = db.execute( @@ -1633,18 +1623,16 @@ def _collection_strips(db: Session, it: PlexItem, user_id: int) -> list[dict]: def _episode_nav(db: Session, it: PlexItem) -> tuple[str | None, str | None]: if it.kind != "episode" or it.show_id is None: return None, None - keys = [ - r[0] - for r in db.execute( + keys = list( + db.execute( select(PlexItem.rating_key) .where(PlexItem.show_id == it.show_id, PlexItem.kind == "episode") .order_by( PlexItem.season_number.asc().nullsfirst(), PlexItem.episode_number.asc().nullsfirst(), ) - ) - .all() - ] + ).scalars() + ) try: i = keys.index(it.rating_key) except ValueError: diff --git a/backend/app/routes/plex/playback.py b/backend/app/routes/plex/playback.py index fed33581..58eafb3b 100644 --- a/backend/app/routes/plex/playback.py +++ b/backend/app/routes/plex/playback.py @@ -30,6 +30,7 @@ from app.models import ( ) from app.plex import imgcache, subtitles from app.plex import paths as plex_paths +from app.plex.lookups import by_rating_key from app.plex import people as plex_people from app.plex import stream as plex_stream from app.plex import watch_sync as plex_watch @@ -412,9 +413,7 @@ def show_state( db: Session = Depends(get_db), ) -> dict: """Mark a whole show watched/unwatched (all its episodes) for the current user + push to Plex.""" - sh = db.execute( - select(PlexShow).where(PlexShow.rating_key == str(rating_key)) - ).scalars().first() + sh = by_rating_key(db, PlexShow, rating_key) if sh is None: raise HTTPException(status_code=404, detail="Unknown Plex show") watched = body.watched @@ -434,9 +433,7 @@ def season_state( db: Session = Depends(get_db), ) -> dict: """Mark a whole season watched/unwatched (all its episodes) for the current user + push to Plex.""" - se = db.execute( - select(PlexSeason).where(PlexSeason.rating_key == str(rating_key)) - ).scalars().first() + se = by_rating_key(db, PlexSeason, rating_key) if se is None: raise HTTPException(status_code=404, detail="Unknown Plex season") watched = body.watched diff --git a/backend/tests/test_plex_query_conversion.py b/backend/tests/test_plex_query_conversion.py index 014bfe7a..d4b2ee98 100644 --- a/backend/tests/test_plex_query_conversion.py +++ b/backend/tests/test_plex_query_conversion.py @@ -57,16 +57,21 @@ def _seed(db): return {"user": user, "movie_lib": movie_lib, "show_lib": show_lib, "show": show} -def _get(db, user, monkeypatch, path): +def _request(db, user, monkeypatch, method, path, json=None): monkeypatch.setattr(state, "_configured_cache", True) # the request gate 503s an unconfigured app app.dependency_overrides[get_db] = lambda: db app.dependency_overrides[current_user] = lambda: user try: - return TestClient(app).get(path) # no context manager: skip lifespan (scheduler/HLS sweep) + client = TestClient(app) # no context manager: skip lifespan (scheduler/HLS sweep) + return client.request(method, path, json=json) finally: app.dependency_overrides.clear() +def _get(db, user, monkeypatch, path): + return _request(db, user, monkeypatch, "GET", path) + + class TestUnifiedLibrary: def test_both_scope_unions_movies_and_shows_ordered_by_added(self, db, monkeypatch): s = _seed(db) @@ -197,3 +202,128 @@ class TestSyncUpsertIdempotency: assert ep.rating_key == "e1" assert ep.show_id == shows[0].id # episode linked to its show assert (ep.season_number, ep.episode_number) == (1, 1) + + +class TestFacetsWithData: + """The facets endpoint's wsq builder is the most complex converted query (genre/content-rating + JSONB unnest + year/rating/duration bounds + the watch-state join). The existing smoke test only + covers the null-genre guard; this locks the aggregated numbers with real data.""" + + def test_facet_aggregation(self, db, monkeypatch): + movie_lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True) + show_lib = PlexLibrary(plex_key="2", title="Shows", kind="show", enabled=True) + db.add_all([movie_lib, show_lib]) + db.flush() + db.add_all([ + PlexItem(rating_key="m1", library_id=movie_lib.id, kind="movie", title="M1", + genres=["Comedy", "Drama"], content_rating="PG", year=2000, rating=7.5, + duration_s=6000), + PlexItem(rating_key="m2", library_id=movie_lib.id, kind="movie", title="M2", + genres=["Comedy"], content_rating="R", year=2010, rating=8.0, duration_s=7200), + PlexShow(rating_key="s1", library_id=show_lib.id, title="S1", + genres=["Drama"], content_rating="TV-14", year=2005, rating=9.0), + ]) + user = User(email="facetdata@example.com") + db.add(user) + db.commit() + + body = _get(db, user, monkeypatch, "/api/plex/facets?scope=both").json() + assert {g["value"]: g["count"] for g in body["genres"]} == {"Comedy": 2, "Drama": 2} + assert {c["value"]: c["count"] for c in body["content_ratings"]} == { + "PG": 1, "R": 1, "TV-14": 1 + } + assert (body["year_min"], body["year_max"]) == (2000, 2010) + assert body["rating_max"] == 9.0 + # Duration bounds are movie-only. + assert (body["duration_min"], body["duration_max"]) == (6000, 7200) + + +class TestPlaylistAndCollectionEndpoints: + """Smoke coverage for the converted CRUD endpoints that had no tests: playlist create/list/detail/ + add/reorder/remove/delete, and the collections list. A 1.x->2.0 conversion slip in any of these + (a lost .scalars(), a wrong .all(), a broken order_by) surfaces here instead of shipping green.""" + + def _movie(self, db, lib_id, rk, title): + return PlexItem(rating_key=rk, library_id=lib_id, kind="movie", title=title) + + def test_playlist_crud_roundtrip(self, db, monkeypatch): + lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True) + db.add(lib) + db.flush() + db.add_all([self._movie(db, lib.id, "m1", "First"), self._movie(db, lib.id, "m2", "Second")]) + user = User(email="pl@example.com") + db.add(user) + db.commit() + + # create (seeded with m1) -> item_count 1 + created = _request(db, user, monkeypatch, "POST", "/api/plex/playlists", + {"title": "Faves", "item_rating_key": "m1"}) + assert created.status_code == 200, created.text + pid = created.json()["id"] + assert created.json()["item_count"] == 1 + + # list -> the playlist with its count + listed = _get(db, user, monkeypatch, "/api/plex/playlists").json()["playlists"] + assert [(p["id"], p["item_count"]) for p in listed] == [(pid, 1)] + + # add m2, then reorder to [m2, m1] + add = _request(db, user, monkeypatch, "POST", f"/api/plex/playlists/{pid}/items", + {"item_rating_key": "m2"}) + assert add.status_code == 200, add.text + _request(db, user, monkeypatch, "PUT", f"/api/plex/playlists/{pid}/order", + {"item_rating_keys": ["m2", "m1"]}) + detail = _get(db, user, monkeypatch, f"/api/plex/playlists/{pid}").json() + assert [it["id"] for it in detail["items"]] == ["m2", "m1"] # reorder applied, in order + + # remove m1 -> one item left + rm = _request(db, user, monkeypatch, "DELETE", f"/api/plex/playlists/{pid}/items/m1") + assert rm.status_code == 200, rm.text + after = _get(db, user, monkeypatch, f"/api/plex/playlists/{pid}").json() + assert [it["id"] for it in after["items"]] == ["m2"] + + # delete the playlist -> gone from the list + _request(db, user, monkeypatch, "DELETE", f"/api/plex/playlists/{pid}") + assert _get(db, user, monkeypatch, "/api/plex/playlists").json()["playlists"] == [] + + def test_collections_list(self, db, monkeypatch): + from app.models import PlexCollection + lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True) + db.add(lib) + db.flush() + db.add_all([ + PlexCollection(rating_key="c1", library_id=lib.id, title="Marvel", child_count=12), + PlexCollection(rating_key="c2", library_id=lib.id, title="Pixar", child_count=5), + ]) + user = User(email="col@example.com") + db.add(user) + db.commit() + + # unified scope (no library param) -> most-populated first + body = _get(db, user, monkeypatch, "/api/plex/collections").json() + assert [c["title"] for c in body["collections"]] == ["Marvel", "Pixar"] + # name filter + filtered = _get(db, user, monkeypatch, "/api/plex/collections?q=pix").json() + assert [c["title"] for c in filtered["collections"]] == ["Pixar"] + + def test_item_state_watched_then_new(self, db, monkeypatch): + # No PlexLink -> the background Plex push is a no-op; only the local PlexState path runs, which + # is what the conversion touched (the st lookup + the delete on ->new). + lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True) + db.add(lib) + db.flush() + db.add(PlexItem(rating_key="m1", library_id=lib.id, kind="movie", title="M1")) + user = User(email="state@example.com") + db.add(user) + db.commit() + + watched = _request(db, user, monkeypatch, "POST", "/api/plex/item/m1/state", + {"status": "watched"}) + assert watched.status_code == 200, watched.text + assert watched.json()["status"] == "watched" + row = db.execute(select(PlexState).where(PlexState.user_id == user.id)).scalars().one() + assert row.status == "watched" + + # -> new deletes the state row + back = _request(db, user, monkeypatch, "POST", "/api/plex/item/m1/state", {"status": "new"}) + assert back.status_code == 200, back.text + assert db.execute(select(PlexState).where(PlexState.user_id == user.id)).scalars().first() is None