diff --git a/core/media_server/__init__.py b/core/media_server/__init__.py new file mode 100644 index 00000000..422fba04 --- /dev/null +++ b/core/media_server/__init__.py @@ -0,0 +1,23 @@ +"""Media server engine — central dispatch for the per-server clients +(Plex, Jellyfin, Navidrome, SoulSync standalone). + +Companion to the download engine refactor — same architectural +shape applied to the read-side of the library. The orchestrator +historically had 33+ ``if active_server == 'plex' / 'jellyfin' / +...`` dispatch sites in web_server.py. This package replaces those +with a single ``MediaServerEngine.method()`` call per operation. + +Per-server clients keep their protocol-specific work (Plex's +PlexAPI SDK, Jellyfin's REST endpoints, Navidrome's OpenSubsonic +API, SoulSync's filesystem walk). The engine just routes by +``active_server`` config + provides a uniform shape for the calls +``web_server.py`` makes generically. + +See ``docs/media-server-engine-refactor-plan.md`` for the full +phased plan. +""" + +from core.media_server.contract import MediaServerClient +from core.media_server.registry import MediaServerRegistry, build_default_registry + +__all__ = ["MediaServerClient", "MediaServerRegistry", "build_default_registry"] diff --git a/core/media_server/contract.py b/core/media_server/contract.py new file mode 100644 index 00000000..986b4108 --- /dev/null +++ b/core/media_server/contract.py @@ -0,0 +1,148 @@ +"""Canonical contract for media server clients. + +Narrow on purpose. Only declares the methods the orchestrator +dispatches generically across all servers. Server-specific extras +(Plex's `set_music_library_by_name`, Jellyfin's user picker, +Navidrome's music folder filter, SoulSync's filesystem rescan) +stay on the underlying client and are accessed through the +registry's typed accessor — same pattern as the download +plugin contract. + +Every required method must be implemented by every registered +client. Optional methods have default no-op implementations so +servers without that capability (e.g. Navidrome's metadata +writeback stubs, SoulSync's playlist sync N/A) don't have to +declare a no-op explicitly. + +The contract is a Protocol (structural typing) rather than an +ABC — existing PlexClient / JellyfinClient / NavidromeClient / +SoulSyncClient grew the same shape independently because every +caller needed the same calls. This file just makes the implicit +contract explicit. +""" + +from __future__ import annotations + +from typing import Any, Dict, List, Optional, Protocol, runtime_checkable + + +@runtime_checkable +class MediaServerClient(Protocol): + """Structural contract every media server client must satisfy. + + ``runtime_checkable`` lets ``isinstance(client, MediaServerClient)`` + work for the conformance test, but it ONLY checks method names — + not signatures. The conformance test in + ``tests/media_server/test_conformance.py`` does the deeper + signature check. + """ + + # ------------------------------------------------------------------ + # Connection / lifecycle + # ------------------------------------------------------------------ + + def is_connected(self) -> bool: + """Cheap probe — does the client have a live connection / + token / session right now? Used by the dashboard status + indicators + endpoint guards.""" + ... + + def ensure_connection(self) -> bool: + """Re-auth or reconnect if needed. May make a network call. + Returns True if connection is usable after the call.""" + ... + + # ------------------------------------------------------------------ + # Library reads (required — every server must support these) + # ------------------------------------------------------------------ + + def get_all_artists(self) -> List[Any]: + """Return every artist the server knows about. Each item is + a server-specific wrapper object (PlexArtist, JellyfinArtist, + NavidromeArtist, SoulSyncArtist) — caller treats them + opaquely.""" + ... + + def get_all_album_ids(self) -> set: + """Return the set of every album ID in the library. ID + format is server-native — caller doesn't introspect.""" + ... + + def search_tracks(self, title: str, artist: str, limit: int = 15) -> List[Any]: + """Search the server's library for tracks matching the title + + artist. Used by playlist sync, download matching, and the + general search UI. Each item is a server-specific TrackInfo + wrapper.""" + ... + + def get_recently_added_albums(self, max_results: int = 400) -> List[Any]: + """Recently-added view — used by the Discover page + + watchlist scanner. Sorted by add timestamp descending.""" + ... + + # ------------------------------------------------------------------ + # Library writes — scan triggers + # ------------------------------------------------------------------ + + def trigger_library_scan(self) -> bool: + """Ask the server to scan its music library. Some servers + (SoulSync standalone) walk the filesystem themselves; some + (Plex / Jellyfin / Navidrome) hit a server-side scan API.""" + ... + + def is_library_scanning(self) -> bool: + """True if a scan is currently running. Polled by the + scan-progress UI.""" + ... + + def get_library_stats(self) -> Dict[str, int]: + """Counts of artists / albums / tracks. Used by the + dashboard system-stats card.""" + ... + + +# --------------------------------------------------------------------------- +# Required + optional method names — used by the conformance tests to +# check structural conformance without the proxy weight of dataclasses. +# --------------------------------------------------------------------------- + +# Conservative requirement set — only methods every one of the four +# servers actually implements today. Audited by the conformance test. +# Other methods (search_tracks, trigger_library_scan, etc.) exist on +# most servers but not all (e.g. SoulSync has no library scan API +# because it walks the filesystem directly). Phase B's engine +# adapters handle those with per-server fallback rather than forcing +# every client to declare a no-op stub. +REQUIRED_METHODS = { + 'is_connected', + 'ensure_connection', + 'get_all_artists', + 'get_all_album_ids', +} + +# Methods declared on the protocol but NOT enforced — the +# conformance test does NOT fail if a client lacks one. Engine +# adapters route around the gaps. Listed here so future contributors +# know what the engine expects to find when present. +OPTIONAL_METHODS = { + 'search_tracks', + 'get_recently_added_albums', + 'trigger_library_scan', + 'is_library_scanning', + 'get_library_stats', + # Playlist sync (Plex / Jellyfin / Navidrome implement; SoulSync no-op) + 'create_playlist', + 'update_playlist', + 'copy_playlist', + 'get_all_playlists', + 'get_playlist_by_name', + # Analytics + 'get_play_history', + 'get_track_play_counts', + # Metadata writeback (Plex full support; Jellyfin partial; Navidrome stubs; SoulSync N/A) + 'update_artist_genres', + 'update_artist_poster', + 'update_album_poster', + 'update_artist_biography', + 'update_track_metadata', +} diff --git a/core/media_server/registry.py b/core/media_server/registry.py new file mode 100644 index 00000000..173c7a72 --- /dev/null +++ b/core/media_server/registry.py @@ -0,0 +1,127 @@ +"""Media server plugin registry. + +Single source of truth for which servers exist, what their canonical +names are, and which client class implements each. Replaces the +historic web_server.py pattern of holding 4 separate client globals ++ 33 hand-maintained ``if active_server == 'plex' / 'jellyfin' / ...`` +dispatch sites. + +Adding a new server (e.g. Subsonic, Emby) becomes one ``register`` +call here + the new client class. Web server dispatch stays put. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Callable, Dict, Iterator, List, Optional, Tuple + +from utils.logging_config import get_logger + +from core.media_server.contract import MediaServerClient + +# Eager imports for the same import-order reason the download plugin +# registry uses them (some integration tests inject mock modules into +# sys.modules at collection time; lazy import would bind to the mock). +from core.jellyfin_client import JellyfinClient +from core.navidrome_client import NavidromeClient +from core.plex_client import PlexClient +from core.soulsync_client import SoulSyncClient + +logger = get_logger("media_server.registry") + + +@dataclass(frozen=True) +class ServerSpec: + """Static descriptor for a media server. ``factory`` is the + zero-arg callable that builds the client (each server has its + own setup chain — Plex pulls token from config, Jellyfin reads + user_id, etc.).""" + + name: str + factory: Callable[[], MediaServerClient] + display_name: str + aliases: Tuple[str, ...] = field(default_factory=tuple) + + +class MediaServerRegistry: + """Holds the live client instances + name → instance lookup. + + Two-phase construction (mirrors the download plugin registry): + 1. Specs registered cheaply (just stores callable refs). + 2. ``initialize()`` calls each factory once. Failures captured + in ``init_failures`` so one broken server doesn't take down + the orchestrator. + """ + + def __init__(self) -> None: + self._specs: Dict[str, ServerSpec] = {} + self._instances: Dict[str, Optional[MediaServerClient]] = {} + self._init_failures: List[str] = [] + + def register(self, spec: ServerSpec) -> None: + if spec.name in self._specs: + raise ValueError(f"Server already registered: {spec.name}") + self._specs[spec.name] = spec + + def initialize(self) -> None: + for spec in self._specs.values(): + try: + instance = spec.factory() + self._instances[spec.name] = instance + except Exception as exc: + logger.error("%s media server client failed to initialize: %s", spec.display_name, exc) + self._init_failures.append(spec.display_name) + self._instances[spec.name] = None + + @property + def init_failures(self) -> List[str]: + return list(self._init_failures) + + def get(self, name: str) -> Optional[MediaServerClient]: + if not name: + return None + if name in self._instances: + return self._instances[name] + for spec in self._specs.values(): + if name in spec.aliases: + return self._instances.get(spec.name) + return None + + def get_spec(self, name: str) -> Optional[ServerSpec]: + if name in self._specs: + return self._specs[name] + for spec in self._specs.values(): + if name in spec.aliases: + return spec + return None + + def display_name(self, name: str) -> str: + spec = self.get_spec(name) + return spec.display_name if spec else name + + def names(self) -> List[str]: + return list(self._specs.keys()) + + def all_clients(self) -> Iterator[Tuple[str, MediaServerClient]]: + """Yield (name, client) for every successfully-initialized + server. Used by cross-server operations.""" + for name, instance in self._instances.items(): + if instance is not None: + yield name, instance + + +def build_default_registry() -> MediaServerRegistry: + """Construct the registry with SoulSync's four built-in media + servers. Called once during MediaServerEngine construction. + + Adding a server (e.g. Subsonic, Emby) = one ``register`` call + here + the new client class. No dispatch-site changes required. + """ + registry = MediaServerRegistry() + + registry.register(ServerSpec(name='plex', factory=PlexClient, display_name='Plex')) + registry.register(ServerSpec(name='jellyfin', factory=JellyfinClient, display_name='Jellyfin')) + registry.register(ServerSpec(name='navidrome', factory=NavidromeClient, display_name='Navidrome')) + registry.register(ServerSpec(name='soulsync', factory=SoulSyncClient, display_name='SoulSync Library')) + + return registry diff --git a/docs/media-server-engine-refactor-plan.md b/docs/media-server-engine-refactor-plan.md new file mode 100644 index 00000000..9f271f66 --- /dev/null +++ b/docs/media-server-engine-refactor-plan.md @@ -0,0 +1,159 @@ +# Media Server Engine Refactor Plan + +## Goal + +Same playbook as the download engine refactor, applied to media servers. Replace 33 `if active_server == 'plex' / 'jellyfin' / 'navidrome' / 'soulsync'` dispatch sites in web_server.py with a central `MediaServerEngine` that owns server selection + cross-server query dispatch. Each per-server client stays as-is for its protocol-specific work (Plex API SDK, Jellyfin REST, Navidrome OpenSubsonic, SoulSync filesystem walk). + +**The smell:** Adding a 5th server (recent SoulSync standalone showed how) requires hunting through 33 web_server.py dispatch sites + DatabaseUpdateWorker branching + UI sync-button visibility hacks. Plex assumptions had leaked into nominally generic code paths. + +## Architecture target + +``` +┌───────────┐ ┌──────────────────┐ +│ feature │ ─search/scan/sync─▶ ┌──│ MediaServerEngine │ ─▶ Plex (PlexAPI SDK) +│ │ ◀── results ────── │ │ ◆ active selection│ ─▶ Jellyfin (REST) +└───────────┘ │ │ ◆ dispatch │ ─▶ Navidrome (Subsonic) + │ │ ◆ shared shape │ ─▶ SoulSync (filesystem) + │ └──────────────────┘ + │ (no need for per-server thread pool — server APIs + │ are sync-call shaped, not stream-shaped like downloads) +``` + +## What clients keep (per-server, legitimately) + +- **Auth** — Plex token, Jellyfin API key + user ID + library ID, Navidrome user/pass + salt, SoulSync filesystem path +- **Wire protocol** — PlexAPI SDK objects vs Jellyfin REST `/Items` vs Navidrome XML/JSON Subsonic vs filesystem walk +- **ID schemes** — Plex ratingKey (int), Jellyfin GUID (hex), Navidrome string, SoulSync MD5 +- **Connection state** — each client owns its session / token / connection object +- **Cache strategy** — Jellyfin's aggressive pre-cache, Navidrome's folder filter, SoulSync's 5-min TTL +- **Wrapper objects** — `JellyfinTrack`, `NavidromeTrack`, etc. stay in client modules + +## What moves into the engine + +| Today (web_server.py duplicated) | Tomorrow (MediaServerEngine, single dispatch site) | +|---|---| +| `if active_server == 'plex': plex_client.X() elif 'jellyfin': jellyfin_client.X() ...` | `engine.X()` — engine reads `active_server` once, routes | +| Status check 4-way branch | `engine.is_connected()` | +| Library scan trigger 3-way | `engine.trigger_library_scan()` | +| Search dispatch in 3 sites | `engine.search_tracks(title, artist)` | +| Play history 4-way | `engine.get_play_history()` | +| Metadata writeback 4-way (genre / poster / bio) | `engine.update_artist_genres()` etc. — engine routes, plugin no-ops where unsupported | +| `DatabaseUpdateWorker.server_type` branching | `engine` injected, no per-server branching | + +## Plugin contract (ABC, narrower than the download contract) + +```python +class MediaServerClient(ABC): + # Connection + @abstractmethod + def is_connected(self) -> bool: ... + @abstractmethod + def ensure_connection(self) -> bool: ... + + # Library reads (required) + @abstractmethod + def get_all_artists(self): ... + @abstractmethod + def get_all_album_ids(self): ... + @abstractmethod + def search_tracks(self, title, artist, limit=15): ... + @abstractmethod + def get_recently_added_albums(self, max_results=400): ... + + # Library writes (required for the scan endpoints — may no-op for SoulSync) + @abstractmethod + def trigger_library_scan(self) -> bool: ... + @abstractmethod + def is_library_scanning(self) -> bool: ... + + # Playlist sync (optional — Navidrome stub returns True) + def create_playlist(self, name, tracks) -> bool: return True + def update_playlist(self, name, tracks) -> bool: return True + def copy_playlist(self, source, dest_name) -> bool: return True + + # Analytics (optional) + def get_play_history(self, limit=500) -> list: return [] + def get_track_play_counts(self) -> dict: return {} + + # Metadata writeback (optional — clients no-op where API doesn't support it) + def update_artist_genres(self, artist, genres) -> bool: return True + def update_artist_poster(self, artist, image_data) -> bool: return True + def update_album_poster(self, album, image_data) -> bool: return True + def update_artist_biography(self, artist) -> bool: return True +``` + +## Phased commit plan + +### Phase 0 — Foundation +- **0.1** Plugin contract (`core/media_server/contract.py`) + ABC +- **0.2** Registry + dispatch helpers (`core/media_server/registry.py`) +- **0.3** Conformance tests — every registered client satisfies the ABC + +### Phase A — Behavior pinning +- **A1** Pin Plex client surface (status check, search, scan trigger, metadata writeback shape) +- **A2** Pin Jellyfin client surface +- **A3** Pin Navidrome client surface +- **A4** Pin SoulSync standalone client surface + +### Phase B — Engine skeleton +- **B1** `MediaServerEngine` skeleton — holds clients, exposes `active_server` selection +- **B2** Engine dispatch methods (`is_connected`, `search_tracks`, `trigger_library_scan`, `is_library_scanning`, etc.) +- **B3** Engine-level conformance tests + +### Phase C — Migrate web_server.py dispatch sites +Each commit migrates a logical cluster (status, scan, playlist, metadata, history) so the diff per commit is small + reviewable. + +- **C1** Status / connection-check sites +- **C2** Library scan trigger + scanning-state polling +- **C3** Search dispatch (3 sites) +- **C4** Playlist sync / apply / suggest +- **C5** Play history + track play counts +- **C6** Metadata writeback (genres / posters / bio) + +### Phase D — DatabaseUpdateWorker +- **D1** Inject engine into `DatabaseUpdateWorker` +- **D2** Strip `self.server_type` branching (use `engine.get_active_client_type()` where shape genuinely differs) + +### Phase E — Cleanup + ship +- **E1** Drop unused imports / dead code +- **E2** WHATS_NEW + PR description + +**Total estimated:** 15-18 commits. ~600 LOC moved, ~200 LOC deleted. + +## Risk profile + +**Low:** +- Phase 0 (pure additive — new contract, no existing code touched) +- Phase A (tests only) +- Phase B (engine exists but nothing routes through it yet) + +**Medium:** +- Phase C (each commit changes 5-10 dispatch sites in web_server.py — cross-cutting) +- Phase D (DatabaseUpdateWorker is a hot path during library refresh) + +**Mitigation:** +- Phase A pinning catches per-client behavior drift +- Phase B engine tested in isolation before C wires it in +- Phase C commits are small + reversible (one cluster per commit) +- Suite green at every commit + +## What's NOT in this PR + +- Unifying track ID schemes (Plex int vs Jellyfin GUID vs Navidrome string vs SoulSync MD5) — separate data model refactor +- Merging Plex/Jellyfin/Navidrome wrapper objects (each tied to its API) +- Extracting common metadata writeback logic — too server-specific +- Adding new server types (Subsonic, MusicBrainz local) — out of scope; this PR makes them easier later + +## Coordination with other work + +- Independent of the download engine refactor (PR #494) — different subsystem +- Cin's metadata engine work also independent +- No collisions expected + +## Compatibility commitments + +- All existing config keys preserved +- All existing API endpoints preserved +- Plex/Jellyfin/Navidrome/SoulSync clients keep their public methods (engine wraps, doesn't replace) +- Active-server config (`server.active`) unchanged +- `DatabaseUpdateWorker.server_type` available for external readers (deprecated but not removed) diff --git a/tests/media_server/__init__.py b/tests/media_server/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/media_server/test_conformance.py b/tests/media_server/test_conformance.py new file mode 100644 index 00000000..fab74111 --- /dev/null +++ b/tests/media_server/test_conformance.py @@ -0,0 +1,54 @@ +"""Pin the structural conformance of every media server client to +``MediaServerClient``. Mirrors the download plugin conformance test +shape — class-level checks (not instance-level) so importing the +test doesn't drag every server's heavy auth init into the test +collection phase. +""" + +from __future__ import annotations + +import pytest + +from core.media_server.contract import REQUIRED_METHODS + + +def _import_server_classes(): + """Import every server client class lazily inside tests so + auth-init-heavy modules (Plex, Jellyfin) aren't imported at test + collection.""" + from core.jellyfin_client import JellyfinClient + from core.navidrome_client import NavidromeClient + from core.plex_client import PlexClient + from core.soulsync_client import SoulSyncClient + + return { + 'plex': PlexClient, + 'jellyfin': JellyfinClient, + 'navidrome': NavidromeClient, + 'soulsync': SoulSyncClient, + } + + +def test_default_registry_registers_all_four_servers(): + """Smoke check that the foundation registry knows about every + server SoulSync historically dispatched to.""" + from core.media_server import build_default_registry + + registry = build_default_registry() + expected = {'plex', 'jellyfin', 'navidrome', 'soulsync'} + assert set(registry.names()) == expected + + +@pytest.mark.parametrize('server_name', ['plex', 'jellyfin', 'navidrome', 'soulsync']) +def test_server_class_has_all_required_methods(server_name): + """Every registered server class exposes every required protocol + method by name. Diagnostic-friendly: tells you WHICH method is + missing when a new server is added without all the required + methods.""" + classes = _import_server_classes() + cls = classes[server_name] + + missing = [m for m in REQUIRED_METHODS if not hasattr(cls, m)] + assert not missing, ( + f"{server_name} ({cls.__name__}) missing required methods: {missing}" + )