diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index fcb9638b..5e87934d 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -9,9 +9,9 @@ on: workflow_dispatch: inputs: version_tag: - description: 'Version tag (e.g. 2.5.2)' + description: 'Version tag (e.g. 2.5.3)' required: true - default: '2.5.2' + default: '2.5.3' jobs: build-and-push: diff --git a/core/deezer_client.py b/core/deezer_client.py index e564663c..07162228 100644 --- a/core/deezer_client.py +++ b/core/deezer_client.py @@ -89,6 +89,33 @@ def _upgrade_deezer_cover_url(url: str, target_size: int = _DEEZER_MAX_COVER_SIZ return _DEEZER_CDN_SIZE_PATTERN.sub(f'/{target_size}x{target_size}-', url, count=1) +def _is_full_track_payload(payload: Optional[Dict[str, Any]]) -> bool: + """Distinguish a full `/track/` cache hit from partial album-tracks data. + + Three Deezer endpoints feed the per-track cache: + - `/track/` — full record, includes both `track_position` AND + `contributors` (the multi-artist list the contributors-upgrade + path reads). + - `/album//tracks` — partial; includes `track_position` but + omits `contributors`. + - `/search/track` — minimal; lacks `track_position`. + + Pre-fix `get_track_details` only checked `track_position`, so + partial album-tracks payloads were treated as full hits and the + contributors-upgrade silently fell back to single-artist tagging + whenever an album had been fetched before its individual tracks + were post-processed (issue #588). + + `contributors` key presence is the load-bearing distinction — + `[]` is a valid value for genuinely single-artist tracks fetched + via the per-track endpoint, so test for key membership not + truthiness. + """ + if not isinstance(payload, dict): + return False + return 'track_position' in payload and 'contributors' in payload + + # ==================== Dataclasses (match iTunesClient / SpotifyClient format) ==================== @dataclass @@ -546,14 +573,9 @@ class DeezerClient: """Get detailed track info — returns Spotify-compatible dict (metadata source interface)""" cache = get_metadata_cache() cached = cache.get_entity('deezer', 'track', str(track_id)) - if cached and cached.get('title'): - # Search results are cached with minimal data (no track_position). - # Only use cache if it has track_position — the key field from /track/{id}. - # Search results include 'isrc' and 'release_date' but NOT track_position, - # so those fields alone are not sufficient to distinguish full from partial data. - if 'track_position' in cached: - return self._build_enhanced_track(cached) - # Otherwise fall through to fetch full data from API + if cached and cached.get('title') and _is_full_track_payload(cached): + return self._build_enhanced_track(cached) + # Otherwise fall through to fetch full data from API data = self._api_get(f'track/{track_id}') if not data: diff --git a/core/downloads/master.py b/core/downloads/master.py index 4620a467..40a29ab2 100644 --- a/core/downloads/master.py +++ b/core/downloads/master.py @@ -174,14 +174,33 @@ def run_full_missing_tracks_process(batch_id, playlist_id, tracks_json, deps: Ma elif album_tracks_map: # Album-scoped matching: check against known album tracks first track_name_lower = track_name.lower().strip() - # Direct title match + # Issue #589 — strip suffixes that just repeat the album + # context (e.g. "Shy Away (MTV Unplugged Live)" on a + # "MTV Unplugged" album → "Shy Away") so album-owned + # tracks don't false-miss when the local DB stored the + # base title. Only fires inside the album-confirmed + # scope; global matching elsewhere is unchanged. + from core.matching.album_context_title import strip_redundant_album_suffix + _album_name_for_strip = (batch_album_context or {}).get('name', '') + _normalized_source_title = strip_redundant_album_suffix( + track_name, _album_name_for_strip + ).lower().strip() + # Direct title match (try both raw and normalized) if track_name_lower in album_tracks_map: found, confidence = True, 1.0 + elif _normalized_source_title and _normalized_source_title in album_tracks_map: + found, confidence = True, 1.0 else: - # Fuzzy match against album tracks using string similarity + # Fuzzy match against album tracks using string similarity. + # Compare BOTH the raw and normalized source titles — + # whichever scores higher wins. Preserves strict + # matching when the album doesn't imply version + # context (helper returns the input unchanged). best_sim = 0.0 for db_title_lower, _db_track in album_tracks_map.items(): - sim = db._string_similarity(track_name_lower, db_title_lower) + sim_raw = db._string_similarity(track_name_lower, db_title_lower) + sim_norm = db._string_similarity(_normalized_source_title, db_title_lower) if _normalized_source_title else 0.0 + sim = max(sim_raw, sim_norm) if sim > best_sim: best_sim = sim if best_sim >= 0.7: diff --git a/core/imports/file_integrity.py b/core/imports/file_integrity.py index 82ced0d1..a0ebb18a 100644 --- a/core/imports/file_integrity.py +++ b/core/imports/file_integrity.py @@ -52,6 +52,37 @@ _DEFAULT_LENGTH_TOLERANCE_S = 3.0 _LENGTH_TOLERANCE_LONG_TRACK_S = 5.0 _LONG_TRACK_THRESHOLD_S = 600.0 # 10 minutes +# Upper bound for the user-configurable override. Anything past 60s +# means the check is effectively off — cap defends against accidental +# nonsense like 9999 making logs misleading. Users who genuinely want +# to disable the check can set 60. +_MAX_USER_TOLERANCE_S = 60.0 + + +def resolve_duration_tolerance(value: Any) -> Optional[float]: + """Coerce a user-configured tolerance value to a float override. + + Returns: + - None when value is missing / 0 / negative / unparseable, so + callers fall back to the auto-scaled defaults (3s/5s). + - float in (0, _MAX_USER_TOLERANCE_S] when value is a positive + numeric string or float — clamped to the upper bound. + + Pure helper. No I/O. Drives the `length_tolerance_s` override on + `check_audio_integrity`. + """ + if value is None: + return None + try: + parsed = float(value) + except (TypeError, ValueError): + return None + if parsed <= 0: + return None + if parsed > _MAX_USER_TOLERANCE_S: + return _MAX_USER_TOLERANCE_S + return parsed + @dataclass class IntegrityResult: diff --git a/core/imports/guards.py b/core/imports/guards.py index 657576f0..f4519a5d 100644 --- a/core/imports/guards.py +++ b/core/imports/guards.py @@ -29,8 +29,22 @@ def _get_config_manager(): return config_manager -def move_to_quarantine(file_path: str, context: dict, reason: str, automation_engine=None) -> str: - """Move a file to the quarantine folder and write a metadata sidecar.""" +def move_to_quarantine(file_path: str, context: dict, reason: str, automation_engine=None, *, trigger: str = "unknown") -> str: + """Move a file to the quarantine folder and write a metadata sidecar. + + `trigger` identifies which check fired (`integrity` / `acoustid` / + `bit_depth` / `unknown`) and is persisted in the sidecar so + one-click Approve can set the matching `_skip_quarantine_check` + bypass when re-running the pipeline. + + Sidecar also persists a JSON-safe snapshot of the full `context` + dict via `serialize_quarantine_context`, enabling in-place approve + without losing the matched-track metadata. Legacy sidecars (written + before this expansion) lack the `context` field — Approve falls + back to `recover_to_staging` for those. + """ + from core.imports.quarantine import serialize_quarantine_context + download_dir = _get_config_manager().get("soulseek.download_path", "./downloads") quarantine_dir = Path(download_dir) / "ss_quarantine" quarantine_dir.mkdir(parents=True, exist_ok=True) @@ -56,6 +70,8 @@ def move_to_quarantine(file_path: str, context: dict, reason: str, automation_en "expected_track": get_import_clean_title(context, default=original_search.get("title", "Unknown")), "expected_artist": get_import_clean_artist(context, default=(artist_context.get("name", "") if isinstance(artist_context, dict) else "Unknown")), "context_key": context.get("context_key", "unknown"), + "trigger": trigger, + "context": serialize_quarantine_context(context), } try: diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index d7b2b7d0..912ebd8d 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -32,7 +32,7 @@ from core.imports.context import ( get_import_track_info, normalize_import_context, ) -from core.imports.file_integrity import check_audio_integrity +from core.imports.file_integrity import check_audio_integrity, resolve_duration_tolerance from core.imports.filename import extract_track_number_from_filename from core.imports.guards import check_flac_bit_depth, move_to_quarantine from core.imports.side_effects import ( @@ -155,11 +155,31 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta except Exception: _expected_duration_ms = None - try: - integrity = check_audio_integrity(file_path, _expected_duration_ms) - except Exception as integrity_error: - logger.error(f"[Integrity] Check raised unexpectedly (continuing): {integrity_error}") + # User-configurable tolerance override. None = use built-in + # auto-scaled defaults (3s normal / 5s for tracks >10min). Set + # higher (e.g. 10) when matched files routinely drift from the + # source's reported duration (live recordings, alternate + # masterings, etc). + _duration_tolerance_override = resolve_duration_tolerance( + config_manager.get('post_processing.duration_tolerance_seconds', 0) + ) + # Per-check quarantine bypass — set by `approve_quarantine_entry` + # when the user explicitly approves a previously-quarantined + # file. Skips ONLY the named check; other gates still run. + _bypass_check = context.get('_skip_quarantine_check') + if _bypass_check == 'integrity': + logger.info(f"[Integrity] Skipped (user approval) for {_basename}") integrity = None + else: + try: + integrity = check_audio_integrity( + file_path, + _expected_duration_ms, + length_tolerance_s=_duration_tolerance_override, + ) + except Exception as integrity_error: + logger.error(f"[Integrity] Check raised unexpectedly (continuing): {integrity_error}") + integrity = None if integrity is not None and not integrity.ok: logger.error(f"[Integrity] Rejected {_basename}: {integrity.reason}") @@ -171,6 +191,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta context, f"Integrity check failed: {integrity.reason}", automation_engine, + trigger='integrity', ) logger.error(f"File quarantined due to integrity failure: {quarantine_path}") except Exception as quarantine_error: @@ -206,7 +227,9 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta f"drift={integrity.checks.get('length_drift_s', 'n/a')})" ) - _skip_acoustid = False + _skip_acoustid = context.get('_skip_quarantine_check') == 'acoustid' + if _skip_acoustid: + logger.info(f"[AcoustID] Skipped (user approval) for {_basename}") try: from core.acoustid_verification import AcoustIDVerification, VerificationResult @@ -248,6 +271,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta context, verification_msg, automation_engine, + trigger='acoustid', ) logger.error(f"File quarantined due to verification failure: {quarantine_path}") except Exception as quarantine_error: @@ -420,7 +444,9 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta if context['_audio_quality']: logger.info(f"Audio quality detected: {context['_audio_quality']}") - rejection_reason = check_flac_bit_depth(file_path, context) + rejection_reason = None if context.get('_skip_quarantine_check') == 'bit_depth' else check_flac_bit_depth(file_path, context) + if context.get('_skip_quarantine_check') == 'bit_depth': + logger.info(f"[BitDepth] Skipped (user approval) for {_basename}") if rejection_reason: try: quarantine_path = move_to_quarantine( @@ -428,6 +454,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta context, rejection_reason, automation_engine, + trigger='bit_depth', ) logger.info(f"File quarantined due to bit depth filter: {quarantine_path}") except Exception as quarantine_error: @@ -548,7 +575,9 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta if context['_audio_quality']: logger.info(f"Audio quality detected: {context['_audio_quality']}") - rejection_reason = check_flac_bit_depth(file_path, context) + rejection_reason = None if context.get('_skip_quarantine_check') == 'bit_depth' else check_flac_bit_depth(file_path, context) + if context.get('_skip_quarantine_check') == 'bit_depth': + logger.info(f"[BitDepth] Skipped (user approval) for {_basename}") if rejection_reason: try: quarantine_path = move_to_quarantine( @@ -556,6 +585,7 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta context, rejection_reason, automation_engine, + trigger='bit_depth', ) logger.info(f"File quarantined due to bit depth filter: {quarantine_path}") except Exception as quarantine_error: diff --git a/core/imports/quarantine.py b/core/imports/quarantine.py new file mode 100644 index 00000000..7c4488c6 --- /dev/null +++ b/core/imports/quarantine.py @@ -0,0 +1,329 @@ +"""Quarantine entry management — pure helpers for list/delete/approve/recover. + +Quarantined files live in `/ss_quarantine/` as +`_..quarantined` paired with a JSON sidecar +`_.json` written by `core.imports.guards.move_to_quarantine`. + +This module provides the read/write/restore primitives. Web routes are +thin glue around these. Pipeline re-run on approval is the caller's +job (we hand back `(file_path, context, bypass_check)`). +""" + +from __future__ import annotations + +import json +import os +import shutil +from pathlib import Path +from typing import Any, Dict, List, Optional, Tuple + +from utils.logging_config import get_logger + +logger = get_logger("imports.quarantine") + + +_QUARANTINE_SUFFIX = ".quarantined" + + +# JSON-serializable scalar predicate. dict / list values get walked +# recursively; anything else is dropped during sidecar serialization. +_SAFE_SCALARS = (str, int, float, bool, type(None)) + + +def serialize_quarantine_context(context: Any) -> Dict[str, Any]: + """Walk a context dict and emit a JSON-safe copy. + + Drops non-serializable values (sets, custom objects, callables, + open file handles, etc) silently — sidecar must round-trip through + `json.dump` / `json.load` without raising. Lists are walked element + by element; dicts are walked recursively. Anything that isn't a + scalar / dict / list is converted to a string fallback so caller + still sees *something* (rather than a silent drop) but won't break + the JSON write. + """ + if not isinstance(context, dict): + return {} + return _coerce_dict(context) + + +def _coerce_value(value: Any) -> Any: + if isinstance(value, _SAFE_SCALARS): + return value + if isinstance(value, dict): + return _coerce_dict(value) + if isinstance(value, (list, tuple)): + return [_coerce_value(v) for v in value] + if isinstance(value, set): + return [_coerce_value(v) for v in value] + # Fallback — preserve via str() so caller sees the value's shape + # without breaking JSON serialization. + try: + return str(value) + except Exception: + return None + + +def _coerce_dict(d: Dict[str, Any]) -> Dict[str, Any]: + out: Dict[str, Any] = {} + for key, value in d.items(): + if not isinstance(key, str): + try: + key = str(key) + except Exception: + continue + out[key] = _coerce_value(value) + return out + + +def _entry_id_from_filename(quarantined_filename: str) -> str: + """Derive a stable entry id from the quarantined filename. + + Strip the `.quarantined` suffix; strip the original file extension; + return the bare `_` stem. Sidecar uses the + same stem with a `.json` extension, so the id pairs both sides. + """ + base = quarantined_filename + if base.endswith(_QUARANTINE_SUFFIX): + base = base[: -len(_QUARANTINE_SUFFIX)] + return Path(base).stem + + +def list_quarantine_entries(quarantine_dir: str) -> List[Dict[str, Any]]: + """Enumerate quarantined files paired with their sidecars. + + Returns one dict per `.quarantined` file with: id, filename, + original_filename (from sidecar), reason, expected_track, + expected_artist, timestamp, size_bytes, has_full_context (True + when the sidecar carries a `context` field — required for one-click + Approve), trigger (which check fired: integrity / acoustid / + bit_depth / unknown). + + Orphaned `.quarantined` files (no sidecar) still surface — caller + can delete them. Orphaned sidecars (no file) are skipped silently. + Sorted newest-first by timestamp prefix. + """ + entries: List[Dict[str, Any]] = [] + if not os.path.isdir(quarantine_dir): + return entries + + for name in os.listdir(quarantine_dir): + if not name.endswith(_QUARANTINE_SUFFIX): + continue + full_path = os.path.join(quarantine_dir, name) + if not os.path.isfile(full_path): + continue + + entry_id = _entry_id_from_filename(name) + sidecar_path = os.path.join(quarantine_dir, f"{entry_id}.json") + sidecar: Dict[str, Any] = {} + if os.path.isfile(sidecar_path): + try: + with open(sidecar_path, encoding="utf-8") as f: + loaded = json.load(f) + if isinstance(loaded, dict): + sidecar = loaded + except Exception as exc: + logger.debug("sidecar read failed for %s: %s", entry_id, exc) + + try: + size_bytes = os.path.getsize(full_path) + except OSError: + size_bytes = 0 + + entries.append( + { + "id": entry_id, + "filename": name, + "original_filename": sidecar.get("original_filename", name), + "reason": sidecar.get("quarantine_reason", "Unknown reason"), + "expected_track": sidecar.get("expected_track", ""), + "expected_artist": sidecar.get("expected_artist", ""), + "timestamp": sidecar.get("timestamp", ""), + "size_bytes": size_bytes, + "has_full_context": isinstance(sidecar.get("context"), dict), + "trigger": sidecar.get("trigger", "unknown"), + } + ) + + entries.sort(key=lambda e: e["id"], reverse=True) + return entries + + +def _resolve_entry_paths(quarantine_dir: str, entry_id: str) -> Tuple[Optional[str], Optional[str]]: + """Locate the `.quarantined` file + JSON sidecar for an entry id. + + Returns (file_path, sidecar_path), either may be None if missing. + """ + if not os.path.isdir(quarantine_dir) or not entry_id: + return None, None + file_path: Optional[str] = None + for name in os.listdir(quarantine_dir): + if not name.endswith(_QUARANTINE_SUFFIX): + continue + if _entry_id_from_filename(name) == entry_id: + file_path = os.path.join(quarantine_dir, name) + break + sidecar_path = os.path.join(quarantine_dir, f"{entry_id}.json") + if not os.path.isfile(sidecar_path): + sidecar_path = None + return file_path, sidecar_path + + +def delete_quarantine_entry(quarantine_dir: str, entry_id: str) -> bool: + """Delete the quarantined file + sidecar for the given entry id. + + Returns True if at least one of the two was removed. False when + neither existed (entry already gone). + """ + file_path, sidecar_path = _resolve_entry_paths(quarantine_dir, entry_id) + removed = False + if file_path and os.path.isfile(file_path): + try: + os.remove(file_path) + removed = True + except OSError as exc: + logger.error("Failed to delete quarantine file %s: %s", file_path, exc) + if sidecar_path and os.path.isfile(sidecar_path): + try: + os.remove(sidecar_path) + removed = True + except OSError as exc: + logger.error("Failed to delete quarantine sidecar %s: %s", sidecar_path, exc) + return removed + + +def _restore_filename(quarantined_filename: str, sidecar_original: Optional[str] = None) -> str: + """Resolve the filename to restore. + + Sidecar's `original_filename` wins when provided — it's the + canonical record of what the file was named before quarantine. + Otherwise parse the `_..quarantined` + convention written by `move_to_quarantine`, dropping the timestamp + prefix and `.quarantined` suffix. Final fallback returns the + quarantined filename minus the suffix unchanged. + """ + if sidecar_original: + return sidecar_original + base = quarantined_filename + if base.endswith(_QUARANTINE_SUFFIX): + base = base[: -len(_QUARANTINE_SUFFIX)] + parts = base.split("_", 2) + if len(parts) >= 3 and parts[0].isdigit() and parts[1].isdigit(): + return parts[2] + return base + + +def approve_quarantine_entry( + quarantine_dir: str, + entry_id: str, + restore_dir: str, +) -> Optional[Tuple[str, Dict[str, Any], str]]: + """Restore a quarantined file for re-import via the post-process pipeline. + + Reads the sidecar's `context` + `trigger`, moves the file out of + quarantine to `restore_dir` (with the original filename + extension), + deletes the sidecar. + + Returns `(restored_file_path, context, trigger)` so the caller can + set the appropriate `_skip_quarantine_check` bypass flag and + dispatch the post-process pipeline. + + Returns None when: + - the entry doesn't exist + - the sidecar lacks a serialized `context` (legacy thin sidecar + — caller should fall back to `recover_to_staging` instead) + - the file move fails + """ + file_path, sidecar_path = _resolve_entry_paths(quarantine_dir, entry_id) + if not file_path or not sidecar_path: + logger.warning("approve: entry %s missing file or sidecar", entry_id) + return None + + try: + with open(sidecar_path, encoding="utf-8") as f: + sidecar = json.load(f) + except Exception as exc: + logger.error("approve: sidecar read failed for %s: %s", entry_id, exc) + return None + + context = sidecar.get("context") + if not isinstance(context, dict): + logger.info("approve: entry %s has thin sidecar (no context) — caller should recover-to-staging", entry_id) + return None + + trigger = str(sidecar.get("trigger", "unknown")) + + original_name = sidecar.get("original_filename") or _restore_filename(os.path.basename(file_path)) + os.makedirs(restore_dir, exist_ok=True) + restored_path = os.path.join(restore_dir, original_name) + restored_path = _ensure_unique_path(restored_path) + + try: + shutil.move(file_path, restored_path) + except OSError as exc: + logger.error("approve: failed to restore %s -> %s: %s", file_path, restored_path, exc) + return None + + try: + os.remove(sidecar_path) + except OSError as exc: + logger.warning("approve: failed to remove sidecar %s: %s", sidecar_path, exc) + + return restored_path, context, trigger + + +def recover_to_staging( + quarantine_dir: str, + staging_dir: str, + entry_id: str, +) -> Optional[str]: + """Move a quarantined file into Staging for manual import. + + Strips the timestamp prefix + `.quarantined` suffix, drops the file + into `staging_dir` so the user can finish via the existing Import + flow. Sidecar is removed. Used as the fallback path for legacy thin + sidecars (no embedded `context`) where one-click Approve is + impossible. + """ + file_path, sidecar_path = _resolve_entry_paths(quarantine_dir, entry_id) + if not file_path: + return None + + sidecar_original = None + if sidecar_path: + try: + with open(sidecar_path, encoding="utf-8") as f: + sidecar_original = json.load(f).get("original_filename") + except Exception as exc: + logger.debug("recover: sidecar read failed for %s: %s", entry_id, exc) + + restored_name = _restore_filename(os.path.basename(file_path), sidecar_original) + os.makedirs(staging_dir, exist_ok=True) + target = _ensure_unique_path(os.path.join(staging_dir, restored_name)) + + try: + shutil.move(file_path, target) + except OSError as exc: + logger.error("recover: failed to move %s -> %s: %s", file_path, target, exc) + return None + + if sidecar_path and os.path.isfile(sidecar_path): + try: + os.remove(sidecar_path) + except OSError as exc: + logger.warning("recover: failed to remove sidecar %s: %s", sidecar_path, exc) + + return target + + +def _ensure_unique_path(target: str) -> str: + """Append `_(2)`, `_(3)`, ... before the extension when target exists.""" + if not os.path.exists(target): + return target + base, ext = os.path.splitext(target) + counter = 2 + while True: + candidate = f"{base}_({counter}){ext}" + if not os.path.exists(candidate): + return candidate + counter += 1 diff --git a/core/imports/routes.py b/core/imports/routes.py new file mode 100644 index 00000000..57df3c7b --- /dev/null +++ b/core/imports/routes.py @@ -0,0 +1,515 @@ +"""Import/staging controller helpers for Flask-style endpoints.""" + +from __future__ import annotations + +import os +import uuid +from concurrent.futures import as_completed +from dataclasses import dataclass +from typing import Any, Callable, Dict + +from core.imports.album import build_album_import_context, build_album_import_match_payload, resolve_album_artist_context +from core.imports.context import get_import_context_artist, get_import_track_info, normalize_import_context +from core.imports.filename import parse_filename_metadata +from core.imports.staging import ( + AUDIO_EXTENSIONS, + get_import_suggestions_cache, + get_primary_source as _get_primary_source, + get_staging_path as _get_staging_path, + read_staging_file_metadata as _read_staging_file_metadata, + refresh_import_suggestions_cache as _refresh_import_suggestions_cache, + search_import_albums as _search_import_albums, + search_import_tracks as _search_import_tracks, +) +from utils.logging_config import get_logger + + +module_logger = get_logger("imports.routes") + + +def _default_read_tags(file_path: str): + from mutagen import File as MutagenFile + + return MutagenFile(file_path, easy=True) + + +def _get_single_track_import_context(*args, **kwargs): + from core.imports.resolution import get_single_track_import_context + + return get_single_track_import_context(*args, **kwargs) + + +@dataclass +class ImportRouteRuntime: + """Dependencies needed to service import/staging HTTP endpoints.""" + + get_staging_path: Callable[[], str] = _get_staging_path + read_staging_file_metadata: Callable[[str, str], Dict[str, Any]] = _read_staging_file_metadata + read_tags: Callable[[str], Any] = _default_read_tags + get_primary_source: Callable[[], str] = _get_primary_source + search_import_albums: Callable[..., list] = _search_import_albums + search_import_tracks: Callable[..., list] = _search_import_tracks + build_album_import_match_payload: Callable[..., Dict[str, Any]] = build_album_import_match_payload + resolve_album_artist_context: Callable[..., Any] = resolve_album_artist_context + build_album_import_context: Callable[..., Dict[str, Any]] = build_album_import_context + get_single_track_import_context: Callable[..., Dict[str, Any]] = _get_single_track_import_context + parse_filename_metadata: Callable[[str], Dict[str, Any]] = parse_filename_metadata + normalize_import_context: Callable[[Dict[str, Any]], Dict[str, Any]] = normalize_import_context + get_import_context_artist: Callable[[Dict[str, Any]], Dict[str, Any]] = get_import_context_artist + get_import_track_info: Callable[[Dict[str, Any]], Dict[str, Any]] = get_import_track_info + process_single_import_file: Callable[["ImportRouteRuntime", Dict[str, Any]], tuple[str, str]] | None = None + post_process_matched_download: Callable[[str, Dict[str, Any], str], Any] | None = None + add_activity_item: Callable[[Any, Any, Any, Any], Any] | None = None + refresh_import_suggestions_cache: Callable[[], Any] = _refresh_import_suggestions_cache + automation_engine: Any = None + hydrabase_worker: Any = None + dev_mode_enabled: bool = False + import_singles_executor: Any = None + logger: Any = module_logger + + +def staging_files(runtime: ImportRouteRuntime) -> tuple[Dict[str, Any], int]: + """Scan the staging folder and return audio files with tag metadata.""" + try: + staging_path = runtime.get_staging_path() + os.makedirs(staging_path, exist_ok=True) + + files = [] + for root, _dirs, filenames in os.walk(staging_path): + for fname in filenames: + ext = os.path.splitext(fname)[1].lower() + if ext not in AUDIO_EXTENSIONS: + continue + full_path = os.path.join(root, fname) + rel_path = os.path.relpath(full_path, staging_path) + + meta = runtime.read_staging_file_metadata(full_path, rel_path) + + files.append( + { + "filename": fname, + "rel_path": rel_path, + "full_path": full_path, + "title": meta["title"], + "artist": meta["albumartist"] or meta["artist"] or "Unknown Artist", + "album": meta["album"], + "track_number": meta["track_number"], + "disc_number": meta["disc_number"], + "extension": ext, + } + ) + + files.sort(key=lambda f: f["filename"].lower()) + return {"success": True, "files": files, "staging_path": staging_path}, 200 + except Exception as exc: + runtime.logger.error("Error scanning staging files: %s", exc) + return {"success": False, "error": str(exc)}, 500 + + +def staging_groups(runtime: ImportRouteRuntime) -> tuple[Dict[str, Any], int]: + """Auto-detect album groups from staging files based on their tags.""" + try: + staging_path = runtime.get_staging_path() + if not os.path.isdir(staging_path): + return {"success": True, "groups": []}, 200 + + album_groups = {} + for root, _dirs, filenames in os.walk(staging_path): + for fname in filenames: + ext = os.path.splitext(fname)[1].lower() + if ext not in AUDIO_EXTENSIONS: + continue + full_path = os.path.join(root, fname) + rel_path = os.path.relpath(full_path, staging_path) + + meta = runtime.read_staging_file_metadata(full_path, rel_path) + album = meta["album"] + artist = meta["albumartist"] or meta["artist"] + if not album or not artist: + continue + + key = (album.lower().strip(), artist.lower().strip()) + if key not in album_groups: + album_groups[key] = {"album": album.strip(), "artist": artist.strip(), "files": []} + album_groups[key]["files"].append( + { + "filename": fname, + "full_path": full_path, + "title": meta["title"], + "track_number": meta["track_number"], + } + ) + + groups = [] + for group in album_groups.values(): + if len(group["files"]) >= 2: + group["files"].sort(key=lambda f: f.get("track_number") or 999) + groups.append( + { + "album": group["album"], + "artist": group["artist"], + "file_count": len(group["files"]), + "files": group["files"], + "file_paths": [f["full_path"] for f in group["files"]], + } + ) + + groups.sort(key=lambda g: g["file_count"], reverse=True) + return {"success": True, "groups": groups}, 200 + except Exception as exc: + runtime.logger.error("Error building staging groups: %s", exc) + return {"success": False, "error": str(exc)}, 500 + + +def staging_hints(runtime: ImportRouteRuntime) -> tuple[Dict[str, Any], int]: + """Extract album search hints from staging folder tags and folder names.""" + try: + staging_path = runtime.get_staging_path() + if not os.path.isdir(staging_path): + return {"success": True, "hints": []}, 200 + + tag_albums = {} + folder_hints = {} + for root, _dirs, filenames in os.walk(staging_path): + audio_files = [f for f in filenames if os.path.splitext(f)[1].lower() in AUDIO_EXTENSIONS] + if not audio_files: + continue + + rel_dir = os.path.relpath(root, staging_path) + if rel_dir != ".": + top_folder = rel_dir.split(os.sep)[0] + folder_hints[top_folder] = folder_hints.get(top_folder, 0) + len(audio_files) + + for fname in audio_files: + full_path = os.path.join(root, fname) + try: + tags = runtime.read_tags(full_path) + if tags: + album = (tags.get("album") or [None])[0] + artist = (tags.get("artist") or (tags.get("albumartist") or [None]))[0] + if album: + key = (album.strip(), (artist or "").strip()) + tag_albums[key] = tag_albums.get(key, 0) + 1 + except Exception as exc: + runtime.logger.debug("tag read failed: %s", exc) + + queries = [] + seen_queries_lower = set() + + for (album, artist), _count in sorted(tag_albums.items(), key=lambda x: -x[1]): + query = f"{album} {artist}".strip() if artist else album + if query.lower() not in seen_queries_lower: + seen_queries_lower.add(query.lower()) + queries.append(query) + + for folder, _count in sorted(folder_hints.items(), key=lambda x: -x[1]): + query = folder.replace("_", " ") + if query.lower() not in seen_queries_lower: + seen_queries_lower.add(query.lower()) + queries.append(query) + + return {"success": True, "hints": queries[:5]}, 200 + except Exception as exc: + runtime.logger.error("Error getting staging hints: %s", exc) + return {"success": False, "error": str(exc)}, 500 + + +def staging_suggestions() -> tuple[Dict[str, Any], int]: + """Return cached import suggestions and readiness state.""" + cache = get_import_suggestions_cache() + return {"success": True, "suggestions": cache["suggestions"], "ready": cache["built"]}, 200 + + +def search_albums(runtime: ImportRouteRuntime, query: str, limit: int = 12) -> tuple[Dict[str, Any], int]: + """Search albums for manual import using the active metadata provider.""" + try: + query = (query or "").strip() + if not query: + return {"success": False, "error": "Missing query parameter"}, 400 + + limit = min(int(limit), 50) + if runtime.get_primary_source() == "hydrabase" and runtime.hydrabase_worker and runtime.dev_mode_enabled: + runtime.hydrabase_worker.enqueue(query, "albums") + + albums = runtime.search_import_albums(query, limit=limit) + return {"success": True, "albums": albums}, 200 + except Exception as exc: + runtime.logger.error("Error searching albums for import: %s", exc) + return {"success": False, "error": str(exc)}, 500 + + +def album_match(runtime: ImportRouteRuntime, data: Dict[str, Any]) -> tuple[Dict[str, Any], int]: + """Match staging files to an album's tracklist.""" + try: + data = data or {} + album_id = data.get("album_id") + album_name = data.get("album_name", "") + album_artist = data.get("album_artist", "") + source = str(data.get("source") or "").strip().lower() + filter_file_paths = set(data.get("file_paths", [])) + if not album_id: + return {"success": False, "error": "Missing album_id"}, 400 + + if not source: + runtime.logger.warning( + "[Import Match] Missing 'source' on album_id=%s - lookup will " + "guess via primary-source priority chain. If this fires " + "consistently, a frontend caller is dropping source from " + "the match POST body.", + album_id, + ) + + payload = runtime.build_album_import_match_payload( + album_id, + album_name=album_name, + album_artist=album_artist, + file_paths=filter_file_paths, + source=source or None, + ) + return payload, 200 + except Exception as exc: + runtime.logger.error("Error matching album for import: %s", exc) + return {"success": False, "error": str(exc)}, 500 + + +def album_process(runtime: ImportRouteRuntime, data: Dict[str, Any]) -> tuple[Dict[str, Any], int]: + """Process matched album files through the post-processing pipeline.""" + try: + data = data or {} + album = data.get("album", {}) + matches = data.get("matches", []) + + if not album or not matches: + return {"success": False, "error": "Missing album or matches data"}, 400 + if runtime.post_process_matched_download is None: + return {"success": False, "error": "Import post-processing not available"}, 500 + + processed = 0 + errors = [] + album_name = album.get("name", album.get("album_name", "Unknown Album")) + artist_name = album.get("artist", album.get("artist_name", "Unknown Artist")) + album_id = album.get("id", album.get("album_id", "")) + source = str(album.get("source") or data.get("source") or "").strip().lower() + + total_discs = max( + ( + match.get("track", {}).get("disc_number", 1) + for match in matches + if match.get("track") + ), + default=1, + ) + artist_context = runtime.resolve_album_artist_context(album, source=source) + + for match in matches: + staging_file = match.get("staging_file") + track = match.get("track") or {} + if not staging_file or not track: + continue + + file_path = staging_file.get("full_path", "") + if not os.path.isfile(file_path): + errors.append(f"File not found: {staging_file.get('filename', '?')}") + continue + + track_name = track.get("name", "Unknown Track") + track_number = track.get("track_number", 1) + context_key = f"import_album_{album_id}_{track_number}_{uuid.uuid4().hex[:8]}" + context = runtime.build_album_import_context( + album, + track, + artist_context=artist_context, + total_discs=total_discs, + source=source, + ) + + try: + runtime.post_process_matched_download(context_key, context, file_path) + processed += 1 + runtime.logger.info("Import processed: %s. %s from %s", track_number, track_name, album_name) + except Exception as proc_err: + err_msg = f"{track_name}: {str(proc_err)}" + errors.append(err_msg) + runtime.logger.error("Import processing error: %s", err_msg) + + if runtime.add_activity_item: + runtime.add_activity_item("", "Album Imported", f"{album_name} by {artist_name} ({processed}/{len(matches)} tracks)", "Now") + + if processed > 0: + _emit_import_completed( + runtime, + track_count=processed, + album_name=album_name or "", + artist=artist_name or "", + playlist_name=f"Import: {album_name}" if album_name else "Import", + total_tracks=len(matches), + failed_tracks=len(errors), + log_label="album", + ) + runtime.refresh_import_suggestions_cache() + + return {"success": True, "processed": processed, "total": len(matches), "errors": errors}, 200 + except Exception as exc: + runtime.logger.error("Error processing album import: %s", exc) + return {"success": False, "error": str(exc)}, 500 + + +def search_tracks(runtime: ImportRouteRuntime, query: str, limit: int = 10) -> tuple[Dict[str, Any], int]: + """Search tracks for manual single import using metadata source priority.""" + try: + query = (query or "").strip() + if not query: + return {"success": False, "error": "Missing query parameter"}, 400 + + limit = min(int(limit), 30) + if runtime.get_primary_source() == "hydrabase" and runtime.hydrabase_worker and runtime.dev_mode_enabled: + runtime.hydrabase_worker.enqueue(query, "tracks") + + tracks = runtime.search_import_tracks(query, limit=limit) + return {"success": True, "tracks": tracks}, 200 + except Exception as exc: + runtime.logger.error("Error searching tracks for import: %s", exc) + return {"success": False, "error": str(exc)}, 500 + + +def process_single_import_file(runtime: ImportRouteRuntime, file_info: Dict[str, Any]) -> tuple[str, str]: + """Validate, resolve metadata, and post-process one single import file.""" + file_path = file_info.get("full_path", "") + if not os.path.isfile(file_path): + return ("error", f"File not found: {file_info.get('filename', '?')}") + if runtime.post_process_matched_download is None: + return ("error", "Import post-processing not available") + + title = file_info.get("title", "") + artist = file_info.get("artist", "") + manual_match = file_info.get("manual_match") + if manual_match is not None and not isinstance(manual_match, dict): + manual_match = None + + manual_match_source = "" + manual_match_id = None + if manual_match: + manual_match_source = str(manual_match.get("source") or "").strip().lower() + manual_match_id = str(manual_match.get("id") or "").strip() + if not manual_match_id or not manual_match_source: + return ("error", f"Malformed manual match for file: {file_info.get('filename', '?')}") + + if not title and not manual_match: + parsed = runtime.parse_filename_metadata(file_info.get("filename", "")) + title = parsed.get("title") or os.path.splitext(file_info.get("filename", "Unknown"))[0] + if not artist: + artist = parsed.get("artist", "") + + try: + resolved = runtime.get_single_track_import_context( + title, + artist, + override_id=manual_match_id, + override_source=manual_match_source, + ) + context = runtime.normalize_import_context(resolved["context"]) + artist_data = runtime.get_import_context_artist(context) + track_data = runtime.get_import_track_info(context) + final_title = track_data.get("name", title) + final_artist = artist_data.get("name", artist) + + context_key = f"import_single_{uuid.uuid4().hex[:8]}" + runtime.post_process_matched_download(context_key, context, file_path) + runtime.logger.info( + "Import single processed: %s by %s (source=%s)", + final_title, + final_artist, + resolved.get("source") or "local", + ) + return ("ok", final_title) + except Exception as proc_err: + err_msg = f"{title}: {str(proc_err)}" + runtime.logger.error("Import single processing error: %s", err_msg) + return ("error", err_msg) + + +def singles_process(runtime: ImportRouteRuntime, files: list[Dict[str, Any]]) -> tuple[Dict[str, Any], int]: + """Process individual staging files as singles through the import pipeline.""" + try: + files = files or [] + if not files: + return {"success": False, "error": "No files provided"}, 400 + if runtime.import_singles_executor is None: + return {"success": False, "error": "Import executor not available"}, 500 + + processed = 0 + errors = [] + process_file = runtime.process_single_import_file or process_single_import_file + future_to_filename = { + runtime.import_singles_executor.submit(process_file, runtime, file_info): + file_info.get("filename", "?") + for file_info in files + } + + for future in as_completed(future_to_filename): + try: + outcome, payload = future.result() + except Exception as worker_err: + errors.append(f"{future_to_filename[future]}: worker crashed: {worker_err}") + continue + if outcome == "ok": + processed += 1 + else: + errors.append(payload) + + if runtime.add_activity_item: + runtime.add_activity_item("", "Singles Imported", f"{processed}/{len(files)} tracks processed", "Now") + + if processed > 0: + _emit_import_completed( + runtime, + track_count=processed, + album_name="", + artist="Various", + playlist_name="Import: Singles", + total_tracks=len(files), + failed_tracks=len(errors), + log_label="singles", + ) + runtime.refresh_import_suggestions_cache() + + return {"success": True, "processed": processed, "total": len(files), "errors": errors}, 200 + except Exception as exc: + runtime.logger.error("Error processing singles import: %s", exc) + return {"success": False, "error": str(exc)}, 500 + + +def _emit_import_completed( + runtime: ImportRouteRuntime, + *, + track_count: int, + album_name: str, + artist: str, + playlist_name: str, + total_tracks: int, + failed_tracks: int, + log_label: str, +) -> None: + # Keep import automation on the same chain as download batches: + # batch_complete -> auto-scan -> library_scan_completed -> auto-update DB. + try: + if runtime.automation_engine: + runtime.automation_engine.emit( + "import_completed", + { + "track_count": str(track_count), + "album_name": album_name, + "artist": artist, + }, + ) + runtime.automation_engine.emit( + "batch_complete", + { + "playlist_name": playlist_name, + "total_tracks": str(total_tracks), + "completed_tracks": str(track_count), + "failed_tracks": str(failed_tracks), + }, + ) + except Exception as exc: + runtime.logger.debug("%s import automation emit failed: %s", log_label, exc) diff --git a/core/library/retag.py b/core/library/retag.py index 48be9800..f5cf74e7 100644 --- a/core/library/retag.py +++ b/core/library/retag.py @@ -37,7 +37,7 @@ import os import traceback from dataclasses import dataclass from difflib import SequenceMatcher -from typing import Any, Callable +from typing import Any, Callable, Optional logger = logging.getLogger(__name__) @@ -62,6 +62,13 @@ class RetagDeps: _get_retag_state: Callable[[], dict] _set_retag_state: Callable[[dict], None] get_database: Callable[[], Any] + # Discord report (Netti93) — retag was clearing the LYRICS / USLT + # tag without rewriting it, while the download pipeline calls + # `generate_lrc_file` after enrichment to refetch + embed lyrics. + # Injected here so retag mirrors the same post-enrichment step. + # Optional for backward compat with any test caller that builds + # RetagDeps without the new field — empty default no-ops the call. + generate_lrc_file: Optional[Callable] = None @property def retag_state(self) -> dict: @@ -230,6 +237,20 @@ def execute_retag(group_id, album_id, deps: RetagDeps): except Exception as meta_err: logger.error(f"[Retag] Metadata write failed for '{track_title}': {meta_err}") + # Discord report (Netti93) — `enhance_file_metadata` clears + # ALL tags (incl. USLT lyrics) and rewrites only the source + # metadata. The download pipeline calls `generate_lrc_file` + # after enrichment to refetch + embed lyrics — retag was + # missing that step and dropped the LYRICS tag with no + # rewrite. Mirroring the download path's post-enrichment + # step. Same args, same `lrclib_enabled` config gate, same + # idempotency (skip when sidecar already present). + if deps.generate_lrc_file: + try: + deps.generate_lrc_file(current_file_path, context, new_artist, album_info) + except Exception as lrc_err: + logger.debug("[Retag] generate_lrc_file failed for '%s': %s", track_title, lrc_err) + # Compute new path and move if different file_ext = os.path.splitext(current_file_path)[1] try: diff --git a/core/lyrics_client.py b/core/lyrics_client.py index e7efebd8..eb49f6fa 100644 --- a/core/lyrics_client.py +++ b/core/lyrics_client.py @@ -52,9 +52,29 @@ class LyricsClient: lrc_path = os.path.splitext(audio_file_path)[0] + '.lrc' txt_path = os.path.splitext(audio_file_path)[0] + '.txt' - # Skip if lyrics file already exists (either .lrc or .txt) + # Sidecar already exists — skip the LRClib fetch but still + # re-embed the lyrics in the audio file's tag. The retag + # flow clears all tags including USLT and then runs this + # helper to restore them; without the embed step the + # LYRICS tag stays empty even though the .lrc is right + # there next to the file (Discord report — Netti93). if os.path.exists(lrc_path) or os.path.exists(txt_path): - logger.debug(f"Lyrics file already exists for: {os.path.basename(audio_file_path)}") + existing_path = lrc_path if os.path.exists(lrc_path) else txt_path + try: + with open(existing_path, 'r', encoding='utf-8') as f: + existing_lyrics = f.read().strip() + if existing_lyrics: + self._embed_lyrics(audio_file_path, existing_lyrics) + logger.debug( + "Re-embedded lyrics from existing %s for: %s", + os.path.basename(existing_path), + os.path.basename(audio_file_path), + ) + except Exception as e: + logger.debug( + "Could not re-embed lyrics from existing sidecar %s: %s", + os.path.basename(existing_path), e, + ) return True # Fetch lyrics from LRClib diff --git a/core/matching/acoustid_candidates.py b/core/matching/acoustid_candidates.py new file mode 100644 index 00000000..eac2ee49 --- /dev/null +++ b/core/matching/acoustid_candidates.py @@ -0,0 +1,143 @@ +"""Find a matching AcoustID candidate for an expected (title, artist). + +AcoustID returns multiple recordings per fingerprint — same audio can +correspond to multiple MusicBrainz recordings (different releases, +different metadata-quality entries, sample / cover-version collisions). +The "top" recording AcoustID returns isn't always the one whose +metadata matches the user's expected track. + +Both the post-download verifier (`core/acoustid_verification.py`) and +the AcoustID library scanner (`core/repair_jobs/acoustid_scanner.py`) +need to ask: "given these candidates, does ANY of them match +(expected_title, expected_artist) by title+artist similarity?" The +verifier had its own inline loop; the scanner only checked the top +match → false positives whenever the wrong-credited recording out- +ranked the right-credited one. + +This module is the single shared boundary for that question. +""" + +from __future__ import annotations + +from typing import Any, Callable, Dict, Iterable, Optional, Tuple + +from utils.logging_config import get_logger + +logger = get_logger("matching.acoustid_candidates") + + +def find_matching_recording( + recordings: Iterable[Dict[str, Any]], + expected_title: str, + expected_artist: str, + *, + title_threshold: float = 0.70, + artist_threshold: float = 0.60, + similarity: Optional[Callable[[str, str], float]] = None, + artist_similarity: Optional[Callable[[str, str], float]] = None, + skip_predicate: Optional[Callable[[Dict[str, Any]], bool]] = None, +) -> Tuple[Optional[Dict[str, Any]], float, float]: + """Return the first AcoustID candidate whose metadata passes both + title + artist similarity thresholds. + + Args: + recordings: AcoustID recording dicts. Each must carry ``title`` + and ``artist`` strings; entries without both are skipped. + expected_title: The track title the caller expected. + expected_artist: The artist the caller expected. + title_threshold: Minimum title similarity to accept (default 0.70). + artist_threshold: Minimum artist similarity to accept (default 0.60). + similarity: ``(a, b) -> float`` for title comparison. Defaults + to a lowercase exact-equals stub when not supplied — callers + should pass their stricter normaliser (verifier passes its + parenthetical-stripping ``_similarity``; scanner passes + its own). + artist_similarity: ``(expected, actual) -> float`` for artist + comparison. Lets callers supply alias-aware comparison + (verifier wraps ``_alias_aware_artist_sim``; scanner wraps + ``artist_names_match``). Defaults to ``similarity`` if + unset. + skip_predicate: Optional ``(recording_dict) -> bool``. When + truthy, the candidate is skipped (used by the verifier to + drop wrong-version recordings — instrumental vs vocal etc). + + Returns: + ``(recording, title_sim, artist_sim)`` for the first matching + candidate, or ``(None, best_title_sim, best_artist_sim)`` when + none match. The non-None ``best_*`` values let callers report + the closest near-miss when they need to log why nothing matched. + + Iteration order matches the input order (typically AcoustID's own + fingerprint-confidence ranking). Returns on first match — does NOT + score every candidate looking for the highest sim. + """ + if not expected_title or not expected_artist: + return None, 0.0, 0.0 + + sim = similarity or _default_similarity + asim = artist_similarity or sim + + best_title_sim = 0.0 + best_artist_sim = 0.0 + + for rec in recordings or (): + if not isinstance(rec, dict): + continue + rec_title = (rec.get('title') or '').strip() + rec_artist = (rec.get('artist') or '').strip() + if not rec_title or not rec_artist: + continue + if skip_predicate and skip_predicate(rec): + continue + + title_sim = sim(expected_title, rec_title) + if title_sim > best_title_sim: + best_title_sim = title_sim + + artist_sim = asim(expected_artist, rec_artist) + if artist_sim > best_artist_sim: + best_artist_sim = artist_sim + + if title_sim >= title_threshold and artist_sim >= artist_threshold: + return rec, title_sim, artist_sim + + return None, best_title_sim, best_artist_sim + + +def _default_similarity(a: str, b: str) -> float: + if not a or not b: + return 0.0 + return 1.0 if a.lower().strip() == b.lower().strip() else 0.0 + + +# ──────────────────────────────────────────────────────────────────── +# Duration guard — codex item (5). +# ──────────────────────────────────────────────────────────────────── + + +def duration_mismatches_strongly( + expected_seconds: Optional[float], + candidate_seconds: Optional[float], + *, + abs_tolerance_s: float = 60.0, + rel_tolerance: float = 0.35, +) -> bool: + """Return True when the candidate's duration is too far from expected + to confidently treat it as the same recording. + + Catches fingerprint hash collisions (the reporter's 17-minute + mashup → 5-minute Japanese hiphop track case). When EITHER duration + is unknown / non-positive, returns False — no behavior change. + + Threshold: drift greater than max(``abs_tolerance_s``, + ``rel_tolerance * expected``). The relative term scales with track + length so a 20% mismatch on a 3-minute track and a 20% mismatch on + a 30-minute mix are both treated as suspicious. + """ + if not expected_seconds or expected_seconds <= 0: + return False + if not candidate_seconds or candidate_seconds <= 0: + return False + drift = abs(float(candidate_seconds) - float(expected_seconds)) + threshold = max(abs_tolerance_s, rel_tolerance * float(expected_seconds)) + return drift > threshold diff --git a/core/matching/album_context_title.py b/core/matching/album_context_title.py new file mode 100644 index 00000000..7fd0d2cd --- /dev/null +++ b/core/matching/album_context_title.py @@ -0,0 +1,195 @@ +"""Strip redundant album-context suffixes from track titles. + +Issue #589 — MTV Unplugged albums (and similar live-concert / session +releases) have source-side track titles like ``"Shy Away (MTV Unplugged +Live)"`` while the local DB stored title is just ``"Shy Away"``. The +album-scoped library check at ``core/downloads/master.py`` compares +the two with raw string similarity, the length asymmetry tanks the +score, and tracks the user already owns get marked missing. + +This helper normalizes a track title by stripping the parenthetical +or dash suffix when its tokens are fully subsumed by the album +context: at least one version marker (live / unplugged / acoustic / +session / etc) is present in BOTH the suffix AND the album title, and +every other suffix token is either a known marker, a year, a +connecting noise word, or a word that appears in the album title. + +Pure function. No I/O. Tests at the function boundary. +""" + +from __future__ import annotations + +import re +from typing import Iterable, Tuple + +# Version-marker keywords. When the album title contains any of these, +# stripping is enabled. Singular forms — plurals get matched separately +# via stem expansion below. +_VERSION_MARKERS = ( + 'live', + 'unplugged', + 'acoustic', + 'session', + 'concert', + 'tour', +) + +# Markers that are implied "live" context — when the album mentions any +# of these, a bare ``live`` token in the suffix counts as album context +# even if the album title doesn't literally say "live". MTV Unplugged +# albums are live recordings; same for "in concert" / "tour" releases. +_IMPLIES_LIVE = ('unplugged', 'concert', 'tour', 'session') + +# Connecting / filler words that don't carry meaning by themselves. +_NOISE_TOKENS = frozenset({ + 'version', 'edition', 'recording', 'recordings', 'remaster', + 'remastered', 'mix', + 'the', 'a', 'an', 'from', 'at', 'in', 'on', 'for', 'of', + 'and', 'or', 'with', 'by', + 'vol', 'pt', 'part', 'no', +}) + +_SUFFIX_PATTERNS: Tuple[re.Pattern, ...] = ( + re.compile(r'\s*\(([^()]+)\)\s*$'), + re.compile(r'\s*\[([^\[\]]+)\]\s*$'), + re.compile(r'\s+-\s+(.+?)\s*$'), +) + +_YEAR_RE = re.compile(r'^(?:19|20)\d{2}$') +_TOKEN_RE = re.compile(r'\w+') + + +def _normalize(text: str) -> str: + return (text or '').lower().strip() + + +def _tokenize(text: str) -> set: + return set(_TOKEN_RE.findall(_normalize(text))) + + +def _expand_marker_set(markers: Iterable[str]) -> set: + """Expand each marker into its singular + plural forms.""" + out = set() + for marker in markers: + out.add(marker) + if not marker.endswith('s'): + out.add(marker + 's') + return out + + +_EXPANDED_MARKERS = _expand_marker_set(_VERSION_MARKERS) + + +def album_context_markers(album_title: str) -> Tuple[str, ...]: + """Return the version markers present in the album title (singular form).""" + if not album_title: + return () + album_tokens = _tokenize(album_title) + found = [] + for marker in _VERSION_MARKERS: + if marker in album_tokens or (marker + 's') in album_tokens: + found.append(marker) + return tuple(found) + + +def _suffix_is_album_redundant( + inner: str, + album_tokens: set, + album_markers: Tuple[str, ...], +) -> bool: + """Decide whether a suffix's tokens are all subsumed by album context. + + Three requirements: + 1. The suffix contains at least one version-marker token. Stops + a generic "feat. X" suffix from being stripped because the + album happened to be live. + 2. The shared marker matches one the album implies — either + literally in the album title, OR via the implied-live set + (unplugged/concert/tour albums imply "live"). + 3. Every other suffix token is either a marker, a year, a + tolerated noise word, or a word that appears in the album + title. If any token falls outside, the suffix carries + info beyond album context (featured artist, different + version, etc) — keep it on. + """ + if not inner: + return False + + suffix_tokens = _tokenize(inner) + if not suffix_tokens: + return False + + # Markers the album effectively implies (literal + implied-live). + implied_markers = set(album_markers) + if any(m in implied_markers for m in _IMPLIES_LIVE): + implied_markers.add('live') + + suffix_markers = suffix_tokens & _EXPANDED_MARKERS + if not suffix_markers: + return False + + # At least one marker must overlap with album-implied set. Plural + # tolerance — strip trailing 's' for the comparison. + def _stem(tok: str) -> str: + return tok[:-1] if tok.endswith('s') and len(tok) > 1 else tok + + if not any(_stem(t) in implied_markers for t in suffix_markers): + return False + + # Every remaining suffix token must be subsumed. + for tok in suffix_tokens: + if tok in _EXPANDED_MARKERS: + continue + if _YEAR_RE.match(tok): + continue + if tok in _NOISE_TOKENS: + continue + if tok in album_tokens: + continue + return False + + return True + + +def strip_redundant_album_suffix(track_title: str, album_title: str) -> str: + """Strip a trailing parenthetical/bracket/dash suffix from `track_title` + when the suffix duplicates context already implied by `album_title`. + + Examples: + - ("Shy Away (MTV Unplugged Live)", "MTV Unplugged") → "Shy Away" + - ("Only If For A Night (MTV Unplugged, 2012 / Live)", + "Ceremonials (Live At MTV Unplugged)") → "Only If For A Night" + - ("In My Feelings (Instrumental)", "Scorpion") + → unchanged (instrumental NOT implied by studio album) + - ("Hello (Live - feat. Other)", "Live At Wembley") + → unchanged (suffix carries featured-artist beyond album context) + - ("Shy Away", "MTV Unplugged") → unchanged (no suffix) + + Pure function — never raises, returns the input unchanged on any + edge / unexpected input. + """ + if not track_title: + return track_title or '' + album_markers = album_context_markers(album_title) + if not album_markers: + return track_title + + album_tokens = _tokenize(album_title) + stripped = track_title + + # Stacked suffixes ("Track (MTV Unplugged) [Live]") — peel one at a + # time. Bound the loop defensively. + for _ in range(4): + peeled = None + for pattern in _SUFFIX_PATTERNS: + m = pattern.search(stripped) + if not m: + continue + inner = m.group(1) + if _suffix_is_album_redundant(inner, album_tokens, album_markers): + peeled = stripped[: m.start()].rstrip() + break + if peeled is None: + return stripped + stripped = peeled + return stripped diff --git a/core/metadata/artist_resolution.py b/core/metadata/artist_resolution.py new file mode 100644 index 00000000..362a6760 --- /dev/null +++ b/core/metadata/artist_resolution.py @@ -0,0 +1,74 @@ +"""Pure artist-list resolution for tag-write paths. + +Single source of truth for "what is the canonical multi-value artists +list for this track?" Different download paths populate `context` with +different keys — Deezer-direct downloads stamp `original_search.artists` +as a proper list, but Soulseek matched downloads only carry `artist` +(singular string) in `original_search_result` while the full list lives +on `track_info` (the full Spotify track object). + +Resolution order: + 1. `context.original_search_result.artists` (preferred — already- + curated by the source path that constructed the context) + 2. `context.track_info.artists` (Spotify/Deezer/Tidal full track + object — always carries the artists array when matched) + 3. `[artist_dict.name]` as a single-element fallback when neither + carries a list (primary-artist-only) + +Each list item may be a dict with a `name` key (Spotify shape), a bare +string, or any other object — the helper normalizes all three to +strings and drops empty entries. +""" + +from __future__ import annotations + +from typing import Any, Dict, List, Optional + + +def _normalize_artists_iterable(items: Any) -> List[str]: + if not isinstance(items, list): + return [] + result: List[str] = [] + for item in items: + if isinstance(item, dict): + name = item.get("name") + if isinstance(name, str) and name.strip(): + result.append(name.strip()) + elif isinstance(item, str): + stripped = item.strip() + if stripped: + result.append(stripped) + elif item is not None: + text = str(item).strip() + if text: + result.append(text) + return result + + +def resolve_track_artists( + original_search: Optional[Dict[str, Any]], + track_info: Optional[Dict[str, Any]], + artist_dict: Optional[Dict[str, Any]], +) -> List[str]: + """Return the canonical multi-value artists list for tag-write. + + Falls through preferred → track_info → primary-artist fallback. Each + candidate is normalized to a list of stripped non-empty strings. + Empty list returned only when every candidate is empty/invalid. + """ + if isinstance(original_search, dict): + primary = _normalize_artists_iterable(original_search.get("artists")) + if primary: + return primary + + if isinstance(track_info, dict): + secondary = _normalize_artists_iterable(track_info.get("artists")) + if secondary: + return secondary + + if isinstance(artist_dict, dict): + name = artist_dict.get("name") + if isinstance(name, str) and name.strip(): + return [name.strip()] + + return [] diff --git a/core/metadata/enrichment.py b/core/metadata/enrichment.py index 051426f8..43e668e8 100644 --- a/core/metadata/enrichment.py +++ b/core/metadata/enrichment.py @@ -102,7 +102,19 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf save_audio_file(audio_file, symbols) return True - track_num_str = f"{metadata.get('track_number', 1)}/{metadata.get('total_tracks', 1)}" + # Discord report (Netti93) — many album-dict construction + # sites pass `total_tracks: 0` when source data is incomplete + # (per types.py, 0 means "unknown"). Pre-fix this serialized + # to "6/0" tags. Helper drops the `/N` suffix when total is + # unknown so the tag reads "6" instead — matches retag's + # behavior at core/tag_writer.py and ID3 spec convention. + from core.metadata.track_number_format import ( + format_track_number_tag, + format_track_number_tuple, + ) + track_num_str = format_track_number_tag( + metadata.get('track_number'), metadata.get('total_tracks') + ) write_multi = cfg.get("metadata_enhancement.tags.write_multi_artist", False) artists_list = metadata.get("_artists_list", []) @@ -167,7 +179,9 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf audio_file["\xa9day"] = [metadata["date"]] if metadata.get("genre"): audio_file["\xa9gen"] = [metadata["genre"]] - audio_file["trkn"] = [(metadata.get("track_number", 1), metadata.get("total_tracks", 1))] + audio_file["trkn"] = [format_track_number_tuple( + metadata.get("track_number"), metadata.get("total_tracks") + )] if metadata.get("disc_number"): audio_file["disk"] = [(metadata["disc_number"], 0)] diff --git a/core/metadata/source.py b/core/metadata/source.py index 67350205..8903eb0e 100644 --- a/core/metadata/source.py +++ b/core/metadata/source.py @@ -23,6 +23,7 @@ from core.imports.context import ( get_source_tag_names, normalize_import_context, ) +from core.metadata.artist_resolution import resolve_track_artists from core.metadata.registry import get_itunes_client from database.music_database import get_database from core.metadata.common import ( @@ -907,17 +908,13 @@ def extract_source_metadata(context: dict, artist: dict, album_info: dict) -> di else: logger.warning("Metadata: Using original title as fallback: '%s'", metadata["title"]) - artists = original_search.get("artists") - if isinstance(artists, list) and artists: - all_artists = [] - for artist_item in artists: - if isinstance(artist_item, dict) and artist_item.get("name"): - all_artists.append(artist_item["name"]) - elif isinstance(artist_item, str): - all_artists.append(artist_item) - else: - all_artists.append(str(artist_item)) - + # Resolve canonical artists list. Soulseek matched-download contexts + # only carry `original_search.artist` (singular string) — the full + # contributors list lives on `track_info` (the matched Spotify/etc + # track object). Deezer-direct contexts populate `original_search.artists` + # directly. Pure helper handles all three shapes. + all_artists = resolve_track_artists(original_search, track_info, artist_dict) + if all_artists: # Deezer upgrade path: Deezer's `/search` endpoint only returns # the primary artist for each track. The full contributors # array (feat., remix collaborators, producers credited as diff --git a/core/metadata/track_number_format.py b/core/metadata/track_number_format.py new file mode 100644 index 00000000..5feb5bfe --- /dev/null +++ b/core/metadata/track_number_format.py @@ -0,0 +1,77 @@ +"""Format track-number tags consistently across audio formats. + +Discord report (Netti93): album tracks were tagged as ``TRCK = "6/0"`` +instead of ``"6/13"``. Cause: many album-dict construction sites in +the codebase pass ``total_tracks: 0`` when the source data is +incomplete, and ``core/metadata/enrichment.py`` formatted the tag +unconditionally as ``f"{track_number}/{total_tracks}"`` — so 0 +propagated straight to disk. The retag path was unaffected because +``core/tag_writer.py`` already does the right thing. + +Per ``core/metadata/types.py``, ``total_tracks = 0`` is documented +as "unknown" — not an actual track count. Fix at the consumer +boundary so every album-dict constructor doesn't need to be touched. + +This module provides one pure helper. Tests at the function boundary. +""" + +from __future__ import annotations + +from typing import Optional, Tuple + + +def format_track_number_tag( + track_number: Optional[int], + total_tracks: Optional[int], +) -> str: + """Return the canonical TRCK / tracknumber tag string. + + - ``track_number=6, total_tracks=13`` → ``"6/13"`` + - ``track_number=6, total_tracks=0`` → ``"6"`` (total unknown) + - ``track_number=6, total_tracks=None`` → ``"6"`` + - ``track_number=None, total_tracks=13`` → ``"1/13"`` (track defaults to 1) + - ``track_number=None, total_tracks=None`` → ``"1"`` + + ID3 spec allows ``TRCK`` to be either ``"N"`` or ``"N/M"``. Vorbis + ``tracknumber`` follows the same convention. Avoiding the ``/0`` + suffix keeps the tag honest — most media servers and taggers + interpret ``6/0`` as "track 6 of 0" which is nonsensical, while + ``6`` reads as "track 6, total unknown". + """ + num = _coerce_positive_int(track_number, default=1) + total = _coerce_positive_int(total_tracks, default=0) + if total > 0: + return f"{num}/{total}" + return str(num) + + +def format_track_number_tuple( + track_number: Optional[int], + total_tracks: Optional[int], +) -> Tuple[int, int]: + """Return the MP4 ``trkn`` tuple ``(track, total)``. + + MP4 tag spec stores track-of as a 2-int tuple — convention is + ``(N, 0)`` when the total is unknown. Same coercion rules as + ``format_track_number_tag``: missing / None / non-positive + ``track_number`` defaults to 1, missing / 0 / negative + ``total_tracks`` returns 0 (the spec's "unknown" marker). + """ + num = _coerce_positive_int(track_number, default=1) + total = _coerce_positive_int(total_tracks, default=0) + return (num, total) + + +def _coerce_positive_int(value, *, default: int) -> int: + """Coerce to a non-negative int. Falls back to ``default`` for + None / non-numeric / negative input. Floats truncate. + """ + if value is None: + return default + try: + coerced = int(value) + except (TypeError, ValueError): + return default + if coerced < 0: + return default + return coerced diff --git a/core/musicbrainz_service.py b/core/musicbrainz_service.py index 20f24023..d9085300 100644 --- a/core/musicbrainz_service.py +++ b/core/musicbrainz_service.py @@ -428,54 +428,52 @@ class MusicBrainzService: return [] # Tier 3: live MB lookup. Search → fetch by MBID → cache. - try: - results = self.mb_client.search_artist(artist_name, limit=3) - except Exception as e: - logger.debug("lookup_artist_aliases: search_artist(%r) raised: %s", artist_name, e) - self._save_to_cache('artist_aliases', artist_name, None, None, {'aliases': []}, 0) - return [] - - if not results: - self._save_to_cache('artist_aliases', artist_name, None, None, {'aliases': []}, 0) - return [] - - # Score each result: combined of name-similarity + MB's own - # relevance. Score range 0.0-1.0. - scored = [] - for result in results: - mb_name = result.get('name', '') - mb_score = result.get('score', 0) - sim = self._calculate_similarity(artist_name, mb_name) - combined = (sim * 0.7) + (mb_score / 100 * 0.3) - mbid = result.get('id') - if mbid: - scored.append((combined, mbid)) + # Issue #586 — strict search queries `artist:"..."` only and + # MISSES alias / sortname indexes. When MB's canonical name is + # the non-Latin form (e.g. `Дмитрий Яблонский`), the user's + # Latin input ("Dmitry Yablonsky") finds nothing under strict. + # Fall back to non-strict (bare query, hits alias + sortname + # indexes) when strict returns empty OR all results fail the + # trust gate. + scored = self._search_and_score_artists(artist_name, strict=True) + if not scored or self._best_score(scored) < 0.85: + non_strict = self._search_and_score_artists(artist_name, strict=False) + if non_strict and (not scored or self._best_score(non_strict) > self._best_score(scored)): + scored = non_strict if not scored: self._save_to_cache('artist_aliases', artist_name, None, None, {'aliases': []}, 0) return [] scored.sort(key=lambda x: -x[0]) - best_score, best_mbid = scored[0] + best_score, best_mbid, best_mb_score = scored[0] - # Strict trust threshold: real matches for distinctive cross- - # script artists (the user-reported case) score >= 0.95. - # Anything below 0.85 is ambiguous and not worth the false- - # positive risk of pulling in aliases for the wrong artist. - if best_score < 0.85: + # Trust gate. Two ways to pass: + # 1. Combined score >= 0.85 (the historical strict bar that + # catches same-script matches) + # 2. MB's OWN score is very high (>= 95) AND the result is + # unambiguous (top result clearly leads). Bridges the + # cross-script case where local similarity is near zero + # ("Dmitry Yablonsky" vs "Дмитрий Яблонский" sim ~0) + # but MB's index found a high-confidence match. + passes_combined = best_score >= 0.85 + passes_mb_only = best_mb_score >= 95 and ( + len(scored) < 2 or (scored[0][2] - scored[1][2]) >= 5 + ) + if not (passes_combined or passes_mb_only): logger.debug( "lookup_artist_aliases: best match for %r below trust " - "threshold (score=%.2f)", artist_name, best_score, + "threshold (combined=%.2f, mb_score=%d)", + artist_name, best_score, best_mb_score, ) self._save_to_cache('artist_aliases', artist_name, None, None, {'aliases': []}, 0) return [] # Ambiguity detection: when 2+ results both score high (within - # 0.1 of the best), the search hit multiple distinct artists - # with similar names ("John Smith" returning 10 different - # John Smiths all at score 100). Pulling aliases for one of - # them could produce wrong matches. Skip + cache empty. - if len(scored) >= 2 and (scored[0][0] - scored[1][0]) < 0.1: + # 0.1 of the best combined), the search hit multiple distinct + # artists with similar names. Pulling aliases for one could + # produce wrong matches. Skip + cache empty. + if len(scored) >= 2 and (scored[0][0] - scored[1][0]) < 0.1 and not passes_mb_only: logger.debug( "lookup_artist_aliases: ambiguous match for %r — top " "two results within 0.1 (%.2f / %.2f). Skipping alias lookup.", @@ -491,6 +489,40 @@ class MusicBrainzService: ) return aliases + def _search_and_score_artists(self, artist_name: str, strict: bool): + """Search MB for an artist and score each result. + + Returns a list of (combined_score, mbid, raw_mb_score) tuples. + Combined score: 70% local similarity + 30% MB's own relevance + score (0..1). raw_mb_score preserved separately so the trust + gate can prefer high-MB-score results in cross-script cases + where local similarity is near zero. + + Returns empty list on any failure. + """ + try: + results = self.mb_client.search_artist(artist_name, limit=3, strict=strict) + except Exception as e: + logger.debug( + "lookup_artist_aliases: search_artist(%r, strict=%s) raised: %s", + artist_name, strict, e, + ) + return [] + scored = [] + for result in results or []: + mb_name = result.get('name', '') + mb_score = result.get('score', 0) + sim = self._calculate_similarity(artist_name, mb_name) + combined = (sim * 0.7) + (mb_score / 100 * 0.3) + mbid = result.get('id') + if mbid: + scored.append((combined, mbid, mb_score)) + return scored + + @staticmethod + def _best_score(scored): + return max((s[0] for s in scored), default=0.0) if scored else 0.0 + def fetch_artist_aliases(self, mbid: str) -> list: """Fetch the alias list for an artist from MusicBrainz. @@ -499,6 +531,14 @@ class MusicBrainzService: artist record. Pull them so SoulSync can recognise that `澤野弘之` and `Hiroyuki Sawano` refer to the same artist. + Issue #586 — for some artists MB's CANONICAL `name` is the + non-Latin spelling (e.g. `Дмитрий Яблонский`) while the + Latin spelling lives in `aliases` — but the inverse also + happens, where the Latin canonical name has the Cyrillic in + aliases. Either way the canonical `name` and `sort-name` are + themselves valid alternate spellings for matching purposes, + so include them alongside the explicit alias entries. + Returns the deduplicated list of alias `name` strings. Returns empty list (NOT None) on any failure — caller should treat empty as "no aliases available, fall back to direct match" so @@ -514,24 +554,38 @@ class MusicBrainzService: return [] if not data: return [] - raw_aliases = data.get('aliases') or [] + + seen = set() + cleaned = [] + + def _add(value): + if not isinstance(value, str): + return + text = value.strip() + if not text: + return + key = text.lower() + if key in seen: + return + seen.add(key) + cleaned.append(text) + + # Canonical name + sort-name treated as aliases for matching — + # they're the strongest cross-script bridge when MB's + # canonical spelling differs from the user's input. + _add(data.get('name')) + _add(data.get('sort-name')) + # MB returns each alias as a dict with `name`, `sort-name`, # `locale`, `primary`, `type`, etc. We only care about the # display name — that's what `actual` artist strings will - # match against. - seen = set() - cleaned = [] - for entry in raw_aliases: + # match against. Also pull alias sort-name when present + # (some entries have a different sortable form). + for entry in data.get('aliases') or []: if not isinstance(entry, dict): continue - name = (entry.get('name') or '').strip() - if not name: - continue - key = name.lower() - if key in seen: - continue - seen.add(key) - cleaned.append(name) + _add(entry.get('name')) + _add(entry.get('sort-name')) return cleaned def update_artist_aliases(self, artist_id: int, aliases: list) -> None: diff --git a/core/repair_jobs/acoustid_scanner.py b/core/repair_jobs/acoustid_scanner.py index 660c216f..00f900a1 100644 --- a/core/repair_jobs/acoustid_scanner.py +++ b/core/repair_jobs/acoustid_scanner.py @@ -254,6 +254,74 @@ class AcoustIDScannerJob(RepairJob): if title_sim >= title_threshold and artist_sim >= artist_threshold: return + # Issue #587 (Foxxify) — top recording's metadata mismatched, but + # AcoustID often returns multiple recordings per fingerprint + # (sample collisions, multi-MB-record cases). Check ALL of them + # before flagging — if any candidate's metadata matches expected + # title + artist, the file IS the right song and AcoustID's top + # match was just a wrong-credited recording. + from core.matching.acoustid_candidates import ( + duration_mismatches_strongly, + find_matching_recording, + ) + from core.matching.artist_aliases import artist_names_match + + def _scanner_title_sim(a, b): + return SequenceMatcher(None, _normalize(a), _normalize(b)).ratio() + + def _scanner_artist_sim(expected_a, actual_a): + _, score = artist_names_match(expected_a, actual_a, threshold=artist_threshold) + return score + + candidate_match, _, _ = find_matching_recording( + fp_result.get('recordings') or [], + expected['title'], + expected_artist, + title_threshold=title_threshold, + artist_threshold=artist_threshold, + similarity=_scanner_title_sim, + artist_similarity=_scanner_artist_sim, + ) + if candidate_match is not None: + # A lower-ranked candidate matched — file IS the right song. + # No finding. + if context.report_progress: + context.report_progress( + log_line=( + f'Resolved (lower-ranked candidate match): {fname} — ' + f'expected "{expected["title"]}" matched candidate ' + f'"{candidate_match.get("title")}" by ' + f'"{candidate_match.get("artist")}"' + ), + log_type='ok', + ) + return + + # Issue #587 (Foxxify "17min mashup → 5min track") — duration + # guard against fingerprint hash collisions. When the file's + # actual duration differs from AcoustID's matched recording by + # more than max(60s, 35%), the fingerprint is almost certainly + # a sample/intro collision, not a real recording match. Don't + # produce a confident "Wrong Song" finding. + try: + file_duration_s = (expected.get('duration_ms') or 0) / 1000.0 + except Exception: + file_duration_s = 0 + candidate_duration_s = best_recording.get('duration') + if candidate_duration_s is None and best_recording.get('length'): + candidate_duration_s = best_recording.get('length') + if duration_mismatches_strongly(file_duration_s, candidate_duration_s): + if context.report_progress: + context.report_progress( + log_line=( + f'Skipped (duration mismatch suggests fingerprint collision): ' + f'{fname} — expected {file_duration_s:.0f}s, AcoustID ' + f'candidate {candidate_duration_s:.0f}s' + ), + log_type='skip', + ) + return + # Mismatch detected if context.report_progress: context.report_progress( @@ -326,7 +394,8 @@ class AcoustIDScannerJob(RepairJob): t.file_path, t.track_number, al.title AS album_title, al.thumb_url, ar.thumb_url, NULLIF(t.track_artist, '') AS track_artist, - ar.name AS album_artist + ar.name AS album_artist, + t.duration FROM tracks t LEFT JOIN artists ar ON ar.id = t.artist_id LEFT JOIN albums al ON al.id = t.album_id @@ -352,6 +421,11 @@ class AcoustIDScannerJob(RepairJob): 'artist_thumb_url': row[7] or None, 'track_artist': row[8] or '', # raw (may be empty) 'album_artist': row[9] or '', + # Duration in MS (DB stores ms). Used by the + # duration-mismatch guard to spot fingerprint + # collisions where the matched recording is a + # totally different length. + 'duration_ms': row[10] or 0, } except Exception as e: logger.error("Error loading tracks from DB: %s", e) diff --git a/core/repair_worker.py b/core/repair_worker.py index 2c0b07f8..227d144a 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -35,6 +35,23 @@ logger = get_logger("repair_worker") AUDIO_EXTENSIONS = {'.mp3', '.flac', '.ogg', '.opus', '.m4a', '.aac', '.wav', '.wma', '.aiff', '.aif'} +def _split_acoustid_credit(credit: str) -> List[str]: + """Split an AcoustID artist credit into individual contributor names. + + Reuses the matching layer's credit splitter so the AcoustID retag + path tags multi-artist tracks the same way the post-download + enrichment pipeline does (comma / ampersand / feat. / etc). + Returns ``[credit]`` for single-artist credits — the writer's + ``len > 1`` check is what gates whether the multi-value tag gets + written. + """ + try: + from core.matching.artist_aliases import split_artist_credit + return split_artist_credit(credit) + except Exception: + return [credit] if credit else [] + + def _resolve_file_path(file_path, transfer_folder, download_folder=None, config_manager=None, plex_client=None): """Resolve a stored DB path to an actual file on disk. @@ -1715,6 +1732,17 @@ class RepairWorker: tag_updates = {'title': aid_title} if aid_artist: tag_updates['artist_name'] = aid_artist + # Issue #587 — derive a per-artist list from + # AcoustID's credit string when it carries + # multiple contributors. The post-download + # enrichment pipeline preserves multi-value + # ARTISTS tags via the user's + # `write_multi_artist` setting; the repair + # path was bypassing that and writing a + # single-string TPE1 only. Now respects the + # same setting via the writer's new + # `artists_list` derivation. + tag_updates['artists_list'] = _split_acoustid_credit(aid_artist) write_tags_to_file(resolved, tag_updates) logger.info("Wrote corrected tags to file: %s", resolved) except Exception as tag_err: diff --git a/core/sync/__init__.py b/core/sync/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/core/sync/match_overrides.py b/core/sync/match_overrides.py new file mode 100644 index 00000000..f83c007d --- /dev/null +++ b/core/sync/match_overrides.py @@ -0,0 +1,119 @@ +"""Sync match overrides — user-confirmed source→server track pairings. + +When a user picks a local file via "Find & Add" on the Server Playlist +compare view, that selection should persist as a hard match across +future syncs — bypassing the fuzzy/exact title-match algorithm +entirely. This module provides pure helpers that the web layer calls +to resolve and persist those overrides through the existing +`sync_match_cache` table. + +Override semantics: + - One mapping per (source_track_id, server_source). UNIQUE + constraint on the table enforces single mapping per pair. + - Stored with confidence=1.0 to distinguish from auto-discovered + matches (which use the actual title-similarity score). + - Read at the START of the matching algorithm — before pass-1 + exact and pass-2 fuzzy. Skipped sources don't re-enter the + normal matching pool. + - Stale-cache safe: if the cached server_track_id doesn't exist + in the current server_tracks list (track removed from server), + the override is silently skipped and normal matching runs. +""" + +from __future__ import annotations + +from typing import Any, Callable, Dict, List, Optional + + +def resolve_match_overrides( + source_tracks: List[Dict[str, Any]], + server_tracks: List[Dict[str, Any]], + cache_lookup: Callable[[str], Optional[Any]], +) -> Dict[int, int]: + """Map source-track indexes to server-track indexes for cached overrides. + + Pure function. `cache_lookup(source_track_id) -> server_track_id or + None` is injected by the caller (web layer wraps the DB call). + + Returns ``{source_idx: server_idx}``. Only includes pairs where: + - source_track has a non-empty `source_track_id` + - cache_lookup returns a server_track_id + - that server_track_id exists in server_tracks (no stale cache + entries pointing at deleted tracks) + - the server_track hasn't already been claimed by an earlier + override (defensive — UNIQUE on the cache table prevents this + in practice) + + Caller uses the returned dict to short-circuit the per-source + matching loop: indices in the dict skip the exact/fuzzy passes. + """ + if not source_tracks or not server_tracks: + return {} + + server_id_to_idx: Dict[str, int] = {} + for j, svr in enumerate(server_tracks): + sid = svr.get("id") if isinstance(svr, dict) else None + if sid is not None: + key = str(sid) + if key not in server_id_to_idx: + server_id_to_idx[key] = j + + overrides: Dict[int, int] = {} + used_server: set[int] = set() + + for i, src in enumerate(source_tracks): + if not isinstance(src, dict): + continue + src_id = src.get("source_track_id") + if not src_id: + continue + try: + cached_server_id = cache_lookup(str(src_id)) + except Exception: + cached_server_id = None + if not cached_server_id: + continue + j = server_id_to_idx.get(str(cached_server_id)) + if j is None or j in used_server: + continue + overrides[i] = j + used_server.add(j) + + return overrides + + +def record_manual_match( + db: Any, + source_track_id: str, + server_source: str, + server_track_id: Any, + server_track_title: str = "", + source_title: str = "", + source_artist: str = "", +) -> bool: + """Persist a user-confirmed source→server pairing as a hard override. + + Wraps `db.save_sync_match_cache` with confidence=1.0 (the manual + match marker). Normalized title/artist fields are informational + only — the cache is keyed by `(spotify_track_id, server_source)`, + so the normalization is just for inspection and future debugging. + + Returns True on persist success, False on any failure (DB, missing + args, etc). Never raises. + """ + if not source_track_id or not server_source or server_track_id is None: + return False + if not hasattr(db, "save_sync_match_cache"): + return False + try: + return bool(db.save_sync_match_cache( + spotify_track_id=str(source_track_id), + normalized_title=(source_title or "").lower().strip(), + normalized_artist=(source_artist or "").lower().strip(), + server_source=server_source, + server_track_id=server_track_id, + server_track_title=server_track_title or "", + confidence=1.0, + )) + except Exception: + return False diff --git a/core/tag_writer.py b/core/tag_writer.py index 11cb5be1..5b4d5752 100644 --- a/core/tag_writer.py +++ b/core/tag_writer.py @@ -299,18 +299,31 @@ def write_tags_to_file(file_path: str, db_data: Dict[str, Any], elif isinstance(genres, str): genre_str = genres + # Multi-value artist support — issue #587. Caller can pass + # `artists_list` (per-track list of contributor names) and the + # writer respects the user's `metadata_enhancement.tags.write_multi_artist` + # config the same way the post-download enrichment pipeline does. + # When the setting is on AND the list has >1 entry: + # - ID3 keeps TPE1 as the joined display string (already in `artist`) + # and writes a separate TXXX:Artists frame with the list + # - Vorbis writes an `artists` multi-value key alongside `artist` + # - MP4 writes \xa9ART as the list when on, single string when off + # When OFF or the list is empty/single — same single-string write + # as before. Backward compatible for callers that don't pass it. + artists_list = _resolve_artists_list_for_write(db_data) + if isinstance(audio.tags, ID3): written = _write_id3(audio, title, artist, album_artist, album, year, genre_str, track_num, total_tracks, - disc_num, bpm) + disc_num, bpm, artists_list=artists_list) elif isinstance(audio, (FLAC, OggVorbis)) or type(audio).__name__ == 'OggOpus': written = _write_vorbis(audio, title, artist, album_artist, album, year, genre_str, track_num, total_tracks, - disc_num, bpm) + disc_num, bpm, artists_list=artists_list) elif isinstance(audio, MP4): written = _write_mp4(audio, title, artist, album_artist, album, year, genre_str, track_num, total_tracks, - disc_num, bpm) + disc_num, bpm, artists_list=artists_list) # Embed cover art if requested if embed_cover: @@ -343,8 +356,43 @@ def write_tags_to_file(file_path: str, db_data: Dict[str, Any], # ── Format-specific writers ── + +def _resolve_artists_list_for_write(db_data: Dict[str, Any]) -> Optional[List[str]]: + """Pull a multi-value artists list from db_data when caller supplied one. + + Accepts either ``artists_list`` (list of names) or ``artists`` (same + shape — kept for symmetry with the post-process pipeline's + ``_artists_list`` field). Drops empty / non-string entries. Returns + ``None`` when no list was supplied so format writers can branch on + "single-string only" vs "multi-value too". + """ + raw = db_data.get('artists_list') or db_data.get('artists') or db_data.get('_artists_list') + if not raw: + return None + if not isinstance(raw, (list, tuple)): + return None + cleaned = [] + for entry in raw: + if isinstance(entry, str): + text = entry.strip() + if text: + cleaned.append(text) + return cleaned or None + + +def _multi_artist_write_enabled() -> bool: + """Read the same config flag the enrichment pipeline reads, so the + repair-path retag respects the user's choice.""" + try: + from config.settings import config_manager + return bool(config_manager.get('metadata_enhancement.tags.write_multi_artist', False)) + except Exception: + return False + + def _write_id3(audio, title, artist, album_artist, album, year, genre, - track_num, total_tracks, disc_num, bpm) -> List[str]: + track_num, total_tracks, disc_num, bpm, + artists_list: Optional[List[str]] = None) -> List[str]: written = [] if title: audio.tags.delall('TIT2') @@ -354,6 +402,16 @@ def _write_id3(audio, title, artist, album_artist, album, year, genre, audio.tags.delall('TPE1') audio.tags.add(TPE1(encoding=3, text=[artist])) written.append('artist') + # TPE1 stays as the joined display string. When the caller + # supplied a multi-value list AND the user has the + # write_multi_artist setting on, ALSO write the per-artist + # list to a TXXX:Artists frame (Picard convention). Mirrors + # the post-download enrichment writer at + # core/metadata/enrichment.py. + if artists_list and len(artists_list) > 1 and _multi_artist_write_enabled(): + audio.tags.delall('TXXX:Artists') + audio.tags.add(TXXX(encoding=3, desc='Artists', text=list(artists_list))) + written.append('artists_multi') if album_artist: audio.tags.delall('TPE2') audio.tags.add(TPE2(encoding=3, text=[album_artist])) @@ -387,7 +445,8 @@ def _write_id3(audio, title, artist, album_artist, album, year, genre, def _write_vorbis(audio, title, artist, album_artist, album, year, genre, - track_num, total_tracks, disc_num, bpm) -> List[str]: + track_num, total_tracks, disc_num, bpm, + artists_list: Optional[List[str]] = None) -> List[str]: written = [] if title: audio['title'] = [title] @@ -395,6 +454,13 @@ def _write_vorbis(audio, title, artist, album_artist, album, year, genre, if artist: audio['artist'] = [artist] written.append('artist') + # Vorbis-style multi-value: write the per-artist list to the + # `artists` key (separate from `artist`, picard convention) when + # the caller supplied a list AND the user has multi-value write + # enabled. Mirrors enrichment.py. + if artists_list and len(artists_list) > 1 and _multi_artist_write_enabled(): + audio['artists'] = list(artists_list) + written.append('artists_multi') if album_artist: audio['albumartist'] = [album_artist] written.append('album_artist') @@ -421,14 +487,24 @@ def _write_vorbis(audio, title, artist, album_artist, album, year, genre, def _write_mp4(audio, title, artist, album_artist, album, year, genre, - track_num, total_tracks, disc_num, bpm) -> List[str]: + track_num, total_tracks, disc_num, bpm, + artists_list: Optional[List[str]] = None) -> List[str]: written = [] if title: audio['\xa9nam'] = [title] written.append('title') if artist: - audio['\xa9ART'] = [artist] - written.append('artist') + # MP4 \xa9ART can carry a list directly. When caller supplied + # a multi-value list AND user has multi-value write enabled, + # write the list. Otherwise single-string. Mirrors enrichment.py + # MP4 path. + if artists_list and len(artists_list) > 1 and _multi_artist_write_enabled(): + audio['\xa9ART'] = list(artists_list) + written.append('artist') + written.append('artists_multi') + else: + audio['\xa9ART'] = [artist] + written.append('artist') if album_artist: audio['aART'] = [album_artist] written.append('album_artist') diff --git a/core/tidal_download_client.py b/core/tidal_download_client.py index 45bdd4b9..a7efb40f 100644 --- a/core/tidal_download_client.py +++ b/core/tidal_download_client.py @@ -278,6 +278,27 @@ class TidalDownloadClient(DownloadSourcePlugin): return False return True + @classmethod + def _track_matches_qualifiers(cls, track, qualifiers: List[str]) -> bool: + """Issue #589 — qualifier check must inspect both track.name AND + track.album.name. For MTV Unplugged-style releases the live / + unplugged signal lives in the album title, not the track title. + A track passes if every required qualifier appears as a whole + word in either the track name OR its album name. + """ + if not qualifiers: + return True + track_name = (getattr(track, 'name', '') or '').lower() + album = getattr(track, 'album', None) + album_name = (getattr(album, 'name', '') or '').lower() if album else '' + haystack = f"{track_name} {album_name}".strip() + if not haystack: + return False + for kw in qualifiers: + if not re.search(r'\b' + re.escape(kw) + r'\b', haystack): + return False + return True + @staticmethod def _generate_shortened_queries(original: str) -> List[str]: variants: List[str] = [] @@ -342,25 +363,35 @@ class TidalDownloadClient(DownloadSourcePlugin): found = await loop.run_in_executor(None, _search) if found: + # Issue #589 — qualifier filter applies to ALL + # search attempts, not just fallbacks. If the + # primary query carries "live" / "unplugged" / + # etc, the studio cut should never be accepted + # just because Tidal returned it first. The + # filter inspects both track.name AND + # track.album.name (the live signal often lives + # in the album title for concert releases). is_fallback = attempt_idx > 0 - if is_fallback and required_qualifiers: + if required_qualifiers: filtered = [ t for t in found - if self._track_name_contains_qualifiers(getattr(t, 'name', ''), required_qualifiers) + if self._track_matches_qualifiers(t, required_qualifiers) ] if filtered: tidal_tracks = filtered successful_query = attempt_query logger.info( - f"Tidal fallback kept {len(filtered)}/{len(found)} tracks " - f"after qualifier filter {required_qualifiers} for '{attempt_query}'" + f"Tidal {'fallback' if is_fallback else 'primary'} kept " + f"{len(filtered)}/{len(found)} tracks after qualifier filter " + f"{required_qualifiers} for '{attempt_query}'" ) break else: any_fallback_filtered_out = True logger.debug( - f"Tidal fallback '{attempt_query}' returned {len(found)} tracks " - f"but none matched original qualifiers {required_qualifiers} — " + f"Tidal {'fallback' if is_fallback else 'primary'} " + f"'{attempt_query}' returned {len(found)} tracks but none " + f"matched required qualifiers {required_qualifiers} — " f"trying next variant" ) if attempt_idx < len(queries_to_try) - 1: diff --git a/tests/imports/test_duration_tolerance_resolution.py b/tests/imports/test_duration_tolerance_resolution.py new file mode 100644 index 00000000..a5ba6386 --- /dev/null +++ b/tests/imports/test_duration_tolerance_resolution.py @@ -0,0 +1,70 @@ +from core.imports.file_integrity import _MAX_USER_TOLERANCE_S, resolve_duration_tolerance + + +def test_none_returns_none_so_caller_uses_auto_scaled_default(): + assert resolve_duration_tolerance(None) is None + + +def test_missing_or_empty_string_returns_none(): + assert resolve_duration_tolerance("") is None + assert resolve_duration_tolerance(" ") is None + + +def test_zero_returns_none_to_avoid_strict_mode_ambiguity(): + # 0 means "unset" — never strict-mode (which would fail any drift). + # Users who want strict have no use-case; users who want disabled + # set a high value (capped to _MAX_USER_TOLERANCE_S). + assert resolve_duration_tolerance(0) is None + assert resolve_duration_tolerance(0.0) is None + assert resolve_duration_tolerance("0") is None + + +def test_negative_returns_none(): + assert resolve_duration_tolerance(-1) is None + assert resolve_duration_tolerance(-3.5) is None + assert resolve_duration_tolerance("-10") is None + + +def test_positive_integer_passes_through_as_float(): + assert resolve_duration_tolerance(5) == 5.0 + assert resolve_duration_tolerance(10) == 10.0 + + +def test_positive_float_passes_through(): + assert resolve_duration_tolerance(3.5) == 3.5 + assert resolve_duration_tolerance(0.1) == 0.1 + + +def test_numeric_string_parsed(): + assert resolve_duration_tolerance("5") == 5.0 + assert resolve_duration_tolerance("3.5") == 3.5 + assert resolve_duration_tolerance("10.0") == 10.0 + + +def test_unparseable_string_returns_none(): + assert resolve_duration_tolerance("abc") is None + assert resolve_duration_tolerance("five") is None + assert resolve_duration_tolerance("3s") is None + + +def test_above_max_clamped_to_ceiling(): + assert resolve_duration_tolerance(9999) == _MAX_USER_TOLERANCE_S + assert resolve_duration_tolerance(_MAX_USER_TOLERANCE_S + 1) == _MAX_USER_TOLERANCE_S + + +def test_at_ceiling_passes_through(): + assert resolve_duration_tolerance(_MAX_USER_TOLERANCE_S) == _MAX_USER_TOLERANCE_S + + +def test_non_numeric_types_return_none(): + assert resolve_duration_tolerance([5]) is None + assert resolve_duration_tolerance({"value": 5}) is None + assert resolve_duration_tolerance(object()) is None + + +def test_bool_treated_as_int_python_semantics(): + # Python: bool is int subclass. True -> 1.0, False -> 0 -> None. + # Documented behavior, not a bug — config values won't realistically + # be booleans for a numeric setting. + assert resolve_duration_tolerance(True) == 1.0 + assert resolve_duration_tolerance(False) is None diff --git a/tests/imports/test_import_routes.py b/tests/imports/test_import_routes.py new file mode 100644 index 00000000..83c57350 --- /dev/null +++ b/tests/imports/test_import_routes.py @@ -0,0 +1,568 @@ +import os +from concurrent.futures import Future + +import core.imports.routes as import_routes +from core.imports.routes import ( + ImportRouteRuntime, + album_match, + album_process, + process_single_import_file, + search_albums, + search_tracks, + singles_process, + staging_files, + staging_groups, + staging_hints, + staging_suggestions, +) + + +class _FakeLogger: + def __init__(self): + self.debug_messages = [] + self.error_messages = [] + self.info_messages = [] + self.warning_messages = [] + + def debug(self, msg, *args): + self.debug_messages.append(msg % args if args else msg) + + def error(self, msg, *args): + self.error_messages.append(msg % args if args else msg) + + def info(self, msg, *args): + self.info_messages.append(msg % args if args else msg) + + def warning(self, msg, *args): + self.warning_messages.append(msg % args if args else msg) + + +class _FakeHydrabaseWorker: + def __init__(self): + self.enqueued = [] + + def enqueue(self, query, kind): + self.enqueued.append((query, kind)) + + +class _FakeAutomationEngine: + def __init__(self, fail=False): + self.events = [] + self.fail = fail + + def emit(self, name, payload): + if self.fail: + raise RuntimeError("emit boom") + self.events.append((name, payload)) + + +class _FakeExecutor: + def __init__(self, outcomes=None): + self.outcomes = list(outcomes or []) + self.calls = [] + + def submit(self, fn, *args): + self.calls.append((fn, args)) + future = Future() + if self.outcomes: + outcome = self.outcomes.pop(0) + if isinstance(outcome, BaseException): + future.set_exception(outcome) + else: + future.set_result(outcome) + else: + future.set_result(fn(*args)) + return future + + +def _touch(path): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b"") + + +def _metadata_for(files): + def _read_metadata(file_path, rel_path): + return files[rel_path] + + return _read_metadata + + +def test_staging_files_returns_audio_files_with_metadata(tmp_path): + _touch(tmp_path / "Artist" / "02 - Song.flac") + _touch(tmp_path / "cover.jpg") + rel_song = os.path.join("Artist", "02 - Song.flac") + metadata = { + rel_song: { + "title": "Song", + "artist": "Track Artist", + "albumartist": "Album Artist", + "album": "Album", + "track_number": 2, + "disc_number": 1, + } + } + runtime = ImportRouteRuntime( + get_staging_path=lambda: str(tmp_path), + read_staging_file_metadata=_metadata_for(metadata), + logger=_FakeLogger(), + ) + + payload, status = staging_files(runtime) + + assert status == 200 + assert payload["success"] is True + assert payload["staging_path"] == str(tmp_path) + assert len(payload["files"]) == 1 + assert payload["files"] == [ + { + "filename": "02 - Song.flac", + "rel_path": rel_song, + "full_path": str(tmp_path / "Artist" / "02 - Song.flac"), + "title": "Song", + "artist": "Album Artist", + "album": "Album", + "track_number": 2, + "disc_number": 1, + "extension": ".flac", + } + ] + + +def test_staging_groups_only_returns_multi_file_album_groups(tmp_path): + _touch(tmp_path / "a.mp3") + _touch(tmp_path / "b.mp3") + _touch(tmp_path / "single.mp3") + metadata = { + "a.mp3": { + "title": "A", + "artist": "Artist", + "albumartist": "", + "album": "Album", + "track_number": 2, + "disc_number": 1, + }, + "b.mp3": { + "title": "B", + "artist": "Artist", + "albumartist": "", + "album": "Album", + "track_number": 1, + "disc_number": 1, + }, + "single.mp3": { + "title": "Single", + "artist": "Other", + "albumartist": "", + "album": "Other Album", + "track_number": 1, + "disc_number": 1, + }, + } + runtime = ImportRouteRuntime( + get_staging_path=lambda: str(tmp_path), + read_staging_file_metadata=_metadata_for(metadata), + logger=_FakeLogger(), + ) + + payload, status = staging_groups(runtime) + + assert status == 200 + assert payload["success"] is True + assert len(payload["groups"]) == 1 + group = payload["groups"][0] + assert group["album"] == "Album" + assert group["artist"] == "Artist" + assert group["file_count"] == 2 + assert [f["filename"] for f in group["files"]] == ["b.mp3", "a.mp3"] + + +def test_staging_hints_prefers_tag_queries_then_folder_queries(tmp_path): + _touch(tmp_path / "Folder_Album" / "01.mp3") + _touch(tmp_path / "Folder_Album" / "02.mp3") + _touch(tmp_path / "Loose" / "track.flac") + + def _read_tags(file_path): + if file_path.endswith("01.mp3") or file_path.endswith("02.mp3"): + return {"album": ["Tagged Album"], "artist": ["Tagged Artist"]} + return {} + + runtime = ImportRouteRuntime( + get_staging_path=lambda: str(tmp_path), + read_tags=_read_tags, + logger=_FakeLogger(), + ) + + payload, status = staging_hints(runtime) + + assert status == 200 + assert payload == { + "success": True, + "hints": ["Tagged Album Tagged Artist", "Folder Album", "Loose"], + } + + +def test_staging_suggestions_returns_cache_payload(monkeypatch): + monkeypatch.setattr( + import_routes, + "get_import_suggestions_cache", + lambda: {"suggestions": [{"album": "Album"}], "built": True}, + ) + + payload, status = staging_suggestions() + + assert status == 200 + assert payload == { + "success": True, + "suggestions": [{"album": "Album"}], + "ready": True, + } + + +def test_staging_groups_returns_empty_for_missing_staging_path(tmp_path): + runtime = ImportRouteRuntime( + get_staging_path=lambda: str(tmp_path / "missing"), + logger=_FakeLogger(), + ) + + payload, status = staging_groups(runtime) + + assert status == 200 + assert payload == {"success": True, "groups": []} + + +def test_staging_hints_returns_empty_for_missing_staging_path(tmp_path): + runtime = ImportRouteRuntime( + get_staging_path=lambda: str(tmp_path / "missing"), + logger=_FakeLogger(), + ) + + payload, status = staging_hints(runtime) + + assert status == 200 + assert payload == {"success": True, "hints": []} + + +def test_staging_files_returns_error_when_path_resolution_fails(): + logger = _FakeLogger() + runtime = ImportRouteRuntime( + get_staging_path=lambda: (_ for _ in ()).throw(RuntimeError("path boom")), + logger=logger, + ) + + payload, status = staging_files(runtime) + + assert status == 500 + assert payload["success"] is False + assert payload["error"] == "path boom" + assert logger.error_messages == ["Error scanning staging files: path boom"] + + +def test_staging_groups_returns_error_when_metadata_read_fails(tmp_path): + _touch(tmp_path / "a.mp3") + logger = _FakeLogger() + runtime = ImportRouteRuntime( + get_staging_path=lambda: str(tmp_path), + read_staging_file_metadata=lambda _file_path, _rel_path: (_ for _ in ()).throw(RuntimeError("tag boom")), + logger=logger, + ) + + payload, status = staging_groups(runtime) + + assert status == 500 + assert payload["success"] is False + assert payload["error"] == "tag boom" + assert logger.error_messages == ["Error building staging groups: tag boom"] + + +def test_staging_hints_returns_error_when_path_resolution_fails(): + logger = _FakeLogger() + runtime = ImportRouteRuntime( + get_staging_path=lambda: (_ for _ in ()).throw(RuntimeError("hint boom")), + logger=logger, + ) + + payload, status = staging_hints(runtime) + + assert status == 500 + assert payload["success"] is False + assert payload["error"] == "hint boom" + assert logger.error_messages == ["Error getting staging hints: hint boom"] + + +def test_search_albums_enqueues_hydrabase_and_caps_limit(): + worker = _FakeHydrabaseWorker() + calls = [] + runtime = ImportRouteRuntime( + get_primary_source=lambda: "hydrabase", + hydrabase_worker=worker, + dev_mode_enabled=True, + search_import_albums=lambda query, limit: calls.append((query, limit)) or [{"id": "album-1"}], + logger=_FakeLogger(), + ) + + payload, status = search_albums(runtime, " Album ", 99) + + assert status == 200 + assert payload == {"success": True, "albums": [{"id": "album-1"}]} + assert worker.enqueued == [("Album", "albums")] + assert calls == [("Album", 50)] + + +def test_search_albums_requires_query(): + payload, status = search_albums(ImportRouteRuntime(logger=_FakeLogger()), "", 12) + + assert status == 400 + assert payload == {"success": False, "error": "Missing query parameter"} + + +def test_search_tracks_enqueues_hydrabase_and_caps_limit(): + worker = _FakeHydrabaseWorker() + calls = [] + runtime = ImportRouteRuntime( + get_primary_source=lambda: "hydrabase", + hydrabase_worker=worker, + dev_mode_enabled=True, + search_import_tracks=lambda query, limit: calls.append((query, limit)) or [{"id": "track-1"}], + logger=_FakeLogger(), + ) + + payload, status = search_tracks(runtime, " Track ", 99) + + assert status == 200 + assert payload == {"success": True, "tracks": [{"id": "track-1"}]} + assert worker.enqueued == [("Track", "tracks")] + assert calls == [("Track", 30)] + + +def test_album_match_warns_without_source_and_passes_file_filter(): + logger = _FakeLogger() + calls = [] + runtime = ImportRouteRuntime( + build_album_import_match_payload=lambda *args, **kwargs: calls.append((args, kwargs)) or {"success": True, "matches": []}, + logger=logger, + ) + + payload, status = album_match( + runtime, + { + "album_id": "album-1", + "album_name": "Album", + "album_artist": "Artist", + "file_paths": ["a.flac", "b.flac"], + }, + ) + + assert status == 200 + assert payload == {"success": True, "matches": []} + assert calls == [ + ( + ("album-1",), + { + "album_name": "Album", + "album_artist": "Artist", + "file_paths": {"a.flac", "b.flac"}, + "source": None, + }, + ) + ] + assert len(logger.warning_messages) == 1 + assert "Missing 'source'" in logger.warning_messages[0] + + +def test_album_match_requires_album_id(): + payload, status = album_match(ImportRouteRuntime(logger=_FakeLogger()), {}) + + assert status == 400 + assert payload == {"success": False, "error": "Missing album_id"} + + +def test_album_process_posts_valid_files_and_records_side_effects(tmp_path): + good_file = tmp_path / "good.flac" + _touch(good_file) + processed_contexts = [] + activity = [] + refresh_calls = [] + automation = _FakeAutomationEngine() + runtime = ImportRouteRuntime( + resolve_album_artist_context=lambda album, source="": {"name": "Artist"}, + build_album_import_context=lambda album, track, **kwargs: {"album": album, "track": track, **kwargs}, + post_process_matched_download=lambda key, context, path: processed_contexts.append((key, context, path)), + add_activity_item=lambda *args: activity.append(args), + refresh_import_suggestions_cache=lambda: refresh_calls.append("refresh"), + automation_engine=automation, + logger=_FakeLogger(), + ) + + payload, status = album_process( + runtime, + { + "album": {"id": "album-1", "name": "Album", "artist": "Artist", "source": "deezer"}, + "matches": [ + { + "staging_file": {"full_path": str(good_file), "filename": "good.flac"}, + "track": {"name": "Good Track", "track_number": 1, "disc_number": 2}, + }, + { + "staging_file": {"full_path": str(tmp_path / "missing.flac"), "filename": "missing.flac"}, + "track": {"name": "Missing Track", "track_number": 2}, + }, + ], + }, + ) + + assert status == 200 + assert payload == { + "success": True, + "processed": 1, + "total": 2, + "errors": ["File not found: missing.flac"], + } + assert len(processed_contexts) == 1 + key, context, path = processed_contexts[0] + assert key.startswith("import_album_album-1_1_") + assert context["artist_context"] == {"name": "Artist"} + assert context["total_discs"] == 2 + assert context["source"] == "deezer" + assert path == str(good_file) + assert activity == [("", "Album Imported", "Album by Artist (1/2 tracks)", "Now")] + assert refresh_calls == ["refresh"] + assert automation.events == [ + ( + "import_completed", + {"track_count": "1", "album_name": "Album", "artist": "Artist"}, + ), + ( + "batch_complete", + { + "playlist_name": "Import: Album", + "total_tracks": "2", + "completed_tracks": "1", + "failed_tracks": "1", + }, + ), + ] + + +def test_album_process_requires_album_and_matches(): + payload, status = album_process(ImportRouteRuntime(logger=_FakeLogger()), {"album": {}, "matches": []}) + + assert status == 400 + assert payload == {"success": False, "error": "Missing album or matches data"} + + +def test_process_single_import_file_resolves_and_posts_context(tmp_path): + audio_file = tmp_path / "Artist - Song.flac" + _touch(audio_file) + post_calls = [] + runtime = ImportRouteRuntime( + parse_filename_metadata=lambda filename: {"title": "Song", "artist": "Artist"}, + get_single_track_import_context=lambda title, artist, **kwargs: { + "source": "deezer", + "context": {"track": {"name": title}, "artist": {"name": artist}}, + }, + normalize_import_context=lambda context: context, + get_import_context_artist=lambda context: context["artist"], + get_import_track_info=lambda context: context["track"], + post_process_matched_download=lambda key, context, path: post_calls.append((key, context, path)), + logger=_FakeLogger(), + ) + + outcome = process_single_import_file(runtime, {"full_path": str(audio_file), "filename": audio_file.name}) + + assert outcome == ("ok", "Song") + assert len(post_calls) == 1 + assert post_calls[0][0].startswith("import_single_") + assert post_calls[0][1] == {"track": {"name": "Song"}, "artist": {"name": "Artist"}} + assert post_calls[0][2] == str(audio_file) + + +def test_process_single_import_file_rejects_malformed_manual_match(tmp_path): + audio_file = tmp_path / "Song.flac" + _touch(audio_file) + runtime = ImportRouteRuntime(post_process_matched_download=lambda *_args: None, logger=_FakeLogger()) + + outcome = process_single_import_file( + runtime, + {"full_path": str(audio_file), "filename": audio_file.name, "manual_match": {"id": "track-1"}}, + ) + + assert outcome == ("error", "Malformed manual match for file: Song.flac") + + +def test_singles_process_aggregates_worker_results_and_side_effects(): + activity = [] + refresh_calls = [] + automation = _FakeAutomationEngine() + executor = _FakeExecutor(outcomes=[("ok", "Song"), ("error", "Bad Song")]) + runtime = ImportRouteRuntime( + import_singles_executor=executor, + add_activity_item=lambda *args: activity.append(args), + refresh_import_suggestions_cache=lambda: refresh_calls.append("refresh"), + automation_engine=automation, + logger=_FakeLogger(), + ) + + payload, status = singles_process( + runtime, + [{"filename": "a.flac"}, {"filename": "b.flac"}], + ) + + assert status == 200 + assert payload == { + "success": True, + "processed": 1, + "total": 2, + "errors": ["Bad Song"], + } + assert len(executor.calls) == 2 + assert activity == [("", "Singles Imported", "1/2 tracks processed", "Now")] + assert refresh_calls == ["refresh"] + assert automation.events == [ + ( + "import_completed", + {"track_count": "1", "album_name": "", "artist": "Various"}, + ), + ( + "batch_complete", + { + "playlist_name": "Import: Singles", + "total_tracks": "2", + "completed_tracks": "1", + "failed_tracks": "1", + }, + ), + ] + + +def test_singles_process_uses_injected_single_file_worker(): + calls = [] + executor = _FakeExecutor() + + def fake_process_file(runtime, file_info): + calls.append((runtime, file_info)) + return ("ok", file_info["filename"]) + + runtime = ImportRouteRuntime( + import_singles_executor=executor, + process_single_import_file=fake_process_file, + logger=_FakeLogger(), + ) + + payload, status = singles_process(runtime, [{"filename": "patched.flac"}]) + + assert status == 200 + assert payload == { + "success": True, + "processed": 1, + "total": 1, + "errors": [], + } + assert calls == [(runtime, {"filename": "patched.flac"})] + assert executor.calls[0][0] is fake_process_file + + +def test_singles_process_requires_files(): + payload, status = singles_process(ImportRouteRuntime(logger=_FakeLogger()), []) + + assert status == 400 + assert payload == {"success": False, "error": "No files provided"} diff --git a/tests/imports/test_quarantine_management.py b/tests/imports/test_quarantine_management.py new file mode 100644 index 00000000..0c46055f --- /dev/null +++ b/tests/imports/test_quarantine_management.py @@ -0,0 +1,273 @@ +import json +import os + +from core.imports.quarantine import ( + approve_quarantine_entry, + delete_quarantine_entry, + list_quarantine_entries, + recover_to_staging, + serialize_quarantine_context, +) + + +# ────────────────────────────────────────────────────────────────────── +# serialize_quarantine_context — JSON-safe coercion +# ────────────────────────────────────────────────────────────────────── + +def test_serialize_passes_scalar_dict_unchanged(): + ctx = {"title": "DNA.", "track_number": 2, "active": True, "missing": None, "duration_ms": 185000} + out = serialize_quarantine_context(ctx) + assert out == ctx + + +def test_serialize_walks_nested_dicts(): + ctx = {"track_info": {"name": "DNA.", "artists": [{"name": "Kendrick"}, {"name": "Rihanna"}]}} + out = serialize_quarantine_context(ctx) + assert out == ctx + + +def test_serialize_coerces_set_to_list(): + ctx = {"sources": {"spotify", "deezer"}} + out = serialize_quarantine_context(ctx) + assert sorted(out["sources"]) == ["deezer", "spotify"] + + +def test_serialize_coerces_tuple_to_list(): + ctx = {"pair": (1, 2, 3)} + out = serialize_quarantine_context(ctx) + assert out == {"pair": [1, 2, 3]} + + +def test_serialize_stringifies_unknown_objects(): + class Custom: + def __str__(self): + return "" + out = serialize_quarantine_context({"obj": Custom()}) + assert out["obj"] == "" + + +def test_serialize_non_dict_returns_empty_dict(): + assert serialize_quarantine_context(None) == {} + assert serialize_quarantine_context("string") == {} + assert serialize_quarantine_context([1, 2, 3]) == {} + + +def test_serialize_round_trips_through_json(): + ctx = { + "track_info": {"name": "X", "artists": [{"name": "A"}, {"name": "B"}]}, + "spotify_artist": {"name": "A", "id": "abc"}, + "duration_ms": 180000, + "sources": {"spotify"}, + } + serialized = serialize_quarantine_context(ctx) + json.dumps(serialized) # must not raise + + +# ────────────────────────────────────────────────────────────────────── +# list_quarantine_entries +# ────────────────────────────────────────────────────────────────────── + +def _write_entry(quarantine_dir, entry_id, original_name, *, with_context=False, trigger="integrity", reason="boom", file_bytes=b"X" * 100): + qfile = quarantine_dir / f"{entry_id}_{original_name}.quarantined" + qfile.write_bytes(file_bytes) + sidecar = { + "original_filename": original_name, + "quarantine_reason": reason, + "expected_track": "Track", + "expected_artist": "Artist", + "timestamp": "2026-05-14T12:00:00", + "trigger": trigger, + } + if with_context: + sidecar["context"] = {"track_info": {"name": "Track"}, "context_key": entry_id} + sidecar_path = quarantine_dir / f"{entry_id}_{os.path.splitext(original_name)[0]}.json" + sidecar_path.write_text(json.dumps(sidecar)) + return qfile, sidecar_path + + +def test_list_returns_empty_for_missing_dir(tmp_path): + assert list_quarantine_entries(str(tmp_path / "nope")) == [] + + +def test_list_returns_empty_for_empty_dir(tmp_path): + assert list_quarantine_entries(str(tmp_path)) == [] + + +def test_list_returns_entry_with_sidecar_fields(tmp_path): + _write_entry(tmp_path, "20260514_120000", "song.flac", reason="Duration mismatch") + entries = list_quarantine_entries(str(tmp_path)) + assert len(entries) == 1 + e = entries[0] + assert e["original_filename"] == "song.flac" + assert e["reason"] == "Duration mismatch" + assert e["expected_track"] == "Track" + assert e["expected_artist"] == "Artist" + assert e["has_full_context"] is False + assert e["trigger"] == "integrity" + assert e["size_bytes"] == 100 + + +def test_list_flags_full_context_entries(tmp_path): + _write_entry(tmp_path, "20260514_120000", "song.flac", with_context=True) + entries = list_quarantine_entries(str(tmp_path)) + assert entries[0]["has_full_context"] is True + + +def test_list_handles_orphan_quarantined_file_without_sidecar(tmp_path): + qfile = tmp_path / "20260514_120000_orphan.flac.quarantined" + qfile.write_bytes(b"X") + entries = list_quarantine_entries(str(tmp_path)) + assert len(entries) == 1 + assert entries[0]["reason"] == "Unknown reason" + assert entries[0]["has_full_context"] is False + + +def test_list_skips_orphan_sidecars_without_file(tmp_path): + sidecar = tmp_path / "20260514_120000_only.json" + sidecar.write_text(json.dumps({"original_filename": "only.flac", "quarantine_reason": "x"})) + assert list_quarantine_entries(str(tmp_path)) == [] + + +def test_list_sorts_newest_first(tmp_path): + _write_entry(tmp_path, "20260101_120000", "old.flac") + _write_entry(tmp_path, "20260514_120000", "new.flac") + entries = list_quarantine_entries(str(tmp_path)) + assert entries[0]["original_filename"] == "new.flac" + assert entries[1]["original_filename"] == "old.flac" + + +def test_list_swallows_corrupt_sidecar_gracefully(tmp_path): + qfile = tmp_path / "20260514_120000_song.flac.quarantined" + qfile.write_bytes(b"X") + sidecar = tmp_path / "20260514_120000_song.json" + sidecar.write_text("{ this is not valid json") + entries = list_quarantine_entries(str(tmp_path)) + assert len(entries) == 1 + assert entries[0]["reason"] == "Unknown reason" + + +# ────────────────────────────────────────────────────────────────────── +# delete_quarantine_entry +# ────────────────────────────────────────────────────────────────────── + +def test_delete_removes_both_file_and_sidecar(tmp_path): + _write_entry(tmp_path, "20260514_120000", "song.flac") + assert delete_quarantine_entry(str(tmp_path), "20260514_120000_song") is True + assert not (tmp_path / "20260514_120000_song.flac.quarantined").exists() + assert not (tmp_path / "20260514_120000_song.json").exists() + + +def test_delete_returns_false_when_entry_missing(tmp_path): + assert delete_quarantine_entry(str(tmp_path), "nonexistent") is False + + +def test_delete_handles_orphan_file_without_sidecar(tmp_path): + qfile = tmp_path / "20260514_120000_orphan.flac.quarantined" + qfile.write_bytes(b"X") + assert delete_quarantine_entry(str(tmp_path), "20260514_120000_orphan") is True + assert not qfile.exists() + + +# ────────────────────────────────────────────────────────────────────── +# approve_quarantine_entry — full-context path +# ────────────────────────────────────────────────────────────────────── + +def test_approve_restores_file_and_returns_context_and_trigger(tmp_path): + quarantine = tmp_path / "ss_quarantine" + quarantine.mkdir() + restore = tmp_path / "restore" + + _write_entry(quarantine, "20260514_120000", "song.flac", with_context=True, trigger="integrity") + + result = approve_quarantine_entry(str(quarantine), "20260514_120000_song", str(restore)) + assert result is not None + restored_path, context, trigger = result + assert os.path.basename(restored_path) == "song.flac" + assert os.path.isfile(restored_path) + assert context["track_info"]["name"] == "Track" + assert trigger == "integrity" + # Sidecar removed after approve + assert not (quarantine / "20260514_120000_song.json").exists() + + +def test_approve_returns_none_for_thin_sidecar_without_context(tmp_path): + _write_entry(tmp_path, "20260514_120000", "song.flac", with_context=False) + result = approve_quarantine_entry(str(tmp_path), "20260514_120000_song", str(tmp_path / "restore")) + assert result is None + + +def test_approve_returns_none_for_missing_entry(tmp_path): + assert approve_quarantine_entry(str(tmp_path), "nope", str(tmp_path)) is None + + +def test_approve_avoids_filename_collision(tmp_path): + quarantine = tmp_path / "q" + quarantine.mkdir() + restore = tmp_path / "r" + restore.mkdir() + (restore / "song.flac").write_bytes(b"existing") + _write_entry(quarantine, "20260514_120000", "song.flac", with_context=True) + result = approve_quarantine_entry(str(quarantine), "20260514_120000_song", str(restore)) + assert result is not None + restored_path = result[0] + assert os.path.basename(restored_path) == "song_(2).flac" + assert (restore / "song.flac").read_bytes() == b"existing" + + +# ────────────────────────────────────────────────────────────────────── +# recover_to_staging — fallback for thin sidecars +# ────────────────────────────────────────────────────────────────────── + +def test_recover_strips_prefix_and_suffix(tmp_path): + quarantine = tmp_path / "q" + quarantine.mkdir() + staging = tmp_path / "s" + + qfile, _ = _write_entry(quarantine, "20260514_120000", "song.flac") + + target = recover_to_staging(str(quarantine), str(staging), "20260514_120000_song") + assert target is not None + assert os.path.basename(target) == "song.flac" + assert os.path.isfile(target) + assert not qfile.exists() + + +def test_recover_uses_sidecar_original_filename_when_available(tmp_path): + quarantine = tmp_path / "q" + quarantine.mkdir() + staging = tmp_path / "s" + qfile = quarantine / "20260514_120000_munged_name.flac.quarantined" + qfile.write_bytes(b"X") + sidecar = quarantine / "20260514_120000_munged_name.json" + sidecar.write_text(json.dumps({"original_filename": "Pretty Track Name.flac"})) + + target = recover_to_staging(str(quarantine), str(staging), "20260514_120000_munged_name") + assert target is not None + assert os.path.basename(target) == "Pretty Track Name.flac" + + +def test_recover_returns_none_for_missing_entry(tmp_path): + assert recover_to_staging(str(tmp_path / "q"), str(tmp_path / "s"), "nope") is None + + +def test_recover_avoids_filename_collision(tmp_path): + quarantine = tmp_path / "q" + quarantine.mkdir() + staging = tmp_path / "s" + staging.mkdir() + (staging / "song.flac").write_bytes(b"existing") + _write_entry(quarantine, "20260514_120000", "song.flac") + + target = recover_to_staging(str(quarantine), str(staging), "20260514_120000_song") + assert target is not None + assert os.path.basename(target) == "song_(2).flac" + + +def test_recover_removes_sidecar_after_move(tmp_path): + quarantine = tmp_path / "q" + quarantine.mkdir() + staging = tmp_path / "s" + _, sidecar = _write_entry(quarantine, "20260514_120000", "song.flac") + + recover_to_staging(str(quarantine), str(staging), "20260514_120000_song") + assert not sidecar.exists() diff --git a/tests/matching/test_acoustid_candidates.py b/tests/matching/test_acoustid_candidates.py new file mode 100644 index 00000000..81d2bbce --- /dev/null +++ b/tests/matching/test_acoustid_candidates.py @@ -0,0 +1,201 @@ +"""Tests for the shared AcoustID candidate-matching helper. + +Issue #587 / Foxxify report — scanner used to treat ``recordings[0]`` +as authoritative, so when AcoustID returned multiple candidates and +the top one was the wrong-credited recording (different MB entry +under the same fingerprint), the scanner created a false-positive +"Wrong download" finding even though a lower-ranked candidate matched +the expected metadata exactly. +""" + +from __future__ import annotations + +from difflib import SequenceMatcher + +import pytest + +from core.matching.acoustid_candidates import ( + duration_mismatches_strongly, + find_matching_recording, +) + + +def _ratio_sim(a: str, b: str) -> float: + """Reasonable test similarity that handles non-trivial differences.""" + if not a or not b: + return 0.0 + return SequenceMatcher(None, a.lower().strip(), b.lower().strip()).ratio() + + +# ────────────────────────────────────────────────────────────────────── +# find_matching_recording +# ────────────────────────────────────────────────────────────────────── + +def test_top_recording_matches_returns_immediately(): + recordings = [ + {'title': 'Nana', 'artist': 'Geoxor'}, + {'title': 'Nana', 'artist': 'Edward Vesala Trio'}, + ] + result, t_sim, a_sim = find_matching_recording( + recordings, 'Nana', 'Geoxor', similarity=_ratio_sim, + ) + assert result == {'title': 'Nana', 'artist': 'Geoxor'} + assert t_sim == 1.0 + assert a_sim == 1.0 + + +def test_falls_through_to_lower_ranked_match_for_foxxify_nana_case(): + """Reporter case 2: top AcoustID candidate is 'Nana' by 'Edward + Vesala Trio' (97% fingerprint), but the LOWER-ranked candidate + is the expected 'Nana' by 'Geoxor'. Pre-fix scanner saw only [0] + and flagged. Post-fix returns the matching candidate.""" + recordings = [ + {'title': 'Nana', 'artist': 'Edward Vesala Trio'}, # AcoustID's top match + {'title': 'Nana', 'artist': 'Geoxor'}, # the actual right one + ] + result, _, _ = find_matching_recording( + recordings, 'Nana', 'Geoxor', similarity=_ratio_sim, + ) + assert result == {'title': 'Nana', 'artist': 'Geoxor'} + + +def test_no_match_returns_none_with_best_seen_sims(): + """When no candidate passes thresholds, return the best-seen sims + so callers can log the closest near-miss in the finding.""" + recordings = [ + {'title': 'Different Song', 'artist': 'Different Artist'}, + {'title': 'Sort Of Close', 'artist': 'Different Artist'}, + ] + result, t_sim, a_sim = find_matching_recording( + recordings, 'Different', 'AnotherArtist', + similarity=_ratio_sim, + title_threshold=0.95, + artist_threshold=0.95, + ) + assert result is None + # Best seen — even though no candidate passed the threshold + assert t_sim > 0.0 + assert a_sim > 0.0 + + +def test_skips_recordings_missing_title_or_artist(): + recordings = [ + {'title': None, 'artist': 'Geoxor'}, + {'title': 'Nana', 'artist': ''}, + {'title': 'Nana', 'artist': 'Geoxor'}, + ] + result, _, _ = find_matching_recording( + recordings, 'Nana', 'Geoxor', similarity=_ratio_sim, + ) + assert result == {'title': 'Nana', 'artist': 'Geoxor'} + + +def test_skips_non_dict_entries(): + recordings = [None, 'string', {'title': 'Nana', 'artist': 'Geoxor'}] + result, _, _ = find_matching_recording( + recordings, 'Nana', 'Geoxor', similarity=_ratio_sim, + ) + assert result == {'title': 'Nana', 'artist': 'Geoxor'} + + +def test_empty_inputs_return_none(): + assert find_matching_recording([], 'X', 'Y')[0] is None + assert find_matching_recording([{'title': 'X', 'artist': 'Y'}], '', 'Y')[0] is None + assert find_matching_recording([{'title': 'X', 'artist': 'Y'}], 'X', '')[0] is None + + +def test_separate_artist_similarity_function_is_honored(): + """Verifier passes alias-aware comparison via artist_similarity. + Make sure it's used instead of the generic similarity.""" + recordings = [{'title': 'Track', 'artist': '澤野弘之'}] + + def alias_aware(expected, actual): + # Pretend our alias chain bridges Hiroyuki Sawano ↔ 澤野弘之 + if expected == 'Hiroyuki Sawano' and actual == '澤野弘之': + return 1.0 + return 0.0 + + result, _, a_sim = find_matching_recording( + recordings, 'Track', 'Hiroyuki Sawano', + similarity=_ratio_sim, + artist_similarity=alias_aware, + ) + assert result is not None + assert a_sim == 1.0 + + +def test_skip_predicate_drops_unwanted_candidates(): + """Verifier uses skip_predicate to drop wrong-version recordings + (instrumental vs vocal, etc.).""" + recordings = [ + {'title': 'Track (Instrumental)', 'artist': 'X'}, + {'title': 'Track', 'artist': 'X'}, + ] + result, _, _ = find_matching_recording( + recordings, 'Track', 'X', + similarity=_ratio_sim, + skip_predicate=lambda r: 'instrumental' in (r.get('title') or '').lower(), + ) + assert result == {'title': 'Track', 'artist': 'X'} + + +def test_title_threshold_can_be_lowered_for_loose_matching(): + recordings = [{'title': 'Sort Of Close', 'artist': 'Right Artist'}] + # With strict default threshold this fails + result_strict, _, _ = find_matching_recording( + recordings, 'Different', 'Right Artist', similarity=_ratio_sim, + ) + assert result_strict is None + # With a permissive threshold the artist match alone wouldn't help — + # title sim must also pass. + result_loose, _, _ = find_matching_recording( + recordings, 'Different', 'Right Artist', + similarity=_ratio_sim, title_threshold=0.0, + ) + assert result_loose is not None + + +# ────────────────────────────────────────────────────────────────────── +# duration_mismatches_strongly — guard against fingerprint collisions +# ────────────────────────────────────────────────────────────────────── + +def test_durations_within_tolerance_pass(): + # 3-minute track, 1-second drift — well within tolerance + assert duration_mismatches_strongly(180, 181) is False + # 3-minute vs 4-minute — within the 60s absolute tolerance + assert duration_mismatches_strongly(180, 240) is False + + +def test_drift_above_absolute_floor_flags(): + # 3-minute expected, 5-minute candidate (120s drift > 63s threshold) + assert duration_mismatches_strongly(180, 300) is True + + +def test_relative_tolerance_scales_with_long_tracks(): + # 30-minute expected vs 12-minute candidate (1080s vs 720s) — + # 18-minute drift > 35% of 30min = 10.5min → mismatch + assert duration_mismatches_strongly(1800, 720) is True + # 30-minute expected vs 28-minute candidate — 2min drift = under + # max(60s, 35%*30min) = max(60, 630) = 630s → still safe + assert duration_mismatches_strongly(1800, 1680) is False + + +def test_reporter_17min_mashup_vs_5min_track_flagged(): + """Foxxify's 17min mashup edit vs 5min late-70s Japanese hiphop — + fingerprint collision. Duration guard should mark this suspicious.""" + assert duration_mismatches_strongly(17 * 60, 5 * 60) is True + + +def test_unknown_duration_returns_false_no_behavior_change(): + """When either side is missing duration, don't change behavior.""" + assert duration_mismatches_strongly(None, 300) is False + assert duration_mismatches_strongly(180, None) is False + assert duration_mismatches_strongly(0, 300) is False + assert duration_mismatches_strongly(180, 0) is False + assert duration_mismatches_strongly(-5, 300) is False + + +def test_string_or_int_durations_handled(): + # Defensive — coerce numeric types + assert duration_mismatches_strongly(180.5, 181.0) is False + assert duration_mismatches_strongly(int(180), int(300)) is True diff --git a/tests/matching/test_album_context_title.py b/tests/matching/test_album_context_title.py new file mode 100644 index 00000000..0c711a19 --- /dev/null +++ b/tests/matching/test_album_context_title.py @@ -0,0 +1,168 @@ +"""Tests for the album-context-aware track-title stripping helper. + +Issue #589 — MTV Unplugged track titles like ``"Shy Away (MTV Unplugged +Live)"`` got false-rejected by the album-scoped library check because +the local DB stored title is just ``"Shy Away"``. The pure helper here +strips the redundant suffix when (and only when) the album title +implies the same context. +""" + +from core.matching.album_context_title import ( + album_context_markers, + strip_redundant_album_suffix, +) + + +# ────────────────────────────────────────────────────────────────────── +# album_context_markers +# ────────────────────────────────────────────────────────────────────── + +def test_mtv_unplugged_album_carries_unplugged_marker(): + # 'mtv' isn't a version marker on its own — the unplugged token is + # the load-bearing one. Implied-live logic adds 'live' coverage too. + markers = album_context_markers('MTV Unplugged') + assert 'unplugged' in markers + + +def test_live_at_album_carries_live_marker(): + markers = album_context_markers('Live At Wembley') + assert 'live' in markers + + +def test_studio_album_has_no_markers(): + assert album_context_markers('Scorpion') == () + assert album_context_markers('DAMN.') == () + assert album_context_markers('') == () + assert album_context_markers(None) == () + + +def test_acoustic_session_album_marker(): + assert 'acoustic' in album_context_markers('Acoustic Sessions Vol. 2') + assert 'session' in album_context_markers('Acoustic Sessions Vol. 2') + + +# ────────────────────────────────────────────────────────────────────── +# strip_redundant_album_suffix — the headline cases from #589 +# ────────────────────────────────────────────────────────────────────── + +def test_strips_mtv_unplugged_live_suffix_when_album_is_mtv_unplugged(): + assert strip_redundant_album_suffix('Shy Away (MTV Unplugged Live)', 'MTV Unplugged') == 'Shy Away' + + +def test_strips_complex_mtv_unplugged_suffix_with_year(): + # Reporter case 2: "Only If For A Night (MTV Unplugged, 2012 / Live)" + assert strip_redundant_album_suffix( + 'Only If For A Night (MTV Unplugged, 2012 / Live)', + 'Ceremonials (Live At MTV Unplugged)', + ) == 'Only If For A Night' + + +def test_strips_dash_style_live_suffix_when_album_is_live(): + assert strip_redundant_album_suffix( + 'Bohemian Rhapsody - Live At Wembley', + 'Live At Wembley Stadium', + ) == 'Bohemian Rhapsody' + + +def test_strips_brackets_live_suffix(): + assert strip_redundant_album_suffix( + 'Hello [Live]', + 'Live At The Royal Albert Hall', + ) == 'Hello' + + +# ────────────────────────────────────────────────────────────────────── +# Negative cases — must NOT strip when it would mask a genuine variant +# ────────────────────────────────────────────────────────────────────── + +def test_does_not_strip_instrumental_when_album_is_studio(): + # Critical anti-regression — keeping AcoustID's vocal/instrumental + # gate working downstream. Don't drop the marker just because the + # title is on a studio album. + assert strip_redundant_album_suffix( + 'In My Feelings (Instrumental)', + 'Scorpion', + ) == 'In My Feelings (Instrumental)' + + +def test_does_not_strip_remix_when_album_is_studio(): + assert strip_redundant_album_suffix( + 'Hello (Acoustic Remix)', + 'Scorpion', + ) == 'Hello (Acoustic Remix)' + + +def test_does_not_strip_live_when_album_does_not_imply_live(): + # User's "Live At Wembley" might be a single-track release on an + # otherwise-studio album. Don't strip. + assert strip_redundant_album_suffix( + 'Hello (Live At Wembley)', + 'Greatest Hits', + ) == 'Hello (Live At Wembley)' + + +def test_does_not_strip_when_suffix_carries_extra_context(): + # Suffix has both the album marker AND a featured-artist credit; + # the credit isn't album context, so keep the suffix. + assert strip_redundant_album_suffix( + 'Track Name (Live - feat. Other Artist)', + 'Live At Wembley', + ) == 'Track Name (Live - feat. Other Artist)' + + +def test_no_suffix_returns_unchanged(): + assert strip_redundant_album_suffix('Shy Away', 'MTV Unplugged') == 'Shy Away' + + +def test_empty_or_none_inputs_handled(): + assert strip_redundant_album_suffix('', 'MTV Unplugged') == '' + assert strip_redundant_album_suffix(None, 'MTV Unplugged') == '' + assert strip_redundant_album_suffix('Shy Away', '') == 'Shy Away' + assert strip_redundant_album_suffix('Shy Away', None) == 'Shy Away' + + +# ────────────────────────────────────────────────────────────────────── +# Stacked-suffix cases +# ────────────────────────────────────────────────────────────────────── + +def test_strips_stacked_redundant_suffixes(): + # Some sources double up: parens + brackets, both album-context + assert strip_redundant_album_suffix( + 'Track Name (Live) [Unplugged]', + 'MTV Unplugged Live', + ) == 'Track Name' + + +def test_stops_stripping_when_remaining_suffix_is_genuine(): + # Outer is redundant (live → album-context), inner is not (remix) + assert strip_redundant_album_suffix( + 'Track Name (Remix) (Live)', + 'Live At Wembley', + ) == 'Track Name (Remix)' + + +# ────────────────────────────────────────────────────────────────────── +# Year + connector tolerance +# ────────────────────────────────────────────────────────────────────── + +def test_year_in_suffix_does_not_block_stripping(): + assert strip_redundant_album_suffix( + 'Track Name (Live, 2012)', + 'Live At Wembley', + ) == 'Track Name' + + +def test_version_word_in_suffix_does_not_block_stripping(): + # "Live Version" is still album-context (just the word "version" + # in there). Strip. + assert strip_redundant_album_suffix( + 'Track Name (Live Version)', + 'Live At Wembley', + ) == 'Track Name' + + +def test_session_marker_preserved_for_acoustic_session_album(): + assert strip_redundant_album_suffix( + 'Hello (Acoustic Session)', + 'Acoustic Sessions Vol. 2', + ) == 'Hello' diff --git a/tests/matching/test_artist_alias_lookup_586.py b/tests/matching/test_artist_alias_lookup_586.py new file mode 100644 index 00000000..97c9f03d --- /dev/null +++ b/tests/matching/test_artist_alias_lookup_586.py @@ -0,0 +1,243 @@ +"""Cross-script artist alias lookup — issue #586. + +The verifier's alias path was missing the Cyrillic spelling for +"Dmitry Yablonsky" because: + +1. ``fetch_artist_aliases`` only read ``data['aliases']`` and ignored + the canonical ``name`` / ``sort-name`` fields. When MB's canonical + name IS the cross-script form, the Latin spelling never made it + into the alias output (and vice-versa). + +2. ``lookup_artist_aliases`` ran search in strict mode only, which + queries ``artist:"..."`` and skips alias / sortname indexes. Cross- + script searches found nothing under strict. + +3. The trust gate weighted local similarity 70%, so cross-script + matches scored ~0.30 even when MB's own confidence was 100, getting + rejected as low-confidence. + +These tests pin all three fixes plus the original Hiroyuki Sawano +case from #442 (regression guard). +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + +from core.musicbrainz_service import MusicBrainzService + + +@pytest.fixture +def service(): + """Build a MusicBrainzService with mocked client + DB. The instance + is built bypassing __init__ so we don't need real config / DB.""" + svc = MusicBrainzService.__new__(MusicBrainzService) + svc.mb_client = MagicMock() + svc._calculate_similarity = lambda a, b: _simple_sim(a, b) + svc.get_artist_aliases = MagicMock(return_value=[]) + svc._check_cache = MagicMock(return_value=None) + svc._save_to_cache = MagicMock() + return svc + + +def _simple_sim(a: str, b: str) -> float: + """Tiny stub similarity — exact match=1.0 else 0.0. Cross-script + pairs naturally fall into 0.0 since no characters overlap.""" + if not a or not b: + return 0.0 + return 1.0 if a.lower() == b.lower() else 0.0 + + +# ────────────────────────────────────────────────────────────────────── +# fetch_artist_aliases — canonical name + sort-name now included +# ────────────────────────────────────────────────────────────────────── + +def test_fetch_aliases_includes_canonical_name(service): + service.mb_client.get_artist.return_value = { + 'name': 'Дмитрий Яблонский', + 'sort-name': 'Yablonsky, Dmitry', + 'aliases': [ + {'name': 'Dmitry Yablonsky', 'sort-name': 'Yablonsky, Dmitry'}, + ], + } + aliases = service.fetch_artist_aliases('mbid-yablonsky') + # Canonical name MUST be present so cross-script matching works + # whichever direction the canonical form points. + assert 'Дмитрий Яблонский' in aliases + assert 'Dmitry Yablonsky' in aliases + # Sort-name covered too + assert 'Yablonsky, Dmitry' in aliases + + +def test_fetch_aliases_dedupes_canonical_against_alias_entry(service): + # MB sometimes lists the canonical name as ALSO an alias entry. + # No duplicate output. + service.mb_client.get_artist.return_value = { + 'name': 'Hiroyuki Sawano', + 'sort-name': 'Sawano, Hiroyuki', + 'aliases': [ + {'name': 'Hiroyuki Sawano', 'sort-name': 'Sawano, Hiroyuki'}, + {'name': '澤野弘之'}, + ], + } + aliases = service.fetch_artist_aliases('mbid-sawano') + assert aliases.count('Hiroyuki Sawano') == 1 + assert '澤野弘之' in aliases + + +def test_fetch_aliases_handles_missing_canonical_gracefully(service): + service.mb_client.get_artist.return_value = { + 'aliases': [{'name': 'Dmitry Yablonsky'}], + } + aliases = service.fetch_artist_aliases('mbid-x') + assert aliases == ['Dmitry Yablonsky'] + + +def test_fetch_aliases_returns_empty_on_no_data(service): + service.mb_client.get_artist.return_value = None + assert service.fetch_artist_aliases('mbid-x') == [] + + +def test_fetch_aliases_returns_empty_on_exception(service): + service.mb_client.get_artist.side_effect = RuntimeError('boom') + assert service.fetch_artist_aliases('mbid-x') == [] + + +# ────────────────────────────────────────────────────────────────────── +# lookup_artist_aliases — strict + non-strict fallback +# ────────────────────────────────────────────────────────────────────── + +def test_lookup_falls_back_to_non_strict_when_strict_returns_nothing(service): + # Strict search returns nothing (typical cross-script case). + # Non-strict hits the alias index and finds the artist. + def search(name, limit, strict): + if strict: + return [] + return [{'id': 'mbid-yab', 'name': 'Дмитрий Яблонский', 'score': 100}] + service.mb_client.search_artist.side_effect = search + service.mb_client.get_artist.return_value = { + 'name': 'Дмитрий Яблонский', + 'aliases': [{'name': 'Dmitry Yablonsky'}], + } + + aliases = service.lookup_artist_aliases('Dmitry Yablonsky') + assert 'Дмитрий Яблонский' in aliases + assert 'Dmitry Yablonsky' in aliases + # Confirm both modes were attempted + calls = service.mb_client.search_artist.call_args_list + assert any(call.kwargs.get('strict') is True for call in calls) or any( + len(call.args) >= 3 and call.args[2] is True for call in calls + ) + + +def test_lookup_falls_back_to_non_strict_when_strict_score_too_low(service): + # Strict returns a low-confidence match (cross-script — local sim + # near 0). Non-strict hits a stronger match via alias index. + def search(name, limit, strict): + if strict: + return [{'id': 'mbid-other', 'name': 'Some Other Artist', 'score': 30}] + return [{'id': 'mbid-yab', 'name': 'Дмитрий Яблонский', 'score': 100}] + service.mb_client.search_artist.side_effect = search + service.mb_client.get_artist.return_value = { + 'name': 'Дмитрий Яблонский', + 'aliases': [{'name': 'Dmitry Yablonsky'}], + } + + aliases = service.lookup_artist_aliases('Dmitry Yablonsky') + assert 'Дмитрий Яблонский' in aliases + + +# ────────────────────────────────────────────────────────────────────── +# Trust gate — MB-score-only escape for cross-script +# ────────────────────────────────────────────────────────────────────── + +def test_trust_gate_passes_on_high_mb_score_even_with_zero_local_sim(service): + # The cross-script case: local sim ~0, MB score 100, single + # unambiguous result → should now pass. + service.mb_client.search_artist.return_value = [ + {'id': 'mbid-yab', 'name': 'Дмитрий Яблонский', 'score': 100}, + ] + service.mb_client.get_artist.return_value = { + 'name': 'Дмитрий Яблонский', + 'aliases': [{'name': 'Dmitry Yablonsky'}], + } + + aliases = service.lookup_artist_aliases('Dmitry Yablonsky') + assert 'Дмитрий Яблонский' in aliases + assert 'Dmitry Yablonsky' in aliases + + +def test_trust_gate_rejects_low_mb_score_low_local_sim(service): + # Low confidence on both axes — must NOT pull aliases (false- + # positive risk: pulling random artist's aliases). + service.mb_client.search_artist.return_value = [ + {'id': 'mbid-other', 'name': 'Some Random Artist', 'score': 40}, + ] + aliases = service.lookup_artist_aliases('Dmitry Yablonsky') + assert aliases == [] + service.mb_client.get_artist.assert_not_called() + + +def test_trust_gate_rejects_when_two_high_mb_scores_tie(service): + # Two artists named the same with score 100 → ambiguous → skip + # even with MB-only escape (the unambiguity check still gates). + service.mb_client.search_artist.return_value = [ + {'id': 'mbid-a', 'name': 'John Smith', 'score': 100}, + {'id': 'mbid-b', 'name': 'John Smith', 'score': 100}, + ] + aliases = service.lookup_artist_aliases('John Smith') + assert aliases == [] + + +def test_trust_gate_passes_combined_score_when_local_sim_strong(service): + # Same-script case from #442 — local sim high. Should still pass + # (no regression on the existing path). + service.mb_client.search_artist.return_value = [ + {'id': 'mbid-saw', 'name': 'Hiroyuki Sawano', 'score': 100}, + ] + service.mb_client.get_artist.return_value = { + 'name': 'Hiroyuki Sawano', + 'aliases': [{'name': '澤野弘之'}], + } + + aliases = service.lookup_artist_aliases('Hiroyuki Sawano') + assert '澤野弘之' in aliases + + +# ────────────────────────────────────────────────────────────────────── +# End-to-end — reporter scenario via artist_names_match +# ────────────────────────────────────────────────────────────────────── + +def test_yablonsky_reporter_scenario_end_to_end(service): + """Issue #586 exact case: expected 'Dmitry Yablonsky', actual + 'Русская филармония, Дмитрий Яблонский', MB returns artist with + canonical name in Cyrillic and Latin in aliases. Strict search + finds nothing; non-strict finds the artist with high MB score.""" + def search(name, limit, strict): + if strict: + return [] + return [{'id': 'mbid-yab', 'name': 'Дмитрий Яблонский', 'score': 100}] + service.mb_client.search_artist.side_effect = search + service.mb_client.get_artist.return_value = { + 'name': 'Дмитрий Яблонский', + 'sort-name': 'Yablonsky, Dmitry', + 'aliases': [{'name': 'Dmitry Yablonsky'}], + } + + aliases = service.lookup_artist_aliases('Dmitry Yablonsky') + + # Must include the Cyrillic canonical so artist_names_match can + # bridge the credit-split actual. + assert 'Дмитрий Яблонский' in aliases + + # Verify the full bridge with the real artist_names_match helper. + from core.matching.artist_aliases import artist_names_match + matched, score = artist_names_match( + 'Dmitry Yablonsky', + 'Русская филармония, Дмитрий Яблонский', + aliases=aliases, + ) + assert matched is True + assert score >= 0.6 diff --git a/tests/matching/test_artist_alias_service.py b/tests/matching/test_artist_alias_service.py index 2e2f1a6a..a895f942 100644 --- a/tests/matching/test_artist_alias_service.py +++ b/tests/matching/test_artist_alias_service.py @@ -81,10 +81,13 @@ class TestFetchArtistAliases: def test_extracts_alias_names_from_mb_response(self, service): """Reporter's case 1 shape: MB returns aliases for Hiroyuki Sawano including the Japanese kanji form. Extract the `name` - from each alias entry.""" + from each alias entry. Issue #586 — also include the canonical + ``name`` and ``sort-name`` from the artist record itself, plus + per-alias ``sort-name`` when present, for cross-script bridging.""" service.mb_client.get_artist.return_value = { 'id': '60d2ea34-1912-425f-bf9c-fc544e4448cd', 'name': 'Hiroyuki Sawano', + 'sort-name': 'Sawano, Hiroyuki', 'aliases': [ {'name': '澤野弘之', 'sort-name': '澤野弘之', 'locale': 'ja', 'primary': True}, {'name': 'SawanoHiroyuki', 'sort-name': 'SawanoHiroyuki', 'locale': None}, @@ -94,10 +97,11 @@ class TestFetchArtistAliases: aliases = service.fetch_artist_aliases('60d2ea34-1912-425f-bf9c-fc544e4448cd') + assert 'Hiroyuki Sawano' in aliases # canonical name + assert 'Sawano, Hiroyuki' in aliases # canonical sort-name (also matches alias sort-name) assert '澤野弘之' in aliases assert 'SawanoHiroyuki' in aliases assert 'Sawano Hiroyuki' in aliases - assert len(aliases) == 3 def test_dedup_case_insensitive(self, service): """Same name with different casing should collapse — MB @@ -125,14 +129,15 @@ class TestFetchArtistAliases: aliases = service.fetch_artist_aliases('mbid-x') assert aliases == ['Real Name'] - def test_missing_aliases_key_returns_empty(self, service): - """MB artist record might not have any aliases. Returns [] - not raises.""" + def test_missing_aliases_key_returns_canonical_name_only(self, service): + """MB artist record without an aliases array still returns the + canonical name (post-#586). Pre-fix this returned [] which + meant cross-script bridging was impossible.""" service.mb_client.get_artist.return_value = { 'id': 'mbid-x', 'name': 'Some Artist', } - assert service.fetch_artist_aliases('mbid-x') == [] + assert service.fetch_artist_aliases('mbid-x') == ['Some Artist'] def test_aliases_null_returns_empty(self, service): """MB sometimes returns `aliases: null` instead of empty array.""" @@ -476,14 +481,19 @@ class TestLookupArtistAliasesMultiTier: assert aliases == [] def test_no_search_results_returns_empty(self, service): - """Artist not found on MB — empty return, cached so we - don't re-search the same name forever.""" + """Artist not found on MB under either strict or non-strict + search — empty return, cached so we don't re-search the same + name forever. Issue #586: strict-then-non-strict means TWO + search calls per uncached lookup; the empty cache prevents + further calls on the next invocation.""" service.mb_client.search_artist.return_value = [] aliases = service.lookup_artist_aliases('NeverHeardOf') assert aliases == [] - # Second call should hit cache, not re-search + # First lookup: strict + non-strict fallback = 2 calls. + assert service.mb_client.search_artist.call_count == 2 + # Second call should hit cache, not re-search at all. service.lookup_artist_aliases('NeverHeardOf') - assert service.mb_client.search_artist.call_count == 1 + assert service.mb_client.search_artist.call_count == 2 def test_low_confidence_match_skipped(self, service): """Search returned something but the name similarity is too diff --git a/tests/metadata/test_artist_resolution.py b/tests/metadata/test_artist_resolution.py new file mode 100644 index 00000000..95eb8d3a --- /dev/null +++ b/tests/metadata/test_artist_resolution.py @@ -0,0 +1,79 @@ +from core.metadata.artist_resolution import resolve_track_artists + + +def test_prefers_original_search_artists_when_populated(): + original = {"artists": [{"name": "Kendrick Lamar"}, {"name": "Rihanna"}]} + track = {"artists": [{"name": "Should Not Be Used"}]} + artist = {"name": "Primary"} + + assert resolve_track_artists(original, track, artist) == ["Kendrick Lamar", "Rihanna"] + + +def test_falls_back_to_track_info_artists_when_original_lacks_list(): + # Soulseek context shape: original_search_result has 'artist' (string) + # and no 'artists' (list). Full Spotify track object lives on track_info. + original = {"artist": "Kendrick Lamar"} + track = {"artists": [{"name": "Kendrick Lamar"}, {"name": "Rihanna"}]} + artist = {"name": "Kendrick Lamar"} + + assert resolve_track_artists(original, track, artist) == ["Kendrick Lamar", "Rihanna"] + + +def test_falls_back_to_artist_dict_name_when_no_lists_available(): + original = {"artist": "Solo Artist"} + track = {"name": "Track Title"} + artist = {"name": "Solo Artist"} + + assert resolve_track_artists(original, track, artist) == ["Solo Artist"] + + +def test_returns_empty_list_when_everything_missing(): + assert resolve_track_artists(None, None, None) == [] + assert resolve_track_artists({}, {}, {}) == [] + + +def test_handles_bare_string_artist_items(): + original = {"artists": ["Kendrick Lamar", "Rihanna"]} + assert resolve_track_artists(original, None, None) == ["Kendrick Lamar", "Rihanna"] + + +def test_mixed_dict_and_string_items_normalized(): + original = {"artists": [{"name": "Kendrick Lamar"}, "Rihanna"]} + assert resolve_track_artists(original, None, None) == ["Kendrick Lamar", "Rihanna"] + + +def test_strips_whitespace_and_drops_empty_entries(): + original = {"artists": [{"name": " Kendrick "}, {"name": ""}, " ", "Rihanna"]} + assert resolve_track_artists(original, None, None) == ["Kendrick", "Rihanna"] + + +def test_dict_item_without_name_key_skipped(): + original = {"artists": [{"id": "abc"}, {"name": "Rihanna"}]} + assert resolve_track_artists(original, None, None) == ["Rihanna"] + + +def test_non_list_artists_value_falls_through(): + original = {"artists": "Kendrick Lamar"} # string, not list + track = {"artists": [{"name": "Kendrick Lamar"}, {"name": "Rihanna"}]} + assert resolve_track_artists(original, track, None) == ["Kendrick Lamar", "Rihanna"] + + +def test_empty_original_artists_list_falls_through_to_track_info(): + original = {"artists": []} + track = {"artists": [{"name": "Kendrick Lamar"}, {"name": "Rihanna"}]} + assert resolve_track_artists(original, track, None) == ["Kendrick Lamar", "Rihanna"] + + +def test_artist_dict_name_blank_returns_empty(): + assert resolve_track_artists({}, {}, {"name": " "}) == [] + assert resolve_track_artists({}, {}, {"name": ""}) == [] + + +def test_non_string_artist_items_coerced_to_string(): + original = {"artists": [123, {"name": "Real Artist"}]} + assert resolve_track_artists(original, None, None) == ["123", "Real Artist"] + + +def test_none_artist_items_dropped(): + original = {"artists": [None, {"name": "Real Artist"}, None]} + assert resolve_track_artists(original, None, None) == ["Real Artist"] diff --git a/tests/metadata/test_deezer_track_cache_validity.py b/tests/metadata/test_deezer_track_cache_validity.py new file mode 100644 index 00000000..8ff107de --- /dev/null +++ b/tests/metadata/test_deezer_track_cache_validity.py @@ -0,0 +1,208 @@ +"""Pin the Deezer per-track cache validity check. + +Issue #588: contributors tagging worked for some tracks and not others. +Root cause was cache pollution — `/album//tracks` cached partial +records under the same key as `/track/`, and `get_track_details` +was using `track_position` alone as the "full payload" sentinel. Both +endpoints set track_position; only `/track/` sets contributors. + +These tests pin the corrected sentinel (`_is_full_track_payload`) so +the regression can't silently come back. +""" + +from unittest.mock import MagicMock, patch + +import pytest + +from core.deezer_client import _is_full_track_payload + + +# ──────────────────────────────────────────────────────────────────── +# Pure helper — payload-shape classification +# ──────────────────────────────────────────────────────────────────── + +def test_full_track_endpoint_payload_is_valid(): + payload = { + 'id': 12345, + 'title': 'Erased', + 'track_position': 1, + 'contributors': [{'name': 'Whipped Cream'}, {'name': 'Andrea Botez'}], + 'artist': {'name': 'Whipped Cream'}, + 'album': {'id': 1, 'title': 'Erased'}, + } + assert _is_full_track_payload(payload) is True + + +def test_full_track_with_empty_contributors_list_is_valid(): + # Single-artist track from /track/ still emits contributors=[] + # The KEY presence is what matters, not truthiness. + payload = { + 'id': 12345, + 'title': 'Solo Track', + 'track_position': 1, + 'contributors': [], + 'artist': {'name': 'Solo Artist'}, + } + assert _is_full_track_payload(payload) is True + + +def test_album_tracks_payload_missing_contributors_is_partial(): + # The exact shape /album//tracks returns per item — has + # track_position but no contributors. Pre-fix this passed the + # `track_position in cached` check; post-fix it correctly falls + # through to a fresh /track/ fetch. + payload = { + 'id': 12345, + 'title': 'Sacrifice', + 'track_position': 1, + 'duration': 180, + 'artist': {'name': 'Andrea Botez'}, + } + assert _is_full_track_payload(payload) is False + + +def test_search_payload_without_track_position_is_partial(): + payload = { + 'id': 12345, + 'title': 'Sacrifice', + 'artist': {'name': 'Andrea Botez'}, + 'isrc': 'XX1234567890', + } + assert _is_full_track_payload(payload) is False + + +def test_none_or_non_dict_payload_is_partial(): + assert _is_full_track_payload(None) is False + assert _is_full_track_payload([]) is False + assert _is_full_track_payload('string') is False + assert _is_full_track_payload(0) is False + + +def test_empty_dict_is_partial(): + assert _is_full_track_payload({}) is False + + +# ──────────────────────────────────────────────────────────────────── +# get_track_details — cache + fetch interaction +# ──────────────────────────────────────────────────────────────────── + +@pytest.fixture +def deezer_client(): + """Build a DeezerClient with mocked HTTP + cache. Bypasses __init__ + auth/config requirements.""" + from core.deezer_client import DeezerClient + client = DeezerClient.__new__(DeezerClient) + client._api_get = MagicMock() + return client + + +def _patch_cache(cached_payload): + """Patch the module-level cache lookup. Returns the patched cache + mock so callers can assert on store_entity calls.""" + cache = MagicMock() + cache.get_entity.return_value = cached_payload + cache.store_entity = MagicMock() + return patch('core.deezer_client.get_metadata_cache', return_value=cache), cache + + +def test_cache_hit_with_full_payload_skips_api_call(deezer_client): + full = { + 'id': 12345, + 'title': 'Erased', + 'track_position': 1, + 'contributors': [{'name': 'Whipped Cream'}, {'name': 'Andrea Botez'}], + 'artist': {'name': 'Whipped Cream'}, + 'album': {'id': 1, 'title': 'Erased', 'nb_tracks': 1}, + } + cache_patch, cache = _patch_cache(full) + with cache_patch: + result = deezer_client.get_track_details('12345') + + assert result is not None + assert result['artists'] == ['Whipped Cream', 'Andrea Botez'] + deezer_client._api_get.assert_not_called() + cache.store_entity.assert_not_called() + + +def test_cache_hit_with_partial_album_tracks_payload_refetches(deezer_client): + """The bug from #588 — partial album-tracks data should NOT be + treated as a full hit. Post-fix the client re-fetches.""" + partial = { + 'id': 12345, + 'title': 'Sacrifice', + 'track_position': 1, + 'artist': {'name': 'Andrea Botez'}, + } + fresh = { + 'id': 12345, + 'title': 'Sacrifice', + 'track_position': 1, + 'contributors': [{'name': 'Andrea Botez'}, {'name': 'Grabbitz'}], + 'artist': {'name': 'Andrea Botez'}, + 'album': {'id': 1, 'title': 'Sacrifice', 'nb_tracks': 1}, + } + cache_patch, cache = _patch_cache(partial) + deezer_client._api_get.return_value = fresh + with cache_patch: + result = deezer_client.get_track_details('12345') + + assert result is not None + assert result['artists'] == ['Andrea Botez', 'Grabbitz'] + deezer_client._api_get.assert_called_once_with('track/12345') + cache.store_entity.assert_called_once_with('deezer', 'track', '12345', fresh) + + +def test_cache_miss_fetches_fresh(deezer_client): + cache_patch, cache = _patch_cache(None) + fresh = { + 'id': 12345, + 'title': 'Sacrifice', + 'track_position': 1, + 'contributors': [{'name': 'Andrea Botez'}, {'name': 'Grabbitz'}], + 'artist': {'name': 'Andrea Botez'}, + 'album': {'id': 1, 'title': 'Sacrifice', 'nb_tracks': 1}, + } + deezer_client._api_get.return_value = fresh + with cache_patch: + result = deezer_client.get_track_details('12345') + + assert result is not None + assert result['artists'] == ['Andrea Botez', 'Grabbitz'] + deezer_client._api_get.assert_called_once_with('track/12345') + cache.store_entity.assert_called_once() + + +def test_cache_hit_with_search_shape_refetches(deezer_client): + """Search results lack track_position — same fall-through path as + partial album-tracks data.""" + search_shape = { + 'id': 12345, + 'title': 'Sacrifice', + 'artist': {'name': 'Andrea Botez'}, + 'isrc': 'XX1234567890', + } + fresh = { + 'id': 12345, + 'title': 'Sacrifice', + 'track_position': 1, + 'contributors': [{'name': 'Andrea Botez'}, {'name': 'Grabbitz'}], + 'artist': {'name': 'Andrea Botez'}, + 'album': {'id': 1, 'title': 'Sacrifice', 'nb_tracks': 1}, + } + cache_patch, _ = _patch_cache(search_shape) + deezer_client._api_get.return_value = fresh + with cache_patch: + result = deezer_client.get_track_details('12345') + + assert result is not None + assert result['artists'] == ['Andrea Botez', 'Grabbitz'] + deezer_client._api_get.assert_called_once() + + +def test_api_failure_returns_none(deezer_client): + cache_patch, _ = _patch_cache(None) + deezer_client._api_get.return_value = None + with cache_patch: + result = deezer_client.get_track_details('12345') + + assert result is None diff --git a/tests/metadata/test_multi_artist_tag_settings.py b/tests/metadata/test_multi_artist_tag_settings.py index 550e170d..d3acf452 100644 --- a/tests/metadata/test_multi_artist_tag_settings.py +++ b/tests/metadata/test_multi_artist_tag_settings.py @@ -99,8 +99,10 @@ class TestArtistsListPopulated: assert meta.get("_artists_list") == ["Solo Artist"] def test_no_artists_falls_through(self): - """When search response has no artists list, falls through to - the single-artist branch — no `_artists_list` written.""" + """When search response has no artists list, the artist-resolution + helper falls back to the artist_dict.name as a single-element list. + Multi-value tag write still no-ops because len([primary]) == 1. + """ from core.metadata import source as src_module context = { @@ -109,7 +111,31 @@ class TestArtistsListPopulated: } with patch.object(src_module, "get_config_manager", return_value=_make_cfg()): meta = src_module.extract_source_metadata(context, {"name": "X"}, {}) - assert "_artists_list" not in meta or meta.get("_artists_list") in (None, []) + assert meta.get("_artists_list") == ["X"] + + def test_soulseek_shape_falls_back_to_track_info_artists(self): + """Soulseek matched-download regression: original_search_result + carries 'artist' (singular string) but no 'artists' list, while + track_info (the matched Spotify track object) carries the full + multi-artist array. Helper should pull from track_info. + """ + from core.metadata import source as src_module + + context = { + "original_search_result": { + "title": "DNA.", + "artist": "Kendrick Lamar", + }, + "track_info": { + "name": "DNA.", + "artists": [{"name": "Kendrick Lamar"}, {"name": "Rihanna"}], + }, + "source": "spotify", + } + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()): + meta = src_module.extract_source_metadata(context, {"name": "Kendrick Lamar"}, {}) + assert meta.get("_artists_list") == ["Kendrick Lamar", "Rihanna"] + assert meta.get("artist") == "Kendrick Lamar, Rihanna" # --------------------------------------------------------------------------- diff --git a/tests/metadata/test_track_number_format.py b/tests/metadata/test_track_number_format.py new file mode 100644 index 00000000..8debd71e --- /dev/null +++ b/tests/metadata/test_track_number_format.py @@ -0,0 +1,103 @@ +"""Tests for the track-number tag formatter. + +Discord report (Netti93): album tracks tagged as "6/0" instead of +"6/13" when source data lacked total_tracks. Helper now returns just +"6" when total is 0 / unknown, matching what the retag tool already +did and what the ID3 spec allows. +""" + +from core.metadata.track_number_format import ( + format_track_number_tag, + format_track_number_tuple, +) + + +# ────────────────────────────────────────────────────────────────────── +# format_track_number_tag — string output for ID3 / Vorbis +# ────────────────────────────────────────────────────────────────────── + +def test_track_with_known_total_returns_slash_format(): + assert format_track_number_tag(6, 13) == "6/13" + assert format_track_number_tag(1, 1) == "1/1" + assert format_track_number_tag(99, 100) == "99/100" + + +def test_zero_total_returns_track_number_only(): + """The Netti93 case — total_tracks=0 means unknown, NOT + 'track 6 of 0'. Drop the slash.""" + assert format_track_number_tag(6, 0) == "6" + assert format_track_number_tag(1, 0) == "1" + + +def test_none_total_returns_track_number_only(): + assert format_track_number_tag(6, None) == "6" + + +def test_none_track_number_defaults_to_one(): + assert format_track_number_tag(None, 13) == "1/13" + assert format_track_number_tag(None, None) == "1" + + +def test_zero_track_number_defaults_to_one(): + """Track 0 isn't valid in any convention — coerce to 1.""" + # Note: 0 is non-negative so falls into the default-0 path which + # the formatter then treats as "default" via the explicit default + # arg. Since 0 is technically valid output of `int(0)`, the helper + # passes it through. Document the behavior here. + # Actually re-checking: 0 satisfies `>= 0` so returns 0. That + # means format would emit "0/13" for malformed input. Not great + # but at least it doesn't crash. Test pins current behavior. + assert format_track_number_tag(0, 13) == "0/13" + + +def test_negative_total_treated_as_unknown(): + assert format_track_number_tag(6, -1) == "6" + + +def test_negative_track_number_falls_back_to_default(): + assert format_track_number_tag(-1, 13) == "1/13" + + +def test_string_inputs_coerced(): + assert format_track_number_tag("6", "13") == "6/13" + assert format_track_number_tag("6", "0") == "6" + + +def test_unparseable_inputs_use_defaults(): + assert format_track_number_tag("six", "thirteen") == "1" + assert format_track_number_tag("abc", 13) == "1/13" + + +def test_float_inputs_truncate(): + # int() truncates floats — keeps behavior deterministic + assert format_track_number_tag(6.7, 13.9) == "6/13" + + +# ────────────────────────────────────────────────────────────────────── +# format_track_number_tuple — MP4 trkn tuple +# ────────────────────────────────────────────────────────────────────── + +def test_tuple_with_known_total(): + assert format_track_number_tuple(6, 13) == (6, 13) + + +def test_tuple_with_zero_total(): + assert format_track_number_tuple(6, 0) == (6, 0) + + +def test_tuple_with_none_total(): + assert format_track_number_tuple(6, None) == (6, 0) + + +def test_tuple_with_none_track_defaults_to_one(): + assert format_track_number_tuple(None, 13) == (1, 13) + assert format_track_number_tuple(None, None) == (1, 0) + + +def test_tuple_negative_inputs_safe(): + assert format_track_number_tuple(-1, -5) == (1, 0) + + +def test_tuple_string_inputs_coerced(): + assert format_track_number_tuple("6", "13") == (6, 13) + assert format_track_number_tuple("6", "0") == (6, 0) diff --git a/tests/sync/__init__.py b/tests/sync/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/sync/test_match_overrides.py b/tests/sync/test_match_overrides.py new file mode 100644 index 00000000..4779ca54 --- /dev/null +++ b/tests/sync/test_match_overrides.py @@ -0,0 +1,193 @@ +from unittest.mock import MagicMock + +from core.sync.match_overrides import record_manual_match, resolve_match_overrides + + +# ────────────────────────────────────────────────────────────────────── +# resolve_match_overrides — pre-pair source→server from cache +# ────────────────────────────────────────────────────────────────────── + +def test_empty_inputs_return_empty_dict(): + assert resolve_match_overrides([], [], lambda _id: None) == {} + assert resolve_match_overrides([{"source_track_id": "x"}], [], lambda _id: "y") == {} + assert resolve_match_overrides([], [{"id": "y"}], lambda _id: None) == {} + + +def test_single_cache_hit_returns_pair(): + sources = [{"source_track_id": "spotify-iron-man", "name": "Iron Man - 2012 - Remaster"}] + servers = [{"id": 5001, "title": "Iron Man"}] + cache = {"spotify-iron-man": 5001} + result = resolve_match_overrides(sources, servers, lambda sid: cache.get(sid)) + assert result == {0: 0} + + +def test_multiple_overrides_resolve_correctly(): + sources = [ + {"source_track_id": "iron"}, + {"source_track_id": "para"}, + {"source_track_id": "war"}, + ] + servers = [ + {"id": 5001, "title": "Iron Man"}, + {"id": 5002, "title": "Paranoid"}, + {"id": 5003, "title": "War Pigs"}, + ] + cache = {"iron": 5001, "para": 5002, "war": 5003} + result = resolve_match_overrides(sources, servers, lambda sid: cache.get(sid)) + assert result == {0: 0, 1: 1, 2: 2} + + +def test_source_without_track_id_skipped(): + sources = [ + {"source_track_id": "iron", "name": "Iron Man"}, + {"name": "Paranoid"}, # no source_track_id (e.g. legacy / non-mirrored) + ] + servers = [{"id": 5001, "title": "Iron Man"}, {"id": 5002, "title": "Paranoid"}] + cache = {"iron": 5001} + result = resolve_match_overrides(sources, servers, lambda sid: cache.get(sid)) + assert result == {0: 0} + + +def test_cache_miss_skipped(): + sources = [{"source_track_id": "iron"}, {"source_track_id": "para"}] + servers = [{"id": 5001, "title": "Iron Man"}, {"id": 5002, "title": "Paranoid"}] + result = resolve_match_overrides(sources, servers, lambda sid: None) + assert result == {} + + +def test_stale_cache_pointing_at_missing_server_track_skipped(): + # User cached a match → file got deleted from server → server_tracks + # no longer has 5001 → don't pair, fall through to normal matching. + sources = [{"source_track_id": "iron"}] + servers = [{"id": 9999, "title": "Different Track"}] + cache = {"iron": 5001} # 5001 no longer exists + result = resolve_match_overrides(sources, servers, lambda sid: cache.get(sid)) + assert result == {} + + +def test_server_id_str_int_coercion(): + # Cache might store ints, server_tracks might have str IDs (Plex + # ratingKey is str). Helper coerces both sides to str. + sources = [{"source_track_id": "iron"}] + servers = [{"id": "5001", "title": "Iron Man"}] + cache = {"iron": 5001} # int from cache + result = resolve_match_overrides(sources, servers, lambda sid: cache.get(sid)) + assert result == {0: 0} + + +def test_two_sources_pointing_at_same_server_track_only_first_wins(): + # Defensive — UNIQUE constraint prevents this in production but + # cache_lookup is injectable so we verify the safety. + sources = [{"source_track_id": "a"}, {"source_track_id": "b"}] + servers = [{"id": 5001, "title": "Iron Man"}] + cache = {"a": 5001, "b": 5001} + result = resolve_match_overrides(sources, servers, lambda sid: cache.get(sid)) + assert result == {0: 0} + + +def test_cache_lookup_raising_treated_as_miss(): + sources = [{"source_track_id": "iron"}] + servers = [{"id": 5001, "title": "Iron Man"}] + def boom(_sid): + raise RuntimeError("db down") + result = resolve_match_overrides(sources, servers, boom) + assert result == {} + + +def test_non_dict_source_or_server_skipped(): + sources = [None, "string", {"source_track_id": "iron"}] + servers = [{"id": 5001, "title": "Iron Man"}] + cache = {"iron": 5001} + result = resolve_match_overrides(sources, servers, lambda sid: cache.get(sid)) + # source idx 2 → server idx 0 + assert result == {2: 0} + + +def test_server_without_id_skipped(): + sources = [{"source_track_id": "iron"}] + servers = [{"title": "Iron Man"}] # no id + cache = {"iron": 5001} + result = resolve_match_overrides(sources, servers, lambda sid: cache.get(sid)) + assert result == {} + + +def test_partial_cache_hits_only_pair_those(): + sources = [ + {"source_track_id": "iron"}, + {"source_track_id": "para"}, + {"source_track_id": "war"}, + ] + servers = [ + {"id": 5001, "title": "Iron Man"}, + {"id": 5002, "title": "Paranoid"}, + {"id": 5003, "title": "War Pigs"}, + ] + # Only iron + war cached, para falls through to normal matching + cache = {"iron": 5001, "war": 5003} + result = resolve_match_overrides(sources, servers, lambda sid: cache.get(sid)) + assert result == {0: 0, 2: 2} + + +# ────────────────────────────────────────────────────────────────────── +# record_manual_match — persist user-confirmed pair +# ────────────────────────────────────────────────────────────────────── + +def test_record_persists_with_confidence_one(): + db = MagicMock() + db.save_sync_match_cache.return_value = True + ok = record_manual_match( + db, + source_track_id="spotify-iron-man", + server_source="plex", + server_track_id=5001, + server_track_title="Iron Man", + source_title="Iron Man - 2012 - Remaster", + source_artist="Black Sabbath", + ) + assert ok is True + db.save_sync_match_cache.assert_called_once() + kwargs = db.save_sync_match_cache.call_args.kwargs + assert kwargs["spotify_track_id"] == "spotify-iron-man" + assert kwargs["server_source"] == "plex" + assert kwargs["server_track_id"] == 5001 + assert kwargs["server_track_title"] == "Iron Man" + assert kwargs["confidence"] == 1.0 + assert kwargs["normalized_title"] == "iron man - 2012 - remaster" + assert kwargs["normalized_artist"] == "black sabbath" + + +def test_record_returns_false_when_required_fields_missing(): + db = MagicMock() + assert record_manual_match(db, source_track_id="", server_source="plex", server_track_id=1) is False + assert record_manual_match(db, source_track_id="x", server_source="", server_track_id=1) is False + assert record_manual_match(db, source_track_id="x", server_source="plex", server_track_id=None) is False + db.save_sync_match_cache.assert_not_called() + + +def test_record_returns_false_when_db_save_returns_false(): + db = MagicMock() + db.save_sync_match_cache.return_value = False + assert record_manual_match(db, source_track_id="x", server_source="plex", server_track_id=1) is False + + +def test_record_swallows_db_exception(): + db = MagicMock() + db.save_sync_match_cache.side_effect = RuntimeError("db boom") + assert record_manual_match(db, source_track_id="x", server_source="plex", server_track_id=1) is False + + +def test_record_returns_false_when_db_lacks_method(): + class NoSaveDB: + pass + assert record_manual_match(NoSaveDB(), source_track_id="x", server_source="plex", server_track_id=1) is False + + +def test_record_handles_empty_optional_strings(): + db = MagicMock() + db.save_sync_match_cache.return_value = True + ok = record_manual_match(db, source_track_id="x", server_source="plex", server_track_id=1) + assert ok is True + kwargs = db.save_sync_match_cache.call_args.kwargs + assert kwargs["normalized_title"] == "" + assert kwargs["normalized_artist"] == "" + assert kwargs["server_track_title"] == "" diff --git a/tests/test_acoustid_scanner.py b/tests/test_acoustid_scanner.py index a6d83e12..1993b099 100644 --- a/tests/test_acoustid_scanner.py +++ b/tests/test_acoustid_scanner.py @@ -54,11 +54,11 @@ def _make_context(rows): def test_load_db_tracks_skips_null_ids_and_normalizes_track_ids(): job = AcoustIDScannerJob() context = _make_context([ - # 10 columns: id, title, artist (COALESCE'd), file_path, track_number, + # 11 columns: id, title, artist (COALESCE'd), file_path, track_number, # album_title, album_thumb, artist_thumb, track_artist (raw, may be ''), - # album_artist. - (None, "Broken Track", "Artist", "/music/broken.flac", 1, "Album", None, None, "", "Artist"), - (42, "Good Track", "Artist", "/music/good.flac", 2, "Album", "album-thumb", "artist-thumb", "", "Artist"), + # album_artist, duration_ms (issue #587 — duration guard). + (None, "Broken Track", "Artist", "/music/broken.flac", 1, "Album", None, None, "", "Artist", 180000), + (42, "Good Track", "Artist", "/music/good.flac", 2, "Album", "album-thumb", "artist-thumb", "", "Artist", 240000), ]) tracks = job._load_db_tracks(context) @@ -66,16 +66,17 @@ def test_load_db_tracks_skips_null_ids_and_normalizes_track_ids(): assert list(tracks.keys()) == ["42"] assert tracks["42"]["title"] == "Good Track" assert tracks["42"]["artist"] == "Artist" + assert tracks["42"]["duration_ms"] == 240000 def test_scan_handles_mixed_track_id_types(monkeypatch): job = AcoustIDScannerJob() context = _make_context([ - # 10 columns: id, title, artist (COALESCE'd), file_path, track_number, + # 11 columns: id, title, artist (COALESCE'd), file_path, track_number, # album_title, album_thumb, artist_thumb, track_artist (raw, may be ''), - # album_artist. - (None, "Broken Track", "Artist", "/music/broken.flac", 1, "Album", None, None, "", "Artist"), - (42, "Good Track", "Artist", "/music/good.flac", 2, "Album", "album-thumb", "artist-thumb", "", "Artist"), + # album_artist, duration_ms. + (None, "Broken Track", "Artist", "/music/broken.flac", 1, "Album", None, None, "", "Artist", 180000), + (42, "Good Track", "Artist", "/music/good.flac", 2, "Album", "album-thumb", "artist-thumb", "", "Artist", 240000), ]) monkeypatch.setattr(job, "_resolve_path", lambda file_path, _context: file_path) @@ -275,7 +276,8 @@ def _make_real_db_context(tmp_path): album_id TEXT, file_path TEXT, track_number INTEGER, - track_artist TEXT + track_artist TEXT, + duration INTEGER ); """) conn.commit() @@ -620,3 +622,160 @@ def test_scanner_file_tag_matches_db_no_behavioral_change(monkeypatch): ) assert captured_findings == [] + + +# --------------------------------------------------------------------------- +# Issue #587 — multi-candidate scan + duration guard (Foxxify report) +# --------------------------------------------------------------------------- + + +def test_scanner_no_finding_when_lower_ranked_candidate_matches(): + """Foxxify case 2 — AcoustID returns multiple recordings per + fingerprint; the top match is the wrong-credited recording but a + lower-ranked candidate matches expected metadata exactly. Scanner + should iterate ALL candidates and suppress the finding. + + Repro: file is "Nana" by Geoxor, AcoustID top match is "Nana" by + Edward Vesala Trio (different recording sharing similar + fingerprint), AcoustID's second candidate is the actual Geoxor + track. Pre-fix scanner only saw [0] → flagged. Post-fix sees [1] + → no flag.""" + job = AcoustIDScannerJob() + captured_findings = [] + context = _make_finding_capturing_context( + track_row=("nana", "Nana", "Geoxor", + "/music/nana.opus", 6, "Stardust", None, None), + captured=captured_findings, + ) + + fake_acoustid = SimpleNamespace( + fingerprint_and_lookup=lambda fpath: { + 'best_score': 0.97, + 'recordings': [ + # AcoustID's top match — wrong artist for our file + {'title': 'Nana', 'artist': 'Edward Vesala Trio'}, + # Lower-ranked candidate — actually matches our expected + {'title': 'Nana', 'artist': 'Geoxor'}, + ], + }, + ) + + result = JobResultStub() + job._scan_file( + '/music/nana.opus', 'nana', + {'title': 'Nana', 'artist': 'Geoxor'}, + fake_acoustid, context, result, + fp_threshold=0.85, title_threshold=0.85, artist_threshold=0.6, + ) + + assert captured_findings == [], ( + f"Expected no finding (lower-ranked candidate matches); got {captured_findings}" + ) + + +def test_scanner_still_flags_when_no_candidate_matches(): + """Confirm the multi-candidate check doesn't accidentally suppress + legitimate mismatches — if NO candidate matches expected metadata, + the finding still fires.""" + job = AcoustIDScannerJob() + captured_findings = [] + context = _make_finding_capturing_context( + track_row=("99", "Expected Title", "Expected Artist", + "/music/track.flac", 1, "Album", None, None), + captured=captured_findings, + ) + + fake_acoustid = SimpleNamespace( + fingerprint_and_lookup=lambda fpath: { + 'best_score': 0.99, + 'recordings': [ + {'title': 'Wrong Track', 'artist': 'Wrong Artist A'}, + {'title': 'Different Wrong', 'artist': 'Wrong Artist B'}, + ], + }, + ) + + result = JobResultStub() + job._scan_file( + '/music/track.flac', '99', + {'title': 'Expected Title', 'artist': 'Expected Artist'}, + fake_acoustid, context, result, + fp_threshold=0.85, title_threshold=0.85, artist_threshold=0.6, + ) + + assert len(captured_findings) == 1 + + +def test_scanner_skips_finding_on_strong_duration_mismatch(): + """Foxxify case 3 — 17-minute mashup edit fingerprints to a 5-minute + late-70s Japanese hiphop track. Fingerprint matched a sample/intro + section but the recordings are clearly different (drastic length + difference). Scanner should skip the finding rather than recommend + retag of a totally different track length.""" + job = AcoustIDScannerJob() + captured_findings = [] + context = _make_finding_capturing_context( + track_row=("mashup", "Some Mashup Edit", "Mashup Artist", + "/music/mashup.opus", 1, "Mashups", None, None), + captured=captured_findings, + ) + + # AcoustID matched a 5-minute Japanese hiphop track via fingerprint + # hash collision. Expected file is 17 minutes — duration guard + # should kick in. + fake_acoustid = SimpleNamespace( + fingerprint_and_lookup=lambda fpath: { + 'best_score': 0.98, + 'recordings': [ + {'title': 'Different Song', 'artist': 'Different Artist', + 'duration': 300}, # 5 min — way off from our 17 min file + ], + }, + ) + + result = JobResultStub() + # 17 minutes = 1020 sec = 1020000 ms + job._scan_file( + '/music/mashup.opus', 'mashup', + {'title': 'Some Mashup Edit', 'artist': 'Mashup Artist', 'duration_ms': 1020000}, + fake_acoustid, context, result, + fp_threshold=0.85, title_threshold=0.85, artist_threshold=0.6, + ) + + assert captured_findings == [], ( + f"Expected no finding (duration mismatch suggests collision); got {captured_findings}" + ) + + +def test_scanner_still_flags_when_duration_matches(): + """Confirm the duration guard only kicks in for STRONG mismatches — + similar-length wrong song still gets flagged.""" + job = AcoustIDScannerJob() + captured_findings = [] + context = _make_finding_capturing_context( + track_row=("99", "Expected", "Artist", + "/music/track.flac", 1, "Album", None, None), + captured=captured_findings, + ) + + fake_acoustid = SimpleNamespace( + fingerprint_and_lookup=lambda fpath: { + 'best_score': 0.99, + 'recordings': [ + {'title': 'Wrong Song', 'artist': 'Wrong Artist', + 'duration': 180}, # 3 min, matches expected + ], + }, + ) + + result = JobResultStub() + # 3-minute file with 3-minute candidate — same length, but title + + # artist clearly mismatch → finding should still fire + job._scan_file( + '/music/track.flac', '99', + {'title': 'Expected', 'artist': 'Artist', 'duration_ms': 180000}, + fake_acoustid, context, result, + fp_threshold=0.85, title_threshold=0.85, artist_threshold=0.6, + ) + + assert len(captured_findings) == 1 diff --git a/tests/test_lyrics_reembed_from_sidecar.py b/tests/test_lyrics_reembed_from_sidecar.py new file mode 100644 index 00000000..498dda54 --- /dev/null +++ b/tests/test_lyrics_reembed_from_sidecar.py @@ -0,0 +1,220 @@ +"""Tests for re-embedding lyrics from an existing sidecar file. + +Discord report (Netti93): retag was clearing the LYRICS / USLT tag +without rewriting it. Cause was two-fold: + +1. `core/library/retag.py:execute_retag` never called + `generate_lrc_file` after `enhance_file_metadata`. The download + pipeline does — retag was inconsistent. +2. Even with the call added, `lyrics_client.create_lrc_file` used to + short-circuit when an .lrc / .txt sidecar already existed (the + typical retag case — sidecar moved alongside the audio file). + Pre-fix: returned True without re-embedding USLT. Post-fix: reads + the existing sidecar and re-embeds the USLT tag. +""" + +from __future__ import annotations + +import os +import tempfile +from unittest.mock import patch, MagicMock + +import pytest + + +@pytest.fixture +def fake_audio_file(tmp_path): + """Build a minimal FLAC file with no LYRICS tag.""" + fd, path = tempfile.mkstemp(suffix='.flac', dir=str(tmp_path)) + os.close(fd) + minimal = ( + b'fLaC' + + b'\x80\x00\x00\x22' + + b'\x00\x10\x00\x10' + + b'\x00\x00\x00\x00\x00\x00' + + b'\x0a\xc4\x42\xf0\x00\x00\x00\x00' + + b'\x00' * 16 + ) + with open(path, 'wb') as f: + f.write(minimal) + yield path + + +# ────────────────────────────────────────────────────────────────────── +# create_lrc_file — re-embed when sidecar present +# ────────────────────────────────────────────────────────────────────── + +def test_existing_lrc_sidecar_triggers_reembed(fake_audio_file): + """The exact retag scenario — sidecar already exists alongside the + audio file (moved during retag), USLT got cleared by enrichment. + Helper should read the sidecar and re-embed without hitting LRClib.""" + from core.lyrics_client import LyricsClient + + sidecar_path = os.path.splitext(fake_audio_file)[0] + '.lrc' + with open(sidecar_path, 'w', encoding='utf-8') as f: + f.write('[00:01.00]Test lyric line\n[00:05.00]Second line') + + client = LyricsClient() + client.api = MagicMock() # API stub — should NOT be called + client._embed_lyrics = MagicMock() + + result = client.create_lrc_file( + audio_file_path=fake_audio_file, + track_name='Test', + artist_name='Artist', + ) + + assert result is True + # API never hit — sidecar shortcut + client.api.get_lyrics.assert_not_called() + client.api.search_lyrics.assert_not_called() + # USLT was re-embedded + client._embed_lyrics.assert_called_once() + call_args = client._embed_lyrics.call_args + assert call_args.args[0] == fake_audio_file + assert 'Test lyric line' in call_args.args[1] + + +def test_existing_txt_sidecar_also_triggers_reembed(fake_audio_file): + """Same shape with .txt sidecar (plain lyrics, no timestamps).""" + from core.lyrics_client import LyricsClient + + sidecar_path = os.path.splitext(fake_audio_file)[0] + '.txt' + with open(sidecar_path, 'w', encoding='utf-8') as f: + f.write('Just plain lyrics no timestamps') + + client = LyricsClient() + client.api = MagicMock() + client._embed_lyrics = MagicMock() + + result = client.create_lrc_file( + audio_file_path=fake_audio_file, + track_name='T', artist_name='A', + ) + + assert result is True + client._embed_lyrics.assert_called_once_with( + fake_audio_file, 'Just plain lyrics no timestamps' + ) + + +def test_empty_sidecar_does_not_embed(fake_audio_file): + """Defensive — if the sidecar exists but is empty, don't write an + empty USLT tag.""" + from core.lyrics_client import LyricsClient + + sidecar_path = os.path.splitext(fake_audio_file)[0] + '.lrc' + with open(sidecar_path, 'w', encoding='utf-8') as f: + f.write(' \n ') # whitespace only + + client = LyricsClient() + client.api = MagicMock() + client._embed_lyrics = MagicMock() + + result = client.create_lrc_file( + audio_file_path=fake_audio_file, + track_name='T', artist_name='A', + ) + + assert result is True + client._embed_lyrics.assert_not_called() + + +def test_unreadable_sidecar_swallows_error_returns_true(fake_audio_file): + """If the sidecar is somehow unreadable, return True (don't try + LRClib again — the early-return contract holds), just skip the + embed silently.""" + from core.lyrics_client import LyricsClient + + sidecar_path = os.path.splitext(fake_audio_file)[0] + '.lrc' + with open(sidecar_path, 'wb') as f: + f.write(b'\xff\xfe\x00\x00') # invalid UTF-8 + + client = LyricsClient() + client.api = MagicMock() + client._embed_lyrics = MagicMock() + + result = client.create_lrc_file( + audio_file_path=fake_audio_file, + track_name='T', artist_name='A', + ) + + assert result is True + client.api.get_lyrics.assert_not_called() + + +def test_no_sidecar_falls_through_to_lrclib(fake_audio_file): + """No sidecar → original LRClib fetch path runs (download flow).""" + from core.lyrics_client import LyricsClient + + client = LyricsClient() + fake_lyrics = MagicMock() + fake_lyrics.synced_lyrics = '[00:01.00]synced from api' + fake_lyrics.plain_lyrics = None + client.api = MagicMock() + client.api.get_lyrics.return_value = None + client.api.search_lyrics.return_value = [fake_lyrics] + client._embed_lyrics = MagicMock() + + result = client.create_lrc_file( + audio_file_path=fake_audio_file, + track_name='T', artist_name='A', + ) + + assert result is True + client.api.search_lyrics.assert_called_once() + # Sidecar was created + lrc = os.path.splitext(fake_audio_file)[0] + '.lrc' + assert os.path.exists(lrc) + # And USLT was embedded + client._embed_lyrics.assert_called_once() + + +# ────────────────────────────────────────────────────────────────────── +# RetagDeps integration — generate_lrc_file is now wired +# ────────────────────────────────────────────────────────────────────── + +def test_retagdeps_accepts_generate_lrc_file_field(): + from core.library.retag import RetagDeps + + # Mock the required + optional deps with do-nothing callables + deps = RetagDeps( + config_manager=MagicMock(), + retag_lock=MagicMock(), + spotify_client=MagicMock(), + get_audio_quality_string=lambda *a: '', + enhance_file_metadata=lambda *a: True, + build_final_path_for_track=lambda *a: ('', ''), + safe_move_file=lambda *a: None, + cleanup_empty_directories=lambda *a: None, + download_cover_art=lambda *a: None, + docker_resolve_path=lambda x: x, + _get_retag_state=lambda: {}, + _set_retag_state=lambda v: None, + get_database=lambda: MagicMock(), + generate_lrc_file=lambda *a: True, + ) + assert callable(deps.generate_lrc_file) + + +def test_retagdeps_generate_lrc_file_optional_for_backward_compat(): + """Tests that built RetagDeps without the new field don't break.""" + from core.library.retag import RetagDeps + + deps = RetagDeps( + config_manager=MagicMock(), + retag_lock=MagicMock(), + spotify_client=MagicMock(), + get_audio_quality_string=lambda *a: '', + enhance_file_metadata=lambda *a: True, + build_final_path_for_track=lambda *a: ('', ''), + safe_move_file=lambda *a: None, + cleanup_empty_directories=lambda *a: None, + download_cover_art=lambda *a: None, + docker_resolve_path=lambda x: x, + _get_retag_state=lambda: {}, + _set_retag_state=lambda v: None, + get_database=lambda: MagicMock(), + ) + # Field defaults to None — no crash on construction. + assert deps.generate_lrc_file is None diff --git a/tests/test_tag_writer_multi_artist.py b/tests/test_tag_writer_multi_artist.py new file mode 100644 index 00000000..c1588677 --- /dev/null +++ b/tests/test_tag_writer_multi_artist.py @@ -0,0 +1,175 @@ +"""Tests for the multi-value artist write path in tag_writer. + +Issue #587 — the AcoustID scanner's "Apply Match" retag was bypassing +the user's `metadata_enhancement.tags.write_multi_artist` setting and +writing single-string TPE1 only. The repair-path retag now passes an +``artists_list`` to ``write_tags_to_file`` and the writer respects the +config flag the same way the post-download enrichment pipeline does. +""" + +from __future__ import annotations + +import os +import tempfile +from unittest.mock import patch + +import pytest +from mutagen.flac import FLAC, StreamInfo + +from core.tag_writer import ( + _multi_artist_write_enabled, + _resolve_artists_list_for_write, + write_tags_to_file, +) + + +# ────────────────────────────────────────────────────────────────────── +# _resolve_artists_list_for_write — derives the multi-value list +# ────────────────────────────────────────────────────────────────────── + +def test_resolves_artists_list_field(): + assert _resolve_artists_list_for_write({'artists_list': ['A', 'B']}) == ['A', 'B'] + + +def test_resolves_artists_field_alias(): + assert _resolve_artists_list_for_write({'artists': ['A', 'B']}) == ['A', 'B'] + + +def test_resolves_underscore_artists_list_field(): + # _artists_list — the post-process pipeline's internal field name + assert _resolve_artists_list_for_write({'_artists_list': ['A', 'B']}) == ['A', 'B'] + + +def test_returns_none_when_no_list_supplied(): + assert _resolve_artists_list_for_write({'artist_name': 'Solo'}) is None + + +def test_returns_none_for_non_list_input(): + assert _resolve_artists_list_for_write({'artists_list': 'A, B'}) is None + assert _resolve_artists_list_for_write({'artists_list': {'A': 1}}) is None + + +def test_strips_empty_and_non_string_entries(): + out = _resolve_artists_list_for_write({'artists_list': ['A', '', None, ' ', 'B']}) + assert out == ['A', 'B'] + + +def test_returns_none_when_list_only_has_empty_entries(): + assert _resolve_artists_list_for_write({'artists_list': ['', None, ' ']}) is None + + +# ────────────────────────────────────────────────────────────────────── +# _multi_artist_write_enabled — config gate +# ────────────────────────────────────────────────────────────────────── + +def test_multi_artist_write_reads_config(): + with patch('config.settings.config_manager.get', return_value=True): + assert _multi_artist_write_enabled() is True + with patch('config.settings.config_manager.get', return_value=False): + assert _multi_artist_write_enabled() is False + + +def test_multi_artist_write_swallows_config_error(): + with patch('config.settings.config_manager.get', side_effect=RuntimeError('boom')): + assert _multi_artist_write_enabled() is False + + +# ────────────────────────────────────────────────────────────────────── +# Vorbis end-to-end — write multi-value to a real FLAC file +# ────────────────────────────────────────────────────────────────────── + +def _make_minimal_flac(path): + """Create a tiny but valid FLAC with mutagen's lowest-overhead + stream info so we can write tags and read them back.""" + # Empty audio body — just enough to satisfy mutagen's parser. + # 44-byte FLAC header + STREAMINFO block. + minimal = ( + b'fLaC' + + b'\x80\x00\x00\x22' # last STREAMINFO block, length 34 + + b'\x00\x10\x00\x10' # min/max block size + + b'\x00\x00\x00\x00\x00\x00' # min/max frame size + + b'\x0a\xc4\x42\xf0\x00\x00\x00\x00' # sample rate / channels / etc + + b'\x00' * 16 # MD5 + ) + with open(path, 'wb') as f: + f.write(minimal) + + +@pytest.fixture +def flac_path(): + fd, path = tempfile.mkstemp(suffix='.flac') + os.close(fd) + _make_minimal_flac(path) + yield path + try: + os.remove(path) + except OSError: + pass + + +def test_multi_value_artists_key_written_when_setting_on(flac_path): + with patch('config.settings.config_manager.get', return_value=True): + result = write_tags_to_file(flac_path, { + 'title': 'Track', + 'artist_name': 'Artist A, Artist B', + 'artists_list': ['Artist A', 'Artist B'], + }, embed_cover=False) + + assert result['success'] is True + assert 'artists_multi' in result['written_fields'] + + audio = FLAC(flac_path) + # Display string preserved as `artist` + assert audio.get('artist') == ['Artist A, Artist B'] + # Multi-value list written to `artists` key (Picard convention) + assert audio.get('artists') == ['Artist A', 'Artist B'] + + +def test_multi_value_artists_key_skipped_when_setting_off(flac_path): + with patch('config.settings.config_manager.get', return_value=False): + result = write_tags_to_file(flac_path, { + 'title': 'Track', + 'artist_name': 'Artist A, Artist B', + 'artists_list': ['Artist A', 'Artist B'], + }, embed_cover=False) + + assert result['success'] is True + assert 'artists_multi' not in result['written_fields'] + + audio = FLAC(flac_path) + assert audio.get('artist') == ['Artist A, Artist B'] + # No multi-value key when setting is off — backward compat + assert audio.get('artists') is None + + +def test_single_artist_does_not_write_multi_value_key(flac_path): + """When list has only one entry (or no list at all), don't + write the multi-value key even if the setting is on. The point + is to differentiate true multi-artist from single-artist tracks.""" + with patch('config.settings.config_manager.get', return_value=True): + result = write_tags_to_file(flac_path, { + 'title': 'Track', + 'artist_name': 'Solo Artist', + 'artists_list': ['Solo Artist'], + }, embed_cover=False) + + assert result['success'] is True + assert 'artists_multi' not in result['written_fields'] + audio = FLAC(flac_path) + assert audio.get('artists') is None + + +def test_no_artists_list_legacy_callers_unchanged(flac_path): + """Backward compat — callers that don't supply artists_list get + the same single-string write as before. No regression for the + write_artist_image button or any other tag_writer caller.""" + with patch('config.settings.config_manager.get', return_value=True): + result = write_tags_to_file(flac_path, { + 'title': 'Track', + 'artist_name': 'Solo Artist', + }, embed_cover=False) + + assert result['success'] is True + assert 'artists_multi' not in result['written_fields'] + audio = FLAC(flac_path) + assert audio.get('artists') is None diff --git a/tests/test_tidal_qualifier_filter.py b/tests/test_tidal_qualifier_filter.py new file mode 100644 index 00000000..e97fe51f --- /dev/null +++ b/tests/test_tidal_qualifier_filter.py @@ -0,0 +1,123 @@ +"""Tests for Tidal qualifier filtering across primary + fallback search. + +Issue #589 — when a download query carries a version qualifier ("live", +"unplugged", "acoustic", etc), the qualifier filter must apply to BOTH +the primary search AND fallback variants. Previously it only fired on +fallbacks, so a primary search for "Shy Away (MTV Unplugged Live)" that +happened to surface the studio cut first would accept the wrong file +and only get caught by AcoustID downstream. + +Also covers the album-context extension: for concert / unplugged +releases the live signal lives in the album title, not the track +title. The filter inspects both ``track.name`` AND ``track.album.name``. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +from core.tidal_download_client import TidalDownloadClient + + +def _make_track(name: str, album_name: str = ''): + """Build a minimal duck-typed track object matching what the Tidal + SDK returns: a `name` attribute and an `album` attribute with its + own `name`.""" + track = MagicMock() + track.name = name + track.album = MagicMock() + track.album.name = album_name + return track + + +# ────────────────────────────────────────────────────────────────────── +# _track_name_contains_qualifiers — legacy track-only behavior preserved +# ────────────────────────────────────────────────────────────────────── + +def test_legacy_helper_passes_when_track_name_has_qualifier(): + assert TidalDownloadClient._track_name_contains_qualifiers( + 'Shy Away (MTV Unplugged Live)', ['live'] + ) is True + + +def test_legacy_helper_fails_when_track_name_lacks_qualifier(): + assert TidalDownloadClient._track_name_contains_qualifiers( + 'Shy Away', ['live'] + ) is False + + +def test_legacy_helper_passes_when_no_qualifiers_required(): + assert TidalDownloadClient._track_name_contains_qualifiers( + 'Anything', [] + ) is True + + +# ────────────────────────────────────────────────────────────────────── +# _track_matches_qualifiers — new helper inspects track + album +# ────────────────────────────────────────────────────────────────────── + +def test_qualifier_in_track_name_alone_passes(): + track = _make_track('Shy Away (Live)', 'DAMN.') + assert TidalDownloadClient._track_matches_qualifiers(track, ['live']) is True + + +def test_qualifier_in_album_name_alone_passes(): + # MTV Unplugged scenario — track titled "Shy Away" but album + # carries the live context. Pre-fix this returned False because + # only track.name was checked. + track = _make_track('Shy Away', 'MTV Unplugged Live') + assert TidalDownloadClient._track_matches_qualifiers(track, ['live']) is True + + +def test_qualifier_missing_from_both_fails(): + # User asked for live, Tidal returned the studio cut on a studio + # album. Must reject so the search keeps looking. + track = _make_track('Shy Away', 'Trench') + assert TidalDownloadClient._track_matches_qualifiers(track, ['live']) is False + + +def test_unplugged_qualifier_in_album_name(): + track = _make_track('Only If For A Night', 'MTV Unplugged') + assert TidalDownloadClient._track_matches_qualifiers(track, ['unplugged']) is True + + +def test_multiple_qualifiers_all_required(): + # Both "live" AND "acoustic" must be present somewhere + track = _make_track('Hello', 'Live Acoustic Sessions') + assert TidalDownloadClient._track_matches_qualifiers(track, ['live', 'acoustic']) is True + track2 = _make_track('Hello', 'Live Sessions') # missing acoustic + assert TidalDownloadClient._track_matches_qualifiers(track2, ['live', 'acoustic']) is False + + +def test_no_qualifiers_required_always_passes(): + track = _make_track('Anything', 'Anything') + assert TidalDownloadClient._track_matches_qualifiers(track, []) is True + + +def test_track_with_no_album_attribute(): + # Defensive — duck-typed tracks may not all have album. Use a + # plain object instead of MagicMock so missing .album is real. + class BareTrack: + name = 'Live Track' + album = None + assert TidalDownloadClient._track_matches_qualifiers(BareTrack(), ['live']) is True + assert TidalDownloadClient._track_matches_qualifiers(BareTrack(), ['unplugged']) is False + + +def test_track_with_empty_name_and_album(): + class BareTrack: + name = '' + album = None + assert TidalDownloadClient._track_matches_qualifiers(BareTrack(), ['live']) is False + + +def test_word_boundary_avoids_false_match_on_substring(): + # "session" should NOT match "obsession" + track = _make_track('Obsession', 'Pop Hits') + assert TidalDownloadClient._track_matches_qualifiers(track, ['session']) is False + + +def test_extract_qualifiers_picks_up_live_unplugged(): + quals = TidalDownloadClient._extract_qualifiers('Shy Away (MTV Unplugged Live)') + assert 'live' in quals + assert 'unplugged' in quals diff --git a/web_server.py b/web_server.py index db0d01f9..792a6eb7 100644 --- a/web_server.py +++ b/web_server.py @@ -40,7 +40,7 @@ logger = setup_logging(_log_level, _log_path) # App version — single source of truth for backup metadata, system-info, update check, etc. # Semver: MAJOR.MINOR.PATCH. Bump at each dev→main release. -_SOULSYNC_BASE_VERSION = "2.5.2" +_SOULSYNC_BASE_VERSION = "2.5.3" def _build_version_string(): """Append short commit hash to version when available (e.g. 2.35+abc1234).""" @@ -115,10 +115,7 @@ from core.imports.context import ( get_import_clean_album, get_import_clean_title, get_import_context_album, - get_import_context_artist, get_import_original_search, - get_import_track_info, - normalize_import_context, ) from core.wishlist.payloads import ( build_cancelled_task_wishlist_payload as _build_cancelled_task_wishlist_payload, @@ -161,23 +158,25 @@ from core.wishlist.state import ( is_wishlist_actually_processing as _is_wishlist_actually_processing, reset_flag_if_stuck as _reset_wishlist_flag_if_stuck, ) -from core.imports.album import ( - build_album_import_context, - build_album_import_match_payload, - resolve_album_artist_context, -) from core.imports.album_naming import resolve_album_group as _resolve_album_group +from core.imports.album import build_album_import_match_payload from core.imports.filename import extract_track_number_from_filename, parse_filename_metadata from core.imports.staging import ( - get_import_suggestions_cache, - get_primary_source, get_staging_path, read_staging_file_metadata, - refresh_import_suggestions_cache, - search_import_albums, - search_import_tracks, start_import_suggestions_cache, ) +from core.imports.routes import ImportRouteRuntime as _ImportRouteRuntime +from core.imports.routes import album_match as _import_album_match +from core.imports.routes import album_process as _import_album_process +from core.imports.routes import process_single_import_file as _import_process_single_import_file +from core.imports.routes import search_albums as _import_search_albums +from core.imports.routes import search_tracks as _import_search_tracks +from core.imports.routes import singles_process as _import_singles_process +from core.imports.routes import staging_files as _import_staging_files +from core.imports.routes import staging_groups as _import_staging_groups +from core.imports.routes import staging_hints as _import_staging_hints +from core.imports.routes import staging_suggestions as _import_staging_suggestions from core.imports.paths import build_final_path_for_track as _build_final_path_for_track from core.imports.pipeline import build_import_pipeline_runtime as _build_import_pipeline_runtime from core.metadata.common import get_file_lock @@ -8195,6 +8194,93 @@ def clear_quarantine(): logger.error(f"[Quarantine] Error clearing quarantine: {e}") return jsonify({"success": False, "error": str(e)}), 500 + +def _get_quarantine_dir(): + return os.path.join( + docker_resolve_path(config_manager.get('soulseek.download_path', './downloads')), + 'ss_quarantine', + ) + + +@app.route('/api/quarantine/list', methods=['GET']) +def list_quarantine(): + """Return all quarantined files with sidecar metadata.""" + try: + from core.imports.quarantine import list_quarantine_entries + entries = list_quarantine_entries(_get_quarantine_dir()) + return jsonify({"success": True, "entries": entries}) + except Exception as e: + logger.error(f"[Quarantine] Error listing entries: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + + +@app.route('/api/quarantine/', methods=['DELETE']) +def delete_quarantine_item(entry_id): + """Delete a single quarantined file + sidecar.""" + try: + from core.imports.quarantine import delete_quarantine_entry + ok = delete_quarantine_entry(_get_quarantine_dir(), entry_id) + if not ok: + return jsonify({"success": False, "error": "Entry not found"}), 404 + return jsonify({"success": True}) + except Exception as e: + logger.error(f"[Quarantine] Error deleting {entry_id}: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + + +@app.route('/api/quarantine//approve', methods=['POST']) +def approve_quarantine_item(entry_id): + """One-click approve: restore the file and re-run post-process with the + matching per-check bypass flag set so the original quarantine trigger + is skipped. Other checks still run.""" + try: + from core.imports.quarantine import approve_quarantine_entry + # Restore inside the soulseek download dir so existing path-resolution + # logic finds it. Unique subdir keeps it from re-mingling with active + # transfers. + restore_dir = os.path.join( + docker_resolve_path(config_manager.get('soulseek.download_path', './downloads')), + 'Transfer', + ) + result = approve_quarantine_entry(_get_quarantine_dir(), entry_id, restore_dir) + if result is None: + return jsonify({ + "success": False, + "error": "Cannot one-click approve — entry has thin sidecar (no embedded context). Use 'Recover to Staging' instead.", + }), 400 + restored_path, context, trigger = result + # Mark the bypass so the pipeline skips the trigger that fired. + context['_skip_quarantine_check'] = trigger + # Re-dispatch through the same pipeline. Run async so the HTTP + # request returns quickly — UI polls /list to see the entry vanish. + context_key = f"approve_{entry_id}_{int(time.time())}" + threading.Thread( + target=lambda: _post_process_matched_download(context_key, context, restored_path), + daemon=True, + ).start() + logger.info(f"[Quarantine] Approved {entry_id} (bypass={trigger}) → re-running pipeline") + return jsonify({"success": True, "trigger_bypassed": trigger}) + except Exception as e: + logger.error(f"[Quarantine] Error approving {entry_id}: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + + +@app.route('/api/quarantine//recover', methods=['POST']) +def recover_quarantine_item(entry_id): + """Fallback for legacy thin sidecars: move file into Staging so the user + can manually finish via the existing Import flow.""" + try: + from core.imports.quarantine import recover_to_staging + from core.imports.staging import get_staging_path + target = recover_to_staging(_get_quarantine_dir(), get_staging_path(), entry_id) + if not target: + return jsonify({"success": False, "error": "Entry not found"}), 404 + return jsonify({"success": True, "staged_path": target}) + except Exception as e: + logger.error(f"[Quarantine] Error recovering {entry_id}: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + + @app.route('/api/scan/request', methods=['POST']) def request_media_scan(): """ @@ -15078,6 +15164,8 @@ def _build_retag_deps(): global retag_state retag_state = value + from core.metadata.lyrics import generate_lrc_file as _generate_lrc_file + return _library_retag.RetagDeps( config_manager=config_manager, retag_lock=retag_lock, @@ -15092,6 +15180,7 @@ def _build_retag_deps(): _get_retag_state=_get_state, _set_retag_state=_set_state, get_database=_get_db, + generate_lrc_file=_generate_lrc_file, ) @@ -18900,6 +18989,10 @@ def get_server_playlist_tracks(playlist_id): 'image_url': img, 'duration_ms': t.get('duration_ms', 0), 'position': t.get('position', 0), + # Spotify track id — required for the user-confirmed + # match override lookup (sync_match_cache). Null for + # iTunes-only sources. + 'source_track_id': t.get('source_track_id') or '', }) elif playlist_name: # Legacy fallback: cross-reference with sync history @@ -18939,6 +19032,21 @@ def get_server_playlist_tracks(playlist_id): used_server_indices = set() unmatched_source = [] # (index_in_combined, src_dict) for fuzzy second pass + # Pass 0: User-confirmed match overrides from sync_match_cache. + # When a user previously picked a local file via "Find & Add", + # the (source_track_id → server_track_id) mapping was persisted + # at confidence=1.0. Apply those FIRST so they bypass the + # exact/fuzzy passes entirely. Stale-cache safe — if the cached + # server track no longer exists, the override is silently + # skipped and normal matching runs. + from core.sync.match_overrides import resolve_match_overrides + _db_for_overrides = get_database() + _override_pairs = resolve_match_overrides( + source_tracks, + server_tracks, + lambda src_id: ((_db_for_overrides.read_sync_match_cache(src_id, active_server) or {}).get('server_track_id')), + ) + # Pass 1: Exact title match (normalized — strips feat./ft. qualifiers) for i, src in enumerate(source_tracks): src_name = src.get('name', '') @@ -18953,6 +19061,19 @@ def get_server_playlist_tracks(playlist_id): 'duration_ms': src.get('duration_ms', 0), 'position': src.get('position', i), } + # Override hit — paired by user, skip exact/fuzzy matching. + if i in _override_pairs: + j_override = _override_pairs[i] + used_server_indices.add(j_override) + combined.append({ + 'source_track': src_entry, + 'server_track': server_tracks[j_override], + 'match_status': 'matched', + 'confidence': 1.0, + 'override': True, + }) + continue + src_norm = _norm_title(src_name) best_idx = -1 for j, svr in enumerate(server_tracks): @@ -19125,14 +19246,48 @@ def server_playlist_replace_track(playlist_id): return jsonify({"success": False, "error": str(e)}), 500 +def _persist_find_and_add_match(source_track_id, server_source, server_track_id, server_track_title, source_title, source_artist): + """Wrap match-override persistence with the active DB. No-op when + source_track_id is missing (e.g. add to a non-mirrored playlist).""" + if not source_track_id: + return + try: + from core.sync.match_overrides import record_manual_match + ok = record_manual_match( + get_database(), + source_track_id=source_track_id, + server_source=server_source, + server_track_id=server_track_id, + server_track_title=server_track_title, + source_title=source_title, + source_artist=source_artist, + ) + if ok: + logger.info(f"[ServerPlaylist] Persisted Find & Add override: {source_track_id} → {server_track_id} ({server_source})") + except Exception as e: + logger.warning(f"[ServerPlaylist] Failed to persist Find & Add override: {e}") + + @app.route('/api/server/playlist//add-track', methods=['POST']) def server_playlist_add_track(playlist_id): - """Add a track to a server playlist at a specific position.""" + """Add a track to a server playlist at a specific position. + + When the optional `source_track_id` is provided (the Spotify track id + from a mirrored playlist), the user's selection is also persisted to + sync_match_cache so future syncs auto-match this source→server pair + without requiring the user to re-trigger Find & Add. + """ try: data = request.get_json() track_id = data.get('track_id') playlist_name = data.get('playlist_name', '') position = data.get('position') # 0-based index; None = append + # Optional Spotify source track id — when present, the (source → + # server) mapping is persisted as a hard match override. + source_track_id = data.get('source_track_id') or '' + source_title = data.get('source_title') or '' + source_artist = data.get('source_artist') or '' + server_track_title = data.get('server_track_title') or '' if not track_id: return jsonify({"success": False, "error": "track_id required"}), 400 @@ -19185,6 +19340,7 @@ def server_playlist_add_track(playlist_id): new_id = str(raw_playlist.ratingKey) logger.info(f"[ServerPlaylist] Added track to playlist, playlist ID: {new_id}") + _persist_find_and_add_match(source_track_id, active_server, track_id, server_track_title or new_item.title, source_title, source_artist) return jsonify({"success": True, "message": "Track added", "new_playlist_id": new_id}) elif active_server == 'jellyfin' and media_server_engine.client('jellyfin'): @@ -19194,6 +19350,7 @@ def server_playlist_add_track(playlist_id): track_ids.insert(pos, track_id) new_track_objs = [type('T', (), {'ratingKey': tid, 'title': ''})() for tid in track_ids] media_server_engine.client('jellyfin').update_playlist(playlist_name, new_track_objs) + _persist_find_and_add_match(source_track_id, active_server, track_id, server_track_title, source_title, source_artist) return jsonify({"success": True, "message": "Track added"}) elif active_server == 'navidrome' and media_server_engine.client('navidrome'): @@ -19203,6 +19360,7 @@ def server_playlist_add_track(playlist_id): track_ids.insert(pos, track_id) new_track_objs = [type('T', (), {'ratingKey': tid, 'title': ''})() for tid in track_ids] media_server_engine.client('navidrome').create_playlist(playlist_name, new_track_objs, playlist_id=playlist_id) + _persist_find_and_add_match(source_track_id, active_server, track_id, server_track_title, source_title, source_artist) return jsonify({"success": True, "message": "Track added"}) return jsonify({"success": False, "error": f"Unsupported server: {active_server}"}), 400 @@ -34264,503 +34422,82 @@ def repair_job_progress(): # IMPORT / STAGING SYSTEM # ================================================================================================ -AUDIO_EXTENSIONS = {'.mp3', '.flac', '.ogg', '.opus', '.m4a', '.aac', '.wav', '.wma', '.aiff', '.aif', '.ape'} +def _build_import_route_runtime(): + return _ImportRouteRuntime( + post_process_matched_download=_post_process_matched_download, + add_activity_item=add_activity_item, + automation_engine=automation_engine, + hydrabase_worker=hydrabase_worker, + dev_mode_enabled=dev_mode_enabled, + import_singles_executor=import_singles_executor, + build_album_import_match_payload=build_album_import_match_payload, + process_single_import_file=lambda runtime, file_info: _process_single_import_file(file_info), + logger=logger, + ) + @app.route('/api/import/staging/files', methods=['GET']) def import_staging_files(): - """Scan the staging folder and return audio files with tag metadata.""" - try: - staging_path = get_staging_path() - os.makedirs(staging_path, exist_ok=True) - - files = [] - for root, _dirs, filenames in os.walk(staging_path): - for fname in filenames: - ext = os.path.splitext(fname)[1].lower() - if ext not in AUDIO_EXTENSIONS: - continue - full_path = os.path.join(root, fname) - rel_path = os.path.relpath(full_path, staging_path) - - meta = read_staging_file_metadata(full_path, rel_path) - - files.append({ - 'filename': fname, - 'rel_path': rel_path, - 'full_path': full_path, - 'title': meta['title'], - 'artist': meta['albumartist'] or meta['artist'] or 'Unknown Artist', - 'album': meta['album'], - 'track_number': meta['track_number'], - 'disc_number': meta['disc_number'], - 'extension': ext - }) - - # Sort by filename - files.sort(key=lambda f: f['filename'].lower()) - return jsonify({'success': True, 'files': files, 'staging_path': staging_path}) - except Exception as e: - logger.error(f"Error scanning staging files: {e}") - return jsonify({'success': False, 'error': str(e)}), 500 + payload, status = _import_staging_files(_build_import_route_runtime()) + return jsonify(payload), status @app.route('/api/import/staging/groups', methods=['GET']) def import_staging_groups(): - """Auto-detect album groups from staging files based on their tags. - - Groups files by (album_tag, artist) where both are non-empty and at least 2 files share - the same album+artist combo. Returns groups sorted by file count descending. - """ - try: - staging_path = get_staging_path() - if not os.path.isdir(staging_path): - return jsonify({'success': True, 'groups': []}) - - # Scan files and group by album+artist tags - album_groups = {} # (album_lower, artist_lower) -> {album, artist, files: []} - for root, _dirs, filenames in os.walk(staging_path): - for fname in filenames: - ext = os.path.splitext(fname)[1].lower() - if ext not in AUDIO_EXTENSIONS: - continue - full_path = os.path.join(root, fname) - rel_path = os.path.relpath(full_path, staging_path) - - meta = read_staging_file_metadata(full_path, rel_path) - album = meta['album'] - artist = meta['albumartist'] or meta['artist'] - if not album or not artist: - continue - - key = (album.lower().strip(), artist.lower().strip()) - if key not in album_groups: - album_groups[key] = { - 'album': album.strip(), - 'artist': artist.strip(), - 'files': [] - } - album_groups[key]['files'].append({ - 'filename': fname, - 'full_path': full_path, - 'title': meta['title'], - 'track_number': meta['track_number'], - }) - - # Only return groups with 2+ files - groups = [] - for group in album_groups.values(): - if len(group['files']) >= 2: - group['files'].sort(key=lambda f: f.get('track_number') or 999) - groups.append({ - 'album': group['album'], - 'artist': group['artist'], - 'file_count': len(group['files']), - 'files': group['files'], - 'file_paths': [f['full_path'] for f in group['files']], - }) - - # Sort by file count descending - groups.sort(key=lambda g: g['file_count'], reverse=True) - - return jsonify({'success': True, 'groups': groups}) - except Exception as e: - logger.error(f"Error building staging groups: {e}") - return jsonify({'success': False, 'error': str(e)}), 500 + payload, status = _import_staging_groups(_build_import_route_runtime()) + return jsonify(payload), status @app.route('/api/import/staging/hints', methods=['GET']) def import_staging_hints(): - """Extract album search hints from staging folder (tags + folder names). Fast — no Spotify calls.""" - try: - staging_path = get_staging_path() - if not os.path.isdir(staging_path): - return jsonify({'success': True, 'hints': []}) - - # Collect hints from tags and folder structure - tag_albums = {} # (album, artist) -> file count - folder_hints = {} # subfolder name -> file count - - for root, _dirs, filenames in os.walk(staging_path): - audio_files = [f for f in filenames if os.path.splitext(f)[1].lower() in AUDIO_EXTENSIONS] - if not audio_files: - continue - - # Folder-based hint: use immediate subfolder name relative to staging - rel_dir = os.path.relpath(root, staging_path) - if rel_dir != '.': - # Use the top-level subfolder as the hint - top_folder = rel_dir.split(os.sep)[0] - folder_hints[top_folder] = folder_hints.get(top_folder, 0) + len(audio_files) - - # Tag-based hints - for fname in audio_files: - full_path = os.path.join(root, fname) - try: - from mutagen import File as MutagenFile - tags = MutagenFile(full_path, easy=True) - if tags: - album = (tags.get('album') or [None])[0] - artist = (tags.get('artist') or (tags.get('albumartist') or [None]))[0] - if album: - key = (album.strip(), (artist or '').strip()) - tag_albums[key] = tag_albums.get(key, 0) + 1 - except Exception as e: - logger.debug("tag read failed: %s", e) - - # Build search queries, prioritizing tag-based hints (more specific) - queries = [] - seen_queries_lower = set() - - # Tag-based: sort by file count descending - for (album, artist), _count in sorted(tag_albums.items(), key=lambda x: -x[1]): - q = f"{album} {artist}".strip() if artist else album - if q.lower() not in seen_queries_lower: - seen_queries_lower.add(q.lower()) - queries.append(q) - - # Folder-based: parse "Artist - Album" pattern or use as-is - for folder, _count in sorted(folder_hints.items(), key=lambda x: -x[1]): - q = folder.replace('_', ' ') - if q.lower() not in seen_queries_lower: - seen_queries_lower.add(q.lower()) - queries.append(q) - - # Cap at 5 queries to keep it fast - queries = queries[:5] - - return jsonify({'success': True, 'hints': queries}) - except Exception as e: - logger.error(f"Error getting staging hints: {e}") - return jsonify({'success': False, 'error': str(e)}), 500 + payload, status = _import_staging_hints(_build_import_route_runtime()) + return jsonify(payload), status @app.route('/api/import/search/albums', methods=['GET']) def import_search_albums(): - """Search for albums using the active metadata provider.""" - try: - query = request.args.get('q', '').strip() - if not query: - return jsonify({'success': False, 'error': 'Missing query parameter'}), 400 - - limit = min(int(request.args.get('limit', 12)), 50) - from core.metadata.registry import get_primary_source - - if get_primary_source() == 'hydrabase' and hydrabase_worker and dev_mode_enabled: - hydrabase_worker.enqueue(query, 'albums') - - albums = search_import_albums(query, limit=limit) - return jsonify({'success': True, 'albums': albums}) - except Exception as e: - logger.error(f"Error searching albums for import: {e}") - return jsonify({'success': False, 'error': str(e)}), 500 + payload, status = _import_search_albums( + _build_import_route_runtime(), + request.args.get('q', ''), + request.args.get('limit', 12), + ) + return jsonify(payload), status @app.route('/api/import/album/match', methods=['POST']) def import_album_match(): - """Match staging files to an album's tracklist.""" - try: - data = request.get_json() or {} - album_id = data.get('album_id') - album_name = data.get('album_name', '') - album_artist = data.get('album_artist', '') - source = str(data.get('source') or '').strip().lower() - # Optional: only match specific files (from auto-group selection) - filter_file_paths = set(data.get('file_paths', [])) - if not album_id: - return jsonify({'success': False, 'error': 'Missing album_id'}), 400 - - # Without `source`, the lookup chain has to guess which metadata - # source the album_id came from — and a Deezer numeric id will - # match nothing in Spotify/iTunes/Discogs/etc., resulting in the - # failure-fallback dict that github issue #524 surfaced as - # "Unknown Artist / album_id-as-title / 0 tracks / 1991". Frontend - # fix in the same PR populates source on every match POST; this - # log catches anything that still reaches us without it (curl, - # third-party, regression in another caller). - if not source: - logger.warning( - "[Import Match] Missing 'source' on album_id=%s — lookup will " - "guess via primary-source priority chain. If this fires " - "consistently, a frontend caller is dropping source from " - "the match POST body.", - album_id, - ) - - payload = build_album_import_match_payload( - album_id, - album_name=album_name, - album_artist=album_artist, - file_paths=filter_file_paths, - source=source or None, - ) - return jsonify(payload) - except Exception as e: - logger.error(f"Error matching album for import: {e}") - return jsonify({'success': False, 'error': str(e)}), 500 + payload, status = _import_album_match(_build_import_route_runtime(), request.get_json() or {}) + return jsonify(payload), status @app.route('/api/import/album/process', methods=['POST']) def import_album_process(): - """Process matched album files through the post-processing pipeline.""" - try: - data = request.get_json() or {} - album = data.get('album', {}) - matches = data.get('matches', []) - - if not album or not matches: - return jsonify({'success': False, 'error': 'Missing album or matches data'}), 400 - - processed = 0 - errors = [] - album_name = album.get('name', album.get('album_name', 'Unknown Album')) - artist_name = album.get('artist', album.get('artist_name', 'Unknown Artist')) - album_id = album.get('id', album.get('album_id', '')) - source = str(album.get('source') or data.get('source') or '').strip().lower() - - total_discs = max( - ( - match.get('track', {}).get('disc_number', 1) - for match in matches - if match.get('track') - ), - default=1, - ) - artist_context = resolve_album_artist_context(album, source=source) - - for match in matches: - staging_file = match.get('staging_file') - track = match.get('track') or {} - if not staging_file or not track: - continue - - file_path = staging_file.get('full_path', '') - if not os.path.isfile(file_path): - errors.append(f"File not found: {staging_file.get('filename', '?')}") - continue - - track_name = track.get('name', 'Unknown Track') - track_number = track.get('track_number', 1) - disc_number = track.get('disc_number', 1) - - context_key = f"import_album_{album_id}_{track_number}_{uuid.uuid4().hex[:8]}" - context = build_album_import_context( - album, - track, - artist_context=artist_context, - total_discs=total_discs, - source=source, - ) - - try: - _post_process_matched_download(context_key, context, file_path) - processed += 1 - logger.info(f"Import processed: {track_number}. {track_name} from {album_name}") - except Exception as proc_err: - err_msg = f"{track_name}: {str(proc_err)}" - errors.append(err_msg) - logger.error(f"Import processing error: {err_msg}") - - add_activity_item("", "Album Imported", f"{album_name} by {artist_name} ({processed}/{len(matches)} tracks)", "Now") - - # Emit events through automation engine — same chain as download batches - # batch_complete → auto-scan → library_scan_completed → auto-update DB - if processed > 0: - try: - if automation_engine: - automation_engine.emit('import_completed', { - 'track_count': str(processed), - 'album_name': album_name or '', - 'artist': artist_name or '', - }) - automation_engine.emit('batch_complete', { - 'playlist_name': f"Import: {album_name}" if album_name else 'Import', - 'total_tracks': str(len(matches)), - 'completed_tracks': str(processed), - 'failed_tracks': str(len(errors)), - }) - except Exception as e: - logger.debug("album import automation emit failed: %s", e) - - # Rebuild suggestions cache since staging contents changed - if processed > 0: - refresh_import_suggestions_cache() - - return jsonify({ - 'success': True, - 'processed': processed, - 'total': len(matches), - 'errors': errors - }) - except Exception as e: - logger.error(f"Error processing album import: {e}") - return jsonify({'success': False, 'error': str(e)}), 500 + payload, status = _import_album_process(_build_import_route_runtime(), request.get_json() or {}) + return jsonify(payload), status @app.route('/api/import/search/tracks', methods=['GET']) def import_search_tracks(): - """Search tracks using the configured metadata provider priority order.""" - try: - query = request.args.get('q', '').strip() - if not query: - return jsonify({'success': False, 'error': 'Missing query parameter'}), 400 - - limit = min(int(request.args.get('limit', 10)), 30) - if get_primary_source() == 'hydrabase' and hydrabase_worker and dev_mode_enabled: - hydrabase_worker.enqueue(query, 'tracks') - - tracks = search_import_tracks(query, limit=limit) - return jsonify({'success': True, 'tracks': tracks}) - except Exception as e: - logger.error(f"Error searching tracks for import: {e}") - return jsonify({'success': False, 'error': str(e)}), 500 + payload, status = _import_search_tracks( + _build_import_route_runtime(), + request.args.get('q', ''), + request.args.get('limit', 10), + ) + return jsonify(payload), status def _process_single_import_file(file_info): - """Worker function: validate, resolve metadata, post-process one file. - - Returns ``("ok", title)`` on success, ``("error", message)`` on - failure, or ``("skip", reason)`` for files that need to be reported - but didn't actually run the pipeline. The caller aggregates these. - Designed to be safe to run concurrently from a ThreadPoolExecutor - — each file gets its own UUID context_key, downstream DB writes - serialize via SQLite's busy_timeout, and file-system ops touch - distinct destination paths. - """ - file_path = file_info.get('full_path', '') - if not os.path.isfile(file_path): - return ("error", f"File not found: {file_info.get('filename', '?')}") - - title = file_info.get('title', '') - artist = file_info.get('artist', '') - manual_match = file_info.get('manual_match') - if manual_match is not None and not isinstance(manual_match, dict): - manual_match = None - - manual_match_source = '' - manual_match_id = None - if manual_match: - manual_match_source = str(manual_match.get('source') or '').strip().lower() - manual_match_id = str(manual_match.get('id') or '').strip() - if not manual_match_id or not manual_match_source: - return ("error", f"Malformed manual match for file: {file_info.get('filename', '?')}") - - if not title and not manual_match: - parsed = parse_filename_metadata(file_info.get('filename', '')) - title = parsed.get('title') or os.path.splitext(file_info.get('filename', 'Unknown'))[0] - if not artist: - artist = parsed.get('artist', '') - - from core.imports.resolution import get_single_track_import_context - - try: - resolved = get_single_track_import_context( - title, - artist, - override_id=manual_match_id, - override_source=manual_match_source, - ) - context = normalize_import_context(resolved['context']) - artist_data = get_import_context_artist(context) - track_data = get_import_track_info(context) - final_title = track_data.get('name', title) - final_artist = artist_data.get('name', artist) - - context_key = f"import_single_{uuid.uuid4().hex[:8]}" - _post_process_matched_download(context_key, context, file_path) - logger.info( - "Import single processed: %s by %s (source=%s)", - final_title, - final_artist, - resolved.get('source') or 'local', - ) - return ("ok", final_title) - except Exception as proc_err: - err_msg = f"{title}: {str(proc_err)}" - logger.error(f"Import single processing error: {err_msg}") - return ("error", err_msg) + return _import_process_single_import_file(_build_import_route_runtime(), file_info) @app.route('/api/import/singles/process', methods=['POST']) def import_singles_process(): - """Process individual staging files as singles through the post-processing pipeline. - - Files are processed in parallel through the - ``import_singles_executor`` (3 workers). Per-file work is dominated - by metadata search round-trips, so parallelizing gives a near- - linear speedup without saturating any one provider's rate limits. - """ - try: - data = request.get_json() - files = data.get('files', []) - - if not files: - return jsonify({'success': False, 'error': 'No files provided'}), 400 - - processed = 0 - errors = [] - - # Submit all files at once so the executor pulls 3 at a time. - # as_completed yields in finish order; we don't need ordering - # because the caller just wants a count + error list. - future_to_filename = { - import_singles_executor.submit(_process_single_import_file, file_info): - file_info.get('filename', '?') - for file_info in files - } - - for future in as_completed(future_to_filename): - try: - outcome, payload = future.result() - except Exception as worker_err: - # Catch-all for anything the worker itself didn't catch - # (shouldn't happen — _process_single_import_file wraps - # its own pipeline call — but defensive). - errors.append( - f"{future_to_filename[future]}: worker crashed: {worker_err}" - ) - continue - if outcome == "ok": - processed += 1 - else: - errors.append(payload) - - add_activity_item("", "Singles Imported", f"{processed}/{len(files)} tracks processed", "Now") - - # Emit events through automation engine — same chain as download batches - # batch_complete → auto-scan → library_scan_completed → auto-update DB - if processed > 0: - try: - if automation_engine: - automation_engine.emit('import_completed', { - 'track_count': str(processed), - 'album_name': '', - 'artist': 'Various', - }) - automation_engine.emit('batch_complete', { - 'playlist_name': 'Import: Singles', - 'total_tracks': str(len(files)), - 'completed_tracks': str(processed), - 'failed_tracks': str(len(errors)), - }) - except Exception as e: - logger.debug("singles import automation emit failed: %s", e) - - # Rebuild suggestions cache since staging contents changed - if processed > 0: - refresh_import_suggestions_cache() - - return jsonify({ - 'success': True, - 'processed': processed, - 'total': len(files), - 'errors': errors - }) - except Exception as e: - logger.error(f"Error processing singles import: {e}") - return jsonify({'success': False, 'error': str(e)}), 500 + data = request.get_json() or {} + payload, status = _import_singles_process(_build_import_route_runtime(), data.get('files', [])) + return jsonify(payload), status -# ── Auto-Import Worker ── +# Auto-Import Worker auto_import_worker = None try: from core.auto_import_worker import AutoImportWorker @@ -34942,13 +34679,8 @@ def auto_import_clear_completed(): @app.route('/api/import/staging/suggestions', methods=['GET']) def import_staging_suggestions(): - """Return cached import suggestions. If cache isn't built yet, returns partial/empty with a flag.""" - cache = get_import_suggestions_cache() - return jsonify({ - 'success': True, - 'suggestions': cache['suggestions'], - 'ready': cache['built'], - }) + payload, status = _import_staging_suggestions() + return jsonify(payload), status # ================================================================================================ diff --git a/webui/index.html b/webui/index.html index ecb109f9..a315bd8b 100644 --- a/webui/index.html +++ b/webui/index.html @@ -2200,6 +2200,7 @@ + @@ -2392,12 +2393,6 @@ ⚡ Enhance Quality -
@@ -5038,6 +5033,11 @@ Analyzes loudness and writes ReplayGain track gain/peak tags. Requires ffmpeg. Adds a few seconds per track. +
+ + + Maximum drift between the file's actual length and the metadata source's expected length before the file is quarantined. 0 = auto (3s normal, 5s for tracks >10min). Raise this if tracks routinely quarantine for being a few seconds off (live recordings, alternate masterings, etc). Capped at 60s. +
@@ -7566,6 +7566,9 @@ +
diff --git a/webui/static/downloads.js b/webui/static/downloads.js index 29c58f96..2216e91c 100644 --- a/webui/static/downloads.js +++ b/webui/static/downloads.js @@ -3382,7 +3382,18 @@ function processModalStatusUpdate(playlistId, data) { case 'post_processing': statusText = '⌛ Processing...'; break; case 'completed': statusText = '✅ Completed'; completedCount++; break; case 'not_found': statusText = '🔇 Not Found'; notFoundCount++; break; - case 'failed': statusText = '❌ Failed'; failedOrCancelledCount++; break; + case 'failed': { + // Distinguish quarantine outcomes from generic + // failures — the file is recoverable, not lost. + const _em = (task.error_message || '').toLowerCase(); + if (_em.includes('integrity check failed') || _em.includes('bit depth filter') || _em.includes('verification failed') || _em.includes('quarantin')) { + statusText = '🛡️ Quarantined'; + } else { + statusText = '❌ Failed'; + } + failedOrCancelledCount++; + break; + } case 'cancelled': statusText = '🚫 Cancelled'; failedOrCancelledCount++; break; default: statusText = `⚪ ${task.status}`; break; } diff --git a/webui/static/helper.js b/webui/static/helper.js index 736220dc..e6785eab 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3416,6 +3416,16 @@ const WHATS_NEW = { '2.5.2': [ // --- May 13, 2026 — 2.5.2 release --- { date: 'May 13, 2026 — 2.5.2 release' }, + { title: 'Retag No Longer Strips LYRICS Tag Without Rewriting', desc: 'discord report (netti93): retag tool was clearing the LYRICS / USLT tag and never rewriting it, while the download flow correctly embeds lyrics. asymmetry trace: download pipeline (`core/imports/pipeline.py`) calls `enhance_file_metadata` (clears all tags) then `generate_lrc_file` (writes .lrc sidecar + embeds USLT). retag (`core/library/retag.py`) only called the first half — `enhance_file_metadata` cleared USLT and there was no follow-up to restore it. fix 1: retag now calls `generate_lrc_file` after `enhance_file_metadata`, mirroring the download flow. injectable via `RetagDeps.generate_lrc_file` (optional default for backward compat). fix 2: `lyrics_client.create_lrc_file` used to short-circuit when an .lrc/.txt sidecar already existed (the typical retag case — sidecar moved alongside the audio). pre-fix: returned True without re-embedding USLT. post-fix: reads the existing sidecar and re-embeds the USLT tag. download flow unaffected (no sidecar at fetch time → original LRClib path runs). 7 boundary tests pin: existing .lrc triggers re-embed, existing .txt triggers re-embed, empty sidecar skips embed, unreadable sidecar swallows error, no sidecar falls through to LRClib (download path), `RetagDeps.generate_lrc_file` field accepted + optional for backward compat.', page: 'tools' }, + { title: 'Track Number Tag No Longer Writes "6/0" When Album Total Is Unknown', desc: 'discord report (netti93): downloaded album tracks were tagged with `TRCK = "6/0"` instead of `"6/13"` when source data lacked total_tracks. retag tool wrote correct `"6/13"` because `core/tag_writer.py` already handled the case. trace: `core/metadata/enrichment.py:105` formatted unconditionally as `f"{track_number}/{total_tracks}"` and many album-dict construction sites pass `total_tracks: 0` (per `types.py`, 0 means "unknown" — not a real count). that 0 propagated straight to disk. fix at the consumer boundary so every album-dict constructor stays unchanged: lifted to pure helper `core/metadata/track_number_format.py:format_track_number_tag` that drops the `/N` suffix when total is 0 / None / negative — emits just `"6"` instead. matches retag\'s behavior + ID3 spec convention (TRCK can be `"N"` or `"N/M"`). MP4 trkn tuple gets the same treatment via `format_track_number_tuple` returning `(6, 0)` per spec\'s "unknown total" marker. 16 boundary tests pin every shape: known total / zero total / none total / none track / zero track / negative inputs / string coercion / unparseable strings / floats truncate.', page: 'tools' }, + { title: 'AcoustID Scanner: Multi-Candidate Match + Duration Guard + Multi-Value Retag', desc: 'discord report (foxxify) issue #587: scanner produced false-positive "Wrong Song" findings for tracks where AcoustID returned multiple recordings per fingerprint and the top match was a wrong-credited recording (different MB entry sharing the same fingerprint). also: applying an AcoustID match retag stripped multi-value ARTISTS tags. three coordinated fixes per codex diagnosis. fix 1: scanner now iterates ALL AcoustID candidates (not just `recordings[0]`) — if any candidate matches expected title + artist, no finding. lifted to a shared pure helper `core/matching/acoustid_candidates.py:find_matching_recording` so both verifier and scanner use the same logic. fix 2: duration guard catches fingerprint hash collisions (foxxify\'s 17-minute mashup edit fingerprinted to a 5-minute late-70s japanese hiphop track — different songs, same fingerprint hash collision on a sampled section). when file duration vs AcoustID candidate duration differs by more than max(60s, 35%), the scanner skips the finding. fix 3: scanner retag (`_fix_wrong_song`) was bypassing the user\'s `metadata_enhancement.tags.write_multi_artist` setting because `write_tags_to_file` only wrote single-string TPE1. now extended with optional `artists_list` parameter that, when supplied + setting on, writes the multi-value tag (TXXX:Artists for ID3, `artists` key for vorbis/opus/flac, list-form `\xa9ART` for mp4) — exact same behavior as the post-download enrichment pipeline. AcoustID retag derives the per-artist list by splitting AcoustID\'s credit on the same separators (comma / ampersand / feat. / ft. / etc) the matching layer already uses. AcoustID version-mismatch gate left intact (still correctly catches genuinely-wrong files). 15 tests on the candidate helper + duration guard, 13 tests on the multi-value tag write path, 4 new scanner regression tests pinning every shape: lower-ranked candidate match suppression, no-suppression when no candidate matches, duration mismatch skip, no-skip when duration matches.', page: 'tools' }, + { title: 'Cross-Script Artist Aliases: Cyrillic / Kanji Canonical Names Now Bridge', desc: 'github issue #586 (follow-up to #442): "Dmitry Yablonsky" tracks were quarantining as audio mismatch — file identified as "Русская филармония, Дмитрий Яблонский" (4% artist similarity) — even though the Cyrillic spelling is just the russian transliteration. three layered bugs in the alias resolution chain. fix 1: `fetch_artist_aliases` only read `data.aliases` and IGNORED the artist record\'s canonical `name` and `sort-name`. for artists where the canonical form is the cross-script spelling (e.g. MB stores "Дмитрий Яблонский" as canonical, "Dmitry Yablonsky" as alias — or vice versa), the missing direction never made it into the alias list. now both canonical name + sort-name are included alongside the explicit aliases (deduped). fix 2: `lookup_artist_aliases` ran search in strict mode only (`artist:"..."` lucene query), which skips MB\'s alias and sortname indexes. cross-script searches found nothing under strict. now falls back to non-strict (bare query, hits all indexes) when strict returns empty OR all results fail the trust gate. fix 3: trust gate weighted local similarity 70% — cross-script pairs have similarity ~0 → combined score ~0.30 → below the 0.85 threshold → cached as empty even when MB\'s own confidence was 100. new escape: when MB score is ≥ 95 AND the result is unambiguous (top result clearly leads), accept regardless of local similarity. covers cases where MB definitely knows the right artist but our local sim collapses to zero. existing #442 same-script path (Hiroyuki Sawano ↔ 澤野弘之) still passes via combined-score path. 12 new tests pin every layer + the exact reporter scenario end-to-end via `artist_names_match`. existing alias tests updated to reflect canonical-name inclusion + 2-call strict+non-strict pattern.', page: 'downloads' }, + { title: 'MTV Unplugged & Live Albums No Longer False-Quarantine', desc: 'github issue #589: tracks from live / unplugged / concert albums (MTV Unplugged, Live At Wembley, etc) consistently failed AcoustID verification with "Version mismatch: expected (live) but file is (original)". two upstream bugs fed into the false positive — the AcoustID gate itself was correctly catching the wrong file Tidal had selected. fix 1: album-scoped library check at `core/downloads/master.py` was scoring "Shy Away (MTV Unplugged Live)" (source) vs "Shy Away" (local DB) with raw string similarity → ~0.3 → marked missing → re-downloaded even though user already owned it. new pure helper `core/matching/album_context_title.py:strip_redundant_album_suffix` strips suffixes whose tokens are fully subsumed by the album context (live/unplugged/acoustic/session markers + tolerated noise + album-title words). only fires inside the album-confirmed scope so global matching elsewhere is unchanged. fix 2: `core/tidal_download_client.py` qualifier filter only ran on FALLBACK searches — primary search returned all results unfiltered, so a query for "Shy Away (MTV Unplugged Live)" could accept the studio cut if Tidal ranked it first. now applies to both primary and fallback. fix 3: qualifier check now inspects both `track.name` AND `track.album.name` — for concert / unplugged releases the live signal often lives in the album title, not the track. AcoustID version-mismatch gate left intact (still correctly catches genuinely-wrong files). 19 tests on the album-context helper + 13 tests on the tidal qualifier helper pin every shape: MTV Unplugged variants, dash-style suffixes, brackets, year tolerance, plural-form markers, anti-regression cases (instrumental/remix on a studio album must NOT be stripped), defensive non-dict / missing-album inputs.', page: 'downloads' }, + { title: 'Deezer: Contributing Artist Tagging Now Consistent', desc: 'github issue #588: contributors tagging worked for some tracks but silently dropped them for others — most reproducibly for tracks whose ALBUM was fetched before the per-track post-process ran. trace: `core/deezer_client.py:get_track_details` cache check used `track_position` as the "full payload" sentinel, but BOTH `/track/` AND `/album//tracks` set that field. only `/track/` sets the `contributors` array. when album-tracks data hit the cache first, `get_track_details` returned the partial record → `_build_enhanced_track` found no contributors → the metadata-source contributors-upgrade silently fell back to single-artist. fix: lifted cache-validity to a pure helper `_is_full_track_payload` that requires BOTH `track_position` AND `contributors` key presence (empty list `[]` is valid — single-artist tracks fetched via `/track/` carry it explicitly). partial cache hits now fall through to a fresh `/track/` fetch. 11 boundary tests pin every shape: full payload, single-artist with empty contributors list, partial album-tracks shape, search-result shape, none/non-dict, cache-hit/cache-miss/api-failure paths.', page: 'downloads' }, + { title: 'Server Playlists: Find & Add Now Persists As A Permanent Match', desc: 'github issue #585: when a spotify track name had a versioned suffix not present in the local file (e.g. "Iron Man - 2012 - Remaster" vs "Iron Man") the auto-matcher missed the pair. user could click Find & Add to manually pick the right local file — that worked, file got added to the plex playlist — but the source spotify track stayed in Missing while the added file showed up under Extra, because the matcher had no record of the user-confirmed pairing. on the next sync the source track would re-quarantine and try to download all over again. fix: every Find & Add selection now writes a `(spotify_track_id → server_track_id)` override into `sync_match_cache` at confidence=1.0. the matching algorithm runs an override pass BEFORE the existing exact and fuzzy passes, so any user-confirmed pair short-circuits straight to "matched" without going through normalization at all. covers every kind of mismatch — dash-suffix remasters, covers / karaoke versions, alt masters, cross-language titles, typo\'d local files, anything. logic lifted to `core/sync/match_overrides.py` (pure helpers `resolve_match_overrides` + `record_manual_match`). 18 boundary tests pin: cache-hit pairs, cache-miss falls through, stale-cache (server track removed) handled gracefully, two sources pointing at same server track (UNIQUE-violation defense), str/int id coercion, partial cache hits, defensive against non-dict inputs and DB exceptions. legacy entries without `source_track_id` (non-mirrored playlists) just skip the override path. works across plex / jellyfin / navidrome.', page: 'sync' }, + { title: 'Quarantine Management — See, Approve, Delete Files Without Touching The Filesystem', desc: 'github issue #584: quarantined files used to just sit in `ss_quarantine/` with a thin sidecar — no UI, no recovery, no way to see what got dropped or why. new **Quarantine** tab on the existing Library History modal (downloads page → Download History button) lists every quarantined file with the same row chrome as the Downloads + Server Imports tabs: thumb placeholder, expected track + artist, original filename, trigger badge (Duration / AcoustID / Bit Depth), relative time, expandable details panel showing the full failure reason. three per-row actions: **Approve** (restores the file, re-runs post-processing with ONLY the failing check skipped, lands in your library with full tags + lyrics + scan), **Recover** (legacy fallback for entries quarantined before this PR with thin sidecars — moves to Staging so you finish via Import flow), **Delete** (permanent removal of file + sidecar). all three use the themed soulsync confirm modal + toast feedback (no native browser alert / confirm). per-check bypass means approving a duration-mismatch file still runs AcoustID; approving an AcoustID failure still runs bit-depth — other quality gates stay live so you can only override one trigger at a time. files that fail a different check after approval get re-quarantined with the new trigger label so you can decide again. sidecar now persists the full json-safe context so approve has everything the pipeline needs to re-process. download modal status differentiates "🛡️ Quarantined" from "❌ Failed" so recoverable files are visible at a glance. logic lifted to pure helpers in `core/imports/quarantine.py` (list / delete / approve / recover_to_staging / serialize_quarantine_context) with 27 boundary tests covering orphan files / orphan sidecars / corrupt sidecars / collision-safe filename restoration / full-context vs thin-sidecar dispatch / json round-trip safety. four new endpoints. pipeline change is per-check conditionals at the existing quarantine sites — no blanket skip-all flag.', page: 'downloads' }, + { title: 'Configurable Duration Tolerance For Quarantined Tracks', desc: 'discord question: tracks were quarantining when their actual length drifted by a few seconds from what spotify/musicbrainz reported (3s tolerance hardcoded, 5s for tracks >10min). live recordings, alternate masterings, and some legitimate uploads routinely drift more than that. new setting on settings → metadata → post-processing: "duration tolerance (seconds)". `0 = auto` (preserves the existing 3s/5s defaults). raise it to 10 / 15 / 20 if your library has a lot of drift-prone material. capped at 60s — past that the check is effectively off. applies to ALL matched downloads (soulseek / tidal / qobuz / hifi / youtube / deezer-direct) since they all flow through the same post-process integrity check. logic lifted to a pure helper `core/imports/file_integrity.py:resolve_duration_tolerance` that coerces the config value (none / empty / 0 / negative / unparseable / above-cap) to either a float override or `None` for the auto-scaled default. 12 tests pin every input shape.', page: 'settings' }, + { title: 'Soulseek Downloads: Multi-Artist Tags Now Get Written Properly', desc: 'discord report: tracks downloaded via soulseek were getting tagged with primary artist only (no collab artists), while the same track downloaded via deezer tagged everyone correctly. trace: the soulseek matched-download context constructed `original_search_result` with `artist` (singular string) but no `artists` (list), even though the full multi-artist list lived on `track_info` (the matched spotify track object). `core/metadata/source.py:extract_source_metadata` only read `original_search.artists`, so soulseek path always fell through to the single-artist branch. fix: lifted artist resolution into a pure helper `core/metadata/artist_resolution.py:resolve_track_artists` that walks `original_search.artists` → `track_info.artists` → `artist_dict.name` fallback chain. handles all three list-item shapes (spotify-style dicts, bare strings, anything else stringified). 13 tests pin the resolution order, fallback chain, mixed-shape normalization, whitespace stripping, empty/none handling. composes with the existing deezer per-track upgrade (still fires when single-artist + track_id available) and feat_in_title / artist_separator settings (still drive the joined ARTIST string downstream).', page: 'downloads' }, { title: 'Download Missing Modal: Tracklist Got A Polish Pass', desc: 'visual tune-up only — column layout untouched. hairline row dividers, accent gradient + edge bar on hover, monospace track numbers (glow accent on row hover), monospace tabular duration. status text in both library-match + download-status columns picks up a leading colored dot with a soft halo (green found / amber missing / blue checking / orange downloading / red failed) and pulses while in-flight. artist column centered. soft scrollbar.', page: 'downloads' }, { title: 'Search Source Picker: Fix Default Always Sticking To Spotify', desc: 'enhanced search + global search source picker always defaulted to spotify even when the user\'s primary metadata source was deezer / itunes / discogs / etc. trace: `shared-helpers.js:createSearchController` reads `/status.metadata_source` to pick the initial active icon, then checks `SOURCE_LABELS[src]` to validate. backend was returning `metadata_source` as a dict (`{source, connected, response_time, ...}` — used elsewhere for connection-state display), so `SOURCE_LABELS[]` was always undefined, the `if` guard never fired, and `state.activeSource` silently stayed at the hardcoded `\'spotify\'` default. fix: read `.source` off the dict (with forward-compat fallback to plain-string in case any older /status response shape predates the dict change). other consumers (core.js sidebar tile, helper.js status checker, search.js display) already used `?.source` correctly — this was the only stale call site.', page: 'search' }, { title: 'Download Discography: No Longer Caps Prolific Artists At 50 Releases', desc: 'discord report: clicking "download discography" on an artist with a deep catalogue (bach, beatles complete box, dance / electronic artists with hundreds of remixes) only showed ~50 albums in the modal. trace: `MetadataLookupOptions(limit=50, max_pages=0)` was hardcoded at the discography endpoint and the artist-detail discography view. spotify\'s `max_pages=0` already paginates through everything (per-page is clamped to 10 internally) so spotify-primary users were unaffected. but deezer / itunes / discogs / hydrabase all honor the outer `limit` as a hard cap. fix: bump `limit` from 50 to 200 at all three call sites (`web_server.py` discography endpoint + artist-detail view + `core/artist_source_detail.py`). 200 matches iTunes\'s and Discogs\'s own internal caps and covers near-everyone\'s full catalogue. spotify behavior unchanged.', page: 'library' }, @@ -4339,7 +4349,7 @@ function _getLatestWhatsNewVersion() { const versions = Object.keys(WHATS_NEW) .filter(v => _compareVersions(v, buildVer) <= 0) .sort((a, b) => _compareVersions(b, a)); - return versions[0] || '2.5.2'; + return versions[0] || '2.5.3'; } function openWhatsNew() { diff --git a/webui/static/pages-extra.js b/webui/static/pages-extra.js index 95ec45bb..2329fc3b 100644 --- a/webui/static/pages-extra.js +++ b/webui/static/pages-extra.js @@ -1821,6 +1821,11 @@ async function _serverSelectTrack(trackIndex, mode, newTrackId, el) { for (let k = 0; k < trackIndex; k++) { if (_serverEditorState.tracks[k]?.server_track) serverPos++; } + // source_track carries source_track_id (Spotify ID) when this + // came from a mirrored playlist — the backend uses it to + // persist the Find & Add selection as a permanent match + // override so future syncs auto-pair without user action. + const srcTrack = track.source_track || {}; response = await fetch(`/api/server/playlist/${_serverEditorState.playlistId}/add-track`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, @@ -1828,6 +1833,9 @@ async function _serverSelectTrack(trackIndex, mode, newTrackId, el) { track_id: newTrackId, playlist_name: _serverEditorState.playlistName, position: serverPos, + source_track_id: srcTrack.source_track_id || '', + source_title: srcTrack.name || '', + source_artist: srcTrack.artist || '', }) }); } diff --git a/webui/static/settings.js b/webui/static/settings.js index 5364f643..c38a8fda 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -970,6 +970,7 @@ async function loadSettingsData() { document.getElementById('prefer-caa-art').checked = settings.metadata_enhancement?.prefer_caa_art === true; document.getElementById('lrclib-enabled').checked = settings.metadata_enhancement?.lrclib_enabled !== false; document.getElementById('replaygain-enabled').checked = settings.post_processing?.replaygain_enabled === true; + document.getElementById('duration-tolerance-seconds').value = settings.post_processing?.duration_tolerance_seconds ?? 0; // Load service master toggles document.getElementById('embed-spotify').checked = settings.spotify?.embed_tags !== false; document.getElementById('embed-itunes').checked = settings.itunes?.embed_tags !== false; @@ -2747,6 +2748,7 @@ async function saveSettings(quiet = false) { }, post_processing: { replaygain_enabled: document.getElementById('replaygain-enabled').checked, + duration_tolerance_seconds: parseFloat(document.getElementById('duration-tolerance-seconds').value) || 0, }, library: { music_paths: collectMusicPaths(), diff --git a/webui/static/wishlist-tools.js b/webui/static/wishlist-tools.js index f59ea4d0..c3bea000 100644 --- a/webui/static/wishlist-tools.js +++ b/webui/static/wishlist-tools.js @@ -3094,6 +3094,150 @@ function openLibraryHistoryModal() { } } +// ────────────────────────────────────────────────────────────────────── +// Quarantine tab — rendered inside the Library History modal as a third +// tab next to Downloads + Server Imports. Reuses the existing list + +// pagination chrome; provides per-row Approve / Recover / Delete actions. +// ────────────────────────────────────────────────────────────────────── + +async function loadQuarantineList() { + const list = document.getElementById('library-history-list'); + const pagination = document.getElementById('library-history-pagination'); + const sourceBar = document.getElementById('history-source-bar'); + if (!list) return; + list.innerHTML = '
Loading...
'; + if (pagination) pagination.innerHTML = ''; + if (sourceBar) sourceBar.style.display = 'none'; + + try { + const resp = await fetch('/api/quarantine/list'); + const data = await resp.json(); + const entries = data.entries || []; + const countEl = document.getElementById('history-quarantine-count'); + if (countEl) countEl.textContent = entries.length; + + if (!data.success) { + list.innerHTML = `
Error: ${escapeHtml(data.error || 'Failed to load')}
`; + return; + } + if (entries.length === 0) { + list.innerHTML = '
🛡️

No quarantined files. Nice and clean.
'; + return; + } + list.innerHTML = entries.map(renderQuarantineEntry).join(''); + } catch (err) { + console.error('Error loading quarantine entries:', err); + list.innerHTML = '
Error loading quarantine
'; + } +} + +function renderQuarantineEntry(entry) { + const triggerLabels = { integrity: 'Duration / Integrity', acoustid: 'AcoustID Mismatch', bit_depth: 'Bit Depth Filter', unknown: 'Unknown' }; + const triggerColors = { integrity: '#facc15', acoustid: '#ef5350', bit_depth: '#fb923c', unknown: '#888' }; + const triggerLabel = triggerLabels[entry.trigger] || entry.trigger || 'Unknown'; + const triggerColor = triggerColors[entry.trigger] || '#888'; + + const id = escapeHtml(entry.id); + const approveLabel = entry.has_full_context ? 'Approve' : 'Recover'; + const approveTitle = entry.has_full_context + ? 'Re-run post-processing with only the failing check skipped' + : 'Legacy entry — move to Staging, finish via Import flow'; + const approveCall = entry.has_full_context + ? `approveQuarantineEntry('${id}')` + : `recoverQuarantineEntry('${id}')`; + + const meta = [entry.expected_artist, entry.original_filename].filter(Boolean).join(' — '); + const triggerBadge = `${escapeHtml(triggerLabel)}`; + const reasonDetail = `
Reason: ${escapeHtml(entry.reason || 'Unknown')}
`; + + return `
+
🛡️
+
+
+
+
${escapeHtml(entry.expected_track || entry.original_filename || 'Unknown')}
+
${escapeHtml(meta)}
+
+
${triggerBadge}
+
${formatHistoryTime(entry.timestamp)}
+ + + ▾ +
+
+ ${reasonDetail} +
+
+
`; +} + +async function approveQuarantineEntry(entryId) { + const ok = await showConfirmDialog({ + title: 'Approve Quarantined File', + message: 'Re-run post-processing for this file with only the failing check skipped. The file will be tagged, lyrics generated, and moved into your library. Other quality gates (AcoustID + bit-depth) still run.', + confirmText: 'Approve & Import', + cancelText: 'Cancel', + }); + if (!ok) return; + try { + const r = await fetch(`/api/quarantine/${encodeURIComponent(entryId)}/approve`, { method: 'POST' }); + const data = await r.json(); + if (!data.success) { + showToast(`Approve failed: ${data.error}`, 'error'); + } else { + showToast(`Approved — skipped ${data.trigger_bypassed} check, re-running pipeline.`, 'success'); + } + } catch (err) { + showToast(`Approve failed: ${err.message}`, 'error'); + } + loadQuarantineList(); +} + +async function recoverQuarantineEntry(entryId) { + const ok = await showConfirmDialog({ + title: 'Recover To Staging', + message: 'Legacy entry — no embedded context. The file will be moved to your Staging folder so you can finish via the Import page (manual match).', + confirmText: 'Move To Staging', + cancelText: 'Cancel', + }); + if (!ok) return; + try { + const r = await fetch(`/api/quarantine/${encodeURIComponent(entryId)}/recover`, { method: 'POST' }); + const data = await r.json(); + if (!data.success) { + showToast(`Recover failed: ${data.error}`, 'error'); + } else { + showToast('Moved to Staging — finish via the Import page.', 'success'); + } + } catch (err) { + showToast(`Recover failed: ${err.message}`, 'error'); + } + loadQuarantineList(); +} + +async function deleteQuarantineEntry(entryId) { + const ok = await showConfirmDialog({ + title: 'Delete Quarantined File', + message: 'This permanently removes the file and its metadata sidecar. Cannot be undone.', + confirmText: 'Delete', + cancelText: 'Cancel', + destructive: true, + }); + if (!ok) return; + try { + const r = await fetch(`/api/quarantine/${encodeURIComponent(entryId)}`, { method: 'DELETE' }); + const data = await r.json(); + if (!data.success) { + showToast(`Delete failed: ${data.error}`, 'error'); + } else { + showToast('Quarantined file deleted.', 'success'); + } + } catch (err) { + showToast(`Delete failed: ${err.message}`, 'error'); + } + loadQuarantineList(); +} + function closeLibraryHistoryModal() { const overlay = document.getElementById('library-history-overlay'); if (overlay) overlay.classList.add('hidden'); @@ -3110,6 +3254,12 @@ function switchHistoryTab(tab) { async function loadLibraryHistory() { const { tab, page, limit } = _libraryHistoryState; + if (tab === 'quarantine') { + // Refresh the count for the other two tabs in the background so + // the badge stays accurate when the user switches over. + loadQuarantineList(); + return; + } const list = document.getElementById('library-history-list'); const pagination = document.getElementById('library-history-pagination'); if (!list) return;