#889: fix replace-delete (resolve path) + re-identify now inherits album year/track#
Two real bugs surfaced in testing: 1. Original file not deleted on replace: delete_replaced_track checked os.path.exists on the RAW stored DB path (a Docker/media-server view), so it removed the row but orphaned the file. Now takes a resolve_fn (wired to resolve_library_file_path in the worker) and unlinks the RESOLVED real path. 2. No year / wrong album context: build_identification_from_hint set is_single=True, routing re-identify through _match_tracks' singles fast-path — which never fetches the chosen album, so the re-import got a bare stub (no release_date, total_tracks=1). Added force_album_match so the matcher FETCHES the chosen release even for a lone staged file → the track inherits the real album's year, in-album track number, and art. Holds for single-type releases too (they have a year as well). Normal single-import behavior unchanged (force_album_match absent → same path). 112 auto-import + rematch tests green.
This commit is contained in:
parent
9a36d6f70b
commit
1367108e02
3 changed files with 71 additions and 11 deletions
|
|
@ -1059,10 +1059,20 @@ class AutoImportWorker:
|
|||
failure is logged, never raised, since the re-import already succeeded."""
|
||||
try:
|
||||
from core.imports.rematch_hints import consume_hint, delete_replaced_track
|
||||
|
||||
def _resolve_old(stored):
|
||||
# The old row's path is a STORED path (Docker/media-server view) — map
|
||||
# it to a file this process can actually unlink, same as everywhere else.
|
||||
try:
|
||||
from core.library.path_resolver import resolve_library_file_path
|
||||
return resolve_library_file_path(stored, config_manager=getattr(self, '_config_manager', None))
|
||||
except Exception:
|
||||
return None
|
||||
|
||||
conn = self.database._get_connection()
|
||||
try:
|
||||
cursor = conn.cursor()
|
||||
removed = delete_replaced_track(cursor, hint.replace_track_id)
|
||||
removed = delete_replaced_track(cursor, hint.replace_track_id, resolve_fn=_resolve_old)
|
||||
consume_hint(cursor, hint.id)
|
||||
conn.commit()
|
||||
finally:
|
||||
|
|
@ -1500,8 +1510,11 @@ class AutoImportWorker:
|
|||
|
||||
def _match_tracks(self, candidate: FolderCandidate, identification: Dict) -> Optional[Dict]:
|
||||
"""Match staging files to the identified album's tracklist."""
|
||||
# Singles: no album tracklist to match against — the file IS the match
|
||||
if candidate.is_single or identification.get('is_single'):
|
||||
# Singles: no album tracklist to match against — the file IS the match.
|
||||
# force_album_match (set by a re-identify hint) overrides this: even a lone
|
||||
# staged file is matched INTO the chosen album, so it inherits the album's
|
||||
# year / track number / art instead of the bare singles stub (#889).
|
||||
if not identification.get('force_album_match') and (candidate.is_single or identification.get('is_single')):
|
||||
conf = identification.get('identification_confidence', 0.7)
|
||||
track_data = {
|
||||
'name': identification.get('track_name', identification.get('album_name', '')),
|
||||
|
|
|
|||
|
|
@ -28,7 +28,7 @@ from __future__ import annotations
|
|||
|
||||
import os
|
||||
from dataclasses import dataclass
|
||||
from typing import Any, Optional
|
||||
from typing import Any, Callable, Optional
|
||||
|
||||
# Columns in INSERT/SELECT order — single source of truth so the dataclass, the
|
||||
# write, and the read can't drift apart.
|
||||
|
|
@ -212,12 +212,23 @@ def build_identification_from_hint(hint: RematchHint) -> dict:
|
|||
"source": hint.source,
|
||||
"method": "rematch_hint",
|
||||
"identification_confidence": 1.0,
|
||||
"is_single": True,
|
||||
# is_single reflects the CHOSEN release, but force_album_match makes the
|
||||
# matcher FETCH that release (even for a lone staged file) instead of taking
|
||||
# the singles fast-path — so the re-imported track gets the real album
|
||||
# metadata: year, the correct in-album track number, and the album art.
|
||||
"is_single": (str(hint.album_type or "").lower() == "single"),
|
||||
"force_album_match": True,
|
||||
"album_type": hint.album_type,
|
||||
}
|
||||
|
||||
|
||||
def delete_replaced_track(cursor: Any, replace_track_id: Any, *, unlink=os.remove) -> Optional[str]:
|
||||
def delete_replaced_track(
|
||||
cursor: Any,
|
||||
replace_track_id: Any,
|
||||
*,
|
||||
unlink=os.remove,
|
||||
resolve_fn: Optional[Callable[[str], Optional[str]]] = None,
|
||||
) -> Optional[str]:
|
||||
"""Remove the OLD library row a re-identify replaces, and its file.
|
||||
|
||||
Called only AFTER the re-import has landed the track at its new home, so the
|
||||
|
|
@ -226,7 +237,12 @@ def delete_replaced_track(cursor: Any, replace_track_id: Any, *, unlink=os.remov
|
|||
yanking a file a different row legitimately points to). Returns the path it
|
||||
removed, or ``None`` if there was nothing to do. ``unlink`` is injectable for
|
||||
tests. Assumes the new home differs from the old — the Re-identify modal never
|
||||
offers the track's current release as a target, so this holds."""
|
||||
offers the track's current release as a target, so this holds.
|
||||
|
||||
``resolve_fn`` maps the STORED DB path to the file's actual on-disk location
|
||||
before the exists/unlink check — essential because the stored path may be a
|
||||
Docker/media-server view this process can't read literally (without it we'd
|
||||
delete the row but orphan the file)."""
|
||||
if not replace_track_id:
|
||||
return None
|
||||
cursor.execute("SELECT file_path FROM tracks WHERE id = ?", (replace_track_id,))
|
||||
|
|
@ -239,14 +255,22 @@ def delete_replaced_track(cursor: Any, replace_track_id: Any, *, unlink=os.remov
|
|||
|
||||
if not old_path:
|
||||
return None
|
||||
# Only unlink if no surviving row still points at this file.
|
||||
# Only unlink if no surviving row still points at this file (rows store the
|
||||
# stored path, so compare against the stored path, not the resolved one).
|
||||
cursor.execute("SELECT 1 FROM tracks WHERE file_path = ? LIMIT 1", (old_path,))
|
||||
if cursor.fetchone() is not None:
|
||||
return None
|
||||
# Resolve to the real on-disk path before touching the filesystem.
|
||||
real_path = old_path
|
||||
if resolve_fn is not None:
|
||||
try:
|
||||
real_path = resolve_fn(old_path) or old_path
|
||||
except Exception:
|
||||
real_path = old_path
|
||||
try:
|
||||
if os.path.exists(old_path):
|
||||
unlink(old_path)
|
||||
return old_path
|
||||
if os.path.exists(real_path):
|
||||
unlink(real_path)
|
||||
return real_path
|
||||
except OSError:
|
||||
pass
|
||||
return None
|
||||
|
|
|
|||
|
|
@ -69,7 +69,18 @@ def test_build_identification_maps_hint_fields():
|
|||
assert ident["track_number"] == 5
|
||||
assert ident["method"] == "rematch_hint"
|
||||
assert ident["identification_confidence"] == 1.0
|
||||
# album_type 'album' → not a single, and force_album_match makes the matcher
|
||||
# fetch the real album (year/track#/art) instead of the singles stub.
|
||||
assert ident["is_single"] is False
|
||||
assert ident["force_album_match"] is True
|
||||
|
||||
|
||||
def test_build_identification_single_release_still_forces_album_fetch():
|
||||
# Even a chosen SINGLE release is fetched (it has a year too); is_single flags
|
||||
# the type, force_album_match drives the album path regardless.
|
||||
ident = build_identification_from_hint(_hint(album_type="single"))
|
||||
assert ident["is_single"] is True
|
||||
assert ident["force_album_match"] is True
|
||||
|
||||
|
||||
# ── pure: safe replacement ────────────────────────────────────────────────────
|
||||
|
|
@ -101,6 +112,18 @@ def test_delete_replaced_track_noops_on_missing_id(conn):
|
|||
assert delete_replaced_track(cur, 999) is None # no such row
|
||||
|
||||
|
||||
def test_delete_replaced_track_resolves_path_before_unlink(conn):
|
||||
# The stored path is a server/Docker view this process can't read literally;
|
||||
# resolve_fn maps it to the real file so we unlink the RIGHT path (not orphan it).
|
||||
cur = conn.cursor()
|
||||
cur.execute("INSERT INTO tracks (id, file_path) VALUES (7, '/mnt/serverview/Song.flac')")
|
||||
removed = []
|
||||
out = delete_replaced_track(cur, 7, unlink=lambda p: removed.append(p),
|
||||
resolve_fn=lambda stored: '/real/local/Song.flac')
|
||||
assert out == '/real/local/Song.flac'
|
||||
assert removed == ['/real/local/Song.flac'] # unlinked the RESOLVED path
|
||||
|
||||
|
||||
# patch os.path.exists so the unlink branch is reachable without real files
|
||||
@pytest.fixture(autouse=True)
|
||||
def _exists(monkeypatch):
|
||||
|
|
|
|||
Loading…
Reference in a new issue