diff --git a/README.md b/README.md index a79eeb7e..325ee12b 100644 --- a/README.md +++ b/README.md @@ -105,6 +105,21 @@ SoulSync bridges streaming services to your music library with automated discove - Catches wrong versions (live, remix, cover) even from streaming API sources - Fail-open design: verification errors never block downloads +#### AcoustID API key + +AcoustID verification is opt-in. To enable it, request a free API key +at and paste it into +Settings → AcoustID. Without a key, downloads still complete but the +verification step is skipped silently. + +If a track was previously tagged by AcoustID but the retag action in +the AcoustID Scanner no longer changes anything, see issue #704 — the +most common cause is that the file already carries a +`MUSICBRAINZ_TRACKID` tag, which the retag step uses as a short-circuit +and therefore never overwrites. Removing the cached +`MUSICBRAINZ_TRACKID` (and the `ACOUSTID_ID` if present) from the file +restores the retag. + ### Metadata & Enrichment **10 Background Enrichment Workers**: Spotify, MusicBrainz, iTunes, Deezer, Discogs, AudioDB, Last.fm, Genius, Tidal, Qobuz diff --git a/SPEC_canonical_album_version.md b/SPEC_canonical_album_version.md new file mode 100644 index 00000000..fafb05bd --- /dev/null +++ b/SPEC_canonical_album_version.md @@ -0,0 +1,144 @@ +# Spec: Canonical Album Version (fixes #765 + #767-Bug2) + +**Status:** design only — no code yet. +**Goal:** Pin ONE canonical `(source, album_id)` per album, chosen by best-fit to +the user's actual files, so the Library Reorganizer, Track Number Repair, and +tagging/enrichment all agree on the same release. Today each re-resolves +independently and they contradict each other (Spotify Believer=4 vs MusicBrainz +Believer=3; standard album mislinked to a deluxe release). + +**Canonical-selection rule (decided):** *match the user's actual files.* The +canonical release is the candidate whose track count + per-track durations + +titles best fit what's on disk. Self-correcting: picks standard when you own the +standard, deluxe when you own the deluxe. + +--- + +## Hard requirement: don't disrupt the running app + +Every stage below is **additive and dormant until explicitly consumed**, and +every consumer **falls back to today's behavior when no canonical is set**. So: +- albums with no resolved canonical behave EXACTLY as they do now; +- each stage is independently shippable and reversible; +- nothing big-bangs. + +--- + +## Stage 1 — Schema + pure scorer (ships dormant, zero behavior change) + +### Schema (additive, nullable → migration-safe) +Add to `albums` (guarded `ALTER TABLE ... ADD COLUMN`, idempotent — mirror the +existing column-exists checks; see [[db-schema-review]] migration-safety notes): +- `canonical_source TEXT` — e.g. 'spotify' / 'itunes' / 'musicbrainz' +- `canonical_album_id TEXT` +- `canonical_score REAL` — best-fit score (for transparency / re-resolve gating) +- `canonical_resolved_at TIMESTAMP` + +All nullable. Existing rows = NULL → "unresolved" → consumers fall back. No +backfill in this stage. No reads in this stage. + +### Pure core helper (the testable heart) — `core/metadata/canonical_version.py` +``` +score_release_against_files(file_tracks, release_tracks) -> float +pick_canonical_release(file_tracks, candidates) -> (best, score) | (None, 0) +``` +- `file_tracks`: list of {duration_ms, title, track_number?} read from disk. +- `release_tracks`: a candidate release's tracklist (same shape). +- Scoring (tunable weights): + - **track-count fit** — exact match strongly preferred; |Δcount| penalized. + - **duration alignment** — greedily match each file to its closest release + track by duration (within a tolerance, e.g. ±3s); reward coverage. + - **title overlap** — token/fuzzy overlap as a tiebreaker. + - **graceful degradation** — if a source gives no per-track durations, fall + back to count + title only (never crash, never force-pick). +- Returns the best candidate + score, or (None, 0) when nothing clears a floor + (so we never pin a bad guess — leave it unresolved, consumers fall back). + +### Tests (extreme, like the rest of this codebase) +- standard (11) vs deluxe (17) with 11 files on disk → picks standard. +- same album, 17 files → picks deluxe. +- duration disambiguation when track counts tie (e.g. radio edit vs album). +- missing-duration source → count+title fallback still picks sanely. +- no candidate clears the floor → (None, 0). +- "Believer" standard(=track 3 listing) vs Spotify(=4) with the user's files → + whichever the files actually match. + +**End of Stage 1: scorer exists + tested, columns exist, NOTHING reads/writes +them yet. Provably zero behavior change.** + +--- + +## Stage 2 — Resolver populates canonical (writes, still no consumers) + +A function `resolve_canonical_for_album(album_id, db, ...)`: +1. Gather on-disk file metadata for the album (durations/titles) via the + library's known file paths. +2. Gather candidate releases: every source the album has an ID for + (spotify/itunes/deezer/discogs/soul/musicbrainz) AND — for the deluxe/standard + case — sibling editions discoverable from those. Fetch each tracklist + (cached, rate-limited). +3. `pick_canonical_release(files, candidates)` → store `(source, album_id, score)` + on the album row if it clears the floor. + +Wiring: a small **backfill repair job** (dry-run-capable) + a hook in enrichment +when an album is (re)enriched. Still **no tool READS canonical**, so behavior is +unchanged — this stage only populates the new columns. Reversible: clearing the +columns reverts to unresolved. + +Tests: resolver picks the right release for the standard/deluxe fixtures; stores +nothing when below floor; idempotent re-resolve. + +Cost note: fetching multiple candidate releases = more API calls. Mitigate via +cache + only-on-(re)enrich + the existing rate trackers. Surface in the job's +progress so it's not silent. + +--- + +## Stage 3 — Reorganizer reads canonical (first real behavior change, gated) + +In `library_reorganize._resolve_source`: if the album has +`canonical_source`/`canonical_album_id`, use THAT first; else fall back to the +current `get_source_priority` walk. One-line precedence change, fully gated on +non-NULL. + +Tests: with canonical set → resolves to it; with canonical NULL → byte-identical +to today. Re-run the existing reorganize battery (148 tests) — must stay green. + +**This alone fixes #767-Bug2** (a standard album whose files match the standard +release pins the standard, so reorganize stops targeting the deluxe folder). + +--- + +## Stage 4 — Track Number Repair reads canonical (closes #765) + +In `track_number_repair._resolve_album_tracklist`: add **Fallback -1** (before +everything) — if the album has a canonical `(source, album_id)`, use it. The +existing 6-level cascade stays as the fallback for albums with no canonical +(preserves its all-01-album rescue ability — the regression risk we refused to +take in the reactive fix). + +Now both tools resolve the SAME release → same track numbers → no contradiction. + +Tests: canonical present → both tools agree (shared-release test); canonical +NULL → existing cascade unchanged. + +--- + +## Risks & mitigations +- **Extra API calls** (Stage 2 fetches multiple releases) → cache, rate-limit, + only-on-(re)enrich, progress-logged. +- **Sources without per-track durations** → scorer degrades to count+title. +- **Schema migration** → additive nullable columns only; idempotent guards. +- **Wrong pick** → floor gate (never pin a low-confidence guess); `canonical_score` + stored for inspection/re-resolve; manual override possible later. +- **Backward-compat** → every consumer falls back to today's path when NULL, so + un-resolved albums (incl. all existing albums until backfilled) are unaffected. + +## Out of scope (for now) +- Per-album manual version override UI (can layer on later — the columns support it). +- Merging the two tools into one (the reporter's alt suggestion) — unnecessary + once they share the canonical. + +## Suggested order to build +1, then 2, then 3, then 4 — each shippable and verifiable on its own. We can stop +after any stage and the app is consistent (just with fewer consumers wired). diff --git a/core/amazon_worker.py b/core/amazon_worker.py index c4dbc0c7..ee4b9751 100644 --- a/core/amazon_worker.py +++ b/core/amazon_worker.py @@ -174,6 +174,16 @@ class AmazonWorker: cursor = conn.cursor() self._ensure_amazon_schema(cursor) + # Pinned-group override (Manage Enrichment Workers): process one + # entity type first, then fall through to the normal chain. Unset or + # exhausted ⇒ default artist→album→track order, unchanged. + from core.worker_utils import read_enrichment_priority, priority_pending_item + _prio = read_enrichment_priority('amazon') + if _prio: + _pi = priority_pending_item(cursor, 'amazon', _prio) + if _pi: + return _pi + # Priority 1: Unattempted artists cursor.execute(""" SELECT id, name FROM artists diff --git a/core/audiodb_worker.py b/core/audiodb_worker.py index ec687346..a8767cb8 100644 --- a/core/audiodb_worker.py +++ b/core/audiodb_worker.py @@ -162,6 +162,16 @@ class AudioDBWorker: conn = self.db._get_connection() cursor = conn.cursor() + # Pinned-group override (Manage Enrichment Workers): process one + # entity type first, then fall through to the normal chain. Unset or + # exhausted ⇒ default artist→album→track order, unchanged. + from core.worker_utils import read_enrichment_priority, priority_pending_item + _prio = read_enrichment_priority('audiodb') + if _prio: + _pi = priority_pending_item(cursor, 'audiodb', _prio) + if _pi: + return _pi + # Priority 1: Unattempted artists cursor.execute(""" SELECT id, name diff --git a/core/automation/deps.py b/core/automation/deps.py index 08d73d47..09c52a17 100644 --- a/core/automation/deps.py +++ b/core/automation/deps.py @@ -28,7 +28,7 @@ from __future__ import annotations import threading from dataclasses import dataclass, field -from typing import Any, Callable, Optional +from typing import Any, Callable, Dict, Optional @dataclass @@ -105,6 +105,8 @@ class AutomationDeps: # --- Playlist pipeline entry points --- run_playlist_discovery_worker: Callable[..., Any] run_sync_task: Callable[..., Any] + run_playlist_organize_download: Callable[..., Dict[str, Any]] + missing_download_executor: Any load_sync_status_file: Callable[[], dict] get_deezer_client: Callable[[], Any] parse_youtube_playlist: Callable[[str], Any] diff --git a/core/automation/handlers/_pipeline_shared.py b/core/automation/handlers/_pipeline_shared.py index 560ae3c3..b5261cbb 100644 --- a/core/automation/handlers/_pipeline_shared.py +++ b/core/automation/handlers/_pipeline_shared.py @@ -148,9 +148,45 @@ def run_sync_and_wishlist( log_type='success' if sync_errors == 0 else 'warning', ) + organize_playlists = [pl for pl in playlists if pl.get('organize_by_playlist')] + organize_started = 0 + if organize_playlists and hasattr(deps, 'run_playlist_organize_download'): + for pl in organize_playlists: + pl_id = pl.get('id') + if not pl_id: + continue + pl_name = pl.get('name', '') + try: + org_result = deps.run_playlist_organize_download( + mirrored_playlist_id=int(pl_id), + automation_id=automation_id, + ) + if org_result.get('status') == 'started': + organize_started += 1 + deps.update_progress( + automation_id, + log_line=f'Organize download started for "{pl_name}"', + log_type='success', + ) + elif org_result.get('status') == 'skipped': + deps.update_progress( + automation_id, + log_line=f'Organize download skipped for "{pl_name}": {org_result.get("reason", "")}', + log_type='skip', + ) + except Exception as org_err: # noqa: BLE001 + deps.update_progress( + automation_id, + log_line=f'Organize download error for "{pl_name}": {org_err}', + log_type='warning', + ) + + all_organize = bool(playlists) and len(organize_playlists) == len(playlists) + effective_skip_wishlist = skip_wishlist or all_organize + wishlist_queued = run_wishlist_phase( deps, automation_id, - skip=skip_wishlist, + skip=effective_skip_wishlist, progress_pct=progress_end + 1, wishlist_phase_label=wishlist_phase_label, wishlist_phase_start_log=wishlist_phase_start_log, @@ -161,6 +197,7 @@ def run_sync_and_wishlist( 'skipped': total_skipped, 'errors': sync_errors, 'wishlist_queued': wishlist_queued, + 'organize_downloads_started': organize_started, } diff --git a/core/automation/handlers/database_update.py b/core/automation/handlers/database_update.py index a9d51036..d123be24 100644 --- a/core/automation/handlers/database_update.py +++ b/core/automation/handlers/database_update.py @@ -21,12 +21,49 @@ from typing import Any, Dict from core.automation.deps import AutomationDeps -_TIMEOUT_SECONDS = 7200 # 2 hours — covers the worst large-library case -_STALL_WARNING_SECONDS = 600 # 10 minutes without progress = stall +# Time out on STALL (no progress), not total runtime: a large library can scan +# for many hours while progressing fine — a hard total cap would falsely mark a +# healthy scan 'error' (the scan thread keeps running uncancelled). We only give +# up when progress hasn't moved for a long stretch, with a generous absolute +# backstop against a truly stuck monitor loop. +_STALL_WARNING_SECONDS = 600 # warn after 10 min with no progress (repeats) +_STALL_TIMEOUT_SECONDS = 1800 # 30 min with no progress at all = genuinely stalled +_ABSOLUTE_CAP_SECONDS = 86400 # 24h hard backstop (runaway-loop guard only) _POLL_INTERVAL_SECONDS = 3 _INITIAL_DELAY_SECONDS = 1 +def scan_wait_action( + *, + status: str, + idle_seconds: float, + total_seconds: float, + stall_timeout_s: float = _STALL_TIMEOUT_SECONDS, + stall_warn_s: float = _STALL_WARNING_SECONDS, + abs_cap_s: float = _ABSOLUTE_CAP_SECONDS, +) -> str: + """Decide what the monitor loop should do on a poll tick (pure/testable). + + ``idle_seconds`` is time since progress last changed; ``total_seconds`` is + time since the wait began. Returns one of: + ``'finished'`` (task no longer running), ``'stall_timeout'`` (no progress for + too long → give up), ``'abs_timeout'`` (absolute backstop), ``'warn'`` + (stalled long enough to warn but not give up), or ``'continue'``. + + Crucially, an actively-progressing scan keeps resetting ``idle_seconds``, so + it never hits ``stall_timeout`` no matter how long the whole scan takes. + """ + if status != 'running': + return 'finished' + if total_seconds >= abs_cap_s: + return 'abs_timeout' + if idle_seconds >= stall_timeout_s: + return 'stall_timeout' + if idle_seconds >= stall_warn_s: + return 'warn' + return 'continue' + + def auto_start_database_update(config: Dict[str, Any], deps: AutomationDeps) -> Dict[str, Any]: """Run a full or incremental DB update via ``run_db_update_task``.""" return _run_with_progress( @@ -36,7 +73,7 @@ def auto_start_database_update(config: Dict[str, Any], deps: AutomationDeps) -> initial_phase='Initializing...', stall_label='Database update', finished_extras=lambda: {'full_refresh': str(config.get('full_refresh', False))}, - timeout_label='Database update timed out after 2 hours', + timeout_label='Database update timed out after 24 hours', ) @@ -49,7 +86,7 @@ def auto_deep_scan_library(config: Dict[str, Any], deps: AutomationDeps) -> Dict initial_phase='Deep scan: Initializing...', stall_label='Deep scan', finished_extras=lambda: {}, - timeout_label='Deep scan timed out after 2 hours', + timeout_label='Deep scan timed out after 24 hours', ) @@ -83,30 +120,54 @@ def _run_with_progress( deps.db_update_executor.submit(task, *task_args) # Monitor progress (callbacks handle card updates, we just block until done). + # We time out on STALL, not total runtime: ``processed`` advances on every + # artist, so an actively-progressing scan keeps resetting the idle clock and + # is never falsely failed no matter how long the whole library takes. time.sleep(_INITIAL_DELAY_SECONDS) poll_start = time.time() last_progress_time = time.time() - last_progress_val = 0 - while time.time() - poll_start < _TIMEOUT_SECONDS: + # Any of these advancing means the scan is alive. current_item (the artist + # being processed) changes every artist even when the rounded progress % + # holds steady, so it guards against a false stall during slow stretches. + last_progress_val = (0, 0, '') + last_warn_time = 0.0 + outcome = 'finished' + while True: time.sleep(_POLL_INTERVAL_SECONDS) + now = time.time() with deps.db_update_lock: current_status = state.get('status', 'idle') - current_progress = state.get('progress', 0) - if current_status != 'running': + current_val = (state.get('processed', 0), state.get('progress', 0), + state.get('current_item', '')) + if current_val != last_progress_val: + last_progress_val = current_val + last_progress_time = now + + action = scan_wait_action( + status=current_status, + idle_seconds=now - last_progress_time, + total_seconds=now - poll_start, + ) + if action in ('finished', 'stall_timeout', 'abs_timeout'): + outcome = action break - # Stall detection — if no progress change in 10 minutes, warn. - if current_progress != last_progress_val: - last_progress_val = current_progress - last_progress_time = time.time() - elif time.time() - last_progress_time > _STALL_WARNING_SECONDS: + if action == 'warn' and (now - last_warn_time) > _STALL_WARNING_SECONDS: + idle_min = int((now - last_progress_time) / 60) deps.update_progress( automation_id, - log_line=f'{stall_label} appears stalled — waiting...', + log_line=f'{stall_label} — no progress for {idle_min} min, still waiting...', log_type='warning', ) - last_progress_time = time.time() # Reset so warning repeats every 10 min. - else: - # 2-hour timeout reached. + last_warn_time = now + + if outcome == 'stall_timeout': + deps.update_progress( + automation_id, status='error', phase='Stalled', + log_line=f'{stall_label} made no progress for {_STALL_TIMEOUT_SECONDS // 60} minutes — giving up', + log_type='error', + ) + return {'status': 'error', 'reason': 'Stalled (no progress)', '_manages_own_progress': True} + if outcome == 'abs_timeout': deps.update_progress( automation_id, status='error', phase='Timed out', log_line=timeout_label, log_type='error', diff --git a/core/automation/handlers/sync_playlist.py b/core/automation/handlers/sync_playlist.py index 5734b48f..509c25ef 100644 --- a/core/automation/handlers/sync_playlist.py +++ b/core/automation/handlers/sync_playlist.py @@ -145,13 +145,36 @@ def auto_sync_playlist(config: Dict[str, Any], deps: AutomationDeps) -> Dict[str tracks_hash = hashlib.md5(track_ids_str.encode()).hexdigest() sync_id_key = f"auto_mirror_{playlist_id}" + # Full mirror identity (every source_track_id on the playlist). tracks_hash + # only covers tracks_json — if a new mirror row is skipped (no discovery / + # no source id), tracks_hash stays identical to the pre-add sync and we + # used to no-op with "unchanged" while the new song never hit wishlist. + mirror_ids_str = ','.join( + sorted(t.get('source_track_id', '') or '' for t in tracks if t.get('source_track_id')) + ) + mirror_tracks_hash = hashlib.md5(mirror_ids_str.encode()).hexdigest() if mirror_ids_str else '' + + event_data = config.get('_event_data') or {} + try: + tracks_added = int(event_data.get('added') or 0) + except (TypeError, ValueError): + tracks_added = 0 + force_sync = tracks_added > 0 or skipped_count > 0 + try: sync_statuses = deps.load_sync_status_file() last_status = sync_statuses.get(sync_id_key, {}) last_hash = last_status.get('tracks_hash', '') + last_mirror_hash = last_status.get('mirror_tracks_hash', '') last_matched = last_status.get('matched_tracks', -1) - if last_hash == tracks_hash and last_matched >= len(tracks_json): + mirror_changed = bool(mirror_tracks_hash) and mirror_tracks_hash != last_mirror_hash + if ( + not force_sync + and not mirror_changed + and last_hash == tracks_hash + and last_matched >= len(tracks_json) + ): # Exact same tracks, all matched last time — nothing to do. deps.update_progress( auto_id, @@ -162,6 +185,21 @@ def auto_sync_playlist(config: Dict[str, Any], deps: AutomationDeps) -> Dict[str 'status': 'skipped', 'reason': f'All {len(tracks_json)} tracks unchanged since last sync', } + if force_sync and last_hash == tracks_hash and last_matched >= len(tracks_json): + deps.update_progress( + auto_id, + log_line=( + f'Forcing sync: playlist changed ({tracks_added} added) or ' + f'{skipped_count} track(s) need discovery' + ), + log_type='info', + ) + elif mirror_changed: + deps.update_progress( + auto_id, + log_line='Mirror track list changed — running sync', + log_type='info', + ) except Exception as e: deps.logger.debug("mirror sync last-status read: %s", e) @@ -180,9 +218,11 @@ def auto_sync_playlist(config: Dict[str, Any], deps: AutomationDeps) -> Dict[str log_line=f'Starting sync: {len(tracks_json)} tracks', log_type='success', ) + skip_wishlist_add = bool(pl.get('organize_by_playlist')) threading.Thread( target=deps.run_sync_task, args=(sync_id, pl['name'], tracks_json, auto_id, 1, pl.get('image_url', '')), + kwargs={'skip_wishlist_add': skip_wishlist_add}, daemon=True, name=f'auto-sync-{playlist_id}', ).start() diff --git a/core/deezer_worker.py b/core/deezer_worker.py index aba5c6fb..9b592627 100644 --- a/core/deezer_worker.py +++ b/core/deezer_worker.py @@ -163,6 +163,16 @@ class DeezerWorker: conn = self.db._get_connection() cursor = conn.cursor() + # Pinned-group override (Manage Enrichment Workers): process one + # entity type first, then fall through to the normal chain. Unset or + # exhausted ⇒ default artist→album→track order, unchanged. + from core.worker_utils import read_enrichment_priority, priority_pending_item + _prio = read_enrichment_priority('deezer') + if _prio: + _pi = priority_pending_item(cursor, 'deezer', _prio) + if _pi: + return _pi + # Priority 1: Unattempted artists cursor.execute(""" SELECT id, name diff --git a/core/discogs_worker.py b/core/discogs_worker.py index 8663934f..429ba320 100644 --- a/core/discogs_worker.py +++ b/core/discogs_worker.py @@ -174,6 +174,16 @@ class DiscogsWorker: conn = self.db._get_connection() cursor = conn.cursor() + # Pinned-group override (Manage Enrichment Workers): process one + # entity type first, then fall through to the normal chain. Discogs + # has no track endpoint, so only artist/album are honored. + from core.worker_utils import read_enrichment_priority, priority_pending_item + _prio = read_enrichment_priority('discogs') + if _prio in ('artist', 'album'): + _pi = priority_pending_item(cursor, 'discogs', _prio) + if _pi: + return _pi + # Priority 1: Unattempted artists cursor.execute(""" SELECT id, name FROM artists diff --git a/core/discovery/sync.py b/core/discovery/sync.py index eb338edc..c3b513ce 100644 --- a/core/discovery/sync.py +++ b/core/discovery/sync.py @@ -47,6 +47,90 @@ class SyncDeps: update_and_save_sync_status: Callable sync_states: dict sync_lock: Any # threading.Lock + # Optional: post-sync download follow-up for mirrored-playlist automations. + process_wishlist_automatically: Callable[..., Any] | None = None + run_playlist_organize_download: Callable[..., Any] | None = None + is_wishlist_actually_processing: Callable[[], bool] | None = None + + +def _post_sync_automation_followup( + deps: SyncDeps, + *, + automation_id: str, + playlist_id: str, + skip_wishlist_add: bool, + result: Any, +) -> None: + """Queue downloads after an automation sync finishes. + + Sync Playlist runs in a background thread and returns immediately, so a + separate scheduled "Process Wishlist" action often runs on an empty wishlist. + Organize-by-playlist skips sync-time wishlist adds and expects a folder + download batch instead — that only ran in Playlist Pipeline before this hook. + """ + if not automation_id or not str(playlist_id).startswith('auto_mirror_'): + return + try: + mirrored_id = int(str(playlist_id).replace('auto_mirror_', '', 1)) + except ValueError: + return + + failed = int(getattr(result, 'failed_tracks', 0) or 0) + wishlist_added = int(getattr(result, 'wishlist_added_count', 0) or 0) + + if skip_wishlist_add: + org_fn = deps.run_playlist_organize_download + if failed <= 0: + return + if not org_fn: + logger.warning( + "Organize-by-playlist sync left %s missing tracks but organize download is unavailable", + failed, + ) + deps.update_automation_progress( + automation_id, + log_line=f'{failed} missing — enable Playlist Pipeline or disable Organize by Playlist', + log_type='warning', + ) + return + org_result = org_fn(mirrored_playlist_id=mirrored_id, automation_id=automation_id) + status = org_result.get('status', 'unknown') if isinstance(org_result, dict) else 'unknown' + reason = org_result.get('reason', '') if isinstance(org_result, dict) else '' + log_type = 'success' if status == 'started' else 'warning' + detail = f' ({reason})' if reason and status != 'started' else '' + deps.update_automation_progress( + automation_id, + log_line=f'Organize download {status} for {failed} missing track(s){detail}', + log_type=log_type, + ) + return + + if wishlist_added <= 0: + if failed > 0: + deps.update_automation_progress( + automation_id, + log_line=f'{failed} missing but none added to wishlist — check logs', + log_type='warning', + ) + return + + proc_fn = deps.process_wishlist_automatically + if not proc_fn: + return + is_busy = deps.is_wishlist_actually_processing + if is_busy and is_busy(): + deps.update_automation_progress( + automation_id, + log_line=f'Added {wishlist_added} to wishlist; download worker already running', + log_type='info', + ) + return + proc_fn(automation_id=automation_id) + deps.update_automation_progress( + automation_id, + log_line=f'Started wishlist download for {wishlist_added} track(s)', + log_type='success', + ) async def _database_only_find_track(spotify_track, candidate_pool=None): @@ -133,7 +217,17 @@ async def _database_only_find_track(spotify_track, candidate_pool=None): return None, 0.0 -def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, profile_id=1, playlist_image_url='', deps: SyncDeps = None, sync_mode: str = 'replace'): +def run_sync_task( + playlist_id, + playlist_name, + tracks_json, + automation_id=None, + profile_id=1, + playlist_image_url='', + deps: SyncDeps = None, + sync_mode: str = 'replace', + skip_wishlist_add: bool = False, +): """The actual sync function that runs in the background thread.""" sync_states = deps.sync_states sync_lock = deps.sync_lock @@ -360,7 +454,14 @@ def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, p # Wing It mode — skip wishlist for unmatched tracks with sync_lock: is_wing_it = sync_states.get(playlist_id, {}).get('wing_it', False) + sync_service._skip_unmatched_wishlist = is_wing_it or skip_wishlist_add sync_service._skip_wishlist = is_wing_it + if skip_wishlist_add: + logger.info( + "[Organize by Playlist] Skipping sync-time wishlist for '%s' — " + "organize download + batch failure handling cover missing tracks", + playlist_name, + ) # Run the sync (this is a blocking call within this thread) result = deps.run_async(sync_service.sync_playlist(playlist, download_missing=False, profile_id=profile_id, sync_mode=sync_mode)) @@ -459,9 +560,21 @@ def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, p matched = getattr(result, 'matched_tracks', 0) total = getattr(result, 'total_tracks', 0) failed = getattr(result, 'failed_tracks', 0) + wishlist_added = getattr(result, 'wishlist_added_count', 0) or 0 deps.update_automation_progress(automation_id, status='finished', progress=100, phase='Sync complete', - log_line=f'Done: {matched}/{total} matched, {failed} failed', log_type='success') + log_line=( + f'Done: {matched}/{total} in library, {failed} missing' + + (f', {wishlist_added} added to wishlist' if wishlist_added else '') + ), + log_type='success') + _post_sync_automation_followup( + deps, + automation_id=automation_id, + playlist_id=playlist_id, + skip_wishlist_add=skip_wishlist_add, + result=result, + ) # Emit playlist_synced event for automation engine try: @@ -480,12 +593,28 @@ def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, p import hashlib as _hl _track_ids_str = ','.join(sorted(t.get('id', '') for t in tracks_json)) _tracks_hash = _hl.md5(_track_ids_str.encode()).hexdigest() + _mirror_tracks_hash = None + if str(playlist_id).startswith('auto_mirror_'): + try: + _mp_id = int(str(playlist_id).replace('auto_mirror_', '', 1)) + from database.music_database import MusicDatabase + _mtracks = MusicDatabase().get_mirrored_playlist_tracks(_mp_id) + _mids = ','.join( + sorted(t.get('source_track_id', '') or '' for t in _mtracks if t.get('source_track_id')) + ) + _mirror_tracks_hash = _hl.md5(_mids.encode()).hexdigest() if _mids else '' + except Exception as e: + logger.debug("mirror_tracks_hash for sync status: %s", e) snapshot_id = getattr(playlist, 'snapshot_id', None) - deps.update_and_save_sync_status(playlist_id, playlist_name, playlist.owner, snapshot_id, + _status_kwargs = dict( matched_tracks=getattr(result, 'matched_tracks', 0), total_tracks=getattr(result, 'total_tracks', 0), discovered_tracks=len(tracks_json), - tracks_hash=_tracks_hash) + tracks_hash=_tracks_hash, + ) + if _mirror_tracks_hash is not None: + _status_kwargs['mirror_tracks_hash'] = _mirror_tracks_hash + deps.update_and_save_sync_status(playlist_id, playlist_name, playlist.owner, snapshot_id, **_status_kwargs) except Exception as e: logger.error(f"SYNC FAILED for {playlist_id}: {e}") diff --git a/core/downloads/master.py b/core/downloads/master.py index f80d46b6..3769dca3 100644 --- a/core/downloads/master.py +++ b/core/downloads/master.py @@ -343,6 +343,10 @@ def run_full_missing_tracks_process(batch_id, playlist_id, tracks_json, deps: Ma batch_is_album = False batch_profile_id = 1 batch_source = 'spotify' + batch_playlist_folder_mode = False + batch_playlist_name = 'Unknown Playlist' + batch_playlist_id = playlist_id + batch_source_playlist_ref = '' with tasks_lock: if batch_id in download_batches: force_download_all = download_batches[batch_id].get('force_download_all', False) @@ -352,6 +356,30 @@ def run_full_missing_tracks_process(batch_id, playlist_id, tracks_json, deps: Ma batch_artist_context = download_batches[batch_id].get('artist_context') batch_profile_id = download_batches[batch_id].get('profile_id', 1) or 1 batch_source = download_batches[batch_id].get('batch_source', 'spotify') or 'spotify' + batch_playlist_folder_mode = download_batches[batch_id].get('playlist_folder_mode', False) + batch_playlist_name = download_batches[batch_id].get('playlist_name', 'Unknown Playlist') + batch_playlist_id = download_batches[batch_id].get('playlist_id', playlist_id) + batch_source_playlist_ref = ( + download_batches[batch_id].get('source_playlist_ref') or '' + ).strip() + + from core.downloads.playlist_folder import ( + resolve_playlist_folder_mode_for_batch, + track_exists_in_playlist_folder_from_track_data, + ) + effective_playlist_folder_mode, effective_playlist_name = resolve_playlist_folder_mode_for_batch( + db, + playlist_id=str(batch_playlist_id), + playlist_name=batch_playlist_name, + batch_playlist_folder_mode=batch_playlist_folder_mode, + profile_id=batch_profile_id, + source=batch_source, + ) + if effective_playlist_folder_mode and not batch_playlist_folder_mode: + with tasks_lock: + if batch_id in download_batches: + download_batches[batch_id]['playlist_folder_mode'] = True + download_batches[batch_id]['playlist_name'] = effective_playlist_name if force_download_all: logger.warning(f"[Force Download] Force download mode enabled for batch {batch_id} - treating all tracks as missing") @@ -445,6 +473,27 @@ def run_full_missing_tracks_process(batch_id, playlist_id, tracks_json, deps: Ma }) continue + if effective_playlist_folder_mode and not force_download_all: + if track_exists_in_playlist_folder_from_track_data( + effective_playlist_name, + track_data, + ): + logger.info( + f"[Playlist Folder] '{track_name}' already on disk in playlist folder — skipping download" + ) + try: + deps.check_and_remove_track_from_wishlist_by_metadata(track_data) + except Exception as _wl_err: + logger.debug(f"[Playlist Folder] Wishlist removal attempt failed: {_wl_err}") + analysis_results.append({ + 'track_index': track_index, + 'track': track_data, + 'found': True, + 'confidence': 1.0, + 'match_reason': 'playlist_folder_file', + }) + continue + # Skip database check if force download is enabled if force_download_all: logger.warning(f"[Force Download] Skipping database check for '{track_name}' - treating as missing") @@ -982,13 +1031,47 @@ def run_full_missing_tracks_process(batch_id, playlist_id, tracks_json, deps: Ma logger.info(f"[Wishlist] Added album context for: '{track_info.get('name')}' -> '{album_ctx['name']}'") - # Add playlist folder mode flag for sync page playlists - if batch_playlist_folder_mode: + # Add playlist folder mode flag for sync page playlists and wishlist + # tracks tied to a mirrored playlist with organize_by_playlist enabled. + task_pl_folder_mode = batch_playlist_folder_mode + task_pl_name = batch_playlist_name + if not task_pl_folder_mode and playlist_id == 'wishlist': + wl_source = track_info.get('source_info') or {} + if isinstance(wl_source, str): + try: + wl_source = json.loads(wl_source) + except (json.JSONDecodeError, TypeError): + wl_source = {} + wl_pl_ref = wl_source.get('playlist_id') + wl_pl_name = wl_source.get('playlist_name') + wl_pl_source = wl_source.get('source') or 'spotify' + if wl_pl_ref and hasattr(db, 'resolve_mirrored_playlist'): + wl_mirrored = db.resolve_mirrored_playlist( + wl_pl_ref, + profile_id=batch_profile_id, + default_source=wl_pl_source, + ) + if wl_mirrored and wl_mirrored.get('organize_by_playlist'): + task_pl_folder_mode = True + task_pl_name = wl_pl_name or wl_mirrored.get('name') or batch_playlist_name + if task_pl_folder_mode: track_info['_playlist_folder_mode'] = True - track_info['_playlist_name'] = batch_playlist_name - logger.info(f"[Task Creation] Added playlist folder mode for: {track_info.get('name')} → {batch_playlist_name}") + track_info['_playlist_name'] = task_pl_name + if batch_source_playlist_ref: + track_info['source_info'] = { + 'playlist_id': batch_source_playlist_ref, + 'playlist_name': task_pl_name, + 'source': batch_source, + } + logger.info( + f"[Task Creation] Added playlist folder mode for: " + f"{track_info.get('name')} → {task_pl_name}" + ) else: - logger.debug(f"[Debug] Task Creation - playlist folder mode NOT enabled for: {track_info.get('name')}") + logger.debug( + f"[Debug] Task Creation - playlist folder mode NOT enabled for: " + f"{track_info.get('name')}" + ) download_tasks[task_id] = { 'status': 'pending', 'track_info': track_info, diff --git a/core/downloads/playlist_folder.py b/core/downloads/playlist_folder.py new file mode 100644 index 00000000..f9ef994e --- /dev/null +++ b/core/downloads/playlist_folder.py @@ -0,0 +1,128 @@ +"""Playlist-folder layout helpers for download analysis and existence checks.""" + +from __future__ import annotations + +import os +from typing import Any, Dict, List, Optional + +from core.downloads.file_finder import AUDIO_EXTENSIONS +from core.imports.paths import ( + _get_config_manager, + docker_resolve_path, + get_file_path_from_template, + sanitize_filename, +) + + +def _first_artist_name(artists: Any) -> str: + if not artists: + return '' + first = artists[0] + if isinstance(first, dict): + return str(first.get('name', '') or '').strip() + return str(first).strip() + + +def candidate_playlist_folder_paths( + playlist_name: str, + artist: str, + title: str, +) -> List[str]: + """Return absolute candidate paths for a track in playlist-folder layout.""" + if not playlist_name or not title: + return [] + + artist_name = (artist or 'Unknown Artist').strip() + track_name = title.strip() + transfer_dir = docker_resolve_path( + _get_config_manager().get('soulseek.transfer_path', './Transfer') + ) + + template_context = { + 'artist': artist_name, + 'albumartist': artist_name, + 'album': track_name, + 'title': track_name, + 'playlist_name': playlist_name, + 'track_number': 1, + 'disc_number': 1, + 'year': '', + 'quality': '', + 'albumtype': '', + '_artists_list': [{'name': artist_name}], + } + + candidates: List[str] = [] + folder_path, filename_base = get_file_path_from_template(template_context, 'playlist_path') + if folder_path and filename_base: + base = os.path.join(transfer_dir, folder_path, filename_base) + for ext in AUDIO_EXTENSIONS: + candidates.append(base + ext) + else: + playlist_name_sanitized = sanitize_filename(playlist_name) + playlist_dir = os.path.join(transfer_dir, playlist_name_sanitized) + artist_name_sanitized = sanitize_filename(artist_name) + track_name_sanitized = sanitize_filename(track_name) + stem = f'{artist_name_sanitized} - {track_name_sanitized}' + for ext in AUDIO_EXTENSIONS: + candidates.append(os.path.join(playlist_dir, stem + ext)) + + return candidates + + +def track_exists_in_playlist_folder( + playlist_name: str, + artist: str, + title: str, +) -> bool: + """Return True if any audio file exists at the playlist-folder path for this track.""" + for path in candidate_playlist_folder_paths(playlist_name, artist, title): + if os.path.isfile(path): + return True + return False + + +def track_exists_in_playlist_folder_from_track_data( + playlist_name: str, + track_data: Dict[str, Any], +) -> bool: + """Check playlist-folder existence using Spotify-style track payload.""" + title = track_data.get('name', '') or track_data.get('track_name', '') + artist = _first_artist_name(track_data.get('artists', [])) + if not artist: + artist = str(track_data.get('artist_name', '') or '').strip() + return track_exists_in_playlist_folder(playlist_name, artist, title) + + +def resolve_playlist_folder_mode_for_batch( + db: Any, + *, + playlist_id: str, + playlist_name: str, + batch_playlist_folder_mode: bool, + profile_id: int = 1, + source: str = 'spotify', +) -> tuple[bool, str]: + """Merge batch flag with persisted mirrored-playlist preference.""" + if batch_playlist_folder_mode: + return True, playlist_name + + if not hasattr(db, 'resolve_mirrored_playlist'): + return False, playlist_name + + # Pass the batch's source so numeric upstream ids (e.g. Deezer) resolve by + # source instead of colliding with the mirrored-playlists primary key. + mirrored = db.resolve_mirrored_playlist( + playlist_id, profile_id=profile_id, default_source=source or 'spotify' + ) + if mirrored and mirrored.get('organize_by_playlist'): + return True, mirrored.get('name') or playlist_name + return False, playlist_name + + +__all__ = [ + 'candidate_playlist_folder_paths', + 'track_exists_in_playlist_folder', + 'track_exists_in_playlist_folder_from_track_data', + 'resolve_playlist_folder_mode_for_batch', +] diff --git a/core/enrichment/api.py b/core/enrichment/api.py index e4d097d7..99c26110 100644 --- a/core/enrichment/api.py +++ b/core/enrichment/api.py @@ -18,9 +18,14 @@ from __future__ import annotations from typing import Any, Callable, Optional -from flask import Blueprint, jsonify +from flask import Blueprint, jsonify, request from core.enrichment.services import EnrichmentService, get_service +from core.enrichment.unmatched import ( + SERVICE_ENTITY_SUPPORT, + UnmatchedQueryError, + supported_entity_types, +) from utils.logging_config import get_logger @@ -30,25 +35,33 @@ logger = get_logger("enrichment.api") # Hooks the host wires up so the blueprint can persist pause state and # clean up auto-pause / yield-override sets without circular imports. _config_set: Optional[Callable[[str, Any], None]] = None +_config_get: Optional[Callable[[str, Any], Any]] = None _auto_paused_discard: Optional[Callable[[str], None]] = None _yield_override_add: Optional[Callable[[str], None]] = None +_db_getter: Optional[Callable[[], Any]] = None def configure( *, config_set: Optional[Callable[[str, Any], None]] = None, + config_get: Optional[Callable[[str, Any], Any]] = None, auto_paused_discard: Optional[Callable[[str], None]] = None, yield_override_add: Optional[Callable[[str], None]] = None, + db_getter: Optional[Callable[[], Any]] = None, ) -> None: """Wire host-side mutators that the generic routes call after pause/resume. Each is optional — pass None for hosts that don't have a corresponding - mechanism (e.g. tests). + mechanism (e.g. tests). ``db_getter`` returns the live ``MusicDatabase`` + for the unmatched-browser routes; ``config_get``/``config_set`` read and + write the per-worker priority override. """ - global _config_set, _auto_paused_discard, _yield_override_add + global _config_set, _config_get, _auto_paused_discard, _yield_override_add, _db_getter _config_set = config_set + _config_get = config_get _auto_paused_discard = auto_paused_discard _yield_override_add = yield_override_add + _db_getter = db_getter def _persist_paused(service: EnrichmentService, paused: bool) -> None: @@ -153,4 +166,133 @@ def create_blueprint() -> Blueprint: logger.error("Error resuming %s worker: %s", service.id, e) return jsonify({'error': str(e)}), 500 + @bp.route('/api/enrichment//breakdown', methods=['GET']) + def enrichment_breakdown(service_id: str): + """matched / not_found / pending tallies per entity type for the modal.""" + if service_id not in SERVICE_ENTITY_SUPPORT: + return jsonify({'error': f'Unknown enrichment service: {service_id}'}), 404 + if _db_getter is None: + return jsonify({'error': 'database unavailable'}), 503 + try: + db = _db_getter() + breakdown = { + entity: db.get_enrichment_breakdown(service_id, entity) + for entity in supported_entity_types(service_id) + } + return jsonify({'service': service_id, 'breakdown': breakdown}), 200 + except UnmatchedQueryError as e: + return jsonify({'error': str(e)}), 400 + except Exception as e: + logger.error("Error building %s enrichment breakdown: %s", service_id, e) + return jsonify({'error': str(e)}), 500 + + @bp.route('/api/enrichment//unmatched', methods=['GET']) + def enrichment_unmatched(service_id: str): + """Paginated list of items this source hasn't matched (for manual match). + + Query params: ``entity_type`` (artist|album|track), ``status`` + (not_found|pending|unmatched), ``q`` (name search), ``limit``, ``offset``. + """ + if service_id not in SERVICE_ENTITY_SUPPORT: + return jsonify({'error': f'Unknown enrichment service: {service_id}'}), 404 + if _db_getter is None: + return jsonify({'error': 'database unavailable'}), 503 + + entity_type = (request.args.get('entity_type') or 'artist').strip() + status = (request.args.get('status') or 'not_found').strip() + query = (request.args.get('q') or '').strip() or None + try: + limit = int(request.args.get('limit', 50)) + offset = int(request.args.get('offset', 0)) + except (TypeError, ValueError): + return jsonify({'error': 'limit/offset must be integers'}), 400 + + try: + result = _db_getter().get_enrichment_unmatched( + service_id, entity_type, status, query, limit, offset + ) + except UnmatchedQueryError as e: + return jsonify({'error': str(e)}), 400 + except Exception as e: + logger.error("Error listing %s unmatched %ss: %s", service_id, entity_type, e) + return jsonify({'error': str(e)}), 500 + + result.update({ + 'service': service_id, + 'entity_type': entity_type, + 'status': status, + 'limit': limit, + 'offset': offset, + 'entity_types': list(supported_entity_types(service_id)), + }) + return jsonify(result), 200 + + @bp.route('/api/enrichment//retry', methods=['POST']) + def enrichment_retry(service_id: str): + """Re-queue item(s) so the worker re-attempts them. + + Body: ``entity_type`` (artist|album|track), ``scope`` (item|failed), + ``entity_id`` (required when scope='item'). 'failed' re-queues every + not_found item of that entity type. + """ + if service_id not in SERVICE_ENTITY_SUPPORT: + return jsonify({'error': f'Unknown enrichment service: {service_id}'}), 404 + if _db_getter is None: + return jsonify({'error': 'database unavailable'}), 503 + + data = request.get_json(silent=True) or {} + entity_type = (data.get('entity_type') or 'artist').strip() + scope = (data.get('scope') or 'item').strip() + entity_id = data.get('entity_id') + try: + count = _db_getter().reset_enrichment(service_id, entity_type, scope, entity_id) + except UnmatchedQueryError as e: + return jsonify({'error': str(e)}), 400 + except Exception as e: + logger.error("Error re-queuing %s %s (%s): %s", service_id, entity_type, scope, e) + return jsonify({'error': str(e)}), 500 + return jsonify({'success': True, 'reset': count, 'service': service_id, + 'entity_type': entity_type, 'scope': scope}), 200 + + @bp.route('/api/enrichment//priority', methods=['GET']) + def enrichment_get_priority(service_id: str): + """Return the pinned 'process this group first' entity for a worker.""" + if service_id not in SERVICE_ENTITY_SUPPORT: + return jsonify({'error': f'Unknown enrichment service: {service_id}'}), 404 + priority = '' + if _config_get is not None: + try: + priority = (_config_get(f'{service_id}_enrichment_priority', '') or '').strip().lower() + except Exception as e: + logger.debug("reading %s priority: %s", service_id, e) + if priority not in supported_entity_types(service_id): + priority = '' + return jsonify({'service': service_id, 'priority': priority, + 'entity_types': list(supported_entity_types(service_id))}), 200 + + @bp.route('/api/enrichment//priority', methods=['POST']) + def enrichment_set_priority(service_id: str): + """Pin (or clear) the entity type the worker should process first. + + Body: ``entity`` = 'artist'|'album'|'track' to pin, or '' / null / 'none' + to clear. Must be an entity type the source actually enriches. + """ + if service_id not in SERVICE_ENTITY_SUPPORT: + return jsonify({'error': f'Unknown enrichment service: {service_id}'}), 404 + if _config_set is None: + return jsonify({'error': 'config unavailable'}), 503 + data = request.get_json(silent=True) or {} + entity = (data.get('entity') or '').strip().lower() + if entity in ('none', 'clear'): + entity = '' + if entity and entity not in supported_entity_types(service_id): + return jsonify({'error': f'{service_id} does not enrich {entity!r}'}), 400 + try: + _config_set(f'{service_id}_enrichment_priority', entity) + except Exception as e: + logger.error("setting %s priority: %s", service_id, e) + return jsonify({'error': str(e)}), 500 + logger.info("%s enrichment priority set to %r via UI", service_id, entity or '(none)') + return jsonify({'success': True, 'service': service_id, 'priority': entity}), 200 + return bp diff --git a/core/enrichment/unmatched.py b/core/enrichment/unmatched.py new file mode 100644 index 00000000..0d63ac79 --- /dev/null +++ b/core/enrichment/unmatched.py @@ -0,0 +1,269 @@ +"""Read-side helpers for browsing the items an enrichment source hasn't matched. + +The dashboard "Manage Enrichment Workers" modal lists, per source, the +artists / albums / tracks whose ``_match_status`` is ``'not_found'`` +(or still pending = ``NULL``) so the user can manually match them. Every +enrichment source writes a uniform ``_match_status`` column, so one +parametric query serves all 11 workers. + +This module owns the column mapping and SQL construction. ``service`` and +``entity_type`` are whitelisted against :data:`SERVICE_ENTITY_SUPPORT` and the +entity table map before any column name is interpolated — user-supplied values +(the search term, pagination) are always bound parameters, never interpolated. +""" + +from __future__ import annotations + +from typing import List, Optional, Tuple + +# Which entity types each enrichment source covers. Mirrors the authoritative +# ``_SERVICE_ID_COLUMNS`` map in web_server.py (used by manual-match), kept here +# so the unmatched browser is self-contained and unit-testable. Singular keys +# ('artist'/'album'/'track') match the manual-match entity_type vocabulary. +SERVICE_ENTITY_SUPPORT = { + 'spotify': ('artist', 'album', 'track'), + 'musicbrainz': ('artist', 'album', 'track'), + 'deezer': ('artist', 'album', 'track'), + 'audiodb': ('artist', 'album', 'track'), + 'discogs': ('artist', 'album'), # no track-level id column + 'itunes': ('artist', 'album', 'track'), + 'lastfm': ('artist', 'album', 'track'), + 'genius': ('artist', 'track'), # no album-level id column + 'tidal': ('artist', 'album', 'track'), + 'qobuz': ('artist', 'album', 'track'), + 'amazon': ('artist', 'album', 'track'), + # Relationship enrichment (not a metadata source): the Similar Artists worker + # only operates at the artist level, and its _match_status tracks + # whether MusicMap similars were fetched (not a source-id match). So the + # breakdown / unmatched list here means "artists we have / don't have + # similars for" — informative, even though there's no manual-match action. + 'similar_artists': ('artist',), +} + +# entity_type -> table / display-name column / image expression / optional join +# / parent-context expression (the artist an album belongs to; the album a +# track belongs to) so the UI can disambiguate same-named items. +# tracks carry no artwork column of their own, so we borrow the parent album's. +_ENTITY_TABLE = { + 'artist': { + 'table': 'artists', 'name': 'name', + 'image': 'artists.thumb_url', 'join': '', 'parent': None, + }, + 'album': { + 'table': 'albums', 'name': 'title', + 'image': 'albums.thumb_url', + 'join': 'LEFT JOIN artists par ON albums.artist_id = par.id', + 'parent': 'par.name', + }, + 'track': { + 'table': 'tracks', 'name': 'title', + 'image': 'al.thumb_url', + 'join': 'LEFT JOIN albums al ON tracks.album_id = al.id', + 'parent': 'al.title', + }, +} + +# 'unmatched' = not yet matched at all (pending OR explicitly not_found). +VALID_STATUSES = ('not_found', 'pending', 'unmatched') + +# Hard cap so a malicious/buggy caller can't ask for the whole library at once. +MAX_LIMIT = 200 + + +class UnmatchedQueryError(ValueError): + """Raised for an unknown service / unsupported entity type / bad status.""" + + +def supported_entity_types(service: str) -> Tuple[str, ...]: + """Return the entity types a source enriches, or () for an unknown source.""" + return SERVICE_ENTITY_SUPPORT.get(service, ()) + + +def match_status_column(service: str) -> str: + return f"{service}_match_status" + + +def last_attempted_column(service: str) -> str: + return f"{service}_last_attempted" + + +def _validate(service: str, entity_type: str) -> None: + support = SERVICE_ENTITY_SUPPORT.get(service) + if support is None: + raise UnmatchedQueryError(f"Unknown enrichment service: {service!r}") + if entity_type not in support: + raise UnmatchedQueryError( + f"{service} does not enrich {entity_type!r} entities" + ) + if entity_type not in _ENTITY_TABLE: # defensive — support map drift + raise UnmatchedQueryError(f"No table mapping for entity type {entity_type!r}") + + +def _status_predicate(service: str, status: str, qualifier: str) -> str: + """SQL predicate selecting rows in the requested match state. + + ``qualifier`` (the table name/alias) is always prefixed so the predicate is + unambiguous even when the query joins a second table that also carries a + ``_match_status`` column (tracks LEFT JOIN albums). + """ + col = f"{qualifier}.{match_status_column(service)}" + if status == 'not_found': + return f"{col} = 'not_found'" + if status == 'pending': + return f"{col} IS NULL" + # 'unmatched' + return f"({col} IS NULL OR {col} = 'not_found')" + + +def build_unmatched_query( + service: str, + entity_type: str, + status: str = 'not_found', + query: Optional[str] = None, + limit: int = 50, + offset: int = 0, +) -> Tuple[str, List]: + """Build the paginated SELECT for one (service, entity_type, status) view. + + Returns ``(sql, params)``. Selected columns: id, name, image_url, status, + last_attempted. + """ + _validate(service, entity_type) + if status not in VALID_STATUSES: + raise UnmatchedQueryError(f"Invalid status: {status!r}") + + meta = _ENTITY_TABLE[entity_type] + table, name_col, image_expr, join = ( + meta['table'], meta['name'], meta['image'], meta['join'], + ) + ms = match_status_column(service) + la = last_attempted_column(service) + + where = [_status_predicate(service, status, table)] + params: List = [] + if query: + where.append(f"{table}.{name_col} LIKE ?") + params.append(f"%{query}%") + + parent_expr = meta.get('parent') + parent_select = f"{parent_expr} AS parent" if parent_expr else "NULL AS parent" + sql = ( + f"SELECT {table}.id AS id, {table}.{name_col} AS name, " + f"{image_expr} AS image_url, {parent_select}, {table}.{ms} AS status, " + f"{table}.{la} AS last_attempted " + f"FROM {table} {join} " + f"WHERE {' AND '.join(where)} " + f"ORDER BY {table}.{name_col} COLLATE NOCASE " + f"LIMIT ? OFFSET ?" + ).replace(' ', ' ') + + params.append(_clamp_limit(limit)) + params.append(max(int(offset or 0), 0)) + return sql, params + + +def build_count_query( + service: str, + entity_type: str, + status: str = 'not_found', + query: Optional[str] = None, +) -> Tuple[str, List]: + """Build the COUNT(*) matching :func:`build_unmatched_query`'s filters.""" + _validate(service, entity_type) + if status not in VALID_STATUSES: + raise UnmatchedQueryError(f"Invalid status: {status!r}") + + meta = _ENTITY_TABLE[entity_type] + table, name_col = meta['table'], meta['name'] + + where = [_status_predicate(service, status, table)] + params: List = [] + if query: + where.append(f"{table}.{name_col} LIKE ?") + params.append(f"%{query}%") + + sql = f"SELECT COUNT(*) FROM {table} WHERE {' AND '.join(where)}" + return sql, params + + +# Reset scopes for re-queuing items so the worker re-attempts them. +RESET_SCOPES = ('item', 'failed') + + +def build_reset_query( + service: str, + entity_type: str, + scope: str = 'item', + entity_id=None, +) -> Tuple[str, List]: + """Build the UPDATE that re-queues item(s) for enrichment. + + Re-queuing means clearing ``_match_status`` back to NULL (and + ``_last_attempted`` to NULL): every worker's pending query selects + ``match_status IS NULL`` first, so the item is retried on the next pass. + Nulling last_attempted alone is NOT enough — the not_found retry path uses + ``last_attempted < cutoff`` and ``NULL < cutoff`` is false, so the item + would never be picked up. + + * scope='item' -> a single row (requires entity_id) + * scope='failed' -> every 'not_found' row for this entity type + """ + _validate(service, entity_type) + if scope not in RESET_SCOPES: + raise UnmatchedQueryError(f"Invalid reset scope: {scope!r}") + + meta = _ENTITY_TABLE[entity_type] + table = meta['table'] + ms = match_status_column(service) + la = last_attempted_column(service) + set_clause = f"SET {ms} = NULL, {la} = NULL" + + if scope == 'item': + if not entity_id: + raise UnmatchedQueryError("entity_id is required for an item reset") + return f"UPDATE {table} {set_clause} WHERE id = ?", [entity_id] + # 'failed' — re-queue everything this source explicitly gave up on. + return f"UPDATE {table} {set_clause} WHERE {ms} = 'not_found'", [] + + +def build_breakdown_query(service: str, entity_type: str) -> Tuple[str, List]: + """Build the matched / not_found / pending / total tally for one entity type.""" + _validate(service, entity_type) + meta = _ENTITY_TABLE[entity_type] + table = meta['table'] + ms = f"{table}.{match_status_column(service)}" + sql = ( + "SELECT " + f"SUM(CASE WHEN {ms} = 'matched' THEN 1 ELSE 0 END) AS matched, " + f"SUM(CASE WHEN {ms} = 'not_found' THEN 1 ELSE 0 END) AS not_found, " + f"SUM(CASE WHEN {ms} IS NULL THEN 1 ELSE 0 END) AS pending, " + f"COUNT(*) AS total " + f"FROM {table}" + ) + return sql, [] + + +def _clamp_limit(limit) -> int: + try: + n = int(limit) + except (TypeError, ValueError): + return 50 + if n <= 0: + return 50 + return min(n, MAX_LIMIT) + + +__all__ = [ + 'SERVICE_ENTITY_SUPPORT', + 'VALID_STATUSES', + 'MAX_LIMIT', + 'UnmatchedQueryError', + 'supported_entity_types', + 'match_status_column', + 'last_attempted_column', + 'build_unmatched_query', + 'build_count_query', + 'build_breakdown_query', + 'build_reset_query', + 'RESET_SCOPES', +] diff --git a/core/genius_worker.py b/core/genius_worker.py index e2e731fd..1184e6db 100644 --- a/core/genius_worker.py +++ b/core/genius_worker.py @@ -178,6 +178,16 @@ class GeniusWorker: conn = self.db._get_connection() cursor = conn.cursor() + # Pinned-group override (Manage Enrichment Workers): process one + # entity type first, then fall through to the normal chain. Genius + # is artist/track only, so albums are not honored. + from core.worker_utils import read_enrichment_priority, priority_pending_item + _prio = read_enrichment_priority('genius') + if _prio in ('artist', 'track'): + _pi = priority_pending_item(cursor, 'genius', _prio) + if _pi: + return _pi + # Priority 1: Unattempted artists cursor.execute(""" SELECT id, name diff --git a/core/imports/paths.py b/core/imports/paths.py index af9619d7..5744744e 100644 --- a/core/imports/paths.py +++ b/core/imports/paths.py @@ -418,8 +418,20 @@ def _coerce_int(value: Any, default: int = 1) -> int: return coerced if coerced > 0 else default -def build_final_path_for_track(context, artist_context, album_info, file_ext): - """Shared path builder used by both post-processing and verification.""" +def build_final_path_for_track(context, artist_context, album_info, file_ext, create_dirs: bool = True): + """Shared path builder used by both post-processing and verification. + + ``create_dirs`` gates the directory-creation side effects. The download + import flow leaves it True (it's about to write the file there). The + library-reorganize PREVIEW passes False so a dry run can compute the exact + destination path WITHOUT physically creating the folder — fixes #767 (dry + run was leaving empty destination folders behind).""" + _real_makedirs = os.makedirs + + def _ensure_dir(path, **_kw): + if create_dirs: + _real_makedirs(path, exist_ok=True) + transfer_dir = docker_resolve_path(_get_config_manager().get("soulseek.transfer_path", "./Transfer")) context = normalize_import_context(context) track_info = get_import_track_info(context) @@ -440,7 +452,7 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): original_dir = os.path.dirname(original_path) original_stem = os.path.splitext(os.path.basename(original_path))[0] final_path = os.path.join(original_dir, original_stem + file_ext) - os.makedirs(original_dir, exist_ok=True) + _ensure_dir(original_dir, exist_ok=True) logger.info("[Enhance] Using original file location: %s", final_path) return final_path, True @@ -477,12 +489,12 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): folder_path, filename_base = get_file_path_from_template(template_context, "playlist_path") if folder_path and filename_base: final_path = os.path.join(transfer_dir, folder_path, filename_base + file_ext) - os.makedirs(os.path.join(transfer_dir, folder_path), exist_ok=True) + _ensure_dir(os.path.join(transfer_dir, folder_path), exist_ok=True) return final_path, True playlist_name_sanitized = sanitize_filename(playlist_name) playlist_dir = os.path.join(transfer_dir, playlist_name_sanitized) - os.makedirs(playlist_dir, exist_ok=True) + _ensure_dir(playlist_dir, exist_ok=True) artist_name_sanitized = sanitize_filename(template_context["artist"]) track_name_sanitized = sanitize_filename(track_name) new_filename = f"{artist_name_sanitized} - {track_name_sanitized}{file_ext}" @@ -579,10 +591,10 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): if total_discs > 1 and not user_controls_disc: disc_folder = f"{disc_label} {disc_number}" final_path = os.path.join(transfer_dir, folder_path, disc_folder, filename_base + file_ext) - os.makedirs(os.path.join(transfer_dir, folder_path, disc_folder), exist_ok=True) + _ensure_dir(os.path.join(transfer_dir, folder_path, disc_folder), exist_ok=True) else: final_path = os.path.join(transfer_dir, folder_path, filename_base + file_ext) - os.makedirs(os.path.join(transfer_dir, folder_path), exist_ok=True) + _ensure_dir(os.path.join(transfer_dir, folder_path), exist_ok=True) return final_path, True artist_name_sanitized = sanitize_filename(template_context["albumartist"]) @@ -592,7 +604,7 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): album_dir = os.path.join(artist_dir, album_folder_name) if total_discs > 1: album_dir = os.path.join(album_dir, f"{disc_label} {disc_number}") - os.makedirs(album_dir, exist_ok=True) + _ensure_dir(album_dir, exist_ok=True) final_track_name_sanitized = sanitize_filename(clean_track_name) new_filename = f"{track_number:02d} - {final_track_name_sanitized}{file_ext}" return os.path.join(album_dir, new_filename), True @@ -629,10 +641,10 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): if filename_base: if folder_path: final_path = os.path.join(transfer_dir, folder_path, filename_base + file_ext) - os.makedirs(os.path.join(transfer_dir, folder_path), exist_ok=True) + _ensure_dir(os.path.join(transfer_dir, folder_path), exist_ok=True) else: final_path = os.path.join(transfer_dir, filename_base + file_ext) - os.makedirs(transfer_dir, exist_ok=True) + _ensure_dir(transfer_dir, exist_ok=True) return final_path, True artist_name_sanitized = sanitize_filename(template_context["artist"]) @@ -640,6 +652,6 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): artist_dir = os.path.join(transfer_dir, artist_name_sanitized) single_folder_name = f"{artist_name_sanitized} - {final_track_name_sanitized}" single_dir = os.path.join(artist_dir, single_folder_name) - os.makedirs(single_dir, exist_ok=True) + _ensure_dir(single_dir, exist_ok=True) new_filename = f"{final_track_name_sanitized}{file_ext}" return os.path.join(single_dir, new_filename), True diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index fa196aa5..dc6c9bf4 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -110,6 +110,33 @@ def _mark_task_quarantined(context: dict, quarantine_path: str | None) -> None: download_tasks[task_id]['quarantine_entry_id'] = entry_id +def import_rejection_reason(context: dict) -> str | None: + """Human-readable reason if post-processing terminally rejected the file + (quarantine or race-guard), else ``None`` for a clean import. + + ``post_process_matched_download`` signals these outcomes by setting context + flags and returning normally — it only raises on unexpected errors. The + download path reads those flags in + ``post_process_matched_download_with_verification`` and marks the task + failed, but the MANUAL-import routes call ``post_process_matched_download`` + directly with no task_id, so without this check a quarantined file (now in + ss_quarantine, not the library) is counted as a successful import and the + UI shows a green "Done" (#764). Pure + testable: it only inspects the + context dict the inner pipeline populated.""" + if context.get('_integrity_failure_msg'): + return f"integrity check failed: {context['_integrity_failure_msg']}" + if context.get('_acoustid_quarantined'): + return ( + "AcoustID verification failed: " + f"{context.get('_acoustid_failure_msg', 'fingerprint mismatch')}" + ) + if context.get('_bitdepth_rejected'): + return "rejected by bit-depth filter" + if context.get('_race_guard_failed'): + return "source file disappeared before import completed" + return None + + def build_import_pipeline_runtime( *, automation_engine: Any | None = None, @@ -563,6 +590,26 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta record_library_history_download(context) record_download_provenance(context) + try: + pf_album_info = build_import_album_info(context, force_album=False) + if not pf_album_info or not pf_album_info.get("album_name"): + pf_album_info = { + "is_album": True, + "album_name": playlist_name, + "track_number": track_info.get("track_number", 1) or 1, + "disc_number": track_info.get("disc_number", 1) or 1, + "clean_track_name": get_import_clean_title( + context, + default=get_import_original_search(context).get("title", "Unknown"), + ), + "source": get_import_source(context) or "spotify", + } + elif not pf_album_info.get("is_album"): + pf_album_info["is_album"] = True + record_soulsync_library_entry(context, artist_context, pf_album_info) + except Exception as lib_err: + logger.error(f"[Playlist Folder] SoulSync library registration failed: {lib_err}") + task_id = context.get('task_id') batch_id = context.get('batch_id') if task_id and batch_id: diff --git a/core/imports/routes.py b/core/imports/routes.py index edd38f08..1be49b49 100644 --- a/core/imports/routes.py +++ b/core/imports/routes.py @@ -11,6 +11,7 @@ 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.pipeline import import_rejection_reason from core.imports.staging import ( AUDIO_EXTENSIONS, get_import_suggestions_cache, @@ -331,8 +332,17 @@ def album_process(runtime: ImportRouteRuntime, data: Dict[str, Any]) -> tuple[Di 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) + # A quarantine/race-guard rejection returns normally (no + # exception) and leaves the file in ss_quarantine, NOT the + # library — so it must be reported as an error, not counted + # as a successful import (#764). + reject_reason = import_rejection_reason(context) + if reject_reason: + errors.append(f"{track_name}: {reject_reason}") + runtime.logger.warning("Import rejected: %s — %s", track_name, reject_reason) + else: + 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) @@ -422,6 +432,13 @@ def process_single_import_file(runtime: ImportRouteRuntime, file_info: Dict[str, context_key = f"import_single_{uuid.uuid4().hex[:8]}" runtime.post_process_matched_download(context_key, context, file_path) + # Quarantine/race-guard returns normally but the file is in + # ss_quarantine, not the library — report it as an error rather than + # "ok", else the UI shows a green "Done" for a file that vanished (#764). + reject_reason = import_rejection_reason(context) + if reject_reason: + runtime.logger.warning("Import single rejected: %s — %s", final_title, reject_reason) + return ("error", f"{final_title}: {reject_reason}") runtime.logger.info( "Import single processed: %s by %s (source=%s)", final_title, diff --git a/core/itunes_worker.py b/core/itunes_worker.py index 50cf0df0..cca60db0 100644 --- a/core/itunes_worker.py +++ b/core/itunes_worker.py @@ -172,6 +172,17 @@ class iTunesWorker: conn = self.db._get_connection() cursor = conn.cursor() + # Pinned-group override (Manage Enrichment Workers): process one + # entity type first, then fall through to the normal chain. Unset or + # exhausted ⇒ default artist→album→track order, unchanged. + from core.worker_utils import read_enrichment_priority, priority_pending_item + _prio = read_enrichment_priority('itunes') + if _prio: + _pi = priority_pending_item(cursor, 'itunes', _prio, + {'album': 'album_individual', 'track': 'track_individual'}) + if _pi: + return _pi + # Priority 1: Unattempted artists cursor.execute(""" SELECT id, name diff --git a/core/lastfm_worker.py b/core/lastfm_worker.py index 69a4c360..c2011e40 100644 --- a/core/lastfm_worker.py +++ b/core/lastfm_worker.py @@ -177,6 +177,16 @@ class LastFMWorker: conn = self.db._get_connection() cursor = conn.cursor() + # Pinned-group override (Manage Enrichment Workers): process one + # entity type first, then fall through to the normal chain. Unset or + # exhausted ⇒ default artist→album→track order, unchanged. + from core.worker_utils import read_enrichment_priority, priority_pending_item + _prio = read_enrichment_priority('lastfm') + if _prio: + _pi = priority_pending_item(cursor, 'lastfm', _prio) + if _pi: + return _pi + # Priority 1: Unattempted artists cursor.execute(""" SELECT id, name diff --git a/core/library_reorganize.py b/core/library_reorganize.py index af795789..02509c11 100644 --- a/core/library_reorganize.py +++ b/core/library_reorganize.py @@ -227,6 +227,25 @@ def _resolve_source(album_data: dict, primary_source: str, strict_source: bool = """ source_ids = _extract_source_ids(album_data) + # #765: if a canonical release was pinned for this album (best-fit to the + # user's actual files), prefer it — so reorganize agrees with Track Number + # Repair and stops mislabelling standard albums as deluxe (#767-Bug2). Gated + # on the album row carrying a canonical, and skipped when the user explicitly + # picked a source in the modal (strict_source) — their choice wins. Falls + # through to the priority walk if the canonical fetch fails. + if not strict_source: + c_source = album_data.get('canonical_source') + c_id = album_data.get('canonical_album_id') + if c_source and c_id: + try: + api_album = get_album_for_source(c_source, c_id) + api_tracks = get_album_tracks_for_source(c_source, c_id) + items = _normalize_album_tracks(api_tracks) + if items and api_album: + return c_source, api_album, items + except Exception as e: + logger.warning(f"[Reorganize] canonical {c_source} lookup raised: {e}") + if strict_source: sources_to_try = [primary_source] if primary_source else [] else: @@ -989,7 +1008,12 @@ def preview_album_reorganize( album_info = _build_album_info(context) try: spotify_artist = context['spotify_artist'] - new_full, _ok = build_final_path_fn(context, spotify_artist, album_info, file_ext) + # Dry run: compute the destination path WITHOUT creating the folder. + # Previously this physically created the album dir during preview, + # leaving empty folders all over the library (#767). + new_full, _ok = build_final_path_fn( + context, spotify_artist, album_info, file_ext, create_dirs=False + ) item['new_path'] = ( os.path.relpath(new_full, transfer_dir) if transfer_dir and new_full and new_full.startswith(transfer_dir) diff --git a/core/metadata/art_apply.py b/core/metadata/art_apply.py new file mode 100644 index 00000000..e267a3e5 --- /dev/null +++ b/core/metadata/art_apply.py @@ -0,0 +1,157 @@ +"""Apply album art to existing library files. + +Two jobs, both reusing the post-processing standard so the user's +``album_art_order`` preference is honored and embedded art matches cover.jpg: + +- Detect whether an album already has art ON DISK (embedded in the audio file + or a cover.jpg/folder.jpg sidecar) — the Cover Art Filler previously only + looked at the DB ``thumb_url``, so albums whose files were artless but whose + DB row had a URL were never flagged. +- Embed found art into the album's audio files (``embed_album_art_metadata``) + and write a cover.jpg (``download_cover_art``). Only ADDS art — it does not + clear or rewrite the user's existing tags. +""" + +from __future__ import annotations + +import contextlib +import os +from typing import Iterable + +from core.metadata.artwork import download_cover_art, embed_album_art_metadata +from core.metadata.common import get_mutagen_symbols +from utils.logging_config import get_logger + +logger = get_logger("metadata.art_apply") + +# Folder-level cover files recognised across players (matches soulsync_client). +_COVER_SIDECARS = ( + "cover.jpg", "cover.jpeg", "cover.png", + "folder.jpg", "folder.jpeg", "folder.png", +) + + +def folder_has_cover_sidecar(folder: str) -> bool: + """True if the album folder already carries a cover.jpg/folder.jpg sidecar.""" + if not folder: + return False + try: + for name in _COVER_SIDECARS: + if os.path.isfile(os.path.join(folder, name)): + return True + except OSError: + return False + return False + + +def file_has_embedded_art(file_path: str) -> bool: + """True if the audio file already has embedded cover art (FLAC picture, + ID3 APIC, MP4 covr, or a Vorbis metadata_block_picture).""" + if not file_path or not os.path.isfile(file_path): + return False + symbols = get_mutagen_symbols() + if not symbols: + return False + try: + return _audio_has_art(symbols.File(file_path), symbols) + except Exception as exc: + logger.debug("art presence check failed for %s: %s", file_path, exc) + return False + + +def _audio_has_art(audio, symbols) -> bool: + """True if an already-open mutagen object carries embedded cover art.""" + if audio is None: + return False + # FLAC / Ogg expose picture blocks directly. + if getattr(audio, "pictures", None): + return True + if isinstance(audio, symbols.MP4): + return bool(audio.get("covr")) + tags = getattr(audio, "tags", None) + if tags is None: + return False + with contextlib.suppress(Exception): + if isinstance(tags, symbols.ID3): + return bool(tags.getall("APIC")) + with contextlib.suppress(Exception): + if "metadata_block_picture" in tags: + return True + return False + + +def album_has_art_on_disk(rep_file_path: str) -> bool: + """Does this album have art on disk? + + Checks the folder for a cover sidecar first (cheap stat) and only opens the + representative audio file when there's no sidecar. Returns True when there's + no local file to inspect (e.g. a media-server-only album) so such albums + aren't wrongly flagged as missing file art. + """ + if not rep_file_path: + return True + folder = os.path.dirname(rep_file_path) + if folder_has_cover_sidecar(folder): + return True + return file_has_embedded_art(rep_file_path) + + +def apply_art_to_album_files( + file_paths: Iterable[str], + metadata: dict, + album_info: dict, + folder: str = None, + context: dict = None, +) -> dict: + """Embed art into each audio file + write cover.jpg, reusing the standard. + + ``metadata`` feeds ``embed_album_art_metadata`` (needs album_artist/artist/ + album, optionally musicbrainz_release_id and album_art_url as the fallback + URL). ``album_info`` feeds ``download_cover_art`` (album_name/album_image_url/ + musicbrainz_release_id). Existing tags are preserved — only art is added. + + Returns counts; never raises (unwritable/read-only files are skipped). + """ + result = {"embedded": 0, "failed": 0, "skipped": 0, "cover_written": False} + symbols = get_mutagen_symbols() + paths = [p for p in (file_paths or []) if p] + if not symbols: + return result + + for fp in paths: + if not os.path.isfile(fp): + result["skipped"] += 1 + continue + try: + audio = symbols.File(fp) + if audio is None: + result["skipped"] += 1 + continue + # Purely additive: never touch a file that already has art. Embedding + # again would APPEND a duplicate picture on FLAC (add_picture doesn't + # replace), so leave already-arted files alone. + if _audio_has_art(audio, symbols): + result["skipped"] += 1 + continue + # ID3 needs a tag container before APIC can be added. + if getattr(audio, "tags", None) is None and hasattr(audio, "add_tags"): + with contextlib.suppress(Exception): + audio.add_tags() + if embed_album_art_metadata(audio, metadata): + audio.save() + result["embedded"] += 1 + else: + result["failed"] += 1 + except Exception as exc: + # Read-only mounts / permission errors land here — skip, don't crash. + logger.warning("Could not embed art into %s: %s", fp, exc) + result["failed"] += 1 + + target_dir = folder or (os.path.dirname(paths[0]) if paths else None) + if target_dir and os.path.isdir(target_dir): + try: + download_cover_art(album_info, target_dir, context) + result["cover_written"] = folder_has_cover_sidecar(target_dir) + except Exception as exc: + logger.warning("cover.jpg write failed for %s: %s", target_dir, exc) + return result diff --git a/core/metadata/art_preservation.py b/core/metadata/art_preservation.py new file mode 100644 index 00000000..337cf7f6 --- /dev/null +++ b/core/metadata/art_preservation.py @@ -0,0 +1,115 @@ +"""Preserve embedded cover art across the metadata-enrichment rewrite. + +Issue #764 (continuation of #755): imported files lost their album art. +``enhance_file_metadata`` rebuilds tags from scratch — for FLAC it calls +``clear_pictures()`` and for MP3/MP4 it clears the whole tag block — *before* +it has the replacement art in hand. It then saves the file regardless of +whether new art was actually embedded. So every failure mode downstream +destroyed the art that shipped with the download: + + - source-metadata extraction returns nothing -> early save, no embed + - no album-art URL available / art download fails -> embed returns early + - art rejected by the min-resolution guard -> embed returns early + - art embedding disabled in config -> embed skipped entirely + +In all of those the file was saved with the pictures already cleared and +nothing put back. This module captures the existing art up front (live +mutagen objects, so they re-apply verbatim) and restores it right before a +save *iff the file currently has none* — so the happy path (new art embedded) +is byte-for-byte unchanged, and the only behaviour change is that we never +end up with less art than we started with. + +Scope mirrors ``embed_album_art_metadata``: FLAC ``Picture`` blocks, ID3 +``APIC`` frames, MP4 ``covr`` atoms. OggOpus/OggVorbis store art inside the +Vorbis comment (no ``clear_pictures``), so the enrichment rewrite never +strips it and it needs no preservation here. +""" + +from __future__ import annotations + +from typing import Any, List, Tuple + +from utils.logging_config import get_logger + +logger = get_logger("metadata.art_preservation") + +# Each snapshot entry is (kind, payload) where payload is a list of live +# mutagen objects captured before the tag rewrite. +ArtSnapshot = List[Tuple[str, list]] + + +def has_embedded_art(audio_file: Any, symbols: Any) -> bool: + """True iff ``audio_file`` currently carries embedded cover art in any + of the formats the enricher manages (FLAC pictures / ID3 APIC / MP4 covr).""" + try: + if getattr(audio_file, "pictures", None): + return True + tags = getattr(audio_file, "tags", None) + if tags is not None and isinstance(tags, symbols.ID3) and tags.getall("APIC"): + return True + if isinstance(audio_file, symbols.MP4) and tags and tags.get("covr"): + return True + except Exception as exc: # defensive: never let art-detection break a save + logger.debug("has_embedded_art check failed: %s", exc) + return False + + +def snapshot_embedded_art(audio_file: Any, symbols: Any) -> ArtSnapshot: + """Capture existing embedded art so it can be restored if re-embedding + fails. Returns a list of ``(kind, [objects])`` entries, or ``[]`` when the + file has no art. Captures the live mutagen objects (Picture / APIC frame / + MP4Cover) so they re-apply exactly as they were. + + Must be called BEFORE ``clear_pictures()`` / ``tags.clear()``.""" + snap: ArtSnapshot = [] + try: + pictures = getattr(audio_file, "pictures", None) + if pictures: + snap.append(("flac", list(pictures))) + tags = getattr(audio_file, "tags", None) + if tags is not None and isinstance(tags, symbols.ID3): + apics = tags.getall("APIC") + if apics: + snap.append(("id3", list(apics))) + if isinstance(audio_file, symbols.MP4) and tags: + covr = tags.get("covr") + if covr: + snap.append(("mp4", list(covr))) + except Exception as exc: + logger.debug("snapshot_embedded_art failed: %s", exc) + return snap + + +def restore_embedded_art(audio_file: Any, symbols: Any, snapshot: ArtSnapshot) -> bool: + """Re-apply captured art IFF the file currently has none. Returns True if + it restored something. + + No-op (returns False) when the snapshot is empty or the file already has + art — so calling this before a save never overwrites freshly-embedded art, + it only puts back what the rewrite would otherwise have destroyed.""" + if not snapshot or has_embedded_art(audio_file, symbols): + return False + restored = False + for kind, payload in snapshot: + try: + if kind == "flac" and hasattr(audio_file, "add_picture"): + for pic in payload: + audio_file.add_picture(pic) + restored = True + elif kind == "id3": + tags = getattr(audio_file, "tags", None) + if tags is not None: + for frame in payload: + tags.add(frame) + restored = True + elif kind == "mp4": + audio_file["covr"] = payload + restored = True + except Exception as exc: + logger.debug("restore_embedded_art (%s) failed: %s", kind, exc) + if restored: + logger.info("Preserved existing embedded cover art (re-embed produced none).") + return restored + + +__all__ = ["has_embedded_art", "snapshot_embedded_art", "restore_embedded_art", "ArtSnapshot"] diff --git a/core/metadata/artwork.py b/core/metadata/artwork.py index 9b8386af..a3c38c98 100644 --- a/core/metadata/artwork.py +++ b/core/metadata/artwork.py @@ -363,7 +363,7 @@ def embed_album_art_metadata(audio_file, metadata: dict): cfg = get_config_manager() symbols = get_mutagen_symbols() if not symbols: - return + return False try: image_data = None @@ -410,12 +410,12 @@ def embed_album_art_metadata(audio_file, metadata: dict): art_url = metadata.get("album_art_url") if not art_url: logger.warning("No album art URL available for embedding.") - return + return False image_data, mime_type = _fetch_art_bytes(art_url) if not image_data: logger.error("Failed to download album art data.") - return + return False if isinstance(audio_file.tags, symbols.ID3): audio_file.tags.add(symbols.APIC(encoding=3, mime=mime_type, type=3, desc="Cover", data=image_data)) @@ -434,8 +434,10 @@ def embed_album_art_metadata(audio_file, metadata: dict): audio_file["covr"] = [symbols.MP4Cover(image_data, imageformat=fmt)] logger.info("Album art successfully embedded.") + return True except Exception as exc: logger.error("Error embedding album art: %s", exc) + return False def download_cover_art(album_info: dict, target_dir: str, context: dict = None): diff --git a/core/metadata/canonical_resolver.py b/core/metadata/canonical_resolver.py new file mode 100644 index 00000000..a18b4b8b --- /dev/null +++ b/core/metadata/canonical_resolver.py @@ -0,0 +1,281 @@ +"""Resolve (and persist) the canonical release for an album — Stage 2 of #765. + +Stage 1 gave us the pure scorer (``core.metadata.canonical_version``). This +module turns it into an end-to-end resolver: gather the album's candidate +releases (one per metadata-source ID it has), score each against the on-disk +files, and return the best fit. Wiring (backfill job / enrichment hook) and the +DB store live alongside; the decision logic here is kept dependency-injected +(``fetch_tracklist`` is passed in) so it's fully unit-testable without live APIs +or real files. + +Still NO consumer reads the result in Stage 2 — populating the columns is +behavior-neutral. Stages 3-4 wire the Reorganizer and Track Number Repair to +read it. +""" + +from __future__ import annotations + +from typing import Any, Callable, Dict, List, Optional + +from core.metadata.canonical_version import ( + score_release_against_files, + score_release_detail, +) + +# Source-selection modes (a per-job setting). See resolve_canonical_for_album. +MODE_ACTIVE_PREFERRED = "active_preferred" # default: use the active source if it fits, else best-fit +MODE_ACTIVE_ONLY = "active_only" # only ever the active source +MODE_BEST_FIT = "best_fit" # whichever source fits the files best +VALID_MODES = (MODE_ACTIVE_PREFERRED, MODE_ACTIVE_ONLY, MODE_BEST_FIT) + + +def resolve_canonical_for_album( + *, + album_source_ids: Dict[str, str], + file_tracks: List[Dict[str, Any]], + fetch_tracklist: Callable[[str, str], Optional[List[Dict[str, Any]]]], + source_priority: List[str], + min_score: float = 0.5, + mode: str = MODE_ACTIVE_PREFERRED, + primary_source: Optional[str] = None, +) -> Optional[Dict[str, Any]]: + """Pick the canonical release for one album, honoring the source-selection mode. + + ``album_source_ids``: ``{source: album_id}`` the album is linked to. + ``file_tracks``: on-disk track metadata (``{duration_ms, title}``). + ``fetch_tracklist(source, album_id)``: returns that release's tracklist (or + None/[] on miss); injected so callers supply ``get_album_tracks_for_source`` + while tests supply a fake. + ``source_priority``: source order; ties break toward the earlier source. + ``primary_source``: the user's active metadata source (defaults to the first + of ``source_priority``). + + Modes: + - ``active_preferred`` (default): use the active source's release when the + album has an ID for it AND it clears ``min_score``; otherwise fall back + to the best-fit among the remaining sources. So it normally respects the + user's configured source but self-heals when that link is clearly wrong. + - ``active_only``: only ever the active source (pinned if it clears the + floor; never considers other sources). + - ``best_fit``: whichever source's release best matches the files. + + Returns an enriched dict for the chosen release — ``source``, ``album_id``, + ``score``, the per-signal breakdown (``count_fit``/``duration_fit``/ + ``title_fit``), ``file_track_count`` vs ``release_track_count``, and a + ``candidates`` list of everything it scored (so a finding can show WHY the + pick won and what it beat). ``None`` when there are no files, no resolvable + candidates, or nothing clears ``min_score``.""" + if not file_tracks: + return None + primary = primary_source or (source_priority[0] if source_priority else None) + scored: List[Dict[str, Any]] = [] # every source we actually scored + + def _score(source: Optional[str]) -> Optional[Dict[str, Any]]: + if not source or any(e['source'] == source for e in scored): + return next((e for e in scored if e['source'] == source), None) + album_id = album_source_ids.get(source) + if not album_id: + return None + try: + tracks = fetch_tracklist(source, str(album_id)) + except Exception: + tracks = None + if not tracks: + return None + entry = { + 'source': source, 'album_id': str(album_id), + 'track_count': len(tracks), 'score': round(score_release_against_files(file_tracks, tracks), 4), + '_tracks': tracks, + } + scored.append(entry) + return entry + + winner: Optional[Dict[str, Any]] = None + + # Active-source modes: try the primary first. + if mode in (MODE_ACTIVE_ONLY, MODE_ACTIVE_PREFERRED): + p = _score(primary) + if p and p['score'] >= min_score: + winner = p + elif mode == MODE_ACTIVE_ONLY: + return None # never consider other sources + + # best_fit, or active_preferred fallback: score the rest and pick the best. + if winner is None: + for source in source_priority: + _score(source) + best = None + for e in scored: # source_priority order -> strictly-greater = priority tiebreak + if best is None or e['score'] > best['score'] + 1e-9: + best = e + if best and best['score'] >= min_score: + winner = best + + if winner is None: + return None + + detail = score_release_detail(file_tracks, winner['_tracks']) + # Pinned-release track titles — already fetched, so free. Capped so a giant + # box set can't bloat the finding's details_json. + release_titles = [ + (t.get('title') or t.get('name') or '') for t in winner['_tracks'] + ][:60] + return { + 'source': winner['source'], + 'album_id': winner['album_id'], + 'score': winner['score'], + 'file_track_count': detail['file_track_count'], + 'release_track_count': detail['release_track_count'], + 'count_fit': detail['count_fit'], + 'duration_fit': detail['duration_fit'], + 'title_fit': detail['title_fit'], + 'release_track_titles': release_titles, + 'candidates': [ + {'source': e['source'], 'album_id': e['album_id'], + 'track_count': e['track_count'], 'score': e['score']} + for e in scored + ], + } + + +def _item_get(item: Any, key: str, default: Any = None) -> Any: + """Read ``key`` from a track item that may be a dict or an object.""" + return item.get(key, default) if isinstance(item, dict) else getattr(item, key, default) + + +def default_fetch_tracklist(source: str, album_id: str) -> Optional[List[Dict[str, Any]]]: + """Production ``fetch_tracklist``: pull a release's tracklist from a metadata + source and normalise to ``{title, track_number, duration_ms}``. Duration is + best-effort (not every source exposes it); when absent the scorer just leans + on track-count + title. Returns None on any failure.""" + try: + from core.metadata_service import get_album_tracks_for_source + data = get_album_tracks_for_source(source, album_id) + except Exception: + return None + items = data if isinstance(data, list) else ( + (data.get('items') or data.get('tracks') or []) if isinstance(data, dict) else [] + ) + if isinstance(items, dict): # {'tracks': {'items': [...]}} + items = items.get('items') or [] + out: List[Dict[str, Any]] = [] + for it in items: + dur = _item_get(it, 'duration_ms') + if dur is None: + secs = _item_get(it, 'duration') # some sources give seconds + dur = int(secs * 1000) if isinstance(secs, (int, float)) and secs else None + out.append({ + 'title': _item_get(it, 'name') or _item_get(it, 'title') or '', + 'track_number': _item_get(it, 'track_number'), + 'duration_ms': dur, + }) + return out or None + + +def _lookup_artist_thumb(db, artist_id) -> Optional[str]: + """Best-effort artist thumb URL by id. Returns None on missing column / any + error (the artists table doesn't have thumb_url in every schema).""" + if not artist_id: + return None + conn = None + try: + conn = db._get_connection() + cursor = conn.cursor() + cursor.execute("PRAGMA table_info(artists)") + if 'thumb_url' not in {r[1] for r in cursor.fetchall()}: + return None + cursor.execute("SELECT thumb_url FROM artists WHERE id = ?", (str(artist_id),)) + row = cursor.fetchone() + return (row[0] or None) if row else None + except Exception: + return None + finally: + if conn: + conn.close() + + +def resolve_and_store_canonical_for_album( + db, + album_id, + *, + fetch_tracklist: Optional[Callable[[str, str], Any]] = None, + source_priority: Optional[List[str]] = None, + min_score: float = 0.5, + store: bool = True, + mode: str = MODE_ACTIVE_PREFERRED, +) -> Optional[Dict[str, Any]]: + """Gather an album's source IDs + its tracks' (duration, title) from the DB, + resolve the best-fit canonical release, and (when ``store``) persist it. + Returns the resolved ``{source, album_id, score}`` or None when unresolved. + ``store=False`` resolves without writing — used by the backfill job's dry run. + + Uses the SAME album/source-id loader the Reorganizer uses + (``load_album_and_tracks`` + ``_extract_source_ids``) so the canonical is + chosen over exactly the source IDs the reorganizer sees. Scores off the DB + track rows' ``duration`` (stored in ms) + ``title`` — the library's view of + the files — so no per-file disk reads are needed.""" + from core.library_reorganize import _extract_source_ids, load_album_and_tracks + + album_data, tracks = load_album_and_tracks(db, album_id) + if not album_data or not tracks: + return None + source_ids = {s: v for s, v in _extract_source_ids(album_data).items() if v} + if not source_ids: + return None + + file_tracks = [ + {'duration_ms': t.get('duration') or 0, 'title': t.get('title') or ''} + for t in tracks + ] + + if fetch_tracklist is None: + fetch_tracklist = default_fetch_tracklist + primary_source = None + if source_priority is None: + try: + from core.metadata_service import get_primary_source, get_source_priority + primary_source = get_primary_source() + source_priority = get_source_priority(primary_source) + except Exception: + source_priority = list(source_ids.keys()) + + result = resolve_canonical_for_album( + album_source_ids=source_ids, + file_tracks=file_tracks, + fetch_tracklist=fetch_tracklist, + source_priority=source_priority, + min_score=min_score, + mode=mode, + primary_source=primary_source, + ) + if result: + # Album/artist/art context for richer findings (read from the row we + # already loaded — no extra query). Storage only uses source/id/score. + result['album_title'] = album_data.get('title') or '' + result['artist_name'] = album_data.get('artist_name') or '' + # Free context off the album row + the data we already gathered. + if album_data.get('year'): + result['year'] = album_data['year'] + result['db_track_count'] = album_data.get('track_count') or len(file_tracks) + if album_data.get('duration'): + result['db_duration_ms'] = album_data['duration'] + result['linked_sources'] = source_ids # {source: album_id} the album points at now + result['file_track_titles'] = [ft.get('title') or '' for ft in file_tracks][:60] + if album_data.get('thumb_url'): + result['album_thumb_url'] = album_data['thumb_url'] + # Artist thumb via a guarded lookup (not the shared album loader — some + # schemas have no artists.thumb_url column). Only runs for resolved + # albums, so no cost on the no-source-id short-circuit majority. + artist_thumb = _lookup_artist_thumb(db, album_data.get('artist_id')) + if artist_thumb: + result['artist_thumb_url'] = artist_thumb + if store: + db.set_album_canonical(album_id, result['source'], result['album_id'], result['score']) + return result + + +__all__ = [ + "resolve_canonical_for_album", + "resolve_and_store_canonical_for_album", + "default_fetch_tracklist", +] diff --git a/core/metadata/canonical_version.py b/core/metadata/canonical_version.py new file mode 100644 index 00000000..ccc07451 --- /dev/null +++ b/core/metadata/canonical_version.py @@ -0,0 +1,210 @@ +"""Pick the canonical album release by best-fit to the user's actual files. + +Issue #765 / #767-Bug2: SoulSync never pins ONE canonical album version per +album, so the Library Reorganizer, Track Number Repair, and tagging each +re-resolve independently and can land on different releases (standard vs +deluxe; Spotify vs MusicBrainz track numbering) and contradict each other. + +This module is the pure, testable heart of the fix: given the metadata of the +files actually on disk and a set of candidate releases, score each release by +how well it FITS those files and pick the best. "Best-fit to the files" means: + + - track-count fit — a 17-track deluxe is a poor fit for 11 files on disk + - duration alignment — each file should line up with a release track by length + - title overlap — a tiebreaker / sanity check + +What this does and does NOT solve: + - It DOES pick the right EDITION (standard vs deluxe) — the discriminating + signal is track count + durations. + - It does NOT (and cannot) decide which of two listings of the SAME album is + "more correct" when they differ only in track numbering (same files match + both equally). Instead ``pick_canonical_release`` is DETERMINISTIC and + breaks ties toward the earlier candidate — so the caller passes candidates + in source-priority order and every tool that reads the pinned result agrees + on the same release. Agreement is what resolves #765, not picking a + "winner" of the numbering disagreement. + +Pure, no I/O. Callers fetch candidate tracklists and read on-disk file metadata; +this module only scores. +""" + +from __future__ import annotations + +import re +from difflib import SequenceMatcher +from typing import Any, Dict, List, Optional, Tuple + +# Weights for the three fit signals. Count + duration dominate because "matches +# my files" is fundamentally about having the right NUMBER of the right-LENGTH +# tracks; title is a tiebreaker. Missing signals are dropped and the present +# ones renormalized (see _combine). +_W_COUNT = 0.4 +_W_DURATION = 0.4 +_W_TITLE = 0.2 + +_DEFAULT_DURATION_TOLERANCE_MS = 3000 # ±3s — covers encode/version length jitter +_DEFAULT_MIN_SCORE = 0.5 # never pin below this — leave unresolved +_TITLE_FUZZY_THRESHOLD = 0.85 + + +def _norm_title(text: str) -> str: + """Lowercase, drop bracketed qualifiers ((feat. …), [Remastered]), strip + punctuation, collapse whitespace.""" + if not text: + return "" + t = str(text).lower() + t = re.sub(r"[\(\[].*?[\)\]]", "", t) + t = re.sub(r"[^a-z0-9 ]", " ", t) + return " ".join(t.split()) + + +def _count_fit(n_files: int, n_release: int) -> float: + """1.0 when track counts match; decays with the relative difference.""" + if n_files <= 0 or n_release <= 0: + return 0.0 + return 1.0 - min(1.0, abs(n_files - n_release) / max(n_files, n_release)) + + +def _duration_fit( + file_tracks: List[Dict[str, Any]], + release_tracks: List[Dict[str, Any]], + tolerance_ms: int, +) -> Optional[float]: + """Fraction of tracks that line up by duration (greedy nearest match within + tolerance), over the larger of the two track counts — so missing or extra + tracks are penalised. Returns ``None`` when neither side has durations.""" + f_durs = [int(f["duration_ms"]) for f in file_tracks if f.get("duration_ms")] + r_durs = [int(r["duration_ms"]) for r in release_tracks if r.get("duration_ms")] + if not f_durs or not r_durs: + return None + used = [False] * len(r_durs) + matched = 0 + for fd in f_durs: + best_j, best_diff = -1, tolerance_ms + 1 + for j, rd in enumerate(r_durs): + if used[j]: + continue + diff = abs(fd - rd) + if diff <= tolerance_ms and diff < best_diff: + best_diff, best_j = diff, j + if best_j >= 0: + used[best_j] = True + matched += 1 + denom = max(len(file_tracks), len(release_tracks)) + return matched / denom if denom else 0.0 + + +def _title_fit( + file_tracks: List[Dict[str, Any]], + release_tracks: List[Dict[str, Any]], +) -> Optional[float]: + """Fraction of files whose title matches some release title (exact-normalised + or fuzzy), over the larger track count. ``None`` when titles are absent.""" + f_titles = [_norm_title(f.get("title", "")) for f in file_tracks] + f_titles = [t for t in f_titles if t] + r_titles = [_norm_title(r.get("title", "")) for r in release_tracks] + r_titles = [t for t in r_titles if t] + if not f_titles or not r_titles: + return None + r_set = set(r_titles) + matched = 0 + for ft in f_titles: + if ft in r_set or any( + SequenceMatcher(None, ft, rt).ratio() >= _TITLE_FUZZY_THRESHOLD + for rt in r_titles + ): + matched += 1 + denom = max(len(file_tracks), len(release_tracks)) + return matched / denom if denom else 0.0 + + +def _combine(parts: List[Tuple[Optional[float], float]]) -> float: + """Weighted mean over present (non-None) components, renormalising weights.""" + present = [(v, w) for v, w in parts if v is not None] + total_w = sum(w for _, w in present) + if total_w <= 0: + return 0.0 + return sum(v * w for v, w in present) / total_w + + +def score_release_against_files( + file_tracks: List[Dict[str, Any]], + release_tracks: List[Dict[str, Any]], + *, + duration_tolerance_ms: int = _DEFAULT_DURATION_TOLERANCE_MS, +) -> float: + """Score 0.0–1.0 of how well ``release_tracks`` fits the on-disk + ``file_tracks``. Each track dict may carry ``duration_ms`` and ``title``; + missing signals are dropped and the rest renormalised so the function never + crashes on sparse metadata (it just leans on what's available).""" + if not file_tracks or not release_tracks: + return 0.0 + count = _count_fit(len(file_tracks), len(release_tracks)) + dur = _duration_fit(file_tracks, release_tracks, duration_tolerance_ms) + title = _title_fit(file_tracks, release_tracks) + return _combine([(count, _W_COUNT), (dur, _W_DURATION), (title, _W_TITLE)]) + + +def score_release_detail( + file_tracks: List[Dict[str, Any]], + release_tracks: List[Dict[str, Any]], + *, + duration_tolerance_ms: int = _DEFAULT_DURATION_TOLERANCE_MS, +) -> Dict[str, Any]: + """Like ``score_release_against_files`` but returns the per-signal breakdown + so a UI can show WHY a release scored the way it did. ``duration_fit`` / + ``title_fit`` are ``None`` when that signal was absent.""" + if not file_tracks or not release_tracks: + return { + 'score': 0.0, 'count_fit': 0.0, 'duration_fit': None, 'title_fit': None, + 'release_track_count': len(release_tracks), 'file_track_count': len(file_tracks), + } + count = _count_fit(len(file_tracks), len(release_tracks)) + dur = _duration_fit(file_tracks, release_tracks, duration_tolerance_ms) + title = _title_fit(file_tracks, release_tracks) + score = _combine([(count, _W_COUNT), (dur, _W_DURATION), (title, _W_TITLE)]) + return { + 'score': round(score, 4), + 'count_fit': round(count, 3), + 'duration_fit': round(dur, 3) if dur is not None else None, + 'title_fit': round(title, 3) if title is not None else None, + 'release_track_count': len(release_tracks), + 'file_track_count': len(file_tracks), + } + + +def pick_canonical_release( + file_tracks: List[Dict[str, Any]], + candidates: List[Dict[str, Any]], + *, + min_score: float = _DEFAULT_MIN_SCORE, + duration_tolerance_ms: int = _DEFAULT_DURATION_TOLERANCE_MS, +) -> Tuple[Optional[Dict[str, Any]], float]: + """Choose the best-fit candidate release for the on-disk files. + + ``candidates`` is a list of dicts each with a ``'tracks'`` list (plus any + caller fields like ``source``/``album_id``, returned untouched). **Pass + candidates in source-priority order** — ties break toward the EARLIER one, + so the choice is deterministic and priority-respecting (this is what makes + every tool agree, #765). + + Returns ``(best_candidate, score)``, or ``(None, best_score)`` when nothing + clears ``min_score`` — so a low-confidence guess is never pinned (the caller + leaves the album unresolved and falls back to today's behaviour).""" + best: Optional[Dict[str, Any]] = None + best_score = 0.0 + for cand in candidates: + score = score_release_against_files( + file_tracks, cand.get("tracks") or [], + duration_tolerance_ms=duration_tolerance_ms, + ) + # Strictly-greater so equal scores keep the earlier (higher-priority) + # candidate — deterministic tiebreak. + if score > best_score + 1e-9: + best, best_score = cand, score + if best is None or best_score < min_score: + return None, best_score + return best, best_score + + +__all__ = ["score_release_against_files", "pick_canonical_release"] diff --git a/core/metadata/enrichment.py b/core/metadata/enrichment.py index 43e668e8..fdd34042 100644 --- a/core/metadata/enrichment.py +++ b/core/metadata/enrichment.py @@ -6,6 +6,10 @@ import os from types import SimpleNamespace from typing import Any +from core.metadata.art_preservation import ( + restore_embedded_art, + snapshot_embedded_art, +) from core.metadata.artwork import embed_album_art_metadata from core.metadata.common import ( get_config_manager, @@ -76,6 +80,8 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf file_lock = get_file_lock(file_path) with file_lock: logger.info("Enhancing metadata for: %s", os.path.basename(file_path)) + art_snapshot = [] # defined up front so the except handler can restore + audio_file = None try: strip_all_non_audio_tags(file_path) audio_file = symbols.File(file_path) @@ -83,6 +89,13 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf logger.error("Could not load audio file with Mutagen: %s", file_path) return False + # Capture any embedded cover art BEFORE we clear it. The rewrite + # below clears pictures/tags up front, but new art isn't fetched + # until much later (and may fail / be unavailable / be disabled). + # Without this snapshot, every such failure saves the file with + # the art destroyed and nothing put back — issue #764. + art_snapshot = snapshot_embedded_art(audio_file, symbols) + if hasattr(audio_file, "clear_pictures"): audio_file.clear_pictures() @@ -99,6 +112,9 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf metadata = extract_source_metadata(context, artist, album_info) if not metadata: logger.error("Could not extract source metadata, saving with cleared tags.") + # Don't destroy the original art just because we couldn't + # enrich the tags — put it back before saving. + restore_embedded_art(audio_file, symbols, art_snapshot) save_audio_file(audio_file, symbols) return True @@ -202,6 +218,13 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf elif isinstance(audio_file, symbols.MP4): audio_file["----:com.apple.iTunes:QUALITY"] = [symbols.MP4FreeForm(quality.encode("utf-8"))] + # If art embedding was skipped/disabled or produced nothing (no + # URL, download failed, rejected by the min-resolution guard), + # the file still has the pictures we cleared above. Restore the + # original so import never strips existing art (#764). No-op when + # new art was embedded — that path is unchanged. + restore_embedded_art(audio_file, symbols, art_snapshot) + save_audio_file(audio_file, symbols) verified = verify_metadata_written(file_path) @@ -219,4 +242,18 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf logger.warning("[Metadata Debug] Artist: %s", artist.get("name", "MISSING") if artist else "None") logger.warning("[Metadata Debug] Album info: %s", album_info.get("album_name", "MISSING") if album_info else "None") logger.error("[Metadata Debug] Traceback:\n%s", traceback.format_exc()) + # We cleared the file's art early; if the rewrite then crashed + # before re-embedding, the on-disk file (already saved cleared at + # the start) would be left art-less. Best-effort: put the original + # art back and persist it so a mid-enrichment crash never destroys + # the cover (#764). Guarded so a failure here can't mask the + # original error. + try: + if audio_file is not None and art_snapshot and restore_embedded_art( + audio_file, symbols, art_snapshot + ): + save_audio_file(audio_file, symbols) + logger.info("Restored original cover art after enrichment error.") + except Exception as restore_exc: + logger.debug("Art restore after error failed: %s", restore_exc) return False diff --git a/core/metadata/similar_artists.py b/core/metadata/similar_artists.py index 95d70381..9fbefee0 100644 --- a/core/metadata/similar_artists.py +++ b/core/metadata/similar_artists.py @@ -269,10 +269,17 @@ def iter_musicmap_similar_artist_events( except requests.exceptions.RequestException as exc: logger.debug("Error fetching MusicMap for %s: %s", artist_name, exc) + # A 404 from MusicMap means the artist simply has no map page — that's a + # not-found, not a fetch failure. Surface the real HTTP status (when the + # exception carries a response) so callers classify it correctly instead + # of flattening every network-layer error to a 502 "error". Timeouts / + # connection errors carry no response → fall back to 502 (still an error). + resp = getattr(exc, 'response', None) + status_code = getattr(resp, 'status_code', None) or 502 yield { 'type': 'error', 'error': f'Failed to fetch from MusicMap: {exc}', - 'status_code': 502, + 'status_code': status_code, } except ValueError as exc: status_code = 404 if 'Could not find artist map on MusicMap' in str(exc) else 400 diff --git a/core/musicbrainz_worker.py b/core/musicbrainz_worker.py index bc494433..4f3adf55 100644 --- a/core/musicbrainz_worker.py +++ b/core/musicbrainz_worker.py @@ -166,6 +166,16 @@ class MusicBrainzWorker: conn = self.db._get_connection() cursor = conn.cursor() + # Pinned-group override (Manage Enrichment Workers): process one + # entity type first, then fall through to the normal chain. Unset or + # exhausted ⇒ default artist→album→track order, unchanged. + from core.worker_utils import read_enrichment_priority, priority_pending_item + _prio = read_enrichment_priority('musicbrainz') + if _prio: + _pi = priority_pending_item(cursor, 'musicbrainz', _prio) + if _pi: + return _pi + # Priority 1: Unattempted artists cursor.execute(""" SELECT id, name diff --git a/core/navidrome_client.py b/core/navidrome_client.py index 7ec9d286..765153bc 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -3,6 +3,7 @@ import hashlib import secrets from typing import List, Optional, Dict, Any from datetime import datetime +from urllib.parse import urlencode import json from utils.logging_config import get_logger from config.settings import config_manager @@ -316,6 +317,42 @@ class NavidromeClient(MediaServerClient): 'f': 'json' # Response format } + # Fixed salt for cover-art URLs ONLY. Subsonic token auth (t=md5(password + # +salt), s=salt) does not require a unique salt per request, and a stable + # one makes the cover URL deterministic — so the image cache and the + # browser cache actually HIT. The rotating salt from _generate_auth_params + # would make every request a unique URL → cache miss every time + a dead, + # never-reused cache row per fetch (#766 review). The password is never + # exposed either way (only its salted md5). + _COVER_ART_SALT = 'soulsync-cover' + + def build_cover_art_url(self, cover_id, size=None) -> Optional[str]: + """Absolute, Subsonic-authenticated getCoverArt URL for ``cover_id``. + + Deterministic for a given (server, password, cover_id) so it caches. + The web layer proxies this to the browser (sync editor + modals). + Returns ``None`` when not connected or no id was supplied. #766: the + ``/api/navidrome/cover/`` route had no working URL behind it, so + every Navidrome cover came back blank.""" + if not self.base_url or not cover_id: + return None + if not self.username or not self.password: + return None + salt = self._COVER_ART_SALT + token = hashlib.md5((self.password + salt).encode()).hexdigest() + params = { + 'u': self.username, + 't': token, + 's': salt, + 'v': '1.16.1', + 'c': 'SoulSync', + 'f': 'json', # harmless for getCoverArt — it returns image binary + 'id': str(cover_id), + } + if size: + params['size'] = str(size) + return f"{self.base_url}/rest/getCoverArt?{urlencode(params)}" + # Subsonic endpoints that modify data — use POST to avoid URL length limits _WRITE_ENDPOINTS = frozenset({ 'createPlaylist', 'updatePlaylist', 'deletePlaylist', diff --git a/core/playlists/organize_download.py b/core/playlists/organize_download.py new file mode 100644 index 00000000..2fc8d9cb --- /dev/null +++ b/core/playlists/organize_download.py @@ -0,0 +1,182 @@ +"""Download missing tracks into playlist-folder layout for mirrored playlists.""" + +from __future__ import annotations + +import json +import logging +import uuid +from typing import Any, Callable, Dict, List, Optional + +logger = logging.getLogger(__name__) + + +def mirrored_tracks_to_download_json(tracks: List[Dict[str, Any]]) -> List[Dict[str, Any]]: + """Convert mirrored playlist rows to the payload expected by the download master.""" + out: List[Dict[str, Any]] = [] + for t in tracks: + extra = {} + if t.get('extra_data'): + try: + extra = json.loads(t['extra_data']) if isinstance(t['extra_data'], str) else t['extra_data'] + except (json.JSONDecodeError, TypeError): + extra = {} + + if extra.get('discovered') and extra.get('matched_data'): + md = extra['matched_data'] + album_raw = md.get('album', '') + album_obj = album_raw if isinstance(album_raw, dict) else {'name': album_raw or ''} + entry = { + 'name': md.get('name', ''), + 'artists': md.get('artists', [{'name': t.get('artist_name', '')}]), + 'album': album_obj, + 'duration_ms': md.get('duration_ms', 0), + 'id': md.get('id', ''), + } + if md.get('track_number'): + entry['track_number'] = md['track_number'] + if md.get('disc_number'): + entry['disc_number'] = md['disc_number'] + out.append(entry) + continue + + hint = extra.get('spotify_hint', {}) + track_image = (t.get('image_url') or '').strip() + album_obj = { + 'name': (t.get('album_name') or '').strip(), + 'images': [{'url': track_image, 'height': 300, 'width': 300}] if track_image else [], + } + + if hint.get('id') and hint.get('name'): + hint_artists = hint.get('artists', []) + if hint_artists and isinstance(hint_artists[0], str): + hint_artists = [{'name': a} for a in hint_artists] + elif not hint_artists: + hint_artists = [{'name': t.get('artist_name', '')}] + out.append({ + 'name': hint['name'], + 'artists': hint_artists, + 'album': album_obj, + 'duration_ms': t.get('duration_ms', 0), + 'id': hint['id'], + }) + elif t.get('source_track_id') and (t.get('track_name') or '').strip(): + out.append({ + 'name': t['track_name'].strip(), + 'artists': [{'name': (t.get('artist_name') or '').strip() or 'Unknown Artist'}], + 'album': album_obj, + 'duration_ms': t.get('duration_ms', 0), + 'id': t['source_track_id'], + }) + return out + + +def run_playlist_organize_download( + deps: Any, + *, + mirrored_playlist_id: int, + profile_id: int = 1, + get_batch_max_concurrent: Callable[[bool], int], + run_full_missing_tracks_process: Callable[..., Any], + record_sync_history_start: Optional[Callable[..., Any]] = None, + detect_sync_source: Optional[Callable[[str], str]] = None, +) -> Dict[str, Any]: + """Queue a playlist-folder missing-tracks batch for one mirrored playlist.""" + db = deps.get_database() + pl = db.get_mirrored_playlist(int(mirrored_playlist_id)) + if not pl: + return {'status': 'error', 'reason': 'Playlist not found'} + + source_playlist_ref = (pl.get('source_playlist_id') or '').strip() + source = (pl.get('source') or 'spotify').strip() or 'spotify' + + tracks = db.get_mirrored_playlist_tracks(int(mirrored_playlist_id)) + tracks_json = mirrored_tracks_to_download_json(tracks) + if not tracks_json: + return {'status': 'skipped', 'reason': 'No processable tracks'} + + batch_id = str(uuid.uuid4()) + playlist_id = str(mirrored_playlist_id) + playlist_name = pl.get('name', 'Unknown Playlist') + + download_batches = deps.get_download_batches() + tasks_lock = deps.tasks_lock + + with tasks_lock: + active_analysis = sum( + 1 for batch in download_batches.values() if batch.get('phase') == 'analysis' + ) + if active_analysis >= 3: + return {'status': 'error', 'reason': 'Too many analysis processes running'} + + download_batches[batch_id] = { + 'phase': 'analysis', + 'playlist_id': playlist_id, + 'playlist_name': playlist_name, + 'queue': [], + 'active_count': 0, + 'max_concurrent': get_batch_max_concurrent(False), + 'permanently_failed_tracks': [], + 'cancelled_tracks': set(), + 'queue_index': 0, + 'analysis_total': len(tracks_json), + 'profile_id': profile_id, + 'analysis_processed': 0, + 'analysis_results': [], + 'force_download_all': False, + 'ignore_manual_matches': False, + 'playlist_folder_mode': True, + 'is_album_download': False, + 'album_context': None, + 'artist_context': None, + 'wing_it': False, + 'batch_source': source, + 'auto_initiated': False, + 'organize_by_playlist': True, + 'source_playlist_ref': source_playlist_ref, + 'mirrored_playlist_id': int(mirrored_playlist_id), + } + + if record_sync_history_start: + try: + record_sync_history_start( + batch_id, + playlist_id, + playlist_name, + tracks_json, + False, + None, + None, + True, + source_page='automation', + ) + except Exception as hist_err: + logger.debug("organize download sync history: %s", hist_err) + + try: + deps.missing_download_executor.submit( + run_full_missing_tracks_process, + batch_id, + playlist_id, + tracks_json, + ) + except Exception as submit_err: + # Don't leave the batch stranded in 'analysis' holding one of the limited + # analysis slots if the executor refuses the job. + logger.error("[Organize Download] Failed to submit batch %s: %s", batch_id, submit_err) + with tasks_lock: + download_batches.pop(batch_id, None) + return {'status': 'error', 'reason': f'submit failed: {submit_err}'} + logger.info( + "[Organize Download] Started batch %s for mirrored playlist %s (%s tracks)", + batch_id, + playlist_name, + len(tracks_json), + ) + return { + 'status': 'started', + 'batch_id': batch_id, + 'track_count': len(tracks_json), + } + + +__all__ = ['mirrored_tracks_to_download_json', 'run_playlist_organize_download'] diff --git a/core/qobuz_worker.py b/core/qobuz_worker.py index c6ba2be7..706d6295 100644 --- a/core/qobuz_worker.py +++ b/core/qobuz_worker.py @@ -186,6 +186,16 @@ class QobuzWorker: conn = self.db._get_connection() cursor = conn.cursor() + # Pinned-group override (Manage Enrichment Workers): process one + # entity type first, then fall through to the normal chain. Unset or + # exhausted ⇒ default artist→album→track order, unchanged. + from core.worker_utils import read_enrichment_priority, priority_pending_item + _prio = read_enrichment_priority('qobuz') + if _prio: + _pi = priority_pending_item(cursor, 'qobuz', _prio) + if _pi: + return _pi + # Priority 1: Unattempted artists cursor.execute(""" SELECT id, name diff --git a/core/repair_jobs/__init__.py b/core/repair_jobs/__init__.py index c6f780e8..40fd7a04 100644 --- a/core/repair_jobs/__init__.py +++ b/core/repair_jobs/__init__.py @@ -44,6 +44,7 @@ _JOB_MODULES = [ 'core.repair_jobs.live_commentary_cleaner', 'core.repair_jobs.unknown_artist_fixer', 'core.repair_jobs.discography_backfill', + 'core.repair_jobs.canonical_version_resolve', ] diff --git a/core/repair_jobs/base.py b/core/repair_jobs/base.py index 4ac7f52b..d0564545 100644 --- a/core/repair_jobs/base.py +++ b/core/repair_jobs/base.py @@ -100,6 +100,9 @@ class RepairJob(ABC): default_enabled: bool = False default_interval_hours: int = 24 default_settings: Dict[str, Any] = {} + # Optional {setting_key: [allowed values]} — the UI renders a dropdown for + # these instead of a free-text box. Keys not listed render by value type. + setting_options: Dict[str, list] = {} auto_fix: bool = False @abstractmethod diff --git a/core/repair_jobs/canonical_version_resolve.py b/core/repair_jobs/canonical_version_resolve.py new file mode 100644 index 00000000..f7fd2f00 --- /dev/null +++ b/core/repair_jobs/canonical_version_resolve.py @@ -0,0 +1,221 @@ +"""Resolve Canonical Album Versions — backfill job (#765 Stage 2 trigger). + +Pins each album's canonical release (best-fit to its files) so the Library +Reorganizer (Stage 3) and Track Number Repair (Stage 4) resolve the SAME +release and stop contradicting each other. The resolution logic lives in the +tested core.metadata.canonical_resolver; this job is the opt-in, rate-limited, +progress-reported bulk runner. + +Opt-in (``default_enabled = False``) because resolving compares an album's +candidate releases across sources, which costs metadata-source API calls — done +once per album, then stored. Albums that already have a canonical are skipped. +""" + +import os +from typing import Optional + +from core.metadata.canonical_resolver import resolve_and_store_canonical_for_album +from core.repair_jobs import register_job +from core.repair_jobs.base import JobContext, JobResult, RepairJob +from utils.logging_config import get_logger + +logger = get_logger("repair_job.canonical_version") + + +def _pct(v) -> str: + return f"{round(v * 100)}%" if isinstance(v, (int, float)) else "n/a" + + +def _describe_pin(resolved: dict) -> str: + """Human-readable, judge-able explanation of WHY this release was chosen.""" + artist = resolved.get('artist_name') or '' + album = resolved.get('album_title') or '' + head = f"{artist} — {album}".strip(" —") or resolved.get('album_id', '') + year = resolved.get('year') + if year: + head += f" ({year})" + lines = [ + f"{head}" if head else "", + f"Pin {resolved['source']} release {resolved['album_id']} " + f"(confidence {_pct(resolved.get('score'))}).", + f"Fit to your library: {resolved.get('file_track_count', '?')} files vs " + f"{resolved.get('release_track_count', '?')} tracks on this release — " + f"track count {_pct(resolved.get('count_fit'))}, " + f"durations {_pct(resolved.get('duration_fit'))}, " + f"titles {_pct(resolved.get('title_fit'))}.", + ] + + # What the album is currently linked to vs what we'd pin. + linked = resolved.get('linked_sources') or {} + if linked: + linked_str = ", ".join(f"{s}={i}" for s, i in linked.items()) + lines.append(f"Currently linked: {linked_str} → pinning {resolved['source']}.") + + others = [c for c in resolved.get('candidates', []) if c.get('source') != resolved.get('source')] + if others: + comp = ", ".join( + f"{c['source']} {_pct(c['score'])} ({c['track_count']} tk)" for c in others + ) + lines.append(f"Beat: {comp}.") + elif len(resolved.get('candidates', [])) == 1: + lines.append("Only this source had a release linked for this album.") + + # Track listing of the pinned release (so you can eyeball the actual songs). + titles = resolved.get('release_track_titles') or [] + if titles: + shown = "; ".join(f"{i+1}. {t}" for i, t in enumerate(titles[:25])) + more = f" (+{len(titles) - 25} more)" if len(titles) > 25 else "" + lines.append(f"Release tracks: {shown}{more}") + + return "\n".join(l for l in lines if l) + + +@register_job +class CanonicalVersionResolveJob(RepairJob): + job_id = 'canonical_version_resolve' + display_name = 'Resolve Canonical Album Versions' + description = ( + 'Pins the best-fit release per album (by track count + durations) so ' + 'reorganize and track-number repair agree (dry run by default)' + ) + help_text = ( + 'For each album, compares the releases its linked metadata sources point ' + 'at and pins the one that best matches the files you actually have ' + '(track count + durations + titles). The Library Reorganizer and Track ' + 'Number Repair then both use that pinned release, so they stop ' + 'contradicting each other (e.g. standard vs deluxe, or Spotify vs ' + 'MusicBrainz track numbering).\n\n' + 'In dry run mode (default) it reports what it would pin without saving. ' + 'Disable dry run to store the pins. Albums already pinned are skipped.\n\n' + 'Opt-in: resolving costs metadata-source API calls (once per album).' + ) + icon = 'repair-icon-tracknumber' + default_enabled = False + default_interval_hours = 168 # weekly, but disabled by default + default_settings = { + 'dry_run': True, + 'min_score': 0.5, + # Which source's release to pin: 'active_preferred' (default — use your + # active metadata source when it fits, else best-fit fallback), + # 'active_only' (only ever the active source), or 'best_fit' (whichever + # source matches the files best, regardless of which it is). + 'source_selection': 'active_preferred', + } + # Render source_selection as a dropdown (not a text box) in the settings UI. + setting_options = { + 'source_selection': ['active_preferred', 'active_only', 'best_fit'], + } + auto_fix = True + + def _get_settings(self, context: JobContext) -> dict: + merged = dict(self.default_settings) + if context.config_manager: + merged.update(context.config_manager.get(f'repair.jobs.{self.job_id}.settings', {}) or {}) + return merged + + def _load_album_ids(self, db, active_server: Optional[str]) -> list: + conn = None + try: + conn = db._get_connection() + cursor = conn.cursor() + if active_server: + cursor.execute( + "SELECT al.id, al.title FROM albums al WHERE al.server_source = ? ORDER BY al.id", + (active_server,), + ) + else: + cursor.execute("SELECT al.id, al.title FROM albums al ORDER BY al.id") + return [(row[0], row[1]) for row in cursor.fetchall()] + except Exception as e: + logger.error("Error loading albums for canonical resolve: %s", e) + return [] + finally: + if conn: + conn.close() + + def scan(self, context: JobContext) -> JobResult: + result = JobResult() + settings = self._get_settings(context) + dry_run = settings.get('dry_run', True) + min_score = settings.get('min_score', 0.5) + mode = settings.get('source_selection', 'active_preferred') + + active_server = None + if context.config_manager: + try: + active_server = context.config_manager.get_active_media_server() + except Exception as e: + logger.warning("Couldn't read active media server: %s", e) + + albums = self._load_album_ids(context.db, active_server) + total = len(albums) + if context.report_progress: + mode = 'DRY RUN' if dry_run else 'LIVE' + context.report_progress( + phase=f'Resolving canonical versions for {total} albums ({mode})...', + total=total, scanned=0, log_type='info', + ) + + for i, (album_id, album_title) in enumerate(albums): + if context.check_stop(): + return result + if i % 20 == 0 and context.wait_if_paused(): + return result + + # Skip albums already pinned — one-time cost per album. + try: + if context.db.get_album_canonical(album_id): + result.skipped += 1 + result.scanned += 1 + continue + except Exception: # noqa: S110 — best-effort skip check; on read error just resolve it + pass + + try: + resolved = resolve_and_store_canonical_for_album( + context.db, album_id, min_score=min_score, store=not dry_run, mode=mode, + ) + except Exception as e: + logger.warning("Canonical resolve failed for album %s ('%s'): %s", + album_id, album_title, e) + result.errors += 1 + result.scanned += 1 + continue + + result.scanned += 1 + if resolved: + if dry_run and context.create_finding: + artist = resolved.get('artist_name') or '' + label = f"{artist} — {album_title}" if artist else (album_title or str(album_id)) + inserted = context.create_finding( + job_id=self.job_id, + finding_type='canonical_version', + severity='info', + entity_type='album', + entity_id=str(album_id), + file_path=None, + title=f'Pin {resolved["source"]} as canonical: {label}', + description=_describe_pin(resolved), + details={'album_id': str(album_id), **resolved}, + ) + if inserted: + result.findings_created += 1 + else: + result.findings_skipped_dedup += 1 + elif not dry_run: + result.auto_fixed += 1 + + if context.report_progress and (i + 1) % 25 == 0: + context.report_progress(scanned=i + 1, total=total, + phase=f'Resolving ({i+1}/{total})...') + + return result + + def estimate_scope(self, context: JobContext) -> int: + active_server = None + if context.config_manager: + try: + active_server = context.config_manager.get_active_media_server() + except Exception: # noqa: S110 — best-effort; fall back to no server filter + pass + return len(self._load_album_ids(context.db, active_server)) diff --git a/core/repair_jobs/missing_cover_art.py b/core/repair_jobs/missing_cover_art.py index 4c292873..e6690981 100644 --- a/core/repair_jobs/missing_cover_art.py +++ b/core/repair_jobs/missing_cover_art.py @@ -1,5 +1,9 @@ """Missing Cover Art Filler Job — finds albums without artwork and locates art from APIs.""" +import os +import re + +from core.metadata.art_apply import album_has_art_on_disk from core.metadata_service import get_client_for_source, get_primary_source, get_source_priority from core.repair_jobs import register_job from core.repair_jobs.base import JobContext, JobResult, RepairJob @@ -7,6 +11,33 @@ from utils.logging_config import get_logger logger = get_logger("repair_job.cover_art") +# Stopwords dropped before comparing album/artist names so trivial words +# ("the", "and") don't make two different names look like a match. +_NAME_STOPWORDS = {'the', 'a', 'an', 'and', 'of', 'feat', 'ft', 'featuring'} + + +def _norm_name(value) -> str: + """Lowercase, strip bracketed qualifiers (Deluxe/Remaster/feat.) and + punctuation so names can be compared on their significant words.""" + s = (value or '').lower() + s = re.sub(r'[\(\[\{].*?[\)\]\}]', ' ', s) # drop (...) [...] qualifiers + s = re.sub(r'\b(?:feat|ft|featuring)\b.*', ' ', s) # drop trailing "feat. X" + s = re.sub(r'[^a-z0-9]+', ' ', s) + return ' '.join(s.split()) + + +def _name_tokens(value) -> set: + return set(_norm_name(value).split()) - _NAME_STOPWORDS + + +def _names_match(a, b) -> bool: + """True when two names share all the significant words of the shorter one + (so "Album" matches "Album (Deluxe)", but unrelated titles don't).""" + ta, tb = _name_tokens(a), _name_tokens(b) + if not ta or not tb: + return False + return ta <= tb or tb <= ta + @register_job class MissingCoverArtJob(RepairJob): @@ -59,6 +90,12 @@ class MissingCoverArtJob(RepairJob): "al.spotify_album_id", "al.thumb_url", "ar.thumb_url", + # A representative local track path, so we can check whether the + # album actually has art ON DISK (embedded / cover.jpg) — not just + # whether the DB row has a thumb_url. + ("(SELECT t.file_path FROM tracks t WHERE t.album_id = al.id " + "AND t.file_path IS NOT NULL AND t.file_path != '' " + "ORDER BY t.disc_number, t.track_number LIMIT 1) AS rep_path"), ] column_map = [ ("itunes_album_id", "al.itunes_album_id"), @@ -72,12 +109,14 @@ class MissingCoverArtJob(RepairJob): column_index[alias] = len(select_cols) select_cols.append(f"{column} AS {alias}") + # Scan every titled album — we decide per-album whether art is + # missing in the DB OR on disk (the file/cover.jpg). The on-disk + # check is cheap-first (a sidecar stat before opening any audio). cursor.execute(f""" SELECT {', '.join(select_cols)} FROM albums al LEFT JOIN artists ar ON ar.id = al.artist_id - WHERE (al.thumb_url IS NULL OR al.thumb_url = '') - AND al.title IS NOT NULL AND al.title != '' + WHERE al.title IS NOT NULL AND al.title != '' """) albums = cursor.fetchall() except Exception as e: @@ -103,7 +142,7 @@ class MissingCoverArtJob(RepairJob): if i % 10 == 0 and context.wait_if_paused(): return result - album_id, title, artist_name, spotify_album_id, _, artist_thumb = row[:6] + album_id, title, artist_name, spotify_album_id, album_thumb, artist_thumb, rep_path = row[:7] source_album_ids = { 'spotify': spotify_album_id, 'itunes': row[column_index['itunes_album_id']] if 'itunes_album_id' in column_index else None, @@ -113,6 +152,14 @@ class MissingCoverArtJob(RepairJob): } result.scanned += 1 + # Art can be missing in the DB (no thumb_url) and/or on disk (no + # embedded art and no cover.jpg). Skip albums that already have both. + db_missing = not (str(album_thumb).strip() if album_thumb else '') + disk_missing = bool(rep_path) and not album_has_art_on_disk(rep_path) + if not db_missing and not disk_missing: + result.skipped += 1 + continue + if context.report_progress: context.report_progress( scanned=i + 1, total=total, @@ -154,6 +201,12 @@ class MissingCoverArtJob(RepairJob): 'found_artwork_url': artwork_url, 'spotify_album_id': spotify_album_id, 'artist_thumb_url': artist_thumb or None, + # Where the files live + what was missing, so the + # apply can embed into the audio + write cover.jpg. + 'album_folder': os.path.dirname(rep_path) if rep_path else None, + 'db_missing': db_missing, + 'disk_missing': disk_missing, + 'musicbrainz_release_id': None, } ) if inserted: @@ -192,19 +245,70 @@ class MissingCoverArtJob(RepairJob): return artwork_url if query and hasattr(client, 'search_albums'): - results = client.search_albums(query, limit=1) - if results: - artwork_url = self._extract_artwork_url(results[0]) + # Pull a few results and only accept one whose title AND artist + # actually match this album. The old code grabbed results[0]'s + # artwork unconditionally, so a loose full-text search returning + # the wrong album gave the wrong cover. + results = client.search_albums(query, limit=5) or [] + for res in results: + if not self._result_matches(res, title, artist_name): + continue + artwork_url = self._extract_artwork_url(res) if artwork_url: return artwork_url - candidate_id = self._extract_album_id(results[0]) + candidate_id = self._extract_album_id(res) if candidate_id: album_data = self._get_album_for_source(source, client, candidate_id) - return self._extract_artwork_url(album_data) + artwork_url = self._extract_artwork_url(album_data) + if artwork_url: + return artwork_url except Exception as e: logger.debug("%s art lookup failed for '%s': %s", source.capitalize(), title, e) return None + @staticmethod + def _result_title_artist(item): + """Pull (title, artist) from a search result that may be a dict or an + Album-like object, across the various source clients.""" + if item is None: + return '', '' + if isinstance(item, dict): + title = item.get('title') or item.get('name') or item.get('album') or '' + artist = item.get('artist') or item.get('artist_name') or '' + if not artist: + artists = item.get('artists') or [] + if isinstance(artists, list) and artists: + a0 = artists[0] + artist = a0.get('name', '') if isinstance(a0, dict) else str(a0) + else: + title = getattr(item, 'title', None) or getattr(item, 'name', None) or getattr(item, 'album', None) or '' + artist = getattr(item, 'artist', None) or getattr(item, 'artist_name', None) or '' + if not artist: + arts = getattr(item, 'artists', None) or [] + if isinstance(arts, list) and arts: + a0 = arts[0] + artist = a0.get('name', '') if isinstance(a0, dict) else str(a0) + return str(title or ''), str(artist or '') + + @classmethod + def _result_matches(cls, result, album_title, album_artist) -> bool: + """Reject a search result unless it confidently matches the album. + + Title must match; if both the result and the album carry an artist, the + artist must match too (the strongest guard against wrong covers). When + the result has no artist to compare, require an exact title match. + """ + r_title, r_artist = cls._result_title_artist(result) + # Title may carry extra qualifiers (Deluxe/Remaster) → allow subset. + if not _names_match(r_title, album_title): + return False + # Artist is the strong guard, so require its significant words to match + # EXACTLY (not subset) — "Different Artist" must NOT match "Artist". + if r_artist and album_artist: + return _name_tokens(r_artist) == _name_tokens(album_artist) + # No artist on the result → require an exact title match instead. + return _norm_name(r_title) == _norm_name(album_title) + @staticmethod def _get_album_for_source(source, client, album_id): if source == 'spotify': @@ -248,10 +352,11 @@ class MissingCoverArtJob(RepairJob): try: conn = context.db._get_connection() cursor = conn.cursor() + # Upper bound: every titled album is examined (the per-album DB/disk + # art check decides which actually need filling). cursor.execute(""" SELECT COUNT(*) FROM albums - WHERE (thumb_url IS NULL OR thumb_url = '') - AND title IS NOT NULL AND title != '' + WHERE title IS NOT NULL AND title != '' """) row = cursor.fetchone() return row[0] if row else 0 diff --git a/core/repair_jobs/track_number_repair.py b/core/repair_jobs/track_number_repair.py index 8a569de6..a7c4292a 100644 --- a/core/repair_jobs/track_number_repair.py +++ b/core/repair_jobs/track_number_repair.py @@ -275,6 +275,22 @@ class TrackNumberRepairJob(RepairJob): primary_source = get_primary_source() source_priority = get_source_priority(primary_source) + # Fallback -1 (#765): a pinned canonical release wins over the whole + # cascade below — so Track Number Repair resolves the SAME release the + # Reorganizer does (Stage 3) and the two stop contradicting each other. + # Gated on the album carrying a canonical; everything below is untouched + # for albums without one (preserving the all-01-album rescue this job + # exists for — the regression we refused to take in a reactive fix). + canonical = _lookup_canonical_from_db(file_track_data, context) + if canonical: + c_source, c_id = canonical + if _is_valid_album_id(c_id): + tracks = _get_album_tracklist(c_source, c_id, cache) + if tracks: + logger.info("[Repair] %s — resolved via canonical %s album ID: %s", + folder_name, c_source, c_id) + return tracks + # Fallback 0: Check DB first. If any tracked file already has source IDs, # prefer the configured source order and use the first available album ID. source_album_ids = _lookup_album_ids_from_db(file_track_data, context) @@ -806,6 +822,46 @@ def _update_db_file_path(db, old_path: str, new_path: str): conn.close() +def _lookup_canonical_from_db(file_track_data: List[Tuple[str, str, Any]], + context: JobContext) -> Optional[Tuple[str, str]]: + """Return the album's pinned canonical ``(source, album_id)`` or None. + + #765: when the album this folder's files belong to has a canonical release + pinned (best-fit to the files), Track Number Repair uses it first so it + agrees with the Reorganizer. Resolves by matching a file path to its DB + track row. None when no DB, no match, columns absent, or unresolved.""" + if not context.db: + return None + conn = None + try: + conn = context.db._get_connection() + cursor = conn.cursor() + cursor.execute("PRAGMA table_info(albums)") + cols = {row[1] for row in cursor.fetchall()} + if 'canonical_source' not in cols or 'canonical_album_id' not in cols: + return None + for fpath, _, _ in file_track_data: + cursor.execute( + """ + SELECT al.canonical_source, al.canonical_album_id + FROM tracks t + JOIN albums al ON al.id = t.album_id + WHERE t.file_path = ? + LIMIT 1 + """, + (fpath,), + ) + row = cursor.fetchone() + if row and row[0] and row[1]: + return (str(row[0]), str(row[1])) + except Exception as e: + logger.debug("Error looking up canonical from DB: %s", e) + finally: + if conn: + conn.close() + return None + + def _lookup_album_ids_from_db(file_track_data: List[Tuple[str, str, Any]], context: JobContext) -> Dict[str, Optional[str]]: """Look up album IDs from the database using file paths. diff --git a/core/repair_worker.py b/core/repair_worker.py index 87ba8abb..4ab4af0c 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -371,6 +371,9 @@ class RepairWorker: 'interval_hours': config['interval_hours'], 'settings': config['settings'], 'default_settings': job.default_settings.copy(), + # Per-setting choice lists so the UI can render a dropdown + # instead of a free-text box (e.g. canonical source_selection). + 'setting_options': dict(getattr(job, 'setting_options', {}) or {}), 'last_run': last_run, 'next_run': next_run, 'is_running': self._current_job_id == job_id, @@ -1274,28 +1277,85 @@ class RepairWorker: return {'success': False, 'error': str(e)} def _fix_missing_cover_art(self, entity_type, entity_id, file_path, details): - """Update album thumbnail URL from the found artwork.""" + """Apply found artwork: update the DB thumbnail AND embed art into the + album's audio files + write cover.jpg (using the post-processing + standard, so the user's album_art_order preference is honored).""" artwork_url = details.get('found_artwork_url') if not artwork_url: return {'success': False, 'error': 'No artwork URL found in finding details'} album_id = details.get('album_id') or entity_id if not album_id: return {'success': False, 'error': 'No album ID associated with this finding'} + conn = None + track_paths = [] + album_title = details.get('album_title') + artist_name = details.get('artist') + mbid = details.get('musicbrainz_release_id') try: conn = self.db._get_connection() cursor = conn.cursor() cursor.execute("UPDATE albums SET thumb_url = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?", (artwork_url, album_id)) conn.commit() - if cursor.rowcount > 0: - return {'success': True, 'action': 'applied_cover_art', - 'message': 'Applied cover art to album'} - return {'success': False, 'error': 'Album not found in database'} + if cursor.rowcount == 0: + return {'success': False, 'error': 'Album not found in database'} + + # Pull album metadata + local track paths so we can write art to disk. + cursor.execute(""" + SELECT al.title, ar.name, al.musicbrainz_release_id + FROM albums al LEFT JOIN artists ar ON ar.id = al.artist_id + WHERE al.id = ? + """, (album_id,)) + meta_row = cursor.fetchone() + if meta_row: + album_title = album_title or meta_row[0] + artist_name = artist_name or meta_row[1] + mbid = mbid or meta_row[2] + cursor.execute(""" + SELECT file_path FROM tracks + WHERE album_id = ? AND file_path IS NOT NULL AND file_path != '' + """, (album_id,)) + track_paths = [r[0] for r in cursor.fetchall()] finally: if conn: conn.close() + # Resolve container/host path mismatches, keep only files that exist. + download_folder = self._config_manager.get('soulseek.download_path', '') if self._config_manager else None + resolved = [] + for p in track_paths: + rp = _resolve_file_path(p, self.transfer_folder, download_folder, config_manager=self._config_manager) or p + if os.path.isfile(rp): + resolved.append(rp) + + if not resolved: + # Media-server-only album (no local files): DB thumbnail is all we can set. + return {'success': True, 'action': 'applied_cover_art', + 'message': 'Applied cover art to album (database only — no local files found)'} + + from core.metadata.art_apply import apply_art_to_album_files + metadata = { + 'artist': artist_name, 'album_artist': artist_name, + 'album': album_title, 'album_art_url': artwork_url, + 'musicbrainz_release_id': mbid, + } + album_info = { + 'album_name': album_title, 'album_image_url': artwork_url, + 'musicbrainz_release_id': mbid, + } + folder = details.get('album_folder') or os.path.dirname(resolved[0]) + art_result = apply_art_to_album_files(resolved, metadata, album_info, folder=folder) + + embedded = art_result.get('embedded', 0) + msg = f'Applied cover art: embedded into {embedded}/{len(resolved)} file(s)' + if art_result.get('cover_written'): + msg += ' + wrote cover.jpg' + if embedded == 0 and not art_result.get('cover_written'): + # DB updated but nothing reached disk (e.g. read-only mount). + msg = 'Updated database thumbnail, but could not write art to files (read-only?)' + return {'success': True, 'action': 'applied_cover_art', 'message': msg, 'art_result': art_result} + def _fix_metadata_gap(self, entity_type, entity_id, file_path, details): """Apply found metadata fields to the track.""" found_fields = details.get('found_fields') diff --git a/core/similar_artists_worker.py b/core/similar_artists_worker.py new file mode 100644 index 00000000..1349b73c --- /dev/null +++ b/core/similar_artists_worker.py @@ -0,0 +1,343 @@ +"""Background worker that fills the ``similar_artists`` table for LIBRARY artists. + +The watchlist scanner only populates similar artists for *watchlist* artists, so +the artist map / discover surfaces are rich for watchlisted artists and sparse +for the rest of the library. This worker closes that gap: for every +source-matched library artist it asks MusicMap for ~25 similar artists, matches +each one to the user's metadata source chain (primary + active fallbacks) via the +shared :func:`core.metadata.similar_artists.get_musicmap_similar_artists`, and +stores the matched results keyed by the library artist's **metadata source id** — +the same key the watchlist scanner and the artist map use, so the two cooperate +(idempotent upsert + a retry window keep them from double-fetching). + +It plugs into the existing enrichment-worker pattern (background thread, status / +pause / resume, ``matched / not_found / pending / errors`` stats) so it shows up +as a bubble in the dashboard / Manage Enrichment Workers modal like every other +source worker. + +The pure seams below (:func:`pick_source_artist_id`, +:func:`map_payload_to_store_kwargs`, :func:`process_artist`) carry the logic and +are unit-tested in isolation; the class wires them to the DB + MusicMap. +""" + +from __future__ import annotations + +import threading +from typing import Any, Callable, Dict, Optional + +from utils.logging_config import get_logger +from core.worker_utils import interruptible_sleep + +logger = get_logger("similar_artists_worker") + + +# A matched MusicMap payload is {id, source, name, image_url, genres, popularity}. +# Map its single (id, source) onto the right add_or_update_similar_artist id +# kwarg. The table only has columns for these four providers; a match on any +# other source (e.g. discogs) is still stored, name-only. +_SOURCE_ID_FIELD = { + 'spotify': 'similar_artist_spotify_id', + 'itunes': 'similar_artist_itunes_id', + 'deezer': 'similar_artist_deezer_id', + 'musicbrainz': 'similar_artist_musicbrainz_id', +} + +# Library-artist source-id columns, in the same priority the watchlist scanner +# uses to key its rows — so a library artist and (if also watchlisted) its +# watchlist row resolve to the SAME source_artist_id and don't duplicate work. +_LIBRARY_ID_COLUMNS = ('spotify_artist_id', 'itunes_artist_id', 'deezer_id', 'musicbrainz_id') + + +def map_payload_to_store_kwargs(payload: Dict[str, Any]) -> Dict[str, Optional[str]]: + """Turn a matched MusicMap payload into the id kwarg for the store call.""" + src = str(payload.get('source') or '').lower() + pid = str(payload.get('id') or '') + field = _SOURCE_ID_FIELD.get(src) + return {field: pid} if (field and pid) else {} + + +def pick_source_artist_id(row: Dict[str, Any]) -> Optional[str]: + """The metadata source id to key a library artist's similars by, or None if + the artist isn't matched to any metadata source yet (→ skip it).""" + for key in _LIBRARY_ID_COLUMNS: + v = row.get(key) + if v: + return str(v) + return None + + +def process_artist( + source_artist_id: str, + artist_name: str, + fetch_similars: Callable[[str, int], Dict[str, Any]], + store_similar: Callable[..., bool], + limit: int = 25, + profile_id: int = 1, +) -> tuple: + """Fetch + store similar artists for one library artist. + + ``fetch_similars(name, limit)`` returns the ``get_musicmap_similar_artists`` + payload; ``store_similar(**kwargs)`` is ``add_or_update_similar_artist``. + Returns ``(status, stored_count, detail)`` where status is one of: + - ``'matched'`` — stored ≥1 similar artist + - ``'not_found'`` — MusicMap had no entry / nothing matched a source + - ``'error'`` — MusicMap/source failure (transient; eligible for retry) + ``detail`` is a short human-readable reason (status code + message, or + ``'no matches'`` / ``''``) so the worker can surface WHY a fetch failed + instead of swallowing it — needed to diagnose error rates. + """ + try: + result = fetch_similars(artist_name, limit) or {} + except Exception as exc: + return ('error', 0, f'exception: {exc}') + + if not result.get('success'): + # 404/400 = genuinely no MusicMap entry → 'not_found' (don't keep retrying); + # anything else (timeout, 5xx, no providers) = transient → 'error' (retry). + code = result.get('status_code') + detail = f"{code}: {result.get('error') or 'unknown'}" + return ('not_found' if code in (400, 404) else 'error', 0, detail) + + sims = result.get('similar_artists') or [] + if not sims: + return ('not_found', 0, 'no matches') + + stored = 0 + for rank, s in enumerate(sims, 1): + name = s.get('name') + if not name: + continue + kwargs = map_payload_to_store_kwargs(s) + if not kwargs: + # The match resolved to a source with no id column in similar_artists + # (e.g. discogs). Storing it name-only would be useless — you can't + # navigate/explore/download it. Enforce the standard: every stored + # similar carries a metadata source id, or we skip it. + logger.debug("Skipping similar '%s' (matched %s — no storable source id)", name, s.get('source')) + continue + try: + ok = store_similar( + source_artist_id=source_artist_id, + similar_artist_name=name, + similarity_rank=rank, + profile_id=profile_id, + image_url=s.get('image_url'), + genres=s.get('genres'), + popularity=s.get('popularity', 0) or 0, + **kwargs, + ) + if ok: + stored += 1 + except Exception as exc: + logger.debug("store similar failed for %s: %s", name, exc) + + return ('matched' if stored else 'not_found', stored, '') + + +class SimilarArtistsWorker: + """Background worker that populates similar artists for library artists.""" + + def __init__(self, database): + self.db = database + self.running = False + self.paused = False + self.should_stop = False + self.thread: Optional[threading.Thread] = None + self._stop_event = threading.Event() + self.current_item: Optional[str] = None + self.stats = {'matched': 0, 'not_found': 0, 'pending': 0, 'errors': 0} + self.retry_days = 30 + self.limit = 25 + self._err_logged = 0 # how many fetch errors we've logged at WARNING this session + self._last_error = None # most recent fetch-error reason (for diagnosis) + # similar_artists rows are profile-scoped; the library is shared. v1 keys + # under the default profile (matches single-profile setups, which is the + # common case). Multi-profile per-source-chain population is future work. + self.profile_id = 1 + logger.info("Similar Artists background worker initialized") + + # ── lifecycle (mirrors the other enrichment workers) ────────────────── + def start(self): + if self.running: + return + self.running = True + self.should_stop = False + self._stop_event.clear() + self.thread = threading.Thread(target=self._run, daemon=True) + self.thread.start() + logger.info("Similar Artists worker started") + + def stop(self): + self.should_stop = True + self.running = False + self._stop_event.set() + + def pause(self): + self.paused = True + + def resume(self): + self.paused = False + + def get_stats(self) -> Dict[str, Any]: + # Report PERSISTENT counts from the DB (not the in-memory session + # counters), so the dashboard orb and the Manage modal always agree and + # survive restarts — same approach as the other enrichment workers. + c = self._db_counts() + self.stats = { + 'matched': c['matched'], 'not_found': c['not_found'], + 'pending': c['pending'], 'errors': c['error'], + } + total = c['total'] + is_running = self.running and (self.thread is not None and self.thread.is_alive()) + is_idle = is_running and not self.paused and c['pending'] == 0 and self.current_item is None + return { + 'enabled': True, + 'running': is_running and not self.paused, + 'paused': self.paused, + 'idle': is_idle, + 'current_item': self.current_item, + 'stats': self.stats.copy(), + # Artist-only progress (no album/track phases) so the orb tooltip can + # show "matched / total (percent%)" like every other worker. + 'progress': { + 'artists': { + 'matched': c['matched'], 'total': total, + 'percent': int(round(c['matched'] / total * 100)) if total else 0, + } + }, + } + + # ── worker loop ──────────────────────────────────────────────────────── + def _run(self): + logger.info("Similar Artists worker thread started") + # Imported lazily so the worker module stays import-light for tests. + from core.metadata.similar_artists import get_musicmap_similar_artists + + while not self.should_stop: + try: + if self.paused: + interruptible_sleep(self._stop_event, 1) + continue + + self.current_item = None + artist = self._get_next_artist() + if not artist: + interruptible_sleep(self._stop_event, 15) + continue + + sid = pick_source_artist_id(artist) + if not sid: + # Query already filters to artists with a source id; guard anyway. + self._mark(artist['id'], 'error') + continue + + self.current_item = artist.get('name') + status, count, detail = process_artist( + sid, artist['name'], + get_musicmap_similar_artists, + self.db.add_or_update_similar_artist, + limit=self.limit, profile_id=self.profile_id, + ) + self._mark(artist['id'], status) + if status == 'matched': + self.stats['matched'] += 1 + logger.debug("Similar artists: %s → stored %d", artist['name'], count) + elif status == 'not_found': + self.stats['not_found'] += 1 + else: + self.stats['errors'] += 1 + # Surface WHY fetches error — the first handful at WARNING (so + # the cause is visible in app.log without spamming a 4000-artist + # run), the rest at DEBUG. Keep the most recent reason for stats. + self._last_error = detail + if self._err_logged < 15: + self._err_logged += 1 + logger.warning("Similar artists fetch error for '%s' — %s", artist['name'], detail) + else: + logger.debug("Similar artists fetch error for '%s' — %s", artist['name'], detail) + + # Pace MusicMap (name search per candidate is heavy + rate-limited). + interruptible_sleep(self._stop_event, 3) + except Exception as exc: + logger.error("Similar Artists worker loop error: %s", exc) + interruptible_sleep(self._stop_event, 5) + + logger.info("Similar Artists worker thread finished") + + # ── DB helpers (thin; the testable logic lives in the pure seams above) ── + def _has_source_id_clause(self) -> str: + return '(' + ' OR '.join(f"{c} IS NOT NULL AND {c} != ''" for c in _LIBRARY_ID_COLUMNS) + ')' + + def _get_next_artist(self) -> Optional[Dict[str, Any]]: + try: + conn = self.db._get_connection() + cursor = conn.cursor() + cols = "id, name, " + ", ".join(_LIBRARY_ID_COLUMNS) + have_id = self._has_source_id_clause() + # 1) Unattempted source-matched artists. + cursor.execute(f""" + SELECT {cols} FROM artists + WHERE id IS NOT NULL AND name IS NOT NULL + AND similar_artists_match_status IS NULL + AND {have_id} + ORDER BY id ASC LIMIT 1 + """) + row = cursor.fetchone() + # 2) Retry transient failures (and re-check 'not_found') after retry_days. + if not row: + cursor.execute(f""" + SELECT {cols} FROM artists + WHERE id IS NOT NULL AND name IS NOT NULL + AND similar_artists_match_status IN ('error', 'not_found') + AND (similar_artists_last_attempted IS NULL + OR similar_artists_last_attempted < datetime('now', ?)) + AND {have_id} + ORDER BY similar_artists_last_attempted ASC LIMIT 1 + """, (f'-{self.retry_days} days',)) + row = cursor.fetchone() + if not row: + return None + keys = ['id', 'name'] + list(_LIBRARY_ID_COLUMNS) + return dict(zip(keys, row, strict=False)) + except Exception as exc: + logger.debug("Similar Artists _get_next_artist failed: %s", exc) + return None + + def _mark(self, artist_id, status: str): + try: + conn = self.db._get_connection() + cursor = conn.cursor() + cursor.execute( + "UPDATE artists SET similar_artists_match_status = ?, " + "similar_artists_last_attempted = CURRENT_TIMESTAMP WHERE id = ?", + (status, artist_id), + ) + conn.commit() + except Exception as exc: + logger.debug("Similar Artists _mark failed for %s: %s", artist_id, exc) + + def _count_pending(self) -> int: + return self._db_counts()['pending'] + + def _db_counts(self) -> Dict[str, int]: + """Persistent tallies over the worker's universe (source-matched library + artists): matched / not_found / error / pending(NULL) / total.""" + out = {'matched': 0, 'not_found': 0, 'error': 0, 'pending': 0, 'total': 0} + try: + conn = self.db._get_connection() + cursor = conn.cursor() + cursor.execute(f""" + SELECT + SUM(CASE WHEN similar_artists_match_status = 'matched' THEN 1 ELSE 0 END), + SUM(CASE WHEN similar_artists_match_status = 'not_found' THEN 1 ELSE 0 END), + SUM(CASE WHEN similar_artists_match_status = 'error' THEN 1 ELSE 0 END), + SUM(CASE WHEN similar_artists_match_status IS NULL THEN 1 ELSE 0 END), + COUNT(*) + FROM artists WHERE {self._has_source_id_clause()} + """) + row = cursor.fetchone() or (0, 0, 0, 0, 0) + out.update(matched=int(row[0] or 0), not_found=int(row[1] or 0), + error=int(row[2] or 0), pending=int(row[3] or 0), total=int(row[4] or 0)) + except Exception as exc: + logger.debug("Similar Artists _db_counts failed: %s", exc) + return out diff --git a/core/spotify_public_api.py b/core/spotify_public_api.py new file mode 100644 index 00000000..d0dc49c1 --- /dev/null +++ b/core/spotify_public_api.py @@ -0,0 +1,118 @@ +"""Full public-playlist fetch for the 'Spotify link' path, via the OPTIONAL +SpotipyFree library (no Spotify credentials needed). + +Why a library: the embed scraper caps at ~100 tracks, and getting the full list +with no login means talking to Spotify's private API the way the web player +does — including client-auth headers Spotify rotates constantly. Rather than +chase those ourselves (we tried; Spotify 429s the bare token), we lean on +SpotipyFree — the maintained no-creds ``spotipy`` drop-in that spotDL uses, +which tracks those rotating bits for us. + +Licensing: SpotipyFree is GPL-3.0, so it is NOT bundled or required by SoulSync +(MIT). It's an OPTIONAL install — if the user has run ``pip install spotipyFree`` +this lights up; otherwise the import fails, this raises, and the caller +(``spotify_public_scraper.fetch_spotify_public``) falls back to the embed +scraper (today's ≤100). So SoulSync ships zero GPL code and stays cleanly MIT. + +``client_factory`` is injectable so the orchestration is unit-testable without +the library or the network. +""" + +from __future__ import annotations + +import hashlib +import logging +from typing import Any, Callable, Dict, List, Optional + +logger = logging.getLogger('soulsync.spotify_public') + +_MAX_TRACKS = 10000 # safety cap + + +def normalize_api_track(item: Any, index: int) -> Optional[Dict[str, Any]]: + """Convert a spotipy-shape playlist item to the embed scraper's track shape. + + Returns None for items without a usable track id (local files, removed + tracks, podcast episodes) so the caller can skip them. + """ + track = (item or {}).get('track') or {} + track_id = track.get('id') + if not track_id: + return None + artists = [{'name': a.get('name', '')} for a in (track.get('artists') or []) if a.get('name')] + return { + 'id': track_id, + 'name': track.get('name', 'Unknown Track'), + 'artists': artists or [{'name': 'Unknown Artist'}], + 'duration_ms': track.get('duration_ms', 0), + 'is_explicit': bool(track.get('explicit', False)), + 'track_number': index + 1, + } + + +def _default_client(): + """Create a no-credentials SpotipyFree client. + + Raises ImportError when the optional GPL-3.0 library isn't installed — the + caller treats that like any other failure and falls back to the embed + scraper. + """ + from SpotipyFree import Spotify # optional, user-installed (GPL-3.0) + return Spotify() + + +def fetch_public_playlist_full( + spotify_id: str, + *, + client_factory: Optional[Callable[[], Any]] = None, +) -> Dict[str, Any]: + """Pull a public playlist's FULL track list with no credentials. + + Uses a SpotipyFree client (spotipy-compatible: ``playlist`` for metadata, + ``playlist_items`` + ``next`` for paginated tracks). Returns the embed + scraper's shape. Raises on any failure (incl. the library not being + installed) so the caller can fall back to the embed scraper. + """ + client = (client_factory or _default_client)() + + meta: Dict[str, Any] = {} + try: + # limit=1: we only want name/owner here — tracks come from the paginated + # playlist_items call below, so don't pull the whole list twice. + meta = client.playlist(spotify_id, limit=1) or {} + except Exception as e: # metadata is nice-to-have; tracks are the point + logger.debug("playlist metadata fetch failed (%s); continuing", e) + name = meta.get('name', 'Unknown') + subtitle = (meta.get('owner') or {}).get('display_name', '') + + tracks: List[Dict[str, Any]] = [] + results = client.playlist_items(spotify_id) + while results: + for item in results.get('items', []): + t = normalize_api_track(item, len(tracks)) + if t: + tracks.append(t) + if len(tracks) >= _MAX_TRACKS: + break + if results.get('next'): + results = client.next(results) + else: + break + + if not tracks: + raise RuntimeError('SpotipyFree returned no usable tracks') + + logger.info("SpotipyFree full fetch: %s (%d tracks)", name, len(tracks)) + source_url = f'https://open.spotify.com/playlist/{spotify_id}' + return { + 'id': spotify_id, + 'type': 'playlist', + 'name': name, + 'subtitle': subtitle, + 'tracks': tracks, + 'url': source_url, + 'url_hash': hashlib.md5(source_url.encode()).hexdigest()[:12], + } + + +__all__ = ['normalize_api_track', 'fetch_public_playlist_full'] diff --git a/core/spotify_public_scraper.py b/core/spotify_public_scraper.py index fa299817..1d5d653b 100644 --- a/core/spotify_public_scraper.py +++ b/core/spotify_public_scraper.py @@ -9,7 +9,8 @@ import logging import hashlib import requests -logger = logging.getLogger(__name__) +# 'soulsync.*' so these lines land in app.log (the bare module name isn't captured). +logger = logging.getLogger('soulsync.spotify_public') def parse_spotify_url(url: str) -> dict: @@ -82,10 +83,18 @@ def scrape_spotify_embed(spotify_type: str, spotify_id: str) -> dict: logger.error(f"Failed to fetch Spotify embed: {e}") return {'error': f'Failed to fetch Spotify page: {str(e)}'} + return parse_embed_html(response.text, spotify_type, spotify_id) + + +def parse_embed_html(html: str, spotify_type: str, spotify_id: str) -> dict: + """Parse a Spotify embed page's ``__NEXT_DATA__`` into the standard result + shape. Shared by the embed scraper and the full public fetch, which pulls + the name + first-page tracks from the same embed page it reads the token + from (so it needs no extra metadata request).""" # Extract __NEXT_DATA__ JSON match = re.search( r'', - response.text + html ) if not match: logger.error("No __NEXT_DATA__ found in Spotify embed response") @@ -151,3 +160,31 @@ def scrape_spotify_embed(spotify_type: str, spotify_id: str) -> dict: logger.info(f"Scraped Spotify {spotify_type}: {result['name']} ({len(tracks)} tracks)") return result + + +def fetch_spotify_public(spotify_type: str, spotify_id: str) -> dict: + """Fetch a public Spotify link, preferring the FULL track list. + + Playlists are pulled via the anonymous public-API path + (``spotify_public_api.fetch_public_playlist_full``), which paginates past + the embed widget's ~100-track cap. On ANY failure — or for albums, which + the embed already returns whole — this falls back to ``scrape_spotify_embed`` + (today's behaviour). Same return shape either way, so callers don't care + which path produced the data. + """ + if spotify_type == 'playlist': + try: + from core.spotify_public_api import fetch_public_playlist_full + result = fetch_public_playlist_full(spotify_id) + if result and result.get('tracks'): + logger.info( + f"Spotify public API (full): {result.get('name')} " + f"({len(result['tracks'])} tracks)" + ) + return result + except Exception as e: + logger.info( + f"Spotify public full fetch failed ({e}); " + f"falling back to embed scraper (≤100 tracks)" + ) + return scrape_spotify_embed(spotify_type, spotify_id) diff --git a/core/spotify_worker.py b/core/spotify_worker.py index 9349bb74..4e0b3e66 100644 --- a/core/spotify_worker.py +++ b/core/spotify_worker.py @@ -260,6 +260,17 @@ class SpotifyWorker: conn = self.db._get_connection() cursor = conn.cursor() + # Pinned-group override (Manage Enrichment Workers): process one + # entity type first, then fall through to the normal chain. Unset or + # exhausted ⇒ default artist→album→track order, unchanged. + from core.worker_utils import read_enrichment_priority, priority_pending_item + _prio = read_enrichment_priority('spotify') + if _prio: + _pi = priority_pending_item(cursor, 'spotify', _prio, + {'album': 'album_individual', 'track': 'track_individual'}) + if _pi: + return _pi + # Priority 1: Unattempted artists cursor.execute(""" SELECT id, name diff --git a/core/sync/playlist_edit.py b/core/sync/playlist_edit.py new file mode 100644 index 00000000..c2cf34d5 --- /dev/null +++ b/core/sync/playlist_edit.py @@ -0,0 +1,74 @@ +"""Pure planners for sync-editor playlist mutations (#768 Bug C). + +The sync editor's "Find & add" and remove actions rewrite the whole server +playlist from a flat list of track IDs (Subsonic/Navidrome + Jellyfin have no +position-level ops). Two bugs lived in the inline endpoint logic: + +* **Duplicate on manual match.** "Find & add" always *inserted* the chosen + track — but when the user is matching an UNMATCHED source to a server track + that's already in the playlist (an orphan "extra"), the intent is to LINK + them, not add a second copy. Each attempt appended another duplicate + (positions 72, 73, 74…). ``plan_playlist_add`` skips the insert when it's a + link to an already-present track (the caller still persists the override). + +* **Delete removes ALL copies.** The inline remove filtered out *every* entry + with the target ID. With duplicates present, deleting one removed them all. + ``remove_one_occurrence`` drops a single entry (duplicates are the same + track, so removing any one is correct). + +Pure, no I/O — the caller fetches the current track-id list and applies the +returned plan to the media-server client. +""" + +from __future__ import annotations + +from typing import List, Optional, Tuple + + +def plan_playlist_add( + current_ids: List[str], + track_id: str, + *, + is_link: bool, + position: Optional[int] = None, +) -> dict: + """Plan a "Find & add" against a flat track-id playlist. + + ``is_link`` is True when the add carries a ``source_track_id`` (i.e. the + user is matching an unmatched source to this server track). In that case, + if the track is ALREADY in the playlist, return ``should_insert=False`` so + the caller only records the override and never duplicates it. + + Returns ``{'should_insert': bool, 'new_ids': [...]}``. ``new_ids`` equals + the input (stringified) when no insert is needed.""" + tid = str(track_id) + current = [str(t) for t in current_ids] + if is_link and tid in current: + return {"should_insert": False, "new_ids": current} + pos = len(current) if position is None else max(0, min(int(position), len(current))) + new_ids = current[:pos] + [tid] + current[pos:] + return {"should_insert": True, "new_ids": new_ids} + + +def remove_one_occurrence( + track_ids: List[str], + target_id: str, + position: Optional[int] = None, +) -> Tuple[List[str], bool]: + """Remove a SINGLE occurrence of ``target_id`` from a flat id list. + + If ``position`` is given and the id there matches, that exact entry is + removed (so the user removes the row they clicked); otherwise the first + matching id is removed. Returns ``(new_ids, removed)``. ``removed`` is + False when the id isn't present (caller should 404).""" + target = str(target_id) + ids = [str(t) for t in track_ids] + if position is not None and 0 <= position < len(ids) and ids[position] == target: + return ids[:position] + ids[position + 1:], True + for idx, tid in enumerate(ids): + if tid == target: + return ids[:idx] + ids[idx + 1:], True + return ids, False + + +__all__ = ["plan_playlist_add", "remove_one_occurrence"] diff --git a/core/sync/playlist_reconcile.py b/core/sync/playlist_reconcile.py new file mode 100644 index 00000000..eb402b6d --- /dev/null +++ b/core/sync/playlist_reconcile.py @@ -0,0 +1,197 @@ +"""Reconcile a source playlist against a media-server playlist (pure). + +Lifted verbatim from the inline three-pass matcher in +``web_server.get_server_playlist_tracks`` so it can be unit-tested and so the +two #768 fixes live in importable, covered code: + + Pass 0 user-confirmed overrides (``sync_match_cache``), applied first. + Pass 1 exact normalized-title match. + Pass 2 fuzzy match on ``"artist title"`` (SequenceMatcher >= 0.75). + Extra server tracks no source claimed -> ``match_status='extra'``. + +Two bug fixes over the original inline version: + +* #768 Bug A — the source side is YouTube/streaming-shaped (title + ``"Artist - Song"``, artist ``"Official Artist"``). The original passes + compared the raw title, so ``"Arctic Monkeys - Do I Wanna Know?"`` never + matched the library's ``"Do I Wanna Know?"`` and the track showed as + unmatched while its server copy showed as an orphan "extra". We now also try + the canonicalized source title/artist (see ``core.text.source_title``). + +* #768 Bug B — the original built the per-source ``src_entry`` WITHOUT + ``source_track_id``, so the editor UI never received it; "Find & add" then + posted an empty id and the manual match was never persisted (it reverted to + "extra" on reload, and re-adding duplicated the track). ``src_entry`` now + carries ``source_track_id``. + +Pure, no I/O. ``override_pairs`` (``{source_idx: server_idx}``) is computed by +the caller via ``core.sync.match_overrides.resolve_match_overrides`` so the DB +lookup stays out of this module. +""" + +from __future__ import annotations + +import re +from difflib import SequenceMatcher +from typing import Any, Dict, List, Optional + +from core.text.source_title import canonical_source_track + +_FUZZY_THRESHOLD = 0.75 + +_FEAT_RE = re.compile(r'\s*[\(\[](?:feat|ft)\.?[^\)\]]*[\)\]]', re.IGNORECASE) +_REMASTER_RE = re.compile( + r'\s*[\(\[](?:\d{4}\s+)?remaster(?:ed)?(?:\s+version)?\s*[\)\]]', re.IGNORECASE) +_EDITION_RE = re.compile( + r'\s*[\(\[](?:deluxe|special|anniversary|legacy|expanded|limited)(?:\s+edition)?\s*[\)\]]', + re.IGNORECASE) + + +def norm_title(t: str) -> str: + """Strip feat./ft., remaster, and edition qualifiers for comparison only. + Byte-faithful to web_server's inline ``_norm_title``.""" + t = _FEAT_RE.sub('', t or '') + t = _REMASTER_RE.sub('', t) + t = _EDITION_RE.sub('', t) + return t.lower().strip() + + +def _src_entry(src: Dict[str, Any], position_fallback: int) -> Dict[str, Any]: + return { + 'name': src.get('name', ''), + 'artist': src.get('artist', ''), + 'album': src.get('album', ''), + 'image_url': src.get('image_url', ''), + 'duration_ms': src.get('duration_ms', 0), + 'position': src.get('position', position_fallback), + # #768 Bug B: echo the source id back so the editor can persist a + # manual "Find & add" override against it. + 'source_track_id': src.get('source_track_id', '') or '', + } + + +def _resolved_artist(src: Dict[str, Any]) -> str: + """Artist string, falling back to the first of an ``artists`` list.""" + artist = src.get('artist', '') + if not artist and src.get('artists'): + a = src['artists'][0] if src['artists'] else '' + artist = a.get('name', a) if isinstance(a, dict) else str(a) + return artist or '' + + +def reconcile_playlist( + source_tracks: List[Dict[str, Any]], + server_tracks: List[Dict[str, Any]], + override_pairs: Optional[Dict[int, int]] = None, +) -> List[Dict[str, Any]]: + """Return the combined matched/missing/extra view (list of dicts). + + Each combined entry has ``source_track``, ``server_track``, + ``match_status`` ('matched'|'missing'|'extra'), ``confidence``, and + ``override: True`` on override hits.""" + override_pairs = override_pairs or {} + combined: List[Dict[str, Any]] = [] + used_server_indices: set[int] = set() + unmatched_source: List[tuple[int, Dict[str, Any], str]] = [] + + # Precompute normalized server titles once. + server_norm = [norm_title(svr.get('title', '')) for svr in server_tracks] + + for i, src in enumerate(source_tracks): + src_artist = _resolved_artist(src) + src_name = src.get('name', '') + src_entry = _src_entry({**src, 'artist': src_artist}, i) + + # Pass 0: user-confirmed override. + if i in override_pairs: + j = override_pairs[i] + used_server_indices.add(j) + combined.append({ + 'source_track': src_entry, + 'server_track': server_tracks[j], + 'match_status': 'matched', + 'confidence': 1.0, + 'override': True, + }) + continue + + # Pass 1: exact normalized-title match — try the raw source title AND + # the canonicalized one (strips "Artist - " prefix / channel artist). + canon_title, _canon_artist = canonical_source_track(src_name, src_artist) + candidates = {norm_title(src_name), norm_title(canon_title)} + best_idx = -1 + for j, svr_norm in enumerate(server_norm): + if j in used_server_indices: + continue + if svr_norm in candidates: + best_idx = j + break + + if best_idx >= 0: + used_server_indices.add(best_idx) + combined.append({ + 'source_track': src_entry, + 'server_track': server_tracks[best_idx], + 'match_status': 'matched', + 'confidence': 1.0, + }) + else: + idx = len(combined) + combined.append({ + 'source_track': src_entry, + 'server_track': None, + 'match_status': 'missing', + 'confidence': 0.0, + }) + # Carry the canonical artist for the fuzzy pass. + unmatched_source.append((idx, src_entry, _canon_artist or src_artist)) + + # Pass 2: fuzzy match on remaining unmatched source tracks. Build the key + # from the canonicalized title/artist so YouTube-shaped sources can pair. + for combo_idx, src_entry, canon_artist in unmatched_source: + canon_title, _ = canonical_source_track(src_entry['name'], src_entry['artist']) + src_key = f"{canon_artist} {norm_title(canon_title)}".strip().lower() + best_score = 0.0 + best_j = -1 + for j, svr in enumerate(server_tracks): + if j in used_server_indices: + continue + svr_key = f"{svr.get('artist', '')} {norm_title(svr.get('title', ''))}".strip().lower() + score = SequenceMatcher(None, src_key, svr_key).ratio() + if score > best_score and score >= _FUZZY_THRESHOLD: + best_score = score + best_j = j + if best_j >= 0: + used_server_indices.add(best_j) + combined[combo_idx] = { + 'source_track': src_entry, + 'server_track': server_tracks[best_j], + 'match_status': 'matched', + 'confidence': round(best_score, 3), + } + + # Extra: server tracks no source claimed. + for j, svr in enumerate(server_tracks): + if j not in used_server_indices: + combined.append({ + 'source_track': None, + 'server_track': svr, + 'match_status': 'extra', + 'confidence': 0.0, + }) + + # #766: a source row with no art of its own (e.g. a YouTube source, which + # provides none) borrows its MATCHED server track's cover so both sides of + # the editor show an image. Keyed off the actual pairing — works for + # "Artist - Title" rows that a fuzzy title lookup would miss. Source rows + # that already have their own art (Spotify CDN, etc.) keep it. + for entry in combined: + st = entry.get('source_track') + sv = entry.get('server_track') + if st and sv and not st.get('image_url') and sv.get('thumb'): + st['image_url'] = sv['thumb'] + + return combined + + +__all__ = ["reconcile_playlist", "norm_title"] diff --git a/core/text/source_title.py b/core/text/source_title.py new file mode 100644 index 00000000..03c80e2a --- /dev/null +++ b/core/text/source_title.py @@ -0,0 +1,105 @@ +"""Normalize streaming/YouTube-style source track metadata for matching. + +Issue #768: source playlists — especially ones seeded from YouTube — carry +video-style metadata: the title is ``"Artist - Song"`` and the artist is a +channel name like ``"Official Arctic Monkeys"``, ``"Arctic Monkeys - Topic"``, +or ``"ColdplayVEVO"``. The library/media-server side has the clean ``"Song"`` / +``"Arctic Monkeys"``. Both matching paths (the sync confidence scorer and the +playlist-editor reconcile) then fail to pair them — the track is reported +"not matched" / shows up as an orphan "extra" even though it exists. + +These helpers strip that channel/video decoration so the cleaned source can be +compared against the clean library metadata. Pure, no I/O. + +Conservative by construction: +- ``strip_artist_prefix`` removes a leading ``""`` only when the + prefix EQUALS the artist we're matching against. So ``"Death - Pull the + Plug"`` by ``"Death"`` is helped, while ``"Marvin Gaye"`` by Charlie Puth + (title is not ``"Charlie Puth - ..."``) is left untouched, and a hyphenated + word like ``"Self-Titled"`` is never split (a separator needs surrounding + whitespace, or a colon). +- ``clean_source_artist`` only removes well-known channel decorations. + +Both are intended to be applied as ADDITIONAL match candidates (best-of), so +an over-eager strip can only add a comparison, never remove the original. +""" + +from __future__ import annotations + +import re + +from core.text.normalize import normalize_for_comparison + +# Artist/title separator: a dash/pipe/tilde flanked by whitespace, OR a colon +# (with optional trailing space). Whitespace-flanking keeps "Self-Titled" and +# "Jay-Z" intact while still splitting "Artist - Title". +_SEP_SPLIT = re.compile(r"\s+[-–—|~]\s+|\s*:\s+") + +# YouTube auto-generated artist channel: "Arctic Monkeys - Topic". +_TOPIC_SUFFIX = re.compile(r"\s*-\s*topic\s*$", re.IGNORECASE) +# "Official " / "The Official " channel prefix. +_OFFICIAL_PREFIX = re.compile(r"^\s*(?:the\s+)?official\s+", re.IGNORECASE) +# Trailing VEVO, attached ("ColdplayVEVO") or spaced ("Coldplay VEVO"). +_VEVO_SUFFIX = re.compile(r"\s*vevo\s*$", re.IGNORECASE) + + +def clean_source_artist(artist: str) -> str: + """Strip well-known streaming-channel decoration from an artist name. + + ``"Official Arctic Monkeys"`` → ``"Arctic Monkeys"``; + ``"Arctic Monkeys - Topic"`` → ``"Arctic Monkeys"``; + ``"ColdplayVEVO"`` → ``"Coldplay"``. Returns the input unchanged when + nothing matches, and never returns empty for non-empty input.""" + if not artist: + return artist + s = artist.strip() + + topic = _TOPIC_SUFFIX.sub("", s).strip() + if topic and topic != s: + s = topic + + official = _OFFICIAL_PREFIX.sub("", s).strip() + if official: + s = official + + # Only strip VEVO if at least 2 chars of name remain (don't empty "VEVO"). + vevo = _VEVO_SUFFIX.sub("", s).strip() + if len(vevo) >= 2 and vevo != s: + s = vevo + + return s or artist + + +def strip_artist_prefix(title: str, artist: str) -> str: + """Remove a leading ``""`` from ``title`` when the prefix + equals ``artist`` (accent/case-folded). Otherwise return ``title`` unchanged. + + ``("Arctic Monkeys - Do I Wanna Know?", "Arctic Monkeys")`` → ``"Do I Wanna + Know?"``. Never returns an empty string.""" + if not title or not artist: + return title + na = normalize_for_comparison(artist) + if not na: + return title + parts = _SEP_SPLIT.split(title, maxsplit=1) + if len(parts) == 2: + left, right = parts + right = right.strip() + if right and normalize_for_comparison(left) == na: + return right + return title + + +def canonical_source_track(title: str, artist: str) -> tuple[str, str]: + """Best-effort clean (title, artist) for matching a streaming/YouTube source + against clean library metadata. Cleans the artist first, then strips a + leading artist prefix from the title using EITHER the cleaned or the raw + artist (YouTube titles prepend the real artist, not the channel name).""" + cleaned_artist = clean_source_artist(artist) + new_title = strip_artist_prefix(title, cleaned_artist) + if new_title == title and cleaned_artist != artist: + new_title = strip_artist_prefix(title, artist) + return new_title, cleaned_artist + + +__all__ = ["clean_source_artist", "strip_artist_prefix", "canonical_source_track"] diff --git a/core/text/title_match.py b/core/text/title_match.py new file mode 100644 index 00000000..ed8ca4a2 --- /dev/null +++ b/core/text/title_match.py @@ -0,0 +1,81 @@ +"""Guard against char-level title false positives in track matching. + +Issue #769: playlist sync matched tracks that aren't in the library to a +DIFFERENT song by the SAME artist, with high confidence — e.g. "Dani +California" -> "Californication" (Red Hot Chili Peppers), "Under The Bridge" +-> "Around the World". The confidence formula is ``0.5*title + 0.5*artist``, +and a same-artist comparison always yields ``artist = 1.0``, so the title score +is the only thing that can tell two of an artist's songs apart. But the title +score is a ``difflib.SequenceMatcher`` character ratio, which over-credits +unrelated titles that happen to share a long substring ("californi…") or only a +stopword ("the"): 0.67 and 0.62 respectively. With the flat 0.5 artist term +that lands at 0.83 / 0.81 — well over the 0.7 sync threshold. + +``titles_plausibly_same`` adds a cheap word-level sanity check on top of the +char ratio: accept a pair only when it's near-identical char-wise (so typos and +punctuation/casing variants — "Beleive"/"Believe", "HUMBLE."/"Humble" — still +match) OR the two titles share at least one significant (non-stopword) token. +Two genuinely different songs by the same artist share no content word, so they +get rejected; the real track is then correctly reported missing. +""" + +from __future__ import annotations + +import re + +# Articles / prepositions / conjunctions only. Deliberately NOT pronouns +# ("you", "me", "i") — those carry meaning in song titles and dropping them +# could strip the only shared word from a real match. "the" MUST stay here: +# without it "Under The Bridge" and "Around the World" would falsely share it. +_TITLE_STOPWORDS = frozenset({ + "the", "a", "an", "of", "and", "or", "to", "in", "on", + "for", "with", "at", "by", "from", +}) + +_TOKEN_RE = re.compile(r"[a-z0-9]+") + +# Char ratio at/above which two titles are treated as the same regardless of +# shared words — covers typos, punctuation, casing, accents. Tuned so single- +# word typos ("Beleive"/"Believe" = 0.857) pass while the #769 false positives +# ("Dani California"/"Californication" = 0.667) do not. +_NEAR_IDENTICAL = 0.85 + + +def _content_tokens(text: str) -> set[str]: + return {t for t in _TOKEN_RE.findall((text or "").lower()) if t not in _TITLE_STOPWORDS} + + +def titles_plausibly_same( + title_a: str, + title_b: str, + char_similarity: float, + *, + near_identical: float = _NEAR_IDENTICAL, +) -> bool: + """Whether two titles could be the same track, given their char similarity. + + ``title_a`` / ``title_b`` should already be normalised/cleaned (lowercased, + brackets stripped) the same way the caller computed ``char_similarity``. + + Returns ``True`` when the pair is near-identical char-wise OR shares at + least one significant (non-stopword) token. Returns ``False`` for two + titles that are only moderately char-similar and share no content word — + i.e. different songs the char ratio over-credited (#769).""" + if char_similarity >= near_identical: + return True + ta = _content_tokens(title_a) + tb = _content_tokens(title_b) + # Word-overlap is only a reliable "different song" signal when at least one + # side has 2+ content words — that's the #769 case where the char ratio + # over-credits a shared substring ("Dani California"/"Californication") or + # a stopword ("Under The Bridge"/"Around the World"). For single-word + # titles there's no other word to share, so applying it would wrongly fail + # legitimate stylized spellings ("Grey"/"Gray", "Tonite"/"Tonight", + # "Thru"/"Through") that the char ratio rightly accepts. In that case defer + # to the caller's existing char-similarity floor instead of force-failing. + if max(len(ta), len(tb)) < 2 or not ta or not tb: + return True + return not ta.isdisjoint(tb) + + +__all__ = ["titles_plausibly_same"] diff --git a/core/tidal_worker.py b/core/tidal_worker.py index 4bcb434a..051f2843 100644 --- a/core/tidal_worker.py +++ b/core/tidal_worker.py @@ -198,6 +198,16 @@ class TidalWorker: conn = self.db._get_connection() cursor = conn.cursor() + # Pinned-group override (Manage Enrichment Workers): process one + # entity type first, then fall through to the normal chain. Unset or + # exhausted ⇒ default artist→album→track order, unchanged. + from core.worker_utils import read_enrichment_priority, priority_pending_item + _prio = read_enrichment_priority('tidal') + if _prio: + _pi = priority_pending_item(cursor, 'tidal', _prio) + if _pi: + return _pi + # Priority 1: Unattempted artists cursor.execute(""" SELECT id, name diff --git a/core/torrent_clients/qbittorrent.py b/core/torrent_clients/qbittorrent.py index 579c8ec8..147c359b 100644 --- a/core/torrent_clients/qbittorrent.py +++ b/core/torrent_clients/qbittorrent.py @@ -117,7 +117,19 @@ class QBittorrentAdapter: headers={'Referer': self._url}, timeout=self.DEFAULT_TIMEOUT, ) - if not resp.ok or resp.text.strip() != 'Ok.': + body = resp.text.strip() + has_sid = bool(sess.cookies.get('SID')) + # qBittorrent reports BAD credentials as HTTP 200 + body "Fails." + # (it does NOT use a 4xx). SUCCESS is the SID auth cookie and/or a + # success body: "Ok." on <= 5.1, or an empty HTTP 204 on 5.2.0+, + # which changed /api/v2/auth/login to return 204 No Content. + # The old check required body == "Ok." and so rejected 5.2.0+. + login_ok = ( + resp.ok + and body.lower() != 'fails.' + and (has_sid or resp.status_code == 204 or body in ('', 'Ok.')) + ) + if not login_ok: logger.error("qBittorrent login failed: HTTP %s body=%r", resp.status_code, resp.text[:200]) return None self._session = sess diff --git a/core/wishlist/processing.py b/core/wishlist/processing.py index fb6cc20e..2074921e 100644 --- a/core/wishlist/processing.py +++ b/core/wishlist/processing.py @@ -394,12 +394,17 @@ def resolve_wishlist_source_type_for_batch(batch: Dict[str, Any]) -> str: def build_wishlist_source_context(batch: Dict[str, Any], current_time: datetime | None = None) -> Dict[str, Any]: """Build the source_context payload used when adding failed tracks back to the wishlist.""" current_time = current_time or datetime.now() + playlist_id = batch.get('source_playlist_ref') or batch.get('playlist_id') context = { 'playlist_name': batch.get('playlist_name', 'Unknown Playlist'), - 'playlist_id': batch.get('playlist_id', None), + 'playlist_id': playlist_id, 'added_from': 'webui_modal', 'timestamp': current_time.isoformat(), } + if batch.get('mirrored_playlist_id') is not None: + context['mirrored_playlist_id'] = batch.get('mirrored_playlist_id') + if batch.get('organize_by_playlist'): + context['organize_by_playlist'] = True # Preserve album-batch provenance so wishlist requeue has a real signal # for album-vs-single routing instead of relying on per-track album dicts # that may have been mangled by reconstruction fallbacks. diff --git a/core/worker_utils.py b/core/worker_utils.py index e572c702..a8d965b9 100644 --- a/core/worker_utils.py +++ b/core/worker_utils.py @@ -78,3 +78,67 @@ def set_album_api_track_count(cursor, album_id, count): logger.warning( "Failed to cache api_track_count for album %s: %s", album_id, e ) + + +# --- Enrichment "process this group first" override ----------------------- +# Each enrichment worker normally processes artist -> album -> track. A user +# can pin one entity type to run first via the Manage Enrichment Workers modal; +# the choice is stored in config as "_enrichment_priority" and read +# at the top of each worker's _get_next_item so it takes effect live. When the +# pinned group is exhausted (or unset), the worker falls back to its normal +# chain — so the default path is unchanged. + +PRIORITY_ENTITIES = ('artist', 'album', 'track') + + +def read_enrichment_priority(service: str) -> str: + """Return the pinned entity ('artist'|'album'|'track') for a worker, or ''. + + Read every loop so the override applies without restarting the worker. + Any error / unset / invalid value yields '' (no override).""" + try: + from config.settings import config_manager + val = (config_manager.get(f'{service}_enrichment_priority', '') or '') + val = str(val).strip().lower() + return val if val in PRIORITY_ENTITIES else '' + except Exception: + return '' + + +def priority_pending_item(cursor, service, entity, type_overrides=None): + """Return one pending (NULL match_status) item of `entity`, or None. + + `service` is the column prefix (e.g. 'spotify' -> spotify_match_status) and + MUST be a trusted worker-supplied literal (it is interpolated into SQL). + `type_overrides` maps the canonical entity to the worker's dispatch 'type' + string — Spotify/iTunes process individual items as 'album_individual' / + 'track_individual', the other workers use 'album' / 'track'. The returned + dict matches the shape those workers already return from _get_next_item.""" + if not str(service).isalpha() or entity not in PRIORITY_ENTITIES: + return None + type_overrides = type_overrides or {} + ms = f"{service}_match_status" + + if entity == 'artist': + cursor.execute( + f"SELECT id, name FROM artists WHERE {ms} IS NULL AND id IS NOT NULL " + f"ORDER BY id ASC LIMIT 1" + ) + r = cursor.fetchone() + return {'type': type_overrides.get('artist', 'artist'), 'id': r[0], 'name': r[1]} if r else None + + if entity == 'album': + cursor.execute( + f"SELECT a.id, a.title, ar.name FROM albums a JOIN artists ar ON a.artist_id = ar.id " + f"WHERE a.{ms} IS NULL AND a.id IS NOT NULL ORDER BY a.id ASC LIMIT 1" + ) + r = cursor.fetchone() + return {'type': type_overrides.get('album', 'album'), 'id': r[0], 'name': r[1], 'artist': r[2]} if r else None + + # track + cursor.execute( + f"SELECT t.id, t.title, ar.name FROM tracks t JOIN artists ar ON t.artist_id = ar.id " + f"WHERE t.{ms} IS NULL AND t.id IS NOT NULL ORDER BY t.id ASC LIMIT 1" + ) + r = cursor.fetchone() + return {'type': type_overrides.get('track', 'track'), 'id': r[0], 'name': r[1], 'artist': r[2]} if r else None diff --git a/database/music_database.py b/database/music_database.py index 85beef86..e978df0c 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -427,6 +427,9 @@ class MusicDatabase: # Add Amazon artist ID column (migration) self._add_amazon_columns(cursor) + # Add Similar-Artists worker tracking columns (migration) + self._add_similar_artists_worker_columns(cursor) + # Backfill match_status for rows that already have an external ID but # NULL status. Prevents enrichment workers from re-processing these # rows forever. Must run AFTER all *_match_status columns have been @@ -571,6 +574,7 @@ class MusicDatabase: # Add explored_at to mirrored_playlists (migration) self._add_mirrored_playlist_explored_column(cursor) + self._add_mirrored_playlist_organize_column(cursor) # Add notification columns to automations (migration) self._add_automation_notify_columns(cursor) @@ -956,9 +960,147 @@ class MusicDatabase: if album_cols and 'api_track_count' not in album_cols: cursor.execute("ALTER TABLE albums ADD COLUMN api_track_count INTEGER DEFAULT NULL") logger.info("Repaired missing api_track_count column on albums table") + + # Canonical album version (#765 / #767-Bug2). Additive + nullable: + # a NULL canonical means "unresolved" and every tool falls back to + # today's behavior, so this is safe to ship dormant. Columns are + # populated/consumed in later stages. + _canonical_cols = { + 'canonical_source': 'TEXT DEFAULT NULL', + 'canonical_album_id': 'TEXT DEFAULT NULL', + 'canonical_score': 'REAL DEFAULT NULL', + 'canonical_resolved_at': 'TIMESTAMP DEFAULT NULL', + } + for _col, _typedef in _canonical_cols.items(): + if album_cols and _col not in album_cols: + cursor.execute(f"ALTER TABLE albums ADD COLUMN {_col} {_typedef}") + logger.info("Added %s column to albums table (canonical version)", _col) except Exception as e: logger.error("Error repairing core media schema columns: %s", e) + def set_album_canonical(self, album_id, source: str, canonical_album_id: str, score: float) -> bool: + """Persist the resolved canonical (source, album_id, score) for an album + (#765 Stage 2). Returns True if a row was updated.""" + conn = self._get_connection() + try: + cursor = conn.cursor() + cursor.execute( + "UPDATE albums SET canonical_source = ?, canonical_album_id = ?, " + "canonical_score = ?, canonical_resolved_at = CURRENT_TIMESTAMP " + "WHERE id = ?", + (source, str(canonical_album_id), float(score), album_id), + ) + conn.commit() + return cursor.rowcount > 0 + except Exception as e: + logger.error("Error setting album canonical for %s: %s", album_id, e) + return False + finally: + conn.close() + + def get_album_canonical(self, album_id) -> Optional[dict]: + """Return ``{'source','album_id','score','resolved_at'}`` for an album's + pinned canonical release, or ``None`` when unresolved (#765 Stage 2). + Consumers treat ``None`` as 'fall back to today's behavior'.""" + conn = self._get_connection() + try: + cursor = conn.cursor() + cursor.execute( + "SELECT canonical_source, canonical_album_id, canonical_score, " + "canonical_resolved_at FROM albums WHERE id = ?", + (album_id,), + ) + row = cursor.fetchone() + if not row or not row[0] or not row[1]: + return None + return { + 'source': row[0], + 'album_id': row[1], + 'score': row[2], + 'resolved_at': row[3], + } + except Exception as e: + logger.error("Error reading album canonical for %s: %s", album_id, e) + return None + finally: + conn.close() + + def get_enrichment_unmatched( + self, + service: str, + entity_type: str, + status: str = 'not_found', + query: str = None, + limit: int = 50, + offset: int = 0, + ) -> dict: + """List items a given enrichment source hasn't matched, paginated. + + Powers the "Manage Enrichment Workers" modal's unmatched browser. + Returns ``{'total': int, 'items': [{id, name, image_url, status, + last_attempted}]}``. Raises ``UnmatchedQueryError`` for an unknown + service / unsupported entity type / bad status (the caller maps that to + an HTTP 400).""" + from core.enrichment.unmatched import ( + build_count_query, + build_unmatched_query, + ) + + sql, params = build_unmatched_query( + service, entity_type, status, query, limit, offset + ) + count_sql, count_params = build_count_query(service, entity_type, status, query) + conn = self._get_connection() + try: + cursor = conn.cursor() + total = cursor.execute(count_sql, count_params).fetchone()[0] + rows = cursor.execute(sql, params).fetchall() + items = [dict(row) for row in rows] + return {'total': total or 0, 'items': items} + finally: + conn.close() + + def get_enrichment_breakdown(self, service: str, entity_type: str) -> dict: + """Return ``{matched, not_found, pending, total}`` for a source/entity. + + The per-worker ``get_stats().progress`` lumps matched + not_found into a + single 'processed' count; this splits them so the modal can show the + real match rate. Raises ``UnmatchedQueryError`` on bad input.""" + from core.enrichment.unmatched import build_breakdown_query + + sql, params = build_breakdown_query(service, entity_type) + conn = self._get_connection() + try: + row = conn.cursor().execute(sql, params).fetchone() + if not row: + return {'matched': 0, 'not_found': 0, 'pending': 0, 'total': 0} + return { + 'matched': row[0] or 0, + 'not_found': row[1] or 0, + 'pending': row[2] or 0, + 'total': row[3] or 0, + } + finally: + conn.close() + + def reset_enrichment(self, service: str, entity_type: str, scope: str = 'item', entity_id=None) -> int: + """Re-queue item(s) for a source by clearing match_status back to NULL. + + scope='item' resets one row (entity_id); scope='failed' resets every + 'not_found' row for that entity type. Returns the number of rows reset. + Raises ``UnmatchedQueryError`` on bad input.""" + from core.enrichment.unmatched import build_reset_query + + sql, params = build_reset_query(service, entity_type, scope, entity_id) + conn = self._get_connection() + try: + cursor = conn.cursor() + cursor.execute(sql, params) + conn.commit() + return cursor.rowcount or 0 + finally: + conn.close() + def _add_mirrored_playlist_explored_column(self, cursor): """Add explored_at column to mirrored_playlists to persist explore badge.""" try: @@ -970,6 +1112,19 @@ class MusicDatabase: except Exception as e: logger.error(f"Error adding explored_at column to mirrored_playlists: {e}") + def _add_mirrored_playlist_organize_column(self, cursor): + """Add organize_by_playlist preference for playlist-folder downloads.""" + try: + cursor.execute("PRAGMA table_info(mirrored_playlists)") + cols = [c[1] for c in cursor.fetchall()] + if 'organize_by_playlist' not in cols: + cursor.execute( + "ALTER TABLE mirrored_playlists ADD COLUMN organize_by_playlist INTEGER NOT NULL DEFAULT 0" + ) + logger.info("Added organize_by_playlist column to mirrored_playlists table") + except Exception as e: + logger.error(f"Error adding organize_by_playlist column to mirrored_playlists: {e}") + def _add_automation_notify_columns(self, cursor): """Add notification and result columns to automations table.""" try: @@ -2279,6 +2434,28 @@ class MusicDatabase: except Exception as e: logger.error(f"Error adding Discogs columns: {e}") + def _add_similar_artists_worker_columns(self, cursor): + """Add Similar-Artists worker tracking columns to the artists table. + + Mirrors the per-source enrichment pattern: a match_status (NULL = + unattempted, then 'matched'/'not_found'/'error') + last_attempted + timestamp so the SimilarArtistsWorker can pick the next library artist to + fetch MusicMap similars for and retry transient failures after a window. + Idempotent — only adds columns that aren't already present. + """ + try: + cursor.execute("PRAGMA table_info(artists)") + artists_columns = [column[1] for column in cursor.fetchall()] + + if 'similar_artists_match_status' not in artists_columns: + cursor.execute("ALTER TABLE artists ADD COLUMN similar_artists_match_status TEXT") + if 'similar_artists_last_attempted' not in artists_columns: + cursor.execute("ALTER TABLE artists ADD COLUMN similar_artists_last_attempted TIMESTAMP") + + cursor.execute("CREATE INDEX IF NOT EXISTS idx_artists_similarartists_status ON artists (similar_artists_match_status)") + except Exception as e: + logger.error(f"Error adding similar-artists worker columns: {e}") + def _add_amazon_columns(self, cursor): """Add Amazon enrichment tracking columns to artists, albums, and tracks.""" try: @@ -7248,6 +7425,22 @@ class MusicDatabase: # Titles differ in length by more than 30% — penalize heavily best_title_similarity *= len_ratio + # Word-level guard: SequenceMatcher's char ratio over-credits + # different songs that share a long substring or only a stopword + # ("Dani California" vs "Californication" = 0.67; "Under The Bridge" + # vs "Around the World" = 0.62). Since a same-artist comparison + # always scores artist = 1.0, the title is the only discriminator, + # so a bad-but-moderate title score gets carried over the threshold + # (#769). Reject pairs that aren't near-identical AND share no + # significant word — the real track is then reported missing. + from core.text.title_match import titles_plausibly_same + if not titles_plausibly_same( + clean_search_title or search_title_norm, + clean_db_title or db_title_norm, + best_title_similarity, + ): + return best_title_similarity * 0.5 # below any threshold + # Require minimum title similarity to prevent a perfect artist match from # carrying a bad title match over the threshold (e.g. "Time" vs "Time Flies") if best_title_similarity < 0.6: @@ -9119,6 +9312,70 @@ class MusicDatabase: logger.error(f"Error getting top similar artists: {e}") return [] + def get_recommendation_sources( + self, + similar_artist_names: List[str], + profile_id: int = 1, + max_per: int = 6, + ) -> Dict[str, List[str]]: + """The 'because you have X, Y, Z' explanation behind each recommendation. + + For each name in ``similar_artist_names``, return the display names of the + user's OWN artists (library or watchlist) that list it as a similar + artist. ``similar_artists.source_artist_id`` is a polymorphic provider id + (the spotify / itunes / deezer / musicbrainz id of one of the user's + artists), so we resolve it back to a name by matching against every + provider-id column on ``artists`` and ``watchlist_artists``. + + Returns ``{similar_artist_name: [source_name, ...]}`` — deduped, + name-sorted, capped at ``max_per`` per recommendation. Names with no + resolvable source are omitted from the dict. + """ + names = [n for n in (similar_artist_names or []) if n] + if not names: + return {} + try: + with self._get_connection() as conn: + cursor = conn.cursor() + placeholders = ','.join('?' for _ in names) + cursor.execute(f""" + SELECT sa.similar_artist_name AS rec_name, + COALESCE(a.name, wa.artist_name) AS source_name + FROM similar_artists sa + LEFT JOIN artists a ON ( + (a.spotify_artist_id IS NOT NULL AND a.spotify_artist_id = sa.source_artist_id) + OR (a.itunes_artist_id IS NOT NULL AND a.itunes_artist_id = sa.source_artist_id) + OR (a.deezer_id IS NOT NULL AND a.deezer_id = sa.source_artist_id) + OR (a.musicbrainz_id IS NOT NULL AND a.musicbrainz_id = sa.source_artist_id) + ) + LEFT JOIN watchlist_artists wa ON ( + wa.profile_id = ? AND ( + (wa.spotify_artist_id IS NOT NULL AND wa.spotify_artist_id = sa.source_artist_id) + OR (wa.itunes_artist_id IS NOT NULL AND wa.itunes_artist_id = sa.source_artist_id) + OR (wa.deezer_artist_id IS NOT NULL AND wa.deezer_artist_id = sa.source_artist_id) + OR (wa.musicbrainz_artist_id IS NOT NULL AND wa.musicbrainz_artist_id = sa.source_artist_id) + ) + ) + WHERE sa.profile_id = ? AND sa.similar_artist_name IN ({placeholders}) + """, (profile_id, profile_id, *names)) + + # Collect distinct source names per recommendation, preserving + # nothing-special order then sorting for a deterministic result. + buckets: Dict[str, set] = {} + for row in cursor.fetchall(): + src = row['source_name'] + if not src: + continue + buckets.setdefault(row['rec_name'], set()).add(src) + + return { + rec: sorted(srcs, key=lambda s: s.lower())[:max_per] + for rec, srcs in buckets.items() + } + except Exception as e: + logger.error(f"Error resolving recommendation sources: {e}") + return {} + def mark_artists_featured(self, artist_names: List[str]): """Update last_featured timestamp for artists shown in the hero slider""" if not artist_names: @@ -12171,7 +12428,11 @@ class MusicDatabase: WHERE profile_id = ? ORDER BY updated_at DESC """, (profile_id,)) - return [dict(row) for row in cursor.fetchall()] + return [ + self._normalize_mirrored_playlist_row(row) + for row in cursor.fetchall() + if row + ] except Exception as e: logger.error(f"Error getting mirrored playlists: {e}") return [] @@ -12198,11 +12459,93 @@ class MusicDatabase: cursor = conn.cursor() cursor.execute("SELECT * FROM mirrored_playlists WHERE id = ?", (playlist_id,)) row = cursor.fetchone() - return dict(row) if row else None + return self._normalize_mirrored_playlist_row(row) except Exception as e: logger.error(f"Error getting mirrored playlist: {e}") return None + @staticmethod + def _normalize_mirrored_playlist_row(row) -> Optional[Dict]: + if not row: + return None + pl = dict(row) + pl['organize_by_playlist'] = bool(pl.get('organize_by_playlist', 0)) + return pl + + def get_mirrored_playlist_by_source( + self, + source: str, + source_playlist_id: str, + profile_id: int = 1, + ) -> Optional[Dict]: + """Return a mirrored playlist by upstream source id.""" + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute( + """ + SELECT * FROM mirrored_playlists + WHERE source = ? AND source_playlist_id = ? AND profile_id = ? + """, + (source, str(source_playlist_id), profile_id), + ) + row = cursor.fetchone() + return self._normalize_mirrored_playlist_row(row) + except Exception as e: + logger.error(f"Error getting mirrored playlist by source: {e}") + return None + + def resolve_mirrored_playlist( + self, + playlist_ref: Any, + profile_id: int = 1, + *, + default_source: str = 'spotify', + ) -> Optional[Dict]: + """Resolve a mirrored playlist from an upstream source id or numeric PK. + + Resolves by ``(source, source_playlist_id)`` FIRST, then falls back to + treating an all-digit ref as the mirrored-playlists primary key. The + order matters: some sources (e.g. Deezer) use all-numeric upstream ids, + and the old PK-first logic mistook those for the PK — so the Deezer + organize-by-playlist toggle resolved the wrong row (or nothing). + """ + if playlist_ref is None or playlist_ref == '': + return None + ref = str(playlist_ref).strip() + if not ref: + return None + if default_source: + row = self.get_mirrored_playlist_by_source(default_source, ref, profile_id) + if row: + return row + if ref.isdigit(): + return self.get_mirrored_playlist(int(ref)) + return None + + def set_mirrored_playlist_organize_by_playlist( + self, + playlist_id: int, + enabled: bool, + ) -> bool: + """Persist whether downloads for this playlist use playlist-folder layout.""" + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute( + """ + UPDATE mirrored_playlists + SET organize_by_playlist = ?, updated_at = CURRENT_TIMESTAMP + WHERE id = ? + """, + (1 if enabled else 0, playlist_id), + ) + conn.commit() + return cursor.rowcount > 0 + except Exception as e: + logger.error(f"Error updating organize_by_playlist for playlist {playlist_id}: {e}") + return False + def get_mirrored_playlist_tracks(self, playlist_id: int) -> List[Dict]: """Return all tracks for a mirrored playlist ordered by position.""" try: diff --git a/requirements.txt b/requirements.txt index bc1ade54..148d3c0c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -65,3 +65,13 @@ tidalapi==0.8.11 flask-socketio==5.6.1 gunicorn==26.0.0 simple-websocket==1.1.0 + +# Full public-playlist link imports (no Spotify credentials). The "Spotify link" +# tab scrapes Spotify's embed widget, which caps at ~100 tracks; SpotipyFree +# (the no-creds spotipy drop-in spotDL uses) pulls the full list. It is GPL-3.0, +# used here as a normal pip dependency — aggregation, exactly like spotDL (also +# MIT). It is NOT vendored into SoulSync's own code, so the project stays MIT. +# The code soft-imports it and falls back to the embed scraper (~100 tracks) if +# it's missing or fails, so this is never a hard runtime requirement. +spotipyFree>=1.1.2,<2 +websockets>=12 # required by SpotipyFree/spotapi, not pulled in automatically diff --git a/services/sync_service.py b/services/sync_service.py index e51980ef..072185d4 100644 --- a/services/sync_service.py +++ b/services/sync_service.py @@ -115,6 +115,12 @@ class PlaylistSyncService: logger.error("Navidrome client not provided to sync service") return None, "navidrome" return client, "navidrome" + elif active_server == "soulsync": + client = self._media_client('soulsync') + if not client: + logger.error("SoulSync library client not provided to sync service") + return None, "soulsync" + return client, "soulsync" else: # Default to Plex client = self._media_client('plex') if profile_id and client: @@ -292,63 +298,92 @@ class PlaylistSyncService: return self._create_error_result(playlist.name, ["Sync cancelled"]) media_client, server_type = self._get_active_media_client() - self._update_progress(playlist.name, f"Creating/updating {server_type.title()} playlist", "", 80, 5, 4, - total_tracks=total_tracks, - matched_tracks=len(matched_tracks), - failed_tracks=len(unmatched_tracks)) - - # Get the actual media server track objects - media_tracks = [r.plex_track for r in matched_tracks if r.plex_track] # plex_track is a generic name here - logger.info(f"Creating playlist with {len(media_tracks)} matched tracks") + unmatched_count = len(unmatched_tracks) - # Validate that all tracks have proper ratingKey attributes for playlist creation - valid_tracks = [] - for i, track in enumerate(media_tracks): - if track and hasattr(track, 'ratingKey'): - valid_tracks.append(track) - logger.debug(f"Track {i+1} valid for playlist: '{track.title}' (ratingKey: {track.ratingKey})") - else: - logger.warning(f"Track {i+1} invalid for playlist: {track} (type: {type(track)}, has ratingKey: {hasattr(track, 'ratingKey') if track else 'N/A'})") - - logger.info(f"Playlist validation: {len(valid_tracks)}/{len(media_tracks)} tracks are valid {server_type.title()} objects with ratingKeys") - - # Deduplicate by ratingKey — media servers silently drop duplicates, - # so count should reflect what actually ends up in the playlist - seen_keys = set() - deduped_tracks = [] - for t in valid_tracks: - if t.ratingKey not in seen_keys: - seen_keys.add(t.ratingKey) - deduped_tracks.append(t) - if len(deduped_tracks) < len(valid_tracks): - logger.info(f"Deduplicated {len(valid_tracks) - len(deduped_tracks)} duplicate ratingKeys ({len(valid_tracks)} → {len(deduped_tracks)} tracks)") - - # Use the deduplicated tracks for the sync - plex_tracks = deduped_tracks - - # Use active media server for playlist sync - media_client, server_type = self._get_active_media_client() - if not media_client: - logger.error("No active media client available for playlist sync") - sync_success = False - else: - logger.info( - f"Syncing playlist '{playlist.name}' to {server_type.upper()} server " - f"(mode: {sync_mode})" + # SoulSync standalone has no server playlists — only library match + + # wishlist for missing files. Previously we fell through to Plex here, + # showed "Creating/updating Plex playlist", playlist write failed, and + # failed_tracks was computed as total - 0 synced (= entire playlist). + if server_type == 'soulsync': + self._update_progress( + playlist.name, + "Standalone: library check complete", + "", + 80, 5, 4, + total_tracks=total_tracks, + matched_tracks=len(matched_tracks), + failed_tracks=unmatched_count, ) - # sync_mode == 'append' preserves user-added tracks on the server - # playlist (CJFC discord report) — never deletes, only adds new - # ones via the per-server `append_to_playlist`. The sync UI - # hides the Sync button entirely on SoulSync standalone (which - # has no playlist methods), so every client that reaches this - # point implements both methods. - if sync_mode == 'append': - sync_success = media_client.append_to_playlist(playlist.name, valid_tracks) + sync_success = True + synced_tracks = len(matched_tracks) + failed_tracks = unmatched_count + logger.info( + f"Standalone playlist sync '{playlist.name}': " + f"{len(matched_tracks)} in library, {unmatched_count} missing" + ) + else: + self._update_progress( + playlist.name, + f"Creating/updating {server_type.title()} playlist", + "", + 80, 5, 4, + total_tracks=total_tracks, + matched_tracks=len(matched_tracks), + failed_tracks=unmatched_count, + ) + + # Get the actual media server track objects + media_tracks = [r.plex_track for r in matched_tracks if r.plex_track] + logger.info(f"Creating playlist with {len(media_tracks)} matched tracks") + + # Validate that all tracks have proper ratingKey attributes for playlist creation + valid_tracks = [] + for i, track in enumerate(media_tracks): + if track and hasattr(track, 'ratingKey'): + valid_tracks.append(track) + logger.debug(f"Track {i+1} valid for playlist: '{track.title}' (ratingKey: {track.ratingKey})") + else: + logger.warning( + f"Track {i+1} invalid for playlist: {track} " + f"(type: {type(track)}, has ratingKey: {hasattr(track, 'ratingKey') if track else 'N/A'})" + ) + + logger.info( + f"Playlist validation: {len(valid_tracks)}/{len(media_tracks)} tracks are valid " + f"{server_type.title()} objects with ratingKeys" + ) + + # Deduplicate by ratingKey — media servers silently drop duplicates + seen_keys = set() + deduped_tracks = [] + for t in valid_tracks: + if t.ratingKey not in seen_keys: + seen_keys.add(t.ratingKey) + deduped_tracks.append(t) + if len(deduped_tracks) < len(valid_tracks): + logger.info( + f"Deduplicated {len(valid_tracks) - len(deduped_tracks)} duplicate ratingKeys " + f"({len(valid_tracks)} → {len(deduped_tracks)} tracks)" + ) + + plex_tracks = deduped_tracks + + if not media_client: + logger.error("No active media client available for playlist sync") + sync_success = False else: - sync_success = media_client.update_playlist(playlist.name, valid_tracks) - - synced_tracks = len(plex_tracks) if sync_success else 0 - failed_tracks = len(playlist.tracks) - synced_tracks - downloaded_tracks + logger.info( + f"Syncing playlist '{playlist.name}' to {server_type.upper()} server " + f"(mode: {sync_mode})" + ) + if sync_mode == 'append': + sync_success = media_client.append_to_playlist(playlist.name, valid_tracks) + else: + sync_success = media_client.update_playlist(playlist.name, valid_tracks) + + synced_tracks = len(plex_tracks) if sync_success else 0 + # Not in library (for wishlist), not "total minus playlist size". + failed_tracks = unmatched_count self._update_progress(playlist.name, "Sync completed", "", 100, 5, 5, total_tracks=total_tracks, @@ -357,8 +392,11 @@ class PlaylistSyncService: # Auto-add unmatched tracks to wishlist (skip in Wing It mode) wishlist_added_count = 0 - if unmatched_tracks and getattr(self, '_skip_wishlist', False): - logger.info(f"[Wing It] Skipping wishlist for {len(unmatched_tracks)} unmatched tracks") + if unmatched_tracks and getattr(self, '_skip_unmatched_wishlist', False): + logger.info( + "Skipping sync-time wishlist for %s unmatched tracks (wing-it or organize-by-playlist)", + len(unmatched_tracks), + ) unmatched_tracks = [] # Clear so the loop below doesn't run if unmatched_tracks: try: @@ -548,20 +586,13 @@ class PlaylistSyncService: server_track_id = cached['server_track_id'] db_track_check = cache_db.get_track_by_id(server_track_id) if db_track_check: - if server_type == "jellyfin": - class JellyfinTrackFromCache: + if server_type in ("jellyfin", "navidrome", "soulsync"): + class DbTrackFromCache: def __init__(self, db_t): self.ratingKey = db_t.id self.title = db_t.title self.id = db_t.id - actual_track = JellyfinTrackFromCache(db_track_check) - elif server_type == "navidrome": - class NavidromeTrackFromCache: - def __init__(self, db_t): - self.ratingKey = db_t.id - self.title = db_t.title - self.id = db_t.id - actual_track = NavidromeTrackFromCache(db_track_check) + actual_track = DbTrackFromCache(db_track_check) else: try: actual_track = media_client.server.fetchItem(int(server_track_id)) @@ -586,17 +617,35 @@ class PlaylistSyncService: artist_name = _artist_name(artist) + # YouTube/streaming sources arrive video-shaped: title + # "Artist - Song", artist "Official Artist"/"Artist - Topic"/ + # "ArtistVEVO". Try the raw (title, artist) first, then a + # canonicalized variant so those still match the clean library + # metadata instead of being reported missing (#768). + from core.text.source_title import canonical_source_track + _attempts = [(original_title, artist_name)] + _canon_title, _canon_artist = canonical_source_track(original_title, artist_name) + if (_canon_title, _canon_artist) != (original_title, artist_name): + _attempts.append((_canon_title, _canon_artist)) + # Use the improved database check_track_exists method with server awareness try: db = MusicDatabase() - artist_candidates = self._get_or_fetch_artist_candidates( - candidate_pool, db, artist_name, active_server, - ) - db_track, confidence = db.check_track_exists( - original_title, artist_name, - confidence_threshold=0.7, server_source=active_server, - candidate_tracks=artist_candidates, - ) + # Try every candidate form and keep the BEST — don't stop at + # the first that clears the threshold, or a marginal raw + # match could mask a stronger (correct) canonical one. + db_track, confidence = None, 0.0 + for _try_title, _try_artist in _attempts: + artist_candidates = self._get_or_fetch_artist_candidates( + candidate_pool, db, _try_artist, active_server, + ) + _cand_track, _cand_conf = db.check_track_exists( + _try_title, _try_artist, + confidence_threshold=0.7, server_source=active_server, + candidate_tracks=artist_candidates, + ) + if _cand_conf > confidence: + db_track, confidence = _cand_track, _cand_conf if db_track and confidence >= 0.7: logger.debug(f"Database match found for '{original_title}' by '{artist_name}': '{db_track.title}' with confidence {confidence:.2f}") @@ -615,27 +664,19 @@ class PlaylistSyncService: # Fetch the actual track object from active media server using the database track ID try: - if server_type == "jellyfin": - # For Jellyfin, create a track object from database info (Jellyfin doesn't have fetchItem) - class JellyfinTrackFromDB: - def __init__(self, db_track): - self.ratingKey = db_track.id - self.title = db_track.title - self.id = db_track.id - - actual_track = JellyfinTrackFromDB(db_track) - logger.debug(f"Created Jellyfin track object for '{db_track.title}' (ID: {actual_track.ratingKey})") - return actual_track, confidence - elif server_type == "navidrome": - # For Navidrome, create a track object from database info (similar to Jellyfin) - class NavidromeTrackFromDB: + if server_type in ("jellyfin", "navidrome", "soulsync"): + # DB-backed servers — no remote fetchItem (SoulSync standalone included). + class DbTrackFromDB: def __init__(self, db_track): self.ratingKey = db_track.id self.title = db_track.title self.id = db_track.id - actual_track = NavidromeTrackFromDB(db_track) - logger.debug(f"Created Navidrome track object for '{db_track.title}' (ID: {actual_track.ratingKey})") + actual_track = DbTrackFromDB(db_track) + logger.debug( + f"Created {server_type} track object for '{db_track.title}' " + f"(ID: {actual_track.ratingKey})" + ) return actual_track, confidence else: # For Plex, use the original fetchItem approach diff --git a/tests/automation/test_handler_registration.py b/tests/automation/test_handler_registration.py index b6660541..357ebc06 100644 --- a/tests/automation/test_handler_registration.py +++ b/tests/automation/test_handler_registration.py @@ -126,6 +126,8 @@ def _build_deps(engine, scan_mgr=None) -> AutomationDeps: get_watchlist_scan_state=lambda: {}, run_playlist_discovery_worker=lambda *a, **k: None, run_sync_task=lambda *a, **k: None, + run_playlist_organize_download=lambda **k: {'status': 'skipped'}, + missing_download_executor=None, load_sync_status_file=lambda: {}, get_deezer_client=lambda: None, parse_youtube_playlist=lambda url: None, diff --git a/tests/automation/test_handlers_maintenance.py b/tests/automation/test_handlers_maintenance.py index a3844b77..e9c62a05 100644 --- a/tests/automation/test_handlers_maintenance.py +++ b/tests/automation/test_handlers_maintenance.py @@ -73,6 +73,8 @@ def _build_deps(**overrides) -> AutomationDeps: get_watchlist_scan_state=lambda: {}, run_playlist_discovery_worker=lambda *a, **k: None, run_sync_task=lambda *a, **k: None, + run_playlist_organize_download=lambda **k: {'status': 'skipped'}, + missing_download_executor=None, load_sync_status_file=lambda: {}, get_deezer_client=lambda: None, parse_youtube_playlist=lambda url: None, diff --git a/tests/automation/test_handlers_personalized_pipeline.py b/tests/automation/test_handlers_personalized_pipeline.py index 049cd466..af91bb03 100644 --- a/tests/automation/test_handlers_personalized_pipeline.py +++ b/tests/automation/test_handlers_personalized_pipeline.py @@ -47,6 +47,8 @@ def _build_deps(**overrides) -> AutomationDeps: get_watchlist_scan_state=lambda: {}, run_playlist_discovery_worker=lambda *a, **k: None, run_sync_task=lambda *a, **k: None, + run_playlist_organize_download=lambda **k: {'status': 'skipped'}, + missing_download_executor=None, load_sync_status_file=lambda: {}, get_deezer_client=lambda: None, parse_youtube_playlist=lambda url: None, diff --git a/tests/automation/test_handlers_playlist.py b/tests/automation/test_handlers_playlist.py index ff8b132b..a690d7b6 100644 --- a/tests/automation/test_handlers_playlist.py +++ b/tests/automation/test_handlers_playlist.py @@ -108,6 +108,8 @@ def _build_deps(**overrides) -> AutomationDeps: get_watchlist_scan_state=lambda: {}, run_playlist_discovery_worker=lambda *a, **k: None, run_sync_task=lambda *a, **k: None, + run_playlist_organize_download=lambda **k: {'status': 'skipped'}, + missing_download_executor=None, load_sync_status_file=lambda: {}, get_deezer_client=lambda: None, parse_youtube_playlist=lambda url: None, @@ -737,8 +739,38 @@ class TestSyncPlaylist: time.sleep(0.01) assert len(sync_calls) == 1 + def test_organize_by_playlist_passes_skip_wishlist_add(self): + discovered_track = { + 'extra_data': json.dumps({ + 'discovered': True, + 'matched_data': { + 'id': 'spot-1', 'name': 'Track', 'artists': [{'name': 'X'}], + 'album': {'name': 'Album'}, 'duration_ms': 200000, + }, + }), + 'artist_name': 'X', + } + db = _StubDB( + playlists=[{'id': 1, 'name': 'P', 'organize_by_playlist': True}], + playlist_tracks={1: [discovered_track]}, + ) + sync_calls: List[tuple] = [] + deps = _build_deps( + get_database=lambda: db, + run_sync_task=lambda *a, **k: sync_calls.append((a, k)), + ) + auto_sync_playlist({'playlist_id': '1'}, deps) + for _ in range(50): + if sync_calls: + break + import time + time.sleep(0.01) + assert sync_calls + assert sync_calls[0][1].get('skip_wishlist_add') is True + def test_unchanged_since_last_sync_returns_skipped(self): discovered_track = { + 'source_track_id': 'spot-1', 'extra_data': json.dumps({ 'discovered': True, 'matched_data': { @@ -758,7 +790,11 @@ class TestSyncPlaylist: import hashlib expected_hash = hashlib.md5('spot-1'.encode()).hexdigest() sync_statuses = { - 'auto_mirror_1': {'tracks_hash': expected_hash, 'matched_tracks': 1} + 'auto_mirror_1': { + 'tracks_hash': expected_hash, + 'mirror_tracks_hash': expected_hash, + 'matched_tracks': 1, + } } deps = _build_deps( @@ -769,6 +805,96 @@ class TestSyncPlaylist: assert result['status'] == 'skipped' assert 'unchanged' in result['reason'] + def test_playlist_changed_event_bypasses_unchanged_skip(self): + discovered_track = { + 'source_track_id': 'spot-1', + 'extra_data': json.dumps({ + 'discovered': True, + 'matched_data': { + 'id': 'spot-1', 'name': 'T', 'artists': [{'name': 'X'}], + 'album': {'name': 'A'}, 'duration_ms': 0, + }, + }), + 'artist_name': 'X', + } + db = _StubDB( + playlists=[{'id': 1, 'name': 'P'}], + playlist_tracks={1: [discovered_track]}, + ) + import hashlib + expected_hash = hashlib.md5('spot-1'.encode()).hexdigest() + sync_statuses = { + 'auto_mirror_1': { + 'tracks_hash': expected_hash, + 'mirror_tracks_hash': expected_hash, + 'matched_tracks': 1, + } + } + sync_calls: List[tuple] = [] + deps = _build_deps( + get_database=lambda: db, + load_sync_status_file=lambda: sync_statuses, + run_sync_task=lambda *a, **k: sync_calls.append((a, k)), + ) + result = auto_sync_playlist( + {'playlist_id': '1', '_event_data': {'added': '1', 'playlist_id': '1'}}, + deps, + ) + assert result['status'] == 'started' + import time + for _ in range(50): + if sync_calls: + break + time.sleep(0.01) + assert len(sync_calls) == 1 + + def test_new_mirror_row_with_skipped_track_bypasses_unchanged_skip(self): + """New playlist row without discovery must not reuse the old tracks_hash skip.""" + discovered_track = { + 'source_track_id': 'spot-1', + 'extra_data': json.dumps({ + 'discovered': True, + 'matched_data': { + 'id': 'spot-1', 'name': 'T', 'artists': [{'name': 'X'}], + 'album': {'name': 'A'}, 'duration_ms': 0, + }, + }), + 'artist_name': 'X', + } + db = _StubDB( + playlists=[{'id': 1, 'name': 'P'}], + playlist_tracks={1: [discovered_track, {}]}, # second row not syncable + ) + import hashlib + old_hash = hashlib.md5('spot-1'.encode()).hexdigest() + new_mirror_hash = hashlib.md5('spot-1,spot-new'.encode()).hexdigest() + sync_statuses = { + 'auto_mirror_1': { + 'tracks_hash': old_hash, + 'mirror_tracks_hash': old_hash, + 'matched_tracks': 1, + } + } + # Give the new mirror row a source id so mirror hash changes. + db.playlist_tracks[1][1]['source_track_id'] = 'spot-new' + + sync_calls: List[tuple] = [] + deps = _build_deps( + get_database=lambda: db, + load_sync_status_file=lambda: sync_statuses, + run_sync_task=lambda *a, **k: sync_calls.append((a, k)), + ) + result = auto_sync_playlist({'playlist_id': '1'}, deps) + assert result['status'] == 'started' + assert result['skipped_tracks'] == '1' + import time + for _ in range(50): + if sync_calls: + break + time.sleep(0.01) + assert len(sync_calls) == 1 + assert new_mirror_hash != old_hash + # ─── playlist_pipeline ─────────────────────────────────────────────── diff --git a/tests/automation/test_handlers_simple.py b/tests/automation/test_handlers_simple.py index 5e535884..9320b96d 100644 --- a/tests/automation/test_handlers_simple.py +++ b/tests/automation/test_handlers_simple.py @@ -60,6 +60,8 @@ def _build_deps(**overrides: Any) -> AutomationDeps: get_watchlist_scan_state=lambda: {}, run_playlist_discovery_worker=lambda *a, **k: None, run_sync_task=lambda *a, **k: None, + run_playlist_organize_download=lambda **k: {'status': 'skipped'}, + missing_download_executor=None, load_sync_status_file=lambda: {}, get_deezer_client=lambda: None, parse_youtube_playlist=lambda url: None, diff --git a/tests/automation/test_playlist_pipeline_folder_mode.py b/tests/automation/test_playlist_pipeline_folder_mode.py new file mode 100644 index 00000000..9c412fce --- /dev/null +++ b/tests/automation/test_playlist_pipeline_folder_mode.py @@ -0,0 +1,117 @@ +"""Tests for organize-by-playlist integration in the playlist pipeline tail.""" + +from unittest.mock import MagicMock + +from core.automation.deps import AutomationDeps, AutomationState +from core.automation.handlers._pipeline_shared import run_sync_and_wishlist +import threading + + +def _minimal_deps(**overrides): + base = dict( + engine=MagicMock(), + state=AutomationState(), + config_manager=MagicMock(), + update_progress=lambda *a, **k: None, + logger=MagicMock(), + get_database=lambda: MagicMock(), + spotify_client=None, + tidal_client=None, + web_scan_manager=None, + process_wishlist_automatically=lambda **k: None, + process_watchlist_scan_automatically=lambda **k: None, + is_wishlist_actually_processing=lambda: False, + is_watchlist_actually_scanning=lambda: False, + get_watchlist_scan_state=lambda: {}, + run_playlist_discovery_worker=lambda *a, **k: None, + run_sync_task=lambda *a, **k: None, + run_playlist_organize_download=lambda **k: {'status': 'started', 'batch_id': 'b1'}, + missing_download_executor=None, + load_sync_status_file=lambda: {}, + get_deezer_client=lambda: None, + parse_youtube_playlist=lambda url: None, + get_sync_states=lambda: {}, + set_db_update_automation_id=lambda v: None, + get_db_update_state=lambda: {}, + db_update_lock=threading.Lock(), + db_update_executor=None, + run_db_update_task=lambda *a, **k: None, + run_deep_scan_task=lambda *a, **k: None, + get_duplicate_cleaner_state=lambda: {}, + duplicate_cleaner_lock=threading.Lock(), + duplicate_cleaner_executor=None, + run_duplicate_cleaner=lambda: None, + get_quality_scanner_state=lambda: {}, + quality_scanner_lock=threading.Lock(), + quality_scanner_executor=None, + run_quality_scanner=lambda *a, **k: None, + download_orchestrator=None, + run_async=lambda coro: None, + tasks_lock=threading.Lock(), + get_download_batches=lambda: {}, + get_download_tasks=lambda: {}, + sweep_empty_download_directories=lambda: 0, + get_staging_path=lambda: '/staging', + docker_resolve_path=lambda p: p, + get_current_profile_id=lambda: 1, + get_watchlist_scanner=lambda spc: None, + get_app=lambda: None, + get_beatport_data_cache=lambda: {'cache_lock': threading.Lock(), 'homepage': {}}, + init_automation_progress=lambda *a, **k: None, + record_progress_history=lambda *a, **k: None, + build_personalized_manager=lambda: None, + ) + base.update(overrides) + return AutomationDeps(**base) # type: ignore[arg-type] + + +def test_all_organize_playlists_skips_wishlist(): + wishlist_calls = [] + + def sync_one(_pl): + return {'status': 'skipped', 'reason': 'unchanged'} + + deps = _minimal_deps( + process_wishlist_automatically=lambda **k: wishlist_calls.append(k), + ) + playlists = [ + {'id': 1, 'name': 'A', 'organize_by_playlist': True}, + ] + result = run_sync_and_wishlist( + deps, + 'auto-1', + playlists, + sync_one_fn=sync_one, + sync_id_for_fn=lambda pl: f'mirror_{pl["id"]}', + skip_wishlist=False, + ) + assert result['wishlist_queued'] == 0 + assert result['organize_downloads_started'] == 1 + assert wishlist_calls == [] + + +def test_mixed_playlists_still_runs_wishlist(): + wishlist_calls = [] + + deps = _minimal_deps( + process_wishlist_automatically=lambda **k: wishlist_calls.append(1), + is_wishlist_actually_processing=lambda: False, + ) + playlists = [ + {'id': 1, 'name': 'Organized', 'organize_by_playlist': True}, + {'id': 2, 'name': 'Normal', 'organize_by_playlist': False}, + ] + + def sync_one(_pl): + return {'status': 'skipped'} + + result = run_sync_and_wishlist( + deps, + None, + playlists, + sync_one_fn=sync_one, + sync_id_for_fn=lambda pl: f'mirror_{pl["id"]}', + ) + assert result['organize_downloads_started'] == 1 + assert result['wishlist_queued'] == 1 + assert len(wishlist_calls) == 1 diff --git a/tests/automation/test_progress_callbacks.py b/tests/automation/test_progress_callbacks.py index 10456bfd..16728255 100644 --- a/tests/automation/test_progress_callbacks.py +++ b/tests/automation/test_progress_callbacks.py @@ -46,6 +46,8 @@ def _build_deps(**overrides) -> AutomationDeps: get_watchlist_scan_state=lambda: {}, run_playlist_discovery_worker=lambda *a, **k: None, run_sync_task=lambda *a, **k: None, + run_playlist_organize_download=lambda **k: {'status': 'skipped'}, + missing_download_executor=None, load_sync_status_file=lambda: {}, get_deezer_client=lambda: None, parse_youtube_playlist=lambda url: None, diff --git a/tests/discovery/test_discovery_sync.py b/tests/discovery/test_discovery_sync.py index 1ac44d76..2a123804 100644 --- a/tests/discovery/test_discovery_sync.py +++ b/tests/discovery/test_discovery_sync.py @@ -20,6 +20,7 @@ class _FakeSyncResult: failed_tracks: int = 1 synced_tracks: int = 4 total_tracks: int = 6 + wishlist_added_count: int = 0 match_details: list = None def __post_init__(self): @@ -139,6 +140,9 @@ def _build_deps( update_automation_progress=None, update_and_save_sync_status=None, run_async=None, + process_wishlist_automatically=None, + run_playlist_organize_download=None, + is_wishlist_actually_processing=None, ): return ds.SyncDeps( config_manager=config or _FakeConfig(), @@ -154,6 +158,9 @@ def _build_deps( update_and_save_sync_status=update_and_save_sync_status or (lambda *a, **kw: None), sync_states=sync_states if sync_states is not None else {}, sync_lock=sync_lock or threading.Lock(), + process_wishlist_automatically=process_wishlist_automatically, + run_playlist_organize_download=run_playlist_organize_download, + is_wishlist_actually_processing=is_wishlist_actually_processing, ) @@ -445,6 +452,64 @@ def test_update_and_save_sync_status_called(patched_db): assert kwargs.get('tracks_hash') # md5 hash present +# --------------------------------------------------------------------------- +# Post-sync automation follow-up +# --------------------------------------------------------------------------- + +def test_post_sync_triggers_wishlist_processor_for_mirror_automation(patched_db): + wishlist_calls = [] + result = _FakeSyncResult( + matched_tracks=5, + failed_tracks=2, + wishlist_added_count=2, + total_tracks=7, + ) + svc = _FakeSyncService(media_client=_FakeMediaClient(), sync_result=result) + deps = _build_deps( + sync_service=svc, + process_wishlist_automatically=lambda **kw: wishlist_calls.append(kw), + is_wishlist_actually_processing=lambda: False, + ) + + ds.run_sync_task( + 'auto_mirror_42', + 'Mirror', + [_track()], + automation_id='auto-1', + deps=deps, + ) + + assert len(wishlist_calls) == 1 + assert wishlist_calls[0]['automation_id'] == 'auto-1' + + +def test_post_sync_starts_organize_download_when_skip_wishlist_add(patched_db): + org_calls = [] + result = _FakeSyncResult( + matched_tracks=50, + failed_tracks=10, + total_tracks=60, + ) + svc = _FakeSyncService(media_client=_FakeMediaClient(), sync_result=result) + deps = _build_deps( + sync_service=svc, + run_playlist_organize_download=lambda **kw: org_calls.append(kw) or {'status': 'started'}, + ) + + ds.run_sync_task( + 'auto_mirror_7', + 'Organized', + [_track()], + automation_id='auto-2', + deps=deps, + skip_wishlist_add=True, + ) + + assert len(org_calls) == 1 + assert org_calls[0]['mirrored_playlist_id'] == 7 + assert org_calls[0]['automation_id'] == 'auto-2' + + # --------------------------------------------------------------------------- # Cleanup (finally) # --------------------------------------------------------------------------- diff --git a/tests/downloads/test_playlist_folder_exists.py b/tests/downloads/test_playlist_folder_exists.py new file mode 100644 index 00000000..e3087829 --- /dev/null +++ b/tests/downloads/test_playlist_folder_exists.py @@ -0,0 +1,88 @@ +"""Tests for playlist-folder existence detection.""" + +import os +from unittest.mock import patch + +import pytest + +from core.downloads.playlist_folder import ( + candidate_playlist_folder_paths, + resolve_playlist_folder_mode_for_batch, + track_exists_in_playlist_folder, +) + + +class _FakeDB: + def __init__(self, mirrored=None): + self._mirrored = mirrored + + def resolve_mirrored_playlist(self, playlist_ref, profile_id=1, default_source='spotify'): + return self._mirrored + + +def test_track_exists_in_playlist_folder_finds_file(tmp_path): + playlist_dir = tmp_path / 'My Playlist' + playlist_dir.mkdir() + track_file = playlist_dir / 'Artist A - Song One.flac' + track_file.write_bytes(b'x') + + with patch('core.downloads.playlist_folder._get_config_manager') as cfg: + cfg.return_value.get.return_value = str(tmp_path) + with patch('core.downloads.playlist_folder.docker_resolve_path', side_effect=lambda p: p): + with patch( + 'core.downloads.playlist_folder.get_file_path_from_template', + return_value=('', ''), + ): + assert track_exists_in_playlist_folder('My Playlist', 'Artist A', 'Song One') + + +def test_track_exists_in_playlist_folder_missing(tmp_path): + with patch('core.downloads.playlist_folder._get_config_manager') as cfg: + cfg.return_value.get.return_value = str(tmp_path) + with patch('core.downloads.playlist_folder.docker_resolve_path', side_effect=lambda p: p): + with patch( + 'core.downloads.playlist_folder.get_file_path_from_template', + return_value=('', ''), + ): + assert not track_exists_in_playlist_folder('My Playlist', 'Artist A', 'Song One') + + +def test_candidate_paths_template_layout(tmp_path): + with patch('core.downloads.playlist_folder._get_config_manager') as cfg: + cfg.return_value.get.return_value = str(tmp_path) + with patch('core.downloads.playlist_folder.docker_resolve_path', side_effect=lambda p: p): + with patch( + 'core.downloads.playlist_folder.get_file_path_from_template', + return_value=('Cool Mix', 'Artist - Title'), + ): + paths = candidate_playlist_folder_paths('Cool Mix', 'Artist', 'Title') + assert any(p.endswith('.flac') for p in paths) + assert all('Cool Mix' in p for p in paths) + + +def test_resolve_playlist_folder_mode_from_mirrored(): + db = _FakeDB(mirrored={ + 'id': 5, + 'name': 'Rekordbox Set', + 'organize_by_playlist': True, + }) + enabled, name = resolve_playlist_folder_mode_for_batch( + db, + playlist_id='37i9dQZF1', + playlist_name='Other Name', + batch_playlist_folder_mode=False, + ) + assert enabled is True + assert name == 'Rekordbox Set' + + +def test_resolve_playlist_folder_mode_batch_flag(): + db = _FakeDB() + enabled, name = resolve_playlist_folder_mode_for_batch( + db, + playlist_id='1', + playlist_name='Batch Name', + batch_playlist_folder_mode=True, + ) + assert enabled is True + assert name == 'Batch Name' diff --git a/tests/enrichment/test_unmatched.py b/tests/enrichment/test_unmatched.py new file mode 100644 index 00000000..1f749568 --- /dev/null +++ b/tests/enrichment/test_unmatched.py @@ -0,0 +1,263 @@ +"""Unmatched-browser backend for the Manage Enrichment Workers modal. + +Three seams: + * pure SQL builders + validation (core.enrichment.unmatched) + * the MusicDatabase read methods against a temp DB + * the Flask routes via a test client +""" + +from __future__ import annotations + +import pytest +from flask import Flask + +from core.enrichment import api as enrichment_api +from core.enrichment.unmatched import ( + MAX_LIMIT, + UnmatchedQueryError, + build_breakdown_query, + build_count_query, + build_reset_query, + build_unmatched_query, + supported_entity_types, +) +from database.music_database import MusicDatabase + + +# -------------------------------------------------------------------------- +# Pure builders / validation +# -------------------------------------------------------------------------- + +def test_unknown_service_rejected(): + with pytest.raises(UnmatchedQueryError): + build_unmatched_query('not-a-service', 'artist') + + +def test_unsupported_entity_type_rejected(): + # Genius enriches artists + tracks but has no album-level id column. + assert 'album' not in supported_entity_types('genius') + with pytest.raises(UnmatchedQueryError): + build_unmatched_query('genius', 'album') + with pytest.raises(UnmatchedQueryError): + build_breakdown_query('discogs', 'track') # discogs has no track column + + +def test_bad_status_rejected(): + with pytest.raises(UnmatchedQueryError): + build_unmatched_query('spotify', 'artist', status='bogus') + + +def test_status_predicates(): + nf, _ = build_count_query('spotify', 'artist', 'not_found') + pend, _ = build_count_query('spotify', 'artist', 'pending') + un, _ = build_count_query('spotify', 'artist', 'unmatched') + assert "artists.spotify_match_status = 'not_found'" in nf + assert "artists.spotify_match_status IS NULL" in pend + assert "IS NULL OR" in un and "= 'not_found'" in un + + +def test_track_query_qualifies_status_to_avoid_join_ambiguity(): + # tracks LEFT JOIN albums for artwork — both carry spotify_match_status, + # so the predicate must be qualified or SQLite errors "ambiguous column". + sql, _ = build_unmatched_query('spotify', 'track', 'not_found') + assert 'LEFT JOIN albums al' in sql + assert 'tracks.spotify_match_status' in sql + assert 'al.thumb_url AS image_url' in sql + + +def test_search_adds_like_param(): + sql, params = build_unmatched_query('spotify', 'artist', 'not_found', query='dragons') + assert 'LIKE ?' in sql + assert '%dragons%' in params + + +def test_limit_is_clamped(): + _, params = build_unmatched_query('spotify', 'artist', 'not_found', limit=99999) + assert params[-2] == MAX_LIMIT # limit + assert params[-1] == 0 # offset + _, params2 = build_unmatched_query('spotify', 'artist', 'not_found', limit=0) + assert params2[-2] == 50 # invalid -> default + + +# -------------------------------------------------------------------------- +# MusicDatabase integration (temp DB) +# -------------------------------------------------------------------------- + +def _seed(db: MusicDatabase): + conn = db._get_connection() + cur = conn.cursor() + # 3 artists: matched / not_found / pending(NULL) + cur.execute("INSERT INTO artists (id, name, spotify_match_status) VALUES ('a1','Matched Artist','matched')") + cur.execute("INSERT INTO artists (id, name, spotify_match_status) VALUES ('a2','Failed Dragons','not_found')") + cur.execute("INSERT INTO artists (id, name) VALUES ('a3','Pending Person')") # NULL status + # album + track to exercise the join-for-artwork path + cur.execute("INSERT INTO albums (id, artist_id, title, thumb_url, spotify_match_status) " + "VALUES ('al1','a2','Evolve','http://img/evolve.jpg','not_found')") + cur.execute("INSERT INTO tracks (id, album_id, artist_id, title, spotify_match_status) " + "VALUES ('t1','al1','a2','Believer','not_found')") + conn.commit() + conn.close() + + +@pytest.fixture +def db(tmp_path): + d = MusicDatabase(str(tmp_path / 'enrich.db')) + _seed(d) + return d + + +def test_breakdown_splits_matched_notfound_pending(db): + bd = db.get_enrichment_breakdown('spotify', 'artist') + assert bd == {'matched': 1, 'not_found': 1, 'pending': 1, 'total': 3} + + +def test_unmatched_not_found_only(db): + res = db.get_enrichment_unmatched('spotify', 'artist', status='not_found') + assert res['total'] == 1 + assert [i['name'] for i in res['items']] == ['Failed Dragons'] + assert res['items'][0]['status'] == 'not_found' + + +def test_unmatched_pending_only(db): + res = db.get_enrichment_unmatched('spotify', 'artist', status='pending') + assert res['total'] == 1 + assert res['items'][0]['name'] == 'Pending Person' + + +def test_unmatched_combined(db): + res = db.get_enrichment_unmatched('spotify', 'artist', status='unmatched') + assert res['total'] == 2 + assert {i['name'] for i in res['items']} == {'Failed Dragons', 'Pending Person'} + + +def test_unmatched_search_filters_by_name(db): + res = db.get_enrichment_unmatched('spotify', 'artist', status='unmatched', query='dragons') + assert res['total'] == 1 + assert res['items'][0]['name'] == 'Failed Dragons' + + +def test_unmatched_pagination(db): + page = db.get_enrichment_unmatched('spotify', 'artist', status='unmatched', limit=1, offset=0) + assert page['total'] == 2 and len(page['items']) == 1 + page2 = db.get_enrichment_unmatched('spotify', 'artist', status='unmatched', limit=1, offset=1) + assert page2['items'][0]['name'] != page['items'][0]['name'] + + +def test_track_unmatched_borrows_album_artwork(db): + res = db.get_enrichment_unmatched('spotify', 'track', status='not_found') + assert res['total'] == 1 + assert res['items'][0]['name'] == 'Believer' + assert res['items'][0]['image_url'] == 'http://img/evolve.jpg' + + +def test_unmatched_includes_parent_context(db): + # album's parent is its artist; track's parent is its album + album = db.get_enrichment_unmatched('spotify', 'album', status='not_found')['items'][0] + assert album['parent'] == 'Failed Dragons' + track = db.get_enrichment_unmatched('spotify', 'track', status='not_found')['items'][0] + assert track['parent'] == 'Evolve' + artist = db.get_enrichment_unmatched('spotify', 'artist', status='not_found')['items'][0] + assert artist['parent'] is None + + +def test_db_raises_on_bad_input(db): + with pytest.raises(UnmatchedQueryError): + db.get_enrichment_unmatched('spotify', 'artist', status='bogus') + + +# -------------------------------------------------------------------------- +# Reset / retry (re-queue) — must clear match_status to NULL so the worker +# re-attempts (nulling last_attempted alone leaves not_found in limbo). +# -------------------------------------------------------------------------- + +def test_reset_builder_requires_id_for_item(): + with pytest.raises(UnmatchedQueryError): + build_reset_query('spotify', 'artist', 'item') # no entity_id + + +def test_reset_builder_bad_scope(): + with pytest.raises(UnmatchedQueryError): + build_reset_query('spotify', 'artist', 'bogus', entity_id='x') + + +def test_reset_builder_nulls_status_not_just_attempted(): + sql, _ = build_reset_query('spotify', 'artist', 'failed') + assert 'spotify_match_status = NULL' in sql + assert 'spotify_last_attempted = NULL' in sql + assert "WHERE spotify_match_status = 'not_found'" in sql + + +def test_reset_item_requeues_to_pending(db): + n = db.reset_enrichment('spotify', 'artist', 'item', entity_id='a2') # was not_found + assert n == 1 + # not_found dropped by 1, pending gained 1 + bd = db.get_enrichment_breakdown('spotify', 'artist') + assert bd == {'matched': 1, 'not_found': 0, 'pending': 2, 'total': 3} + + +def test_reset_failed_requeues_all(db): + n = db.reset_enrichment('spotify', 'album', 'failed') # one not_found album + assert n == 1 + bd = db.get_enrichment_breakdown('spotify', 'album') + assert bd['not_found'] == 0 and bd['pending'] == 1 + + +# -------------------------------------------------------------------------- +# Flask routes +# -------------------------------------------------------------------------- + +@pytest.fixture +def client(db): + enrichment_api.configure(db_getter=lambda: db) + app = Flask(__name__) + app.register_blueprint(enrichment_api.create_blueprint()) + with app.test_client() as c: + yield c + enrichment_api.configure(db_getter=None) # reset module global + + +def test_route_unknown_service_404(client): + assert client.get('/api/enrichment/bogus/unmatched').status_code == 404 + + +def test_route_bad_entity_type_400(client): + # genius has no album column -> 400, not a 500 + r = client.get('/api/enrichment/genius/unmatched?entity_type=album') + assert r.status_code == 400 + + +def test_route_happy_path(client): + r = client.get('/api/enrichment/spotify/unmatched?entity_type=artist&status=unmatched') + assert r.status_code == 200 + body = r.get_json() + assert body['total'] == 2 + assert body['service'] == 'spotify' + assert body['entity_types'] == ['artist', 'album', 'track'] + + +def test_route_breakdown(client): + r = client.get('/api/enrichment/spotify/breakdown') + assert r.status_code == 200 + bd = r.get_json()['breakdown'] + assert bd['artist'] == {'matched': 1, 'not_found': 1, 'pending': 1, 'total': 3} + + +def test_route_retry_item(client): + r = client.post('/api/enrichment/spotify/retry', + json={'entity_type': 'artist', 'scope': 'item', 'entity_id': 'a2'}) + assert r.status_code == 200 + body = r.get_json() + assert body['success'] is True and body['reset'] == 1 + + +def test_route_retry_failed_bulk(client): + r = client.post('/api/enrichment/spotify/retry', + json={'entity_type': 'artist', 'scope': 'failed'}) + assert r.status_code == 200 + assert r.get_json()['reset'] == 1 # one not_found artist re-queued + + +def test_route_retry_item_missing_id_400(client): + r = client.post('/api/enrichment/spotify/retry', + json={'entity_type': 'artist', 'scope': 'item'}) + assert r.status_code == 400 diff --git a/tests/enrichment/test_worker_priority.py b/tests/enrichment/test_worker_priority.py new file mode 100644 index 00000000..e17e5582 --- /dev/null +++ b/tests/enrichment/test_worker_priority.py @@ -0,0 +1,138 @@ +"""Priority 'process this group first' helper for enrichment workers. + +The shared helper returns one pending item of a chosen entity type in the +shape the worker's dispatch already expects (with Spotify/iTunes mapped to +their album_individual / track_individual types). Default path (no override) +is exercised by the workers themselves and unchanged. +""" + +from __future__ import annotations + +import pytest + +from core.worker_utils import ( + PRIORITY_ENTITIES, + priority_pending_item, + read_enrichment_priority, +) +from database.music_database import MusicDatabase + + +@pytest.fixture +def db(tmp_path): + d = MusicDatabase(str(tmp_path / 'prio.db')) + conn = d._get_connection() + cur = conn.cursor() + cur.execute("INSERT INTO artists (id, name) VALUES ('a1', 'Pending Artist')") # NULL status + cur.execute("INSERT INTO artists (id, name, spotify_match_status) VALUES ('a2','Done','matched')") + cur.execute("INSERT INTO albums (id, artist_id, title) VALUES ('al1','a2','Pending Album')") # NULL status + cur.execute("INSERT INTO tracks (id, album_id, artist_id, title) VALUES ('t1','al1','a2','Pending Track')") + conn.commit() + conn.close() + return d + + +def _cur(db): + return db._get_connection().cursor() + + +def test_priority_artist_shape(db): + item = priority_pending_item(_cur(db), 'spotify', 'artist') + assert item == {'type': 'artist', 'id': 'a1', 'name': 'Pending Artist'} + + +def test_priority_album_default_type(db): + item = priority_pending_item(_cur(db), 'spotify', 'album') + assert item['id'] == 'al1' and item['name'] == 'Pending Album' and item['artist'] == 'Done' + assert item['type'] == 'album' # default type string + + +def test_priority_album_type_override_for_spotify_itunes(db): + item = priority_pending_item(_cur(db), 'spotify', 'album', + {'album': 'album_individual', 'track': 'track_individual'}) + assert item['type'] == 'album_individual' # matches Spotify/iTunes dispatch + + +def test_priority_track_shape(db): + item = priority_pending_item(_cur(db), 'spotify', 'track') + assert item['id'] == 't1' and item['type'] == 'track' and item['artist'] == 'Done' + + +def test_priority_returns_none_when_group_exhausted(db): + # No pending artists once a1 is matched -> None, so worker resumes its chain. + conn = db._get_connection(); conn.execute("UPDATE artists SET spotify_match_status='matched' WHERE id='a1'"); conn.commit(); conn.close() + assert priority_pending_item(_cur(db), 'spotify', 'artist') is None + + +def test_priority_rejects_bad_entity_and_service(db): + assert priority_pending_item(_cur(db), 'spotify', 'bogus') is None + assert priority_pending_item(_cur(db), 'spot;drop', 'artist') is None # non-alpha service blocked + + +def test_read_priority_unset_is_empty(): + # Unknown/unset key -> '' (no override). Uses the real config_manager. + assert read_enrichment_priority('definitely_not_a_service') == '' + + +def test_read_priority_roundtrip(): + from config.settings import config_manager + key = 'spotify_enrichment_priority' + old = config_manager.get(key, '') + try: + config_manager.set(key, 'album') + assert read_enrichment_priority('spotify') == 'album' + config_manager.set(key, 'bogus') + assert read_enrichment_priority('spotify') == '' # invalid -> ignored + finally: + config_manager.set(key, old) + + +def test_priority_entities_constant(): + assert PRIORITY_ENTITIES == ('artist', 'album', 'track') + + +# --- priority GET/POST routes --------------------------------------------- + +@pytest.fixture +def client(): + from flask import Flask + from core.enrichment import api as enrichment_api + store = {} + enrichment_api.configure( + config_get=lambda k, d=None: store.get(k, d), + config_set=lambda k, v: store.__setitem__(k, v), + db_getter=lambda: None, + ) + app = Flask(__name__) + app.register_blueprint(enrichment_api.create_blueprint()) + with app.test_client() as c: + c._store = store + yield c + enrichment_api.configure(config_get=None, config_set=None, db_getter=None) + + +def test_route_priority_get_default_empty(client): + r = client.get('/api/enrichment/spotify/priority') + assert r.status_code == 200 + assert r.get_json()['priority'] == '' + + +def test_route_priority_set_and_get(client): + assert client.post('/api/enrichment/spotify/priority', json={'entity': 'album'}).status_code == 200 + assert client._store['spotify_enrichment_priority'] == 'album' + assert client.get('/api/enrichment/spotify/priority').get_json()['priority'] == 'album' + + +def test_route_priority_clear(client): + client.post('/api/enrichment/spotify/priority', json={'entity': 'album'}) + client.post('/api/enrichment/spotify/priority', json={'entity': 'none'}) + assert client.get('/api/enrichment/spotify/priority').get_json()['priority'] == '' + + +def test_route_priority_rejects_unsupported_entity(client): + # Genius has no albums -> 400 + assert client.post('/api/enrichment/genius/priority', json={'entity': 'album'}).status_code == 400 + + +def test_route_priority_unknown_service_404(client): + assert client.get('/api/enrichment/bogus/priority').status_code == 404 diff --git a/tests/imports/test_import_paths.py b/tests/imports/test_import_paths.py index ec94881d..ee4fd1b6 100644 --- a/tests/imports/test_import_paths.py +++ b/tests/imports/test_import_paths.py @@ -17,6 +17,84 @@ def test_sanitize_filename_replaces_illegal_characters(): assert import_paths.sanitize_filename("AUX.txt").startswith("_") +# ── #767: dry-run path build must not create folders ────────────────────── + +def _album_path_config(tmp_path): + return _Config({ + "soulseek.transfer_path": str(tmp_path / "Transfer"), + "file_organization.enabled": True, + "file_organization.templates": { + "album_path": "$albumartist/$albumartist - $album/$track - $title", + "single_path": "$artist/$artist - $title", + }, + "file_organization.collab_artist_mode": "first", + "file_organization.disc_label": "Disc", + }) + + +def _album_context(): + return { + "artist": {"name": "Lenka"}, + "album": {"name": "Lenka", "id": "album-1", "release_date": "2008-01-01", + "total_tracks": 12, "album_type": "album", "artists": [{"name": "Lenka"}]}, + "track_info": {"name": "The Show", "id": "t1", "track_number": 1, + "disc_number": 1, "artists": [{"name": "Lenka"}]}, + "original_search_result": {"title": "The Show", "clean_title": "The Show", + "clean_album": "Lenka", "clean_artist": "Lenka", + "artists": [{"name": "Lenka"}]}, + "source": "deezer", "is_album_download": False, + } + + +def test_create_dirs_false_does_not_create_folders(monkeypatch, tmp_path): + monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _album_path_config(tmp_path)) + monkeypatch.setattr(import_paths, "_get_album_tracks_for_source", lambda *a: None) + + final_path, created = import_paths.build_final_path_for_track( + _album_context(), {"name": "Lenka"}, + {"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1}, + ".flac", create_dirs=False, + ) + + # Path is still computed correctly... + assert created is True + assert final_path == str(tmp_path / "Transfer" / "Lenka" / "Lenka - Lenka" / "01 - The Show.flac") + # ...but NOTHING was written to disk — not even the Transfer root. + assert not (tmp_path / "Transfer").exists() + + +def test_create_dirs_true_still_creates_folders(monkeypatch, tmp_path): + # The download/import flow must keep working (default behavior unchanged). + monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _album_path_config(tmp_path)) + monkeypatch.setattr(import_paths, "_get_album_tracks_for_source", lambda *a: None) + + final_path, created = import_paths.build_final_path_for_track( + _album_context(), {"name": "Lenka"}, + {"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1}, + ".flac", # create_dirs defaults True + ) + + assert created is True + assert (tmp_path / "Transfer" / "Lenka" / "Lenka - Lenka").is_dir() + + +def test_create_dirs_false_and_true_yield_identical_path(monkeypatch, tmp_path): + monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _album_path_config(tmp_path)) + monkeypatch.setattr(import_paths, "_get_album_tracks_for_source", lambda *a: None) + + dry, _ = import_paths.build_final_path_for_track( + _album_context(), {"name": "Lenka"}, + {"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1}, + ".flac", create_dirs=False, + ) + live, _ = import_paths.build_final_path_for_track( + _album_context(), {"name": "Lenka"}, + {"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1}, + ".flac", create_dirs=True, + ) + assert dry == live + + def test_build_simple_download_destination_uses_album_folder(monkeypatch, tmp_path): config = _Config({"soulseek.transfer_path": str(tmp_path / "Transfer")}) monkeypatch.setattr(import_paths, "_get_config_manager", lambda: config) diff --git a/tests/imports/test_import_pipeline.py b/tests/imports/test_import_pipeline.py index 1a4048ae..13684241 100644 --- a/tests/imports/test_import_pipeline.py +++ b/tests/imports/test_import_pipeline.py @@ -202,7 +202,12 @@ def test_post_process_matched_download_forwards_separate_metadata_runtime(tmp_pa monkeypatch.setattr(import_pipeline, "emit_track_downloaded", lambda *args, **kwargs: None) monkeypatch.setattr(import_pipeline, "record_library_history_download", lambda *args, **kwargs: None) monkeypatch.setattr(import_pipeline, "record_download_provenance", lambda *args, **kwargs: None) - monkeypatch.setattr(import_pipeline, "record_soulsync_library_entry", lambda *args, **kwargs: None) + library_calls = [] + + def _record_library(context, artist_context, album_info): + library_calls.append((context, artist_context, album_info)) + + monkeypatch.setattr(import_pipeline, "record_soulsync_library_entry", _record_library) monkeypatch.setattr(import_pipeline, "check_and_remove_from_wishlist", lambda *args, **kwargs: None) monkeypatch.setattr(import_pipeline, "record_retag_download", lambda *args, **kwargs: None) @@ -221,6 +226,8 @@ def test_post_process_matched_download_forwards_separate_metadata_runtime(tmp_pa ) assert seen["runtime"] is metadata_runtime + assert len(library_calls) == 1 + assert library_calls[0][2]["album_name"] == "Album" # --------------------------------------------------------------------------- diff --git a/tests/imports/test_import_rejection_reason.py b/tests/imports/test_import_rejection_reason.py new file mode 100644 index 00000000..9377ff6a --- /dev/null +++ b/tests/imports/test_import_rejection_reason.py @@ -0,0 +1,145 @@ +"""Tests for import_rejection_reason — the manual-import quarantine guard. + +Regression for #764: the manual-import routes call +``post_process_matched_download`` directly. A quarantine (integrity / AcoustID +/ bit-depth) or race-guard rejection returns NORMALLY (no exception) and leaves +the file in ss_quarantine, not the library — but the routes counted any +no-exception return as a successful import, so the UI showed a green "Done" for +a file that had actually vanished. ``import_rejection_reason`` reads the context +flags the inner pipeline sets so the routes can report those as errors. +""" + +from __future__ import annotations + +import os +import tempfile + +from core.imports.pipeline import import_rejection_reason +from core.imports.routes import ImportRouteRuntime, process_single_import_file + + +def test_clean_import_returns_none(): + assert import_rejection_reason({}) is None + assert import_rejection_reason({'is_album': True, 'track_info': {}}) is None + + +def test_integrity_failure_detected(): + reason = import_rejection_reason({'_integrity_failure_msg': 'duration drift 12s'}) + assert reason is not None + assert 'integrity' in reason.lower() + assert 'duration drift 12s' in reason + + +def test_acoustid_quarantine_detected(): + reason = import_rejection_reason({ + '_acoustid_quarantined': True, + '_acoustid_failure_msg': 'wrong artist: got Oasis expected Coldplay', + }) + assert reason is not None + assert 'acoustid' in reason.lower() + assert 'Coldplay' in reason + + +def test_acoustid_quarantine_without_message_still_flags(): + # The flag alone must trip it even if no message was stashed. + reason = import_rejection_reason({'_acoustid_quarantined': True}) + assert reason is not None + assert 'acoustid' in reason.lower() + + +def test_bitdepth_rejection_detected(): + reason = import_rejection_reason({'_bitdepth_rejected': True}) + assert reason is not None + assert 'bit-depth' in reason.lower() + + +def test_race_guard_failure_detected(): + reason = import_rejection_reason({'_race_guard_failed': True}) + assert reason is not None + assert 'disappeared' in reason.lower() + + +def test_falsy_flags_do_not_trip(): + # A flag present but falsy (e.g. integrity passed) must NOT be a rejection. + ctx = { + '_integrity_failure_msg': '', + '_acoustid_quarantined': False, + '_bitdepth_rejected': False, + '_race_guard_failed': False, + } + assert import_rejection_reason(ctx) is None + + +def test_integrity_takes_precedence_when_multiple_set(): + # Deterministic ordering: integrity first. + reason = import_rejection_reason({ + '_integrity_failure_msg': 'truncated', + '_acoustid_quarantined': True, + '_bitdepth_rejected': True, + }) + assert 'integrity' in reason.lower() + + +# ── route-level wiring: a quarantine must NOT report as a successful import ── + + +def _runtime_with_post_process(post_process): + """Build an ImportRouteRuntime wired with stub resolvers + the supplied + post_process_matched_download. Resolvers return a shared context dict so a + flag the post-processor sets is what import_rejection_reason later reads.""" + ctx = {} + + return ImportRouteRuntime( + get_single_track_import_context=lambda *a, **k: {"context": ctx, "source": "local"}, + normalize_import_context=lambda c: c, + get_import_context_artist=lambda c: {"name": "Coldplay"}, + get_import_track_info=lambda c: {"name": "Yellow"}, + post_process_matched_download=post_process, + ), ctx + + +def _tmp_audio_file(): + fd, path = tempfile.mkstemp(suffix=".flac") + os.close(fd) + with open(path, "wb") as f: + f.write(b"fLaC") # only needs to exist for os.path.isfile + return path + + +def test_single_import_quarantine_reported_as_error(): + # post-processing quarantines the file (sets the flag, returns normally). + def quarantining_post_process(context_key, context, file_path): + context['_acoustid_quarantined'] = True + context['_acoustid_failure_msg'] = 'wrong track' + + runtime, _ctx = _runtime_with_post_process(quarantining_post_process) + path = _tmp_audio_file() + try: + outcome, payload = process_single_import_file( + runtime, {"full_path": path, "filename": "Coldplay - Yellow.flac", + "title": "Yellow", "artist": "Coldplay"}, + ) + finally: + os.remove(path) + + assert outcome == "error" # NOT "ok" -> route won't count it processed + assert "AcoustID" in payload + + +def test_single_import_clean_reports_ok(): + # post-processing succeeds (no flags) -> import counts as processed. + def clean_post_process(context_key, context, file_path): + return None + + runtime, _ctx = _runtime_with_post_process(clean_post_process) + path = _tmp_audio_file() + try: + outcome, payload = process_single_import_file( + runtime, {"full_path": path, "filename": "Coldplay - Yellow.flac", + "title": "Yellow", "artist": "Coldplay"}, + ) + finally: + os.remove(path) + + assert outcome == "ok" + assert payload == "Yellow" diff --git a/tests/test_art_apply.py b/tests/test_art_apply.py new file mode 100644 index 00000000..8e240e41 --- /dev/null +++ b/tests/test_art_apply.py @@ -0,0 +1,147 @@ +"""Tests for core.metadata.art_apply — on-disk art detection + applying art.""" + +import sys +import types +from types import SimpleNamespace + +# Stubs so core.metadata.artwork (pulled in transitively) imports without the +# real Spotify / config dependencies (mirrors test_missing_cover_art.py). +if 'spotipy' not in sys.modules: + spotipy = types.ModuleType('spotipy') + oauth2 = types.ModuleType('spotipy.oauth2') + spotipy.Spotify = type('S', (), {}) + oauth2.SpotifyOAuth = oauth2.SpotifyClientCredentials = type('O', (), {}) + spotipy.oauth2 = oauth2 + sys.modules['spotipy'] = spotipy + sys.modules['spotipy.oauth2'] = oauth2 + +if 'config.settings' not in sys.modules: + config_mod = types.ModuleType('config') + settings_mod = types.ModuleType('config.settings') + + class _Cfg: + def get(self, key, default=None): + return default + + def get_active_media_server(self): + return 'plex' + + settings_mod.config_manager = _Cfg() + config_mod.settings = settings_mod + sys.modules['config'] = config_mod + sys.modules['config.settings'] = settings_mod + +from core.metadata import art_apply as aa + + +# ── sidecar detection ── + +def test_folder_has_cover_sidecar(tmp_path): + assert aa.folder_has_cover_sidecar(str(tmp_path)) is False + (tmp_path / 'cover.jpg').write_bytes(b'x') + assert aa.folder_has_cover_sidecar(str(tmp_path)) is True + + +def test_album_has_art_on_disk_no_local_file_is_true(): + # No representative file (e.g. media-server-only album) → not flagged. + assert aa.album_has_art_on_disk('') is True + assert aa.album_has_art_on_disk(None) is True + + +def test_album_has_art_on_disk_sidecar_short_circuits(tmp_path, monkeypatch): + (tmp_path / 'cover.jpg').write_bytes(b'x') + track = tmp_path / '01 song.flac' + track.write_bytes(b'') + # Sidecar present → True without ever opening the audio file. + called = {'n': 0} + monkeypatch.setattr(aa, 'file_has_embedded_art', lambda p: called.__setitem__('n', called['n'] + 1) or False) + assert aa.album_has_art_on_disk(str(track)) is True + assert called['n'] == 0 + + +def test_album_has_art_on_disk_no_sidecar_checks_file(tmp_path, monkeypatch): + track = tmp_path / '01 song.flac' + track.write_bytes(b'') + monkeypatch.setattr(aa, 'file_has_embedded_art', lambda p: False) + assert aa.album_has_art_on_disk(str(track)) is False + monkeypatch.setattr(aa, 'file_has_embedded_art', lambda p: True) + assert aa.album_has_art_on_disk(str(track)) is True + + +# ── embedded-art detection ── + +def _fake_symbols(audio): + return SimpleNamespace( + File=lambda path: audio, + ID3=type('ID3', (), {}), + MP4=type('MP4', (), {}), + FLAC=type('FLAC', (), {}), + ) + + +def test_file_has_embedded_art_flac_picture(tmp_path, monkeypatch): + f = tmp_path / 'a.flac' + f.write_bytes(b'') + audio = SimpleNamespace(pictures=['pic'], tags=None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + assert aa.file_has_embedded_art(str(f)) is True + + +def test_file_has_embedded_art_none(tmp_path, monkeypatch): + f = tmp_path / 'a.flac' + f.write_bytes(b'') + audio = SimpleNamespace(pictures=[], tags=None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + assert aa.file_has_embedded_art(str(f)) is False + + +# ── applying art ── + +def test_apply_embeds_into_each_file_and_writes_cover(tmp_path, monkeypatch): + f1 = tmp_path / '01.flac'; f1.write_bytes(b'') + f2 = tmp_path / '02.flac'; f2.write_bytes(b'') + + saved = [] + audio = SimpleNamespace(tags=None, add_tags=lambda: None, save=lambda: saved.append(True)) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + + embed_calls = [] + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda a, m: embed_calls.append(m) or True) + # download_cover_art is the standard cover.jpg writer — stub it to drop one. + monkeypatch.setattr(aa, 'download_cover_art', + lambda album_info, folder, ctx=None: open(f"{folder}/cover.jpg", 'wb').close()) + + meta = {'artist': 'A', 'album': 'B', 'album_art_url': 'http://x/y.jpg'} + res = aa.apply_art_to_album_files([str(f1), str(f2)], meta, {'album_name': 'B'}, folder=str(tmp_path)) + + assert res['embedded'] == 2 + assert len(saved) == 2 # each file saved after embed + assert res['cover_written'] is True + + +def test_apply_skips_files_that_already_have_art(tmp_path, monkeypatch): + """Purely additive: a file that already has art is left untouched (no + duplicate FLAC picture, no re-embed).""" + f1 = tmp_path / '01.flac'; f1.write_bytes(b'') + audio = SimpleNamespace(pictures=['existing'], tags=None, save=lambda: None) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + embed_calls = [] + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda a, m: embed_calls.append(m) or True) + monkeypatch.setattr(aa, 'download_cover_art', lambda *a, **k: None) + + res = aa.apply_art_to_album_files([str(f1)], {'album': 'B'}, {'album_name': 'B'}, folder=str(tmp_path)) + assert res['embedded'] == 0 + assert res['skipped'] == 1 + assert embed_calls == [] # never re-embedded → no duplicate picture + + +def test_apply_counts_failures_without_raising(tmp_path, monkeypatch): + f1 = tmp_path / '01.flac'; f1.write_bytes(b'') + audio = SimpleNamespace(tags=object(), save=lambda: (_ for _ in ()).throw(OSError('read-only'))) + monkeypatch.setattr(aa, 'get_mutagen_symbols', lambda: _fake_symbols(audio)) + monkeypatch.setattr(aa, 'embed_album_art_metadata', lambda a, m: True) + monkeypatch.setattr(aa, 'download_cover_art', lambda *a, **k: None) + + res = aa.apply_art_to_album_files([str(f1)], {'album': 'B'}, {'album_name': 'B'}, folder=str(tmp_path)) + assert res['embedded'] == 0 + assert res['failed'] == 1 # save() raised (read-only) — counted, not crashed diff --git a/tests/test_art_preservation.py b/tests/test_art_preservation.py new file mode 100644 index 00000000..230f1420 --- /dev/null +++ b/tests/test_art_preservation.py @@ -0,0 +1,179 @@ +"""Tests for embedded-cover-art preservation across the enrichment rewrite. + +Regression for #764 (continuation of #755): importing a file destroyed its +embedded album art whenever the re-embed step couldn't produce new art +(no source metadata, no art URL, download failed, rejected by the min-size +guard, or art embedding disabled). ``enhance_file_metadata`` clears pictures +up front and saves regardless; these helpers snapshot the art first and put +it back iff the file would otherwise be saved with none. + +Uses real mutagen objects (a minimal valid FLAC + a minimal MP3) so the +snapshot/restore round-trips through the actual Picture/APIC APIs the +enricher uses — not a mock of them. +""" + +from __future__ import annotations + +import os +import tempfile + +import pytest + +mutagen = pytest.importorskip("mutagen") + +from mutagen.flac import FLAC, Picture # noqa: E402 +from mutagen.id3 import APIC, ID3, TIT2 # noqa: E402 + +from core.metadata.common import get_mutagen_symbols # noqa: E402 +from core.metadata.art_preservation import ( # noqa: E402 + has_embedded_art, + restore_embedded_art, + snapshot_embedded_art, +) + +SYMBOLS = get_mutagen_symbols() + +# 1x1 PNG — smallest valid image bytes for a real Picture/APIC payload. +_PNG = ( + b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01" + b"\x08\x06\x00\x00\x00\x1f\x15\xc4\x89\x00\x00\x00\nIDATx\x9cc\x00" + b"\x01\x00\x00\x05\x00\x01\r\n-\xb4\x00\x00\x00\x00IEND\xaeB`\x82" +) + + +def _minimal_flac_bytes() -> bytes: + # 4-byte magic + last STREAMINFO block (34 bytes). Mirrors the fixture + # used by tests/test_tag_writer_multi_artist.py. + return ( + 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 + ) + + +@pytest.fixture +def flac_with_art(): + fd, path = tempfile.mkstemp(suffix=".flac") + os.close(fd) + with open(path, "wb") as f: + f.write(_minimal_flac_bytes()) + audio = FLAC(path) + pic = Picture() + pic.data = _PNG + pic.type = 3 + pic.mime = "image/png" + pic.width = 1 + pic.height = 1 + pic.depth = 24 + audio.add_picture(pic) + audio.save() + yield path + try: + os.remove(path) + except OSError: + pass + + +class _ID3Holder: + """Stand-in for an mutagen MP3 object: exposes a real ``ID3`` tag block + (the only thing the snapshot/restore helpers touch) without needing a + syncable MPEG frame on disk. ``__setitem__`` mirrors mutagen's mapping + sugar used by the MP4 restore branch — unused here but kept faithful.""" + + def __init__(self): + self.tags = ID3() + self.tags.add(TIT2(encoding=3, text=["original"])) + self.tags.add(APIC(encoding=3, mime="image/png", type=3, desc="Cover", data=_PNG)) + + def __setitem__(self, key, value): + self.tags[key] = value + + +@pytest.fixture +def mp3_with_art(): + return _ID3Holder() + + +# ── FLAC ──────────────────────────────────────────────────────────────── + + +def test_flac_art_restored_after_clear(flac_with_art): + audio = FLAC(flac_with_art) + assert has_embedded_art(audio, SYMBOLS) + + snap = snapshot_embedded_art(audio, SYMBOLS) + assert snap # something captured + + # Simulate the enrichment rewrite: nuke the art, fail to re-embed. + audio.clear_pictures() + assert not has_embedded_art(audio, SYMBOLS) + + restored = restore_embedded_art(audio, SYMBOLS, snap) + assert restored is True + assert has_embedded_art(audio, SYMBOLS) + assert audio.pictures[0].data == _PNG + + +def test_flac_restore_is_noop_when_new_art_present(flac_with_art): + # Happy path: re-embed succeeded, so restore must NOT touch the file. + audio = FLAC(flac_with_art) + snap = snapshot_embedded_art(audio, SYMBOLS) + + audio.clear_pictures() + new = Picture() + new.data = _PNG + b"NEWART" + new.type = 3 + new.mime = "image/png" + audio.add_picture(new) + + restored = restore_embedded_art(audio, SYMBOLS, snap) + assert restored is False + assert len(audio.pictures) == 1 + assert audio.pictures[0].data == _PNG + b"NEWART" # not clobbered/duplicated + + +def test_flac_no_art_snapshot_empty(): + fd, path = tempfile.mkstemp(suffix=".flac") + os.close(fd) + try: + with open(path, "wb") as f: + f.write(_minimal_flac_bytes()) + audio = FLAC(path) + assert snapshot_embedded_art(audio, SYMBOLS) == [] + # Restoring an empty snapshot is a no-op. + assert restore_embedded_art(audio, SYMBOLS, []) is False + finally: + os.remove(path) + + +# ── MP3 / ID3 ───────────────────────────────────────────────────────────── + + +def test_mp3_apic_restored_after_tags_cleared(mp3_with_art): + audio = mp3_with_art + assert has_embedded_art(audio, SYMBOLS) + snap = snapshot_embedded_art(audio, SYMBOLS) + assert snap + + # The enricher does audio_file.tags.clear() then rewrites tags. + audio.tags.clear() + assert not has_embedded_art(audio, SYMBOLS) + + restored = restore_embedded_art(audio, SYMBOLS, snap) + assert restored is True + apics = audio.tags.getall("APIC") + assert apics and apics[0].data == _PNG + + +def test_mp3_restore_noop_when_new_apic_present(mp3_with_art): + audio = mp3_with_art + snap = snapshot_embedded_art(audio, SYMBOLS) + audio.tags.clear() + audio.tags.add(APIC(encoding=3, mime="image/png", type=3, desc="Cover", data=_PNG + b"NEW")) + + assert restore_embedded_art(audio, SYMBOLS, snap) is False + apics = audio.tags.getall("APIC") + assert len(apics) == 1 and apics[0].data == _PNG + b"NEW" diff --git a/tests/test_canonical_columns_migration.py b/tests/test_canonical_columns_migration.py new file mode 100644 index 00000000..0531bbf1 --- /dev/null +++ b/tests/test_canonical_columns_migration.py @@ -0,0 +1,69 @@ +"""Migration test for the canonical-album-version columns (#765 Stage 1). + +Additive + nullable, so it must: appear on a fresh DB, be idempotent (re-running +the migration is a no-op, not an error), and ALTER them onto an older albums +table that lacks them. NULL = unresolved = tools fall back to today's behavior. +""" + +from __future__ import annotations + +import sqlite3 + +from database.music_database import MusicDatabase + +_CANONICAL_COLS = { + 'canonical_source', 'canonical_album_id', 'canonical_score', 'canonical_resolved_at', +} + + +def _album_cols(cur): + cur.execute("PRAGMA table_info(albums)") + return {c[1] for c in cur.fetchall()} + + +def test_fresh_db_has_canonical_columns(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + cur = db._get_connection().cursor() + assert _CANONICAL_COLS <= _album_cols(cur) + + +def test_canonical_columns_default_null(tmp_path): + # Unresolved by default -> every consumer falls back. Verify each canonical + # column declares DEFAULT NULL and is nullable (notnull flag == 0). + db = MusicDatabase(str(tmp_path / "m.db")) + cur = db._get_connection().cursor() + cur.execute("PRAGMA table_info(albums)") + info = {c[1]: c for c in cur.fetchall()} # name -> (cid, name, type, notnull, dflt, pk) + for col in _CANONICAL_COLS: + assert col in info, f"{col} missing" + assert info[col][3] == 0, f"{col} must be nullable" + dflt = info[col][4] + assert dflt is None or str(dflt).upper() == 'NULL', f"{col} default should be NULL" + + +def test_migration_is_idempotent(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + cur = db._get_connection().cursor() + before = _album_cols(cur) + # Re-running must not raise (the PRAGMA guard skips existing columns). + db._ensure_core_media_schema_columns(cur) + db._ensure_core_media_schema_columns(cur) + assert _album_cols(cur) == before + assert _CANONICAL_COLS <= _album_cols(cur) + + +def test_migration_adds_columns_to_old_albums_table(tmp_path): + # Simulate an upgraded DB whose albums table predates these columns. + path = str(tmp_path / "old.db") + conn = sqlite3.connect(path) + conn.execute("CREATE TABLE albums (id INTEGER PRIMARY KEY, title TEXT)") + conn.commit() + cur = conn.cursor() + assert not (_CANONICAL_COLS & _album_cols(cur)) # none present yet + + # Run the real migration against this old cursor. + db = MusicDatabase(str(tmp_path / "scratch.db")) + db._ensure_core_media_schema_columns(cur) + conn.commit() + + assert _CANONICAL_COLS <= _album_cols(cur) diff --git a/tests/test_canonical_db.py b/tests/test_canonical_db.py new file mode 100644 index 00000000..4bc0496f --- /dev/null +++ b/tests/test_canonical_db.py @@ -0,0 +1,54 @@ +"""DB persistence for canonical album version (#765 Stage 2).""" + +from __future__ import annotations + +from database.music_database import MusicDatabase + + +def _album(db, album_id="alb_evolve"): + # id columns are TEXT (GUID) post-migration, so insert explicit ids and a + # valid FK rather than relying on integer rowids. + conn = db._get_connection() + cur = conn.cursor() + cur.execute("INSERT INTO artists (id, name) VALUES ('art_id', 'Imagine Dragons')") + cur.execute( + "INSERT INTO albums (id, title, artist_id) VALUES (?, 'Evolve', 'art_id')", + (album_id,), + ) + conn.commit() + conn.close() + return album_id + + +def test_set_then_get_roundtrip(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + album_id = _album(db) + + assert db.get_album_canonical(album_id) is None # unresolved by default + + assert db.set_album_canonical(album_id, "spotify", "sp_evolve_123", 0.97) is True + got = db.get_album_canonical(album_id) + assert got["source"] == "spotify" + assert got["album_id"] == "sp_evolve_123" + assert abs(got["score"] - 0.97) < 1e-6 + assert got["resolved_at"] # timestamp populated + + +def test_get_unresolved_returns_none(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + album_id = _album(db) + assert db.get_album_canonical(album_id) is None + + +def test_set_overwrites_previous(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + album_id = _album(db) + db.set_album_canonical(album_id, "spotify", "old", 0.6) + db.set_album_canonical(album_id, "musicbrainz", "new", 0.95) + got = db.get_album_canonical(album_id) + assert got["source"] == "musicbrainz" and got["album_id"] == "new" + + +def test_set_on_missing_album_returns_false(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + assert db.set_album_canonical(999999, "spotify", "x", 0.9) is False diff --git a/tests/test_canonical_orchestration.py b/tests/test_canonical_orchestration.py new file mode 100644 index 00000000..ec933bb1 --- /dev/null +++ b/tests/test_canonical_orchestration.py @@ -0,0 +1,143 @@ +"""End-to-end orchestration for canonical resolve+store (#765 Stage 2 trigger). + +Uses a real temp DB (album + tracks + source IDs) and an INJECTED fetcher, so +the DB gathering + persistence are exercised for real without live APIs. +""" + +from __future__ import annotations + +from core.metadata.canonical_resolver import ( + default_fetch_tracklist, + resolve_and_store_canonical_for_album, +) +from database.music_database import MusicDatabase + +STD = [{"duration_ms": 180_000 + i * 10_000, "title": f"Song {i+1}", "track_number": i + 1} for i in range(11)] +DLX = STD + [{"duration_ms": 320_000 + i * 10_000, "title": f"Bonus {i+1}", "track_number": 12 + i} for i in range(6)] + + +def _seed(db, *, spotify=None, deezer=None, n_files=11): + """Insert an album (with given source IDs) + n_files tracks whose + durations/titles match the STANDARD release.""" + conn = db._get_connection() + cur = conn.cursor() + cur.execute("INSERT INTO artists (id, name) VALUES ('art1', 'Imagine Dragons')") + cur.execute( + "INSERT INTO albums (id, title, artist_id, spotify_album_id, deezer_id) " + "VALUES ('alb1', 'Evolve', 'art1', ?, ?)", + (spotify, deezer), + ) + for i in range(n_files): + cur.execute( + "INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration) " + "VALUES (?, 'alb1', 'art1', ?, ?, ?)", + (f"t{i}", f"Song {i+1}", i + 1, 180_000 + i * 10_000), + ) + conn.commit() + conn.close() + return "alb1" + + +def test_resolve_and_store_picks_best_fit_and_persists(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + album_id = _seed(db, spotify="sp_deluxe", deezer="dz_std") # 11 files + + table = {("spotify", "sp_deluxe"): DLX, ("deezer", "dz_std"): STD} + out = resolve_and_store_canonical_for_album( + db, album_id, + fetch_tracklist=lambda s, a: table.get((s, a)), + source_priority=["spotify", "deezer"], + mode="best_fit", + ) + # best_fit: Deezer's standard matches the 11 files better than Spotify's deluxe. + assert out["source"] == "deezer" and out["album_id"] == "dz_std" + # ...and it was persisted. + stored = db.get_album_canonical(album_id) + assert stored["source"] == "deezer" and stored["album_id"] == "dz_std" + + +def test_default_mode_prefers_active_source(tmp_path): + # Same setup, but default (active_preferred) mode: primary = spotify, whose + # deluxe still clears the floor -> pinned, even though deezer fits better. + db = MusicDatabase(str(tmp_path / "m.db")) + album_id = _seed(db, spotify="sp_deluxe", deezer="dz_std") + table = {("spotify", "sp_deluxe"): DLX, ("deezer", "dz_std"): STD} + out = resolve_and_store_canonical_for_album( + db, album_id, + fetch_tracklist=lambda s, a: table.get((s, a)), + source_priority=["spotify", "deezer"], # default mode + ) + assert out["source"] == "spotify" # active source preferred + + +def test_result_includes_artist_and_album_context(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + conn = db._get_connection() + cur = conn.cursor() + cur.execute("INSERT INTO artists (id, name, thumb_url) VALUES ('art1', 'Imagine Dragons', 'http://artist.jpg')") + cur.execute( + "INSERT INTO albums (id, title, artist_id, thumb_url, spotify_album_id) " + "VALUES ('alb1', 'Evolve', 'art1', 'http://album.jpg', 'sp1')" + ) + for i in range(11): + cur.execute( + "INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration) " + "VALUES (?, 'alb1', 'art1', ?, ?, ?)", + (f"t{i}", f"Song {i+1}", i + 1, 180_000 + i * 10_000), + ) + conn.commit() + conn.close() + + out = resolve_and_store_canonical_for_album( + db, "alb1", fetch_tracklist=lambda s, a: STD, source_priority=["spotify"], + ) + assert out["artist_name"] == "Imagine Dragons" + assert out["album_thumb_url"] == "http://album.jpg" + assert out["artist_thumb_url"] == "http://artist.jpg" + # free context: db track count, linked sources, and both title lists + assert out["db_track_count"] == 11 + assert out["linked_sources"] == {"spotify": "sp1"} + assert out["file_track_titles"][0] == "Song 1" and len(out["file_track_titles"]) == 11 + assert "Song 1" in out["release_track_titles"] + + +def test_resolve_returns_none_when_album_has_no_source_ids(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + album_id = _seed(db, spotify=None, deezer=None) + out = resolve_and_store_canonical_for_album( + db, album_id, fetch_tracklist=lambda s, a: STD, source_priority=["spotify"], + ) + assert out is None + assert db.get_album_canonical(album_id) is None + + +def test_resolve_returns_none_for_missing_album(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + out = resolve_and_store_canonical_for_album( + db, "does-not-exist", fetch_tracklist=lambda s, a: STD, source_priority=["spotify"], + ) + assert out is None + + +# ── default_fetch_tracklist normalization (no DB / no live API) ──────────── + +def test_default_fetcher_normalizes_dict_items(monkeypatch): + import core.metadata_service as ms + monkeypatch.setattr( + ms, "get_album_tracks_for_source", + lambda s, a: [{"name": "A", "track_number": 1, "duration_ms": 200000}, + {"title": "B", "track_number": 2, "duration": 210}], # seconds + raising=False, + ) + out = default_fetch_tracklist("spotify", "x") + assert out[0] == {"title": "A", "track_number": 1, "duration_ms": 200000} + assert out[1] == {"title": "B", "track_number": 2, "duration_ms": 210_000} # sec->ms + + +def test_default_fetcher_handles_failure(monkeypatch): + import core.metadata_service as ms + monkeypatch.setattr( + ms, "get_album_tracks_for_source", + lambda s, a: (_ for _ in ()).throw(RuntimeError("boom")), raising=False, + ) + assert default_fetch_tracklist("spotify", "x") is None diff --git a/tests/test_canonical_resolver.py b/tests/test_canonical_resolver.py new file mode 100644 index 00000000..516eac11 --- /dev/null +++ b/tests/test_canonical_resolver.py @@ -0,0 +1,195 @@ +"""Tests for resolve_canonical_for_album (#765 Stage 2 — injectable core).""" + +from __future__ import annotations + +from core.metadata.canonical_resolver import resolve_canonical_for_album + +STD = [{"duration_ms": 180_000 + i * 10_000, "title": f"Song {i+1}"} for i in range(11)] +DLX = STD + [{"duration_ms": 300_000 + i * 10_000, "title": f"Bonus {i+1}"} for i in range(6)] + +PRIORITY = ["spotify", "itunes", "deezer", "musicbrainz"] + + +def _fetcher(table): + """fetch_tracklist backed by a {(source, album_id): tracks} table.""" + def fetch(source, album_id): + return table.get((source, album_id)) + return fetch + + +def test_best_fit_mode_picks_best_regardless_of_priority(): + files = list(STD) # user owns the 11-track standard + table = { + ("spotify", "sp_deluxe"): DLX, # spotify (primary) linked to deluxe (17) + ("musicbrainz", "mb_std"): STD, # musicbrainz has standard (11) + } + out = resolve_canonical_for_album( + album_source_ids={"spotify": "sp_deluxe", "musicbrainz": "mb_std"}, + file_tracks=files, + fetch_tracklist=_fetcher(table), + source_priority=PRIORITY, + mode="best_fit", + ) + # best_fit: standard matches the files, deluxe doesn't — fit beats priority. + assert out["source"] == "musicbrainz" and out["album_id"] == "mb_std" + assert out["score"] > 0.9 + + +# ── source-selection modes ──────────────────────────────────────────────── + +def test_active_preferred_uses_primary_when_it_fits(): + files = list(STD) + table = {("spotify", "sp1"): STD, ("musicbrainz", "mb1"): STD} # both fit + out = resolve_canonical_for_album( + album_source_ids={"spotify": "sp1", "musicbrainz": "mb1"}, + file_tracks=files, fetch_tracklist=_fetcher(table), + source_priority=PRIORITY, # primary = spotify + ) # default mode = active_preferred + assert out["source"] == "spotify" + + +def test_active_preferred_falls_back_when_primary_clearly_misfits(): + files = list(STD) # 11 tracks + table = { + ("spotify", "sp_bad"): [{"duration_ms": 60_000, "title": "X"}] * 3, # 3-track, fall back to the fitting source. + assert out["source"] == "musicbrainz" + + +def test_active_preferred_keeps_primary_even_if_another_fits_better(): + files = list(STD) + # primary spotify is a deluxe (decent fit, above floor); musicbrainz is exact. + table = {("spotify", "sp_dlx"): DLX, ("musicbrainz", "mb_std"): STD} + out = resolve_canonical_for_album( + album_source_ids={"spotify": "sp_dlx", "musicbrainz": "mb_std"}, + file_tracks=files, fetch_tracklist=_fetcher(table), + source_priority=PRIORITY, mode="active_preferred", + ) + # active_preferred respects the active source as long as it clears the floor, + # even though musicbrainz would fit better (use best_fit for that). + assert out["source"] == "spotify" + + +def test_active_only_pins_primary_and_never_falls_back(): + files = list(STD) + # primary spotify is below floor; a perfect musicbrainz exists but is ignored. + table = { + ("spotify", "sp_bad"): [{"duration_ms": 60_000, "title": "X"}] * 3, + ("musicbrainz", "mb_std"): STD, + } + out = resolve_canonical_for_album( + album_source_ids={"spotify": "sp_bad", "musicbrainz": "mb_std"}, + file_tracks=files, fetch_tracklist=_fetcher(table), + source_priority=PRIORITY, mode="active_only", + ) + assert out is None # primary didn't fit, and active_only won't consider others + + +def test_result_includes_breakdown_and_candidate_comparison(): + files = list(STD) + table = {("spotify", "sp1"): DLX, ("deezer", "dz1"): STD} + out = resolve_canonical_for_album( + album_source_ids={"spotify": "sp1", "deezer": "dz1"}, + file_tracks=files, fetch_tracklist=_fetcher(table), + source_priority=["spotify", "deezer", "itunes", "musicbrainz"], + mode="best_fit", + ) + assert out["source"] == "deezer" + assert out["file_track_count"] == 11 + assert out["release_track_count"] == 11 + assert out["count_fit"] == 1.0 and out["duration_fit"] == 1.0 and out["title_fit"] == 1.0 + by_src = {c["source"]: c for c in out["candidates"]} + assert by_src["deezer"]["track_count"] == 11 and by_src["deezer"]["score"] > 0.9 + assert by_src["spotify"]["track_count"] == 17 and by_src["spotify"]["score"] < 0.8 + + +def test_active_only_pins_primary_when_it_fits(): + files = list(STD) + table = {("spotify", "sp1"): STD, ("musicbrainz", "mb1"): STD} + out = resolve_canonical_for_album( + album_source_ids={"spotify": "sp1", "musicbrainz": "mb1"}, + file_tracks=files, fetch_tracklist=_fetcher(table), + source_priority=PRIORITY, mode="active_only", + ) + assert out["source"] == "spotify" + + +def test_priority_breaks_tie_between_equal_fits(): + files = list(STD) + table = {("spotify", "a"): STD, ("itunes", "b"): STD} # identical fit + out = resolve_canonical_for_album( + album_source_ids={"itunes": "b", "spotify": "a"}, + file_tracks=files, + fetch_tracklist=_fetcher(table), + source_priority=PRIORITY, # spotify before itunes + ) + assert out["source"] == "spotify" + + +def test_skips_sources_without_ids_or_failed_fetch(): + files = list(STD) + + def fetch(source, album_id): + if source == "spotify": + raise RuntimeError("API down") + if source == "deezer": + return STD + return None + + out = resolve_canonical_for_album( + album_source_ids={"spotify": "x", "deezer": "y"}, # no itunes id + file_tracks=files, + fetch_tracklist=fetch, + source_priority=PRIORITY, + ) + assert out["source"] == "deezer" + + +def test_none_when_no_candidates(): + out = resolve_canonical_for_album( + album_source_ids={}, + file_tracks=list(STD), + fetch_tracklist=_fetcher({}), + source_priority=PRIORITY, + ) + assert out is None + + +def test_none_when_no_files(): + out = resolve_canonical_for_album( + album_source_ids={"spotify": "a"}, + file_tracks=[], + fetch_tracklist=_fetcher({("spotify", "a"): STD}), + source_priority=PRIORITY, + ) + assert out is None + + +def test_none_when_below_floor(): + files = list(STD) # 11 tracks + # Only candidate is a wildly-wrong 3-track release. + table = {("spotify", "a"): [{"duration_ms": 60_000, "title": "X"}] * 3} + out = resolve_canonical_for_album( + album_source_ids={"spotify": "a"}, + file_tracks=files, + fetch_tracklist=_fetcher(table), + source_priority=PRIORITY, + ) + assert out is None + + +def test_score_is_rounded(): + out = resolve_canonical_for_album( + album_source_ids={"spotify": "a"}, + file_tracks=list(STD), + fetch_tracklist=_fetcher({("spotify", "a"): STD}), + source_priority=PRIORITY, + ) + assert out["score"] == round(out["score"], 4) diff --git a/tests/test_canonical_version.py b/tests/test_canonical_version.py new file mode 100644 index 00000000..bb8b8558 --- /dev/null +++ b/tests/test_canonical_version.py @@ -0,0 +1,143 @@ +"""Extreme battery for canonical-album-version scoring (#765 / #767-Bug2). + +The scorer must: pick the right EDITION by best-fit to the files on disk +(standard when you have the standard, deluxe when you have the deluxe), break +ties deterministically toward the higher-priority candidate (so every tool +agrees), degrade gracefully when durations/titles are missing, and never pin a +low-confidence guess. +""" + +from __future__ import annotations + +from core.metadata.canonical_version import ( + pick_canonical_release, + score_release_against_files, +) + + +# Helpers — build track lists ---------------------------------------------- + +def _tracks(n, base_ms=180_000, step_ms=10_000, titles=None): + """n tracks with distinct, deterministic durations + optional titles.""" + out = [] + for i in range(n): + t = {"duration_ms": base_ms + i * step_ms, "track_number": i + 1} + if titles: + t["title"] = titles[i] + out.append(t) + return out + + +STANDARD_TITLES = [f"Song {i+1}" for i in range(11)] +DELUXE_TITLES = STANDARD_TITLES + [f"Bonus {i+1}" for i in range(6)] + + +# ── edition discrimination ──────────────────────────────────────────────── + +def test_eleven_files_prefer_standard_over_deluxe(): + files = _tracks(11, titles=STANDARD_TITLES) + standard = _tracks(11, titles=STANDARD_TITLES) + deluxe = _tracks(17, titles=DELUXE_TITLES) + s_std = score_release_against_files(files, standard) + s_dlx = score_release_against_files(files, deluxe) + assert s_std > s_dlx + best, score = pick_canonical_release( + files, + [{"source": "standard", "tracks": standard}, {"source": "deluxe", "tracks": deluxe}], + ) + assert best["source"] == "standard" and score > 0.9 + + +def test_seventeen_files_prefer_deluxe(): + files = _tracks(17, titles=DELUXE_TITLES) + standard = _tracks(11, titles=STANDARD_TITLES) + deluxe = _tracks(17, titles=DELUXE_TITLES) + best, _ = pick_canonical_release( + files, + # deluxe deliberately listed SECOND to prove count/fit beats order here + [{"source": "standard", "tracks": standard}, {"source": "deluxe", "tracks": deluxe}], + ) + assert best["source"] == "deluxe" + + +def test_exact_count_and_durations_scores_near_one(): + files = _tracks(11, titles=STANDARD_TITLES) + assert score_release_against_files(files, _tracks(11, titles=STANDARD_TITLES)) > 0.99 + + +# ── deterministic tiebreak (the #765 resolution) ────────────────────────── + +def test_identical_releases_break_tie_to_first_candidate(): + # Same album from two sources (same files match both equally) — must pick + # the FIRST (higher-priority) deterministically so both tools agree. + files = _tracks(11, titles=STANDARD_TITLES) + a = {"source": "spotify", "tracks": _tracks(11, titles=STANDARD_TITLES)} + b = {"source": "musicbrainz", "tracks": _tracks(11, titles=STANDARD_TITLES)} + best, _ = pick_canonical_release(files, [a, b]) + assert best["source"] == "spotify" + # ...and stable when the order flips (priority is the caller's order). + best2, _ = pick_canonical_release(files, [b, a]) + assert best2["source"] == "musicbrainz" + + +# ── duration disambiguation when counts tie ─────────────────────────────── + +def test_duration_breaks_tie_when_counts_equal(): + # Two 11-track candidates; the files' durations match candidate A's lengths, + # not B's (e.g. album cuts vs radio edits). A must win on duration fit. + files = _tracks(11, base_ms=200_000, step_ms=5_000) + cand_a = {"source": "album", "tracks": _tracks(11, base_ms=200_000, step_ms=5_000)} + cand_b = {"source": "edits", "tracks": _tracks(11, base_ms=140_000, step_ms=5_000)} + best, _ = pick_canonical_release(files, [cand_b, cand_a]) # B listed first + assert best["source"] == "album" # duration fit overrides order + + +# ── graceful degradation ────────────────────────────────────────────────── + +def test_no_durations_falls_back_to_count_and_title(): + files = [{"title": t} for t in STANDARD_TITLES] # no durations + standard = [{"title": t} for t in STANDARD_TITLES] + deluxe = [{"title": t} for t in DELUXE_TITLES] + best, score = pick_canonical_release( + files, + [{"source": "standard", "tracks": standard}, {"source": "deluxe", "tracks": deluxe}], + ) + assert best["source"] == "standard" and score > 0.5 + + +def test_only_counts_available_still_scores(): + files = [{} for _ in range(11)] + assert score_release_against_files(files, [{} for _ in range(11)]) > 0.99 + assert score_release_against_files(files, [{} for _ in range(17)]) < 0.8 + + +def test_fuzzy_titles_still_match(): + files = _tracks(3, titles=["Believer", "Whatever It Takes", "Thunder"]) + rel = _tracks(3, titles=["Believer (Remastered)", "Whatever It Takes", "Thunder!"]) + assert score_release_against_files(files, rel) > 0.9 + + +# ── confidence floor / guards ───────────────────────────────────────────── + +def test_below_floor_returns_none(): + files = _tracks(11, titles=STANDARD_TITLES) + # A wildly wrong candidate (3 unrelated tracks) must not be pinned. + bad = {"source": "wrong", "tracks": _tracks(3, base_ms=60_000, titles=["X", "Y", "Z"])} + best, score = pick_canonical_release(files, [bad]) + assert best is None + assert score < 0.5 + + +def test_empty_inputs_are_safe(): + assert score_release_against_files([], _tracks(11)) == 0.0 + assert score_release_against_files(_tracks(11), []) == 0.0 + best, score = pick_canonical_release(_tracks(11), []) + assert best is None and score == 0.0 + + +def test_min_score_is_tunable(): + files = _tracks(11, titles=STANDARD_TITLES) + near = {"source": "near", "tracks": _tracks(10, titles=STANDARD_TITLES[:10])} + # default floor accepts a 10/11 fit, a strict floor rejects it + assert pick_canonical_release(files, [near])[0] is not None + assert pick_canonical_release(files, [near], min_score=0.99)[0] is None diff --git a/tests/test_canonical_version_job.py b/tests/test_canonical_version_job.py new file mode 100644 index 00000000..075fa461 --- /dev/null +++ b/tests/test_canonical_version_job.py @@ -0,0 +1,160 @@ +"""Backfill job: Resolve Canonical Album Versions (#765 Stage 2 trigger).""" + +from __future__ import annotations + +import types + +import core.repair_jobs.canonical_version_resolve as cvr +from core.repair_jobs import get_all_jobs +from core.repair_jobs.canonical_version_resolve import ( + CanonicalVersionResolveJob, + _describe_pin, +) +from database.music_database import MusicDatabase + + +def _ctx(db, findings): + return types.SimpleNamespace( + db=db, + config_manager=None, # -> active_server None -> all albums + check_stop=lambda: False, + wait_if_paused=lambda: False, + report_progress=None, + update_progress=None, + create_finding=lambda **kw: (findings.append(kw) or True), + ) + + +def _seed_two_albums(db): + conn = db._get_connection() + cur = conn.cursor() + cur.execute("INSERT INTO artists (id, name) VALUES ('art1', 'A')") + cur.execute("INSERT INTO albums (id, title, artist_id) VALUES ('alb1', 'Album One', 'art1')") + cur.execute("INSERT INTO albums (id, title, artist_id) VALUES ('alb2', 'Album Two', 'art1')") + conn.commit() + conn.close() + + +def _fake_resolver(monkeypatch): + def fake(db, album_id, *, min_score=0.5, store=True, mode="active_preferred"): + res = {"source": "spotify", "album_id": f"sp_{album_id}", "score": 0.9} + if store: + db.set_album_canonical(album_id, res["source"], res["album_id"], res["score"]) + return res + monkeypatch.setattr(cvr, "resolve_and_store_canonical_for_album", fake) + + +def test_job_is_registered(): + jobs = get_all_jobs() # {job_id: cls} + assert "canonical_version_resolve" in jobs + assert jobs["canonical_version_resolve"] is CanonicalVersionResolveJob + + +def test_job_is_opt_in_and_dry_run_by_default(): + assert CanonicalVersionResolveJob.default_enabled is False + assert CanonicalVersionResolveJob.default_settings["dry_run"] is True + + +def test_source_selection_defaults_to_active_preferred(): + assert CanonicalVersionResolveJob.default_settings["source_selection"] == "active_preferred" + + +def test_source_selection_exposes_dropdown_options(): + # The UI renders a + Organize by playlist + + `; +} + +async function setAutoSyncOrganizeByPlaylist(playlistId, enabled) { + try { + const res = await fetch(`/api/mirrored-playlists/${playlistId}/preferences`, { + method: 'PATCH', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ organize_by_playlist: !!enabled }), + }); + const data = await res.json(); + if (!res.ok || data.error) throw new Error(data.error || 'Failed to update preference'); + const pl = _autoSyncScheduleState.playlists.find(p => parseInt(p.id, 10) === parseInt(playlistId, 10)); + if (pl) pl.organize_by_playlist = !!enabled; + showToast(enabled ? 'Auto-Sync will use playlist folders' : 'Auto-Sync will use standard download layout', 'success'); + } catch (err) { + showToast(`Error: ${err.message}`, 'error'); + await refreshAutoSyncScheduleModal(); + } +} + function autoSyncScheduledCardHtml(playlist, schedule) { const enabled = schedule?.enabled !== false; const nextLabel = schedule?.next_run ? autoSyncNextRunLabel(schedule.next_run) : ''; @@ -1592,6 +1621,7 @@ function autoSyncScheduledCardHtml(playlist, schedule) { ${_esc(playlist.name)}
${_esc(autoSyncSourceLabel(playlist.source))} · ${playlist.track_count || 0} tracks
+ ${autoSyncOrganizeToggleHtml(playlist)}
${_esc(autoSyncIntervalLabel(schedule?.hours || 24))} ${nextLabel ? `${_esc(nextLabel)}` : ''} diff --git a/webui/static/core.js b/webui/static/core.js index 2ba0d6b2..48843c3e 100644 --- a/webui/static/core.js +++ b/webui/static/core.js @@ -50,6 +50,7 @@ let _lastWatchlistScanStatus = null; let _lastMediaScanStatus = null; let _lastWishlistStats = null; let playlistTrackCache = {}; // Key: playlist_id, Value: tracks array +let playlistTrackSnapshotCache = {}; // Key: playlist_id, Value: upstream snapshot_id at cache time let spotifyPlaylistsLoaded = false; let activeDownloadProcesses = {}; let sequentialSyncManager = null; @@ -381,6 +382,10 @@ function initializeWebSocket() { } socket = io({ + // Polling-first (Socket.IO default) then upgrade — most compatible behind + // reverse proxies that don't cleanly forward WebSocket upgrade headers + // (common in self-hosted setups). websocket-first shaves connect time when + // it works but silently breaks real-time updates where the proxy blocks WS. transports: ['polling', 'websocket'], reconnection: true, reconnectionAttempts: Infinity, @@ -459,12 +464,25 @@ function initializeWebSocket() { socket.on('enrichment:tidal-enrichment', (data) => updateTidalEnrichmentStatusFromData(data)); socket.on('enrichment:qobuz-enrichment', (data) => updateQobuzEnrichmentStatusFromData(data)); socket.on('enrichment:amazon-enrichment', (data) => updateAmazonEnrichmentStatusFromData(data)); + socket.on('enrichment:similar_artists', (data) => updateSimilarArtistsEnrichmentStatusFromData(data)); socket.on('enrichment:hydrabase', (data) => updateHydrabaseStatusFromData(data)); socket.on('enrichment:repair', (data) => updateRepairStatusFromData(data)); socket.on('enrichment:soulid', (data) => updateSoulIDStatusFromData(data)); socket.on('enrichment:listening-stats', () => { }); // Status only, no UI update needed socket.on('repair:progress', (data) => updateRepairJobProgressFromData(data)); + // Forward enrichment status to the dashboard worker-orbs so the hub fires + // a pulse on each real item matched / error (additional listener — does not + // disturb the UI handlers above). + ['musicbrainz', 'audiodb', 'discogs', 'deezer', 'spotify-enrichment', + 'itunes-enrichment', 'lastfm-enrichment', 'genius-enrichment', 'tidal-enrichment', + 'qobuz-enrichment', 'amazon-enrichment', 'similar_artists', 'hydrabase', + 'soulid', 'repair'].forEach((ch) => { + socket.on('enrichment:' + ch, (data) => { + if (window.workerOrbs && window.workerOrbs.onStatus) window.workerOrbs.onStatus(ch, data); + }); + }); + // Phase 4 event listeners (tool progress) socket.on('tool:stream', (data) => updateStreamStatusFromData(data)); socket.on('tool:quality-scanner', (data) => updateQualityScanProgressFromData(data)); @@ -521,6 +539,7 @@ function handleServiceStatusUpdate(data) { _isSoulsyncStandalone = isSoulsyncStandalone; document.querySelectorAll('.sync-to-server-btn, [id$="-sync-btn"], [onclick*="startPlaylistSync"], [onclick*="syncPlaylistToServer"], [onclick*="startDecadeSync"]').forEach(btn => { if (btn.id === 'stats-sync-btn') return; // React stats page owns this control now. + if (btn.classList.contains('soulsync-standalone-action')) return; if (isSoulsyncStandalone) { btn.dataset.hiddenByStandalone = '1'; btn.style.display = 'none'; diff --git a/webui/static/discover.js b/webui/static/discover.js index a4b28736..3790ef00 100644 --- a/webui/static/discover.js +++ b/webui/static/discover.js @@ -27,6 +27,7 @@ async function loadDiscoverPage() { // Load all sections await Promise.all([ loadDiscoverHero(), + loadRecommendedArtistsSection(), loadYourArtists(), loadYourAlbums(), loadDiscoverRecentReleases(), @@ -197,14 +198,10 @@ function displayDiscoverHeroArtist(artist) { } if (subtitleEl) { - // Show recommendation context based on occurrence count - let subtitle = ''; - if (artist.occurrence_count > 1) { - subtitle = `Similar to ${artist.occurrence_count} artists in your watchlist`; - } else { - subtitle = 'Similar to an artist in your watchlist'; - } - subtitleEl.textContent = subtitle; + // Prefer the real "because you have X, Y" names; fall back to a + // library-wide occurrence count (no longer watchlist-only). + subtitleEl.textContent = _recommendationReason(artist); + subtitleEl.title = _recommendationReasonTitle(artist); } // Build metadata section with popularity and genres @@ -388,6 +385,30 @@ async function watchAllHeroArtists(btn) { let _recommendedArtistsCache = null; let _recommendedArtistsSource = null; +// Builds the "because you have X, Y" explanation for a recommendation. +// Prefers the real contributing artist names from the backend (the `because` +// array); falls back to an occurrence count — which is now LIBRARY-wide, not +// just watchlist, thanks to the similar-artists enrichment worker. +function _recommendationReason(artist) { + const names = (artist && artist.because) || []; + if (names.length === 1) return `Because you have ${escapeHtml(names[0])}`; + if (names.length === 2) return `Because you have ${escapeHtml(names[0])} & ${escapeHtml(names[1])}`; + if (names.length >= 3) { + const shown = names.slice(0, 2).map(escapeHtml).join(', '); + return `Because you have ${shown} +${names.length - 2} more`; + } + const n = (artist && artist.occurrence_count) || 0; + return n > 1 + ? `Similar to ${n} artists in your library` + : 'Similar to an artist in your library'; +} + +// Full contributing-artist list for a hover tooltip (when there are names). +function _recommendationReasonTitle(artist) { + const names = (artist && artist.because) || []; + return names.length ? `In your library: ${names.join(', ')}` : ''; +} + async function openRecommendedArtistsModal() { let modal = document.getElementById('recommended-artists-modal'); if (!modal) { @@ -553,9 +574,8 @@ function renderRecommendedArtistsModal(modal, artists, source = null) { const genreTags = (artist.genres || []).slice(0, 3).map(g => `${escapeHtml(g)}` ).join(''); - const similarText = artist.occurrence_count > 1 - ? `Similar to ${artist.occurrence_count} in your watchlist` - : 'Similar to an artist in your watchlist'; + const similarText = _recommendationReason(artist); + const similarTitle = _recommendationReasonTitle(artist); const artistSource = artist.source || source || _recommendedArtistsSource || ''; return `
`; @@ -2633,6 +2656,9 @@ function showDeezerArlPlaylistDetailsModal(playlist, originalDeezerPlaylistId) { } modal.style.display = 'flex'; + if (typeof loadPlaylistOrganizePreferenceIntoModal === 'function') { + void loadPlaylistOrganizePreferenceIntoModal(playlistId, 'deezer'); + } } function closeDeezerArlPlaylistDetailsModal() { @@ -3596,7 +3622,7 @@ function updateDeezerModalButtons(urlHash, phase) { const footerLeft = modal.querySelector('.modal-footer-left'); if (footerLeft) { - footerLeft.innerHTML = getModalActionButtons(urlHash, phase); + setDiscoveryModalFooterActions(urlHash, phase); } } @@ -5621,7 +5647,7 @@ function updateBeatportModalButtons(urlHash, phase) { const footerLeft = modal.querySelector('.modal-footer-left'); if (footerLeft) { - footerLeft.innerHTML = getModalActionButtons(urlHash, phase); + setDiscoveryModalFooterActions(urlHash, phase); } } @@ -7582,11 +7608,11 @@ function updateSpotifyPublicModalButtons(urlHash, phase) { const footerLeft = modal.querySelector('.modal-footer-left'); if (footerLeft) { - footerLeft.innerHTML = getModalActionButtons(urlHash, phase); + setDiscoveryModalFooterActions(urlHash, phase); } } -async function startSpotifyPublicDownloadMissing(urlHash) { +async function startSpotifyPublicDownloadMissing(urlHash, forcePlaylistFolder = false) { try { console.log('🔍 Starting download missing tracks for Spotify public playlist:', urlHash); @@ -7648,7 +7674,9 @@ async function startSpotifyPublicDownloadMissing(urlHash) { discoveryModal.classList.add('hidden'); } - await openDownloadMissingModalForTidal(virtualPlaylistId, playlistName, spotifyTracks); + await openDownloadMissingModalForTidal(virtualPlaylistId, playlistName, spotifyTracks, { + forcePlaylistFolder: !!forcePlaylistFolder, + }); } catch (error) { console.error('Error starting Spotify public download missing:', error); @@ -8606,7 +8634,7 @@ function updateITunesLinkModalButtons(urlHash, phase) { const footerLeft = modal.querySelector('.modal-footer-left'); if (footerLeft) { - footerLeft.innerHTML = getModalActionButtons(urlHash, phase); + setDiscoveryModalFooterActions(urlHash, phase); } } @@ -9431,7 +9459,7 @@ function openYouTubeDiscoveryModal(urlHash) { `; modal.style.display = 'flex'; + if (typeof loadPlaylistOrganizePreferenceIntoModal === 'function') { + void loadPlaylistOrganizePreferenceIntoModal(playlist.id, 'spotify'); + } } function closePlaylistDetailsModal() { @@ -2184,19 +2194,39 @@ async function openDownloadMissingModal(playlistId) { showLoadingOverlay('Loading playlist...'); // **NEW**: Check if a process is already active for this playlist - if (activeDownloadProcesses[playlistId]) { + const playlistMeta = spotifyPlaylists.find(p => p.id === playlistId); + const processStale = typeof isPlaylistDownloadProcessStale === 'function' + ? isPlaylistDownloadProcessStale(playlistId, playlistMeta) + : (typeof playlistTrackCacheIsStale === 'function' + && playlistTrackCacheIsStale(playlistId, playlistMeta)); + + if (activeDownloadProcesses[playlistId] && !processStale) { console.log(`Modal for ${playlistId} already exists. Showing it.`); - closePlaylistDetailsModal(); // Close playlist details modal even when reusing existing modal + closePlaylistDetailsModal(); const process = activeDownloadProcesses[playlistId]; if (process.modalElement) { - // Show helpful message if it's a completed process if (process.status === 'complete') { - showToast('Showing previous results. Close this modal to start a new analysis.', 'info'); + showToast('Showing previous results. Use "Download Missing (New)" for a fresh run.', 'info'); } process.modalElement.style.display = 'flex'; } + if (typeof refreshOrganizePreferenceForDownloadModal === 'function') { + await refreshOrganizePreferenceForDownloadModal(playlistId); + } hideLoadingOverlay(); - return; // Don't create a new one + return; + } + if (processStale && activeDownloadProcesses[playlistId]) { + if (typeof restartPlaylistDownloadMissing === 'function') { + await restartPlaylistDownloadMissing(playlistId); + hideLoadingOverlay(); + return; + } + if (typeof clearPlaylistDownloadProcess === 'function') { + clearPlaylistDownloadProcess(playlistId); + } else if (typeof invalidatePlaylistTrackCache === 'function') { + invalidatePlaylistTrackCache(playlistId); + } } console.log(`📥 Opening Download Missing Tracks modal for playlist: ${playlistId}`); @@ -2210,16 +2240,29 @@ async function openDownloadMissingModal(playlistId) { } let tracks = playlistTrackCache[playlistId]; - if (!tracks) { + const needFreshTracks = !tracks || ( + typeof playlistTrackCacheIsStale === 'function' + && playlistTrackCacheIsStale(playlistId, playlist) + ); + if (needFreshTracks) { try { - const fetchUrl = playlistId.startsWith('deezer_arl_') - ? `/api/deezer/arl-playlist/${playlistId.replace('deezer_arl_', '')}` - : `/api/spotify/playlist/${playlistId}`; - const response = await fetch(fetchUrl); - const fullPlaylist = await response.json(); - if (fullPlaylist.error) throw new Error(fullPlaylist.error); - tracks = fullPlaylist.tracks; - playlistTrackCache[playlistId] = tracks; + if (playlistId.startsWith('deezer_arl_')) { + const fetchUrl = `/api/deezer/arl-playlist/${playlistId.replace('deezer_arl_', '')}`; + const response = await fetch(fetchUrl); + const fullPlaylist = await response.json(); + if (fullPlaylist.error) throw new Error(fullPlaylist.error); + tracks = fullPlaylist.tracks; + playlistTrackCache[playlistId] = tracks; + } else if (typeof fetchAndCacheSpotifyPlaylistTracks === 'function') { + const fullPlaylist = await fetchAndCacheSpotifyPlaylistTracks(playlistId); + tracks = fullPlaylist.tracks; + } else { + const response = await fetch(`/api/spotify/playlist/${playlistId}`); + const fullPlaylist = await response.json(); + if (fullPlaylist.error) throw new Error(fullPlaylist.error); + tracks = fullPlaylist.tracks; + playlistTrackCache[playlistId] = tracks; + } } catch (error) { showToast(`Failed to fetch tracks: ${error.message}`, 'error'); hideLoadingOverlay(); @@ -2335,10 +2378,9 @@ async function openDownloadMissingModal(playlistId) { Force Download All - + ${typeof downloadMissingModalOrganizeCheckboxHtml === 'function' + ? downloadMissingModalOrganizeCheckboxHtml(playlistId) + : ``}