Library Re-tag: cover-mode scans stop producing unappliable "(0 track(s))" findings
On path-mapped setups (Docker mounts etc.) the scan checked a bare os.path.isfile() on the raw DB path — false for EVERY track — while the apply handler resolves container/host mismatches. With a cover-art mode set, the cover_action kept the album past the "anything to do?" gate, so every album produced a finding with an empty tracks list whose apply could only ever fail with "No tracks to re-tag in finding". - the scan now resolves each track path with the same resolver the apply handler uses (resolve_library_file_path) before reachability checks and current-tag reads; plans carry the resolved path - a finding can never be created with zero tracks — cover-action albums with no usable tracks are skipped, with a debug log of why (unreachable/unmatched counts) and the counts surfaced in the finding description - unmatched-but-reachable tracks now get an art-only plan (empty db_data) so album cover art covers ALL the album's files, not just source-matched ones — apply_track_plans already treats empty db_data as a pure cover embed and counts a failed cover download as skipped, never failed (now locked by tests) - cover-only findings are titled "(cover art, N track(s))" instead of the puzzling "(0 track(s))" Tests: +5 (mapped paths resolve into plans, cover-with-nothing-reachable creates no finding, unmatched -> art-only plan, art-only plan embeds cover, failed cover download -> skipped). 87 passed across retag/repair/tag_writer.
This commit is contained in:
parent
07d09d7d0e
commit
df19317dac
2 changed files with 157 additions and 7 deletions
|
|
@ -12,6 +12,7 @@ finding. The apply handler lives in repair_worker (_fix_library_retag).
|
||||||
|
|
||||||
import os
|
import os
|
||||||
|
|
||||||
|
from core.library.path_resolver import resolve_library_file_path
|
||||||
from core.library.retag_planner import (
|
from core.library.retag_planner import (
|
||||||
MODE_FILL_MISSING,
|
MODE_FILL_MISSING,
|
||||||
MODE_OVERWRITE,
|
MODE_OVERWRITE,
|
||||||
|
|
@ -366,15 +367,41 @@ class LibraryRetagJob(RepairJob):
|
||||||
cover_action = self._cover_action(cover_mode, cover_url, library_tracks)
|
cover_action = self._cover_action(cover_mode, cover_url, library_tracks)
|
||||||
|
|
||||||
pairs = match_source_tracks(source_tracks, library_tracks)
|
pairs = match_source_tracks(source_tracks, library_tracks)
|
||||||
|
download_folder = (context.config_manager.get('soulseek.download_path', '')
|
||||||
|
if context.config_manager else None)
|
||||||
track_plans = []
|
track_plans = []
|
||||||
unmatched = []
|
unmatched = []
|
||||||
|
unreachable = 0
|
||||||
for lib, src in pairs:
|
for lib, src in pairs:
|
||||||
|
# Resolve container/host path mismatches the same way the apply
|
||||||
|
# handler does. The old bare os.path.isfile() on the raw DB path
|
||||||
|
# failed for EVERY track on path-mapped setups (Docker mounts), so
|
||||||
|
# cover-mode scans produced "(0 track(s))" findings that the apply
|
||||||
|
# then rejected with "No tracks to re-tag in finding".
|
||||||
|
rp = resolve_library_file_path(
|
||||||
|
lib['file_path'],
|
||||||
|
transfer_folder=getattr(context, 'transfer_folder', None),
|
||||||
|
download_folder=download_folder,
|
||||||
|
config_manager=context.config_manager,
|
||||||
|
)
|
||||||
|
if not rp:
|
||||||
|
unreachable += 1
|
||||||
|
continue # genuinely unreachable from this process
|
||||||
if src is None:
|
if src is None:
|
||||||
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 still
|
||||||
|
# applies to the file, so cover modes include an art-only plan
|
||||||
|
# (empty db_data: apply embeds art and writes no tags).
|
||||||
|
if cover_action:
|
||||||
|
track_plans.append({
|
||||||
|
'file_path': rp,
|
||||||
|
'track_id': lib['id'],
|
||||||
|
'title': lib['title'],
|
||||||
|
'changes': [],
|
||||||
|
'db_data': {},
|
||||||
|
})
|
||||||
continue
|
continue
|
||||||
if not os.path.isfile(lib['file_path']):
|
current = _read_current_tags(rp)
|
||||||
continue # not reachable at the stored path — skip (apply resolves paths)
|
|
||||||
current = _read_current_tags(lib['file_path'])
|
|
||||||
plan = plan_track(current, src, album_meta, mode=mode)
|
plan = plan_track(current, src, album_meta, mode=mode)
|
||||||
# Include a track when its tags change, OR when there's a cover action
|
# Include a track when its tags change, OR when there's a cover action
|
||||||
# to apply to it (db_data may be empty — apply embeds art either way).
|
# to apply to it (db_data may be empty — apply embeds art either way).
|
||||||
|
|
@ -382,7 +409,7 @@ class LibraryRetagJob(RepairJob):
|
||||||
db_data = plan['db_data']
|
db_data = plan['db_data']
|
||||||
_add_source_ids(db_data, source, album_source_id, src)
|
_add_source_ids(db_data, source, album_source_id, src)
|
||||||
tp = {
|
tp = {
|
||||||
'file_path': lib['file_path'],
|
'file_path': rp,
|
||||||
'track_id': lib['id'],
|
'track_id': lib['id'],
|
||||||
'title': lib['title'],
|
'title': lib['title'],
|
||||||
'changes': plan['changes'],
|
'changes': plan['changes'],
|
||||||
|
|
@ -394,7 +421,14 @@ class LibraryRetagJob(RepairJob):
|
||||||
track_plans.append(tp)
|
track_plans.append(tp)
|
||||||
|
|
||||||
tag_change_tracks = sum(1 for tp in track_plans if tp['changes'])
|
tag_change_tracks = sum(1 for tp in track_plans if tp['changes'])
|
||||||
if not tag_change_tracks and not cover_action:
|
if (not tag_change_tracks and not cover_action) or not track_plans:
|
||||||
|
# Nothing actionable. The second clause covers cover-action albums
|
||||||
|
# where no track is reachable/included — creating a finding there
|
||||||
|
# gives an unappliable "(0 track(s))" entry.
|
||||||
|
if cover_action and not track_plans:
|
||||||
|
logger.debug(
|
||||||
|
"Library re-tag: album %s skipped — cover action but no usable tracks "
|
||||||
|
"(%d unreachable, %d unmatched)", album_id, unreachable, len(unmatched))
|
||||||
result.skipped += 1
|
result.skipped += 1
|
||||||
return
|
return
|
||||||
|
|
||||||
|
|
@ -419,7 +453,14 @@ class LibraryRetagJob(RepairJob):
|
||||||
desc = (f'Album "{album_title}" by {artist_name or "Unknown"} would be re-tagged from '
|
desc = (f'Album "{album_title}" by {artist_name or "Unknown"} would be re-tagged from '
|
||||||
f'{source} ({", ".join(summary_bits)}).')
|
f'{source} ({", ".join(summary_bits)}).')
|
||||||
if unmatched:
|
if unmatched:
|
||||||
desc += f' {len(unmatched)} track(s) could not be matched to the source and are left untouched.'
|
desc += (f' {len(unmatched)} track(s) could not be matched to the source — '
|
||||||
|
f'tags left untouched{" (cover art still applied)" if cover_action else ""}.')
|
||||||
|
if unreachable:
|
||||||
|
desc += f' {unreachable} track(s) not reachable on disk and skipped.'
|
||||||
|
|
||||||
|
# Cover-only findings say so instead of the puzzling "(0 track(s))".
|
||||||
|
title_what = (f'{tag_change_tracks} track(s)' if tag_change_tracks
|
||||||
|
else f'cover art, {len(track_plans)} track(s)')
|
||||||
|
|
||||||
if context.create_finding:
|
if context.create_finding:
|
||||||
inserted = context.create_finding(
|
inserted = context.create_finding(
|
||||||
|
|
@ -429,7 +470,7 @@ class LibraryRetagJob(RepairJob):
|
||||||
entity_type='album',
|
entity_type='album',
|
||||||
entity_id=str(album_id),
|
entity_id=str(album_id),
|
||||||
file_path=None,
|
file_path=None,
|
||||||
title=f'Re-tag: {album_title or "Unknown"} ({tag_change_tracks} track(s))',
|
title=f'Re-tag: {album_title or "Unknown"} ({title_what})',
|
||||||
description=desc,
|
description=desc,
|
||||||
details={
|
details={
|
||||||
'album_id': album_id,
|
'album_id': album_id,
|
||||||
|
|
|
||||||
|
|
@ -284,3 +284,112 @@ def test_fix_library_retag_counts_unreachable(tmp_path, monkeypatch):
|
||||||
'cover_action': None, 'cover_url': None}
|
'cover_action': None, 'cover_url': None}
|
||||||
res = worker._fix_library_retag('album', '1', None, details)
|
res = worker._fix_library_retag('album', '1', None, details)
|
||||||
assert res['success'] is False # nothing written (file missing)
|
assert res['success'] is False # nothing written (file missing)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Cover-art scans on path-mapped setups (the "(0 track(s))" / "No tracks to
|
||||||
|
# re-tag in finding" report): the scan must resolve DB paths the same way the
|
||||||
|
# apply handler does, never emit an empty finding, and give unmatched tracks
|
||||||
|
# the album art.
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
def test_scan_resolves_mapped_paths_instead_of_skipping(tmp_path, monkeypatch):
|
||||||
|
"""DB stores a container path the scan process can't see directly; the
|
||||||
|
resolver maps it to the real file. Before the fix the bare isfile() check
|
||||||
|
dropped every track and cover-mode scans produced unappliable 0-track
|
||||||
|
findings."""
|
||||||
|
real = tmp_path / 'track.flac'; real.write_bytes(b'')
|
||||||
|
raw = '/container/music/track.flac' # not a real path here
|
||||||
|
conn = _db_with_album(str(tmp_path / 'm.db'), raw, current_title='Old Title')
|
||||||
|
ctx = _context(conn, {'mode': 'overwrite', 'cover_art': 'replace', 'source': 'spotify'})
|
||||||
|
_patch_source(monkeypatch, {
|
||||||
|
'title': 'Old Title', 'album_artist': 'Real Artist', 'album': 'Real Album',
|
||||||
|
'year': '2021', 'genre': 'Rock', 'track_number': 1, 'disc_number': 1,
|
||||||
|
})
|
||||||
|
monkeypatch.setattr(lr, 'resolve_library_file_path',
|
||||||
|
lambda p, **k: str(real) if p == raw else None)
|
||||||
|
|
||||||
|
result = lr.LibraryRetagJob().scan(ctx)
|
||||||
|
|
||||||
|
assert result.findings_created == 1
|
||||||
|
tracks = ctx.findings[0]['details']['tracks']
|
||||||
|
assert len(tracks) == 1
|
||||||
|
assert tracks[0]['file_path'] == str(real) # plan carries the RESOLVED path
|
||||||
|
|
||||||
|
|
||||||
|
def test_cover_scan_with_no_reachable_tracks_creates_no_finding(tmp_path, monkeypatch):
|
||||||
|
"""Cover action set but no track resolvable: skip the album entirely.
|
||||||
|
The old behavior created a '(0 track(s))' finding whose apply always
|
||||||
|
failed with 'No tracks to re-tag in finding'."""
|
||||||
|
conn = _db_with_album(str(tmp_path / 'm.db'), '/container/music/gone.flac')
|
||||||
|
ctx = _context(conn, {'mode': 'overwrite', 'cover_art': 'replace', 'source': 'spotify'})
|
||||||
|
_patch_source(monkeypatch, {'title': 'Old Title'})
|
||||||
|
monkeypatch.setattr(lr, 'resolve_library_file_path', lambda p, **k: None)
|
||||||
|
|
||||||
|
result = lr.LibraryRetagJob().scan(ctx)
|
||||||
|
|
||||||
|
assert ctx.findings == []
|
||||||
|
assert result.findings_created == 0
|
||||||
|
assert result.skipped == 1
|
||||||
|
|
||||||
|
|
||||||
|
def test_cover_scan_includes_unmatched_tracks_as_art_only(tmp_path, monkeypatch):
|
||||||
|
"""A track with no source match can't be re-tagged, but album cover art
|
||||||
|
still applies to it — cover-mode scans include an art-only plan (empty
|
||||||
|
db_data) and the finding title says 'cover art', not '(0 track(s))'."""
|
||||||
|
track = tmp_path / 'track.flac'; track.write_bytes(b'')
|
||||||
|
conn = _db_with_album(str(tmp_path / 'm.db'), str(track), current_title='Old Title')
|
||||||
|
ctx = _context(conn, {'mode': 'overwrite', 'cover_art': 'replace', 'source': 'spotify'})
|
||||||
|
_patch_source(monkeypatch, {'title': 'Old Title'})
|
||||||
|
# Source tracklist that matches NOTHING in the library.
|
||||||
|
monkeypatch.setattr(lr, 'get_album_tracks_for_source',
|
||||||
|
lambda s, i: [{'name': 'Zzz Unrelated Song', 'track_number': 9,
|
||||||
|
'disc_number': 9, 'id': 'zz'}])
|
||||||
|
|
||||||
|
result = lr.LibraryRetagJob().scan(ctx)
|
||||||
|
|
||||||
|
assert result.findings_created == 1
|
||||||
|
f = ctx.findings[0]
|
||||||
|
assert 'cover art' in f['title']
|
||||||
|
tracks = f['details']['tracks']
|
||||||
|
assert len(tracks) == 1
|
||||||
|
assert not tracks[0]['changes'] and tracks[0]['db_data'] == {} # art-only plan
|
||||||
|
|
||||||
|
|
||||||
|
def test_apply_art_only_plan_embeds_cover(tmp_path, monkeypatch):
|
||||||
|
"""The art-only plans the cover-mode scan now emits (empty db_data) must go
|
||||||
|
through apply_track_plans as a WRITE (cover embed), not a skip/failure."""
|
||||||
|
track = tmp_path / 'track.flac'; track.write_bytes(b'')
|
||||||
|
calls = []
|
||||||
|
monkeypatch.setattr('core.tag_writer.download_cover_art',
|
||||||
|
lambda url: (b'img-bytes', 'image/jpeg'))
|
||||||
|
monkeypatch.setattr('core.tag_writer.write_tags_to_file',
|
||||||
|
lambda fp, db_data, **k: calls.append((fp, db_data, k)) or {'success': True})
|
||||||
|
|
||||||
|
res = lr.apply_track_plans(
|
||||||
|
[{'file_path': str(track), 'db_data': {}}],
|
||||||
|
cover_action='replace', cover_url='http://art/cover.jpg',
|
||||||
|
)
|
||||||
|
|
||||||
|
assert res['written'] == 1 and res['failed'] == 0
|
||||||
|
fp, db_data, kwargs = calls[0]
|
||||||
|
assert db_data == {} and kwargs['embed_cover'] is True
|
||||||
|
assert kwargs['cover_data'] == (b'img-bytes', 'image/jpeg')
|
||||||
|
assert res['cover_written'] is True # cover.jpg written next to the track
|
||||||
|
|
||||||
|
|
||||||
|
def test_apply_art_only_plan_skips_when_cover_download_fails(tmp_path, monkeypatch):
|
||||||
|
"""If the cover can't be downloaded there's nothing to write for an
|
||||||
|
art-only plan — it must count as skipped, never failed."""
|
||||||
|
track = tmp_path / 'track.flac'; track.write_bytes(b'')
|
||||||
|
monkeypatch.setattr('core.tag_writer.download_cover_art',
|
||||||
|
lambda url: (_ for _ in ()).throw(RuntimeError('net down')))
|
||||||
|
monkeypatch.setattr('core.tag_writer.write_tags_to_file',
|
||||||
|
lambda fp, db_data, **k: {'success': True})
|
||||||
|
|
||||||
|
res = lr.apply_track_plans(
|
||||||
|
[{'file_path': str(track), 'db_data': {}}],
|
||||||
|
cover_action='replace', cover_url='http://art/cover.jpg',
|
||||||
|
)
|
||||||
|
|
||||||
|
assert res == {'written': 0, 'failed': 0, 'skipped': 1, 'cover_written': False}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue