From 03711c10df79d22916d842843c3ed9525dcd9293 Mon Sep 17 00:00:00 2001 From: Antti Kettunen Date: Wed, 15 Apr 2026 20:28:09 +0300 Subject: [PATCH] Make metadata gap filler source-aware Only fetch source track details when ISRC enrichment is enabled, and show resolved source/track provenance in the UI. --- core/repair_jobs/metadata_gap_filler.py | 132 +++++++++++++---- tests/test_metadata_gap_filler.py | 179 ++++++++++++++++++++++++ webui/static/script.js | 2 + 3 files changed, 287 insertions(+), 26 deletions(-) create mode 100644 tests/test_metadata_gap_filler.py diff --git a/core/repair_jobs/metadata_gap_filler.py b/core/repair_jobs/metadata_gap_filler.py index f709b857..ead800b1 100644 --- a/core/repair_jobs/metadata_gap_filler.py +++ b/core/repair_jobs/metadata_gap_filler.py @@ -1,8 +1,6 @@ """Metadata Gap Filler Job — finds tracks missing key metadata and locates it from APIs.""" -import time - -from core.metadata_service import get_primary_source +from core.metadata_service import get_client_for_source, get_primary_source, get_source_priority from core.repair_jobs import register_job from core.repair_jobs.base import JobContext, JobResult, RepairJob from utils.logging_config import get_logger @@ -40,6 +38,7 @@ class MetadataGapFillerJob(RepairJob): settings = self._get_settings(context) fill_isrc = settings.get('fill_isrc', True) fill_mb_id = settings.get('fill_musicbrainz_id', True) + source_priority = get_source_priority(get_primary_source()) # Build WHERE clauses for missing fields (only columns that exist on tracks) conditions = [] @@ -53,27 +52,47 @@ class MetadataGapFillerJob(RepairJob): where = " OR ".join(conditions) - # Fetch tracks with gaps, prioritizing those with spotify_track_id + # Fetch tracks with gaps, prioritizing those with source track IDs. tracks = [] conn = None try: conn = context.db._get_connection() cursor = conn.cursor() + cursor.execute("PRAGMA table_info(tracks)") + track_columns = {column[1] for column in cursor.fetchall()} + + select_cols = [ + "t.id", + "t.title", + "ar.name", + "al.title", + "t.isrc", + "t.musicbrainz_recording_id", + "al.thumb_url", + "ar.thumb_url", + ] + column_map = [ + ("spotify_track_id", "t.spotify_track_id"), + ("itunes_track_id", "t.itunes_track_id"), + ("deezer_track_id", "t.deezer_track_id"), + ] + column_index = {} + for alias, column in column_map: + if column.split('.', 1)[1] in track_columns: + column_index[alias] = len(select_cols) + select_cols.append(f"{column} AS {alias}") + cursor.execute(f""" - SELECT t.id, t.title, ar.name, al.title, t.spotify_track_id, - t.isrc, t.musicbrainz_recording_id, - al.thumb_url, ar.thumb_url + SELECT {', '.join(select_cols)} FROM tracks t LEFT JOIN artists ar ON ar.id = t.artist_id LEFT JOIN albums al ON al.id = t.album_id WHERE t.title IS NOT NULL AND t.title != '' AND ({where}) - ORDER BY - CASE WHEN t.spotify_track_id IS NOT NULL AND t.spotify_track_id != '' THEN 0 ELSE 1 END, - t.id LIMIT 500 """) tracks = cursor.fetchall() + tracks = sorted(tracks, key=lambda row: _track_row_priority(row, column_index, source_priority)) except Exception as e: logger.error("Error fetching tracks with metadata gaps: %s", e, exc_info=True) result.errors += 1 @@ -97,7 +116,12 @@ class MetadataGapFillerJob(RepairJob): if i % 20 == 0 and context.wait_if_paused(): return result - track_id, title, artist_name, album_title, spotify_track_id, isrc, mb_id, album_thumb, artist_thumb = row + track_id, title, artist_name, album_title, isrc, mb_id, album_thumb, artist_thumb = row[:8] + source_track_ids = { + 'spotify': row[column_index['spotify_track_id']] if 'spotify_track_id' in column_index else None, + 'itunes': row[column_index['itunes_track_id']] if 'itunes_track_id' in column_index else None, + 'deezer': row[column_index['deezer_track_id']] if 'deezer_track_id' in column_index else None, + } result.scanned += 1 if context.report_progress: @@ -108,20 +132,29 @@ class MetadataGapFillerJob(RepairJob): log_type='info' ) found_fields = {} + resolved_source = None + resolved_track_id = None - # Try Spotify enrichment for ISRC — only when Spotify is the configured primary source. - # If Deezer/iTunes is primary, Spotify may still be authenticated for playlist sync - # but should not be called here, as it burns API quota unnecessarily. - if spotify_track_id and context.spotify_client and not context.is_spotify_rate_limited() and get_primary_source() == 'spotify': - try: - track_data = context.spotify_client.get_track_details(spotify_track_id) - if track_data: - if fill_isrc and not isrc: - ext_ids = track_data.get('external_ids', {}) - if ext_ids.get('isrc'): - found_fields['isrc'] = ext_ids['isrc'] - except Exception as e: - logger.debug("Spotify enrichment failed for track %s: %s", track_id, e) + # Try source-aware track detail lookups only when ISRC enrichment is enabled. + if fill_isrc and not isrc: + for source in source_priority: + track_source_id = source_track_ids.get(source) + if not track_source_id: + continue + try: + client = get_client_for_source(source) + if not client or not hasattr(client, 'get_track_details'): + continue + track_data = client.get_track_details(track_source_id) + if track_data: + isrc_value = _extract_isrc(track_data) + if isrc_value: + found_fields['isrc'] = isrc_value + resolved_source = source + resolved_track_id = track_source_id + break + except Exception as e: + logger.debug("%s enrichment failed for track %s: %s", source.capitalize(), track_id, e) # Try MusicBrainz for MB recording ID if fill_mb_id and not mb_id and context.mb_client: @@ -161,7 +194,9 @@ class MetadataGapFillerJob(RepairJob): 'title': title, 'artist': artist_name, 'album': album_title, - 'spotify_track_id': spotify_track_id, + 'track_ids': source_track_ids, + 'resolved_source': resolved_source, + 'resolved_track_id': resolved_track_id, 'found_fields': found_fields, 'album_thumb_url': album_thumb or None, 'artist_thumb_url': artist_thumb or None, @@ -175,7 +210,7 @@ class MetadataGapFillerJob(RepairJob): result.skipped += 1 # Rate limit API calls - if spotify_track_id: + if fill_isrc and any(source_track_ids.values()): if context.sleep_or_stop(0.5): return result @@ -215,3 +250,48 @@ class MetadataGapFillerJob(RepairJob): finally: if conn: conn.close() + + +def _extract_isrc(track_data): + """Extract ISRC from a track detail payload.""" + if not track_data or not isinstance(track_data, dict): + return None + + external_ids = track_data.get('external_ids') + if isinstance(external_ids, dict): + isrc = external_ids.get('isrc') + if isrc: + return isrc + + isrc = track_data.get('isrc') + if isrc: + return isrc + + raw_data = track_data.get('raw_data') + if isinstance(raw_data, dict): + external_ids = raw_data.get('external_ids') + if isinstance(external_ids, dict) and external_ids.get('isrc'): + return external_ids['isrc'] + if raw_data.get('isrc'): + return raw_data['isrc'] + + return None + + +def _track_row_priority(row, column_index, source_priority): + """Sort rows by the first source track ID available in priority order.""" + source_columns = { + 'spotify': 'spotify_track_id', + 'itunes': 'itunes_track_id', + 'deezer': 'deezer_track_id', + } + + for idx, source in enumerate(source_priority): + column_name = source_columns.get(source) + if not column_name: + continue + column_pos = column_index.get(column_name) + if column_pos is not None and row[column_pos]: + return idx + + return len(source_priority) diff --git a/tests/test_metadata_gap_filler.py b/tests/test_metadata_gap_filler.py new file mode 100644 index 00000000..ba2ab44f --- /dev/null +++ b/tests/test_metadata_gap_filler.py @@ -0,0 +1,179 @@ +import sqlite3 +import sys +import types +from types import SimpleNamespace + +# Stub optional Spotify dependency so metadata_service can import in tests. +if 'spotipy' not in sys.modules: + spotipy = types.ModuleType('spotipy') + oauth2 = types.ModuleType('spotipy.oauth2') + + class _DummySpotify: + pass + + class _DummyOAuth: + pass + + spotipy.Spotify = _DummySpotify + oauth2.SpotifyOAuth = _DummyOAuth + oauth2.SpotifyClientCredentials = _DummyOAuth + spotipy.oauth2 = oauth2 + sys.modules['spotipy'] = spotipy + sys.modules['spotipy.oauth2'] = oauth2 + +if 'config.settings' not in sys.modules: + config_mod = types.ModuleType('config') + settings_mod = types.ModuleType('config.settings') + + class _DummyConfigManager: + def get(self, key, default=None): + return default + + settings_mod.config_manager = _DummyConfigManager() + config_mod.settings = settings_mod + sys.modules['config'] = config_mod + sys.modules['config.settings'] = settings_mod + +from core.repair_jobs import metadata_gap_filler as mgf + + +class _FakeTrackClient: + def __init__(self, source_name, isrc=None): + self.source_name = source_name + self.isrc = isrc + self.calls = [] + + def get_track_details(self, track_id): + self.calls.append(track_id) + if self.isrc is None: + return None + return { + 'id': track_id, + 'external_ids': {'isrc': self.isrc}, + } + + +class _FakeMBClient: + def __init__(self): + self.calls = [] + + def search_recording(self, title, artist_name=None, limit=1): + self.calls.append((title, artist_name, limit)) + return [{'id': 'mb-recording'}] + + +def _make_db(): + conn = sqlite3.connect(':memory:') + conn.row_factory = sqlite3.Row + cursor = conn.cursor() + cursor.execute( + """ + CREATE TABLE artists ( + id INTEGER PRIMARY KEY, + name TEXT, + thumb_url TEXT + ) + """ + ) + cursor.execute( + """ + CREATE TABLE albums ( + id INTEGER PRIMARY KEY, + title TEXT, + thumb_url TEXT + ) + """ + ) + cursor.execute( + """ + CREATE TABLE tracks ( + id INTEGER PRIMARY KEY, + title TEXT, + artist_id INTEGER, + album_id INTEGER, + spotify_track_id TEXT, + itunes_track_id TEXT, + deezer_track_id TEXT, + isrc TEXT, + musicbrainz_recording_id TEXT + ) + """ + ) + cursor.execute("INSERT INTO artists (id, name, thumb_url) VALUES (1, 'Artist', '')") + cursor.execute("INSERT INTO albums (id, title, thumb_url) VALUES (1, 'Album', '')") + cursor.execute( + """ + INSERT INTO tracks + (id, title, artist_id, album_id, spotify_track_id, itunes_track_id, deezer_track_id, isrc, musicbrainz_recording_id) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + (1, 'Track Title', 1, 1, 'sp-1', None, 'dz-1', '', ''), + ) + conn.commit() + return conn + + +def _make_context(conn): + findings = [] + return SimpleNamespace( + db=SimpleNamespace(_get_connection=lambda: conn), + config_manager=SimpleNamespace(get=lambda key, default=None: default), + check_stop=lambda: False, + wait_if_paused=lambda: False, + update_progress=lambda *args, **kwargs: None, + report_progress=lambda *args, **kwargs: None, + sleep_or_stop=lambda seconds: False, + mb_client=_FakeMBClient(), + create_finding=lambda **kwargs: findings.append(kwargs), + findings=findings, + ) + + +def test_metadata_gap_filler_prefers_primary_track_source(monkeypatch): + conn = _make_db() + context = _make_context(conn) + + spotify_client = _FakeTrackClient('spotify', isrc='SP-ISRC') + deezer_client = _FakeTrackClient('deezer', isrc='DZ-ISRC') + itunes_client = _FakeTrackClient('itunes', isrc=None) + + monkeypatch.setattr(mgf, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr( + mgf, + 'get_client_for_source', + lambda source: {'spotify': spotify_client, 'deezer': deezer_client, 'itunes': itunes_client}.get(source), + ) + + result = mgf.MetadataGapFillerJob().scan(context) + + assert result.findings_created == 1 + assert deezer_client.calls == ['dz-1'] + assert spotify_client.calls == [] + assert context.findings[0]['details']['found_fields']['isrc'] == 'DZ-ISRC' + assert context.findings[0]['details']['resolved_source'] == 'deezer' + assert context.findings[0]['details']['resolved_track_id'] == 'dz-1' + + +def test_metadata_gap_filler_skips_track_detail_lookup_when_isrc_disabled(monkeypatch): + conn = _make_db() + context = _make_context(conn) + + spotify_client = _FakeTrackClient('spotify', isrc='SP-ISRC') + deezer_client = _FakeTrackClient('deezer', isrc='DZ-ISRC') + + monkeypatch.setattr(mgf, 'get_primary_source', lambda: 'deezer') + monkeypatch.setattr( + mgf, + 'get_client_for_source', + lambda source: {'spotify': spotify_client, 'deezer': deezer_client}.get(source), + ) + + job = mgf.MetadataGapFillerJob() + monkeypatch.setattr(job, '_get_settings', lambda context: {'fill_isrc': False, 'fill_musicbrainz_id': True}) + + result = job.scan(context) + + assert result.findings_created == 1 + assert spotify_client.calls == [] + assert deezer_client.calls == [] + assert context.findings[0]['details']['found_fields']['musicbrainz_recording_id'] == 'mb-recording' diff --git a/webui/static/script.js b/webui/static/script.js index 34a033d9..2af1524b 100644 --- a/webui/static/script.js +++ b/webui/static/script.js @@ -64089,6 +64089,8 @@ function _renderFindingDetail(f) { if (d.album) rows.push(['Album', d.album]); if (d.title) rows.push(['Title', d.title]); if (d.spotify_track_id) rows.push(['Spotify ID', d.spotify_track_id]); + if (d.resolved_source) rows.push(['Resolved Source', d.resolved_source]); + if (d.resolved_track_id) rows.push(['Resolved Track ID', d.resolved_track_id]); if (d.found_fields && typeof d.found_fields === 'object') { Object.entries(d.found_fields).forEach(([k, v]) => { rows.push([`Found: ${k}`, String(v), 'success']);