From 85b6ddb9970a7370f32b380dcb3ae54e67c2ec8e Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 4 Jun 2026 11:47:31 -0700 Subject: [PATCH] Navidrome: pin music-folder selection by id, not name (survives renames) Follow-up hardening to #789. The selection was keyed purely by folder name, so renaming a music folder in Navidrome silently reverted the scan to all libraries. Now persist the folder id (stable across renames) as the primary key alongside the name (kept for display + back-compat), and restore by id first with a name fallback. Self-heals on reconnect: pre-id installs and drifted/renamed names get the id + fresh name written back, so the settings dropdown keeps highlighting the right folder. Tests: restore-by-id-after-rename (+ name heal), name-fallback self-heals id, no-drift writes nothing. --- core/navidrome_client.py | 33 ++++++++-- tests/media_server/test_navidrome_pinning.py | 69 ++++++++++++++------ 2 files changed, 76 insertions(+), 26 deletions(-) diff --git a/core/navidrome_client.py b/core/navidrome_client.py index fcf65bb6..ee8c23ee 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -224,7 +224,10 @@ class NavidromeClient(MediaServerClient): logger.info(f"Set music folder to: {folder_name} (ID: {self.music_folder_id})") from database.music_database import MusicDatabase db = MusicDatabase() + # Persist the id (stable across renames) as the primary key, + # keep the name for display + back-compat fallback. db.set_preference('navidrome_music_folder', folder_name) + db.set_preference('navidrome_music_folder_id', folder['key']) return True # If folder_name is empty, clear the selection if not folder_name: @@ -233,6 +236,7 @@ class NavidromeClient(MediaServerClient): from database.music_database import MusicDatabase db = MusicDatabase() db.set_preference('navidrome_music_folder', '') + db.set_preference('navidrome_music_folder_id', '') logger.info("Cleared music folder selection — will use all libraries") return True logger.warning(f"Music folder '{folder_name}' not found") @@ -284,18 +288,35 @@ class NavidromeClient(MediaServerClient): try: from database.music_database import MusicDatabase db = MusicDatabase() + saved_id = db.get_preference('navidrome_music_folder_id') saved_folder = db.get_preference('navidrome_music_folder') - if saved_folder: + if saved_id or saved_folder: # Use the non-reentrant fetch: we're still inside # ensure_connection() here (_is_connecting=True), so the # public get_music_folders() would re-enter the guard and # return [], silently dropping the saved selection. folders = self._fetch_music_folders() - for folder in folders: - if folder['title'] == saved_folder: - self.music_folder_id = folder['key'] - logger.info(f"Restored music folder preference: {saved_folder} (ID: {self.music_folder_id})") - break + # Match by id first (stable across renames in Navidrome); + # fall back to name for installs saved before the id was + # persisted. + matched = None + if saved_id: + matched = next((f for f in folders if f['key'] == str(saved_id)), None) + if matched is None and saved_folder: + matched = next((f for f in folders if f['title'] == saved_folder), None) + if matched is not None: + self.music_folder_id = matched['key'] + logger.info(f"Restored music folder preference: {matched['title']} (ID: {self.music_folder_id})") + # Self-heal drifted prefs: a pre-id install (no saved + # id) or a folder renamed in Navidrome (stale name). + # id stays the durable key; name is kept fresh so the + # settings dropdown highlights the right option. + if str(saved_id or '') != matched['key'] or (saved_folder or '') != matched['title']: + try: + db.set_preference('navidrome_music_folder_id', matched['key']) + db.set_preference('navidrome_music_folder', matched['title']) + except Exception as heal_err: + logger.debug(f"Could not self-heal music folder prefs: {heal_err}") except Exception as e: logger.warning(f"Could not restore music folder preference: {e}") else: diff --git a/tests/media_server/test_navidrome_pinning.py b/tests/media_server/test_navidrome_pinning.py index 76d15070..1f6201c5 100644 --- a/tests/media_server/test_navidrome_pinning.py +++ b/tests/media_server/test_navidrome_pinning.py @@ -97,33 +97,62 @@ def test_fetch_music_folders_parses_without_ensure_connection(nav_client): ] +def _fake_request(endpoint, params=None): + if endpoint == 'ping': + return {'status': 'ok', 'version': '1.16.1'} + if endpoint == 'getMusicFolders': + return {'musicFolders': {'musicFolder': [ + {'id': 1, 'name': 'Music'}, + {'id': 2, 'name': 'Audiobooks'}, + ]}} + return None + + +def _prefs(values): + """A MusicDatabase mock whose get_preference reads from `values`.""" + db = MagicMock() + db.get_preference.side_effect = lambda key, *a, **k: values.get(key) + return db + + +def _connect_with(nav_client, fake_db): + with patch('core.navidrome_client.config_manager.get_navidrome_config', + return_value={'base_url': 'http://nav', 'username': 'u', 'password': 'p'}), \ + patch('database.music_database.MusicDatabase', return_value=fake_db), \ + patch.object(nav_client, '_make_request', side_effect=_fake_request): + assert nav_client.ensure_connection() is True + + def test_setup_client_restores_saved_music_folder(nav_client): """Regression for #789: a saved music-folder selection must survive _setup_client. The restore runs while still inside ensure_connection() (_is_connecting=True); the old code called the public get_music_folders(), which re-entered the guard, got [], and left music_folder_id=None — so every scan imported all libraries regardless of the user's selection.""" - def fake_request(endpoint, params=None): - if endpoint == 'ping': - return {'status': 'ok', 'version': '1.16.1'} - if endpoint == 'getMusicFolders': - return {'musicFolders': {'musicFolder': [ - {'id': 1, 'name': 'Music'}, - {'id': 2, 'name': 'Audiobooks'}, - ]}} - return None - - fake_db = MagicMock() - fake_db.get_preference.return_value = 'Audiobooks' - - with patch('core.navidrome_client.config_manager.get_navidrome_config', - return_value={'base_url': 'http://nav', 'username': 'u', 'password': 'p'}), \ - patch('database.music_database.MusicDatabase', return_value=fake_db), \ - patch.object(nav_client, '_make_request', side_effect=fake_request): - assert nav_client.ensure_connection() is True - - # Selection ('Audiobooks') was restored to its folder id, not left None. + fake_db = _prefs({'navidrome_music_folder_id': '2', 'navidrome_music_folder': 'Audiobooks'}) + _connect_with(nav_client, fake_db) assert nav_client.music_folder_id == '2' + # Nothing drifted → no self-heal writes. + fake_db.set_preference.assert_not_called() + + +def test_setup_client_restores_by_id_after_folder_rename(nav_client): + """Hardening: the id is the primary key, so a folder renamed in Navidrome + (stored name no longer matches its title) still resolves by id — and the + stale name is healed so the settings dropdown stays correct.""" + fake_db = _prefs({'navidrome_music_folder_id': '2', 'navidrome_music_folder': 'Old Stale Name'}) + _connect_with(nav_client, fake_db) + assert nav_client.music_folder_id == '2' + fake_db.set_preference.assert_any_call('navidrome_music_folder', 'Audiobooks') + + +def test_setup_client_name_fallback_self_heals_id(nav_client): + """Back-compat: installs saved before the id was persisted match by name, + and the id is written back so a later rename can't break the match.""" + fake_db = _prefs({'navidrome_music_folder_id': None, 'navidrome_music_folder': 'Audiobooks'}) + _connect_with(nav_client, fake_db) + assert nav_client.music_folder_id == '2' + fake_db.set_preference.assert_any_call('navidrome_music_folder_id', '2') def test_navidrome_album_exposes_cover_art_url(nav_client):