Fix: Spotify sync crash 'unexpected keyword argument candidate_pool'
When no media server is connected, discovery/sync patches sync_service's
matcher with a database-only implementation. sync_service calls it as
_find_track_in_media_server(track, candidate_pool=...) (a per-artist candidate
cache), but the database-only override took only (spotify_track) — so every
sync raised 'database_only_find_track() got an unexpected keyword argument
candidate_pool' and aborted.
Lift the override from a nested closure to a module-level
_database_only_find_track(spotify_track, candidate_pool=None) so it (a) accepts
the kwarg for interface parity with the real matcher and (b) is importable and
unit-testable. The DB-only path queries the library directly via
check_track_exists, so it accepts but doesn't need the candidate cache. Also
dropped the dead original_find_track local.
Tests: signature includes candidate_pool; called with candidate_pool={} returns
(None, 0.0) on no match; returns the match when the DB has it.
This commit is contained in:
BoulderBadgeDad
parent
227c9373fe
commit
482d5fbc79
2 changed files with 138 additions and 83 deletions
|
|
@ -49,6 +49,90 @@ class SyncDeps:
|
|||
sync_lock: Any # threading.Lock
|
||||
|
||||
|
||||
async def _database_only_find_track(spotify_track, candidate_pool=None):
|
||||
"""Database-only track matcher used when no media server is connected.
|
||||
|
||||
Patched onto sync_service._find_track_in_media_server. Accepts
|
||||
``candidate_pool`` for interface parity with the real matcher (sync_service
|
||||
calls it with candidate_pool=...); the DB path queries the library directly
|
||||
via check_track_exists, so it doesn't need the per-artist candidate cache —
|
||||
but it MUST accept the kwarg or sync raises "unexpected keyword argument
|
||||
'candidate_pool'". Module-level (not a nested closure) so it's importable
|
||||
and unit-tested.
|
||||
"""
|
||||
logger.info(f"Database-only search for: '{spotify_track.name}' by {spotify_track.artists}")
|
||||
try:
|
||||
from database.music_database import MusicDatabase
|
||||
from config.settings import config_manager
|
||||
|
||||
db = MusicDatabase()
|
||||
active_server = config_manager.get_active_media_server()
|
||||
original_title = spotify_track.name
|
||||
spotify_id = getattr(spotify_track, 'id', '') or ''
|
||||
|
||||
# --- Sync match cache fast-path ---
|
||||
if spotify_id:
|
||||
try:
|
||||
cached = db.read_sync_match_cache(spotify_id, active_server)
|
||||
if cached:
|
||||
db_track_check = db.get_track_by_id(cached['server_track_id'])
|
||||
if db_track_check:
|
||||
class DatabaseTrackCached:
|
||||
def __init__(self, db_t):
|
||||
self.ratingKey = db_t.id
|
||||
self.title = db_t.title
|
||||
self.id = db_t.id
|
||||
logger.debug(f"Sync cache hit: '{original_title}' → server track {cached['server_track_id']}")
|
||||
return DatabaseTrackCached(db_track_check), cached['confidence']
|
||||
logger.warning(f"Sync cache stale for '{original_title}' — track gone")
|
||||
except Exception as e:
|
||||
logger.debug("sync match cache fast-path failed: %s", e)
|
||||
# --- End cache fast-path ---
|
||||
|
||||
# Try each artist
|
||||
for artist in spotify_track.artists:
|
||||
if isinstance(artist, str):
|
||||
artist_name = artist
|
||||
elif isinstance(artist, dict) and 'name' in artist:
|
||||
artist_name = artist['name']
|
||||
else:
|
||||
artist_name = str(artist)
|
||||
|
||||
db_track, confidence = db.check_track_exists(
|
||||
original_title, artist_name,
|
||||
confidence_threshold=0.80,
|
||||
server_source=active_server
|
||||
)
|
||||
|
||||
if db_track and confidence >= 0.80:
|
||||
logger.info(f"Database match: '{db_track.title}' (confidence: {confidence:.2f})")
|
||||
if spotify_id:
|
||||
try:
|
||||
from core.matching_engine import MusicMatchingEngine
|
||||
me = MusicMatchingEngine()
|
||||
db.save_sync_match_cache(
|
||||
spotify_id, me.clean_title(original_title), me.clean_artist(artist_name),
|
||||
active_server, db_track.id, db_track.title, confidence
|
||||
)
|
||||
except Exception as e:
|
||||
logger.debug("save sync match cache failed: %s", e)
|
||||
|
||||
class DatabaseTrackMock:
|
||||
def __init__(self, db_track):
|
||||
self.ratingKey = db_track.id
|
||||
self.title = db_track.title
|
||||
self.id = db_track.id
|
||||
|
||||
return DatabaseTrackMock(db_track), confidence
|
||||
|
||||
logger.warning(f"No database match found for: '{original_title}'")
|
||||
return None, 0.0
|
||||
|
||||
except Exception as e:
|
||||
logger.error(f"Database search error: {e}")
|
||||
return None, 0.0
|
||||
|
||||
|
||||
def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, profile_id=1, playlist_image_url='', deps: SyncDeps = None, sync_mode: str = 'replace'):
|
||||
"""The actual sync function that runs in the background thread."""
|
||||
sync_states = deps.sync_states
|
||||
|
|
@ -260,89 +344,9 @@ def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, p
|
|||
if media_client is None or not media_client.is_connected():
|
||||
logger.info("Media client not connected - patching sync service for database-only matching")
|
||||
|
||||
# Store original method
|
||||
original_find_track = sync_service._find_track_in_media_server
|
||||
|
||||
# Create database-only replacement method
|
||||
async def database_only_find_track(spotify_track):
|
||||
logger.info(f"Database-only search for: '{spotify_track.name}' by {spotify_track.artists}")
|
||||
try:
|
||||
from database.music_database import MusicDatabase
|
||||
from config.settings import config_manager
|
||||
|
||||
db = MusicDatabase()
|
||||
active_server = config_manager.get_active_media_server()
|
||||
original_title = spotify_track.name
|
||||
spotify_id = getattr(spotify_track, 'id', '') or ''
|
||||
|
||||
# --- Sync match cache fast-path ---
|
||||
if spotify_id:
|
||||
try:
|
||||
cached = db.read_sync_match_cache(spotify_id, active_server)
|
||||
if cached:
|
||||
db_track_check = db.get_track_by_id(cached['server_track_id'])
|
||||
if db_track_check:
|
||||
class DatabaseTrackCached:
|
||||
def __init__(self, db_t):
|
||||
self.ratingKey = db_t.id
|
||||
self.title = db_t.title
|
||||
self.id = db_t.id
|
||||
logger.debug(f"Sync cache hit: '{original_title}' → server track {cached['server_track_id']}")
|
||||
return DatabaseTrackCached(db_track_check), cached['confidence']
|
||||
logger.warning(f"Sync cache stale for '{original_title}' — track gone")
|
||||
except Exception as e:
|
||||
logger.debug("sync match cache fast-path failed: %s", e)
|
||||
# --- End cache fast-path ---
|
||||
|
||||
# Try each artist (same logic as original)
|
||||
for artist in spotify_track.artists:
|
||||
# Extract artist name from both string and dict formats
|
||||
if isinstance(artist, str):
|
||||
artist_name = artist
|
||||
elif isinstance(artist, dict) and 'name' in artist:
|
||||
artist_name = artist['name']
|
||||
else:
|
||||
artist_name = str(artist)
|
||||
|
||||
db_track, confidence = db.check_track_exists(
|
||||
original_title, artist_name,
|
||||
confidence_threshold=0.80,
|
||||
server_source=active_server
|
||||
)
|
||||
|
||||
if db_track and confidence >= 0.80:
|
||||
logger.info(f"Database match: '{db_track.title}' (confidence: {confidence:.2f})")
|
||||
|
||||
# Save to sync match cache
|
||||
if spotify_id:
|
||||
try:
|
||||
from core.matching_engine import MusicMatchingEngine
|
||||
me = MusicMatchingEngine()
|
||||
db.save_sync_match_cache(
|
||||
spotify_id, me.clean_title(original_title), me.clean_artist(artist_name),
|
||||
active_server, db_track.id, db_track.title, confidence
|
||||
)
|
||||
except Exception as e:
|
||||
logger.debug("save sync match cache failed: %s", e)
|
||||
|
||||
# Create mock track object for playlist creation
|
||||
class DatabaseTrackMock:
|
||||
def __init__(self, db_track):
|
||||
self.ratingKey = db_track.id
|
||||
self.title = db_track.title
|
||||
self.id = db_track.id
|
||||
|
||||
return DatabaseTrackMock(db_track), confidence
|
||||
|
||||
logger.warning(f"No database match found for: '{original_title}'")
|
||||
return None, 0.0
|
||||
|
||||
except Exception as e:
|
||||
logger.error(f"Database search error: {e}")
|
||||
return None, 0.0
|
||||
|
||||
# Patch the method
|
||||
sync_service._find_track_in_media_server = database_only_find_track
|
||||
# Patch the matcher to the module-level database-only implementation
|
||||
# (importable + unit-tested; accepts candidate_pool for parity).
|
||||
sync_service._find_track_in_media_server = _database_only_find_track
|
||||
logger.info("Patched sync service to use database-only matching")
|
||||
|
||||
sync_start_time = time.time()
|
||||
|
|
|
|||
51
tests/discovery/test_sync_database_only_matcher.py
Normal file
51
tests/discovery/test_sync_database_only_matcher.py
Normal file
|
|
@ -0,0 +1,51 @@
|
|||
"""Regression: the database-only sync matcher must accept candidate_pool.
|
||||
|
||||
sync_service calls _find_track_in_media_server(track, candidate_pool=...). When
|
||||
no media server is connected, discovery/sync patches in a database-only matcher.
|
||||
That override dropped the candidate_pool kwarg, so every Spotify sync failed with
|
||||
"unexpected keyword argument 'candidate_pool'". These pin the contract.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import inspect
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import core.discovery.sync as sync_mod
|
||||
|
||||
|
||||
def test_matcher_signature_accepts_candidate_pool():
|
||||
sig = inspect.signature(sync_mod._database_only_find_track)
|
||||
assert 'candidate_pool' in sig.parameters
|
||||
|
||||
|
||||
def _run(track, **kw):
|
||||
fake_db = MagicMock()
|
||||
fake_db.read_sync_match_cache.return_value = None
|
||||
fake_db.check_track_exists.return_value = (None, 0.0)
|
||||
fake_cm = MagicMock()
|
||||
fake_cm.get_active_media_server.return_value = "plex"
|
||||
with patch("database.music_database.MusicDatabase", return_value=fake_db), \
|
||||
patch("config.settings.config_manager", fake_cm):
|
||||
return asyncio.run(sync_mod._database_only_find_track(track, **kw))
|
||||
|
||||
|
||||
def test_called_with_candidate_pool_no_match():
|
||||
track = SimpleNamespace(name="Song", artists=["Artist"], id="sp1")
|
||||
# The exact call sync_service makes — must not raise TypeError.
|
||||
assert _run(track, candidate_pool={}) == (None, 0.0)
|
||||
|
||||
|
||||
def test_returns_match_when_db_has_it():
|
||||
track = SimpleNamespace(name="HUMBLE.", artists=["Kendrick Lamar"], id="sp2")
|
||||
fake_db = MagicMock()
|
||||
fake_db.read_sync_match_cache.return_value = None
|
||||
fake_db.check_track_exists.return_value = (SimpleNamespace(id="t1", title="HUMBLE."), 0.95)
|
||||
fake_cm = MagicMock()
|
||||
fake_cm.get_active_media_server.return_value = "plex"
|
||||
with patch("database.music_database.MusicDatabase", return_value=fake_db), \
|
||||
patch("config.settings.config_manager", fake_cm):
|
||||
match, conf = asyncio.run(sync_mod._database_only_find_track(track, candidate_pool={}))
|
||||
assert conf == 0.95 and match.id == "t1"
|
||||
Loading…
Reference in a new issue