refactor(reader): the HTTP layer reads through queries and writes through the service

Feed, playlists, saved views, tags and the Home pins do no database work in their routes any more:
reads go through reader/queries (and feedquery for a feed page, its counts and facets), writes
through reader/service and discover/pins, which flush; the handlers commit. Reader is fenced below
its HTTP layer too: its owner filters go through userscope, and the catalog's jobs ask Reader
instead of joining its tables (the auto-tagger's system tags, the cleanup jobs' keep rule, the
maintenance notice). A playlist sync's catalog ingest moves to sync/videos.
This commit is contained in:
2026-10-19 01:20:14 +02:00
parent e82f87acb0
commit dbb4065e7b
24 changed files with 1489 additions and 915 deletions
+8 -5
View File
@@ -1,4 +1,4 @@
"""sync/autotag's orphan cleanup (the module was 32% covered).
"""The auto-tag job's orphan cleanup (`reader.service.prune_orphan_system_tags`).
A system tag no channel carries any more is deleted — unless a saved feed view filters on it. The
views hold tag ids; a deleted tag returns under a NEW id when a channel earns it again, and the feed
@@ -9,7 +9,7 @@ from sqlalchemy import select
from app.models import Channel
from app.reader.models import ChannelTag, SavedView, Tag
from app.sync import autotag
from app.reader import service as reader_service
def test_an_orphan_system_tag_goes_but_one_a_saved_view_filters_on_stays(db, make_user):
@@ -31,7 +31,8 @@ def test_an_orphan_system_tag_goes_but_one_a_saved_view_filters_on_stays(db, mak
db.add(SavedView(user_id=user.id, source="plex", name="P", filters={"genres": ["Drama"]}))
db.commit()
assert autotag._cleanup_orphan_system_tags(db) == 1
assert reader_service.prune_orphan_system_tags(db) == 1
db.commit()
db.expire_all()
names = set(db.execute(select(Tag.name)).scalars())
assert names == {"Gaming", "Music", "Mine"} # only the unreferenced system orphan went
@@ -49,7 +50,8 @@ def test_a_malformed_saved_view_cannot_break_the_cleanup(db, make_user):
):
db.add(SavedView(user_id=user.id, source="feed", name=f"bad{i}", filters=filters))
db.commit()
assert autotag._cleanup_orphan_system_tags(db) == 1
assert reader_service.prune_orphan_system_tags(db) == 1
db.commit()
db.expire_all()
assert db.execute(select(Tag).where(Tag.name == "Cooking")).first() is None
@@ -62,4 +64,5 @@ def test_a_view_kept_tag_list_larger_than_a_bind_parameter_limit_still_cleans_up
huge = list(range(10_000_000, 10_070_000)) # past psycopg's 65535 bind parameters
db.add(SavedView(user_id=user.id, source="feed", name="huge", filters={"tags": huge}))
db.commit()
assert autotag._cleanup_orphan_system_tags(db) == 1
assert reader_service.prune_orphan_system_tags(db) == 1
db.commit()
+2 -2
View File
@@ -75,7 +75,7 @@ class TestMergePlan:
class FakeYouTube:
"""Enough of YouTubeClient for merge_playlist: an in-memory playlist of (item_id, video_id)
and the write ops that mutate it. Every catalog video is pre-seeded so _ensure_videos never
and the write ops that mutate it. Every catalog video is pre-seeded so ensure_videos never
needs get_videos."""
def __init__(
@@ -89,7 +89,7 @@ class FakeYouTube:
self.title = title
self._seq = 0
self.items = [(self._new_id(), v) for v in items]
# Ids the Data API won't return (private/deleted) — get_videos omits them, so _ensure_videos
# Ids the Data API won't return (private/deleted) — get_videos omits them, so ensure_videos
# can't ingest them. They still live in the playlist (`items`) like a real un-ingestable video.
self.unavailable = set(unavailable or ())
# Ids whose add_playlist_item raises, to exercise the partial-failure baseline-hold path.
+2 -2
View File
@@ -280,13 +280,13 @@ def test_no_add_can_land_in_the_target_while_a_move_holds_it(db, make_user):
from sqlalchemy import text
from app.db import SessionLocal
from app.routes import playlists as plroutes
from app.sync.playlists import lock_playlists
user = make_user()
_seed_videos(db, ["v1"])
src = _playlist(db, user, "Source", ["v1"])
dst = _playlist(db, user, "Target", [])
plroutes.lock_playlists(db, src.id, dst.id) # what the move holds until its commit
lock_playlists(db, src.id, dst.id) # what the move holds until its commit
with SessionLocal() as other:
other.execute(text("SET LOCAL lock_timeout = '300ms'"))
other.add(PlaylistItem(playlist_id=dst.id, video_id="v1", position=0))
+307
View File
@@ -0,0 +1,307 @@
"""Reader's service and queries, one use case each, below the HTTP layer.
The routes are thin callers now, and their own tests stay the contract a client sees. These hold
what each write does and leaves on its own: that it flushes and leaves the commit to its caller,
that a row of someone else's reads as a missing one, and the rule each write carries (one default
view per module, a resume point a status keeps, a move that never leaves a video in two lists, the
auto-tagger touching only its own links). DB-backed (real Postgres).
"""
from datetime import datetime, timezone
import pytest
from sqlalchemy import select
from sqlalchemy.exc import IntegrityError
from app.discover import pins
from app.models import Channel, Video
from app.reader import queries, service
from app.reader.models import ChannelTag, ComfortPin, Playlist, PlaylistItem, Tag, VideoState
def _videos(db, *ids):
if db.get(Channel, "UCr") is None:
db.add(Channel(id="UCr", title="R"))
now = datetime.now(timezone.utc)
for v in ids:
db.add(Video(id=v, channel_id="UCr", published_at=now, duration_seconds=60))
db.commit()
def _uncommitted(db):
"""What the caller still has to commit: the session holds pending work."""
return bool(db.new or db.dirty or db.deleted) or db.in_transaction()
def _ids(db, pl):
return [it.video_id for it in queries.items(db, pl.id)]
# --- whose a row is ------------------------------------------------------------------------
def test_a_row_of_someone_elses_reads_as_a_missing_one(db, make_user):
owner, stranger = make_user(), make_user()
tag = service.create_tag(db, owner, "Mine", "other", None)
view = service.create_view(db, owner, "feed", "V", {})
pl = service.create_playlist(db, owner, "L")
db.commit()
assert queries.own_tag(db, owner, tag.id) is tag
assert queries.own_tag(db, stranger, tag.id) is None
assert queries.own_view(db, stranger, view.id) is None
assert queries.own_playlist(db, stranger, pl.id) is None
assert queries.own_playlist(db, owner, pl.id) is pl
assert queries.playlists_for(db, stranger) == []
def test_a_system_tag_is_nobodys_to_edit(db, make_user):
user = make_user()
system = service.ensure_system_tag(db, "Music", "topic", None)
db.commit()
assert queries.own_tag(db, user, system.id) is None
assert [t.id for t, _ in queries.tags_with_counts(db, user)] == [system.id]
# --- tags ------------------------------------------------------------------------------------
def test_a_tag_is_made_once_per_name_and_an_unknown_category_is_other(db, make_user):
user = make_user()
tag = service.create_tag(db, user, "Later", "nonsense", "#fff")
assert _uncommitted(db)
db.commit()
assert tag.category == "other"
with pytest.raises(IntegrityError):
service.create_tag(db, user, "Later", "topic", None)
db.rollback()
def test_a_tag_changes_only_what_was_sent_and_a_sent_null_clears_its_color(db, make_user):
user = make_user()
tag = service.create_tag(db, user, "A", "other", "#fff")
service.update_tag(tag, sent={"name"}, name="B", color=None)
assert (tag.name, tag.color) == ("B", "#fff")
service.update_tag(tag, sent={"color"}, name=None, color=None)
assert (tag.name, tag.color) == ("B", None)
service.delete_tag(db, tag)
db.commit()
assert db.get(Tag, tag.id) is None
def test_the_auto_tagger_moves_only_its_own_links(db, make_user):
user = make_user()
_videos(db)
gaming = service.ensure_system_tag(db, "Gaming", "topic", None)
music = service.ensure_system_tag(db, "Music", "topic", None)
assert service.ensure_system_tag(db, "Gaming", "topic", None) is gaming # made once
mine = service.create_tag(db, user, "Fav", "other", None)
db.add(ChannelTag(channel_id="UCr", tag_id=mine.id, user_id=user.id, source="user"))
service.set_auto_tags(db, "UCr", {gaming.id: 0.5})
db.commit()
service.set_auto_tags(db, "UCr", {music.id: None})
db.commit()
links = db.execute(select(ChannelTag.tag_id, ChannelTag.source)).tuples().all()
assert sorted(links) == sorted([(music.id, "auto"), (mine.id, "user")])
assert db.execute(
select(Channel.id).where(queries.has_auto_tags(Channel.id))
).scalars().all() == ["UCr"]
# --- saved views -----------------------------------------------------------------------------
def test_views_number_per_module_and_one_default_per_module(db, make_user):
user = make_user()
a = service.create_view(db, user, "feed", "A", {})
b = service.create_view(db, user, "feed", "B", {})
plex = service.create_view(db, user, "plex", "P", {})
assert (a.position, b.position, plex.position) == (0, 1, 0)
service.update_view(db, plex, sent={"is_default"}, is_default=True)
service.update_view(db, a, sent={"is_default"}, is_default=True)
service.update_view(db, b, sent={"is_default"}, is_default=True)
db.commit()
db.expire_all()
assert [(v.name, v.is_default) for v in queries.views(db, user, "feed")] == [
("A", False),
("B", True),
]
assert queries.views(db, user, "plex")[0].is_default is True # another module's default stays
def test_reordering_views_skips_ids_that_are_not_the_callers(db, make_user):
user, other = make_user(), make_user()
a = service.create_view(db, user, "feed", "A", {})
b = service.create_view(db, user, "feed", "B", {})
theirs = service.create_view(db, other, "feed", "T", {})
db.commit()
service.reorder_views(db, user, [theirs.id, b.id, a.id])
db.commit()
assert [v.name for v in queries.views(db, user, "feed")] == ["B", "A"]
assert theirs.position == 0
# --- watch state -----------------------------------------------------------------------------
def _state(db, user, vid):
db.expire_all()
return queries.video_state(db, user, vid)
def test_unmarking_keeps_a_resume_point_and_drops_a_row_with_nothing_else(db, make_user):
user = make_user()
_videos(db, "v1", "v2")
service.mark(db, user, "v1", "watched")
service.mark(db, user, "v2", "watched")
service.set_progress(db, user, "v1", 120)
db.commit()
service.unmark(db, user, "v1")
service.unmark(db, user, "v2")
db.commit()
kept = _state(db, user, "v1")
assert (kept.status, kept.watched_at, kept.position_seconds) == ("new", None, 120)
assert _state(db, user, "v2") is None
def test_hiding_a_watched_video_keeps_when_it_was_watched(db, make_user):
user = make_user()
_videos(db, "v1")
service.mark(db, user, "v1", "watched")
db.commit()
watched_at = _state(db, user, "v1").watched_at
service.mark(db, user, "v1", "hidden")
db.commit()
row = _state(db, user, "v1")
assert (row.status, row.watched_at) == ("hidden", watched_at)
assert db.execute(select(VideoState)).scalars().all() == [row] # one row per user and video
def test_a_cleared_resume_point_drops_a_bare_row_and_zeroes_one_with_a_status(db, make_user):
user = make_user()
_videos(db, "v1", "v2")
service.set_progress(db, user, "v1", 30)
service.set_progress(db, user, "v2", 30)
service.mark(db, user, "v2", "hidden")
db.commit()
service.set_progress(db, user, "v1", 0)
service.set_progress(db, user, "v2", 0)
db.commit()
assert _state(db, user, "v1") is None
assert (_state(db, user, "v2").status, _state(db, user, "v2").position_seconds) == ("hidden", 0)
service.clear_state(db, user, "v2")
db.commit()
assert _state(db, user, "v2") is None
# --- playlists -------------------------------------------------------------------------------
def test_watch_later_is_made_once(db, make_user):
user = make_user()
first = service.watch_later(db, user)
db.commit()
assert service.watch_later(db, user) is first
assert first.kind == "watch_later"
assert [p.id for p in queries.playlists_for(db, user)] == [first.id]
def test_an_add_is_idempotent_and_lands_where_asked(db, make_user):
user = make_user()
_videos(db, "a", "b", "c")
pl = service.create_playlist(db, user, "L")
assert service.add_item(db, pl, "a", mark_dirty=False) is True
assert service.add_item(db, pl, "a", mark_dirty=False) is False
service.add_to_playlist(db, pl, "b", None)
service.add_to_playlist(db, pl, "c", 0)
service.add_to_playlist(db, pl, "a", 2) # already there: stays put
assert _uncommitted(db)
db.commit()
assert _ids(db, pl) == ["c", "a", "b"]
assert service.remove_item(db, pl, "a", mark_dirty=True) is True
assert service.remove_item(db, pl, "a", mark_dirty=True) is False
db.commit()
assert _ids(db, pl) == ["c", "b"]
assert queries.item_counts(db, [pl.id]) == {pl.id: 2}
assert queries.durations(db, [pl.id]) == {pl.id: 120}
assert queries.containing(db, [pl.id], "b") == {pl.id}
def test_a_move_never_leaves_a_video_in_two_lists(db, make_user):
user = make_user()
_videos(db, "a", "b")
src = service.create_playlist(db, user, "S")
dst = service.create_playlist(db, user, "D")
service.add_item(db, src, "a", mark_dirty=False)
service.add_item(db, src, "b", mark_dirty=False)
service.add_item(db, dst, "b", mark_dirty=False)
db.commit()
with pytest.raises(service.MoveRefused, match="same_playlist"):
service.move_item(db, src, src, "a")
service.move_item(db, src, dst, "a")
service.move_item(db, src, dst, "b") # already in the target: dropped from the source
db.commit()
assert (_ids(db, src), _ids(db, dst)) == ([], ["b", "a"])
with pytest.raises(service.MoveRefused, match="not_in_playlist"):
service.move_item(db, src, dst, "a")
db.rollback()
def test_a_reorder_keeps_what_it_left_out_after_the_listed(db, make_user):
user = make_user()
_videos(db, "a", "b", "c")
pl = service.create_playlist(db, user, "L")
for v in ("a", "b", "c"):
service.add_item(db, pl, v, mark_dirty=False)
db.commit()
assert service.reorder_items(db, pl, ["c", "zzz", "a"]) == 3
db.commit()
assert _ids(db, pl) == ["c", "a", "b"]
def test_a_rename_and_a_delete(db, make_user):
user = make_user()
_videos(db, "a")
pl = service.create_playlist(db, user, "Old")
service.add_item(db, pl, "a", mark_dirty=False)
service.rename_playlist(db, pl, "New")
db.commit()
assert queries.playlist_totals(db, pl.id) == (1, 60, None)
assert pl.name == "New"
service.delete_playlist(db, pl)
db.commit()
assert db.execute(select(PlaylistItem)).first() is None
assert db.get(Playlist, pl.id) is None
# --- what the other units ask ----------------------------------------------------------------
def test_a_video_someone_holds_is_kept_and_its_playlisters_are_known(db, make_user):
user = make_user()
_videos(db, "watched", "listed", "loose")
service.mark(db, user, "watched", "watched")
pl = service.create_playlist(db, user, "L")
service.add_item(db, pl, "listed", mark_dirty=False)
db.commit()
kept = db.execute(select(Video.id).where(queries.kept_by_a_reader(Video.id))).scalars()
assert sorted(kept) == ["listed", "watched"]
assert queries.playlisted_by(db, ["listed", "loose"]) == [(user.id, "listed")]
# --- the comfort shelf -----------------------------------------------------------------------
def test_a_pin_is_one_per_thing_and_only_its_owners(db, make_user):
user, stranger = make_user(), make_user()
_videos(db, "a")
pin = pins.add(db, user, "video_id", "a")
db.commit()
assert pins.pin_of(db, user, "video_id", "a") is pin
assert pins.pin_of(db, stranger, "video_id", "a") is None
assert pins.own_pin(db, stranger, pin.id) is None
with pytest.raises(IntegrityError):
pins.add(db, user, "video_id", "a")
db.rollback()
pins.remove(db, pins.own_pin(db, user, pin.id))
db.commit()
assert db.execute(select(ComfortPin)).first() is None
+28 -10
View File
@@ -111,9 +111,6 @@ UNIT_ROUTES = {
# route's count is only the unit tables it imports; it is paid by the sprint of the unit it reaches.
ALLOWED = {
"channels.py": 83,
"discover.py": 7,
"feed.py": 36,
"playlists.py": 72,
"plex/_shared.py": 1,
"plex/admin.py": 12,
"plex/browse.py": 93,
@@ -124,11 +121,9 @@ ALLOWED = {
"plex/playback.py": 42,
"plex/playlists.py": 80,
"runs.py": 1,
"saved_views.py": 17,
"scheduler.py": 3,
"search.py": 46,
"sync.py": 38,
"tags.py": 10,
}
# The session methods that read or write data. `commit` and `rollback` stay with the handler.
@@ -149,7 +144,7 @@ SESSION_DATA = {
# What a route may name from SQLAlchemy: the session's type, for its `db: Session` parameter, and
# the errors its commit raises (the commit is the handler's, so is catching what it throws).
SQLALCHEMY_ALLOWED = {"Session"}
SQLALCHEMY_ALLOWED_MODULES = {"sqlalchemy.exc"}
SQLALCHEMY_ALLOWED_MODULES = {"sqlalchemy.exc", "sqlalchemy.orm.exc"}
# Objects a route owns and may set attributes on; anything else it writes to is a model.
OWN_OBJECTS = {"self", "cls", "request", "response"}
UNIT_MODEL_NAMES = {name: unit for unit, names in UNIT_MODELS.items() for name in names}
@@ -311,6 +306,7 @@ DETECTOR_CASES = {
"from app.capture.models import DownloadJob as J": 1,
"def f(cache): cache.get('k')": 0,
"from sqlalchemy.exc import IntegrityError\ntry:\n pass\nexcept IntegrityError:\n pass": 0,
"from sqlalchemy.orm.exc import StaleDataError\ntry:\n pass\nexcept StaleDataError:\n pass": 0,
"from typing import TYPE_CHECKING\nif TYPE_CHECKING:\n from app.downloads.models import Job": 0,
"import typing\nif typing.TYPE_CHECKING:\n from app.models import DownloadJob": 0,
"if TYPE_CHECKING:\n pass\nfrom app.downloads.models import DownloadJob": 1,
@@ -333,11 +329,23 @@ def test_the_detector_sees_each_kind_of_database_work():
# it commits only where a job owns its session.
# Each unit's code outside the HTTP layer: its package, and the entry point that runs its jobs.
UNIT_PACKAGES = {"capture": ("downloads/", "worker.py")}
UNIT_PACKAGES = {
"capture": ("downloads/", "worker.py"),
# Home (`discover/`) is Reader's landing page; a playlist's YouTube sync and the tag suggestions
# work on Reader's tables alone.
"reader": (
"reader/",
"discover/",
"feedquery.py",
"sync/playlists.py",
"sync/playlist_merge.py",
"sync/tagsuggest.py",
),
}
# The units fenced so far; the rest join with their sprint.
FENCED = {"capture"}
FENCED = {"capture", "reader"}
# Whose tables a unit's own code may READ. Core code reads a fenced unit's tables through its queries.
READS = {"capture": {"catalog", "plex"}}
READS = {"capture": {"catalog", "plex"}, "reader": {"catalog", "plex"}}
# The columns that say whose a row is.
OWNER_COLUMNS = {"user_id", "to_user_id", "from_user_id", "created_by"}
# Where a fenced unit's code may commit: the jobs that own their session (the worker and what it
@@ -353,7 +361,17 @@ UNIT_COMMITS = {
"downloads/jobdb.py",
"downloads/pipeline.py",
"downloads/upload.py:ingest_upload",
}
},
# A playlist's YouTube sync runs over many lists and many YouTube calls: it commits what each
# step settled before the next (a list created on YouTube, one list mirrored), so a later
# failure neither forgets a write YouTube already holds nor drops the lists already done. Its
# claim is a lock held on a connection of its own.
"reader": {
"sync/playlists.py:_advisory_claim",
"sync/playlists.py:pull_user_playlists",
"sync/playlists.py:merge_playlist",
"sync/playlists.py:repull_playlist",
},
}
SQL_WRITES = {"insert", "update", "delete", "pg_insert"}