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.
This commit is contained in:
Antti Kettunen
parent
6df6ecb560
commit
03711c10df
3 changed files with 287 additions and 26 deletions
|
|
@ -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)
|
||||
|
|
|
|||
179
tests/test_metadata_gap_filler.py
Normal file
179
tests/test_metadata_gap_filler.py
Normal file
|
|
@ -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'
|
||||
|
|
@ -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']);
|
||||
|
|
|
|||
Loading…
Reference in a new issue