diff --git a/core/jellyfin_client.py b/core/jellyfin_client.py index 28adba99..ee564f6c 100644 --- a/core/jellyfin_client.py +++ b/core/jellyfin_client.py @@ -146,7 +146,10 @@ class JellyfinTrack: return self._client.get_album_by_id(self._album_id) return None -class JellyfinClient: +from core.media_server.contract import MediaServerClient + + +class JellyfinClient(MediaServerClient): def __init__(self): self.base_url: Optional[str] = None self.api_key: Optional[str] = None diff --git a/core/media_server/__init__.py b/core/media_server/__init__.py index c1f5012b..3c125ede 100644 --- a/core/media_server/__init__.py +++ b/core/media_server/__init__.py @@ -15,15 +15,19 @@ API, SoulSync's filesystem walk). The engine just routes by See ``docs/media-server-engine-refactor-plan.md`` for the full phased plan. + +Note: only ``MediaServerClient`` is re-exported here. The engine + +registry are NOT — importing the registry triggers eager imports +of every per-server client class, and those clients now inherit +``MediaServerClient`` (Cin-1), so re-exporting them here would +form a circular import the moment a client tried to resolve its +base class. Import them directly from their submodules: + from core.media_server.engine import MediaServerEngine + from core.media_server.registry import build_default_registry """ from core.media_server.contract import MediaServerClient -from core.media_server.engine import MediaServerEngine -from core.media_server.registry import MediaServerRegistry, build_default_registry __all__ = [ "MediaServerClient", - "MediaServerEngine", - "MediaServerRegistry", - "build_default_registry", ] diff --git a/core/media_server/engine.py b/core/media_server/engine.py index a5d043bb..c1cb202b 100644 --- a/core/media_server/engine.py +++ b/core/media_server/engine.py @@ -234,3 +234,73 @@ class MediaServerEngine: ) return [] return [] + + # ------------------------------------------------------------------ + # Generic accessors — replace per-server attribute reaches in + # callers (Cin's standard from the download refactor). + # ------------------------------------------------------------------ + + def configured_clients(self) -> Dict[str, MediaServerClient]: + """Return ``{name: client}`` for every server that's both + registered AND reports ``is_connected() == True``. Replaces + the legacy per-server `if X and X.is_connected(): ...` + chains in web_server.py.""" + result: Dict[str, MediaServerClient] = {} + for name, client in self.registry.all_clients(): + try: + if not hasattr(client, 'is_connected') or client.is_connected(): + result[name] = client + except Exception as exc: + logger.debug("%s is_connected raised in configured_clients: %s", name, exc) + return result + + def reload_config(self, name: Optional[str] = None) -> bool: + """Reload config on a single server (or every server when + ``name`` is None). Generic dispatch — caller passes the name + instead of reaching for ``plex_client.reload_config()`` + / ``jellyfin_client.reload_config()`` directly. Servers + without a ``reload_config`` method are silently skipped. + """ + names = [name] if name else list(self.registry.names()) + ok = True + for n in names: + client = self.client(n) + if client is None or not hasattr(client, 'reload_config'): + continue + try: + client.reload_config() + except Exception as exc: + logger.warning("%s reload_config failed: %s", n, exc) + ok = False + return ok + + +# --------------------------------------------------------------------------- +# Singleton accessor — mirrors the get_metadata_engine() / +# get_download_orchestrator() pattern so callers that don't need a +# custom registry use this instead of instantiating MediaServerEngine +# directly. web_server.py constructs the singleton at startup and +# installs it via ``set_media_server_engine`` so the factory + the +# global handle share state. +# --------------------------------------------------------------------------- + +_default_engine: Optional['MediaServerEngine'] = None + + +def get_media_server_engine() -> 'MediaServerEngine': + """Return (lazily creating) the process-wide MediaServerEngine + singleton. Mirrors the ``get_metadata_engine()`` / + ``get_download_orchestrator()`` shape.""" + global _default_engine + if _default_engine is None: + _default_engine = MediaServerEngine() + return _default_engine + + +def set_media_server_engine(engine: Optional['MediaServerEngine']) -> None: + """Set the process-wide singleton. Used by web_server.py at boot + to install the engine it constructs (with the pre-built per-client + instances) as the default for callers reaching via + ``get_media_server_engine()``.""" + global _default_engine + _default_engine = engine diff --git a/core/navidrome_client.py b/core/navidrome_client.py index 2cc010a7..0e396fe4 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -150,7 +150,10 @@ class NavidromeTrack: return self._client.get_album_by_id(self._album_id) return None -class NavidromeClient: +from core.media_server.contract import MediaServerClient + + +class NavidromeClient(MediaServerClient): def __init__(self): self.base_url: Optional[str] = None self.username: Optional[str] = None diff --git a/core/plex_client.py b/core/plex_client.py index 43bbf8a8..f9c1a24d 100644 --- a/core/plex_client.py +++ b/core/plex_client.py @@ -74,7 +74,10 @@ class PlexPlaylistInfo: tracks=tracks ) -class PlexClient: +from core.media_server.contract import MediaServerClient + + +class PlexClient(MediaServerClient): def __init__(self): self.server: Optional[PlexServer] = None self.music_library: Optional[MusicSection] = None diff --git a/core/soulsync_client.py b/core/soulsync_client.py index a7b3ff98..41e6bb7d 100644 --- a/core/soulsync_client.py +++ b/core/soulsync_client.py @@ -190,7 +190,10 @@ class SoulSyncArtist: return self._albums -class SoulSyncClient: +from core.media_server.contract import MediaServerClient + + +class SoulSyncClient(MediaServerClient): """Filesystem-based media server client for standalone SoulSync operation. Scans the Transfer folder recursively, reads audio file tags, and diff --git a/tests/media_server/test_conformance.py b/tests/media_server/test_conformance.py index fab74111..dcf789c6 100644 --- a/tests/media_server/test_conformance.py +++ b/tests/media_server/test_conformance.py @@ -32,7 +32,7 @@ def _import_server_classes(): 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 + from core.media_server.registry import build_default_registry registry = build_default_registry() expected = {'plex', 'jellyfin', 'navidrome', 'soulsync'} @@ -52,3 +52,21 @@ def test_server_class_has_all_required_methods(server_name): assert not missing, ( f"{server_name} ({cls.__name__}) missing required methods: {missing}" ) + + +@pytest.mark.parametrize('server_name', ['plex', 'jellyfin', 'navidrome', 'soulsync']) +def test_server_class_explicitly_inherits_contract(server_name): + """Per Cin's standard from the download refactor: clients must + explicitly inherit ``MediaServerClient`` so the contract conformance + is obvious from reading the class declaration. Structural + typing alone (which would still pass `hasattr` checks) leaves + the contract invisible to anyone reading the code — drift in a + future client class wouldn't fail at the contract boundary.""" + from core.media_server.contract import MediaServerClient + + classes = _import_server_classes() + cls = classes[server_name] + assert issubclass(cls, MediaServerClient), ( + f"{cls.__name__} must explicitly inherit MediaServerClient — " + f"structural typing isn't enough" + ) diff --git a/tests/media_server/test_engine.py b/tests/media_server/test_engine.py index f4650a28..6205e306 100644 --- a/tests/media_server/test_engine.py +++ b/tests/media_server/test_engine.py @@ -6,8 +6,8 @@ from unittest.mock import MagicMock import pytest -from core.media_server import MediaServerEngine, MediaServerRegistry -from core.media_server.registry import ServerSpec +from core.media_server.engine import MediaServerEngine +from core.media_server.registry import MediaServerRegistry, ServerSpec class _FakeClient: @@ -190,3 +190,107 @@ def test_is_library_scanning_returns_false_when_client_lacks_method(make_engine) def test_get_library_stats_returns_empty_dict_when_client_lacks_method(make_engine): engine = make_engine({'soulsync': _MinimalClient()}, active='soulsync') assert engine.get_library_stats() == {} + + +# --------------------------------------------------------------------------- +# Generic accessors (Cin's standard from the download refactor) +# --------------------------------------------------------------------------- + + +def test_configured_clients_only_returns_connected_servers(make_engine): + """Replaces the legacy per-server `if X and X.is_connected(): ...` + chains in web_server.py. Single call returns the dict.""" + plex = _FakeClient('plex', connected=True) + jelly = _FakeClient('jellyfin', connected=False) + soulsync = _FakeClient('soulsync', connected=True) + engine = make_engine( + {'plex': plex, 'jellyfin': jelly, 'soulsync': soulsync}, + active='plex', + ) + result = engine.configured_clients() + assert set(result.keys()) == {'plex', 'soulsync'} + assert result['plex'] is plex + assert result['soulsync'] is soulsync + + +def test_configured_clients_skips_clients_whose_is_connected_raises(make_engine): + """Defensive: a single broken is_connected() must not crash the + iteration. Healthy clients still come back.""" + healthy = _FakeClient('plex', connected=True) + broken = _FakeClient('jellyfin') + broken.is_connected = MagicMock(side_effect=RuntimeError("boom")) + engine = make_engine({'plex': healthy, 'jellyfin': broken}, active='plex') + result = engine.configured_clients() + assert 'plex' in result + assert 'jellyfin' not in result + + +def test_reload_config_dispatches_to_named_server(make_engine): + """Generic dispatch — caller passes server name instead of + reaching for plex_client.reload_config() directly.""" + + class _ReloadablePlex(_FakeClient): + def __init__(self): + super().__init__('plex') + self.reload_called = False + + def reload_config(self): + self.reload_called = True + + plex = _ReloadablePlex() + soulsync = _FakeClient('soulsync') # No reload_config method + engine = make_engine({'plex': plex, 'soulsync': soulsync}, active='plex') + + assert engine.reload_config('plex') is True + assert plex.reload_called is True + + +def test_reload_config_skips_clients_without_method(make_engine): + """Servers that don't expose reload_config are skipped silently + (return True).""" + soulsync = _FakeClient('soulsync') + engine = make_engine({'soulsync': soulsync}, active='soulsync') + assert engine.reload_config('soulsync') is True + + +def test_reload_config_with_no_args_reloads_every_server(make_engine): + """When called with no name, hits every registered server that + exposes reload_config.""" + + class _ReloadableClient(_FakeClient): + def __init__(self, name): + super().__init__(name) + self.reload_called = False + + def reload_config(self): + self.reload_called = True + + plex = _ReloadableClient('plex') + jelly = _ReloadableClient('jellyfin') + engine = make_engine({'plex': plex, 'jellyfin': jelly}, active='plex') + + engine.reload_config() + assert plex.reload_called is True + assert jelly.reload_called is True + + +# --------------------------------------------------------------------------- +# Singleton factory (matches get_metadata_engine() / get_download_orchestrator()) +# --------------------------------------------------------------------------- + + +def test_get_media_server_engine_returns_set_singleton(make_engine): + """When set_media_server_engine has been called (web_server.py + does this at boot), get_media_server_engine returns the installed + instance instead of building a fresh one with the default registry.""" + from core.media_server.engine import ( + get_media_server_engine, + set_media_server_engine, + ) + + engine = make_engine({'plex': _FakeClient('plex')}, active='plex') + set_media_server_engine(engine) + try: + assert get_media_server_engine() is engine + finally: + set_media_server_engine(None) diff --git a/web_server.py b/web_server.py index 17b0bd0b..d913d0b3 100644 --- a/web_server.py +++ b/web_server.py @@ -608,13 +608,18 @@ except Exception as e: # ``engine.method()`` dispatch in place of the historic # ``if active_server == 'plex' / 'jellyfin' / ...`` chains. try: - from core.media_server import MediaServerEngine + from core.media_server.engine import MediaServerEngine, set_media_server_engine media_server_engine = MediaServerEngine(clients={ 'plex': plex_client, 'jellyfin': jellyfin_client, 'navidrome': navidrome_client, 'soulsync': soulsync_library_client, }) + # Install as process-wide singleton so callers reaching via + # get_media_server_engine() see the same instance web_server.py + # constructs at boot. Matches the metadata + download engine + # patterns. + set_media_server_engine(media_server_engine) logger.info(" Media server engine initialized") except Exception as e: logger.error(f" Media server engine failed to initialize: {e}")