fix(plex): address S1 code-review findings

- 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.
This commit is contained in:
2026-08-16 07:06:19 +02:00
parent f8df504d18
commit e008c48284
6 changed files with 166 additions and 50 deletions
+5 -13
View File
@@ -9,11 +9,11 @@ The cache directory (``cache.CACHE_DIR``) is shared with the subtitle cache (see
""" """
from fastapi import Response from fastapi import Response
from sqlalchemy import select
from sqlalchemy.orm import Session from sqlalchemy.orm import Session
from app.models import PlexCollection, PlexItem, PlexSeason, PlexShow from app.models import PlexCollection, PlexItem, PlexSeason, PlexShow
from app.plex.cache import CACHE_DIR 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")] _EXT_CT = [("jpg", "image/jpeg"), ("png", "image/png"), ("webp", "image/webp")]
_CT_EXT = {ct: ext for ext, ct in _EXT_CT} _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: 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 """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.""" are proxyable — this is not an open image proxy."""
it = db.execute( it = by_rating_key(db, PlexItem, rating_key)
select(PlexItem).where(PlexItem.rating_key == rating_key)
).scalars().first()
if it is not None: if it is not None:
return it.art_key if variant == "art" else it.thumb_key return it.art_key if variant == "art" else it.thumb_key
sh = db.execute( sh = by_rating_key(db, PlexShow, rating_key)
select(PlexShow).where(PlexShow.rating_key == rating_key)
).scalars().first()
if sh is not None: if sh is not None:
return sh.art_key if variant == "art" else sh.thumb_key return sh.art_key if variant == "art" else sh.thumb_key
se = db.execute( se = by_rating_key(db, PlexSeason, rating_key)
select(PlexSeason).where(PlexSeason.rating_key == rating_key)
).scalars().first()
if se is not None: if se is not None:
return se.thumb_key return se.thumb_key
col = db.execute( col = by_rating_key(db, PlexCollection, rating_key)
select(PlexCollection).where(PlexCollection.rating_key == rating_key)
).scalars().first()
if col is not None: if col is not None:
return col.thumb_key return col.thumb_key
return None return None
+11
View File
@@ -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()
+2 -4
View File
@@ -3,20 +3,18 @@
import logging import logging
from fastapi import HTTPException from fastapi import HTTPException
from sqlalchemy import select
from sqlalchemy.orm import Session from sqlalchemy.orm import Session
from app.models import ( from app.models import (
PlexItem, PlexItem,
) )
from app.plex.lookups import by_rating_key
log = logging.getLogger("siftlode.plex") log = logging.getLogger("siftlode.plex")
def item_or_404(db: Session, rating_key: str) -> PlexItem: def item_or_404(db: Session, rating_key: str) -> PlexItem:
it = db.execute( it = by_rating_key(db, PlexItem, rating_key)
select(PlexItem).where(PlexItem.rating_key == str(rating_key))
).scalars().first()
if it is None: if it is None:
raise HTTPException(status_code=404, detail="Unknown Plex item") raise HTTPException(status_code=404, detail="Unknown Plex item")
return it return it
+13 -25
View File
@@ -43,6 +43,7 @@ from app.plex import people as plex_people
from app.plex import rollup, subtitles from app.plex import rollup, subtitles
from app.plex import sync as plex_sync from app.plex import sync as plex_sync
from app.plex.client import PlexClient, PlexError, PlexNotConfigured 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._common import owned_or_404
from app.routes.admin import admin_user from app.routes.admin import admin_user
from app.routes.feed import to_tsquery_str 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: def _editable_collection_or_403(db: Session, rating_key: str) -> PlexCollection:
col = db.execute( col = by_rating_key(db, PlexCollection, rating_key)
select(PlexCollection).where(PlexCollection.rating_key == str(rating_key))
).scalars().first()
if col is None: if col is None:
raise HTTPException(status_code=404, detail="Unknown collection") raise HTTPException(status_code=404, detail="Unknown collection")
if not _collection_editable(col): if not _collection_editable(col):
@@ -882,9 +881,7 @@ def collection_set_editable(
) -> dict: ) -> dict:
"""Mark an existing Plex collection as 'mine to edit' (or unmark). Smart / auto-list collections """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.""" (IMDb/TMDb/…) can never be made editable — they're regenerated outside Plex."""
col = db.execute( col = by_rating_key(db, PlexCollection, rating_key)
select(PlexCollection).where(PlexCollection.rating_key == str(rating_key))
).scalars().first()
if col is None: if col is None:
raise HTTPException(status_code=404, detail="Unknown collection") raise HTTPException(status_code=404, detail="Unknown collection")
want = body.editable want = body.editable
@@ -1008,9 +1005,7 @@ def create_playlist(
count = 0 count = 0
thumb_rk = None thumb_rk = None
if seed: if seed:
it = db.execute( it = by_rating_key(db, PlexItem, seed)
select(PlexItem).where(PlexItem.rating_key == seed)
).scalars().first()
if it is not None: if it is not None:
db.add(PlexPlaylistItem(playlist_id=pl.id, item_id=it.id, position=0)) db.add(PlexPlaylistItem(playlist_id=pl.id, item_id=it.id, position=0))
count, thumb_rk = 1, it.rating_key count, thumb_rk = 1, it.rating_key
@@ -1105,9 +1100,7 @@ def playlist_add_item(
db: Session = Depends(get_db), db: Session = Depends(get_db),
) -> dict: ) -> dict:
pl = _own_playlist_or_404(db, pid, user) pl = _own_playlist_or_404(db, pid, user)
it = db.execute( it = by_rating_key(db, PlexItem, body.item_rating_key)
select(PlexItem).where(PlexItem.rating_key == body.item_rating_key)
).scalars().first()
if it is None: if it is None:
raise HTTPException(status_code=404, detail="Unknown item") raise HTTPException(status_code=404, detail="Unknown item")
exists = db.execute( exists = db.execute(
@@ -1137,9 +1130,7 @@ def playlist_remove_item(
db: Session = Depends(get_db), db: Session = Depends(get_db),
) -> dict: ) -> dict:
pl = _own_playlist_or_404(db, pid, user) pl = _own_playlist_or_404(db, pid, user)
it = db.execute( it = by_rating_key(db, PlexItem, item_rating_key)
select(PlexItem).where(PlexItem.rating_key == str(item_rating_key))
).scalars().first()
if it is not None: if it is not None:
db.execute( db.execute(
delete(PlexPlaylistItem).where( delete(PlexPlaylistItem).where(
@@ -1237,7 +1228,8 @@ def playlist_remove_bulk(
removed = db.execute( removed = db.execute(
delete(PlexPlaylistItem).where( delete(PlexPlaylistItem).where(
PlexPlaylistItem.playlist_id == pid, PlexPlaylistItem.item_id.in_(ids) PlexPlaylistItem.playlist_id == pid, PlexPlaylistItem.item_id.in_(ids)
) ),
execution_options={"synchronize_session": False},
).rowcount ).rowcount
if removed: if removed:
pl.updated_at = datetime.now(timezone.utc) pl.updated_at = datetime.now(timezone.utc)
@@ -1332,9 +1324,7 @@ def show_detail(
db: Session = Depends(get_db), db: Session = Depends(get_db),
) -> dict: ) -> dict:
"""A show's seasons + episodes (the drill-down view) with per-user watch state.""" """A show's seasons + episodes (the drill-down view) with per-user watch state."""
sh = db.execute( sh = by_rating_key(db, PlexShow, rating_key)
select(PlexShow).where(PlexShow.rating_key == str(rating_key))
).scalars().first()
if sh is None: if sh is None:
raise HTTPException(status_code=404, detail="Unknown Plex show") raise HTTPException(status_code=404, detail="Unknown Plex show")
seasons = db.execute( 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]: def _episode_nav(db: Session, it: PlexItem) -> tuple[str | None, str | None]:
if it.kind != "episode" or it.show_id is None: if it.kind != "episode" or it.show_id is None:
return None, None return None, None
keys = [ keys = list(
r[0] db.execute(
for r in db.execute(
select(PlexItem.rating_key) select(PlexItem.rating_key)
.where(PlexItem.show_id == it.show_id, PlexItem.kind == "episode") .where(PlexItem.show_id == it.show_id, PlexItem.kind == "episode")
.order_by( .order_by(
PlexItem.season_number.asc().nullsfirst(), PlexItem.season_number.asc().nullsfirst(),
PlexItem.episode_number.asc().nullsfirst(), PlexItem.episode_number.asc().nullsfirst(),
) )
) ).scalars()
.all() )
]
try: try:
i = keys.index(it.rating_key) i = keys.index(it.rating_key)
except ValueError: except ValueError:
+3 -6
View File
@@ -30,6 +30,7 @@ from app.models import (
) )
from app.plex import imgcache, subtitles from app.plex import imgcache, subtitles
from app.plex import paths as plex_paths 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 people as plex_people
from app.plex import stream as plex_stream from app.plex import stream as plex_stream
from app.plex import watch_sync as plex_watch from app.plex import watch_sync as plex_watch
@@ -412,9 +413,7 @@ def show_state(
db: Session = Depends(get_db), db: Session = Depends(get_db),
) -> dict: ) -> dict:
"""Mark a whole show watched/unwatched (all its episodes) for the current user + push to Plex.""" """Mark a whole show watched/unwatched (all its episodes) for the current user + push to Plex."""
sh = db.execute( sh = by_rating_key(db, PlexShow, rating_key)
select(PlexShow).where(PlexShow.rating_key == str(rating_key))
).scalars().first()
if sh is None: if sh is None:
raise HTTPException(status_code=404, detail="Unknown Plex show") raise HTTPException(status_code=404, detail="Unknown Plex show")
watched = body.watched watched = body.watched
@@ -434,9 +433,7 @@ def season_state(
db: Session = Depends(get_db), db: Session = Depends(get_db),
) -> dict: ) -> dict:
"""Mark a whole season watched/unwatched (all its episodes) for the current user + push to Plex.""" """Mark a whole season watched/unwatched (all its episodes) for the current user + push to Plex."""
se = db.execute( se = by_rating_key(db, PlexSeason, rating_key)
select(PlexSeason).where(PlexSeason.rating_key == str(rating_key))
).scalars().first()
if se is None: if se is None:
raise HTTPException(status_code=404, detail="Unknown Plex season") raise HTTPException(status_code=404, detail="Unknown Plex season")
watched = body.watched watched = body.watched
+132 -2
View File
@@ -57,16 +57,21 @@ def _seed(db):
return {"user": user, "movie_lib": movie_lib, "show_lib": show_lib, "show": show} 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 monkeypatch.setattr(state, "_configured_cache", True) # the request gate 503s an unconfigured app
app.dependency_overrides[get_db] = lambda: db app.dependency_overrides[get_db] = lambda: db
app.dependency_overrides[current_user] = lambda: user app.dependency_overrides[current_user] = lambda: user
try: 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: finally:
app.dependency_overrides.clear() app.dependency_overrides.clear()
def _get(db, user, monkeypatch, path):
return _request(db, user, monkeypatch, "GET", path)
class TestUnifiedLibrary: class TestUnifiedLibrary:
def test_both_scope_unions_movies_and_shows_ordered_by_added(self, db, monkeypatch): def test_both_scope_unions_movies_and_shows_ordered_by_added(self, db, monkeypatch):
s = _seed(db) s = _seed(db)
@@ -197,3 +202,128 @@ class TestSyncUpsertIdempotency:
assert ep.rating_key == "e1" assert ep.rating_key == "e1"
assert ep.show_id == shows[0].id # episode linked to its show assert ep.show_id == shows[0].id # episode linked to its show
assert (ep.season_number, ep.episode_number) == (1, 1) 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