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.
This commit is contained in:
parent
cf655c5009
commit
85b6ddb997
2 changed files with 76 additions and 26 deletions
|
|
@ -224,7 +224,10 @@ class NavidromeClient(MediaServerClient):
|
||||||
logger.info(f"Set music folder to: {folder_name} (ID: {self.music_folder_id})")
|
logger.info(f"Set music folder to: {folder_name} (ID: {self.music_folder_id})")
|
||||||
from database.music_database import MusicDatabase
|
from database.music_database import MusicDatabase
|
||||||
db = 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', folder_name)
|
||||||
|
db.set_preference('navidrome_music_folder_id', folder['key'])
|
||||||
return True
|
return True
|
||||||
# If folder_name is empty, clear the selection
|
# If folder_name is empty, clear the selection
|
||||||
if not folder_name:
|
if not folder_name:
|
||||||
|
|
@ -233,6 +236,7 @@ class NavidromeClient(MediaServerClient):
|
||||||
from database.music_database import MusicDatabase
|
from database.music_database import MusicDatabase
|
||||||
db = MusicDatabase()
|
db = MusicDatabase()
|
||||||
db.set_preference('navidrome_music_folder', '')
|
db.set_preference('navidrome_music_folder', '')
|
||||||
|
db.set_preference('navidrome_music_folder_id', '')
|
||||||
logger.info("Cleared music folder selection — will use all libraries")
|
logger.info("Cleared music folder selection — will use all libraries")
|
||||||
return True
|
return True
|
||||||
logger.warning(f"Music folder '{folder_name}' not found")
|
logger.warning(f"Music folder '{folder_name}' not found")
|
||||||
|
|
@ -284,18 +288,35 @@ class NavidromeClient(MediaServerClient):
|
||||||
try:
|
try:
|
||||||
from database.music_database import MusicDatabase
|
from database.music_database import MusicDatabase
|
||||||
db = MusicDatabase()
|
db = MusicDatabase()
|
||||||
|
saved_id = db.get_preference('navidrome_music_folder_id')
|
||||||
saved_folder = db.get_preference('navidrome_music_folder')
|
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
|
# Use the non-reentrant fetch: we're still inside
|
||||||
# ensure_connection() here (_is_connecting=True), so the
|
# ensure_connection() here (_is_connecting=True), so the
|
||||||
# public get_music_folders() would re-enter the guard and
|
# public get_music_folders() would re-enter the guard and
|
||||||
# return [], silently dropping the saved selection.
|
# return [], silently dropping the saved selection.
|
||||||
folders = self._fetch_music_folders()
|
folders = self._fetch_music_folders()
|
||||||
for folder in folders:
|
# Match by id first (stable across renames in Navidrome);
|
||||||
if folder['title'] == saved_folder:
|
# fall back to name for installs saved before the id was
|
||||||
self.music_folder_id = folder['key']
|
# persisted.
|
||||||
logger.info(f"Restored music folder preference: {saved_folder} (ID: {self.music_folder_id})")
|
matched = None
|
||||||
break
|
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:
|
except Exception as e:
|
||||||
logger.warning(f"Could not restore music folder preference: {e}")
|
logger.warning(f"Could not restore music folder preference: {e}")
|
||||||
else:
|
else:
|
||||||
|
|
|
||||||
|
|
@ -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):
|
def test_setup_client_restores_saved_music_folder(nav_client):
|
||||||
"""Regression for #789: a saved music-folder selection must survive
|
"""Regression for #789: a saved music-folder selection must survive
|
||||||
_setup_client. The restore runs while still inside ensure_connection()
|
_setup_client. The restore runs while still inside ensure_connection()
|
||||||
(_is_connecting=True); the old code called the public get_music_folders(),
|
(_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
|
which re-entered the guard, got [], and left music_folder_id=None — so
|
||||||
every scan imported all libraries regardless of the user's selection."""
|
every scan imported all libraries regardless of the user's selection."""
|
||||||
def fake_request(endpoint, params=None):
|
fake_db = _prefs({'navidrome_music_folder_id': '2', 'navidrome_music_folder': 'Audiobooks'})
|
||||||
if endpoint == 'ping':
|
_connect_with(nav_client, fake_db)
|
||||||
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.
|
|
||||||
assert nav_client.music_folder_id == '2'
|
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):
|
def test_navidrome_album_exposes_cover_art_url(nav_client):
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue