From 498c22e7c3576a36dd69c5d0ec950f97b5b3db75 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Fri, 10 Apr 2026 12:34:25 -0700 Subject: [PATCH] Centralize metadata source selection in core/metadata_service.py All metadata source decisions now flow through get_primary_source() and get_primary_client() in core/metadata_service.py. Previously 6 different files reimplemented this logic with inconsistent defaults ('itunes' vs 'deezer') and auth checks, causing bugs when any one was missed. Changes: - metadata_service.py: Added canonical get_primary_source/get_primary_client - web_server.py: _get_metadata_fallback_source() and _get_active_discovery_source() are now thin wrappers delegating to metadata_service - seasonal_discovery.py: _get_source() delegates to metadata_service - personalized_playlists.py: _get_active_source() delegates to metadata_service - spotify_client.py: Fixed _fallback_source default from 'itunes' to 'deezer' - watchlist_scanner.py: _get_fallback_metadata_client() delegates to metadata_service Future changes to source selection only need to update one file. --- core/metadata_service.py | 108 +++++++++++++++++++++++++++------ core/personalized_playlists.py | 14 +---- core/seasonal_discovery.py | 13 +--- core/spotify_client.py | 6 +- core/watchlist_scanner.py | 15 +---- web_server.py | 19 +++--- 6 files changed, 110 insertions(+), 65 deletions(-) diff --git a/core/metadata_service.py b/core/metadata_service.py index 2721a13d..b2ec0db4 100644 --- a/core/metadata_service.py +++ b/core/metadata_service.py @@ -1,9 +1,10 @@ """ -Metadata Service - Hot-swappable Spotify/iTunes/Deezer provider +Metadata Service - Centralized metadata source selection -Automatically uses Spotify when authenticated, falls back to the configured -fallback source (iTunes or Deezer) when not. -Provides unified interface for all metadata operations. +ALL metadata source decisions flow through this module. Other files import +get_primary_source() and get_primary_client() instead of reimplementing +the logic. This prevents bugs where different files have different defaults +or auth checks. """ from typing import List, Optional, Dict, Any, Literal @@ -16,37 +17,106 @@ logger = get_logger("metadata_service") MetadataProvider = Literal["spotify", "itunes", "auto"] -def _get_configured_fallback_source(): - """Get the configured metadata fallback source ('itunes' or 'deezer').""" +# ============================================================================= +# CANONICAL SOURCE SELECTION — all code should use these two functions +# ============================================================================= + +def get_primary_source() -> str: + """Get the user's configured primary metadata source. + + Returns 'spotify', 'deezer', 'itunes', 'discogs', or 'hydrabase'. + If the user selected Spotify but it's not authenticated, falls back to 'deezer'. + + This is THE single source of truth for "which metadata source should I use?" + All other modules should import this function instead of reading config directly. + """ try: from config.settings import config_manager - return config_manager.get('metadata.fallback_source', 'itunes') or 'itunes' + source = config_manager.get('metadata.fallback_source', 'deezer') or 'deezer' except Exception: - return 'itunes' + return 'deezer' + + # Validate Spotify selection — can't use it if not authenticated + if source == 'spotify': + try: + import importlib + ws = importlib.import_module('web_server') + sc = getattr(ws, 'spotify_client', None) + if not sc or not sc.is_spotify_authenticated(): + return 'deezer' + except Exception: + return 'deezer' + + return source -def _create_fallback_client(): - """Create the configured fallback metadata client.""" - source = _get_configured_fallback_source() +def get_primary_client(): + """Get the client object for the user's configured primary metadata source. + + Returns a SpotifyClient, DeezerClient, iTunesClient, DiscogsClient, + or HydrabaseClient instance. + + This is THE single source of truth for "which client should I call?" + """ + source = get_primary_source() + + if source == 'spotify': + try: + import importlib + ws = importlib.import_module('web_server') + sc = getattr(ws, 'spotify_client', None) + if sc and sc.is_spotify_authenticated(): + return sc + except Exception: + pass + # Spotify selected but unavailable — fall back to Deezer + from core.deezer_client import DeezerClient + return DeezerClient() + if source == 'deezer': from core.deezer_client import DeezerClient return DeezerClient() + + if source == 'discogs': + try: + from config.settings import config_manager + token = config_manager.get('discogs.token', '') + if token: + from core.discogs_client import DiscogsClient + return DiscogsClient(token=token) + except Exception: + pass + return iTunesClient() + if source == 'hydrabase': try: - from core.hydrabase_client import HydrabaseClient - # Hydrabase client is managed globally — try to import the running instance import importlib - ws_module = importlib.import_module('web_server') - client = getattr(ws_module, 'hydrabase_client', None) + ws = importlib.import_module('web_server') + client = getattr(ws, 'hydrabase_client', None) if client and client.is_connected(): return client except Exception: pass - # Hydrabase not available — fall back to iTunes return iTunesClient() + + # Default: iTunes return iTunesClient() +# ============================================================================= +# LEGACY ALIASES — kept for backward compatibility, delegate to canonical funcs +# ============================================================================= + +def _get_configured_fallback_source(): + """Legacy alias for get_primary_source(). Use get_primary_source() instead.""" + return get_primary_source() + + +def _create_fallback_client(): + """Legacy alias for get_primary_client(). Use get_primary_client() instead.""" + return get_primary_client() + + class MetadataService: """ Unified metadata service that seamlessly switches between Spotify and @@ -94,10 +164,8 @@ class MetadataService: return "spotify" elif self.preferred_provider == "itunes": return self._fallback_source - else: # auto - # Use is_spotify_authenticated() to check actual Spotify auth status - # (is_authenticated() always returns True due to fallback) - return "spotify" if self.spotify.is_spotify_authenticated() else self._fallback_source + else: # auto — use the centralized source selection + return get_primary_source() def _get_client(self): """Get the appropriate client based on provider selection""" diff --git a/core/personalized_playlists.py b/core/personalized_playlists.py index 9bd97388..a00c72d7 100644 --- a/core/personalized_playlists.py +++ b/core/personalized_playlists.py @@ -101,17 +101,9 @@ class PersonalizedPlaylistsService: self.spotify_client = spotify_client def _get_active_source(self) -> str: - """ - Determine which music source is active — respects user's configured primary source. - """ - try: - from config.settings import config_manager - source = config_manager.get('metadata.fallback_source', 'deezer') or 'deezer' - if source == 'spotify' and not (self.spotify_client and hasattr(self.spotify_client, 'is_spotify_authenticated') and self.spotify_client.is_spotify_authenticated()): - return 'deezer' - return source - except Exception: - return 'deezer' + """Determine which music source is active — delegates to centralized metadata_service.""" + from core.metadata_service import get_primary_source + return get_primary_source() def _build_track_dict(self, row, source: str) -> Dict: """Build a standardized track dictionary from a database row.""" diff --git a/core/seasonal_discovery.py b/core/seasonal_discovery.py index 827da764..e1d31e21 100644 --- a/core/seasonal_discovery.py +++ b/core/seasonal_discovery.py @@ -96,16 +96,9 @@ class SeasonalDiscoveryService: self._ensure_database_schema() def _get_source(self): - """Determine active music source — respects user's configured primary source""" - try: - from config.settings import config_manager - source = config_manager.get('metadata.fallback_source', 'deezer') or 'deezer' - # If user selected spotify, verify it's actually authenticated - if source == 'spotify' and not (self.spotify_client and self.spotify_client.is_spotify_authenticated()): - return 'deezer' - return source - except Exception: - return 'deezer' + """Determine active music source — delegates to centralized metadata_service.""" + from core.metadata_service import get_primary_source + return get_primary_source() def _ensure_database_schema(self): """Create seasonal content tables if they don't exist""" diff --git a/core/spotify_client.py b/core/spotify_client.py index 8462417c..2017dfa4 100644 --- a/core/spotify_client.py +++ b/core/spotify_client.py @@ -506,11 +506,11 @@ class SpotifyClient: @property def _fallback_source(self) -> str: - """Get configured metadata fallback source ('itunes', 'deezer', or 'discogs')""" + """Get configured primary metadata source for internal fallback routing.""" try: - return config_manager.get('metadata.fallback_source', 'itunes') or 'itunes' + return config_manager.get('metadata.fallback_source', 'deezer') or 'deezer' except Exception: - return 'itunes' + return 'deezer' @property def _fallback(self): diff --git a/core/watchlist_scanner.py b/core/watchlist_scanner.py index 89f744f1..bb57e6e5 100644 --- a/core/watchlist_scanner.py +++ b/core/watchlist_scanner.py @@ -30,18 +30,9 @@ ITUNES_BASE_DELAY = 1.0 # Base delay in seconds for exponential backoff def _get_fallback_metadata_client(): - """Get the configured metadata fallback client (iTunes or Deezer).""" - try: - from config.settings import config_manager - source = config_manager.get('metadata.fallback_source', 'itunes') or 'itunes' - if source == 'deezer': - from core.deezer_client import DeezerClient - return DeezerClient(), 'deezer' - from core.itunes_client import iTunesClient - return iTunesClient(), 'itunes' - except Exception: - from core.itunes_client import iTunesClient - return iTunesClient(), 'itunes' + """Get the configured metadata client — delegates to centralized metadata_service.""" + from core.metadata_service import get_primary_source, get_primary_client + return get_primary_client(), get_primary_source() def itunes_api_call_with_retry(func, *args, max_retries=ITUNES_MAX_RETRIES, **kwargs): diff --git a/web_server.py b/web_server.py index 8da844fe..31699986 100644 --- a/web_server.py +++ b/web_server.py @@ -33369,11 +33369,12 @@ def _get_deezer_client(): def _get_metadata_fallback_source(): """Get the configured primary metadata source. - Returns 'spotify', 'itunes', 'deezer', 'discogs', or 'hydrabase'.""" - try: - return config_manager.get('metadata.fallback_source', 'deezer') or 'deezer' - except Exception: - return 'deezer' + Returns 'spotify', 'itunes', 'deezer', 'discogs', or 'hydrabase'. + + NOTE: This is a thin wrapper — canonical logic lives in core.metadata_service.get_primary_source(). + Kept as a local function because 70+ callers reference it by name.""" + from core.metadata_service import get_primary_source + return get_primary_source() def _get_metadata_fallback_client(): """Get the active metadata client based on settings. @@ -40240,11 +40241,11 @@ def _get_active_discovery_source(): Determine which music source is active for discovery. Returns the user's configured primary metadata source. If the selected source requires auth and isn't available, falls back. + + NOTE: Thin wrapper — canonical logic lives in core.metadata_service.get_primary_source(). """ - source = _get_metadata_fallback_source() - if source == 'spotify' and not (spotify_client and spotify_client.is_spotify_authenticated()): - return 'deezer' - return source + from core.metadata_service import get_primary_source + return get_primary_source() @app.route('/api/discover/hero', methods=['GET'])