Lyrics retag: fetch query from a read-only lyrics_meta, never db_data (no tag pollution)
Lock-in pass caught a real bug in 1051ef24: the retag lyrics path stuffed the
library title/artist into a plan's db_data to feed the lyrics query — but
db_data is exactly what write_tags_to_file writes ("only writes fields that
have DB values"). So an UNMATCHED track (one with no source match, meant to
get art/lyrics only) would have had its title/artist tags overwritten from
the library values — an unintended tag write on a track we never verified.
Fix: each plan now carries a separate READ-ONLY lyrics_meta
({title, artist, album}) sourced from the library track + album scope, kept
entirely out of db_data. apply_track_plans reads lyrics_meta for the query
(db_data fallback for older plans); unmatched plans keep db_data={} so no tags
are written. _fix_library_retag threads lyrics_meta through the manual-apply
path too.
Tests: +1 regression pinning that an unmatched lyrics plan calls
write_tags_to_file with EMPTY db_data (no title/artist leak) while still
fetching lyrics. 70 lyrics/retag/repair tests pass.
This commit is contained in:
parent
1051ef2402
commit
e93357a385
3 changed files with 57 additions and 20 deletions
|
|
@ -126,19 +126,25 @@ def apply_track_plans(track_plans, cover_action=None, cover_url=None, full=False
|
||||||
|
|
||||||
# Lyrics: fetch/refresh the .lrc for this track (independent of tag write
|
# Lyrics: fetch/refresh the .lrc for this track (independent of tag write
|
||||||
# success — a track with no tag changes may still be missing lyrics).
|
# success — a track with no tag changes may still be missing lyrics).
|
||||||
|
# Query metadata comes from the plan's READ-only lyrics_meta (never
|
||||||
|
# db_data, so nothing here can leak into a tag write). Falls back to
|
||||||
|
# db_data for plans that predate lyrics_meta.
|
||||||
if _lyrics_client:
|
if _lyrics_client:
|
||||||
try:
|
lm = tp.get('lyrics_meta') or {}
|
||||||
dur = db_data.get('duration') or db_data.get('duration_seconds')
|
title = lm.get('title') or db_data.get('title') or ''
|
||||||
if lyrics_client_wrote := _lyrics_client.create_lrc_file(
|
artist = lm.get('artist') or db_data.get('artist') or ''
|
||||||
fp,
|
if title:
|
||||||
db_data.get('title') or db_data.get('track_title') or '',
|
try:
|
||||||
db_data.get('artist') or db_data.get('artist_name') or '',
|
dur = lm.get('duration') or db_data.get('duration')
|
||||||
album_name=db_data.get('album') or db_data.get('album_title'),
|
wrote = _lyrics_client.create_lrc_file(
|
||||||
duration_seconds=int(dur) if dur else None,
|
fp, title, artist,
|
||||||
):
|
album_name=lm.get('album') or db_data.get('album'),
|
||||||
result['lyrics_written'] += 1
|
duration_seconds=int(dur) if dur else None,
|
||||||
except Exception as e:
|
)
|
||||||
logger.debug("retag lyrics fetch failed for %s: %s", fp, e)
|
if wrote:
|
||||||
|
result['lyrics_written'] += 1
|
||||||
|
except Exception as e:
|
||||||
|
logger.debug("retag lyrics fetch failed for %s: %s", fp, e)
|
||||||
|
|
||||||
if cover_action and cover_data and last_dir:
|
if cover_action and cover_data and last_dir:
|
||||||
try:
|
try:
|
||||||
|
|
@ -425,18 +431,22 @@ class LibraryRetagJob(RepairJob):
|
||||||
unmatched.append(lib['title'] or os.path.basename(lib['file_path']))
|
unmatched.append(lib['title'] or os.path.basename(lib['file_path']))
|
||||||
# No source match means no re-tag — but album cover art and/or
|
# No source match means no re-tag — but album cover art and/or
|
||||||
# lyrics still apply to the file, so those modes include an
|
# lyrics still apply to the file, so those modes include an
|
||||||
# art/lyrics-only plan (empty db_data → apply writes no tags).
|
# art/lyrics-only plan (empty db_data → apply writes NO tags).
|
||||||
if cover_action or lyrics_action:
|
if cover_action or lyrics_action:
|
||||||
track_plans.append({
|
plan_row = {
|
||||||
'file_path': rp,
|
'file_path': rp,
|
||||||
'track_id': lib['id'],
|
'track_id': lib['id'],
|
||||||
'title': lib['title'],
|
'title': lib['title'],
|
||||||
'changes': [],
|
'changes': [],
|
||||||
# Carry the library title/artist so lyrics fetch has a query
|
'db_data': {}, # never write tags for an unmatched track
|
||||||
# even when there's no source match to build db_data from.
|
}
|
||||||
'db_data': ({'title': lib.get('title'), 'artist': artist_name}
|
if lyrics_action:
|
||||||
if lyrics_action else {}),
|
# READ-only metadata for the lyrics query — kept OUT of
|
||||||
})
|
# db_data so it can never be written as tags.
|
||||||
|
plan_row['lyrics_meta'] = {
|
||||||
|
'title': lib.get('title'), 'artist': artist_name,
|
||||||
|
'album': album_title}
|
||||||
|
track_plans.append(plan_row)
|
||||||
continue
|
continue
|
||||||
current = _read_current_tags(rp)
|
current = _read_current_tags(rp)
|
||||||
plan = plan_track(current, src, album_meta, mode=mode)
|
plan = plan_track(current, src, album_meta, mode=mode)
|
||||||
|
|
@ -453,6 +463,11 @@ class LibraryRetagJob(RepairJob):
|
||||||
'changes': plan['changes'],
|
'changes': plan['changes'],
|
||||||
'db_data': db_data,
|
'db_data': db_data,
|
||||||
}
|
}
|
||||||
|
if lyrics_action:
|
||||||
|
# READ-only lyrics query metadata (never written as tags).
|
||||||
|
tp['lyrics_meta'] = {
|
||||||
|
'title': lib.get('title'), 'artist': artist_name,
|
||||||
|
'album': album_title}
|
||||||
if depth == 'full':
|
if depth == 'full':
|
||||||
tp['full_meta'] = _build_full_meta(
|
tp['full_meta'] = _build_full_meta(
|
||||||
db_data, src, album_title, artist_name, lib['title'])
|
db_data, src, album_title, artist_name, lib['title'])
|
||||||
|
|
|
||||||
|
|
@ -1465,6 +1465,8 @@ class RepairWorker:
|
||||||
plan = {'file_path': rp, 'db_data': t.get('db_data') or {}}
|
plan = {'file_path': rp, 'db_data': t.get('db_data') or {}}
|
||||||
if t.get('full_meta'):
|
if t.get('full_meta'):
|
||||||
plan['full_meta'] = t['full_meta']
|
plan['full_meta'] = t['full_meta']
|
||||||
|
if t.get('lyrics_meta'):
|
||||||
|
plan['lyrics_meta'] = t['lyrics_meta'] # read-only lyrics query metadata
|
||||||
resolved_plans.append(plan)
|
resolved_plans.append(plan)
|
||||||
|
|
||||||
from core.repair_jobs.library_retag import apply_track_plans
|
from core.repair_jobs.library_retag import apply_track_plans
|
||||||
|
|
|
||||||
|
|
@ -185,11 +185,31 @@ def test_apply_track_plans_lyrics_action(tmp_path, monkeypatch):
|
||||||
seen.update(title=title) or True)
|
seen.update(title=title) or True)
|
||||||
monkeypatch.setattr("core.lyrics_client.lyrics_client", fake_client)
|
monkeypatch.setattr("core.lyrics_client.lyrics_client", fake_client)
|
||||||
|
|
||||||
plans = [{"file_path": str(audio), "db_data": {"title": "Song", "artist": "Artist"}}]
|
plans = [{"file_path": str(audio), "db_data": {},
|
||||||
|
"lyrics_meta": {"title": "Song", "artist": "Artist", "album": "Album"}}]
|
||||||
res = library_retag.apply_track_plans(plans, lyrics_action=True)
|
res = library_retag.apply_track_plans(plans, lyrics_action=True)
|
||||||
assert res["lyrics_written"] == 1 and seen["title"] == "Song"
|
assert res["lyrics_written"] == 1 and seen["title"] == "Song"
|
||||||
|
|
||||||
|
|
||||||
|
def test_apply_track_plans_lyrics_never_writes_tags(tmp_path, monkeypatch):
|
||||||
|
# The lyrics query must come from lyrics_meta, NOT db_data — so an
|
||||||
|
# unmatched track (db_data={}) gets lyrics fetched but NO tags written.
|
||||||
|
from core.repair_jobs import library_retag
|
||||||
|
audio = tmp_path / "t.flac"; audio.write_bytes(b"x")
|
||||||
|
written = []
|
||||||
|
monkeypatch.setattr("core.tag_writer.write_tags_to_file",
|
||||||
|
lambda fp, db_data, **k: written.append(db_data) or {"success": True})
|
||||||
|
monkeypatch.setattr("core.lyrics_client.lyrics_client",
|
||||||
|
SimpleNamespace(create_lrc_file=lambda *a, **k: True))
|
||||||
|
|
||||||
|
plans = [{"file_path": str(audio), "db_data": {},
|
||||||
|
"lyrics_meta": {"title": "Song", "artist": "Artist", "album": "Al"}}]
|
||||||
|
res = library_retag.apply_track_plans(plans, lyrics_action=True)
|
||||||
|
assert res["lyrics_written"] == 1
|
||||||
|
# write_tags_to_file was called with an EMPTY db_data — no title/artist leaked in.
|
||||||
|
assert written == [{}]
|
||||||
|
|
||||||
|
|
||||||
def test_apply_track_plans_no_lyrics_when_disabled(tmp_path, monkeypatch):
|
def test_apply_track_plans_no_lyrics_when_disabled(tmp_path, monkeypatch):
|
||||||
from core.repair_jobs import library_retag
|
from core.repair_jobs import library_retag
|
||||||
audio = tmp_path / "t.flac"; audio.write_bytes(b"x")
|
audio = tmp_path / "t.flac"; audio.write_bytes(b"x")
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue