danvics/soulsync
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

MS pre-review polish — encapsulation + visibility + tests

Browse source 
Five tightening passes anticipating Cin / JohnBaumb's review nits:

(1) Engine no longer reaches into ``registry._instances`` private
attr. New public ``MediaServerRegistry.set_instance(name, client)``
method — engine constructor calls it for the ``clients=`` pre-built
case so internal storage stays encapsulated.

(2) Engine module docstring no longer overclaims. Originally said it
"Replaces the historic 33+ if/elif chains" — but only the four
uniform-shape ``is_connected`` chains were collapsed. The 19 chains
that do server-specific work (Plex raw API vs Jellyfin / Navidrome
client methods returning different shapes) stay explicit per the
"lift what's truly shared" standard. Docstring rewritten to say
exactly that.

(3) Per-method exception swallows upgraded from ``logger.debug`` to
``logger.warning``. Returning safe defaults stays the right behavior
for a read-side engine (Plex offline shouldn't crash the app), but
silent debug-level swallowing made debugging hard — JohnBaumb pushed
the download engine to surface real errors. Same treatment here:
default still safe, but the warning tells you Plex is down.

(4) ``_safe_init_media_client`` in web_server.py now logs the
exception type + traceback. Broad ``except Exception`` is still
intentional (any failure means that one server can't be used; the
others stay up) but the boot log now distinguishes config errors
(ConnectionError, AuthenticationError) from import / dependency
failures.

(5) Two new tests pin the encapsulation + fallback contracts:
- ``test_engine_with_empty_clients_dict_is_safe_to_use`` — empty
  engine returns safe defaults on every method, doesn't raise.
- ``test_engine_uses_registry_set_instance_not_private_attr`` — spy
  on registry.set_instance verifies engine uses the public method.
This commit is contained in:
Broque Thomas 2026-05-05 21:04:14 -07:00
parent f230c93890
commit 860f9a0a8c
4 changed files with 115 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

53
core/media_server/engine.py
View file

@ -1,9 +1,10 @@
"""MediaServerEngine — central dispatch for media server operations.
Replaces the historic 33+ ``if active_server == 'plex' / 'jellyfin' /
'navidrome' / 'soulsync'`` chains in ``web_server.py``. Each
operation web_server.py used to dispatch by hand becomes a single
``engine.method()`` call here that:
Replaces the *uniform-shape* dispatch chains in web_server.py
(``is_connected``, ``get_all_artists``, etc. — anything where every
server returns the same shape and the only branching was on
``active_server == X``). Each such operation is now one
``engine.method()`` call that:
1. Reads the ``server.active`` config to find the current target.
2. Looks up the registered client.
@ -11,14 +12,17 @@ operation web_server.py used to dispatch by hand becomes a single
for methods that don't exist on every client — e.g. SoulSync
has no library-scan API).
Per-server client objects stay accessible via ``engine.client(name)``
so any caller that needs a Plex-specific method (e.g.
``set_music_library_by_name`` for the settings page) keeps working
through ``engine.client('plex').set_music_library_by_name(...)``.
Server-specific dispatch sites (Plex's raw playlist API, Jellyfin /
Navidrome client methods returning different shapes) stay explicit
in web_server.py per the "lift what's truly shared" standard. They
reach individual clients via ``engine.client(name)`` rather than
the per-server globals — same generic-accessor pattern as the
download orchestrator.
Engine itself is constructed once during web_server.py init and
held as a module-level singleton, mirroring the existing pattern
for the per-server client globals.
held as a process-wide singleton via
``set_media_server_engine`` / ``get_media_server_engine``, mirroring
the metadata + download engine factory shape.
"""
from __future__ import annotations
@ -69,15 +73,16 @@ class MediaServerEngine:
if clients is not None:
# Wrap pre-built instances (production case from web_server.py
# init). Skip registry.initialize() — we already have the
# instances, just stash them in the registry's slots so
# registry.get(name) works.
# instances, hand them off via the registry's public
# set_instance(name, client) method so internal storage stays
# encapsulated.
for name, client in clients.items():
self.registry._instances[name] = client
self.registry.set_instance(name, client)
# Mark any registered-but-not-supplied as failed init so
# active_client() returns None for them.
for name in self.registry._specs:
if name not in self.registry._instances:
self.registry._instances[name] = None
for name in self.registry.names():
if self.registry.get(name) is None and name not in clients:
self.registry.set_instance(name, None)
else:
self.registry.initialize()
@ -119,7 +124,7 @@ class MediaServerEngine:
try:
return client.is_connected()
except Exception as exc:
logger.debug("%s is_connected raised: %s", self.active_server, exc)
logger.warning("%s is_connected raised: %s", self.active_server, exc)
return False
def ensure_connection(self) -> bool:
@ -131,7 +136,7 @@ class MediaServerEngine:
try:
return client.ensure_connection()
except Exception as exc:
logger.debug("%s ensure_connection raised: %s", self.active_server, exc)
logger.warning("%s ensure_connection raised: %s", self.active_server, exc)
return False
def get_all_artists(self) -> List[Any]:
@ -143,7 +148,7 @@ class MediaServerEngine:
try:
return client.get_all_artists()
except Exception as exc:
logger.debug("%s get_all_artists raised: %s", self.active_server, exc)
logger.warning("%s get_all_artists raised: %s", self.active_server, exc)
return []
def get_all_album_ids(self) -> set:
@ -155,7 +160,7 @@ class MediaServerEngine:
try:
return client.get_all_album_ids()
except Exception as exc:
logger.debug("%s get_all_album_ids raised: %s", self.active_server, exc)
logger.warning("%s get_all_album_ids raised: %s", self.active_server, exc)
return set()
# ------------------------------------------------------------------
@ -174,7 +179,7 @@ class MediaServerEngine:
try:
return client.search_tracks(title, artist, limit)
except Exception as exc:
logger.debug("%s search_tracks raised: %s", self.active_server, exc)
logger.warning("%s search_tracks raised: %s", self.active_server, exc)
return []
def trigger_library_scan(self) -> bool:
@ -188,7 +193,7 @@ class MediaServerEngine:
try:
return client.trigger_library_scan()
except Exception as exc:
logger.debug("%s trigger_library_scan raised: %s", self.active_server, exc)
logger.warning("%s trigger_library_scan raised: %s", self.active_server, exc)
return False
def is_library_scanning(self) -> bool:
@ -200,7 +205,7 @@ class MediaServerEngine:
try:
return client.is_library_scanning()
except Exception as exc:
logger.debug("%s is_library_scanning raised: %s", self.active_server, exc)
logger.warning("%s is_library_scanning raised: %s", self.active_server, exc)
return False
def get_library_stats(self) -> Dict[str, int]:
@ -212,7 +217,7 @@ class MediaServerEngine:
try:
return client.get_library_stats()
except Exception as exc:
logger.debug("%s get_library_stats raised: %s", self.active_server, exc)
logger.warning("%s get_library_stats raised: %s", self.active_server, exc)
return {}
def get_recently_added_albums(self, max_results: int = 400) -> List[Any]:

9
core/media_server/registry.py
View file

@ -73,6 +73,15 @@ class MediaServerRegistry:
self._init_failures.append(spec.display_name)
self._instances[spec.name] = None
def set_instance(self, name: str, instance: Optional[MediaServerClient]) -> None:
"""Stash a pre-built client instance into the registry without
running the spec's factory. Used by callers (web_server.py at
boot) that already constructed the per-server clients and want
the engine to wrap those exact instances rather than build new
ones. Replaces the old pattern of reaching into ``_instances``
directly from the engine."""
self._instances[name] = instance
@property
def init_failures(self) -> List[str]:
return list(self._init_failures)

65
tests/media_server/test_engine.py
View file

@ -294,3 +294,68 @@ def test_get_media_server_engine_returns_set_singleton(make_engine):
assert get_media_server_engine() is engine
finally:
set_media_server_engine(None)
# ---------------------------------------------------------------------------
# Empty-engine fallback (web_server.py boot resilience)
# ---------------------------------------------------------------------------
def test_engine_with_empty_clients_dict_is_safe_to_use():
"""web_server.py falls back to ``MediaServerEngine(clients={})`` if
full engine init raises — preserves the resilience the per-server
globals had pre-refactor (each one had its own try/except so engine
failure didn't take down dispatch sites). Pin the contract: empty
engine still answers safely on every method instead of raising."""
registry = MediaServerRegistry()
registry.register(ServerSpec(
name='plex', factory=lambda: _FakeClient('plex'), display_name='Plex',
))
engine = MediaServerEngine(
registry=registry,
clients={}, # No pre-built clients passed.
active_server_resolver=lambda: 'plex',
)
# client(name) returns None for every server — engine doesn't crash.
assert engine.client('plex') is None
assert engine.client('jellyfin') is None
# Cross-server methods return safe defaults instead of raising.
assert engine.is_connected() is False
assert engine.get_all_artists() == []
assert engine.get_all_album_ids() == set()
assert engine.search_tracks('t', 'a') == []
# configured_clients() returns empty dict cleanly.
assert engine.configured_clients() == {}
# ---------------------------------------------------------------------------
# Registry encapsulation (engine reaches via public set_instance,
# not the private _instances dict)
# ---------------------------------------------------------------------------
def test_engine_uses_registry_set_instance_not_private_attr():
"""Engine constructor with ``clients=`` must hand instances off
via the registry's public ``set_instance(name, client)`` method
rather than reaching into ``registry._instances`` directly. Pin
by giving the registry a ``set_instance`` spy and verifying the
engine calls it."""
plex = _FakeClient('plex')
registry = MediaServerRegistry()
registry.register(ServerSpec(
name='plex', factory=lambda: _FakeClient('plex'), display_name='Plex',
))
set_instance_calls = []
original = registry.set_instance
def _spy(name, client):
set_instance_calls.append((name, client))
original(name, client)
registry.set_instance = _spy
MediaServerEngine(
registry=registry,
clients={'plex': plex},
active_server_resolver=lambda: 'plex',
)
assert ('plex', plex) in set_instance_calls

14
web_server.py
View file

@ -581,13 +581,23 @@ except Exception as e:
def _safe_init_media_client(factory, name):
"""Build a media-server client, capturing per-server init failures
so one broken server doesn't take the engine down with it."""
so one broken server doesn't take the engine down with it.
Logs the exception class so the boot log distinguishes config errors
(ConnectionError, AuthenticationError) from genuine import / dependency
failures — the broad ``Exception`` catch is intentional (any failure
on this code path means the user can't use that server, and we want
the other three to keep working) but the log makes the cause
diagnosable."""
try:
instance = factory()
logger.info(f" {name} client initialized")
return instance
except Exception as exc:
logger.error(f" {name} client failed to initialize: {exc}")
logger.error(
f" {name} client failed to initialize: {type(exc).__name__}: {exc}",
exc_info=True,
)
return None