diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index f920a0ce..c3d2399f 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -9,9 +9,9 @@ on: workflow_dispatch: inputs: version_tag: - description: 'Version tag (e.g. 2.4.3)' + description: 'Version tag (e.g. 2.5.1)' required: true - default: '2.4.3' + default: '2.5.1' jobs: build-and-push: diff --git a/Dockerfile b/Dockerfile index d4caefe5..1e2d77b9 100644 --- a/Dockerfile +++ b/Dockerfile @@ -57,8 +57,13 @@ COPY --chown=soulsync:soulsync . . # Create runtime mount-point directories the app expects to exist. # NOTE: /app/data is for database FILES, /app/database is the Python package -RUN mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/MusicVideos /app/scripts && \ - chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/MusicVideos /app/scripts +# NOTE: /app/Staging is required even though most users bind-mount it — +# the entrypoint mkdir runs early and is gated by `set -e`, so a missing +# pre-baked directory would crash the container into a restart loop on +# rootless Docker/Podman where in-container "root" can't write to /app. +# Pre-baking the dir here makes the entrypoint mkdir a guaranteed no-op. +RUN mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/MusicVideos /app/scripts && \ + chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/MusicVideos /app/scripts # Create defaults directory and copy template files # These will be used by entrypoint.sh to initialize empty volumes diff --git a/config/settings.py b/config/settings.py index 18c42124..2cfc3eeb 100644 --- a/config/settings.py +++ b/config/settings.py @@ -466,7 +466,14 @@ class ConfigManager: "download_path": "./downloads", "transfer_path": "./Transfer", "max_peer_queue": 0, - "download_timeout": 600 + "download_timeout": 600, + # Reddit report (YeloMelo95, Bell Canada): the existing + # 35-per-220s sliding-window cap allows all 35 searches in + # rapid succession before throttling — that burst trips ISP + # anti-abuse. This knob forces a min gap between consecutive + # searches even when the window cap isn't hit. 0 = disabled + # (preserves prior behavior). + "search_min_delay_seconds": 0, }, "download_source": { "mode": "soulseek", # Options: "soulseek", "youtube", "tidal", "qobuz", "hifi", "hybrid" diff --git a/core/audiodb_worker.py b/core/audiodb_worker.py index 37c404d9..ec687346 100644 --- a/core/audiodb_worker.py +++ b/core/audiodb_worker.py @@ -200,42 +200,45 @@ class AudioDBWorker: if row: return {'type': 'track', 'id': row[0], 'name': row[1], 'artist': row[2], 'artist_audiodb_id': row[3]} - # Priority 4: Retry 'not_found' artists after retry_days - not_found_cutoff = datetime.now() - timedelta(days=self.retry_days) + # Priority 4: Retry 'not_found' OR 'error' artists after retry_days. + # 'error' status covers transient AudioDB outages (timeouts, 500s) + # that the issue-#553 fix marks rather than leaving NULL — without + # this retry path those rows would stay errored forever. + retry_cutoff = datetime.now() - timedelta(days=self.retry_days) cursor.execute(""" SELECT id, name FROM artists - WHERE audiodb_match_status = 'not_found' AND audiodb_last_attempted < ? + WHERE audiodb_match_status IN ('not_found', 'error') AND audiodb_last_attempted < ? ORDER BY audiodb_last_attempted ASC LIMIT 1 - """, (not_found_cutoff,)) + """, (retry_cutoff,)) row = cursor.fetchone() if row: logger.info(f"Retrying artist '{row[1]}' (last attempted before cutoff)") return {'type': 'artist', 'id': row[0], 'name': row[1]} - # Priority 5: Retry 'not_found' albums + # Priority 5: Retry 'not_found' OR 'error' albums cursor.execute(""" SELECT a.id, a.title, ar.name AS artist_name, ar.audiodb_id AS artist_audiodb_id FROM albums a JOIN artists ar ON a.artist_id = ar.id - WHERE a.audiodb_match_status = 'not_found' AND a.audiodb_last_attempted < ? + WHERE a.audiodb_match_status IN ('not_found', 'error') AND a.audiodb_last_attempted < ? ORDER BY a.audiodb_last_attempted ASC LIMIT 1 - """, (not_found_cutoff,)) + """, (retry_cutoff,)) row = cursor.fetchone() if row: return {'type': 'album', 'id': row[0], 'name': row[1], 'artist': row[2], 'artist_audiodb_id': row[3]} - # Priority 6: Retry 'not_found' tracks + # Priority 6: Retry 'not_found' OR 'error' tracks cursor.execute(""" SELECT t.id, t.title, ar.name AS artist_name, ar.audiodb_id AS artist_audiodb_id FROM tracks t JOIN artists ar ON t.artist_id = ar.id - WHERE t.audiodb_match_status = 'not_found' AND t.audiodb_last_attempted < ? + WHERE t.audiodb_match_status IN ('not_found', 'error') AND t.audiodb_last_attempted < ? ORDER BY t.audiodb_last_attempted ASC LIMIT 1 - """, (not_found_cutoff,)) + """, (retry_cutoff,)) row = cursor.fetchone() if row: return {'type': 'track', 'id': row[0], 'name': row[1], 'artist': row[2], 'artist_audiodb_id': row[3]} @@ -374,8 +377,22 @@ class AudioDBWorker: return except Exception as e: logger.warning(f"Direct lookup failed for existing AudioDB ID {existing_id}: {e}") - # Direct lookup failed — don't overwrite manual match - logger.debug(f"Preserving manual match for {item_type} '{item_name}' (AudioDB ID: {existing_id})") + # Direct lookup returned no metadata (None) or raised — don't + # fall through to the name-search path below, which could + # overwrite a manually-matched audiodb_id with a wrong guess. + # Mark status='error' so the queue's NULL-status filter stops + # re-picking this row on every tick (issue #553: AudioDB + # `track.php` timeouts caused infinite enrichment loops as + # the row was repeatedly picked + re-attempted because it + # never left the NULL state). The error-retry priority block + # in `_get_next_item` re-attempts after `retry_days` so + # transient AudioDB outages still recover automatically. + self._mark_status(item_type, item_id, 'error') + self.stats['errors'] += 1 + logger.debug( + f"Preserving manual match for {item_type} '{item_name}' " + f"(AudioDB ID: {existing_id}); marked error pending retry" + ) return if item_type == 'artist': diff --git a/core/auto_import_worker.py b/core/auto_import_worker.py index 910f6808..2b49bf48 100644 --- a/core/auto_import_worker.py +++ b/core/auto_import_worker.py @@ -199,6 +199,53 @@ def _quality_rank(ext: str) -> int: return ranks.get(ext.lower(), 1) +# Weight constants for `_score_album_search_result` — exposed at module +# level so they're greppable + bumpable in one place. Pre-fix these were +# magic numbers inline. +_ALBUM_NAME_WEIGHT = 0.5 # title fuzzy similarity +_ARTIST_NAME_WEIGHT = 0.2 # primary artist fuzzy similarity (skipped when target is empty) +_TRACK_COUNT_WEIGHT = 0.3 # how close the source's track count is to the file count + + +def _score_album_search_result(album_result, target_album: str, + target_artist: Optional[str], + file_count: int) -> float: + """Pure scoring helper for `_search_metadata_source`. + + Weights how well an `album_result` from a metadata source's + `search_albums` matches the search inputs. Returns float in [0.0, 1.0]. + Pre-extraction this lived inline in the loop body; lifting it out + lets the weight math be pinned independently of the orchestrator + (per-source iteration, exception containment, threshold check). + + `album_result` is expected to expose: + - `.name` (str) + - `.artists` (list of dict-like with 'name', optional 'id') or list[str] + - `.total_tracks` (int, optional) + """ + score = 0.0 + + # Album name similarity (default 50%) + name = getattr(album_result, 'name', '') or '' + score += _similarity(target_album, name) * _ALBUM_NAME_WEIGHT + + # Artist similarity (default 20%) — only when target_artist provided + if target_artist: + artists = getattr(album_result, 'artists', None) or [] + r_artist = artists[0] if artists else '' + if isinstance(r_artist, dict): + r_artist = r_artist.get('name', '') + score += _similarity(target_artist, str(r_artist)) * _ARTIST_NAME_WEIGHT + + # Track count match (default 30%) — only when both sides have a count + r_tracks = getattr(album_result, 'total_tracks', 0) or 0 + if r_tracks > 0 and file_count > 0: + count_ratio = 1.0 - abs(r_tracks - file_count) / max(r_tracks, file_count) + score += max(0.0, count_ratio) * _TRACK_COUNT_WEIGHT + + return score + + class AutoImportWorker: """Background worker that watches the staging folder and auto-imports music. @@ -1260,87 +1307,120 @@ class AutoImportWorker: def _search_metadata_source(self, artist: Optional[str], album: str, method: str, candidate: FolderCandidate, query: str = None) -> Optional[Dict]: - """Search the active metadata source for an album match.""" + """Search configured metadata sources for an album match. + + Iterates `get_source_priority(get_primary_source())` so primary + is tried first and the rest are tried as fallback. Returns the + FIRST source whose best result clears the 0.4 score threshold. + + Pre-fix this only queried the primary, which meant indie/niche + albums missing from the user's primary (e.g. Bandcamp releases + not on Spotify) failed auto-import even when manual search + could find them on Tidal/Deezer. The manual search bar at the + bottom of the Import tab already iterates the full source + chain via `search_import_albums` — this aligns auto-import + with that behavior. + """ try: - from core.metadata_service import get_primary_source, get_client_for_source - - source = get_primary_source() - client = get_client_for_source(source) - if not client or not hasattr(client, 'search_albums'): - return None + from core.metadata_service import ( + get_primary_source, + get_source_priority, + get_client_for_source, + ) + primary_source = get_primary_source() + source_chain = get_source_priority(primary_source) search_query = query or (f"{artist} {album}" if artist else album) - results = client.search_albums(search_query, limit=5) - if not results: - return None - # Score each result - best_result = None - best_score = 0 + for source in source_chain: + client = get_client_for_source(source) + if not client or not hasattr(client, 'search_albums'): + continue - for r in results: - score = 0 - # Album name similarity (50%) - score += _similarity(album, r.name) * 0.5 - # Artist similarity (20%) - if artist: - r_artist = r.artists[0] if hasattr(r, 'artists') and r.artists else '' - if isinstance(r_artist, dict): - r_artist = r_artist.get('name', '') - score += _similarity(artist, str(r_artist)) * 0.2 - # Track count match (30%) - r_tracks = getattr(r, 'total_tracks', 0) or 0 + try: + results = client.search_albums(search_query, limit=5) + except Exception as e: + # Per-source failures (rate limit, auth, transient HTTP) + # shouldn't abort the fallback chain. Log + continue. + logger.debug( + f"Auto-import: search_albums failed on {source}: {e}" + ) + continue + + if not results: + continue + + # Score each result via the pure helper. Helper is + # tested independently in + # `tests/imports/test_album_search_scoring.py` so the + # weight math is pinned at the function boundary, not + # through the orchestrator path. file_count = len(candidate.audio_files) - if r_tracks > 0 and file_count > 0: - count_ratio = 1.0 - abs(r_tracks - file_count) / max(r_tracks, file_count) - score += max(0, count_ratio) * 0.3 + best_result = None + best_score = 0.0 + for r in results: + score = _score_album_search_result(r, album, artist, file_count) + if score > best_score: + best_score = score + best_result = r - if score > best_score: - best_score = score - best_result = r + if not best_result or best_score < 0.4: + # Primary returned weak/no match — fall through to next source + if source != primary_source: + logger.debug( + f"Auto-import: {source} best score {best_score:.2f} " + f"below threshold for '{album}', trying next source" + ) + continue - if not best_result or best_score < 0.4: - return None + # Get image + image_url = '' + if hasattr(best_result, 'image_url'): + image_url = best_result.image_url or '' + elif hasattr(best_result, 'images') and best_result.images: + img = best_result.images[0] + image_url = img.get('url', '') if isinstance(img, dict) else str(img) - # Get image - image_url = '' - if hasattr(best_result, 'image_url'): - image_url = best_result.image_url or '' - elif hasattr(best_result, 'images') and best_result.images: - img = best_result.images[0] - image_url = img.get('url', '') if isinstance(img, dict) else str(img) + r_artist = '' + r_artist_id = '' + if hasattr(best_result, 'artists') and best_result.artists: + a = best_result.artists[0] + if isinstance(a, dict): + r_artist = a.get('name', str(a)) + # Surface the metadata-source artist ID so the + # standalone-library write can land it on the right + # `_artist_id` column. Without this the + # artists row gets created but with NULL on the + # source-id, and watchlist scans can't recognise + # the artist as already in library by stable ID. + r_artist_id = str(a.get('id', '') or '') + else: + r_artist = str(a) - r_artist = '' - r_artist_id = '' - if hasattr(best_result, 'artists') and best_result.artists: - a = best_result.artists[0] - if isinstance(a, dict): - r_artist = a.get('name', str(a)) - # Surface the metadata-source artist ID so the - # standalone-library write can land it on the right - # `_artist_id` column. Without this the - # artists row gets created but with NULL on the - # source-id, and watchlist scans can't recognise - # the artist as already in library by stable ID. - r_artist_id = str(a.get('id', '') or '') - else: - r_artist = str(a) + # Get release date + release_date = getattr(best_result, 'release_date', '') or '' - # Get release date - release_date = getattr(best_result, 'release_date', '') or '' + if source != primary_source: + logger.info( + f"Auto-import: identified '{album}' via fallback " + f"source {source!r} (score {best_score:.2f}, primary " + f"{primary_source!r} returned nothing usable)" + ) - return { - 'album_id': best_result.id, - 'album_name': best_result.name, - 'artist_name': r_artist or artist or '', - 'artist_id': r_artist_id, - 'image_url': image_url, - 'release_date': release_date, - 'total_tracks': getattr(best_result, 'total_tracks', 0), - 'source': source, - 'method': method, - 'identification_confidence': best_score, - } + return { + 'album_id': best_result.id, + 'album_name': best_result.name, + 'artist_name': r_artist or artist or '', + 'artist_id': r_artist_id, + 'image_url': image_url, + 'release_date': release_date, + 'total_tracks': getattr(best_result, 'total_tracks', 0), + 'source': source, + 'method': method, + 'identification_confidence': best_score, + } + + return None except Exception as e: logger.debug(f"Metadata search failed for '{album}': {e}") diff --git a/core/debug_info.py b/core/debug_info.py index 88a5ece9..852c3463 100644 --- a/core/debug_info.py +++ b/core/debug_info.py @@ -14,7 +14,12 @@ from pathlib import Path from flask import jsonify, request from config.settings import config_manager -from core.metadata.registry import get_spotify_client +from core.metadata.registry import ( + get_spotify_client, + get_primary_source, + is_hydrabase_enabled, +) +from core.metadata.status import get_spotify_status logger = logging.getLogger(__name__) @@ -183,21 +188,47 @@ def get_debug_info(): info['paths']['music_videos_path'] = music_videos_path info['paths']['music_videos_path_exists'] = os.path.isdir(music_videos_path) - # Services from status cache - spotify_cache = _status_cache.get('spotify', {}) + # Services. `_status_cache` only carries 'media_server' and 'soulseek' + # (no 'spotify' key) so anything we used to read from `spotify_cache` + # silently defaulted to the missing-value fallback — that's the + # "music_source: unknown" bug. Spotify status now comes from the + # canonical `get_spotify_status` accessor; primary metadata source + # comes from `get_primary_source` (which already accounts for the + # auth-fallback chain — Spotify drops back to Deezer when not + # authenticated). media_server_cache = _status_cache.get('media_server', {}) soulseek_cache = _status_cache.get('soulseek', {}) + spotify_status = _safe_check(lambda: get_spotify_status(spotify_client=spotify_client), default={}) + if not isinstance(spotify_status, dict): + spotify_status = {} info['services'] = { - 'music_source': spotify_cache.get('source', 'unknown'), - 'spotify_connected': spotify_cache.get('connected', False), - 'spotify_rate_limited': spotify_cache.get('rate_limited', False), + 'music_source': _safe_check(get_primary_source, default='unknown') or 'unknown', + 'spotify_connected': bool(spotify_status.get('connected', False)), + 'spotify_rate_limited': bool(spotify_status.get('rate_limited', False)), 'media_server_type': media_server_cache.get('type', 'none'), 'media_server_connected': media_server_cache.get('connected', False), 'soulseek_connected': soulseek_cache.get('connected', False), 'download_source': config_manager.get('download_source.mode', 'hybrid'), 'tidal_connected': _safe_check(lambda: bool(tidal_client and tidal_client.is_authenticated())), 'qobuz_connected': _safe_check(lambda: bool(qobuz_enrichment_worker and qobuz_enrichment_worker.client and qobuz_enrichment_worker.client.is_authenticated())), + 'hydrabase_connected': _safe_check(is_hydrabase_enabled), + # YouTube is URL-based via yt-dlp — no auth, always reachable as + # long as the binary is installed. Surfaced so the debug dump + # documents that YouTube is one of the available download sources + # rather than implying it doesn't exist. + 'youtube_available': True, } + # HiFi instance count — separate from connection status because each + # instance is its own independent endpoint with its own auth state. + info['services']['hifi_instance_count'] = _safe_check( + lambda: len(get_database().get_hifi_instances()), default=0 + ) + # Always-available public metadata sources (no auth, no per-user + # connection state). Listed so the debug dump reflects the full + # metadata surface SoulSync queries from, not just the auth-gated ones. + info['services']['always_available_metadata_sources'] = [ + 'deezer', 'itunes', 'musicbrainz', + ] # Enrichment workers workers = {} @@ -300,7 +331,11 @@ def get_debug_info(): # API rate monitor — current calls/min, 24h totals, peaks, rate limit events try: from core.api_call_tracker import api_call_tracker - from core.metadata.status import get_spotify_status + # `get_spotify_status` is already imported at module level. A + # local re-import here would make Python treat the name as a + # function-scoped local for the WHOLE body, breaking the lambda + # at the top of get_debug_info that closes over the module-level + # binding (Python 3.12 NameError on free variables). rates = api_call_tracker.get_all_rates() info['api_rates'] = rates # Rich 24h debug summary with peaks, totals, per-endpoint breakdown, events diff --git a/core/discovery/sync.py b/core/discovery/sync.py index 1f068450..7d63a51f 100644 --- a/core/discovery/sync.py +++ b/core/discovery/sync.py @@ -49,7 +49,7 @@ class SyncDeps: sync_lock: Any # threading.Lock -def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, profile_id=1, playlist_image_url='', deps: SyncDeps = None): +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'): """The actual sync function that runs in the background thread.""" sync_states = deps.sync_states sync_lock = deps.sync_lock @@ -359,7 +359,7 @@ def run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, p sync_service._skip_wishlist = is_wing_it # 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)) + result = deps.run_async(sync_service.sync_playlist(playlist, download_missing=False, profile_id=profile_id, sync_mode=sync_mode)) # Clear progress callback immediately to prevent race condition where a # late-firing progress callback overwrites the "finished" state below diff --git a/core/jellyfin_client.py b/core/jellyfin_client.py index 1257d4ab..20a2594c 100644 --- a/core/jellyfin_client.py +++ b/core/jellyfin_client.py @@ -1533,6 +1533,78 @@ class JellyfinClient(MediaServerClient): logger.debug(f"Could not set playlist poster for '{playlist_name}': {e}") return False + def append_to_playlist(self, playlist_name: str, tracks) -> bool: + """Append tracks to an existing playlist (creates it if missing). + + Differs from `update_playlist`: never deletes existing tracks, + never recreates the playlist, no backup. Used by sync mode + 'append' so user-added tracks on the server playlist survive + re-syncing the source. Dedupe-by-Id ensures we don't re-add + tracks the playlist already contains.""" + if not self.ensure_connection(): + return False + + try: + existing_playlist = self.get_playlist_by_name(playlist_name) + if not existing_playlist: + logger.info( + f"Jellyfin append: playlist '{playlist_name}' doesn't exist yet — " + f"creating with {len(tracks)} tracks" + ) + return self.create_playlist(playlist_name, tracks) + + playlist_id = existing_playlist.id + existing_tracks = self.get_playlist_tracks(playlist_id) + existing_ids = { + str(t.id) for t in existing_tracks if hasattr(t, 'id') and t.id + } + + new_track_ids = [] + for t in tracks: + tid = None + if hasattr(t, 'id'): + tid = str(t.id) if t.id else None + elif isinstance(t, dict): + tid = str(t.get('Id') or t.get('id') or '') + if tid and tid not in existing_ids and self._is_valid_guid(tid): + new_track_ids.append(tid) + + if not new_track_ids: + logger.info( + f"Jellyfin append: no new tracks to add to '{playlist_name}' " + f"(all matched tracks already present)" + ) + return True + + import requests + batch_size = 100 + total_added = 0 + for i in range(0, len(new_track_ids), batch_size): + batch = new_track_ids[i:i + batch_size] + add_url = f"{self.base_url}/Playlists/{playlist_id}/Items" + add_params = {'Ids': ','.join(batch), 'UserId': self.user_id} + resp = requests.post( + add_url, params=add_params, + headers={'X-Emby-Token': self.api_key}, timeout=30, + ) + if resp.status_code in (200, 204): + total_added += len(batch) + else: + logger.error( + f"Jellyfin append batch failed: HTTP {resp.status_code} - " + f"{resp.text[:200]}" + ) + return False + + logger.info( + f"Jellyfin append: added {total_added} new tracks to '{playlist_name}' " + f"(skipped {len(tracks) - total_added} already present or invalid)" + ) + return True + except Exception as e: + logger.error(f"Error appending to Jellyfin playlist '{playlist_name}': {e}") + return False + def update_playlist(self, playlist_name: str, tracks) -> bool: """Update an existing playlist or create it if it doesn't exist""" if not self.ensure_connection(): diff --git a/core/library/path_resolver.py b/core/library/path_resolver.py index 44a157d8..712ec3c2 100644 --- a/core/library/path_resolver.py +++ b/core/library/path_resolver.py @@ -32,7 +32,8 @@ from any background worker. from __future__ import annotations import os -from typing import Any, Iterable, Optional +from dataclasses import dataclass, field +from typing import Any, Iterable, List, Optional, Tuple from utils.logging_config import get_logger @@ -40,6 +41,33 @@ from utils.logging_config import get_logger logger = get_logger("library.path_resolver") +@dataclass +class ResolveAttempt: + """Diagnostic record for a single `resolve_library_file_path` call. + + Returned by `resolve_library_file_path_with_diagnostic` so callers + that need to surface a useful error message (instead of just a + silent None) can describe what was tried. Pure data — no side + effects, no rendering opinions. + + Fields: + raw_path_existed: True if `os.path.exists(file_path)` returned + True at the start of the resolver. When this is True the + resolver short-circuits and `base_dirs_tried` will be empty. + base_dirs_tried: The ordered list of base directories the + resolver suffix-walked against (already filtered by + `os.path.isdir`). + had_config_manager: Whether a config_manager was supplied. Useful + for distinguishing "no candidates discovered" from "couldn't + even read config to discover". + had_plex_client: Same, for the Plex API probe. + """ + raw_path_existed: bool = False + base_dirs_tried: List[str] = field(default_factory=list) + had_config_manager: bool = False + had_plex_client: bool = False + + def _docker_resolve_path(path_str: Any) -> Optional[str]: """Translate Windows-style paths to the Docker container layout. @@ -149,16 +177,52 @@ def resolve_library_file_path( The first existing path on disk, or None when no match is found. Never raises — failure is the None return. """ + resolved, _ = resolve_library_file_path_with_diagnostic( + file_path, + transfer_folder=transfer_folder, + download_folder=download_folder, + config_manager=config_manager, + plex_client=plex_client, + ) + return resolved + + +def resolve_library_file_path_with_diagnostic( + file_path: Any, + *, + transfer_folder: Optional[str] = None, + download_folder: Optional[str] = None, + config_manager: Any = None, + plex_client: Any = None, +) -> Tuple[Optional[str], ResolveAttempt]: + """Same as ``resolve_library_file_path`` but also returns a + ``ResolveAttempt`` describing what the resolver tried. + + Use this when you need to surface a useful "we tried X, Y, Z" error + to the user instead of a silent None. Issue #558 (gabistek, Navidrome + on Docker): the resolver was returning None because Navidrome doesn't + expose library filesystem paths via API (unlike Plex), and the user + hadn't configured ``library.music_paths``. The Album Completeness + fix endpoint surfaced a generic "Could not determine album folder" + error with no diagnostic — user had no way to know what to configure. + """ + attempt = ResolveAttempt( + had_config_manager=config_manager is not None, + had_plex_client=plex_client is not None, + ) + if not isinstance(file_path, str) or not file_path: - return None + return None, attempt if os.path.exists(file_path): - return file_path + attempt.raw_path_existed = True + return file_path, attempt path_parts = file_path.replace("\\", "/").split("/") base_dirs = _collect_base_dirs(transfer_folder, download_folder, config_manager, plex_client) + attempt.base_dirs_tried = list(base_dirs) if not base_dirs: - return None + return None, attempt # Skip index 0 to avoid drive-letter / leading-slash artifacts # (e.g. "E:" or "" from a leading "/"). @@ -166,8 +230,12 @@ def resolve_library_file_path( for i in range(1, len(path_parts)): candidate = os.path.join(base, *path_parts[i:]) if os.path.exists(candidate): - return candidate - return None + return candidate, attempt + return None, attempt -__all__ = ["resolve_library_file_path"] +__all__ = [ + "ResolveAttempt", + "resolve_library_file_path", + "resolve_library_file_path_with_diagnostic", +] diff --git a/core/media_server/contract.py b/core/media_server/contract.py index d051482a..db50be31 100644 --- a/core/media_server/contract.py +++ b/core/media_server/contract.py @@ -97,6 +97,7 @@ KNOWN_PER_SERVER_METHODS = ( 'get_library_stats', 'create_playlist', 'update_playlist', + 'append_to_playlist', 'copy_playlist', 'get_all_playlists', 'get_playlist_by_name', diff --git a/core/metadata/discography_filters.py b/core/metadata/discography_filters.py new file mode 100644 index 00000000..4e1e0ecd --- /dev/null +++ b/core/metadata/discography_filters.py @@ -0,0 +1,182 @@ +"""Track-level filters for the user-facing Download Discography flow. + +GitHub issue #559 (trackhacs): clicking "Download Discography" on an +artist also pulled in tracks where the artist's name appeared in the +title of someone else's song. Two failure modes underneath: + +1. **Cross-artist tracks.** Spotify's `artist_albums` endpoint returns + compilation / appears_on / various-artists albums where the requested + artist is featured on one or two tracks. The endpoint then added + *every* track from those albums to the wishlist, including tracks by + unrelated artists that just happened to mention the requested artist + in the title. + +2. **Remix / live / acoustic / instrumental versions.** The watchlist + scanner has user-toggleable filters for these (default: exclude), + stored at `watchlist.global_include_*`. The discography backfill + repair job already honors them. The user-facing Download Discography + endpoint did not — those filters never fired for one-off discography + downloads, so users got remix-ladder bloat. + +These helpers live alongside the existing `core.metadata.discography` +because they belong to the same conceptual layer (discography fetch +results, pre-wishlist) and are independently testable. The watchlist +content-type detectors (``is_remix_version`` etc.) are reused from +``core.watchlist_scanner`` rather than re-implemented — same patterns, +single source of truth. +""" + +from __future__ import annotations + +from typing import Any, Dict, List, Optional + +from core.watchlist_scanner import ( + is_acoustic_version, + is_instrumental_version, + is_live_version, + is_remix_version, +) + + +def track_artist_matches(track_artists: Any, requested_artist_name: str) -> bool: + """Return True if the requested artist appears in the track's + artists list (case-insensitive exact-name membership). + + `track_artists` can be the list-of-strings shape produced by + ``core.metadata.album_tracks._normalize_track_artists`` (which is + what the discography fetch returns), or the list-of-dicts shape + that some upstreams pass directly. Both are accepted. + + Returns True for primary-artist tracks AND feature appearances — + the requested artist need only be one of the listed artists. Only + drops tracks where the requested artist isn't named at all (the + cross-artist compilation case from #559). + """ + if not requested_artist_name: + # No artist to compare against — don't filter; let the caller + # decide. Defensive: avoids dropping every track when the + # caller forgot to pass the artist name. + return True + + target = requested_artist_name.strip().lower() + if not target: + return True + + if not track_artists: + return False + + for entry in track_artists: + if isinstance(entry, dict): + name = entry.get('name', '') or '' + else: + name = str(entry or '') + if name.strip().lower() == target: + return True + + return False + + +def content_type_skip_reason( + track_name: str, + album_name: str, + settings: Dict[str, Any], +) -> Optional[str]: + """Return a short skip-reason string if the track is a content type + the user has chosen to exclude, else None. + + `settings` is a dict keyed by the same names as the watchlist + globals (``include_live`` / ``include_remixes`` / ``include_acoustic`` + / ``include_instrumentals``). All default to False — i.e. exclude + by default — matching the watchlist scanner's default contract. + """ + if not settings.get('include_live', False) and is_live_version(track_name, album_name): + return 'live' + if not settings.get('include_remixes', False) and is_remix_version(track_name, album_name): + return 'remix' + if not settings.get('include_acoustic', False) and is_acoustic_version(track_name, album_name): + return 'acoustic' + if not settings.get('include_instrumentals', False) and is_instrumental_version(track_name, album_name): + return 'instrumental' + return None + + +def load_global_content_filter_settings(config_manager: Any) -> Dict[str, Any]: + """Read the four watchlist content-type globals from config. + + Centralises the key names so the endpoint and the helper agree on + where the settings live. All four default to False (exclude) — same + contract as the watchlist scanner. + """ + if config_manager is None: + return { + 'include_live': False, + 'include_remixes': False, + 'include_acoustic': False, + 'include_instrumentals': False, + } + try: + return { + 'include_live': bool(config_manager.get('watchlist.global_include_live', False)), + 'include_remixes': bool(config_manager.get('watchlist.global_include_remixes', False)), + 'include_acoustic': bool(config_manager.get('watchlist.global_include_acoustic', False)), + 'include_instrumentals': bool(config_manager.get('watchlist.global_include_instrumentals', False)), + } + except Exception: + return { + 'include_live': False, + 'include_remixes': False, + 'include_acoustic': False, + 'include_instrumentals': False, + } + + +def track_already_owned( + db: Any, + track_name: str, + requested_artist: str, + album_name: str, + server_source: Optional[str], + confidence_threshold: float = 0.7, +) -> bool: + """Return True if the track is already in the user's library. + + Discord report (Skowl): clicking "Download Discography" twice on + the same artist re-queued every track instead of skipping the + half already on disk. Trace: the endpoint added each track to the + wishlist via ``db.add_to_wishlist``, which only dedups against the + wishlist itself — once a wishlist track downloads it leaves the + wishlist, so the second discography click re-inserted everything. + + The discography backfill repair job already runs the same check + via ``db.check_track_exists`` — this helper centralises the + contract so the user-facing endpoint matches that behavior. + + `check_track_exists` is name+artist+album based, format-agnostic. + Skowl's "Blasphemy mode" library (FLAC converted to MP3 then + original deleted) matches just fine — track_name + artist + album + don't change with format. + + Returns False on any exception so a transient DB hiccup doesn't + silently nuke a discography fetch — a redundant wishlist add is + much cheaper to recover from than a missed track. + """ + if not requested_artist or not track_name: + return False + try: + match, confidence = db.check_track_exists( + track_name, requested_artist, + confidence_threshold=confidence_threshold, + server_source=server_source, + album=album_name or None, + ) + except Exception: + return False + return bool(match) and confidence >= confidence_threshold + + +__all__ = [ + 'track_artist_matches', + 'content_type_skip_reason', + 'load_global_content_filter_settings', + 'track_already_owned', +] diff --git a/core/metadata/enrichment.py b/core/metadata/enrichment.py index 6317cedf..051426f8 100644 --- a/core/metadata/enrichment.py +++ b/core/metadata/enrichment.py @@ -110,9 +110,21 @@ def enhance_file_metadata(file_path: str, context: dict, artist: dict, album_inf if metadata.get("title"): audio_file.tags.add(symbols.TIT2(encoding=3, text=[metadata["title"]])) if metadata.get("artist"): + # TPE1 = display artist string (already joined by + # source.py with the configured separator, or + # primary-only when feat_in_title is on). audio_file.tags.add(symbols.TPE1(encoding=3, text=[metadata["artist"]])) + # When write_multi_artist is on, ALSO write the + # multi-value list to a TXXX:Artists frame (Picard + # convention). Keeps TPE1 as the display string AND + # exposes the per-artist list for media servers + # that read ARTISTS. Pre-fix this path overwrote + # TPE1 with the list, which clobbered the + # configured separator + feat_in_title semantics. if write_multi and len(artists_list) > 1: - audio_file.tags.add(symbols.TPE1(encoding=3, text=artists_list)) + audio_file.tags.add( + symbols.TXXX(encoding=3, desc='Artists', text=list(artists_list)) + ) if metadata.get("album_artist"): audio_file.tags.add(symbols.TPE2(encoding=3, text=[metadata["album_artist"]])) if metadata.get("album"): diff --git a/core/metadata/source.py b/core/metadata/source.py index bbe8dab9..67350205 100644 --- a/core/metadata/source.py +++ b/core/metadata/source.py @@ -917,8 +917,84 @@ def extract_source_metadata(context: dict, artist: dict, album_info: dict) -> di all_artists.append(artist_item) else: all_artists.append(str(artist_item)) - metadata["artist"] = ", ".join(all_artists) - logger.info("Metadata: Using all artists: '%s'", metadata["artist"]) + + # Deezer upgrade path: Deezer's `/search` endpoint only returns + # the primary artist for each track. The full contributors + # array (feat., remix collaborators, producers credited as + # artists) lives on `/track/` and gets parsed by + # `_build_enhanced_track`. Without this upgrade Deezer-sourced + # tracks never get multi-artist tags even with the right + # settings on. One extra API call per Deezer-sourced track, + # only when the search response had a single artist (so it's + # a no-op when search already returned multiple). + if (source == "deezer" and len(all_artists) == 1 + and source_ids.get("track_id")): + try: + from core.metadata import get_deezer_client + deezer = get_deezer_client() + if deezer: + full = deezer.get_track_details(str(source_ids["track_id"])) + if full and isinstance(full.get("artists"), list) and len(full["artists"]) > 1: + upgraded = [a for a in full["artists"] if a] + if upgraded: + logger.info( + "Metadata: Deezer contributors upgrade — search returned " + "%d artist, /track/ returned %d (%s)", + len(all_artists), len(upgraded), upgraded, + ) + all_artists = upgraded + except Exception as e: + logger.debug("Deezer contributors upgrade failed: %s", e) + + # Store the multi-artist list so the enrichment writer can emit + # proper multi-value ARTIST tags (TPE1 multi-value for ID3, + # "artists" key for Vorbis) when `write_multi_artist` is on. + # Without this assignment the field was always empty and the + # multi-artist write path silently no-op'd. + metadata["_artists_list"] = list(all_artists) + + # `feat_in_title` (when true): pull featured artists out of the + # ARTIST tag entirely and append "(feat. X, Y)" to the title. + # Matches Picard / Beets convention and lets media servers + # group by primary artist instead of treating "A, B & C" as a + # distinct artist string. + # `artist_separator`: when feat_in_title is off (or there's + # only one artist) and write_multi_artist is on, this is the + # delimiter used to join all artists into the single ARTIST + # string. Picard defaults to "; " — we default to ", " to + # preserve historical behavior for users who haven't touched + # the setting. + feat_in_title = cfg.get("metadata_enhancement.tags.feat_in_title", False) + artist_separator = cfg.get("metadata_enhancement.tags.artist_separator", ", ") + + if feat_in_title and len(all_artists) > 1: + metadata["artist"] = all_artists[0] + featured = all_artists[1:] + existing_title = metadata.get("title", "") or "" + # Don't double-append if the title already carries the + # featured artists. Source titles vary: "(feat. X)", + # "(featuring X)", "(ft. X)", "ft. X" (no parens), "[feat X]" + # (no period, brackets), etc. Word-boundary regex catches + # `feat`, `feat.`, `featuring`, `ft`, `ft.` regardless of + # surrounding punctuation. Case-insensitive. + import re as _feat_re + already_has_feat = bool(_feat_re.search( + r'\b(?:feat|feat\.|featuring|ft|ft\.)\b', + existing_title, + _feat_re.IGNORECASE, + )) + if existing_title and not already_has_feat: + metadata["title"] = f"{existing_title} (feat. {', '.join(featured)})" + logger.info( + "Metadata: feat_in_title — primary='%s', featured=%s, title='%s'", + metadata["artist"], featured, metadata["title"], + ) + else: + metadata["artist"] = artist_separator.join(all_artists) + logger.info( + "Metadata: Using all artists joined with %r: '%s'", + artist_separator, metadata["artist"], + ) else: metadata["artist"] = artist_dict.get("name", "") or get_import_clean_artist(context) logger.info("Metadata: Using primary artist: '%s'", metadata["artist"]) diff --git a/core/navidrome_client.py b/core/navidrome_client.py index 11995dff..b50582af 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -974,6 +974,73 @@ class NavidromeClient(MediaServerClient): matches.append(playlist) return matches + def append_to_playlist(self, playlist_name: str, tracks) -> bool: + """Append tracks to an existing playlist (creates it if missing). + + Differs from `update_playlist`: never deletes existing tracks, + never recreates the playlist, no backup. Used by sync mode + 'append' so user-added tracks on the server playlist survive + re-syncing the source. Dedupe-by-id ensures we don't re-add + tracks the playlist already contains.""" + if not self.ensure_connection(): + return False + + try: + existing_playlists = self.get_playlists_by_name(playlist_name) + if not existing_playlists: + logger.info( + f"Navidrome append: playlist '{playlist_name}' doesn't exist yet — " + f"creating with {len(tracks)} tracks" + ) + return self.create_playlist(playlist_name, tracks) + + primary = existing_playlists[0] + existing_tracks = self.get_playlist_tracks(primary.id) + existing_ids = { + str(t.id) for t in existing_tracks if hasattr(t, 'id') and t.id + } + + new_track_ids = [] + for t in tracks: + tid = None + if hasattr(t, 'ratingKey'): + tid = str(t.ratingKey) + elif hasattr(t, 'id'): + tid = str(t.id) if t.id else None + elif isinstance(t, dict): + tid = str(t.get('id') or '') + if tid and tid not in existing_ids: + new_track_ids.append(tid) + + if not new_track_ids: + logger.info( + f"Navidrome append: no new tracks to add to '{playlist_name}' " + f"(all matched tracks already present)" + ) + return True + + # Subsonic updatePlaylist: `songIdToAdd` accepts repeated values + # (requests serializes list values as repeated query/form params). + params = { + 'playlistId': primary.id, + 'songIdToAdd': new_track_ids, + } + response = self._make_request('updatePlaylist', params) + if response and response.get('status') == 'ok': + logger.info( + f"Navidrome append: added {len(new_track_ids)} new tracks to " + f"'{playlist_name}' (skipped {len(tracks) - len(new_track_ids)} " + f"already present)" + ) + return True + logger.error( + f"Failed to append to Navidrome playlist '{playlist_name}'" + ) + return False + except Exception as e: + logger.error(f"Error appending to Navidrome playlist '{playlist_name}': {e}") + return False + def update_playlist(self, playlist_name: str, tracks) -> bool: """Update an existing playlist or create it if it doesn't exist. Handles duplicates.""" if not self.ensure_connection(): diff --git a/core/plex_client.py b/core/plex_client.py index 50c971fd..9dcc0dda 100644 --- a/core/plex_client.py +++ b/core/plex_client.py @@ -620,6 +620,53 @@ class PlexClient(MediaServerClient): logger.error(f"Error copying playlist '{source_name}' to '{target_name}': {e}") return False + def append_to_playlist(self, playlist_name: str, tracks: List[TrackInfo]) -> bool: + """Append tracks to an existing playlist (creates it if missing). + + Differs from `update_playlist`: never deletes existing tracks, + never recreates the playlist, no backup. Used by sync mode + 'append' so user-added tracks on the server playlist survive + re-syncing the source. Dedupe-by-ratingKey ensures we don't + re-add tracks the playlist already contains.""" + if not self.ensure_connection(): + return False + + try: + try: + existing_playlist = self.server.playlist(playlist_name) + except NotFound: + logger.info( + f"Plex append: playlist '{playlist_name}' doesn't exist yet — " + f"creating with {len(tracks)} tracks" + ) + return self.create_playlist(playlist_name, tracks) + + existing_keys = { + str(t.ratingKey) for t in existing_playlist.items() + if hasattr(t, 'ratingKey') + } + new_tracks = [ + t for t in tracks + if hasattr(t, 'ratingKey') and str(t.ratingKey) not in existing_keys + ] + + if not new_tracks: + logger.info( + f"Plex append: no new tracks to add to '{playlist_name}' " + f"(all {len(tracks)} matched-tracks already present)" + ) + return True + + existing_playlist.addItems(new_tracks) + logger.info( + f"Plex append: added {len(new_tracks)} new tracks to '{playlist_name}' " + f"(skipped {len(tracks) - len(new_tracks)} already present)" + ) + return True + except Exception as e: + logger.error(f"Error appending to Plex playlist '{playlist_name}': {e}") + return False + def update_playlist(self, playlist_name: str, tracks: List[TrackInfo]) -> bool: if not self.ensure_connection(): return False diff --git a/core/repair_jobs/acoustid_scanner.py b/core/repair_jobs/acoustid_scanner.py index afe83d4a..660c216f 100644 --- a/core/repair_jobs/acoustid_scanner.py +++ b/core/repair_jobs/acoustid_scanner.py @@ -192,10 +192,38 @@ class AcoustIDScannerJob(RepairJob): if not aid_title: return + # Resolve which artist value to compare against, in priority order: + # 1. DB `track_artist` (per-track, manually curated or scanner- + # populated) — trust it when populated. Respects user edits + # from the enhanced library view. + # 2. File's ARTIST tag — ground truth for what's on disk. + # Catches legacy compilation tracks where `track_artist` + # column is NULL because they were downloaded before that + # column existed; the file itself has the correct per- + # track artist (Tidal/Spotify/Deezer all write it). + # 3. Album artist — final fallback for files without proper + # ARTIST tags AND no DB track_artist. + track_artist = (expected.get('track_artist') or '').strip() + if track_artist: + expected_artist = track_artist + else: + file_artist = None + try: + from core.tag_writer import read_file_tags + file_tags = read_file_tags(fpath) + file_artist = (file_tags.get('artist') or '').strip() or None + except Exception as e: + logger.debug("file-tag artist read failed for %s: %s", fname, e) + expected_artist = ( + file_artist + or (expected.get('album_artist') or '').strip() + or expected['artist'] + ) + # Normalize and compare norm_expected_title = _normalize(expected['title']) norm_aid_title = _normalize(aid_title) - norm_expected_artist = _normalize(expected['artist']) + norm_expected_artist = _normalize(expected_artist) norm_aid_artist = _normalize(aid_artist) title_sim = SequenceMatcher(None, norm_expected_title, norm_aid_title).ratio() @@ -216,7 +244,7 @@ class AcoustIDScannerJob(RepairJob): from core.matching.artist_aliases import artist_names_match _, artist_sim = artist_names_match( - expected['artist'], + expected_artist, aid_artist, threshold=artist_threshold, ) @@ -243,14 +271,14 @@ class AcoustIDScannerJob(RepairJob): file_path=fpath, title=f'Wrong download: "{expected["title"]}" is actually "{aid_title}"', description=( - f'Expected "{expected["title"]}" by {expected["artist"]}, ' + f'Expected "{expected["title"]}" by {expected_artist}, ' f'but audio fingerprint matches "{aid_title}" by {aid_artist} ' f'(fingerprint: {best_score:.0%}, title match: {title_sim:.0%}, ' f'artist match: {artist_sim:.0%})' ), details={ 'expected_title': expected['title'], - 'expected_artist': expected['artist'], + 'expected_artist': expected_artist, 'acoustid_title': aid_title, 'acoustid_artist': aid_artist, 'fingerprint_score': round(best_score, 3), @@ -284,11 +312,21 @@ class AcoustIDScannerJob(RepairJob): # import path when different from album artist) and fall # back to the album artist only when the per-track column # is NULL or empty (legacy rows / single-artist albums). + # Load `track_artist` (raw, may be empty) AND `album_artist` + # separately so `_scan_file` can tell the difference between + # 'DB has a curated per-track value' and 'DB fell back to + # album artist'. The COALESCE'd `artist` field is kept as a + # convenience for the existing `expected['artist']` consumers + # that want a single resolved value, but the resolution + # priority that actually drives the comparison is reproduced + # in `_scan_file`: track_artist → file tag → album_artist. cursor.execute(""" SELECT t.id, t.title, COALESCE(NULLIF(t.track_artist, ''), ar.name) AS artist, t.file_path, t.track_number, - al.title AS album_title, al.thumb_url, ar.thumb_url + al.title AS album_title, al.thumb_url, ar.thumb_url, + NULLIF(t.track_artist, '') AS track_artist, + ar.name AS album_artist FROM tracks t LEFT JOIN artists ar ON ar.id = t.artist_id LEFT JOIN albums al ON al.id = t.album_id @@ -312,6 +350,8 @@ class AcoustIDScannerJob(RepairJob): 'album_title': row[5] or '', 'album_thumb_url': row[6] or None, 'artist_thumb_url': row[7] or None, + 'track_artist': row[8] or '', # raw (may be empty) + 'album_artist': row[9] or '', } except Exception as e: logger.error("Error loading tracks from DB: %s", e) diff --git a/core/repair_worker.py b/core/repair_worker.py index 50d90b27..2c0b07f8 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -12,6 +12,7 @@ import os import re import shutil import sys +import sqlite3 import threading import time import uuid @@ -1921,6 +1922,54 @@ class RepairWorker: # Default return '{num:02d} - {title}' + def _build_unresolvable_album_folder_error(self, attempt, sample_db_path): + """Render a diagnostic error string for the Album Completeness + "couldn't find existing track on disk" failure mode. + + Pre-fix this returned a flat + "Could not determine album folder from existing tracks" + which left users (especially Navidrome / Jellyfin Docker setups + where the resolver can't auto-discover library mounts) with no + way to know what to fix. The new message names the active media + server, shows one sample DB-recorded path, and lists the base + directories the resolver actually probed. + + Args: + attempt: ``ResolveAttempt`` from the last resolver call. + May be ``None`` if no attempt was recorded (defensive). + sample_db_path: One example ``tracks.file_path`` value from + the album. Helps the user see what their media server is + reporting so they know what to mount / configure. + """ + active_server = 'unknown' + if self._config_manager is not None: + try: + getter = getattr(self._config_manager, 'get_active_media_server', None) + if callable(getter): + active_server = getter() or 'unknown' + else: + active_server = self._config_manager.get('active_media_server', 'unknown') or 'unknown' + except Exception as e: + logger.debug("active media server lookup failed: %s", e) + + lines = [ + "Could not find any existing track from this album on disk.", + f"Active media server: {active_server}.", + ] + if sample_db_path: + lines.append(f"Example DB-recorded path: {sample_db_path}") + if attempt is not None: + if attempt.base_dirs_tried: + joined = ', '.join(attempt.base_dirs_tried) + lines.append(f"Probed base directories: {joined}") + else: + lines.append("No base directories were available to probe.") + lines.append( + "Fix: Settings → Library → Music Paths → add the path where " + "this container can read your library files." + ) + return ' '.join(lines) + def _fix_incomplete_album(self, entity_type, entity_id, file_path, details): """Auto-fill an incomplete album by finding missing tracks in the library. @@ -1963,14 +2012,24 @@ class RepairWorker: download_folder = self._config_manager.get('soulseek.download_path', '') album_folder = None + last_attempt = None + sample_db_path = None for t in existing_tracks: - resolved = _resolve_file_path(t.file_path, self.transfer_folder, download_folder, config_manager=self._config_manager) + from core.library.path_resolver import resolve_library_file_path_with_diagnostic + resolved, attempt = resolve_library_file_path_with_diagnostic( + t.file_path, transfer_folder=self.transfer_folder, + download_folder=download_folder, config_manager=self._config_manager, + ) + last_attempt = attempt + if sample_db_path is None and isinstance(t.file_path, str) and t.file_path: + sample_db_path = t.file_path if resolved and os.path.exists(resolved): album_folder = os.path.dirname(resolved) break if not album_folder: - return {'success': False, 'error': 'Could not determine album folder from existing tracks'} + return {'success': False, + 'error': self._build_unresolvable_album_folder_error(last_attempt, sample_db_path)} # Detect filename pattern resolved_paths = [] @@ -2231,6 +2290,46 @@ class RepairWorker: album_folder, filename_pattern, download_folder): """Move or copy a candidate track into the album folder and update DB.""" try: + def _fallback_server_source(): + if getattr(candidate, 'server_source', None): + return candidate.server_source + if self._config_manager: + getter = getattr(self._config_manager, 'get_active_media_server', None) + if callable(getter): + return getter() or 'plex' + return self._config_manager.get('active_media_server', 'plex') + return 'plex' + + def _resolve_target_context(cursor): + cursor.execute( + """ + SELECT artist_id, server_source + FROM tracks + WHERE album_id = ? + ORDER BY track_number, title + LIMIT 1 + """, + (album_id,), + ) + row = cursor.fetchone() + if row: + return row[0] or candidate.artist_id, row[1] or _fallback_server_source() + + try: + cursor.execute( + "SELECT artist_id, server_source FROM albums WHERE id = ? LIMIT 1", + (album_id,), + ) + except sqlite3.OperationalError: + row = None + else: + row = cursor.fetchone() + + if row: + return row[0] or candidate.artist_id, row[1] or _fallback_server_source() + + return candidate.artist_id, _fallback_server_source() + # Resolve source file src_path = _resolve_file_path(candidate.file_path, self.transfer_folder, download_folder, config_manager=self._config_manager) if not src_path or not os.path.exists(src_path): @@ -2264,18 +2363,15 @@ class RepairWorker: # Update existing DB record to point to new album and path conn = self.db._get_connection() cursor = conn.cursor() - # Get the target album's artist_id for consistency - cursor.execute("SELECT artist_id FROM tracks WHERE album_id = ? LIMIT 1", (album_id,)) - artist_row = cursor.fetchone() - target_artist_id = artist_row[0] if artist_row else candidate.artist_id + target_artist_id, target_server_source = _resolve_target_context(cursor) cursor.execute(""" UPDATE tracks SET album_id = ?, artist_id = ?, title = ?, - file_path = ?, track_number = ?, + file_path = ?, track_number = ?, server_source = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ? """, (album_id, target_artist_id, track_name, - target_path, track_number, candidate.id)) + target_path, track_number, target_server_source, candidate.id)) # Clean up the source single's album if it's now empty cursor.execute("SELECT COUNT(*) FROM tracks WHERE album_id = ?", (candidate.album_id,)) @@ -2300,17 +2396,14 @@ class RepairWorker: conn = self.db._get_connection() cursor = conn.cursor() - # Get artist_id from existing album tracks - cursor.execute("SELECT artist_id FROM tracks WHERE album_id = ? LIMIT 1", (album_id,)) - artist_row = cursor.fetchone() - target_artist_id = artist_row[0] if artist_row else candidate.artist_id + target_artist_id, target_server_source = _resolve_target_context(cursor) cursor.execute(""" INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration, - file_path, bitrate, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) + file_path, bitrate, server_source, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) """, (new_track_id, album_id, target_artist_id, track_name, track_number, - candidate.duration, target_path, candidate.bitrate)) + candidate.duration, target_path, candidate.bitrate, target_server_source)) conn.commit() finally: diff --git a/core/soulseek_client.py b/core/soulseek_client.py index 1f7cb9c7..6b928688 100644 --- a/core/soulseek_client.py +++ b/core/soulseek_client.py @@ -48,18 +48,87 @@ _SLSKD_DEFAULT_TIMEOUT = aiohttp.ClientTimeout( ) +# Search-rate-limit defaults. Pre-fix these were hardcoded magic numbers +# inside `SoulseekClient.__init__`. Lifted to module level so they're +# greppable + bumpable in one place, and so the reddit-reported case +# (Bell Canada anti-abuse trips on slskd peer-connection bursts) can +# tune them via `soulseek.search_*` config without touching code. +_DEFAULT_MAX_PER_WINDOW = 35 +_DEFAULT_WINDOW_SECONDS = 220 +_DEFAULT_MIN_DELAY_SECONDS = 0 # 0 = disabled (preserves prior behavior) + + +def compute_search_wait_seconds( + timestamps: List[float], + last_search_at: float, + now: float, + *, + max_per_window: int, + window_seconds: float, + min_delay_seconds: float, +) -> float: + """Pure scheduler for the slskd search throttle. + + Returns how many seconds the caller should sleep before issuing + the next search. ``timestamps`` is the list of recent search + timestamps already pruned to the current window (caller's job). + ``last_search_at`` is the timestamp of the most recent search + (0.0 if there hasn't been one). ``now`` is the current monotonic / + wall-clock time (caller chooses — pure function only does math). + + Two independent gates, return the larger: + + 1. **Sliding-window cap** — when ``len(timestamps) >= max_per_window``, + sleep until the oldest timestamp ages out of the window. Same + semantics as the pre-fix hardcoded behavior. + + 2. **Min-delay between searches** — when ``min_delay_seconds > 0``, + sleep until at least that many seconds have passed since + ``last_search_at``. Smooths bursts even when the window isn't + full — this is the actual fix for the Reddit-reported case where + Bell Canada's anti-abuse trips on the rapid peer-connection + bursts that 35 back-to-back searches generate. + + Returns 0.0 (no wait) when ``min_delay_seconds`` is 0 / negative + AND the window isn't full. Pure: no I/O, no side effects, no + mutation of the inputs. + """ + window_wait = 0.0 + if max_per_window > 0 and len(timestamps) >= max_per_window: + oldest = timestamps[0] + window_wait = max(0.0, oldest + window_seconds - now) + + delay_wait = 0.0 + if min_delay_seconds > 0 and last_search_at > 0: + elapsed = now - last_search_at + delay_wait = max(0.0, min_delay_seconds - elapsed) + + return max(window_wait, delay_wait) + + class SoulseekClient(DownloadSourcePlugin): def __init__(self): self.base_url: Optional[str] = None self.api_key: Optional[str] = None self.download_path: Path = Path("./downloads") self.active_searches: Dict[str, bool] = {} # search_id -> still_active - - # Rate limiting for searches - self.search_timestamps: List[float] = [] # Track search timestamps - self.max_searches_per_window = 35 # Conservative limit to prevent Soulseek bans - self.rate_limit_window = 220 # seconds (3 minutes 40 seconds) - + + # Rate limiting for searches. Cap + window stay hardcoded — + # nobody has reported issues with the 35/220 defaults. The + # min-delay knob is the actual fix for the Reddit-reported + # case (Bell Canada anti-abuse cuts the WAN after rapid + # peer-connection bursts) — smooths bursts even when the + # sliding-window cap isn't hit. 0 = disabled (preserves prior + # behavior). + self.search_timestamps: List[float] = [] + self._last_search_at: float = 0.0 + self.max_searches_per_window = _DEFAULT_MAX_PER_WINDOW + self.rate_limit_window = _DEFAULT_WINDOW_SECONDS + self.search_min_delay_seconds = float( + config_manager.get('soulseek.search_min_delay_seconds', _DEFAULT_MIN_DELAY_SECONDS) + or _DEFAULT_MIN_DELAY_SECONDS + ) + self._setup_client() def _setup_client(self): @@ -103,22 +172,36 @@ class SoulseekClient(DownloadSourcePlugin): self.search_timestamps = [ts for ts in self.search_timestamps if ts > cutoff_time] async def _wait_for_rate_limit(self): - """Wait if necessary to respect rate limiting""" + """Wait if necessary to respect search rate limits. + + Delegates the wait math to ``compute_search_wait_seconds`` so + the throttle logic is testable independently of asyncio.sleep + and the singleton client. Two gates apply (max wins): sliding- + window cap on searches per N seconds, plus optional min-delay + between consecutive searches (the burst-smoother). + """ self._clean_old_timestamps() - - if len(self.search_timestamps) >= self.max_searches_per_window: - # Calculate how long to wait - oldest_timestamp = self.search_timestamps[0] - wait_time = oldest_timestamp + self.rate_limit_window - time.time() - - if wait_time > 0: - logger.info(f"Rate limit reached ({len(self.search_timestamps)}/{self.max_searches_per_window} searches). Waiting {wait_time:.1f} seconds...") - await asyncio.sleep(wait_time) - # Clean up again after waiting - self._clean_old_timestamps() - + wait_time = compute_search_wait_seconds( + self.search_timestamps, + self._last_search_at, + time.time(), + max_per_window=self.max_searches_per_window, + window_seconds=self.rate_limit_window, + min_delay_seconds=self.search_min_delay_seconds, + ) + if wait_time > 0: + logger.info( + f"Search rate limit: waiting {wait_time:.1f}s " + f"({len(self.search_timestamps)}/{self.max_searches_per_window} in window, " + f"min_delay={self.search_min_delay_seconds:.1f}s)" + ) + await asyncio.sleep(wait_time) + self._clean_old_timestamps() + # Record this search attempt - self.search_timestamps.append(time.time()) + now = time.time() + self.search_timestamps.append(now) + self._last_search_at = now def get_rate_limit_status(self) -> Dict[str, Any]: """Get current rate limiting status""" diff --git a/core/tidal_client.py b/core/tidal_client.py index d0d22099..a983944c 100644 --- a/core/tidal_client.py +++ b/core/tidal_client.py @@ -1075,6 +1075,121 @@ class TidalClient: logger.error(f"Error getting Tidal album {album_id}: {e}") return None + @rate_limited + def get_album_tracks(self, album_id: str, limit: Optional[int] = None) -> List[Track]: + """Fetch every track on an album with full artist + name + duration + metadata hydrated. + + Two-phase: walk `/v2/albums/{id}/relationships/items?include=items` + cursor chain to enumerate track IDs (with their position metadata — + `meta.trackNumber` + `meta.volumeNumber` for multi-disc), then + feed the IDs through the existing `_get_tracks_batch` helper for + artist + album-name resolution. + + Returns a list of `Track` dataclasses with `track_number` and + `disc_number` attached as ad-hoc attributes so callers that need + per-position info (download modal, virtual playlist build) can + read them. Backend `/api/discover/album//` + serializes these to the same shape Spotify/Deezer return.""" + if not self._ensure_valid_token(): + return [] + + # Phase 1: enumerate track IDs + position metadata via cursor pagination. + # The relationship endpoint pages at 20 items by default. The `meta` + # dict on each ref carries `trackNumber` + `volumeNumber` (multi-disc). + track_meta_by_id: Dict[str, Dict[str, int]] = {} + track_ids: List[str] = [] + next_path: Optional[str] = None + + while True: + if next_path: + url = (next_path if next_path.startswith('http') + else f"https://openapi.tidal.com/v2{next_path}") + params = None + else: + url = f"{self.base_url}/albums/{album_id}/relationships/items" + params = {'countryCode': 'US', 'include': 'items'} + + try: + resp = self.session.get( + url, params=params, + headers={'accept': 'application/vnd.api+json'}, + timeout=15, + ) + except Exception as e: + logger.debug(f"Tidal album-tracks page request failed: {e}") + break + + if resp.status_code != 200: + if resp.status_code == 429: + raise Exception("Rate limited (429) on get_album_tracks") + logger.debug( + f"Tidal album-tracks page returned {resp.status_code}: {resp.text[:200]}" + ) + break + + try: + data = resp.json() + except ValueError: + break + + for item in data.get('data', []): + if item.get('type') != 'tracks': + continue + tid = item.get('id') + if not tid: + continue + tid = str(tid) + meta = item.get('meta', {}) or {} + track_meta_by_id[tid] = { + 'track_number': int(meta.get('trackNumber') or 0), + 'disc_number': int(meta.get('volumeNumber') or 1), + } + track_ids.append(tid) + if limit is not None and len(track_ids) >= limit: + break + + if limit is not None and len(track_ids) >= limit: + break + + next_path = data.get('links', {}).get('next') + if not next_path: + break + + time.sleep(0.3) + + if not track_ids: + return [] + + # Phase 2: batch hydrate via existing helper (artists + album names). + # Annotate each Track with position metadata so callers can build the + # per-track-number payload the download pipeline expects. + hydrated: List[Track] = [] + for i in range(0, len(track_ids), self._COLLECTION_BATCH_SIZE): + batch_ids = track_ids[i:i + self._COLLECTION_BATCH_SIZE] + try: + batch_tracks = self._get_tracks_batch(batch_ids) + except Exception as e: + logger.debug(f"Tidal album-tracks batch hydration failed: {e}") + continue + for t in batch_tracks: + meta = track_meta_by_id.get(str(t.id), {}) + t.track_number = meta.get('track_number', 0) + t.disc_number = meta.get('disc_number', 1) + hydrated.append(t) + + # Tidal's relationship walk returns tracks in album order; the + # batch endpoint may not preserve order. Sort by (disc, track) + # so the modal renders the album top-down. + hydrated.sort(key=lambda t: ( + getattr(t, 'disc_number', 1), + getattr(t, 'track_number', 0), + )) + logger.info( + f"Retrieved {len(hydrated)}/{len(track_ids)} tracks for Tidal album {album_id}" + ) + return hydrated + @rate_limited def get_track(self, track_id: str) -> Optional[Dict]: """Get full track details by Tidal ID.""" @@ -1455,244 +1570,14 @@ class TidalClient: logger.error(f"Error getting Tidal user info: {e}") return None - def get_favorite_artists(self, limit: int = 200) -> list: - """Fetch user's favorite artists from Tidal. - Returns list of dicts with tidal_id, name, image_url.""" - try: - if not self._ensure_valid_token(): - logger.debug("Tidal not authenticated — cannot fetch favorites") - return [] - - user_id, api_version = self._get_user_id() - if not user_id: - logger.warning("Could not get Tidal user ID for favorites") - return [] - - artists = [] - - if api_version == 'v2': - # V2 API: /v2/favorites with filter - offset = 0 - while len(artists) < limit: - try: - headers = self.session.headers.copy() - headers['accept'] = 'application/vnd.api+json' - resp = requests.get( - f"{self.base_url}/favorites", - params={ - 'countryCode': 'US', - 'filter[user.id]': user_id, - 'filter[type]': 'ARTISTS', - 'include': 'artists', - 'page[limit]': min(50, limit - len(artists)), - 'page[offset]': offset - }, - headers=headers, timeout=15 - ) - if resp.status_code != 200: - logger.debug(f"Tidal V2 favorites returned {resp.status_code}, trying V1") - break - data = resp.json() - # Parse included artists - included = data.get('included', []) - if not included: - items = data.get('data', []) - if not items: - break - # Try to extract from data items directly - for item in items: - attrs = item.get('attributes', {}) - name = attrs.get('name', '') - if name: - img = None - img_data = item.get('relationships', {}).get('image', {}).get('data', {}) - if isinstance(img_data, dict) and img_data.get('id'): - img = f"https://resources.tidal.com/images/{img_data['id'].replace('-', '/')}/750x750.jpg" - artists.append({'tidal_id': item.get('id', ''), 'name': name, 'image_url': img}) - else: - for inc in included: - if inc.get('type') == 'artists': - attrs = inc.get('attributes', {}) - img = None - img_rel = inc.get('relationships', {}).get('image', {}).get('data', {}) - if isinstance(img_rel, dict) and img_rel.get('id'): - img = f"https://resources.tidal.com/images/{img_rel['id'].replace('-', '/')}/750x750.jpg" - artists.append({ - 'tidal_id': str(inc.get('id', '')), - 'name': attrs.get('name', ''), - 'image_url': img, - }) - if not data.get('links', {}).get('next'): - break - offset += 50 - import time - time.sleep(0.5) - except Exception as e: - logger.debug(f"Tidal V2 favorites error: {e}") - break - - # Fallback to V1 API if V2 returned nothing - if not artists: - try: - offset = 0 - while len(artists) < limit: - resp = self.session.get( - f"{self.alt_base_url}/users/{user_id}/favorites/artists", - params={'countryCode': 'US', 'limit': min(50, limit - len(artists)), 'offset': offset}, - timeout=15 - ) - if resp.status_code != 200: - logger.debug(f"Tidal V1 favorites returned {resp.status_code}") - break - data = resp.json() - items = data.get('items', []) - if not items: - break - for item in items: - a = item.get('item', item) - img_id = (a.get('picture') or '').replace('-', '/') - img = f"https://resources.tidal.com/images/{img_id}/750x750.jpg" if img_id else None - artists.append({ - 'tidal_id': str(a.get('id', '')), - 'name': a.get('name', ''), - 'image_url': img, - }) - total = data.get('totalNumberOfItems', 0) - offset += len(items) - if offset >= total: - break - import time - time.sleep(0.5) - except Exception as e: - logger.debug(f"Tidal V1 favorites error: {e}") - - logger.info(f"Retrieved {len(artists)} favorite artists from Tidal") - return artists - except Exception as e: - logger.error(f"Error fetching Tidal favorite artists: {e}") - return [] - - def get_favorite_albums(self, limit: int = 200) -> list: - """Fetch user's favorite albums from Tidal. - Returns list of dicts with tidal_id, album_name, artist_name, image_url, release_date, total_tracks.""" - try: - if not self._ensure_valid_token(): - logger.debug("Tidal not authenticated — cannot fetch favorite albums") - return [] - - user_id, api_version = self._get_user_id() - if not user_id: - logger.warning("Could not get Tidal user ID for favorite albums") - return [] - - albums = [] - - if api_version == 'v2': - offset = 0 - while len(albums) < limit: - try: - headers = self.session.headers.copy() - headers['accept'] = 'application/vnd.api+json' - resp = requests.get( - f"{self.base_url}/favorites", - params={ - 'countryCode': 'US', - 'filter[user.id]': user_id, - 'filter[type]': 'ALBUMS', - 'include': 'albums', - 'page[limit]': min(50, limit - len(albums)), - 'page[offset]': offset - }, - headers=headers, timeout=15 - ) - if resp.status_code != 200: - logger.debug(f"Tidal V2 favorite albums returned {resp.status_code}, trying V1") - break - data = resp.json() - included = data.get('included', []) - items = included if included else data.get('data', []) - if not items: - break - for item in items: - if included and item.get('type') not in ('albums', 'album'): - continue - attrs = item.get('attributes', {}) - title = attrs.get('title', '') - if not title: - continue - img = None - img_rel = item.get('relationships', {}).get('image', {}).get('data', {}) - if isinstance(img_rel, dict) and img_rel.get('id'): - img = f"https://resources.tidal.com/images/{img_rel['id'].replace('-', '/')}/750x750.jpg" - artist_name = '' - artist_rel = attrs.get('artists', [{}]) - if artist_rel and isinstance(artist_rel, list): - artist_name = artist_rel[0].get('name', '') if isinstance(artist_rel[0], dict) else '' - albums.append({ - 'tidal_id': str(item.get('id', '')), - 'album_name': title, - 'artist_name': artist_name, - 'image_url': img, - 'release_date': attrs.get('releaseDate', ''), - 'total_tracks': attrs.get('numberOfTracks', 0), - }) - if not data.get('links', {}).get('next'): - break - offset += 50 - import time - time.sleep(0.5) - except Exception as e: - logger.debug(f"Tidal V2 favorite albums error: {e}") - break - - # Fallback to V1 API - if not albums: - try: - offset = 0 - while len(albums) < limit: - resp = self.session.get( - f"{self.alt_base_url}/users/{user_id}/favorites/albums", - params={'countryCode': 'US', 'limit': min(50, limit - len(albums)), 'offset': offset}, - timeout=15 - ) - if resp.status_code != 200: - logger.debug(f"Tidal V1 favorite albums returned {resp.status_code}") - break - data = resp.json() - items = data.get('items', []) - if not items: - break - for item in items: - a = item.get('item', item) - img_id = (a.get('cover') or '').replace('-', '/') - img = f"https://resources.tidal.com/images/{img_id}/750x750.jpg" if img_id else None - artist_name = '' - if isinstance(a.get('artist'), dict): - artist_name = a['artist'].get('name', '') - elif isinstance(a.get('artists'), list) and a['artists']: - artist_name = a['artists'][0].get('name', '') - albums.append({ - 'tidal_id': str(a.get('id', '')), - 'album_name': a.get('title', ''), - 'artist_name': artist_name, - 'image_url': img, - 'release_date': a.get('releaseDate', ''), - 'total_tracks': a.get('numberOfTracks', 0), - }) - total = data.get('totalNumberOfItems', 0) - offset += len(items) - if offset >= total: - break - import time - time.sleep(0.5) - except Exception as e: - logger.debug(f"Tidal V1 favorite albums error: {e}") - - logger.info(f"Retrieved {len(albums)} favorite albums from Tidal") - return albums - except Exception as e: - logger.error(f"Error fetching Tidal favorite albums: {e}") - return [] + # `get_favorite_artists` and `get_favorite_albums` were defined here + # against the legacy `/v2/favorites?filter[type]=...` endpoint with a + # V1 fallback. Both paths are dead in 2026: V2 returns 404 for + # personal favorites (it's scoped to third-party-app-created + # collections only), and V1 returns 403 because modern OAuth tokens + # carry `collection.read` instead of the legacy `r_usr` scope V1 + # demands. Replaced by the V2 user-collection endpoints below — see + # the "Favorited albums + artists" section near the end of this class. # ------------------------------------------------------------------ # User Collection ("Favorite Tracks" — Tidal calls this "My Collection") @@ -1732,15 +1617,23 @@ class TidalClient: # a user-actionable hint instead of silently hiding the row. _COLLECTION_TRACKS_PATH = "userCollectionTracks/me/relationships/items" + _COLLECTION_ALBUMS_PATH = "userCollectionAlbums/me/relationships/items" + _COLLECTION_ARTISTS_PATH = "userCollectionArtists/me/relationships/items" _COLLECTION_BATCH_SIZE = 20 # Tidal `filter[id]` page cap @rate_limited - def _iter_collection_track_ids(self, max_ids: Optional[int] = None) -> List[str]: - """Walk the cursor-paginated collection endpoint and return the - list of track IDs in the user's Favorite Tracks. + def _iter_collection_resource_ids(self, path: str, expected_type: str, + max_ids: Optional[int] = None) -> List[str]: + """Walk a cursor-paginated collection endpoint and return the + list of resource IDs (tracks / albums / artists). - ``max_ids`` caps the walk early — used by callers that only - need a count or a partial list. Returns ``[]`` when not + Generic across all three favorited-resource endpoints — the + only differences between them are the path segment and the + ``type`` field on each ``data[]`` entry. Pagination, auth, + scope-failure detection, and the diagnostic logging are + identical. + + ``max_ids`` caps the walk early. Returns ``[]`` when not authenticated or when the endpoint refuses (e.g. token without ``collection.read`` scope). On 401/403 also flips ``self._collection_needs_reconnect = True`` so the caller can @@ -1750,10 +1643,10 @@ class TidalClient: self._collection_needs_reconnect = False if not self._ensure_valid_token(): - logger.debug("Tidal not authenticated — cannot fetch collection tracks") + logger.debug("Tidal not authenticated — cannot fetch collection %s", expected_type) return [] - track_ids: List[str] = [] + ids: List[str] = [] next_path: Optional[str] = None while True: @@ -1763,7 +1656,7 @@ class TidalClient: url = next_path if next_path.startswith('http') else f"https://openapi.tidal.com/v2{next_path}" params = None else: - url = f"{self.base_url}/{self._COLLECTION_TRACKS_PATH}" + url = f"{self.base_url}/{path}" params = { 'countryCode': 'US', 'locale': 'en-US', @@ -1811,13 +1704,13 @@ class TidalClient: break for item in data.get('data', []): - if item.get('type') != 'tracks': + if item.get('type') != expected_type: continue - tid = item.get('id') - if tid: - track_ids.append(str(tid)) - if max_ids is not None and len(track_ids) >= max_ids: - return track_ids + rid = item.get('id') + if rid: + ids.append(str(rid)) + if max_ids is not None and len(ids) >= max_ids: + return ids next_path = data.get('links', {}).get('next') if not next_path: @@ -1825,7 +1718,25 @@ class TidalClient: time.sleep(0.3) # Cursor pagination courtesy delay - return track_ids + return ids + + def _iter_collection_track_ids(self, max_ids: Optional[int] = None) -> List[str]: + """Favorited tracks — thin wrapper over the generic walker.""" + return self._iter_collection_resource_ids( + self._COLLECTION_TRACKS_PATH, 'tracks', max_ids, + ) + + def _iter_collection_album_ids(self, max_ids: Optional[int] = None) -> List[str]: + """Favorited albums — thin wrapper over the generic walker.""" + return self._iter_collection_resource_ids( + self._COLLECTION_ALBUMS_PATH, 'albums', max_ids, + ) + + def _iter_collection_artist_ids(self, max_ids: Optional[int] = None) -> List[str]: + """Favorited artists — thin wrapper over the generic walker.""" + return self._iter_collection_resource_ids( + self._COLLECTION_ARTISTS_PATH, 'artists', max_ids, + ) def collection_needs_reconnect(self) -> bool: """True when the most recent collection fetch hit a 401/403 — @@ -1879,6 +1790,224 @@ class TidalClient: logger.error(f"Error fetching Tidal collection tracks: {e}") return [] + # ------------------------------------------------------------------ + # Favorited albums + artists — V2 collection endpoints + # ------------------------------------------------------------------ + # + # Same problem the tracks side hit on issue #502: the prior + # `/v2/favorites?filter[type]=ALBUMS|ARTISTS` endpoints are + # deprecated (404) and the V1 fallback (`/v1/users//favorites/ + # albums|artists`) returns 403 because modern OAuth tokens with + # `collection.read` scope don't have the legacy `r_usr` scope V1 + # requires. Discord-reported symptom: Discover → Your Albums (and + # Your Artists) section shows nothing for Tidal users regardless + # of how many albums/artists they've favorited. + # + # Fix mirrors the tracks path: + # 1) Cursor-walk `/v2/userCollection{Albums|Artists}/me/relationships/items` + # via `_iter_collection_album_ids` / `_iter_collection_artist_ids` + # (lifted into the generic `_iter_collection_resource_ids` helper). + # 2) Batch-hydrate via `/v2/{albums|artists}?filter[id]=...&include=...` + # with single-request fan-out (artists+coverArt for albums, + # profileArt for artists). Parses JSON:API `included[]` for + # artist names + image URLs. + # + # Public surface preserves the existing return shape — list of + # dicts matching what `database.upsert_liked_album` / + # `upsert_liked_artist` consume — so web_server.py callers + # (`/api/discover/your-albums-fetch` and equivalent) stay + # byte-identical. + + @rate_limited + def _get_albums_batch(self, album_ids: List[str]) -> List[Dict[str, Any]]: + """Batch-fetch album metadata + cover art + artist names in + one request via JSON:API extended-include semantics. Returns + list of dicts matching `database.upsert_liked_album` kwargs.""" + if not album_ids: + return [] + try: + params = { + 'countryCode': 'US', + 'include': 'artists,coverArt', + 'filter[id]': ','.join(album_ids), + } + headers = {'accept': 'application/vnd.api+json'} + resp = self.session.get( + f"{self.base_url}/albums", + params=params, headers=headers, timeout=15, + ) + if resp.status_code != 200: + logger.debug( + f"Tidal albums batch returned {resp.status_code}: {resp.text[:200]}" + ) + return [] + + data = resp.json() + artists_by_id, artworks_by_id = self._build_included_maps(data.get('included', [])) + + results: List[Dict[str, Any]] = [] + for item in data.get('data', []): + if item.get('type') != 'albums': + continue + attrs = item.get('attributes', {}) + rels = item.get('relationships', {}) + results.append({ + 'tidal_id': str(item.get('id', '')), + 'album_name': attrs.get('title', '') or '', + 'artist_name': self._first_artist_name(rels, artists_by_id), + 'image_url': self._first_artwork_url(rels.get('coverArt', {}), artworks_by_id), + 'release_date': attrs.get('releaseDate', '') or '', + 'total_tracks': int(attrs.get('numberOfItems') or 0), + }) + return results + except Exception as e: + logger.debug(f"Tidal _get_albums_batch error: {e}") + return [] + + @rate_limited + def _get_artists_batch(self, artist_ids: List[str]) -> List[Dict[str, Any]]: + """Batch-fetch artist metadata + profile image. Returns list + of dicts matching the prior `get_favorite_artists` shape + (`tidal_id`, `name`, `image_url`).""" + if not artist_ids: + return [] + try: + params = { + 'countryCode': 'US', + 'include': 'profileArt', + 'filter[id]': ','.join(artist_ids), + } + headers = {'accept': 'application/vnd.api+json'} + resp = self.session.get( + f"{self.base_url}/artists", + params=params, headers=headers, timeout=15, + ) + if resp.status_code != 200: + logger.debug( + f"Tidal artists batch returned {resp.status_code}: {resp.text[:200]}" + ) + return [] + + data = resp.json() + _, artworks_by_id = self._build_included_maps(data.get('included', [])) + + results: List[Dict[str, Any]] = [] + for item in data.get('data', []): + if item.get('type') != 'artists': + continue + attrs = item.get('attributes', {}) + rels = item.get('relationships', {}) + results.append({ + 'tidal_id': str(item.get('id', '')), + 'name': attrs.get('name', '') or '', + 'image_url': self._first_artwork_url(rels.get('profileArt', {}), artworks_by_id), + }) + return results + except Exception as e: + logger.debug(f"Tidal _get_artists_batch error: {e}") + return [] + + @staticmethod + def _build_included_maps(included: List[Dict[str, Any]]): + """Index a JSON:API `included[]` array by resource type so the + per-resource lookup in batch-hydrate is O(1) per relationship + ref rather than O(n).""" + artists_by_id: Dict[str, Dict[str, Any]] = {} + artworks_by_id: Dict[str, Dict[str, Any]] = {} + for inc in included: + inc_id = str(inc.get('id', '')) + if not inc_id: + continue + inc_type = inc.get('type') + if inc_type == 'artists': + artists_by_id[inc_id] = inc + elif inc_type == 'artworks': + artworks_by_id[inc_id] = inc + return artists_by_id, artworks_by_id + + @staticmethod + def _first_artist_name(relationships: Dict[str, Any], + artists_by_id: Dict[str, Dict[str, Any]]) -> str: + """Resolve the primary artist name from a relationships block + + included-artists map. Returns '' if not resolvable so the + upsert path doesn't trip on None.""" + artist_refs = relationships.get('artists', {}).get('data', []) + if not artist_refs: + return '' + first_id = str(artist_refs[0].get('id', '')) + artist_obj = artists_by_id.get(first_id, {}) + return artist_obj.get('attributes', {}).get('name', '') or '' + + @staticmethod + def _first_artwork_url(artwork_relationship: Dict[str, Any], + artworks_by_id: Dict[str, Dict[str, Any]]) -> Optional[str]: + """Resolve the largest cover/profile image URL from an artwork + relationship + included-artworks map. Tidal returns files + largest-first so picking files[0] gets the highest-resolution + variant (typically 1280×1280).""" + refs = artwork_relationship.get('data', []) + if not refs: + return None + first_id = str(refs[0].get('id', '')) + artwork = artworks_by_id.get(first_id, {}) + files = artwork.get('attributes', {}).get('files', []) + if not files: + return None + return files[0].get('href') + + def get_favorite_albums(self, limit: int = 200) -> List[Dict[str, Any]]: + """Fetch user's favorited albums via the V2 user-collection + endpoint. Replaces the prior `/v2/favorites` + V1-fallback + path which is now dead (V2 endpoint deprecated, V1 returns + 403 for modern OAuth tokens lacking `r_usr` scope). + + Returns list of dicts matching `database.upsert_liked_album` + kwargs — the discover.py 'Your Albums' aggregator iterates + these and writes them to the liked_albums table.""" + try: + album_ids = self._iter_collection_album_ids(max_ids=limit) + if not album_ids: + return [] + + results: List[Dict[str, Any]] = [] + for i in range(0, len(album_ids), self._COLLECTION_BATCH_SIZE): + batch = album_ids[i:i + self._COLLECTION_BATCH_SIZE] + results.extend(self._get_albums_batch(batch)) + + logger.info( + f"Retrieved {len(results)}/{len(album_ids)} favorite albums from Tidal" + ) + return results + except Exception as e: + logger.error(f"Error fetching Tidal favorite albums: {e}") + return [] + + def get_favorite_artists(self, limit: int = 200) -> List[Dict[str, Any]]: + """Fetch user's favorited artists via the V2 user-collection + endpoint. Replaces the prior `/v2/favorites` + V1-fallback + path (dead for the same reason as `get_favorite_albums`). + + Returns list of dicts matching the prior shape (`tidal_id`, + `name`, `image_url`) so web_server.py's `/api/discover/ + your-artists-fetch` aggregator path stays byte-identical.""" + try: + artist_ids = self._iter_collection_artist_ids(max_ids=limit) + if not artist_ids: + return [] + + results: List[Dict[str, Any]] = [] + for i in range(0, len(artist_ids), self._COLLECTION_BATCH_SIZE): + batch = artist_ids[i:i + self._COLLECTION_BATCH_SIZE] + results.extend(self._get_artists_batch(batch)) + + logger.info( + f"Retrieved {len(results)}/{len(artist_ids)} favorite artists from Tidal" + ) + return results + except Exception as e: + logger.error(f"Error fetching Tidal favorite artists: {e}") + return [] + # Global instance _tidal_client = None diff --git a/entrypoint.sh b/entrypoint.sh index 8bbf43d8..7d830525 100644 --- a/entrypoint.sh +++ b/entrypoint.sh @@ -72,9 +72,31 @@ chown soulsync:soulsync /app/config/settings.py 2>/dev/null || true # Ensure all directories exist with correct ownership. # Only the directory nodes themselves need chown here — the recursive chown # above already ran if UIDs changed, so avoid walking the whole tree every start. -mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging +# Both the mkdir and chown tolerate failure (`|| true`): the Dockerfile +# pre-bakes every dir and bind-mounted volumes from the host already exist +# at this point, so the only failure modes are: +# - rootless Docker/Podman where in-container root maps to a host UID +# that can't write to a bind-mounted path (mkdir EACCES) +# - read-only mounts or NFS with squashed root (chown EPERM) +# Pre-mid-2026 the chown line had `|| true` but mkdir didn't — combined +# with `set -e`, a permission-denied mkdir crashed the container into a +# restart loop. Both lines are now best-effort. +mkdir -p /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging 2>/dev/null || true chown soulsync:soulsync /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging 2>/dev/null || true +# Writability audit — surface a loud warning if any bind-mounted dir +# isn't writable by the soulsync user. The restart-loop fix above makes +# the container start regardless, but a non-writable Staging / downloads +# / Transfer will fail silently inside the app (auto-import quarantine, +# download writes). Better to log now than to debug missing files later. +for dir in /app/config /app/data /app/logs /app/downloads /app/Transfer /app/Staging /app/MusicVideos /app/scripts; do + if [ -d "$dir" ] && ! gosu soulsync test -w "$dir" 2>/dev/null; then + echo "⚠️ WARNING: $dir is not writable by soulsync (uid $(id -u soulsync))." + echo " Host bind-mount perms likely mismatch the PUID/PGID env vars." + echo " Fix on host: chown -R $(id -u soulsync):$(id -g soulsync) $(echo $dir | sed 's|/app/|./|')" + fi +done + echo "✅ Configuration initialized successfully" # Display final user info diff --git a/services/sync_service.py b/services/sync_service.py index 86b12920..5177c59d 100644 --- a/services/sync_service.py +++ b/services/sync_service.py @@ -171,7 +171,7 @@ class PlaylistSyncService: failed_tracks=failed_tracks )) - async def sync_playlist(self, playlist: SpotifyPlaylist, download_missing: bool = False, profile_id: int = None) -> SyncResult: + async def sync_playlist(self, playlist: SpotifyPlaylist, download_missing: bool = False, profile_id: int = None, sync_mode: str = 'replace') -> SyncResult: self._active_profile_id = profile_id # Check if THIS specific playlist is already syncing if playlist.name in self.syncing_playlists: @@ -314,9 +314,20 @@ class PlaylistSyncService: 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") - # THE FIX: Ensure we are passing the correct, native track objects to the client - sync_success = media_client.update_playlist(playlist.name, valid_tracks) + logger.info( + f"Syncing playlist '{playlist.name}' to {server_type.upper()} server " + f"(mode: {sync_mode})" + ) + # 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) + 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 diff --git a/tests/discovery/test_discovery_sync.py b/tests/discovery/test_discovery_sync.py index 8cd9eee0..1ac44d76 100644 --- a/tests/discovery/test_discovery_sync.py +++ b/tests/discovery/test_discovery_sync.py @@ -73,7 +73,7 @@ class _FakeSyncService: def clear_progress_callback(self, playlist_name): self.cleared_callbacks.append(playlist_name) - async def sync_playlist(self, playlist, download_missing=False, profile_id=1): + async def sync_playlist(self, playlist, download_missing=False, profile_id=1, sync_mode='replace'): if self._raise_on_sync: raise self._raise_on_sync return self._sync_result diff --git a/tests/imports/test_album_search_scoring.py b/tests/imports/test_album_search_scoring.py new file mode 100644 index 00000000..f2e52caf --- /dev/null +++ b/tests/imports/test_album_search_scoring.py @@ -0,0 +1,217 @@ +"""Pin `_score_album_search_result` weight math. + +Helper extracted from the inline scoring block inside +`_search_metadata_source` (auto-import album identification). Lifting +it to a pure function lets each weight be tested in isolation +without mocking the full source-chain orchestrator. + +Weights (constants in `core.auto_import_worker`): + - `_ALBUM_NAME_WEIGHT` = 0.5 + - `_ARTIST_NAME_WEIGHT` = 0.2 (skipped when target_artist falsy) + - `_TRACK_COUNT_WEIGHT` = 0.3 (skipped when either side has 0 tracks) + +Maximum score is 1.0 when all three components match perfectly. The +0.4 threshold in the orchestrator means a result needs at least one +strong signal plus a partial second — pure track-count match alone +(0.3) is below threshold. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest + +from core.auto_import_worker import ( + _ALBUM_NAME_WEIGHT, + _ARTIST_NAME_WEIGHT, + _TRACK_COUNT_WEIGHT, + _score_album_search_result, +) + + +def _result(name: str, artist_name: str = "", total_tracks: int = 0): + """Minimal album-result stub matching the shape `search_albums` + returns. `artists` is the list-of-dicts shape every adapter uses.""" + artists = [{"name": artist_name}] if artist_name else [] + return SimpleNamespace(name=name, artists=artists, total_tracks=total_tracks) + + +# --------------------------------------------------------------------------- +# Component weights — pinned at the boundary +# --------------------------------------------------------------------------- + + +class TestWeightConstants: + def test_weights_sum_to_one(self): + """Total weight budget = 1.0. If a weight is bumped without + adjusting another, perfect-match score drifts above/below 1.0 + and the 0.4 threshold semantics shift silently.""" + total = _ALBUM_NAME_WEIGHT + _ARTIST_NAME_WEIGHT + _TRACK_COUNT_WEIGHT + assert total == pytest.approx(1.0, abs=1e-9), ( + f"Weights must sum to 1.0; got {total}" + ) + + def test_album_weight_is_dominant(self): + """Album name has 50% — strongest signal. If artist or track + count weight ever exceeds album weight, the matching semantics + flip (e.g. a wrong-album-right-count result could outscore + a right-album-wrong-count one). Pin so a future weight tweak + doesn't break this invariant.""" + assert _ALBUM_NAME_WEIGHT > _ARTIST_NAME_WEIGHT + assert _ALBUM_NAME_WEIGHT > _TRACK_COUNT_WEIGHT + + +# --------------------------------------------------------------------------- +# Perfect match → 1.0 +# --------------------------------------------------------------------------- + + +class TestPerfectMatch: + def test_all_three_perfect_returns_one(self): + r = _result("Test Album", "Test Artist", total_tracks=10) + score = _score_album_search_result(r, "Test Album", "Test Artist", file_count=10) + assert score == pytest.approx(1.0, abs=1e-9) + + +# --------------------------------------------------------------------------- +# Album name component +# --------------------------------------------------------------------------- + + +class TestAlbumNameWeight: + def test_exact_album_match_no_other_signals(self): + """Album name matches perfectly but no artist provided and + no track count match — score is just 50%.""" + r = _result("Test Album", total_tracks=0) + score = _score_album_search_result(r, "Test Album", target_artist=None, file_count=0) + assert score == pytest.approx(_ALBUM_NAME_WEIGHT, abs=1e-9) + + def test_completely_different_album_zero_album_component(self): + r = _result("Totally Different", total_tracks=0) + score = _score_album_search_result(r, "Test Album", target_artist=None, file_count=0) + # SequenceMatcher would return some small non-zero similarity even + # for fully different strings, so just verify it's well below 0.5 + assert score < _ALBUM_NAME_WEIGHT * 0.5 + + +# --------------------------------------------------------------------------- +# Artist component +# --------------------------------------------------------------------------- + + +class TestArtistWeight: + def test_artist_match_adds_full_artist_weight(self): + r = _result("Album", "Artist", total_tracks=0) + with_artist = _score_album_search_result(r, "Album", "Artist", file_count=0) + without_artist = _score_album_search_result(r, "Album", None, file_count=0) + # Difference = artist weight + assert with_artist - without_artist == pytest.approx(_ARTIST_NAME_WEIGHT, abs=1e-9) + + def test_target_artist_none_skips_artist_component(self): + """When target_artist is falsy (None / empty), artist weight + contributes zero — not a penalty, not a bonus. Lets album- + only searches (e.g. from a folder name with no artist info) + still hit the threshold via album + track count alone.""" + r = _result("Album", "WrongArtist", total_tracks=10) + score = _score_album_search_result(r, "Album", None, file_count=10) + # Album + track count perfect = 0.5 + 0.3 = 0.8 (artist weight skipped) + assert score == pytest.approx(_ALBUM_NAME_WEIGHT + _TRACK_COUNT_WEIGHT, abs=1e-9) + + def test_string_artist_not_dict_still_scored(self): + """Some adapters return `artists` as list-of-strings instead + of list-of-dicts. Helper must handle both shapes.""" + r = SimpleNamespace(name="Album", artists=["Just A String"], total_tracks=0) + score_string = _score_album_search_result(r, "Album", "Just A String", 0) + assert score_string >= _ALBUM_NAME_WEIGHT + _ARTIST_NAME_WEIGHT - 0.05 + + def test_empty_artists_list_treats_as_no_artist(self): + r = _result("Album", artist_name="", total_tracks=0) # no artist + score = _score_album_search_result(r, "Album", "Some Target", file_count=0) + # Artist sim against empty string is 0 → no artist weight contribution + assert score == pytest.approx(_ALBUM_NAME_WEIGHT, abs=1e-9) + + +# --------------------------------------------------------------------------- +# Track count component +# --------------------------------------------------------------------------- + + +class TestTrackCountWeight: + def test_exact_track_count_match_full_weight(self): + r = _result("Album", total_tracks=10) + score = _score_album_search_result(r, "Album", None, file_count=10) + # Album (perfect) + track count (perfect) — no artist component + assert score == pytest.approx(_ALBUM_NAME_WEIGHT + _TRACK_COUNT_WEIGHT, abs=1e-9) + + def test_off_by_one_track_count_near_full(self): + """1 track off out of 10 → ratio = 1.0 - 1/10 = 0.9 → 0.9 * 0.3 = 0.27""" + r = _result("Album", total_tracks=10) + score = _score_album_search_result(r, "Album", None, file_count=9) + expected = _ALBUM_NAME_WEIGHT + (0.9 * _TRACK_COUNT_WEIGHT) + assert score == pytest.approx(expected, abs=1e-9) + + def test_bandcamp_vs_streaming_track_count_mismatch(self): + """Reporter's exact case: Bandcamp 7-track album vs Spotify + 4-track release. Track count ratio = 1.0 - 3/7 = ~0.571. + With perfect album + artist match, total = 0.5 + 0.2 + 0.171 + = 0.871 → comfortably above the 0.4 threshold so the album + still identifies despite the count mismatch.""" + r = _result("Work in Progress", "Godly the Ruler", total_tracks=4) + score = _score_album_search_result(r, "Work in Progress", "Godly the Ruler", file_count=7) + assert score > 0.4, ( + f"Bandcamp-vs-streaming case must still pass threshold; got {score:.3f}" + ) + # Sanity bound — score should land around 0.87 + assert 0.85 < score < 0.90 + + def test_zero_track_count_from_source_skips_track_component(self): + """Some search responses don't include total_tracks. Helper + must not penalize — just skip the track-count component.""" + r = _result("Album", total_tracks=0) + score = _score_album_search_result(r, "Album", None, file_count=10) + # Only album component contributes + assert score == pytest.approx(_ALBUM_NAME_WEIGHT, abs=1e-9) + + def test_zero_file_count_skips_track_component(self): + """Defensive: candidate has 0 files (somehow). Don't divide + by zero or skew the score.""" + r = _result("Album", total_tracks=10) + score = _score_album_search_result(r, "Album", None, file_count=0) + assert score == pytest.approx(_ALBUM_NAME_WEIGHT, abs=1e-9) + + def test_huge_mismatch_track_count_no_negative_contribution(self): + """File count 1, source track count 100 → ratio = 1 - 99/100 + = 0.01. Tiny but non-negative. `max(0, ...)` guards against + any future formula change introducing a negative.""" + r = _result("Album", total_tracks=100) + score = _score_album_search_result(r, "Album", None, file_count=1) + assert score >= _ALBUM_NAME_WEIGHT # at least the album component + + +# --------------------------------------------------------------------------- +# Edge cases — defensive +# --------------------------------------------------------------------------- + + +class TestEdgeCases: + def test_album_result_without_name_attribute(self): + """If the result somehow lacks `.name` (unusual adapter + return), helper falls back to '' and scores 0 album sim.""" + r = SimpleNamespace(artists=[], total_tracks=0) # no `.name` + score = _score_album_search_result(r, "Test Album", None, file_count=0) + assert score == 0.0 + + def test_album_result_without_artists_attribute(self): + """If `.artists` is missing, treat as empty list.""" + r = SimpleNamespace(name="Album", total_tracks=0) # no .artists + score = _score_album_search_result(r, "Album", "Target Artist", file_count=0) + # Album matches perfectly; artist sim against missing is 0 + assert score == pytest.approx(_ALBUM_NAME_WEIGHT, abs=1e-9) + + def test_album_result_with_none_total_tracks(self): + """Some adapters return None for missing total_tracks instead + of 0. `getattr(..., 'total_tracks', 0) or 0` should handle it.""" + r = SimpleNamespace(name="Album", artists=[], total_tracks=None) + score = _score_album_search_result(r, "Album", None, file_count=10) + assert score == pytest.approx(_ALBUM_NAME_WEIGHT, abs=1e-9) diff --git a/tests/imports/test_auto_import_clear_completed_endpoint.py b/tests/imports/test_auto_import_clear_completed_endpoint.py new file mode 100644 index 00000000..99c5077b --- /dev/null +++ b/tests/imports/test_auto_import_clear_completed_endpoint.py @@ -0,0 +1,232 @@ +"""Pin /api/auto-import/clear-completed behavior. + +Reported case: Clear History button on the Import page left zombie +rows behind — every survivor showed "⧗ Processing" from 2-9 days ago. +Trace: `_record_in_progress` inserts a `status='processing'` row up-front +so the UI can render the in-flight import; `_finalize_result` updates +it to `completed`/`failed` when the import finishes. If the worker is +killed mid-import (server restart, crash), the row never gets finalized +and stays at `processing` forever. The endpoint's SQL delete-list +omitted `processing`, so zombies survived every click. + +Fix added `processing` to the delete list, BUT guards against nuking +genuinely-live imports by intersecting against the worker's +`_snapshot_active()` map — any folder hash currently registered there +is excluded from the delete. + +These tests pin: +- `processing` rows ARE swept (no longer zombies) +- Live `processing` rows (folder hash currently in `_active_imports`) survive +- `pending_review` survives (user still must approve/reject) +- `completed` / `approved` / `failed` / `needs_identification` / + `rejected` rows still get swept (unchanged contract) +- Count returned in the JSON response matches the actual delete count +- Empty active set falls through to the unparameterized DELETE +""" + +from __future__ import annotations + +import sqlite3 +from contextlib import contextmanager +from unittest.mock import MagicMock + +import pytest + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture +def app_test_client(): + import web_server + web_server.app.config['TESTING'] = True + with web_server.app.test_client() as client: + yield client + + +@pytest.fixture +def seeded_db(tmp_path): + """Real sqlite DB with the auto_import_history table populated by + a mix of statuses + folder hashes. Returns a (connection_factory, + rows_seeded) tuple. The factory is a `_get_connection` lookalike + that returns the same connection so the endpoint sees the same data.""" + db_path = str(tmp_path / 'test.db') + conn = sqlite3.connect(db_path, check_same_thread=False) + conn.row_factory = sqlite3.Row + cursor = conn.cursor() + cursor.execute(""" + CREATE TABLE auto_import_history ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + folder_name TEXT NOT NULL, + folder_path TEXT NOT NULL, + folder_hash TEXT, + status TEXT NOT NULL DEFAULT 'scanning', + confidence REAL DEFAULT 0.0, + album_id TEXT, + album_name TEXT, + artist_name TEXT, + image_url TEXT, + total_files INTEGER DEFAULT 0, + matched_files INTEGER DEFAULT 0, + match_data TEXT, + identification_method TEXT, + error_message TEXT, + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + processed_at TIMESTAMP + ) + """) + seeds = [ + # (folder_name, folder_hash, status) + ('Album A', 'hash-completed-1', 'completed'), + ('Album B', 'hash-approved-1', 'approved'), + ('Album C', 'hash-failed-1', 'failed'), + ('Album D', 'hash-needsid-1', 'needs_identification'), + ('Album E', 'hash-rejected-1', 'rejected'), + ('Zombie F', 'hash-zombie-1', 'processing'), # stale + ('Zombie G', 'hash-zombie-2', 'processing'), # stale + ('Live Import H', 'hash-LIVE', 'processing'), # active — must survive + ('Awaiting Review I', 'hash-review-1', 'pending_review'), # must survive + ] + for name, fh, status in seeds: + cursor.execute( + "INSERT INTO auto_import_history (folder_name, folder_path, folder_hash, status) " + "VALUES (?, ?, ?, ?)", + (name, f"/staging/{name}", fh, status), + ) + conn.commit() + + # Build a fake DB object whose `_get_connection` returns a context + # manager wrapping the live connection (matches the endpoint's + # `with db._get_connection() as conn` usage). + @contextmanager + def _conn_cm(): + yield conn + + fake_db = MagicMock() + fake_db._get_connection = _conn_cm + + return fake_db, conn + + +def _statuses_remaining(conn): + cur = conn.cursor() + cur.execute("SELECT folder_name, status FROM auto_import_history ORDER BY id") + return [(row['folder_name'], row['status']) for row in cur.fetchall()] + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + + +class TestClearCompletedEndpoint: + def test_sweeps_zombie_processing_rows_but_keeps_live(self, app_test_client, seeded_db, monkeypatch): + """The bug: a `processing` row that's been there for days is a + zombie (server restart killed `_finalize_result`). Endpoint must + sweep it. But a `processing` row whose folder_hash is currently + registered in `_active_imports` is a LIVE import — must survive + or the UI loses its in-flight row mid-run.""" + fake_db, conn = seeded_db + # Worker reports one live import — folder_hash hash-LIVE + fake_worker = MagicMock() + fake_worker._snapshot_active.return_value = [{'folder_hash': 'hash-LIVE'}] + monkeypatch.setattr('web_server.auto_import_worker', fake_worker) + monkeypatch.setattr('web_server.get_database', lambda: fake_db) + + resp = app_test_client.post('/api/auto-import/clear-completed') + assert resp.status_code == 200 + body = resp.get_json() + assert body['success'] is True + + survivors = _statuses_remaining(conn) + names = {n for n, _ in survivors} + # Live import survives + assert 'Live Import H' in names, ( + f"Live in-flight processing row was deleted; survivors={names}" + ) + # Pending review survives — user still must approve/reject + assert 'Awaiting Review I' in names + # Both zombies swept + assert 'Zombie F' not in names + assert 'Zombie G' not in names + # Standard terminal-status rows swept + for completed_name in ('Album A', 'Album B', 'Album C', 'Album D', 'Album E'): + assert completed_name not in names, f"{completed_name} should have been deleted" + + def test_count_in_response_matches_actual_deletes(self, app_test_client, seeded_db, monkeypatch): + """JSON response carries the rowcount so the UI toast can show + accurate `Cleared N items`.""" + fake_db, conn = seeded_db + fake_worker = MagicMock() + fake_worker._snapshot_active.return_value = [{'folder_hash': 'hash-LIVE'}] + monkeypatch.setattr('web_server.auto_import_worker', fake_worker) + monkeypatch.setattr('web_server.get_database', lambda: fake_db) + + resp = app_test_client.post('/api/auto-import/clear-completed') + body = resp.get_json() + # 9 rows seeded; 7 deletable (5 terminal + 2 zombie processing); + # 2 survive (1 live, 1 pending_review) + assert body['count'] == 7, f"Expected 7 deletes; got {body['count']}" + cur = conn.cursor() + cur.execute("SELECT COUNT(*) FROM auto_import_history") + assert cur.fetchone()[0] == 2 + + def test_empty_active_set_takes_unparameterized_path(self, app_test_client, seeded_db, monkeypatch): + """When no live imports are running, the SQL skips the `AND + folder_hash NOT IN (...)` clause. Pinned because an empty + `IN ()` is a SQL syntax error in sqlite — the branch matters.""" + fake_db, conn = seeded_db + # Remove the live row so all `processing` rows are zombies + cur = conn.cursor() + cur.execute("DELETE FROM auto_import_history WHERE folder_hash = ?", ('hash-LIVE',)) + conn.commit() + + fake_worker = MagicMock() + fake_worker._snapshot_active.return_value = [] # nothing active + monkeypatch.setattr('web_server.auto_import_worker', fake_worker) + monkeypatch.setattr('web_server.get_database', lambda: fake_db) + + resp = app_test_client.post('/api/auto-import/clear-completed') + assert resp.status_code == 200 + body = resp.get_json() + assert body['success'] is True + # 5 terminal + 2 zombie processing = 7. Pending_review (1) survives. + assert body['count'] == 7 + + survivors = _statuses_remaining(conn) + assert len(survivors) == 1 + assert survivors[0][1] == 'pending_review' + + def test_worker_unavailable_returns_500(self, app_test_client, monkeypatch): + """If the auto-import worker isn't initialised, the endpoint + bails early — no DB access, clear error. Pre-fix this branch + was already in place; pinning ensures the active-hash refactor + didn't accidentally start touching the worker before the guard.""" + monkeypatch.setattr('web_server.auto_import_worker', None) + resp = app_test_client.post('/api/auto-import/clear-completed') + assert resp.status_code == 500 + body = resp.get_json() + assert body['success'] is False + assert 'not available' in body['error'].lower() + + def test_pending_review_always_survives(self, app_test_client, seeded_db, monkeypatch): + """Specific pin for the deliberate `pending_review` exclusion. + Even when no imports are active and every other status is being + swept, `pending_review` rows must be left alone — user-action + required, not automatic cleanup.""" + fake_db, conn = seeded_db + fake_worker = MagicMock() + fake_worker._snapshot_active.return_value = [] + monkeypatch.setattr('web_server.auto_import_worker', fake_worker) + monkeypatch.setattr('web_server.get_database', lambda: fake_db) + + app_test_client.post('/api/auto-import/clear-completed') + + cur = conn.cursor() + cur.execute("SELECT COUNT(*) FROM auto_import_history WHERE status = 'pending_review'") + assert cur.fetchone()[0] == 1, ( + "pending_review rows must never be swept by clear-completed" + ) diff --git a/tests/imports/test_auto_import_context_shape.py b/tests/imports/test_auto_import_context_shape.py index de9d797f..427ca8c9 100644 --- a/tests/imports/test_auto_import_context_shape.py +++ b/tests/imports/test_auto_import_context_shape.py @@ -480,6 +480,7 @@ def test_search_metadata_source_extracts_artist_id_from_dict_artist(): worker = AutoImportWorker(database=MagicMock(), process_callback=lambda *a, **k: None) with patch("core.metadata_service.get_primary_source", return_value="spotify"), \ + patch("core.metadata_service.get_source_priority", return_value=["spotify"]), \ patch("core.metadata_service.get_client_for_source", return_value=fake_client): result = worker._search_metadata_source( "Test Artist", "Test Album", "tags", candidate, diff --git a/tests/imports/test_auto_import_multi_source_fallback.py b/tests/imports/test_auto_import_multi_source_fallback.py new file mode 100644 index 00000000..a3a81c4a --- /dev/null +++ b/tests/imports/test_auto_import_multi_source_fallback.py @@ -0,0 +1,365 @@ +"""Pin auto-import multi-source fallback for album identification. + +Discord report (mushy, paraphrased): 16 Bandcamp albums sat in staging +because auto-import couldn't identify them. Manual search at the +bottom of the Import Music tab found the same albums fine via Tidal +or Deezer — the user's primary metadata source (Spotify) just didn't +have them. + +Root cause: `_search_metadata_source` only queried the primary source. +The manual `search_import_albums` path already iterates the full +`get_source_priority(get_primary_source())` chain and breaks on first +source that returns results. This brings auto-import to parity. + +Fix semantics (option C — "primary first, fall through on weak"): + - Try primary source first + - Score result; if best ≥ 0.4 → return with that source + - Otherwise fall through to next source in priority order + - First source that produces a result above threshold wins + - Returns None only if ALL sources fail / score below threshold + +Tests pin: + - Primary success path unchanged (returns primary result, no fallback fired) + - Primary returns nothing → fallback fires to next source + - Primary scores below threshold → fallback fires + - First fallback succeeds → no further sources queried + - All sources fail → None + - Per-source exception is contained (doesn't abort the chain) + - Result `source` field reflects WHICH source actually matched + - `identification_confidence` is the score from the winning source +""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + +from core.auto_import_worker import AutoImportWorker, FolderCandidate + + +def _make_album(name: str, artist_name: str, total_tracks: int, + album_id: str = "alb-id", artist_id: str = "art-id"): + """Build a fake album result matching `search_albums` return shape.""" + return SimpleNamespace( + id=album_id, + name=name, + artists=[{"id": artist_id, "name": artist_name}], + total_tracks=total_tracks, + image_url="https://img.example/cover.jpg", + release_date="2024-01-01", + ) + + +def _make_worker(): + """Bare AutoImportWorker bypassing __init__ side effects.""" + return AutoImportWorker(database=MagicMock(), process_callback=lambda *a, **k: None) + + +def _make_candidate(file_count: int = 7, name: str = "TestAlbum"): + """Folder candidate with N files (no actual disk reads).""" + return FolderCandidate( + path=f"/staging/{name}", + name=name, + audio_files=[f"/staging/{name}/{i:02d}.flac" for i in range(1, file_count + 1)], + ) + + +# --------------------------------------------------------------------------- +# Primary success path — fallback never fires +# --------------------------------------------------------------------------- + + +class TestPrimarySuccess: + def test_primary_returns_strong_match_no_fallback(self): + """Pre-fix behavior preserved: if primary scores above 0.4, + return its result and don't touch other sources.""" + worker = _make_worker() + candidate = _make_candidate(file_count=10) + + spotify_client = MagicMock() + spotify_client.search_albums.return_value = [ + _make_album("Test Album", "Test Artist", total_tracks=10), + ] + tidal_client = MagicMock() # should NEVER be called + + def client_dispatch(source, **kwargs): + return {"spotify": spotify_client, "tidal": tidal_client}.get(source) + + with patch("core.metadata_service.get_primary_source", return_value="spotify"), \ + patch("core.metadata_service.get_source_priority", + return_value=["spotify", "tidal", "deezer"]), \ + patch("core.metadata_service.get_client_for_source", + side_effect=client_dispatch): + result = worker._search_metadata_source( + "Test Artist", "Test Album", "tags", candidate, + ) + + assert result is not None + assert result["source"] == "spotify" + assert result["album_name"] == "Test Album" + spotify_client.search_albums.assert_called_once() + tidal_client.search_albums.assert_not_called() + + +# --------------------------------------------------------------------------- +# Primary fails — fallback fires +# --------------------------------------------------------------------------- + + +class TestFallbackOnNoResults: + def test_primary_empty_falls_through_to_next_source(self): + """Reporter's exact case: Spotify doesn't have the Bandcamp + indie album. Tidal does. Auto-import must find it via Tidal.""" + worker = _make_worker() + candidate = _make_candidate(file_count=7) + + spotify_client = MagicMock() + spotify_client.search_albums.return_value = [] # not on Spotify + tidal_client = MagicMock() + tidal_client.search_albums.return_value = [ + _make_album("Work in Progress", "Godly the Ruler", total_tracks=7, + album_id="tidal-alb-1", artist_id="tidal-art-1"), + ] + + def client_dispatch(source, **kwargs): + return {"spotify": spotify_client, "tidal": tidal_client}.get(source) + + with patch("core.metadata_service.get_primary_source", return_value="spotify"), \ + patch("core.metadata_service.get_source_priority", + return_value=["spotify", "tidal", "deezer"]), \ + patch("core.metadata_service.get_client_for_source", + side_effect=client_dispatch): + result = worker._search_metadata_source( + "Godly the Ruler", "Work in Progress", "tags", candidate, + ) + + assert result is not None + assert result["source"] == "tidal", "Result must carry the source that actually matched" + assert result["album_id"] == "tidal-alb-1" + assert result["artist_id"] == "tidal-art-1" + spotify_client.search_albums.assert_called_once() + tidal_client.search_albums.assert_called_once() + + +class TestFallbackOnWeakScore: + def test_primary_below_threshold_falls_through(self): + """Primary returns results but none score above 0.4 (e.g. + wrong-album false-matches). Fall through to next source for + a stronger match.""" + worker = _make_worker() + candidate = _make_candidate(file_count=7) + + spotify_client = MagicMock() + # Wrong album — name barely matches, no artist match, wrong track count + spotify_client.search_albums.return_value = [ + _make_album("Different", "Wrong Artist", total_tracks=2), + ] + deezer_client = MagicMock() + deezer_client.search_albums.return_value = [ + _make_album("Work in Progress", "Godly the Ruler", total_tracks=7), + ] + + def client_dispatch(source, **kwargs): + return {"spotify": spotify_client, "deezer": deezer_client}.get(source) + + with patch("core.metadata_service.get_primary_source", return_value="spotify"), \ + patch("core.metadata_service.get_source_priority", + return_value=["spotify", "deezer"]), \ + patch("core.metadata_service.get_client_for_source", + side_effect=client_dispatch): + result = worker._search_metadata_source( + "Godly the Ruler", "Work in Progress", "tags", candidate, + ) + + assert result is not None + assert result["source"] == "deezer" + assert result["album_name"] == "Work in Progress" + # Both clients called — primary returned weak, fallback picked up + spotify_client.search_albums.assert_called_once() + deezer_client.search_albums.assert_called_once() + + +# --------------------------------------------------------------------------- +# Chain semantics +# --------------------------------------------------------------------------- + + +class TestChainSemantics: + def test_first_fallback_success_stops_chain(self): + """When fallback succeeds, no further sources are queried. + Don't waste API budget on Deezer if Tidal already gave us a + strong result.""" + worker = _make_worker() + candidate = _make_candidate(file_count=10) + + spotify_client = MagicMock() + spotify_client.search_albums.return_value = [] + tidal_client = MagicMock() + tidal_client.search_albums.return_value = [ + _make_album("Test", "Artist", total_tracks=10), + ] + deezer_client = MagicMock() # should NEVER be called + + def client_dispatch(source, **kwargs): + return {"spotify": spotify_client, + "tidal": tidal_client, + "deezer": deezer_client}.get(source) + + with patch("core.metadata_service.get_primary_source", return_value="spotify"), \ + patch("core.metadata_service.get_source_priority", + return_value=["spotify", "tidal", "deezer"]), \ + patch("core.metadata_service.get_client_for_source", + side_effect=client_dispatch): + result = worker._search_metadata_source("Artist", "Test", "tags", candidate) + + assert result is not None + assert result["source"] == "tidal" + deezer_client.search_albums.assert_not_called() + + def test_all_sources_fail_returns_none(self): + """If every source returns nothing or scores below threshold, + the whole search returns None (caller proceeds to next + identification strategy).""" + worker = _make_worker() + candidate = _make_candidate(file_count=7) + + empty_client = MagicMock() + empty_client.search_albums.return_value = [] + + with patch("core.metadata_service.get_primary_source", return_value="spotify"), \ + patch("core.metadata_service.get_source_priority", + return_value=["spotify", "tidal", "deezer"]), \ + patch("core.metadata_service.get_client_for_source", + return_value=empty_client): + result = worker._search_metadata_source( + "Unknown Artist", "Nonexistent Album", "tags", candidate, + ) + + assert result is None + # All 3 sources got queried + assert empty_client.search_albums.call_count == 3 + + def test_per_source_exception_does_not_abort_chain(self): + """If one source raises (rate limit, auth, transient HTTP), + the chain continues to the next source instead of aborting + the whole identification attempt.""" + worker = _make_worker() + candidate = _make_candidate(file_count=10) + + spotify_client = MagicMock() + spotify_client.search_albums.side_effect = RuntimeError("rate limit") + tidal_client = MagicMock() + tidal_client.search_albums.return_value = [ + _make_album("Test", "Artist", total_tracks=10), + ] + + def client_dispatch(source, **kwargs): + return {"spotify": spotify_client, "tidal": tidal_client}.get(source) + + with patch("core.metadata_service.get_primary_source", return_value="spotify"), \ + patch("core.metadata_service.get_source_priority", + return_value=["spotify", "tidal"]), \ + patch("core.metadata_service.get_client_for_source", + side_effect=client_dispatch): + result = worker._search_metadata_source("Artist", "Test", "tags", candidate) + + assert result is not None + assert result["source"] == "tidal", ( + "Primary exception must not block the fallback chain" + ) + + def test_unconfigured_source_skipped_gracefully(self): + """If `get_client_for_source` returns None for a source + (user hasn't configured it), skip and continue.""" + worker = _make_worker() + candidate = _make_candidate(file_count=10) + + tidal_client = MagicMock() + tidal_client.search_albums.return_value = [ + _make_album("Test", "Artist", total_tracks=10), + ] + + # Spotify returns None (no client configured); Tidal works + def client_dispatch(source, **kwargs): + if source == "spotify": + return None + return {"tidal": tidal_client}.get(source) + + with patch("core.metadata_service.get_primary_source", return_value="spotify"), \ + patch("core.metadata_service.get_source_priority", + return_value=["spotify", "tidal"]), \ + patch("core.metadata_service.get_client_for_source", + side_effect=client_dispatch): + result = worker._search_metadata_source("Artist", "Test", "tags", candidate) + + assert result is not None + assert result["source"] == "tidal" + + +# --------------------------------------------------------------------------- +# Result shape preservation +# --------------------------------------------------------------------------- + + +class TestResultShape: + def test_result_carries_correct_source_for_downstream_match(self): + """`_match_tracks` reads `identification['source']` to know + which client to ask for the album's tracklist. Result MUST + carry the source that actually matched, not the primary + source name.""" + worker = _make_worker() + candidate = _make_candidate(file_count=8) + + spotify_client = MagicMock() + spotify_client.search_albums.return_value = [] + deezer_client = MagicMock() + deezer_client.search_albums.return_value = [ + _make_album("Test", "Artist", total_tracks=8, album_id="dz-123"), + ] + + def client_dispatch(source, **kwargs): + return {"spotify": spotify_client, "deezer": deezer_client}.get(source) + + with patch("core.metadata_service.get_primary_source", return_value="spotify"), \ + patch("core.metadata_service.get_source_priority", + return_value=["spotify", "deezer"]), \ + patch("core.metadata_service.get_client_for_source", + side_effect=client_dispatch): + result = worker._search_metadata_source("Artist", "Test", "tags", candidate) + + assert result["source"] == "deezer" + assert result["album_id"] == "dz-123", ( + "Album ID must be the Deezer ID so _match_tracks queries " + "Deezer's get_album with the right ID format" + ) + + def test_identification_confidence_reflects_winning_source(self): + """`identification_confidence` is used in the overall-confidence + formula and the 0.9 / 0.7 cascade thresholds. It must be the + score from the source that actually matched.""" + worker = _make_worker() + candidate = _make_candidate(file_count=10) + + spotify_client = MagicMock() + spotify_client.search_albums.return_value = [] + # Perfect match on Tidal — all 3 weights at max → score = 1.0 + tidal_client = MagicMock() + tidal_client.search_albums.return_value = [ + _make_album("Test Album", "Test Artist", total_tracks=10), + ] + + def client_dispatch(source, **kwargs): + return {"spotify": spotify_client, "tidal": tidal_client}.get(source) + + with patch("core.metadata_service.get_primary_source", return_value="spotify"), \ + patch("core.metadata_service.get_source_priority", + return_value=["spotify", "tidal"]), \ + patch("core.metadata_service.get_client_for_source", + side_effect=client_dispatch): + result = worker._search_metadata_source( + "Test Artist", "Test Album", "tags", candidate, + ) + + assert result["identification_confidence"] == pytest.approx(1.0, abs=0.01) diff --git a/tests/library/test_path_resolver.py b/tests/library/test_path_resolver.py index 9c32e9e3..713fc8ae 100644 --- a/tests/library/test_path_resolver.py +++ b/tests/library/test_path_resolver.py @@ -403,3 +403,139 @@ def test_docker_resolve_path_pass_through_outside_docker(monkeypatch) -> None: monkeypatch.setattr(os.path, "exists", lambda p: False if p == "/.dockerenv" else real_exists(p)) assert path_resolver._docker_resolve_path("H:\\Music\\track.flac") == "H:\\Music\\track.flac" + + +# --------------------------------------------------------------------------- +# Diagnostic helper — issue #558 (gabistek, Navidrome on Docker) +# --------------------------------------------------------------------------- +# +# `resolve_library_file_path_with_diagnostic` returns +# `(resolved, ResolveAttempt)` so callers can render a useful error +# instead of a silent None. Pre-fix the Album Completeness "Auto-Fill" +# button surfaced "Could not determine album folder from existing +# tracks" with no diagnostic, leaving Navidrome users (whose Subsonic +# API doesn't expose library paths the way Plex's does) with no signal +# about what to configure. + + +from core.library.path_resolver import ( # noqa: E402 + ResolveAttempt, + resolve_library_file_path_with_diagnostic, +) + + +class TestResolveAttemptShape: + def test_returns_tuple_of_path_and_attempt(self, tmp_path: Path) -> None: + real = tmp_path / "track.flac" + real.write_bytes(b"a") + result = resolve_library_file_path_with_diagnostic(str(real)) + assert isinstance(result, tuple) + assert len(result) == 2 + path, attempt = result + assert path == str(real) + assert isinstance(attempt, ResolveAttempt) + + def test_raw_path_existed_true_when_short_circuit(self, tmp_path: Path) -> None: + """Happy path → resolver short-circuits at the first + `os.path.exists` check; `base_dirs_tried` stays empty.""" + real = tmp_path / "track.flac" + real.write_bytes(b"a") + _, attempt = resolve_library_file_path_with_diagnostic(str(real)) + assert attempt.raw_path_existed is True + assert attempt.base_dirs_tried == [] + + def test_raw_path_existed_false_when_walking(self, tmp_path: Path) -> None: + """When the raw path doesn't exist but the suffix-walk finds it, + the attempt should report `raw_path_existed=False` and list the + base dir that succeeded among `base_dirs_tried`.""" + # Create the file under a real base dir at a different parent + base = tmp_path / "library" + target = base / "Artist" / "Album" / "track.flac" + target.parent.mkdir(parents=True) + target.write_bytes(b"a") + + # DB stores it as if scanned at /music/Artist/Album/track.flac + db_path = "/music/Artist/Album/track.flac" + path, attempt = resolve_library_file_path_with_diagnostic( + db_path, transfer_folder=str(base), + ) + assert path == str(target), f"suffix-walk should have found the file under {base}" + assert attempt.raw_path_existed is False + assert str(base) in attempt.base_dirs_tried + + +class TestDiagnosticForFailedResolves: + def test_no_base_dirs_returns_none_with_empty_attempt(self) -> None: + """No transfer/download/config/plex → resolver can't probe. + Diagnostic must report empty `base_dirs_tried` so the caller can + render a "no probe sources configured" hint.""" + path, attempt = resolve_library_file_path_with_diagnostic( + "/music/Artist/Album/track.flac", + ) + assert path is None + assert attempt.raw_path_existed is False + assert attempt.base_dirs_tried == [] + assert attempt.had_config_manager is False + assert attempt.had_plex_client is False + + def test_base_dirs_listed_even_when_walk_fails(self, tmp_path: Path) -> None: + """When base dirs exist but the suffix-walk doesn't find the + file, `base_dirs_tried` must still report what was probed. + Lets the caller surface "we tried X, Y, Z" in the error.""" + base = tmp_path / "transfer" + base.mkdir() + # Don't create the target file — the walk will fail + path, attempt = resolve_library_file_path_with_diagnostic( + "/music/Artist/Album/missing.flac", + transfer_folder=str(base), + ) + assert path is None + assert str(base) in attempt.base_dirs_tried + + def test_had_flags_track_caller_inputs(self, tmp_path: Path) -> None: + """`had_config_manager` / `had_plex_client` reflect what the + caller passed in — useful for distinguishing 'caller didn't + wire up the optional input' from 'optional input was wired up + but produced no usable base dirs'.""" + config = MagicMock() + config.get.return_value = "" # no config-driven paths + plex = SimpleNamespace(server=None, music_library=None) + + path, attempt = resolve_library_file_path_with_diagnostic( + "/music/Artist/track.flac", + config_manager=config, + plex_client=plex, + ) + assert path is None + assert attempt.had_config_manager is True + assert attempt.had_plex_client is True + + +class TestBackwardsCompat: + def test_existing_resolve_function_delegates_to_diagnostic(self, tmp_path: Path) -> None: + """The non-diagnostic `resolve_library_file_path` is now a thin + wrapper that drops the attempt. Pin that the legacy signature + still returns the same path values across all the cases the + old function covered, so existing callers don't see drift.""" + real = tmp_path / "track.flac" + real.write_bytes(b"a") + + # Happy path + assert resolve_library_file_path(str(real)) == str(real) + + # Suffix-walk path + base = tmp_path / "lib" + target = base / "Artist" / "Album" / "track.flac" + target.parent.mkdir(parents=True) + target.write_bytes(b"a") + result = resolve_library_file_path( + "/music/Artist/Album/track.flac", + transfer_folder=str(base), + ) + assert result == str(target) + + # Failure path + assert resolve_library_file_path( + "/music/Artist/missing.flac", + transfer_folder=str(base), + ) is None diff --git a/tests/metadata/test_discography_filters.py b/tests/metadata/test_discography_filters.py new file mode 100644 index 00000000..400e0885 --- /dev/null +++ b/tests/metadata/test_discography_filters.py @@ -0,0 +1,322 @@ +"""Pin track-level filters used by the Download Discography endpoint. + +GitHub issue #559 (trackhacs): "Download Discography" on an artist +pulled in tracks where the artist's name appeared in the title of +someone else's song. Two failure modes: + +1. Cross-artist tracks — compilations / appears_on albums brought in + tracks by unrelated artists. Fixed by `track_artist_matches`. +2. Remix / live / acoustic / instrumental versions never honored the + watchlist content-type filters for one-off discography downloads. + Fixed by `content_type_skip_reason`. + +These helpers live in ``core.metadata.discography_filters``. Tests pin +behavior at the function boundary so the wiring inside +``web_server.download_discography`` doesn't need an endpoint test to +catch a filter regression. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest + +from core.metadata.discography_filters import ( + content_type_skip_reason, + load_global_content_filter_settings, + track_already_owned, + track_artist_matches, +) + + +# --------------------------------------------------------------------------- +# track_artist_matches +# --------------------------------------------------------------------------- + + +class TestTrackArtistMatches: + def test_primary_artist_matches(self): + """When the requested artist is the track's primary artist, + match. This is the most common case — non-feature tracks on an + artist's own album.""" + assert track_artist_matches(['Drake', 'Future'], 'Drake') is True + + def test_featured_artist_matches(self): + """When the requested artist appears as a feature (anywhere in + the list, not just position 0), still match. Keeping feature + appearances is intentional — they're legit discography entries.""" + assert track_artist_matches(['Lil Wayne', 'Drake', 'Kanye West'], 'Drake') is True + + def test_unrelated_artist_drops(self): + """The bug case: a compilation track by an unrelated artist + that just mentions the requested artist in the title. The + artists list contains only the actual performer(s); filter + drops it.""" + assert track_artist_matches(['Random Artist'], 'Drake') is False + + def test_match_is_case_insensitive(self): + """Source data can be cased inconsistently across providers.""" + assert track_artist_matches(['drake'], 'Drake') is True + assert track_artist_matches(['DRAKE'], 'Drake') is True + assert track_artist_matches(['Drake'], 'drake') is True + + def test_match_handles_whitespace_padding(self): + """Trailing whitespace in either side mustn't break the match.""" + assert track_artist_matches([' Drake '], 'Drake') is True + assert track_artist_matches(['Drake'], ' Drake ') is True + + def test_empty_artists_list_drops(self): + """No artists on the track → can't be by anyone → drop.""" + assert track_artist_matches([], 'Drake') is False + assert track_artist_matches(None, 'Drake') is False + + def test_empty_requested_artist_keeps(self): + """Defensive: if the caller forgot to pass the requested artist, + don't drop every track — let the caller's other filters decide. + Better to keep too much than to silently drop everything.""" + assert track_artist_matches(['Drake'], '') is True + assert track_artist_matches(['Drake'], ' ') is True + assert track_artist_matches(['Drake'], None) is True + + def test_accepts_list_of_dicts_shape(self): + """Some upstreams pass `[{'name': 'Drake', 'id': '...'}]` + directly instead of the normalized list-of-strings. Helper + must handle both — easier than forcing a normalization step + at the call site.""" + assert track_artist_matches([{'name': 'Drake'}], 'Drake') is True + assert track_artist_matches([{'name': 'Random'}], 'Drake') is False + + def test_substring_does_not_match(self): + """A song by "Drake & Future" should not match "Drake" via + substring — that's exactly the false-positive case the bug + report describes. Exact full-name match only.""" + assert track_artist_matches(['Drake & Future'], 'Drake') is False + assert track_artist_matches(['Drakeo the Ruler'], 'Drake') is False + + +# --------------------------------------------------------------------------- +# content_type_skip_reason +# --------------------------------------------------------------------------- + + +_ALL_OFF = { + 'include_live': False, + 'include_remixes': False, + 'include_acoustic': False, + 'include_instrumentals': False, +} + +_ALL_ON = { + 'include_live': True, + 'include_remixes': True, + 'include_acoustic': True, + 'include_instrumentals': True, +} + + +class TestContentTypeSkipReason: + def test_returns_none_for_plain_track(self): + """Default settings exclude all four content types, but a plain + original studio track shouldn't trigger any of them.""" + assert content_type_skip_reason('Hotline Bling', 'Views', _ALL_OFF) is None + + def test_remix_skipped_when_excluded(self): + """Default: remixes off → "(Remix)" track gets skipped with + reason 'remix'.""" + assert content_type_skip_reason('Hotline Bling (Remix)', 'Views', _ALL_OFF) == 'remix' + + def test_remix_kept_when_included(self): + """When the user opts in via include_remixes, the same track + passes through.""" + assert content_type_skip_reason('Hotline Bling (Remix)', 'Views', _ALL_ON) is None + + def test_live_skipped_when_excluded(self): + assert content_type_skip_reason('Hotline Bling (Live)', 'Views', _ALL_OFF) == 'live' + + def test_acoustic_skipped_when_excluded(self): + assert content_type_skip_reason('Hotline Bling (Acoustic)', 'Views', _ALL_OFF) == 'acoustic' + + def test_instrumental_skipped_when_excluded(self): + assert content_type_skip_reason('Hotline Bling (Instrumental)', 'Views', _ALL_OFF) == 'instrumental' + + def test_first_match_wins(self): + """If a track somehow matches multiple categories (e.g. a live + remix), it's reported under the first one checked. Order is + live → remix → acoustic → instrumental. Stable for telemetry + and for the user-facing skip-counter aggregation.""" + # "Live Remix" — both live and remix patterns fire. Live first. + reason = content_type_skip_reason('Hotline Bling (Live Remix)', 'Views', _ALL_OFF) + assert reason == 'live' + + def test_settings_missing_keys_default_to_exclude(self): + """Defensive: caller passes an empty dict / partial dict. + Missing keys treated as False (exclude) — same as the watchlist + scanner contract. A remix passed with `{}` still gets skipped.""" + assert content_type_skip_reason('Track (Remix)', 'Album', {}) == 'remix' + + +# --------------------------------------------------------------------------- +# load_global_content_filter_settings +# --------------------------------------------------------------------------- + + +class TestLoadGlobalSettings: + def test_reads_all_four_settings(self): + cfg = SimpleNamespace() + cfg.get = lambda key, default=None: { + 'watchlist.global_include_live': True, + 'watchlist.global_include_remixes': False, + 'watchlist.global_include_acoustic': True, + 'watchlist.global_include_instrumentals': False, + }.get(key, default) + result = load_global_content_filter_settings(cfg) + assert result == { + 'include_live': True, + 'include_remixes': False, + 'include_acoustic': True, + 'include_instrumentals': False, + } + + def test_defaults_all_false_when_config_manager_missing(self): + """No config_manager → all four default to False (exclude). + Same defaults the watchlist scanner uses for unconfigured artists.""" + result = load_global_content_filter_settings(None) + assert result == { + 'include_live': False, + 'include_remixes': False, + 'include_acoustic': False, + 'include_instrumentals': False, + } + + def test_config_get_raising_falls_back_to_defaults(self): + """Defensive: if `config_manager.get` raises (corrupted config, + backend offline, etc.), helper returns all-False defaults + rather than crashing the discography fetch.""" + cfg = SimpleNamespace() + def _boom(*_a, **_k): + raise RuntimeError('config backend exploded') + cfg.get = _boom + result = load_global_content_filter_settings(cfg) + assert result['include_live'] is False + assert result['include_remixes'] is False + + def test_setting_values_coerced_to_bool(self): + """Config can store as int / string — coerce defensively so + downstream callers can rely on the bool contract.""" + cfg = SimpleNamespace() + cfg.get = lambda key, default=None: { + 'watchlist.global_include_live': 1, # int truthy + 'watchlist.global_include_remixes': '', # empty string falsy + 'watchlist.global_include_acoustic': 'on', # string truthy + 'watchlist.global_include_instrumentals': 0, + }.get(key, default) + result = load_global_content_filter_settings(cfg) + assert result['include_live'] is True + assert result['include_remixes'] is False + assert result['include_acoustic'] is True + assert result['include_instrumentals'] is False + + +# --------------------------------------------------------------------------- +# track_already_owned +# --------------------------------------------------------------------------- + + +class _FakeDB: + """Minimal stub for the parts of MusicDatabase the helper touches.""" + + def __init__(self, response): + self._response = response + self.calls = [] + + def check_track_exists(self, title, artist, **kwargs): + self.calls.append({'title': title, 'artist': artist, **kwargs}) + if isinstance(self._response, Exception): + raise self._response + return self._response + + +class TestTrackAlreadyOwned: + def test_returns_true_when_match_clears_threshold(self): + """Skowl's case: the second discography click finds the track + already in library at confidence ≥ 0.7 → skip.""" + db = _FakeDB((object(), 0.85)) + assert track_already_owned( + db, 'Hotline Bling', 'Drake', 'Views', 'plex', + ) is True + + def test_returns_false_when_no_match(self): + """Library doesn't have it → return False so the caller queues + the track. (None track returned with confidence 0.0.)""" + db = _FakeDB((None, 0.0)) + assert track_already_owned( + db, 'New Song', 'Drake', 'New Album', 'plex', + ) is False + + def test_returns_false_when_match_below_threshold(self): + """A weak fuzzy match shouldn't count — better to over-queue + than to silently drop a real missing track. Mirrors the + backfill repair job's `if db_track and confidence >= 0.7` guard.""" + db = _FakeDB((object(), 0.5)) # below default 0.7 + assert track_already_owned( + db, 'Sort Of Like Hotline Bling', 'Drake', 'Views', 'plex', + ) is False + + def test_passes_album_to_check(self): + """Album param is what enables album-aware matching for + multi-artist albums in `check_track_exists`. Pin it gets through.""" + db = _FakeDB((object(), 0.9)) + track_already_owned(db, 'Track', 'Artist', 'Album X', 'plex') + assert db.calls[0]['album'] == 'Album X' + + def test_passes_server_source_to_check(self): + """Active media server scopes the lookup so the skip check + only fires on tracks the user can actually see in their + library through their currently-active server.""" + db = _FakeDB((object(), 0.9)) + track_already_owned(db, 'Track', 'Artist', 'Album', 'navidrome') + assert db.calls[0]['server_source'] == 'navidrome' + + def test_empty_album_passed_as_none(self): + """Empty-string album becomes None so check_track_exists's + album-aware fallback doesn't try to match against ''.""" + db = _FakeDB((None, 0.0)) + track_already_owned(db, 'Track', 'Artist', '', 'plex') + assert db.calls[0]['album'] is None + + def test_missing_track_or_artist_returns_false_without_calling_db(self): + """Don't fire a DB call when we have nothing to match against — + defensive AND avoids polluting query logs with empty lookups.""" + db = _FakeDB((object(), 0.9)) + assert track_already_owned(db, '', 'Artist', 'Album', 'plex') is False + assert track_already_owned(db, 'Track', '', 'Album', 'plex') is False + assert track_already_owned(db, '', '', 'Album', 'plex') is False + assert db.calls == [], "DB must not be called when track or artist is empty" + + def test_db_exception_returns_false(self): + """If the DB call raises (lock contention, schema mismatch, + whatever), treat as 'not owned' and let the caller queue. + A redundant wishlist add is much cheaper to recover from + than a missed track.""" + db = _FakeDB(RuntimeError('db locked')) + assert track_already_owned(db, 'Track', 'Artist', 'Album', 'plex') is False + + def test_custom_confidence_threshold_honored(self): + """Caller can tighten or loosen the threshold. 0.95 means only + very-high-confidence matches count as owned.""" + db = _FakeDB((object(), 0.8)) + # Default threshold (0.7): match counts + assert track_already_owned(db, 'Track', 'Artist', 'Album', 'plex') is True + # Tighter threshold (0.95): same match doesn't count + assert track_already_owned( + db, 'Track', 'Artist', 'Album', 'plex', + confidence_threshold=0.95, + ) is False + + def test_none_server_source_passes_through(self): + """When the caller can't determine the active server, pass + None — `check_track_exists` falls back to a cross-server search.""" + db = _FakeDB((None, 0.0)) + track_already_owned(db, 'Track', 'Artist', 'Album', None) + assert db.calls[0]['server_source'] is None diff --git a/tests/metadata/test_multi_artist_tag_settings.py b/tests/metadata/test_multi_artist_tag_settings.py new file mode 100644 index 00000000..550e170d --- /dev/null +++ b/tests/metadata/test_multi_artist_tag_settings.py @@ -0,0 +1,470 @@ +"""Pin multi-artist tag-write settings (issue: 'Multi artists settings not working'). + +Three settings under `metadata_enhancement.tags`: + - `write_multi_artist` (bool) — write a separate multi-value tag + listing every artist (TXXX:Artists for ID3, "artists" key for + Vorbis). Picard convention. + - `artist_separator` (string, default ", ") — delimiter used to + join multiple artists into the single ARTIST/TPE1 string. + - `feat_in_title` (bool) — when true, ARTIST/TPE1 carries ONLY + the primary artist; featured artists get pulled out and + appended to the title as " (feat. X, Y)". + +Reporter (Netti93): all three were partially or completely +unimplemented. + - Bug 1: `_artists_list` field read by enrichment.py was never + populated by source.py → multi-value writes silently no-op'd. + - Bug 2: `artist_separator` referenced in UI but ZERO Python code + read it → always hardcoded ", ". + - Bug 3: `feat_in_title` referenced in UI but ZERO Python code + read it → no implementation at all. + +These tests pin the fixed `extract_source_metadata` behavior: + - `_artists_list` populated whenever search response has multiple artists + - `artist_separator` config drives the join character for ARTIST string + - `feat_in_title` pulls featured artists into title, leaves only + primary in ARTIST string + - Title-already-has-feat case isn't double-appended + - Single-artist case unaffected by either setting +""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + + +def _make_cfg(overrides=None): + """Stub config_manager. Defaults match the unset-config case + so each test can selectively override.""" + overrides = overrides or {} + defaults = { + "metadata_enhancement.enabled": True, + "metadata_enhancement.tags.write_multi_artist": False, + "metadata_enhancement.tags.feat_in_title": False, + "metadata_enhancement.tags.artist_separator": ", ", + } + full = {**defaults, **overrides} + + cfg = MagicMock() + cfg.get.side_effect = lambda key, default=None: full.get(key, default) + return cfg + + +def _build_context(artists_list): + """Minimal context dict matching what extract_source_metadata reads.""" + return { + "original_search_result": { + "title": "Sample Track", + "artists": [{"name": a} for a in artists_list], + }, + "source": "spotify", + } + + +def _call_extract(artists_list, cfg_overrides=None): + """Helper: patches config + calls extract_source_metadata, returns the + metadata dict. Avoids the broader source-specific embedding loop by + only using fields the multi-artist branch touches.""" + from core.metadata import source as src_module + + context = _build_context(artists_list) + artist_dict = {"name": artists_list[0] if artists_list else ""} + album_info = {"album_name": "Sample Album"} + + with patch.object(src_module, "get_config_manager", return_value=_make_cfg(cfg_overrides)): + return src_module.extract_source_metadata(context, artist_dict, album_info) + + +# --------------------------------------------------------------------------- +# Bug 1: `_artists_list` populated +# --------------------------------------------------------------------------- + + +class TestArtistsListPopulated: + def test_multiple_artists_populate_list(self): + """Reporter's first bug — `_artists_list` field was always + empty. Verify it now contains every artist from the search + response.""" + meta = _call_extract(["Eminem", "Dr. Dre", "50 Cent"]) + assert meta.get("_artists_list") == ["Eminem", "Dr. Dre", "50 Cent"] + + def test_single_artist_still_populates_list(self): + """Edge: even single-artist case populates the list (length 1). + Avoids special-casing downstream — `len(_artists_list) > 1` + check in enrichment.py is the gate.""" + meta = _call_extract(["Solo Artist"]) + assert meta.get("_artists_list") == ["Solo Artist"] + + def test_no_artists_falls_through(self): + """When search response has no artists list, falls through to + the single-artist branch — no `_artists_list` written.""" + from core.metadata import source as src_module + + context = { + "original_search_result": {"title": "T", "artists": None}, + "source": "spotify", + } + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()): + meta = src_module.extract_source_metadata(context, {"name": "X"}, {}) + assert "_artists_list" not in meta or meta.get("_artists_list") in (None, []) + + +# --------------------------------------------------------------------------- +# Bug 2: artist_separator drives ARTIST string +# --------------------------------------------------------------------------- + + +class TestArtistSeparator: + def test_default_separator_is_comma_space(self): + """Default preserves historical behavior — joining with ', ' + so users who haven't set the config see no behavior change.""" + meta = _call_extract(["A", "B", "C"]) + assert meta["artist"] == "A, B, C" + + def test_semicolon_separator(self): + """Reporter's exact case: artist_separator=';'. Picard convention.""" + meta = _call_extract(["A", "B", "C"], cfg_overrides={ + "metadata_enhancement.tags.artist_separator": ";", + }) + assert meta["artist"] == "A;B;C" + + def test_separator_with_space(self): + """Many users prefer '; ' (semi + space). Whatever string + the user puts in the config gets used verbatim — no trimming.""" + meta = _call_extract(["A", "B"], cfg_overrides={ + "metadata_enhancement.tags.artist_separator": "; ", + }) + assert meta["artist"] == "A; B" + + def test_separator_unused_for_single_artist(self): + """Single-artist case: separator irrelevant, ARTIST is just + the one name. No spurious trailing/leading separator.""" + meta = _call_extract(["Solo"], cfg_overrides={ + "metadata_enhancement.tags.artist_separator": ";", + }) + assert meta["artist"] == "Solo" + + +# --------------------------------------------------------------------------- +# Bug 3: feat_in_title — pull featured into title +# --------------------------------------------------------------------------- + + +class TestFeatInTitle: + def test_feat_in_title_pulls_featured_to_title(self): + """Reporter's third bug. With feat_in_title=true, ARTIST holds + only primary; title gets " (feat. ...)" appended for + all-but-first.""" + meta = _call_extract(["Eminem", "Dr. Dre", "50 Cent"], cfg_overrides={ + "metadata_enhancement.tags.feat_in_title": True, + }) + assert meta["artist"] == "Eminem" + assert "(feat. Dr. Dre, 50 Cent)" in meta["title"] + + def test_feat_in_title_off_uses_separator(self): + """When feat_in_title is off (default), all artists join the + ARTIST string per `artist_separator`. Title stays unchanged.""" + meta = _call_extract(["A", "B", "C"], cfg_overrides={ + "metadata_enhancement.tags.feat_in_title": False, + "metadata_enhancement.tags.artist_separator": " & ", + }) + assert meta["artist"] == "A & B & C" + assert "feat" not in meta["title"].lower() + + def test_feat_in_title_skips_when_only_one_artist(self): + """Single-artist case: feat_in_title is a no-op. ARTIST = the + single name, title untouched.""" + meta = _call_extract(["Solo"], cfg_overrides={ + "metadata_enhancement.tags.feat_in_title": True, + }) + assert meta["artist"] == "Solo" + assert "feat" not in meta["title"].lower() + + def test_feat_in_title_no_double_append_when_title_already_has_feat(self): + """Defensive: if the source title already includes 'feat.' or + '(ft.', don't append again. Common on remixes / collabs where + the platform stores the featured artist in the track name.""" + from core.metadata import source as src_module + + context = { + "original_search_result": { + "title": "Track (feat. Already Listed)", + "artists": [{"name": "Primary"}, {"name": "Featured"}], + }, + "source": "spotify", + } + cfg_overrides = {"metadata_enhancement.tags.feat_in_title": True} + with patch.object(src_module, "get_config_manager", return_value=_make_cfg(cfg_overrides)): + meta = src_module.extract_source_metadata(context, {"name": "Primary"}, {}) + + # Primary still pulled out of ARTIST... + assert meta["artist"] == "Primary" + # ...but title NOT double-appended (would be "(feat. X) (feat. Y)") + assert meta["title"].count("feat.") == 1 + + @pytest.mark.parametrize("source_title", [ + "Track (feat. X)", # standard parens + period + "Track (Feat. X)", # capitalized + "Track (FEAT X)", # all caps, no period + "Track (feat X)", # no period, parens + "Track (Featuring X)", # full word + "Track [feat. X]", # square brackets + "Track ft. X", # ft + period, no parens/brackets + "Track (ft X)", # ft no period, parens + "Track FT. X", # FT all caps + ]) + def test_double_append_guard_recognizes_feat_variants(self, source_title): + """Defensive: source platforms (spotify / tidal / deezer) use + wildly different title conventions for featured artists. Guard + must recognize all of them so we never double-append.""" + from core.metadata import source as src_module + + context = { + "original_search_result": { + "title": source_title, + "artists": [{"name": "Primary"}, {"name": "Featured"}], + }, + "source": "spotify", + } + cfg_overrides = {"metadata_enhancement.tags.feat_in_title": True} + with patch.object(src_module, "get_config_manager", return_value=_make_cfg(cfg_overrides)): + meta = src_module.extract_source_metadata(context, {"name": "Primary"}, {}) + + # Title left unchanged — no double-append for any variant + assert meta["title"] == source_title, ( + f"Variant {source_title!r} got double-appended → {meta['title']!r}" + ) + + def test_double_append_guard_does_NOT_falsely_match_substrings(self): + """Sanity: word-boundary regex must NOT match 'ft' or 'feat' + as part of bigger words like 'aftermath', 'shaft', 'feature'. + Otherwise titles containing those words would skip the + legitimate (feat. X) append.""" + from core.metadata import source as src_module + + context = { + "original_search_result": { + "title": "Aftermath", # contains 'ft' as substring + "artists": [{"name": "Primary"}, {"name": "Featured"}], + }, + "source": "spotify", + } + cfg_overrides = {"metadata_enhancement.tags.feat_in_title": True} + with patch.object(src_module, "get_config_manager", return_value=_make_cfg(cfg_overrides)): + meta = src_module.extract_source_metadata(context, {"name": "Primary"}, {}) + + # Should APPEND because 'ft' inside 'Aftermath' isn't a + # standalone "ft" feature marker + assert "(feat. Featured)" in meta["title"] + + +# --------------------------------------------------------------------------- +# Integration — settings combine correctly +# --------------------------------------------------------------------------- + + +class TestSettingsCombination: + def test_feat_in_title_overrides_separator_for_artist_string(self): + """When BOTH settings are on, feat_in_title wins for the + ARTIST string (primary only). Separator is irrelevant in + that branch but `_artists_list` still carries every artist + for the multi-value tag write.""" + meta = _call_extract(["A", "B", "C"], cfg_overrides={ + "metadata_enhancement.tags.feat_in_title": True, + "metadata_enhancement.tags.artist_separator": ";", + }) + assert meta["artist"] == "A" + assert "(feat. B, C)" in meta["title"] + # Multi-value list still complete — write_multi_artist would + # use this regardless of feat_in_title. + assert meta["_artists_list"] == ["A", "B", "C"] + + def test_all_three_off_default_behavior_preserved(self): + """Sanity: unset config → joined ARTIST, no title change, + list still populated. Picks up no behavior change for users + who haven't touched the settings.""" + meta = _call_extract(["A", "B"]) + assert meta["artist"] == "A, B" + assert meta["title"] == "Sample Track" + assert meta["_artists_list"] == ["A", "B"] + + +# --------------------------------------------------------------------------- +# Deezer-specific: upgrade single-artist search results via /track/ +# --------------------------------------------------------------------------- +# +# Deezer's `/search` endpoint only returns the primary artist for each +# track. The full contributors array (feat., remix collaborators, +# producers credited as artists) lives on `/track/`. Reporter said +# their Retag flow worked because it called the per-track endpoint, but +# the initial enrichment used search-result data and missed the +# contributors. The fix: when source==deezer AND search returned only +# one artist AND a track_id is available, fetch the full track details +# and upgrade the artists list. + + +class TestDeezerContributorsUpgrade: + def test_upgrades_when_deezer_search_returns_single_artist(self): + """Reporter's exact case: Deezer track with multiple + contributors, search returns just the primary, /track/ + returns all 3. Upgrade path fetches the full set.""" + from core.metadata import source as src_module + + context = { + "original_search_result": { + "title": "Collab Track", + "artists": [{"name": "Primary"}], # Only one — search-response shape + }, + "source": "deezer", + } + # track_id resolved via original_search_result.id by get_import_source_ids + context["original_search_result"]["id"] = "12345" + + fake_deezer = SimpleNamespace(get_track_details=MagicMock(return_value={ + "id": "12345", + "name": "Collab Track", + "artists": ["Primary", "Featured1", "Featured2"], # Full contributors + })) + + cfg_overrides = {"metadata_enhancement.tags.artist_separator": "; "} + + with patch.object(src_module, "get_config_manager", return_value=_make_cfg(cfg_overrides)), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata(context, {"name": "Primary"}, {}) + + # Upgraded list reaches the multi-value tag + assert meta["_artists_list"] == ["Primary", "Featured1", "Featured2"] + # And the joined ARTIST string respects the separator + assert meta["artist"] == "Primary; Featured1; Featured2" + fake_deezer.get_track_details.assert_called_once_with("12345") + + def test_no_upgrade_when_search_already_returned_multiple(self): + """When search already has multiple artists, skip the upgrade — + no extra API call needed.""" + from core.metadata import source as src_module + + context = { + "original_search_result": { + "title": "T", + "artists": [{"name": "A"}, {"name": "B"}], # Already multi + }, + "source": "deezer", + } + # track_id resolved via original_search_result.id by get_import_source_ids + context["original_search_result"]["id"] = "12345" + + fake_deezer = SimpleNamespace(get_track_details=MagicMock()) + + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata(context, {"name": "A"}, {}) + + assert meta["_artists_list"] == ["A", "B"] + # No upgrade call — search already had what we needed + fake_deezer.get_track_details.assert_not_called() + + def test_no_upgrade_for_non_deezer_sources(self): + """Spotify/iTunes/Tidal already return multi-artist in search, + so the Deezer-specific upgrade path must NOT fire for them. + Otherwise we'd be making redundant API calls.""" + from core.metadata import source as src_module + + context = { + "original_search_result": { + "title": "T", + "artists": [{"name": "A"}], + }, + "source": "spotify", + "source_track_id": "12345", + } + + fake_deezer = SimpleNamespace(get_track_details=MagicMock()) + + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata(context, {"name": "A"}, {}) + + # Single artist preserved, no Deezer upgrade attempted + assert meta["_artists_list"] == ["A"] + fake_deezer.get_track_details.assert_not_called() + + def test_upgrade_failure_falls_through_to_search_result(self): + """Defensive: if /track/ fails (network error, deezer + client unavailable), fall through to the search-result list. + Don't lose the single-artist data we already had.""" + from core.metadata import source as src_module + + context = { + "original_search_result": { + "title": "T", + "artists": [{"name": "Primary"}], + }, + "source": "deezer", + } + # track_id resolved via original_search_result.id by get_import_source_ids + context["original_search_result"]["id"] = "12345" + + fake_deezer = SimpleNamespace(get_track_details=MagicMock( + side_effect=RuntimeError("network down"), + )) + + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata(context, {"name": "Primary"}, {}) + + # Search-result list preserved + assert meta["_artists_list"] == ["Primary"] + assert meta["artist"] == "Primary" + + def test_upgrade_returns_same_count_no_change(self): + """Edge: /track/ returns the same single artist (track + genuinely has one artist on Deezer too). Should preserve the + list without false-positive upgrade.""" + from core.metadata import source as src_module + + context = { + "original_search_result": { + "title": "T", + "artists": [{"name": "Solo"}], + }, + "source": "deezer", + } + # track_id resolved via original_search_result.id by get_import_source_ids + context["original_search_result"]["id"] = "12345" + + fake_deezer = SimpleNamespace(get_track_details=MagicMock(return_value={ + "id": "12345", + "artists": ["Solo"], # Same single artist confirmed + })) + + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata(context, {"name": "Solo"}, {}) + + assert meta["_artists_list"] == ["Solo"] + + def test_no_upgrade_when_no_track_id(self): + """Edge: source==deezer but no track_id. Can't call + /track/ without an id. Don't attempt the upgrade.""" + from core.metadata import source as src_module + + context = { + "original_search_result": { + "title": "T", + "artists": [{"name": "Primary"}], + }, + "source": "deezer", + "source_track_id": "", # Missing + } + + fake_deezer = SimpleNamespace(get_track_details=MagicMock()) + + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata(context, {"name": "Primary"}, {}) + + assert meta["_artists_list"] == ["Primary"] + fake_deezer.get_track_details.assert_not_called() diff --git a/tests/test_acoustid_scanner.py b/tests/test_acoustid_scanner.py index 6b8ee4ef..a6d83e12 100644 --- a/tests/test_acoustid_scanner.py +++ b/tests/test_acoustid_scanner.py @@ -54,8 +54,11 @@ def _make_context(rows): def test_load_db_tracks_skips_null_ids_and_normalizes_track_ids(): job = AcoustIDScannerJob() context = _make_context([ - (None, "Broken Track", "Artist", "/music/broken.flac", 1, "Album", None, None), - (42, "Good Track", "Artist", "/music/good.flac", 2, "Album", "album-thumb", "artist-thumb"), + # 10 columns: id, title, artist (COALESCE'd), file_path, track_number, + # album_title, album_thumb, artist_thumb, track_artist (raw, may be ''), + # album_artist. + (None, "Broken Track", "Artist", "/music/broken.flac", 1, "Album", None, None, "", "Artist"), + (42, "Good Track", "Artist", "/music/good.flac", 2, "Album", "album-thumb", "artist-thumb", "", "Artist"), ]) tracks = job._load_db_tracks(context) @@ -68,8 +71,11 @@ def test_load_db_tracks_skips_null_ids_and_normalizes_track_ids(): def test_scan_handles_mixed_track_id_types(monkeypatch): job = AcoustIDScannerJob() context = _make_context([ - (None, "Broken Track", "Artist", "/music/broken.flac", 1, "Album", None, None), - (42, "Good Track", "Artist", "/music/good.flac", 2, "Album", "album-thumb", "artist-thumb"), + # 10 columns: id, title, artist (COALESCE'd), file_path, track_number, + # album_title, album_thumb, artist_thumb, track_artist (raw, may be ''), + # album_artist. + (None, "Broken Track", "Artist", "/music/broken.flac", 1, "Album", None, None, "", "Artist"), + (42, "Good Track", "Artist", "/music/good.flac", 2, "Album", "album-thumb", "artist-thumb", "", "Artist"), ]) monkeypatch.setattr(job, "_resolve_path", lambda file_path, _context: file_path) @@ -360,3 +366,257 @@ def test_load_db_tracks_falls_back_when_track_artist_empty_string(): # Empty string in track_artist → NULLIF returns NULL → COALESCE # falls back to album artist assert tracks['t1']['artist'] == 'Album Artist' + + +# --------------------------------------------------------------------------- +# File-tag fallback for legacy compilation tracks — Skowl Discord follow-up +# --------------------------------------------------------------------------- +# +# Skowl reported that the AcoustID Scanner was STILL flagging his +# compilation tracks even after the COALESCE(track_artist, album_artist) +# fix shipped. Cause: his tracks were downloaded BEFORE the +# `tracks.track_artist` column existed, so for those rows +# `track_artist IS NULL` and COALESCE falls back to the ALBUM artist +# (the curator) — same wrong-comparison the prior fix was supposed to +# eliminate. +# +# The audio file's ARTIST tag is ground truth for what's on disk: +# Tidal/Spotify/Deezer all write the per-track artist into the file's +# tag at download time, regardless of the SoulSync DB schema. Reading +# it during the scan closes the gap without requiring a DB backfill +# of the legacy rows. These tests pin: +# - File ARTIST tag trumps DB-resolved expected artist when present +# (Skowl's exact case: file says 'Eclypse', DB says 'Andromedik', +# AcoustID returns 'Eclypse' → no finding) +# - Missing file tag falls through to DB value (preserves +# pre-fix behavior for tracks without proper file tags) +# - mutagen failure is swallowed → falls through to DB +# - File tag matches DB → no behavioral change + + +def test_scanner_uses_file_tag_artist_over_db_for_legacy_compilation(monkeypatch): + """Skowl's exact case verbatim: + + DB row: artist_id → 'Andromedik' (album artist), track_artist=NULL + File tag: ARTIST='Eclypse' (Tidal-tagged correctly) + AcoustID: artist='Eclypse' + Pre-fix: expected='Andromedik' vs actual='Eclypse' → flag + Post-fix: file tag trumps DB → expected='Eclypse' → no flag + """ + job = AcoustIDScannerJob() + captured_findings = [] + context = _make_finding_capturing_context( + track_row=("city-lights", "City Lights", "Andromedik", + "/music/eclypse-city-lights.opus", 1, + "High Tea Music: Vol 1", None, None), + captured=captured_findings, + ) + + fake_acoustid = SimpleNamespace( + fingerprint_and_lookup=lambda fpath: { + 'best_score': 0.99, + 'recordings': [{ + 'title': 'City Lights', + 'artist': 'Eclypse', + }], + }, + ) + + # Patch read_file_tags to return Tidal's correct per-track artist. + # The scanner imports lazily inside _scan_file so we patch the + # source module's symbol. + monkeypatch.setattr( + 'core.tag_writer.read_file_tags', + lambda fpath: {'artist': 'Eclypse', 'title': 'City Lights'}, + ) + + result = JobResultStub() + job._scan_file( + '/music/eclypse-city-lights.opus', + 'city-lights', + {'title': 'City Lights', 'artist': 'Andromedik'}, # DB-resolved expected + fake_acoustid, + context, + result, + fp_threshold=0.85, + title_threshold=0.85, + artist_threshold=0.6, + ) + + assert captured_findings == [], ( + f"Expected no finding (file tag matches AcoustID); got {captured_findings}" + ) + + +def test_scanner_falls_back_to_db_when_file_tag_missing(monkeypatch): + """Defensive: file has no ARTIST tag (rare but possible for + non-standard formats / damaged files). MUST fall back to DB + expected value. Otherwise the fix would BREAK the existing + 'flag genuine mismatches' contract for files without tags.""" + job = AcoustIDScannerJob() + captured_findings = [] + context = _make_finding_capturing_context( + track_row=("99", "Some Track", "Foreigner", + "/music/track.flac", 1, "Album", None, None), + captured=captured_findings, + ) + + fake_acoustid = SimpleNamespace( + fingerprint_and_lookup=lambda fpath: { + 'best_score': 0.99, + 'recordings': [{ + 'title': 'Some Track', + 'artist': 'Different Band', + }], + }, + ) + + # File has no ARTIST tag (read_file_tags returns None for the field) + monkeypatch.setattr( + 'core.tag_writer.read_file_tags', + lambda fpath: {'artist': None}, + ) + + result = JobResultStub() + job._scan_file( + '/music/track.flac', + '99', + {'title': 'Some Track', 'artist': 'Foreigner'}, + fake_acoustid, + context, + result, + fp_threshold=0.85, + title_threshold=0.85, + artist_threshold=0.6, + ) + + # Should still flag — file tag was missing, fell back to DB + # ('Foreigner') vs AcoustID ('Different Band') mismatch + assert len(captured_findings) == 1, ( + f"Expected finding (file tag missing → DB fallback → genuine mismatch); got {captured_findings}" + ) + + +def test_scanner_swallows_file_tag_read_exception(monkeypatch): + """Defensive: mutagen errors mid-read shouldn't crash the scan + — must log + fall back to DB value gracefully.""" + job = AcoustIDScannerJob() + captured_findings = [] + context = _make_finding_capturing_context( + track_row=("99", "Track", "RealArtist", + "/music/corrupted.mp3", 1, "Album", None, None), + captured=captured_findings, + ) + + fake_acoustid = SimpleNamespace( + fingerprint_and_lookup=lambda fpath: { + 'best_score': 0.99, + 'recordings': [{'title': 'Track', 'artist': 'RealArtist'}], + }, + ) + + def boom(fpath): + raise RuntimeError("mutagen exploded on corrupted file") + + monkeypatch.setattr('core.tag_writer.read_file_tags', boom) + + result = JobResultStub() + job._scan_file( + '/music/corrupted.mp3', + '99', + {'title': 'Track', 'artist': 'RealArtist'}, + fake_acoustid, + context, + result, + fp_threshold=0.85, + title_threshold=0.85, + artist_threshold=0.6, + ) + + # No finding — DB matches AcoustID after the fallback + assert captured_findings == [] + + +def test_scanner_trusts_curated_db_track_artist_over_stale_file_tag(monkeypatch): + """The flip side of Skowl's case — user manually corrected + `track_artist` in the DB via the enhanced library view but + didn't re-tag the file. Pre-refactor 'file tag always wins' + would flag this as a false positive (file says wrong, DB says + right, AcoustID matches DB). Post-refactor: DB track_artist + is the curated source of truth when populated → file tag is + only consulted when DB is empty. No spurious flag. + + This is why `_load_db_tracks` surfaces `track_artist` as a + separate field instead of just the COALESCE'd `artist`: + `_scan_file` needs to distinguish 'DB has a curated value' + from 'DB fell back to album artist'.""" + job = AcoustIDScannerJob() + captured_findings = [] + context = _make_finding_capturing_context( + track_row=("99", "Track", "AlbumArtist", + "/music/track.flac", 1, "Album", None, None), + captured=captured_findings, + ) + + fake_acoustid = SimpleNamespace( + fingerprint_and_lookup=lambda fpath: { + 'best_score': 0.99, + 'recordings': [{'title': 'Track', 'artist': 'Eclypse'}], + }, + ) + + # File has wrong tag (stale — user edited DB but didn't re-tag), + # DB has correct value, AcoustID matches DB. + monkeypatch.setattr( + 'core.tag_writer.read_file_tags', + lambda fpath: {'artist': 'WrongStaleTag'}, + ) + + result = JobResultStub() + job._scan_file( + '/music/track.flac', '99', + # Simulates the post-refactor _load_db_tracks output: + # track_artist populated (curated) takes priority over file tag. + {'title': 'Track', 'artist': 'Eclypse', + 'track_artist': 'Eclypse', 'album_artist': 'AlbumArtist'}, + fake_acoustid, context, result, + fp_threshold=0.85, title_threshold=0.85, artist_threshold=0.6, + ) + + assert captured_findings == [], ( + f"DB curated value must trump stale file tag; got {captured_findings}" + ) + + +def test_scanner_file_tag_matches_db_no_behavioral_change(monkeypatch): + """Sanity: when file tag and DB agree, behavior is identical to + the pre-fix path. No double-counting, no spurious findings.""" + job = AcoustIDScannerJob() + captured_findings = [] + context = _make_finding_capturing_context( + track_row=("99", "Track", "RealArtist", + "/music/track.flac", 1, "Album", None, None), + captured=captured_findings, + ) + + fake_acoustid = SimpleNamespace( + fingerprint_and_lookup=lambda fpath: { + 'best_score': 0.99, + 'recordings': [{'title': 'Track', 'artist': 'RealArtist'}], + }, + ) + + monkeypatch.setattr( + 'core.tag_writer.read_file_tags', + lambda fpath: {'artist': 'RealArtist'}, + ) + + result = JobResultStub() + job._scan_file( + '/music/track.flac', '99', + {'title': 'Track', 'artist': 'RealArtist'}, + fake_acoustid, context, result, + fp_threshold=0.85, title_threshold=0.85, artist_threshold=0.6, + ) + + assert captured_findings == [] diff --git a/tests/test_audiodb_worker_stuck_track.py b/tests/test_audiodb_worker_stuck_track.py new file mode 100644 index 00000000..87dc68a1 --- /dev/null +++ b/tests/test_audiodb_worker_stuck_track.py @@ -0,0 +1,300 @@ +"""Pin AudioDB worker doesn't infinite-loop on direct-ID-lookup +failures. + +Issue #553: when an entity already has `audiodb_id` populated +(from manual match or earlier scan) but `audiodb_match_status` is +NULL, the worker tries a direct ID lookup. If that lookup fails +(returns None on timeout — AudioDB's `track.php` endpoint is slow +and 10s timeouts are common), the prior code returned WITHOUT +marking status. Result: row stayed in NULL state, queue picked it +up next tick, retried, timed out, returned again — infinite loop. +User saw constant requests with no progress. + +The fix: + - Mark status='error' so the queue's NULL-status filter stops + picking the row on every tick + - Add 'error' to the retry-after-cutoff queries (priorities 4-6) + so transient AudioDB outages still recover automatically after + `retry_days` + - Preserve the existing `audiodb_id` (don't overwrite it via + name-search fallback — original "preserve manual match" intent) + +These tests pin: + - Direct-lookup-returns-None marks status='error' (no infinite loop) + - Direct-lookup-raises-exception marks status='error' + - Direct-lookup-success preserves existing match-success path + - 'error' status is included in retry-cutoff queue so eventual + recovery happens +""" + +from __future__ import annotations + +import sqlite3 +from datetime import datetime, timedelta +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from core.audiodb_worker import AudioDBWorker + + +def _make_real_db_with_audiodb_columns(tmp_path): + """Build a minimal SQLite DB with the artist/album/track schema + the worker needs. Real SQLite (not mocks) so the SQL queries + actually exercise the column names + retry-cutoff logic.""" + db_path = tmp_path / "audiodb_test.db" + conn = sqlite3.connect(str(db_path)) + conn.executescript(""" + CREATE TABLE artists ( + id INTEGER PRIMARY KEY, + name TEXT, + audiodb_id TEXT, + audiodb_match_status TEXT, + audiodb_last_attempted DATETIME, + updated_at DATETIME + ); + CREATE TABLE albums ( + id INTEGER PRIMARY KEY, + title TEXT, + artist_id INTEGER, + audiodb_id TEXT, + audiodb_match_status TEXT, + audiodb_last_attempted DATETIME, + updated_at DATETIME + ); + CREATE TABLE tracks ( + id INTEGER PRIMARY KEY, + title TEXT, + artist_id INTEGER, + audiodb_id TEXT, + audiodb_match_status TEXT, + audiodb_last_attempted DATETIME, + updated_at DATETIME + ); + """) + conn.commit() + conn.close() + + class _RealDB: + def _get_connection(self): + return sqlite3.connect(str(db_path)) + + return _RealDB(), db_path + + +def _make_worker(db, fake_client): + """Build a worker with a real DB + mocked AudioDB client. + Skip __init__ side effects (config load, thread start).""" + worker = AudioDBWorker.__new__(AudioDBWorker) + worker.db = db + worker.client = fake_client + worker.retry_days = 30 + worker.stats = {'matched': 0, 'not_found': 0, 'errors': 0, 'pending': 0} + worker.current_item = None + worker.running = False + worker.paused = False + worker.thread = None + return worker + + +# --------------------------------------------------------------------------- +# Issue #553 — direct-ID lookup failure no longer infinite-loops +# --------------------------------------------------------------------------- + + +class TestDirectLookupFailureMarksError: + def test_lookup_returns_none_marks_status_error(self, tmp_path): + """Reporter's exact scenario: track has audiodb_id set, + match_status is NULL. AudioDB times out → lookup returns None. + Pre-fix: return without marking → infinite loop next tick. + Post-fix: mark status='error' → queue stops re-picking.""" + db, db_path = _make_real_db_with_audiodb_columns(tmp_path) + + # Seed a track with audiodb_id populated, status NULL + with sqlite3.connect(str(db_path)) as seed_conn: + seed_conn.execute( + "INSERT INTO artists (id, name) VALUES (?, ?)", + (1, 'Test Artist'), + ) + seed_conn.execute( + "INSERT INTO tracks (id, title, artist_id, audiodb_id, audiodb_match_status) " + "VALUES (?, ?, ?, ?, ?)", + (32743988, 'Sweet Talk', 1, '12345', None), + ) + seed_conn.commit() + + # AudioDB client returns None on timeout (matches lookup_track_by_id behavior) + fake_client = SimpleNamespace( + lookup_artist_by_id=MagicMock(return_value=None), + lookup_album_by_id=MagicMock(return_value=None), + lookup_track_by_id=MagicMock(return_value=None), + ) + + worker = _make_worker(db, fake_client) + item = { + 'type': 'track', + 'id': 32743988, + 'name': 'Sweet Talk', + 'artist': 'Test Artist', + 'artist_audiodb_id': None, + } + + worker._process_item(item) + + # Verify status was marked (no longer NULL → queue won't re-pick) + with sqlite3.connect(str(db_path)) as verify: + row = verify.execute( + "SELECT audiodb_match_status, audiodb_id, audiodb_last_attempted " + "FROM tracks WHERE id = ?", + (32743988,), + ).fetchone() + + assert row[0] == 'error', f"Expected status='error' to break loop; got {row[0]!r}" + # audiodb_id preserved (manual match not overwritten) + assert row[1] == '12345', f"audiodb_id must NOT be cleared; got {row[1]!r}" + # last_attempted set so retry-cutoff logic can re-pick later + assert row[2] is not None, "audiodb_last_attempted must be set for retry logic" + # Stats updated + assert worker.stats['errors'] == 1 + + def test_lookup_raises_exception_marks_status_error(self, tmp_path): + """Defensive: if the AudioDB client itself raises (not just + returns None) the same loop-protection must apply. Some + client paths re-raise on certain error classes.""" + db, db_path = _make_real_db_with_audiodb_columns(tmp_path) + + with sqlite3.connect(str(db_path)) as seed_conn: + seed_conn.execute( + "INSERT INTO artists (id, name) VALUES (?, ?)", + (1, 'X'), + ) + seed_conn.execute( + "INSERT INTO tracks (id, title, artist_id, audiodb_id, audiodb_match_status) " + "VALUES (?, ?, ?, ?, ?)", + (99, 'Y', 1, '67890', None), + ) + seed_conn.commit() + + fake_client = SimpleNamespace( + lookup_artist_by_id=MagicMock(side_effect=RuntimeError("boom")), + lookup_album_by_id=MagicMock(side_effect=RuntimeError("boom")), + lookup_track_by_id=MagicMock(side_effect=RuntimeError("read timeout")), + ) + + worker = _make_worker(db, fake_client) + item = {'type': 'track', 'id': 99, 'name': 'Y', 'artist': 'X', + 'artist_audiodb_id': None} + + worker._process_item(item) + + with sqlite3.connect(str(db_path)) as verify: + row = verify.execute( + "SELECT audiodb_match_status FROM tracks WHERE id = ?", + (99,), + ).fetchone() + + assert row[0] == 'error' + + def test_lookup_success_preserves_existing_path(self, tmp_path): + """Sanity: when direct lookup SUCCEEDS, the existing match- + success path runs (update + stats['matched'] += 1). Don't + regress the happy path.""" + db, db_path = _make_real_db_with_audiodb_columns(tmp_path) + + with sqlite3.connect(str(db_path)) as seed_conn: + seed_conn.execute("INSERT INTO artists (id, name) VALUES (?, ?)", (1, 'A')) + seed_conn.execute( + "INSERT INTO tracks (id, title, artist_id, audiodb_id) " + "VALUES (?, ?, ?, ?)", + (50, 'T', 1, '111'), + ) + seed_conn.commit() + + fake_client = SimpleNamespace( + lookup_artist_by_id=MagicMock(), + lookup_album_by_id=MagicMock(), + lookup_track_by_id=MagicMock(return_value={ + 'idTrack': '111', + 'strTrack': 'T', + 'idArtist': '999', + }), + ) + + worker = _make_worker(db, fake_client) + # Stub the per-entity update method so we don't need every column + worker._update_track = MagicMock() + worker._verify_artist_id = MagicMock(return_value=True) + + item = {'type': 'track', 'id': 50, 'name': 'T', 'artist': 'A', + 'artist_audiodb_id': None} + worker._process_item(item) + + worker._update_track.assert_called_once() + assert worker.stats['matched'] == 1 + assert worker.stats['errors'] == 0 + + +# --------------------------------------------------------------------------- +# Retry queue includes 'error' status — transient outages eventually recover +# --------------------------------------------------------------------------- + + +class TestErrorRetryAfterCutoff: + def test_error_track_picked_up_after_cutoff(self, tmp_path): + """After fix #553, rows marked 'error' get a 30-day retry + cutoff — same treatment as 'not_found'. Without this they'd + stay errored forever after a transient AudioDB outage.""" + db, db_path = _make_real_db_with_audiodb_columns(tmp_path) + + # Seed a track marked 'error' with last_attempted older than retry_days. + # Artist must be marked 'matched' too — otherwise priority 1 (NULL-status + # artists) wins over priority 6 (error/not_found track retry). + old_attempt = datetime.now() - timedelta(days=31) + with sqlite3.connect(str(db_path)) as seed_conn: + seed_conn.execute( + "INSERT INTO artists (id, name, audiodb_match_status) VALUES (?, ?, ?)", + (1, 'A', 'matched'), + ) + seed_conn.execute( + "INSERT INTO tracks (id, title, artist_id, audiodb_match_status, audiodb_last_attempted) " + "VALUES (?, ?, ?, ?, ?)", + (10, 'OldErrored', 1, 'error', old_attempt), + ) + seed_conn.commit() + + fake_client = SimpleNamespace() # not called for queue check + worker = _make_worker(db, fake_client) + + item = worker._get_next_item() + assert item is not None, "Expected error-status track past retry cutoff to be picked up" + assert item['type'] == 'track' + assert item['id'] == 10 + + def test_error_track_NOT_picked_within_cutoff(self, tmp_path): + """Sanity: rows marked 'error' but recently-attempted should + NOT be picked. Otherwise the retry-cutoff doesn't actually + rate-limit retries and we're back to the loop.""" + db, db_path = _make_real_db_with_audiodb_columns(tmp_path) + + # Just-attempted (within cutoff). Artist marked matched + # so priority 1 doesn't intercept the queue check. + recent_attempt = datetime.now() - timedelta(days=1) + with sqlite3.connect(str(db_path)) as seed_conn: + seed_conn.execute( + "INSERT INTO artists (id, name, audiodb_match_status) VALUES (?, ?, ?)", + (1, 'A', 'matched'), + ) + seed_conn.execute( + "INSERT INTO tracks (id, title, artist_id, audiodb_match_status, audiodb_last_attempted) " + "VALUES (?, ?, ?, ?, ?)", + (20, 'RecentErrored', 1, 'error', recent_attempt), + ) + seed_conn.commit() + + worker = _make_worker(db, SimpleNamespace()) + item = worker._get_next_item() + assert item is None, ( + "Recently-attempted error rows must NOT be picked up — that's " + "the loop-prevention mechanism" + ) diff --git a/tests/test_debug_info_services.py b/tests/test_debug_info_services.py new file mode 100644 index 00000000..aaf69651 --- /dev/null +++ b/tests/test_debug_info_services.py @@ -0,0 +1,167 @@ +"""Pin the `info['services']` block returned by /api/debug-info. + +Pre-fix the `music_source` field always rendered as "unknown" because +the code read `_status_cache.get('spotify', {})` — but the cache only +ever holds 'media_server' and 'soulseek' keys, so the fallback always +fired. Same problem (silently) for `spotify_connected` and +`spotify_rate_limited`. Hydrabase was missing entirely. + +Fix routes those reads through the canonical accessors: +- `music_source` → `core.metadata.registry.get_primary_source` (which + already accounts for the auth-fallback chain — Spotify → Deezer when + unauthenticated) +- `spotify_connected` / `spotify_rate_limited` → + `core.metadata.status.get_spotify_status` +- `hydrabase_connected` → `core.metadata.registry.is_hydrabase_enabled` +- `youtube_available` → constant True (URL-based, no auth) +- `hifi_instance_count` → `db.get_hifi_instances` +- `always_available_metadata_sources` → static list of public-API + sources (Deezer / iTunes / MusicBrainz) +""" + +from __future__ import annotations + +from contextlib import contextmanager +from unittest.mock import patch + +import pytest + + +@pytest.fixture +def app_test_client(): + import web_server + web_server.app.config['TESTING'] = True + with web_server.app.test_client() as client: + yield client + + +@contextmanager +def _patched_endpoint( + primary_source='spotify', + spotify_status=None, + hydrabase_enabled=False, + primary_source_raises=False, + spotify_status_raises=False, + hydrabase_raises=False, +): + """Patch the three module-level lookups inside `core.debug_info` + and yield. Each can either return a fixed value or raise — the + `*_raises` flags select which.""" + if spotify_status is None: + spotify_status = {'connected': True, 'rate_limited': False} + + def _boom(*_a, **_k): + raise RuntimeError('forced failure for test') + + primary_patch = patch( + 'core.debug_info.get_primary_source', + side_effect=_boom if primary_source_raises else None, + return_value=None if primary_source_raises else primary_source, + ) + spotify_patch = patch( + 'core.debug_info.get_spotify_status', + side_effect=_boom if spotify_status_raises else None, + return_value=None if spotify_status_raises else spotify_status, + ) + hydrabase_patch = patch( + 'core.debug_info.is_hydrabase_enabled', + side_effect=_boom if hydrabase_raises else None, + return_value=None if hydrabase_raises else hydrabase_enabled, + ) + with primary_patch, spotify_patch, hydrabase_patch: + yield + + +def _services(client): + resp = client.get('/api/debug-info') + assert resp.status_code == 200 + return resp.get_json()['services'] + + +def test_music_source_uses_primary_source_not_status_cache(app_test_client): + """The bug: music_source always read 'unknown' because it pulled + from a non-existent 'spotify' key in `_status_cache`. Fix routes + it through `get_primary_source` which is the actual authority.""" + with _patched_endpoint(primary_source='tidal'): + services = _services(app_test_client) + assert services['music_source'] == 'tidal' + + +def test_music_source_falls_back_to_unknown_when_lookup_raises(app_test_client): + """Defensive: if `get_primary_source` itself blows up, the field + still renders as 'unknown' rather than crashing the whole endpoint.""" + with _patched_endpoint(primary_source_raises=True): + services = _services(app_test_client) + assert services['music_source'] == 'unknown' + + +def test_spotify_connected_uses_get_spotify_status(app_test_client): + """`spotify_connected` was reading `_status_cache.get('spotify', {})`, + which never had the key. Routed through `get_spotify_status` now.""" + with _patched_endpoint(spotify_status={'connected': True, 'rate_limited': False}): + services = _services(app_test_client) + assert services['spotify_connected'] is True + + +def test_spotify_rate_limited_uses_get_spotify_status(app_test_client): + with _patched_endpoint(spotify_status={'connected': True, 'rate_limited': True}): + services = _services(app_test_client) + assert services['spotify_rate_limited'] is True + + +def test_spotify_status_lookup_failure_does_not_break_endpoint(app_test_client): + """`get_spotify_status` raises → both spotify_* fields default to + False rather than 500'ing the whole debug dump.""" + with _patched_endpoint(spotify_status_raises=True): + services = _services(app_test_client) + assert services['spotify_connected'] is False + assert services['spotify_rate_limited'] is False + + +def test_hydrabase_connected_present(app_test_client): + """Hydrabase status was never surfaced in debug info even though + it's an active metadata source. Now reported.""" + with _patched_endpoint(hydrabase_enabled=True): + services = _services(app_test_client) + assert services['hydrabase_connected'] is True + + +def test_hydrabase_disconnected_when_disabled(app_test_client): + with _patched_endpoint(hydrabase_enabled=False): + services = _services(app_test_client) + assert services['hydrabase_connected'] is False + + +def test_hydrabase_lookup_failure_defaults_false(app_test_client): + with _patched_endpoint(hydrabase_raises=True): + services = _services(app_test_client) + assert services['hydrabase_connected'] is False + + +def test_youtube_available_always_true(app_test_client): + """YouTube is URL-based via yt-dlp, no auth, always available. + Surfaced so the dump documents it as a download source.""" + with _patched_endpoint(): + services = _services(app_test_client) + assert services['youtube_available'] is True + + +def test_always_available_metadata_sources_listed(app_test_client): + """Public-API metadata sources (no auth, no per-user state) listed + so the debug dump reflects the full metadata surface.""" + with _patched_endpoint(): + services = _services(app_test_client) + available = services['always_available_metadata_sources'] + assert 'deezer' in available + assert 'itunes' in available + assert 'musicbrainz' in available + + +def test_hifi_instance_count_present(app_test_client): + """HiFi instance count exposed because each instance is a separate + endpoint with its own auth state — single connected/disconnected + bool wouldn't capture the actual config.""" + with _patched_endpoint(): + services = _services(app_test_client) + assert 'hifi_instance_count' in services + assert isinstance(services['hifi_instance_count'], int) diff --git a/tests/test_repair_worker_album_fill.py b/tests/test_repair_worker_album_fill.py index e8ebc23c..57a44bcd 100644 --- a/tests/test_repair_worker_album_fill.py +++ b/tests/test_repair_worker_album_fill.py @@ -62,6 +62,7 @@ def test_perform_album_fill_copy_branch_generates_track_id(tmp_path, monkeypatch duration INTEGER, file_path TEXT, bitrate INTEGER, + server_source TEXT, created_at TEXT, updated_at TEXT ) @@ -69,10 +70,10 @@ def test_perform_album_fill_copy_branch_generates_track_id(tmp_path, monkeypatch ) conn.execute( """ - INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration, file_path, bitrate, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) + INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration, file_path, bitrate, server_source, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP) """, - ("target-track-1", "target-album", "target-artist", "Existing Track", 1, 180000, str(src_path), 320), + ("target-track-1", "target-album", "target-artist", "Existing Track", 1, 180000, str(src_path), 320, "navidrome"), ) conn.commit() @@ -89,6 +90,7 @@ def test_perform_album_fill_copy_branch_generates_track_id(tmp_path, monkeypatch duration=180000, file_path=str(src_path), bitrate=320, + server_source="soulsync", ), SimpleNamespace( id="source-track-2", @@ -97,6 +99,7 @@ def test_perform_album_fill_copy_branch_generates_track_id(tmp_path, monkeypatch duration=181000, file_path=str(src_path), bitrate=320, + server_source="soulsync", ), ] @@ -120,6 +123,7 @@ def test_perform_album_fill_copy_branch_generates_track_id(tmp_path, monkeypatch duration=180000, file_path=str(src_path), bitrate=320, + server_source="soulsync", ), album_id="target-album", album_title="Target Album", @@ -137,7 +141,7 @@ def test_perform_album_fill_copy_branch_generates_track_id(tmp_path, monkeypatch with sqlite3.connect(db_path) as verify_conn: row = verify_conn.execute( - "SELECT id, title, file_path FROM tracks WHERE title = ?", + "SELECT id, title, file_path, server_source FROM tracks WHERE title = ?", ("New Track",), ).fetchone() assert row is not None @@ -145,4 +149,5 @@ def test_perform_album_fill_copy_branch_generates_track_id(tmp_path, monkeypatch assert row[0].startswith("album_fill_source-track-1_deadbeef") assert row[1] == "New Track" assert Path(row[2]).exists() + assert row[3] == "navidrome" assert verify_conn.execute("SELECT COUNT(*) FROM tracks WHERE id IS NULL").fetchone()[0] == 0 diff --git a/tests/test_repair_worker_unresolvable_folder_error.py b/tests/test_repair_worker_unresolvable_folder_error.py new file mode 100644 index 00000000..50de6a07 --- /dev/null +++ b/tests/test_repair_worker_unresolvable_folder_error.py @@ -0,0 +1,184 @@ +"""Pin the diagnostic error string from +``RepairWorker._build_unresolvable_album_folder_error``. + +GitHub issue #558 (gabistek, Navidrome on Docker / Arch host): the +Album Completeness Auto-Fill button surfaced a flat "Could not +determine album folder from existing tracks" error with no diagnostic. +Reporter is on Navidrome, which (unlike Plex) has no API that exposes +filesystem library paths — so the resolver returns None whenever the +DB-recorded path doesn't already exist as-is in SoulSync's container +view AND the user hasn't manually configured Settings → Library → +Music Paths. + +The fix replaces the flat string with a multi-part diagnostic naming +the active media server, showing one sample DB path, listing the base +directories the resolver actually probed, and pointing the user at the +config that would unblock them. These tests pin each part so future +copy edits don't accidentally drop the actionable hint or the sample +path. +""" + +from __future__ import annotations + +import sys +import types +from types import SimpleNamespace + + +# ── Stub modules that the import of core.repair_worker pulls in ── +if "spotipy" not in sys.modules: + spotipy = types.ModuleType("spotipy") + + class _DummySpotify: + def __init__(self, *args, **kwargs): + pass + + oauth2 = types.ModuleType("spotipy.oauth2") + + class _DummyOAuth: + def __init__(self, *args, **kwargs): + pass + + spotipy.Spotify = _DummySpotify + oauth2.SpotifyOAuth = _DummyOAuth + oauth2.SpotifyClientCredentials = _DummyOAuth + spotipy.oauth2 = oauth2 + sys.modules["spotipy"] = spotipy + sys.modules["spotipy.oauth2"] = oauth2 + +if "config.settings" not in sys.modules: + config_pkg = types.ModuleType("config") + settings_mod = types.ModuleType("config.settings") + + class _DummyConfigManager: + def get(self, key, default=None): + return default + + def get_active_media_server(self): + return "plex" + + settings_mod.config_manager = _DummyConfigManager() + config_pkg.settings = settings_mod + sys.modules["config"] = config_pkg + sys.modules["config.settings"] = settings_mod + + +from core.library.path_resolver import ResolveAttempt +from core.repair_worker import RepairWorker + + +def _make_worker(active_server="plex"): + """Bare RepairWorker with a config_manager that reports the given + active media server. We never run the full job — just exercise the + diagnostic builder.""" + worker = RepairWorker(database=SimpleNamespace()) + cfg = SimpleNamespace() + cfg.get_active_media_server = lambda: active_server + cfg.get = lambda key, default=None: default + worker._config_manager = cfg + return worker + + +def test_error_names_active_media_server(): + """User needs to know which server's path conventions are at play + so they can set the right mount in Settings.""" + worker = _make_worker(active_server="navidrome") + msg = worker._build_unresolvable_album_folder_error( + ResolveAttempt(base_dirs_tried=["/app/Transfer"]), + "/music/Artist/Album/track.flac", + ) + assert "navidrome" in msg.lower(), ( + f"Active server name must appear in error; got: {msg}" + ) + + +def test_error_includes_sample_db_path(): + """One concrete path lets the user see what their media server + is reporting — usually enough to reverse-engineer the right mount.""" + worker = _make_worker() + sample = "/music/Kendrick Lamar/Mr. Morale/01 - United in Grief.flac" + msg = worker._build_unresolvable_album_folder_error( + ResolveAttempt(base_dirs_tried=["/app/Transfer"]), + sample, + ) + assert sample in msg, ( + f"Sample DB path must appear verbatim in error; got: {msg}" + ) + + +def test_error_lists_base_dirs_tried(): + """User needs to know what the resolver probed — otherwise they + can't tell whether to add a new mount or whether the existing one + just doesn't match the recorded path.""" + worker = _make_worker() + attempt = ResolveAttempt( + base_dirs_tried=["/app/Transfer", "/downloads", "/library"], + ) + msg = worker._build_unresolvable_album_folder_error(attempt, "/music/x.flac") + for base in attempt.base_dirs_tried: + assert base in msg, f"Probed base dir {base!r} missing from error: {msg}" + + +def test_error_calls_out_no_base_dirs_when_empty(): + """When the resolver had nothing to probe, that's a different + failure mode than "tried 3 dirs and failed" — the user needs + different action. Pin that the message distinguishes them.""" + worker = _make_worker() + msg = worker._build_unresolvable_album_folder_error( + ResolveAttempt(base_dirs_tried=[]), + "/music/x.flac", + ) + assert "no base director" in msg.lower(), ( + f"Empty-base-dirs case must surface 'no base directories'; got: {msg}" + ) + + +def test_error_always_includes_settings_hint(): + """The actionable fix line must always appear regardless of which + failure mode fired. This is the part the user needs to act on.""" + worker = _make_worker() + for attempt in ( + ResolveAttempt(base_dirs_tried=[]), + ResolveAttempt(base_dirs_tried=["/app/Transfer"]), + None, + ): + msg = worker._build_unresolvable_album_folder_error(attempt, "/music/x.flac") + assert "Settings" in msg, f"Settings hint missing for attempt={attempt}; got: {msg}" + assert "Music Paths" in msg, f"Music Paths hint missing for attempt={attempt}; got: {msg}" + + +def test_error_handles_none_attempt_defensively(): + """If for some reason no ResolveAttempt is collected (e.g. zero + existing tracks loop never ran), the helper must not crash. It + can omit the probe-detail line but must still emit the actionable + Settings hint.""" + worker = _make_worker() + msg = worker._build_unresolvable_album_folder_error(None, "/music/x.flac") + assert "Settings" in msg, f"None attempt must still emit Settings hint; got: {msg}" + assert "/music/x.flac" in msg + + +def test_error_handles_missing_sample_path(): + """If we couldn't sample a DB path (e.g. all entries had None + file_path), the path line is omitted but the rest of the message + still renders.""" + worker = _make_worker() + msg = worker._build_unresolvable_album_folder_error( + ResolveAttempt(base_dirs_tried=["/app/Transfer"]), + None, + ) + assert "Settings" in msg + # No sample-path line means no "Example DB-recorded path" prefix + assert "Example DB-recorded path:" not in msg + + +def test_error_handles_missing_config_manager(): + """RepairWorker may be constructed without a config_manager; the + builder shouldn't crash and should fall back to 'unknown' for the + server name rather than blowing up.""" + worker = RepairWorker(database=SimpleNamespace()) + worker._config_manager = None + msg = worker._build_unresolvable_album_folder_error( + ResolveAttempt(base_dirs_tried=[]), "/music/x.flac", + ) + assert "unknown" in msg.lower() diff --git a/tests/test_server_playlist_append_mode.py b/tests/test_server_playlist_append_mode.py new file mode 100644 index 00000000..8624fb1a --- /dev/null +++ b/tests/test_server_playlist_append_mode.py @@ -0,0 +1,333 @@ +"""Pin server-playlist sync 'append' mode behavior. + +Discord report (CJFC, 2026-04-26): syncing a Spotify playlist to the +server overwrote anything the user had manually added to the server- +side playlist. The fix adds a per-sync mode toggle: + + - 'replace' (default, current behavior) — delete + recreate + - 'append' — keep existing tracks, only add new ones + +Each server client (Plex / Jellyfin / Navidrome) gets a new +`append_to_playlist(name, tracks)` method that: + - Falls back to `create_playlist` when the playlist doesn't exist yet + - Fetches existing track IDs and dedupes incoming tracks against them + - Uses the server's NATIVE append API (no delete-recreate) + +`sync_service.sync_playlist` accepts `sync_mode` and dispatches to +`append_to_playlist` when set to 'append'. Falls back to +`update_playlist` (replace semantics) when the client doesn't +implement append (e.g. SoulSync standalone has no playlist methods +at all). + +These tests pin: + - Per-server append: missing playlist → create_playlist delegation + - Per-server append: existing IDs filtered out (no double-adds) + - Per-server append: empty new-track set short-circuits without API call + - Per-server append: failure paths return False without raising + - sync_service dispatch: mode='append' calls append_to_playlist + - sync_service dispatch: mode='replace' calls update_playlist (default) + - sync_service dispatch: missing append_to_playlist method → falls back to update_playlist +""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + + +# --------------------------------------------------------------------------- +# Plex append_to_playlist +# --------------------------------------------------------------------------- + + +from core.plex_client import PlexClient + + +def _make_plex_client(): + client = PlexClient.__new__(PlexClient) + client.server = MagicMock() + client.music_library = MagicMock() + client._all_libraries_mode = False + client._connection_attempted = True + client._is_connecting = False + client._last_connection_check = 0 + client._connection_check_interval = 30 + return client + + +class TestPlexAppendToPlaylist: + def test_falls_back_to_create_when_playlist_missing(self): + """Reporter's playlist may not exist on the server yet (first + sync). Append mode should create it instead of erroring.""" + from plexapi.exceptions import NotFound + client = _make_plex_client() + client.server.playlist = MagicMock(side_effect=NotFound("not found")) + + new_tracks = [SimpleNamespace(ratingKey='100'), SimpleNamespace(ratingKey='101')] + + with patch.object(client, 'ensure_connection', return_value=True), \ + patch.object(client, 'create_playlist', return_value=True) as mock_create: + result = client.append_to_playlist("Test Playlist", new_tracks) + + assert result is True + mock_create.assert_called_once_with("Test Playlist", new_tracks) + + def test_filters_out_already_present_tracks(self): + """Reporter's exact case: server playlist has tracks A, B + already; sync brings A, B, C. Only C should be added. + Existing tracks must NOT be re-added (would create + duplicates).""" + client = _make_plex_client() + existing_playlist = MagicMock() + existing_playlist.items = MagicMock(return_value=[ + SimpleNamespace(ratingKey='100'), # track A + SimpleNamespace(ratingKey='101'), # track B + ]) + existing_playlist.addItems = MagicMock() + client.server.playlist = MagicMock(return_value=existing_playlist) + + incoming = [ + SimpleNamespace(ratingKey='100'), # already present + SimpleNamespace(ratingKey='101'), # already present + SimpleNamespace(ratingKey='102'), # NEW — only this should be added + ] + + with patch.object(client, 'ensure_connection', return_value=True): + result = client.append_to_playlist("Test Playlist", incoming) + + assert result is True + # Only the new track passed to addItems + called_with = existing_playlist.addItems.call_args[0][0] + assert len(called_with) == 1 + assert called_with[0].ratingKey == '102' + + def test_short_circuits_when_all_tracks_already_present(self): + """All incoming tracks already on the playlist → no API call, + return True (no-op success).""" + client = _make_plex_client() + existing_playlist = MagicMock() + existing_playlist.items = MagicMock(return_value=[ + SimpleNamespace(ratingKey='100'), + SimpleNamespace(ratingKey='101'), + ]) + existing_playlist.addItems = MagicMock() + client.server.playlist = MagicMock(return_value=existing_playlist) + + incoming = [SimpleNamespace(ratingKey='100'), SimpleNamespace(ratingKey='101')] + + with patch.object(client, 'ensure_connection', return_value=True): + result = client.append_to_playlist("Test Playlist", incoming) + + assert result is True + existing_playlist.addItems.assert_not_called() + + def test_returns_false_when_not_connected(self): + """Defensive: ensure_connection False → return False, no API + call. Caller treats as a normal failure.""" + client = _make_plex_client() + with patch.object(client, 'ensure_connection', return_value=False): + result = client.append_to_playlist("Test Playlist", [ + SimpleNamespace(ratingKey='100'), + ]) + assert result is False + + def test_swallows_exceptions_returns_false(self): + """Plex SDK errors mid-append shouldn't crash the sync — log + + return False so the caller can fall back.""" + client = _make_plex_client() + client.server.playlist = MagicMock(side_effect=RuntimeError("plex down")) + with patch.object(client, 'ensure_connection', return_value=True): + result = client.append_to_playlist("Test Playlist", [ + SimpleNamespace(ratingKey='100'), + ]) + assert result is False + + +# --------------------------------------------------------------------------- +# Jellyfin append_to_playlist +# --------------------------------------------------------------------------- + + +from core.jellyfin_client import JellyfinClient + + +def _make_jellyfin_client(): + client = JellyfinClient.__new__(JellyfinClient) + client.base_url = "http://jellyfin.local" + client.api_key = "fake-api-key" + client.user_id = "user-123" + return client + + +class TestJellyfinAppendToPlaylist: + def test_falls_back_to_create_when_playlist_missing(self): + client = _make_jellyfin_client() + new_tracks = [SimpleNamespace(id='item-100')] + with patch.object(client, 'ensure_connection', return_value=True), \ + patch.object(client, 'get_playlist_by_name', return_value=None), \ + patch.object(client, 'create_playlist', return_value=True) as mock_create: + result = client.append_to_playlist("Test", new_tracks) + assert result is True + mock_create.assert_called_once_with("Test", new_tracks) + + def test_filters_out_already_present_tracks(self): + """Reporter's exact case for Jellyfin — only new GUIDs go in.""" + client = _make_jellyfin_client() + existing_playlist = SimpleNamespace(id='pl-1') + existing_tracks = [ + SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000001'), + SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000002'), + ] + incoming = [ + SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000001'), # present + SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000003'), # NEW + ] + + captured_post_params = {} + + def fake_post(url, params=None, headers=None, timeout=None): + captured_post_params['url'] = url + captured_post_params['ids'] = params['Ids'] + return SimpleNamespace(status_code=204, text='') + + with patch.object(client, 'ensure_connection', return_value=True), \ + patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \ + patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \ + patch.object(client, '_is_valid_guid', return_value=True), \ + patch('core.jellyfin_client.requests.post', side_effect=fake_post): + result = client.append_to_playlist("Test", incoming) + + assert result is True + # Only the NEW track id should have been POSTed + assert captured_post_params['ids'] == 'aaaaaaaa-bbbb-cccc-dddd-000000000003' + + def test_short_circuits_when_no_new_tracks(self): + client = _make_jellyfin_client() + existing_playlist = SimpleNamespace(id='pl-1') + existing_tracks = [SimpleNamespace(id='guid-1')] + with patch.object(client, 'ensure_connection', return_value=True), \ + patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \ + patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \ + patch.object(client, '_is_valid_guid', return_value=True), \ + patch('core.jellyfin_client.requests.post') as mock_post: + result = client.append_to_playlist("Test", [SimpleNamespace(id='guid-1')]) + assert result is True + mock_post.assert_not_called() + + def test_returns_false_on_post_error(self): + client = _make_jellyfin_client() + existing_playlist = SimpleNamespace(id='pl-1') + with patch.object(client, 'ensure_connection', return_value=True), \ + patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \ + patch.object(client, 'get_playlist_tracks', return_value=[]), \ + patch.object(client, '_is_valid_guid', return_value=True), \ + patch('core.jellyfin_client.requests.post', + return_value=SimpleNamespace(status_code=500, text='server error')): + result = client.append_to_playlist("Test", [SimpleNamespace(id='new-guid')]) + assert result is False + + +# --------------------------------------------------------------------------- +# Navidrome append_to_playlist +# --------------------------------------------------------------------------- + + +from core.navidrome_client import NavidromeClient + + +def _make_navidrome_client(): + client = NavidromeClient.__new__(NavidromeClient) + client.base_url = "http://navidrome.local" + client.username = "user" + client.password = "pass" + return client + + +class TestNavidromeAppendToPlaylist: + def test_falls_back_to_create_when_playlist_missing(self): + client = _make_navidrome_client() + with patch.object(client, 'ensure_connection', return_value=True), \ + patch.object(client, 'get_playlists_by_name', return_value=[]), \ + patch.object(client, 'create_playlist', return_value=True) as mock_create: + result = client.append_to_playlist("Test", [SimpleNamespace(id='song-1')]) + assert result is True + mock_create.assert_called_once() + + def test_filters_out_already_present_tracks_and_calls_subsonic(self): + client = _make_navidrome_client() + existing_playlists = [SimpleNamespace(id='pl-1', title='Test')] + existing_tracks = [SimpleNamespace(id='100'), SimpleNamespace(id='101')] + incoming = [ + SimpleNamespace(id='100'), # present + SimpleNamespace(id='102'), # NEW + SimpleNamespace(id='103'), # NEW + ] + + captured = {} + + def fake_make_request(endpoint, params=None): + captured['endpoint'] = endpoint + captured['params'] = params + return {'status': 'ok'} + + with patch.object(client, 'ensure_connection', return_value=True), \ + patch.object(client, 'get_playlists_by_name', return_value=existing_playlists), \ + patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \ + patch.object(client, '_make_request', side_effect=fake_make_request): + result = client.append_to_playlist("Test", incoming) + + assert result is True + assert captured['endpoint'] == 'updatePlaylist' + assert captured['params']['playlistId'] == 'pl-1' + # Only NEW song IDs in songIdToAdd, not already-present ones + assert sorted(captured['params']['songIdToAdd']) == ['102', '103'] + + def test_short_circuits_when_no_new_tracks(self): + client = _make_navidrome_client() + existing_playlists = [SimpleNamespace(id='pl-1', title='Test')] + existing_tracks = [SimpleNamespace(id='100')] + with patch.object(client, 'ensure_connection', return_value=True), \ + patch.object(client, 'get_playlists_by_name', return_value=existing_playlists), \ + patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \ + patch.object(client, '_make_request') as mock_req: + result = client.append_to_playlist("Test", [SimpleNamespace(id='100')]) + assert result is True + mock_req.assert_not_called() + + def test_falls_back_when_subsonic_returns_failed(self): + client = _make_navidrome_client() + existing_playlists = [SimpleNamespace(id='pl-1', title='Test')] + with patch.object(client, 'ensure_connection', return_value=True), \ + patch.object(client, 'get_playlists_by_name', return_value=existing_playlists), \ + patch.object(client, 'get_playlist_tracks', return_value=[]), \ + patch.object(client, '_make_request', return_value=None): + # _make_request returns None when Subsonic returns 'failed' status + result = client.append_to_playlist("Test", [SimpleNamespace(id='new-1')]) + assert result is False + + +# --------------------------------------------------------------------------- +# Contract pinning — append_to_playlist is in KNOWN_PER_SERVER_METHODS +# --------------------------------------------------------------------------- + + +def test_append_to_playlist_listed_in_contract(): + """If a future refactor drops `append_to_playlist` from the + contract's KNOWN_PER_SERVER_METHODS list, the conformance test + won't catch it (those are advisory-only). This test is the + explicit pin that the method is part of the recognized + per-server playlist surface.""" + from core.media_server.contract import KNOWN_PER_SERVER_METHODS + assert 'append_to_playlist' in KNOWN_PER_SERVER_METHODS + + +def test_each_client_implements_append_to_playlist(): + """Pin: Plex / Jellyfin / Navidrome all have the method (at the + class level — instance state isn't required for this check). + SoulSync standalone is intentionally excluded — it has no + playlist methods at all per the contract notes.""" + assert hasattr(PlexClient, 'append_to_playlist') + assert hasattr(JellyfinClient, 'append_to_playlist') + assert hasattr(NavidromeClient, 'append_to_playlist') diff --git a/tests/test_soulseek_search_throttle.py b/tests/test_soulseek_search_throttle.py new file mode 100644 index 00000000..e476656e --- /dev/null +++ b/tests/test_soulseek_search_throttle.py @@ -0,0 +1,254 @@ +"""Pin `compute_search_wait_seconds` — the pure scheduler behind the +slskd search throttle. + +Reddit report (YeloMelo95, Bell Canada): ISP anti-abuse cuts the user's +WAN connection after a burst of slskd searches. The pre-fix throttle +was hardcoded to 35 searches per 220s sliding window, which allowed all +35 in rapid succession and only blocked once the cap was hit. That's +fine for soulseek-side bans but doesn't smooth bursts at the ISP layer. + +Fix lifts the cap + window to config and adds a new `min_delay_seconds` +knob. The pure helper takes the throttle inputs and returns how long to +sleep — easy to test independently of asyncio.sleep / the singleton +client / wall-clock time. +""" + +from __future__ import annotations + +import pytest + +from core.soulseek_client import compute_search_wait_seconds + + +# --------------------------------------------------------------------------- +# Defaults / no-throttle path +# --------------------------------------------------------------------------- + + +class TestNoThrottleNeeded: + def test_empty_state_returns_zero(self): + """First search ever → no timestamps, no last-search → no wait.""" + assert compute_search_wait_seconds( + timestamps=[], + last_search_at=0.0, + now=100.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=0, + ) == 0.0 + + def test_below_window_cap_returns_zero(self): + """When timestamps haven't filled the window cap and min-delay + is disabled, no wait. Preserves prior behavior for existing + users who don't tune the new knob.""" + assert compute_search_wait_seconds( + timestamps=[10.0, 20.0, 30.0], + last_search_at=30.0, + now=100.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=0, + ) == 0.0 + + def test_min_delay_zero_is_disabled(self): + """Explicit zero (the default) means no min-delay enforcement + even when the last search was a millisecond ago. Confirms + backwards compat — existing users see no new wait.""" + assert compute_search_wait_seconds( + timestamps=[], + last_search_at=99.99, + now=100.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=0, + ) == 0.0 + + +# --------------------------------------------------------------------------- +# Sliding-window cap (legacy behavior preserved) +# --------------------------------------------------------------------------- + + +class TestSlidingWindowCap: + def test_window_full_waits_for_oldest_to_age_out(self): + """35 timestamps in window → wait until oldest ages out. + Same semantics as the pre-fix hardcoded behavior.""" + timestamps = [10.0 + i for i in range(35)] # 10..44 + # now = 50, window = 220, oldest = 10 → ages out at 230 → wait 180 + wait = compute_search_wait_seconds( + timestamps=timestamps, + last_search_at=44.0, + now=50.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=0, + ) + assert wait == pytest.approx(180.0, abs=1e-9) + + def test_window_full_but_oldest_already_aged_out_returns_zero(self): + """If now is past oldest+window, the negative is clamped to 0 + (the caller is expected to prune timestamps before passing — + this is just defense-in-depth).""" + wait = compute_search_wait_seconds( + timestamps=[10.0] * 35, + last_search_at=10.0, + now=400.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=0, + ) + assert wait == 0.0 + + def test_custom_max_per_window_honored(self): + """User dials max down to 10 (paranoia mode for ISP anti-abuse). + Cap kicks in at 10, not 35.""" + timestamps = [10.0 + i for i in range(10)] + wait = compute_search_wait_seconds( + timestamps=timestamps, + last_search_at=19.0, + now=20.0, + max_per_window=10, + window_seconds=60, + min_delay_seconds=0, + ) + # oldest = 10, ages out at 70, now = 20 → wait 50 + assert wait == pytest.approx(50.0, abs=1e-9) + + def test_max_per_window_zero_disables_window_cap(self): + """Defensive: max=0 means no cap (don't divide by zero, don't + block forever). Min-delay still applies if set.""" + wait = compute_search_wait_seconds( + timestamps=[10.0] * 100, + last_search_at=50.0, + now=51.0, + max_per_window=0, + window_seconds=220, + min_delay_seconds=0, + ) + assert wait == 0.0 + + +# --------------------------------------------------------------------------- +# Min-delay between searches (the new knob — Bell Canada fix) +# --------------------------------------------------------------------------- + + +class TestMinDelayBetweenSearches: + def test_recent_last_search_blocks_for_remaining_delay(self): + """User sets min_delay=5s. Last search 2s ago → wait 3s. + Smooths the burst pattern that trips Bell's anti-abuse even + when the sliding window isn't full.""" + wait = compute_search_wait_seconds( + timestamps=[100.0, 102.0], + last_search_at=102.0, + now=104.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=5, + ) + assert wait == pytest.approx(3.0, abs=1e-9) + + def test_min_delay_already_elapsed_returns_zero(self): + """Last search 10s ago, min-delay 5s → already cleared, no wait.""" + wait = compute_search_wait_seconds( + timestamps=[100.0], + last_search_at=100.0, + now=110.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=5, + ) + assert wait == 0.0 + + def test_min_delay_skipped_on_very_first_search(self): + """`last_search_at == 0` means there's never been a search. + Don't gate the very first one — that would force an arbitrary + startup delay for no reason.""" + wait = compute_search_wait_seconds( + timestamps=[], + last_search_at=0.0, + now=100.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=10, + ) + assert wait == 0.0 + + +# --------------------------------------------------------------------------- +# Both gates active — max wins +# --------------------------------------------------------------------------- + + +class TestMaxOfBothGates: + def test_returns_window_wait_when_window_wait_is_larger(self): + """Window says wait 100s, min-delay says wait 5s → return 100s.""" + timestamps = [0.0 + i for i in range(35)] # 0..34 + # now = 5, window = 220, oldest = 0 → ages out at 220 → wait 215 + # min_delay = 5, last = 4, now = 5 → wait 4 + wait = compute_search_wait_seconds( + timestamps=timestamps, + last_search_at=4.0, + now=5.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=5, + ) + assert wait == pytest.approx(215.0, abs=1e-9) + + def test_returns_min_delay_wait_when_min_delay_is_larger(self): + """Window not full → window wait = 0. Min-delay 30s, last 5s + ago → wait 25s. Min-delay drives it.""" + wait = compute_search_wait_seconds( + timestamps=[100.0, 105.0], + last_search_at=105.0, + now=110.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=30, + ) + assert wait == pytest.approx(25.0, abs=1e-9) + + def test_both_zero_returns_zero(self): + """Window not full + min-delay clear → zero. Sanity.""" + wait = compute_search_wait_seconds( + timestamps=[100.0], + last_search_at=50.0, + now=200.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=10, + ) + assert wait == 0.0 + + +# --------------------------------------------------------------------------- +# Defensive — input shape variations +# --------------------------------------------------------------------------- + + +class TestDefensive: + def test_negative_min_delay_treated_as_disabled(self): + """Defensive: a negative min-delay (somehow) shouldn't return + a negative wait or trigger weird behavior. Treat as disabled.""" + wait = compute_search_wait_seconds( + timestamps=[], + last_search_at=99.0, + now=100.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=-5, + ) + assert wait == 0.0 + + def test_returns_float(self): + """Caller passes to asyncio.sleep which wants a float. Pin shape.""" + wait = compute_search_wait_seconds( + timestamps=[], + last_search_at=0.0, + now=100.0, + max_per_window=35, + window_seconds=220, + min_delay_seconds=0, + ) + assert isinstance(wait, float) diff --git a/tests/test_tidal_album_tracks.py b/tests/test_tidal_album_tracks.py new file mode 100644 index 00000000..83c52edc --- /dev/null +++ b/tests/test_tidal_album_tracks.py @@ -0,0 +1,276 @@ +"""Pin Tidal `get_album_tracks` — fetches every track on an album +with full artist + name + duration metadata hydrated. + +Discord report: clicking 'Download All' on the Your Albums section +showed "Queuing..." but never actually queued any Tidal-only albums. +Root cause: `/api/discover/album//` had no `tidal` +branch and tidal_client had no `get_album_tracks` method — the +frontend's trySources fell back to spotify/deezer which returned +None for Tidal-only IDs. + +This test suite covers the new tidal_client method: + - Cursor-paginated walk of `/v2/albums/{id}/relationships/items` + - Track meta (trackNumber + volumeNumber for multi-disc) + - Batch hydration via `_get_tracks_batch` for artist/album names + - Sort by (disc_number, track_number) so the modal renders in + album order across multi-disc releases + - Empty / error paths return [] without raising +""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + +from core.tidal_client import Track, TidalClient + + +def _make_client(): + client = TidalClient.__new__(TidalClient) + client.access_token = "fake-token" + client.token_expires_at = 9_999_999_999 + client.base_url = "https://openapi.tidal.com/v2" + client.alt_base_url = "https://api.tidal.com/v1" + client.session = MagicMock() + return client + + +class _FakeResp: + def __init__(self, status_code=200, json_body=None, text=""): + self.status_code = status_code + self._body = json_body if json_body is not None else {} + self.text = text or str(self._body) + + def json(self): + return self._body + + +# --------------------------------------------------------------------------- +# Single-page album (12 tracks, single disc) +# --------------------------------------------------------------------------- + + +_SINGLE_PAGE = { + 'data': [ + {'id': '1001', 'type': 'tracks', 'meta': {'volumeNumber': 1, 'trackNumber': 1}}, + {'id': '1002', 'type': 'tracks', 'meta': {'volumeNumber': 1, 'trackNumber': 2}}, + {'id': '1003', 'type': 'tracks', 'meta': {'volumeNumber': 1, 'trackNumber': 3}}, + ], + 'links': {}, # no `next` — single-page album +} + + +class TestSinglePageAlbum: + def test_walks_page_and_hydrates(self): + """Happy path: 3-track album, single page, single disc. + IDs enumerated → batch hydrated → returned in album order.""" + client = _make_client() + client.session.get = MagicMock(return_value=_FakeResp(200, _SINGLE_PAGE)) + client._get_tracks_batch = MagicMock(return_value=[ + Track(id='1001', name='Track One', artists=['Artist'], duration_ms=180000), + Track(id='1002', name='Track Two', artists=['Artist'], duration_ms=200000), + Track(id='1003', name='Track Three', artists=['Artist'], duration_ms=220000), + ]) + + with patch('core.tidal_client.time.sleep'): + tracks = client.get_album_tracks('album-1') + + assert [t.id for t in tracks] == ['1001', '1002', '1003'] + assert [t.track_number for t in tracks] == [1, 2, 3] + # Single disc → all volumeNumber=1 + assert all(t.disc_number == 1 for t in tracks) + + def test_no_token_returns_empty_without_request(self): + """Auth precheck failure short-circuits.""" + client = _make_client() + client.session.get = MagicMock() + with patch.object(client, '_ensure_valid_token', return_value=False): + assert client.get_album_tracks('album-1') == [] + client.session.get.assert_not_called() + + def test_http_error_returns_empty(self): + client = _make_client() + client.session.get = MagicMock(return_value=_FakeResp(404, text='not found')) + with patch('core.tidal_client.time.sleep'): + assert client.get_album_tracks('album-1') == [] + + def test_429_raises_for_rate_limit_decorator(self): + """The `rate_limited` decorator looks for '429' in the exception + message to trigger retry/backoff. Don't swallow rate-limit + responses — propagate so the decorator can handle them.""" + client = _make_client() + client.session.get = MagicMock(return_value=_FakeResp(429, text='rate limited')) + with patch('core.tidal_client.time.sleep'): + with pytest.raises(Exception, match='429'): + client.get_album_tracks('album-1') + + def test_skips_non_track_data_entries(self): + """Forward-compat: schema additions might surface non-track + types alongside tracks — only collect entries with type='tracks'.""" + client = _make_client() + client.session.get = MagicMock(return_value=_FakeResp(200, { + 'data': [ + {'id': '1', 'type': 'tracks', 'meta': {'trackNumber': 1, 'volumeNumber': 1}}, + {'id': '99', 'type': 'videos', 'meta': {'trackNumber': 99}}, + ], + 'links': {}, + })) + client._get_tracks_batch = MagicMock(return_value=[ + Track(id='1', name='Track', artists=['A'], duration_ms=100), + ]) + with patch('core.tidal_client.time.sleep'): + tracks = client.get_album_tracks('album-1') + assert len(tracks) == 1 + assert tracks[0].id == '1' + + +# --------------------------------------------------------------------------- +# Multi-disc album — sort order matters +# --------------------------------------------------------------------------- + + +class TestMultiDiscAlbum: + def test_sorts_by_disc_then_track(self): + """Reporter's albums could be multi-disc compilations. After + batch hydration the tracks may not be in album order + (filter[id] endpoint doesn't guarantee preservation). Verify + the final list is sorted by (disc, track) so the download + modal renders disc 1 → 2 in track order each.""" + client = _make_client() + # Page returns IDs in scrambled order intentionally + client.session.get = MagicMock(return_value=_FakeResp(200, { + 'data': [ + {'id': 'd1t2', 'type': 'tracks', 'meta': {'volumeNumber': 1, 'trackNumber': 2}}, + {'id': 'd2t1', 'type': 'tracks', 'meta': {'volumeNumber': 2, 'trackNumber': 1}}, + {'id': 'd1t1', 'type': 'tracks', 'meta': {'volumeNumber': 1, 'trackNumber': 1}}, + {'id': 'd2t2', 'type': 'tracks', 'meta': {'volumeNumber': 2, 'trackNumber': 2}}, + ], + 'links': {}, + })) + client._get_tracks_batch = MagicMock(return_value=[ + # Batch endpoint may not preserve order — return scrambled too + Track(id='d2t1', name='D2T1', artists=['A'], duration_ms=100), + Track(id='d1t1', name='D1T1', artists=['A'], duration_ms=100), + Track(id='d2t2', name='D2T2', artists=['A'], duration_ms=100), + Track(id='d1t2', name='D1T2', artists=['A'], duration_ms=100), + ]) + + with patch('core.tidal_client.time.sleep'): + tracks = client.get_album_tracks('album-1') + + # Expect: disc 1 first (tracks 1,2), then disc 2 (tracks 1,2) + assert [t.id for t in tracks] == ['d1t1', 'd1t2', 'd2t1', 'd2t2'] + assert [(t.disc_number, t.track_number) for t in tracks] == [ + (1, 1), (1, 2), (2, 1), (2, 2), + ] + + +# --------------------------------------------------------------------------- +# Multi-page album — cursor walk +# --------------------------------------------------------------------------- + + +class TestMultiPageAlbum: + def test_follows_cursor_chain(self): + """Big album (>20 tracks) — cursor chain must be walked. + First page returns links.next, second page returns no next.""" + client = _make_client() + page1 = { + 'data': [ + {'id': '1', 'type': 'tracks', 'meta': {'trackNumber': 1, 'volumeNumber': 1}}, + {'id': '2', 'type': 'tracks', 'meta': {'trackNumber': 2, 'volumeNumber': 1}}, + ], + 'links': {'next': '/albums/x/relationships/items?cursor=ABC'}, + } + page2 = { + 'data': [ + {'id': '3', 'type': 'tracks', 'meta': {'trackNumber': 3, 'volumeNumber': 1}}, + ], + 'links': {}, + } + responses = iter([_FakeResp(200, page1), _FakeResp(200, page2)]) + client.session.get = MagicMock(side_effect=lambda *a, **kw: next(responses)) + client._get_tracks_batch = MagicMock(return_value=[ + Track(id='1', name='T1', artists=['A'], duration_ms=100), + Track(id='2', name='T2', artists=['A'], duration_ms=100), + Track(id='3', name='T3', artists=['A'], duration_ms=100), + ]) + + with patch('core.tidal_client.time.sleep'): + tracks = client.get_album_tracks('album-1') + + assert [t.id for t in tracks] == ['1', '2', '3'] + # Two page requests must have happened + assert client.session.get.call_count == 2 + + def test_limit_short_circuits_at_page_boundary(self): + """`limit` arg caps the walk early — useful for callers that + only want a preview, not the full tracklist.""" + client = _make_client() + page1 = { + 'data': [ + {'id': '1', 'type': 'tracks', 'meta': {'trackNumber': 1, 'volumeNumber': 1}}, + {'id': '2', 'type': 'tracks', 'meta': {'trackNumber': 2, 'volumeNumber': 1}}, + ], + 'links': {'next': '/albums/x/relationships/items?cursor=ABC'}, + } + client.session.get = MagicMock(return_value=_FakeResp(200, page1)) + client._get_tracks_batch = MagicMock(return_value=[ + Track(id='1', name='T1', artists=['A'], duration_ms=100), + ]) + + with patch('core.tidal_client.time.sleep'): + tracks = client.get_album_tracks('album-1', limit=1) + + # Only one page fetched even though links.next was set + assert client.session.get.call_count == 1 + assert len(tracks) == 1 + + +# --------------------------------------------------------------------------- +# Batch hydration robustness +# --------------------------------------------------------------------------- + + +class TestHydrationRobustness: + def test_hydration_exception_returns_partial_results(self): + """If one batch fails to hydrate, other batches still return. + Defensive against transient Tidal errors mid-walk on big albums.""" + client = _make_client() + # Big single-page album → 21 IDs split into two batches (20 + 1) + big_page = { + 'data': [ + {'id': str(i), 'type': 'tracks', 'meta': {'trackNumber': i, 'volumeNumber': 1}} + for i in range(1, 22) + ], + 'links': {}, + } + client.session.get = MagicMock(return_value=_FakeResp(200, big_page)) + + # First batch succeeds, second raises + def batch_side_effect(batch_ids): + if len(batch_ids) == 1: # The trailing batch + raise RuntimeError("transient") + return [ + Track(id=tid, name=f'T{tid}', artists=['A'], duration_ms=100) + for tid in batch_ids + ] + client._get_tracks_batch = MagicMock(side_effect=batch_side_effect) + + with patch('core.tidal_client.time.sleep'): + tracks = client.get_album_tracks('album-1') + + # 20 from the first batch — second batch failed but didn't crash + assert len(tracks) == 20 + + def test_no_track_ids_returns_empty_without_hydrating(self): + """Empty album → no batch call (no point hydrating zero IDs).""" + client = _make_client() + client.session.get = MagicMock(return_value=_FakeResp(200, {'data': [], 'links': {}})) + client._get_tracks_batch = MagicMock() + with patch('core.tidal_client.time.sleep'): + tracks = client.get_album_tracks('album-1') + assert tracks == [] + client._get_tracks_batch.assert_not_called() diff --git a/tests/test_tidal_favorite_albums_artists.py b/tests/test_tidal_favorite_albums_artists.py new file mode 100644 index 00000000..32bee28a --- /dev/null +++ b/tests/test_tidal_favorite_albums_artists.py @@ -0,0 +1,442 @@ +"""Pin Tidal favorite albums + artists fetch via V2 user-collection +endpoints. + +Discord report: Discover → Your Albums section showed nothing for +Tidal users regardless of how many albums they'd favorited. Audit +found `get_favorite_albums` (and `get_favorite_artists`) called the +deprecated `/v2/favorites?filter[type]=ALBUMS|ARTISTS` endpoint +which returns 404 for personal favorites — that endpoint is scoped +to collections the third-party app created itself, not the user's +app-level favorites. The V1 fallback (`/v1/users//favorites/...`) +returns 403 for modern OAuth tokens because they carry +`collection.read` instead of the legacy `r_usr` scope. + +Fix: rewire to the same V2 user-collection cursor-paginated +endpoints we shipped for tracks (issue #502): + - `/v2/userCollectionAlbums/me/relationships/items` + - `/v2/userCollectionArtists/me/relationships/items` + +Plus per-resource batch hydration via `/v2/{albums|artists}` with +extended-include semantics (`include=artists,coverArt` for albums, +`include=profileArt` for artists) so artist names + image URLs come +back in a single request per batch instead of N+1 lookups. + +These tests pin: + - Cursor walkers dispatch correct path + type to the generic + `_iter_collection_resource_ids` helper + - Batch hydrators parse JSON:API `data[]` + `included[]` into the + legacy return shape that `database.upsert_liked_album` / + `upsert_liked_artist` consume — preserves byte-identical wiring + in `web_server.py`'s discover aggregator + - Image URL resolution picks largest variant from artwork files[] + - Artist-name resolution falls through to '' when relationships + are missing (so the upsert path doesn't trip on None) + - Empty-input + HTTP-error paths return [] without raising +""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + +from core.tidal_client import TidalClient + + +def _make_client(): + """Bare TidalClient with auth state primed — no real connection. + Mirrors the helper in test_tidal_collection_tracks.py.""" + client = TidalClient.__new__(TidalClient) + client.access_token = "fake-token" + client.token_expires_at = 9_999_999_999 + client.base_url = "https://openapi.tidal.com/v2" + client.alt_base_url = "https://api.tidal.com/v1" + client.session = MagicMock() + return client + + +class _FakeResp: + def __init__(self, status_code=200, json_body=None, text=""): + self.status_code = status_code + self._body = json_body if json_body is not None else {} + self.text = text or str(self._body) + + def json(self): + return self._body + + +# --------------------------------------------------------------------------- +# Cursor-walker dispatch +# --------------------------------------------------------------------------- + + +class TestCollectionWalkerDispatch: + def test_album_iter_passes_album_path_and_type(self): + """`_iter_collection_album_ids` must dispatch to the generic + walker with the albums path + 'albums' expected_type. If the + wrapper drifts (e.g. typoed path) the IDs come back empty.""" + client = _make_client() + with patch.object(client, '_iter_collection_resource_ids', + return_value=['111', '222']) as mock_walk: + ids = client._iter_collection_album_ids(max_ids=50) + + mock_walk.assert_called_once_with( + 'userCollectionAlbums/me/relationships/items', 'albums', 50, + ) + assert ids == ['111', '222'] + + def test_artist_iter_passes_artist_path_and_type(self): + client = _make_client() + with patch.object(client, '_iter_collection_resource_ids', + return_value=['17275']) as mock_walk: + ids = client._iter_collection_artist_ids() + + mock_walk.assert_called_once_with( + 'userCollectionArtists/me/relationships/items', 'artists', None, + ) + assert ids == ['17275'] + + +# --------------------------------------------------------------------------- +# Helper: included map + relationship resolution +# --------------------------------------------------------------------------- + + +class TestIncludedMaps: + def test_build_included_maps_groups_by_type(self): + included = [ + {'id': 'a1', 'type': 'artists', 'attributes': {'name': 'Foo'}}, + {'id': 'art1', 'type': 'artworks', 'attributes': {'files': []}}, + {'id': 'a2', 'type': 'artists', 'attributes': {'name': 'Bar'}}, + {'id': 'unknown1', 'type': 'something_else'}, + {'type': 'artworks'}, # missing id — should be skipped + ] + artists, artworks = TidalClient._build_included_maps(included) + assert set(artists.keys()) == {'a1', 'a2'} + assert set(artworks.keys()) == {'art1'} + assert artists['a1']['attributes']['name'] == 'Foo' + + def test_first_artist_name_resolves_from_map(self): + artists_map = {'a1': {'attributes': {'name': 'Eminem'}}} + rels = {'artists': {'data': [{'id': 'a1', 'type': 'artists'}]}} + assert TidalClient._first_artist_name(rels, artists_map) == 'Eminem' + + def test_first_artist_name_empty_when_no_refs(self): + """Defensive: relationships block missing or empty → '' so + upsert path doesn't trip on None.""" + assert TidalClient._first_artist_name({}, {}) == '' + assert TidalClient._first_artist_name( + {'artists': {'data': []}}, {} + ) == '' + + def test_first_artist_name_empty_when_unknown_id(self): + """Artist ref points at an ID not in included map — fall + through to '' rather than crash.""" + rels = {'artists': {'data': [{'id': 'missing'}]}} + artists_map = {'other': {'attributes': {'name': 'X'}}} + assert TidalClient._first_artist_name(rels, artists_map) == '' + + def test_first_artwork_url_picks_first_file(self): + """Tidal returns artwork files largest-first. Picking files[0] + gets the highest-resolution variant (typically 1280×1280).""" + artworks_map = { + 'art1': {'attributes': {'files': [ + {'href': 'https://big.jpg', 'meta': {'width': 1280}}, + {'href': 'https://small.jpg', 'meta': {'width': 320}}, + ]}} + } + rel = {'data': [{'id': 'art1', 'type': 'artworks'}]} + url = TidalClient._first_artwork_url(rel, artworks_map) + assert url == 'https://big.jpg' + + def test_first_artwork_url_none_when_no_relationship(self): + assert TidalClient._first_artwork_url({}, {}) is None + assert TidalClient._first_artwork_url({'data': []}, {}) is None + + def test_first_artwork_url_none_when_no_files(self): + """Defensive: artwork resource exists but has no files array. + Return None rather than IndexError.""" + artworks_map = {'art1': {'attributes': {'files': []}}} + rel = {'data': [{'id': 'art1'}]} + assert TidalClient._first_artwork_url(rel, artworks_map) is None + + +# --------------------------------------------------------------------------- +# Batch hydration — albums +# --------------------------------------------------------------------------- + + +_ALBUM_BATCH_RESPONSE = { + 'data': [ + { + 'id': '141121273', + 'type': 'albums', + 'attributes': { + 'title': 'Mr. Morale & The Big Steppers', + 'releaseDate': '2022-05-13', + 'numberOfItems': 18, + }, + 'relationships': { + 'artists': {'data': [{'id': '5034248', 'type': 'artists'}]}, + 'coverArt': {'data': [{'id': 'cover-uuid', 'type': 'artworks'}]}, + }, + }, + { + 'id': '999', + 'type': 'albums', + 'attributes': {'title': 'Album Without Artist or Cover'}, + 'relationships': {}, + }, + ], + 'included': [ + { + 'id': '5034248', 'type': 'artists', + 'attributes': {'name': 'Kendrick Lamar'}, + }, + { + 'id': 'cover-uuid', 'type': 'artworks', + 'attributes': {'files': [ + {'href': 'https://resources.tidal.com/images/cover/1280x1280.jpg'}, + ]}, + }, + ], +} + + +class TestGetAlbumsBatch: + def test_parses_full_album_response(self): + client = _make_client() + client.session.get = MagicMock( + return_value=_FakeResp(200, _ALBUM_BATCH_RESPONSE) + ) + results = client._get_albums_batch(['141121273', '999']) + + assert len(results) == 2 + # First album — full attributes resolved from included + first = results[0] + assert first['tidal_id'] == '141121273' + assert first['album_name'] == 'Mr. Morale & The Big Steppers' + assert first['artist_name'] == 'Kendrick Lamar' + assert first['image_url'] == 'https://resources.tidal.com/images/cover/1280x1280.jpg' + assert first['release_date'] == '2022-05-13' + assert first['total_tracks'] == 18 + # Second album — missing relationships fall through to defaults + second = results[1] + assert second['album_name'] == 'Album Without Artist or Cover' + assert second['artist_name'] == '' + assert second['image_url'] is None + assert second['release_date'] == '' + assert second['total_tracks'] == 0 + + def test_empty_input_returns_empty_without_request(self): + client = _make_client() + client.session.get = MagicMock() + results = client._get_albums_batch([]) + assert results == [] + client.session.get.assert_not_called() + + def test_http_error_returns_empty(self): + client = _make_client() + client.session.get = MagicMock( + return_value=_FakeResp(500, text='server error') + ) + results = client._get_albums_batch(['111']) + assert results == [] + + def test_skips_data_entries_with_wrong_type(self): + """Forward-compat: response shape might surface non-album + resources alongside the request — only collect entries whose + type is 'albums'.""" + client = _make_client() + client.session.get = MagicMock(return_value=_FakeResp(200, { + 'data': [ + {'id': '1', 'type': 'albums', 'attributes': {'title': 'A'}, 'relationships': {}}, + {'id': '2', 'type': 'tracks', 'attributes': {'title': 'Skip Me'}}, + ], + 'included': [], + })) + results = client._get_albums_batch(['1', '2']) + assert len(results) == 1 + assert results[0]['album_name'] == 'A' + + def test_filter_id_param_is_comma_joined(self): + """The Tidal API expects `filter[id]=a,b,c` — verify our + param construction. Drift here would break batching against + production silently.""" + client = _make_client() + captured_params = {} + + def fake_get(url, params=None, headers=None, timeout=None): + captured_params.update(params or {}) + return _FakeResp(200, {'data': [], 'included': []}) + + client.session.get = MagicMock(side_effect=fake_get) + client._get_albums_batch(['111', '222', '333']) + assert captured_params['filter[id]'] == '111,222,333' + assert captured_params['include'] == 'artists,coverArt' + + +# --------------------------------------------------------------------------- +# Batch hydration — artists +# --------------------------------------------------------------------------- + + +_ARTIST_BATCH_RESPONSE = { + 'data': [ + { + 'id': '17275', + 'type': 'artists', + 'attributes': {'name': 'Eminem'}, + 'relationships': { + 'profileArt': {'data': [{'id': 'profile-uuid', 'type': 'artworks'}]}, + }, + }, + ], + 'included': [ + { + 'id': 'profile-uuid', 'type': 'artworks', + 'attributes': {'files': [ + {'href': 'https://resources.tidal.com/images/profile/750x750.jpg'}, + ]}, + }, + ], +} + + +class TestGetArtistsBatch: + def test_parses_full_artist_response(self): + client = _make_client() + client.session.get = MagicMock( + return_value=_FakeResp(200, _ARTIST_BATCH_RESPONSE) + ) + results = client._get_artists_batch(['17275']) + + assert len(results) == 1 + assert results[0]['tidal_id'] == '17275' + assert results[0]['name'] == 'Eminem' + assert results[0]['image_url'] == 'https://resources.tidal.com/images/profile/750x750.jpg' + + def test_empty_input_returns_empty_without_request(self): + client = _make_client() + client.session.get = MagicMock() + assert client._get_artists_batch([]) == [] + client.session.get.assert_not_called() + + def test_http_error_returns_empty(self): + client = _make_client() + client.session.get = MagicMock( + return_value=_FakeResp(404, text='not found') + ) + assert client._get_artists_batch(['17275']) == [] + + def test_filter_id_and_include_params(self): + client = _make_client() + captured = {} + + def fake_get(url, params=None, headers=None, timeout=None): + captured.update(params or {}) + return _FakeResp(200, {'data': [], 'included': []}) + + client.session.get = MagicMock(side_effect=fake_get) + client._get_artists_batch(['17275', '721']) + assert captured['filter[id]'] == '17275,721' + assert captured['include'] == 'profileArt' + + +# --------------------------------------------------------------------------- +# Public methods — orchestrator behavior +# --------------------------------------------------------------------------- + + +class TestGetFavoriteAlbums: + def test_walks_then_batches_then_returns(self): + """End-to-end: iter returns IDs, batch hydrates them, result + is the concatenation. Backward-compatible shape preserved + for `database.upsert_liked_album` callers.""" + client = _make_client() + with patch.object(client, '_iter_collection_album_ids', + return_value=['1', '2', '3']) as mock_iter, \ + patch.object(client, '_get_albums_batch', + return_value=[ + {'tidal_id': '1', 'album_name': 'A', + 'artist_name': 'X', 'image_url': 'u', + 'release_date': '2020', 'total_tracks': 10}, + {'tidal_id': '2', 'album_name': 'B', + 'artist_name': 'Y', 'image_url': None, + 'release_date': '', 'total_tracks': 0}, + ]) as mock_batch: + results = client.get_favorite_albums(limit=100) + + mock_iter.assert_called_once_with(max_ids=100) + # Single batch call since 3 IDs fit in one BATCH_SIZE chunk (20) + assert mock_batch.call_count == 1 + assert len(results) == 2 + assert results[0]['tidal_id'] == '1' + # Verify shape compatibility with upsert_liked_album kwargs + expected_keys = {'tidal_id', 'album_name', 'artist_name', + 'image_url', 'release_date', 'total_tracks'} + assert set(results[0].keys()) == expected_keys + + def test_no_ids_returns_empty_without_batch(self): + client = _make_client() + with patch.object(client, '_iter_collection_album_ids', return_value=[]), \ + patch.object(client, '_get_albums_batch') as mock_batch: + assert client.get_favorite_albums() == [] + mock_batch.assert_not_called() + + def test_chunks_into_batch_size(self): + """41 IDs at BATCH_SIZE 20 → three batches of 20/20/1. + Tidal's filter[id] cap is the per-request limit; orchestrator + must respect it.""" + client = _make_client() + ids = [str(i) for i in range(41)] + captured_batches = [] + + def fake_batch(batch): + captured_batches.append(list(batch)) + return [{'tidal_id': b, 'album_name': f'A{b}', 'artist_name': '', + 'image_url': None, 'release_date': '', 'total_tracks': 0} + for b in batch] + + with patch.object(client, '_iter_collection_album_ids', return_value=ids), \ + patch.object(client, '_get_albums_batch', side_effect=fake_batch): + results = client.get_favorite_albums() + + assert len(results) == 41 + assert [len(b) for b in captured_batches] == [20, 20, 1] + + +class TestGetFavoriteArtists: + def test_walks_then_batches(self): + client = _make_client() + with patch.object(client, '_iter_collection_artist_ids', + return_value=['17275']) as mock_iter, \ + patch.object(client, '_get_artists_batch', + return_value=[{'tidal_id': '17275', 'name': 'Eminem', + 'image_url': 'https://eminem.jpg'}]) as mock_batch: + results = client.get_favorite_artists(limit=200) + + mock_iter.assert_called_once_with(max_ids=200) + mock_batch.assert_called_once() + assert len(results) == 1 + assert results[0]['name'] == 'Eminem' + # Backward-compat shape — exactly the keys the prior + # implementation returned + assert set(results[0].keys()) == {'tidal_id', 'name', 'image_url'} + + def test_no_ids_returns_empty(self): + client = _make_client() + with patch.object(client, '_iter_collection_artist_ids', return_value=[]), \ + patch.object(client, '_get_artists_batch') as mock_batch: + assert client.get_favorite_artists() == [] + mock_batch.assert_not_called() + + def test_swallows_iter_exception_returns_empty(self): + """Defensive: if the cursor walker blows up mid-page, the + public method should return [] (no partial corruption of the + liked-artists table).""" + client = _make_client() + with patch.object(client, '_iter_collection_artist_ids', + side_effect=RuntimeError('boom')): + assert client.get_favorite_artists() == [] diff --git a/web_server.py b/web_server.py index 63557284..4653b741 100644 --- a/web_server.py +++ b/web_server.py @@ -40,7 +40,7 @@ logger = setup_logging(_log_level, _log_path) # App version — single source of truth for backup metadata, system-info, update check, etc. # Semver: MAJOR.MINOR.PATCH. Bump at each dev→main release. -_SOULSYNC_BASE_VERSION = "2.5.0" +_SOULSYNC_BASE_VERSION = "2.5.1" def _build_version_string(): """Append short commit hash to version when available (e.g. 2.35+abc1234).""" @@ -9270,14 +9270,37 @@ def download_discography(artist_id): from database.music_database import MusicDatabase from core.metadata.album_tracks import get_artist_album_tracks + from core.metadata.discography_filters import ( + content_type_skip_reason, + load_global_content_filter_settings, + track_already_owned, + track_artist_matches, + ) db = MusicDatabase() profile_id = get_current_profile_id() + # Honor the same content-type filters the watchlist scanner uses + # (issue #559). One read at the top — settings don't change + # mid-stream and the four bool reads aren't worth re-running per + # track. + content_settings = load_global_content_filter_settings(config_manager) + # Library-ownership check uses the active media server so the + # match is scoped to the same source whose tracks the user can + # actually see in their library. None falls through to a + # cross-server search inside check_track_exists. + active_server = None + try: + active_server = config_manager.get_active_media_server() + except Exception as e: + logger.debug("active media server lookup failed: %s", e) total_added = 0 total_skipped = 0 + total_skipped_artist = 0 + total_skipped_filter = 0 + total_skipped_owned = 0 def generate_ndjson(): - nonlocal total_added, total_skipped + nonlocal total_added, total_skipped, total_skipped_artist, total_skipped_filter, total_skipped_owned for entry in album_entries: album_id = entry['id'] @@ -9325,6 +9348,9 @@ def download_discography(artist_id): added = 0 skipped = 0 + skipped_artist = 0 + skipped_filter = 0 + skipped_owned = 0 for track in tracks: track_name = track.get('name', '') @@ -9333,6 +9359,33 @@ def download_discography(artist_id): track_artists = track.get('artists', []) or album_artists track_id = track.get('id', '') + # Issue #559: drop tracks where the requested + # artist isn't in the track's artists list + # (cross-artist compilation / appears_on + # contamination). Keeps features. + if not track_artist_matches(track_artists, hint_artist): + skipped_artist += 1 + continue + + # Issue #559: honor watchlist global content-type + # filters (live / remix / acoustic / instrumental) + # for one-off discography downloads too — same + # contract as the discography backfill repair job. + skip_reason = content_type_skip_reason(track_name, album_name, content_settings) + if skip_reason: + skipped_filter += 1 + continue + + # Skowl (Discord): clicking Download Discography + # twice re-queued every track because add_to_wishlist + # only dedups against the wishlist, not the library. + # Same library-ownership check the discography + # backfill repair job uses. Format-agnostic so + # Blasphemy mode (FLAC→MP3) doesn't false-miss. + if track_already_owned(db, track_name, hint_artist, album_name, active_server): + skipped_owned += 1 + continue + spotify_track_data = { 'id': track_id, 'name': track_name, @@ -9379,8 +9432,14 @@ def download_discography(artist_id): total_added += added total_skipped += skipped + total_skipped_artist += skipped_artist + total_skipped_filter += skipped_filter + total_skipped_owned += skipped_owned logger.warning( - f"[Discography] {album_name} ({resolved_source}): {added} added, {skipped} skipped" + f"[Discography] {album_name} ({resolved_source}): {added} added, " + f"{skipped} skipped (wishlist), {skipped_artist} skipped (artist mismatch), " + f"{skipped_filter} skipped (content filter), " + f"{skipped_owned} skipped (already in library)" ) yield json.dumps({ "album_id": album_id, @@ -9388,6 +9447,9 @@ def download_discography(artist_id): "status": "done", "tracks_added": added, "tracks_skipped": skipped, + "tracks_skipped_artist": skipped_artist, + "tracks_skipped_filter": skipped_filter, + "tracks_skipped_owned": skipped_owned, "tracks_total": len(tracks), "source": resolved_source, }) + '\n' @@ -9402,12 +9464,17 @@ def download_discography(artist_id): logger.warning( f"[Discography] Complete for {artist_name}: {total_added} tracks added, " - f"{total_skipped} skipped across {len(album_entries)} albums" + f"{total_skipped} skipped (wishlist), {total_skipped_artist} skipped (artist mismatch), " + f"{total_skipped_filter} skipped (content filter), " + f"{total_skipped_owned} skipped (already in library) across {len(album_entries)} albums" ) yield json.dumps({ "status": "complete", "total_added": total_added, "total_skipped": total_skipped, + "total_skipped_artist": total_skipped_artist, + "total_skipped_filter": total_skipped_filter, + "total_skipped_owned": total_skipped_owned, "total_albums": len(album_entries), }) + '\n' @@ -20130,6 +20197,81 @@ def get_discover_album(source, album_id): 'source': fallback_source, }) + elif source == 'tidal': + # Tidal albums from Your Albums (sourced via the V2 user- + # collection endpoint). Two-call resolution: get_album for + # metadata, get_album_tracks for the cursor-paginated + # tracklist. `get_album_tracks` returns `Track` objects + # with `track_number` / `disc_number` annotated so the + # download modal renders in album order across multi-disc + # releases. Serialise to the same shape Spotify/Deezer + # return so the frontend track-mapping stays uniform. + if not tidal_client or not tidal_client.is_authenticated(): + return jsonify({"error": "Tidal not authenticated"}), 401 + + album_meta = tidal_client.get_album(album_id) + tidal_tracks = tidal_client.get_album_tracks(album_id) + + if not album_meta and not tidal_tracks: + return jsonify({"error": "Tidal album not found"}), 404 + + album_name = (album_meta or {}).get('title') or request.args.get('name', '') + release_date = (album_meta or {}).get('releaseDate', '') + total_tracks = (album_meta or {}).get('numberOfItems') or len(tidal_tracks) + album_artist_name = request.args.get('artist', '') + + # Build cover image URL from the album metadata. Tidal + # exposes cover art via the `coverArt` relationship which + # `get_album` doesn't fetch (it's a one-shot attributes + # call). Best-effort: request it inline. + cover_url = '' + try: + cover_resp = tidal_client.session.get( + f"{tidal_client.base_url}/albums/{album_id}", + params={'countryCode': 'US', 'include': 'coverArt'}, + headers={'accept': 'application/vnd.api+json'}, + timeout=10, + ) + if cover_resp.status_code == 200: + payload = cover_resp.json() + _, artworks = tidal_client._build_included_maps(payload.get('included', [])) + cover_rel = (payload.get('data') or {}).get('relationships', {}).get('coverArt', {}) + cover_url = tidal_client._first_artwork_url(cover_rel, artworks) or '' + except Exception as e: + logger.debug(f"Tidal cover-art resolve failed for album {album_id}: {e}") + + tracks_out = [] + for t in tidal_tracks: + tracks_out.append({ + 'id': t.id, + 'name': t.name, + 'artists': [{'name': a} for a in (t.artists or [])], + 'duration_ms': t.duration_ms, + 'track_number': getattr(t, 'track_number', 0), + 'disc_number': getattr(t, 'disc_number', 1), + }) + + # Album-level artist name preference: explicit ?artist= + # query (passed by frontend with the saved-album row) wins + # over guessing from the first track. The saved-album row + # already resolved the canonical artist via the V2 + # collection endpoint. + if not album_artist_name and tidal_tracks: + first_artists = tidal_tracks[0].artists or [] + album_artist_name = first_artists[0] if first_artists else '' + + return jsonify({ + 'id': album_id, + 'name': album_name or 'Unknown Album', + 'artists': [{'name': album_artist_name}] if album_artist_name else [], + 'release_date': release_date, + 'total_tracks': total_tracks, + 'album_type': 'album', + 'images': [{'url': cover_url}] if cover_url else [], + 'tracks': tracks_out, + 'source': 'tidal', + }) + elif source == 'discogs': # Discogs release detail. release_id comes from the Your # Albums Discogs source. Tracklist needs normalizing — @@ -23836,10 +23978,11 @@ def _build_sync_deps(): ) -def _run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, profile_id=1, playlist_image_url=''): +def _run_sync_task(playlist_id, playlist_name, tracks_json, automation_id=None, profile_id=1, playlist_image_url='', sync_mode='replace'): return _discovery_sync.run_sync_task( playlist_id, playlist_name, tracks_json, automation_id, profile_id, playlist_image_url, _build_sync_deps(), + sync_mode=sync_mode, ) @@ -23855,14 +23998,22 @@ def start_playlist_sync(): playlist_name = data.get('playlist_name') tracks_json = data.get('tracks') # Pass the full track list playlist_image_url = data.get('image_url', '') + # 'replace' (default) deletes the server playlist and recreates it from + # the source. 'append' preserves user-added tracks already on the server + # playlist — only adds tracks that aren't there yet. Per-server clients + # implement append via native add APIs (Plex addItems, Jellyfin POST + # /Playlists//Items, Navidrome updatePlaylist?songIdToAdd=...). + sync_mode = data.get('sync_mode', 'replace') + if sync_mode not in ('replace', 'append'): + sync_mode = 'replace' if not all([playlist_id, playlist_name, tracks_json]): return jsonify({"success": False, "error": "Missing playlist_id, name, or tracks."}), 400 # Add activity for sync start - add_activity_item("", "Spotify Sync Started", f"'{playlist_name}' - {len(tracks_json)} tracks", "Now") + add_activity_item("", "Spotify Sync Started", f"'{playlist_name}' - {len(tracks_json)} tracks ({sync_mode})", "Now") - logger.info(f"Starting playlist sync for '{playlist_name}' with {len(tracks_json)} tracks") + logger.info(f"Starting playlist sync for '{playlist_name}' with {len(tracks_json)} tracks (mode: {sync_mode})") logger.debug(f"Request parsed at {time.strftime('%H:%M:%S')} (took {(time.time()-request_start_time)*1000:.1f}ms)") with sync_lock: @@ -23875,7 +24026,7 @@ def start_playlist_sync(): # Submit the task to the thread pool (capture profile_id while still in request context) _sync_profile_id = get_current_profile_id() thread_submit_time = time.time() - future = sync_executor.submit(_run_sync_task, playlist_id, playlist_name, tracks_json, None, _sync_profile_id, playlist_image_url) + future = sync_executor.submit(_run_sync_task, playlist_id, playlist_name, tracks_json, None, _sync_profile_id, playlist_image_url, sync_mode) active_sync_workers[playlist_id] = future thread_submit_duration = (time.time() - thread_submit_time) * 1000 logger.info(f"⏱️ [TIMING] Thread submitted at {time.strftime('%H:%M:%S')} (took {thread_submit_duration:.1f}ms)") @@ -34541,14 +34692,35 @@ def auto_import_approve_all(): @app.route('/api/auto-import/clear-completed', methods=['POST']) def auto_import_clear_completed(): - """Remove completed/imported items from history.""" + """Remove completed/imported items from history. + + `processing` rows are included so zombie entries (server restarted + mid-import → `_record_in_progress` row never got finalized) get + swept. Live in-flight imports are protected by intersecting against + `_snapshot_active()` — anything currently registered in the worker's + `_active_imports` map keeps its row. `pending_review` is left out so + user still has to approve/reject those explicitly. + """ if not auto_import_worker: return jsonify({"success": False, "error": "Auto-import not available"}), 500 try: + active_hashes = {e['folder_hash'] for e in auto_import_worker._snapshot_active()} db = get_database() with db._get_connection() as conn: cursor = conn.cursor() - cursor.execute("DELETE FROM auto_import_history WHERE status IN ('completed', 'approved', 'failed', 'needs_identification', 'rejected')") + base_sql = ( + "DELETE FROM auto_import_history " + "WHERE status IN ('completed', 'approved', 'failed', " + "'needs_identification', 'rejected', 'processing')" + ) + if active_hashes: + placeholders = ','.join('?' * len(active_hashes)) + cursor.execute( + f"{base_sql} AND folder_hash NOT IN ({placeholders})", + tuple(active_hashes), + ) + else: + cursor.execute(base_sql) count = cursor.rowcount conn.commit() return jsonify({"success": True, "count": count}) diff --git a/webui/index.html b/webui/index.html index 77fb01f4..64f3aa50 100644 --- a/webui/index.html +++ b/webui/index.html @@ -4329,6 +4329,16 @@ Extra time to wait for late results (5-60 seconds) +
+ + + Forces a gap between + consecutive searches. Smooths burst patterns + that trip ISP anti-abuse (e.g. Bell Canada + cuts the WAN after rapid peer-connection + spikes). 0 disables. +
+
+ ${img ? `` : '
🎵
'} +
+
+
${escapeHtml(albumName)}
+
${escapeHtml(artistName)}${year ? ' · ' + year : ''}${tracks ? ' · ' + tracks + ' tracks' : ''}${src ? ' · ' + src : ''}
+
+
+ + `; +} + + +function _yourAlbumsBatchSelectAll(select) { + document.querySelectorAll('.your-albums-batch-cb').forEach(cb => { + if (cb.closest('.discog-card').style.display !== 'none') cb.checked = select; + }); + _updateYourAlbumsBatchFooterCount(); +} + + +function _updateYourAlbumsBatchFooterCount() { + const checked = document.querySelectorAll('.your-albums-batch-cb:checked'); + let releases = 0, tracks = 0; + checked.forEach(cb => { + if (cb.closest('.discog-card').style.display !== 'none') { + releases++; + tracks += parseInt(cb.dataset.tracks) || 0; + } + }); + const info = document.getElementById('your-albums-batch-footer-info'); + const btn = document.getElementById('your-albums-batch-submit-text'); + if (info) info.textContent = `${releases} album${releases !== 1 ? 's' : ''}${tracks ? ' · ' + tracks + ' tracks' : ''}`; + if (btn) btn.textContent = releases > 0 ? `Add ${releases} to Wishlist` : 'Select albums'; + const submitBtn = document.getElementById('your-albums-batch-submit-btn'); + if (submitBtn) submitBtn.disabled = releases === 0; +} + + +function _closeYourAlbumsBatchModal() { + const overlay = document.getElementById('your-albums-batch-modal-overlay'); + if (overlay) { + overlay.classList.remove('visible'); + setTimeout(() => overlay.remove(), 200); + } +} + + +async function _startYourAlbumsBatchAddToWishlist() { + const overlay = document.getElementById('your-albums-batch-modal-overlay'); + if (!overlay) return; + const rows = overlay._yourAlbumsRows || []; + + // Collect selected row indices from the checked checkboxes. + const selectedRowIndices = []; + document.querySelectorAll('.your-albums-batch-cb:checked').forEach(cb => { + if (cb.closest('.discog-card').style.display !== 'none') { + selectedRowIndices.push(parseInt(cb.dataset.rowIndex)); + } + }); + const selected = rows.filter(r => selectedRowIndices.includes(r._index)); + if (selected.length === 0) return; + + // Switch to progress view. + const grid = document.getElementById('your-albums-batch-grid'); + const progress = document.getElementById('your-albums-batch-progress'); + const footer = document.getElementById('your-albums-batch-footer'); + const filterBar = overlay.querySelector('.discog-filter-bar'); + + if (grid) grid.style.display = 'none'; + if (filterBar) filterBar.style.display = 'none'; + if (progress) { + progress.style.display = ''; + progress.innerHTML = ''; + } + + selected.forEach(row => { + const item = document.createElement('div'); + item.className = 'discog-progress-item active'; + item.id = `your-albums-batch-prog-${row._src.source}-${row._src.id}`; + item.innerHTML = ` +
${row.image_url ? `` : '🎵'}
+
+
${escapeHtml(row.album_name || '')}
+
Waiting...
+
+
+ `; + progress.appendChild(item); + }); + + const submitBtn = document.getElementById('your-albums-batch-submit-btn'); + if (submitBtn) submitBtn.style.display = 'none'; + if (footer) { + const info = document.getElementById('your-albums-batch-footer-info'); + if (info) info.textContent = 'Processing... this may take a moment'; + } + + // Build per-album payload matching the discography endpoint contract. + // URL artist_id is functionally unused by the endpoint when per-album + // metadata is supplied — backend resolves each album through its own + // `source` + `artist_name`. Placeholder 'your-albums' makes the route + // match without picking an arbitrary library artist. + const albumsPayload = selected.map(r => ({ + id: r._src.id, + name: r.album_name || '', + artist_name: r.artist_name || '', + source: r._src.source, + })); + + try { + const response = await fetch(`/api/artist/your-albums/download-discography`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + albums: albumsPayload, + artist_name: 'Your Albums', + source: null, + }), + }); + + const reader = response.body.getReader(); + const decoder = new TextDecoder(); + let buffer = ''; + let totalAdded = 0, totalSkipped = 0; + + while (true) { + const { done, value } = await reader.read(); + if (done) break; + buffer += decoder.decode(value, { stream: true }); + const lines = buffer.split('\n'); + buffer = lines.pop(); + for (const line of lines) { + if (!line.trim()) continue; + try { + const data = JSON.parse(line); + if (data.status === 'complete') { + totalAdded = data.total_added || 0; + totalSkipped = data.total_skipped || 0; + } else if (data.album_id) { + // Find the matching progress card — match by composite source-id + // pair since the same album_id could appear across sources. + const matching = selected.find(s => s._src.id === String(data.album_id)); + if (matching) { + const item = document.getElementById(`your-albums-batch-prog-${matching._src.source}-${matching._src.id}`); + if (item) { + const status = item.querySelector('.discog-prog-status'); + const icon = item.querySelector('.discog-prog-icon'); + if (data.status === 'done') { + if (status) status.textContent = `${data.tracks_added || 0} added · ${data.tracks_skipped || 0} skipped`; + if (icon) icon.innerHTML = '✓'; + item.classList.add('done'); + item.classList.remove('active'); + } else if (data.status === 'error') { + if (status) status.textContent = `Error: ${data.message || 'unknown'}`; + if (icon) icon.innerHTML = '✗'; + item.classList.add('error'); + item.classList.remove('active'); + } + } + } + } + } catch (parseErr) { + console.debug('your-albums batch ndjson parse:', parseErr); + } + } + } + + if (footer) { + const info = document.getElementById('your-albums-batch-footer-info'); + if (info) info.textContent = `${totalAdded} tracks added to wishlist · ${totalSkipped} skipped`; + } + if (submitBtn) { + submitBtn.style.display = ''; + submitBtn.disabled = true; + const txt = document.getElementById('your-albums-batch-submit-text'); + if (txt) txt.textContent = 'Done'; + } + showToast(`${totalAdded} tracks added to wishlist`, totalAdded > 0 ? 'success' : 'info'); + } catch (e) { + console.error('Error adding your albums to wishlist:', e); showToast(`Error: ${e.message}`, 'error'); } } diff --git a/webui/static/downloads.js b/webui/static/downloads.js index f443ae44..29c58f96 100644 --- a/webui/static/downloads.js +++ b/webui/static/downloads.js @@ -4098,9 +4098,21 @@ async function cancelTrackDownload(playlistId, trackIndex) { } // Find and REPLACE the old startPlaylistSyncFromModal function -async function startPlaylistSync(playlistId) { +async function startPlaylistSync(playlistId, syncModeOverride = null) { const startTime = Date.now(); - console.log(`🚀 [${new Date().toTimeString().split(' ')[0]}] Starting sync for playlist: ${playlistId}`); + // Sync mode: prefer explicit override (e.g. from automation/discover code paths + // that don't render the modal selector), else read the per-playlist + + +
diff --git a/webui/static/sync-spotify.js b/webui/static/sync-spotify.js index 796d1ce5..ee0eeca9 100644 --- a/webui/static/sync-spotify.js +++ b/webui/static/sync-spotify.js @@ -1935,6 +1935,10 @@ function showPlaylistDetailsModal(playlist) { ? '📊 View Download Results' : '📥 Download Missing Tracks'} +