diff --git a/core/imports/paths.py b/core/imports/paths.py index af9619d7..5744744e 100644 --- a/core/imports/paths.py +++ b/core/imports/paths.py @@ -418,8 +418,20 @@ def _coerce_int(value: Any, default: int = 1) -> int: return coerced if coerced > 0 else default -def build_final_path_for_track(context, artist_context, album_info, file_ext): - """Shared path builder used by both post-processing and verification.""" +def build_final_path_for_track(context, artist_context, album_info, file_ext, create_dirs: bool = True): + """Shared path builder used by both post-processing and verification. + + ``create_dirs`` gates the directory-creation side effects. The download + import flow leaves it True (it's about to write the file there). The + library-reorganize PREVIEW passes False so a dry run can compute the exact + destination path WITHOUT physically creating the folder — fixes #767 (dry + run was leaving empty destination folders behind).""" + _real_makedirs = os.makedirs + + def _ensure_dir(path, **_kw): + if create_dirs: + _real_makedirs(path, exist_ok=True) + transfer_dir = docker_resolve_path(_get_config_manager().get("soulseek.transfer_path", "./Transfer")) context = normalize_import_context(context) track_info = get_import_track_info(context) @@ -440,7 +452,7 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): original_dir = os.path.dirname(original_path) original_stem = os.path.splitext(os.path.basename(original_path))[0] final_path = os.path.join(original_dir, original_stem + file_ext) - os.makedirs(original_dir, exist_ok=True) + _ensure_dir(original_dir, exist_ok=True) logger.info("[Enhance] Using original file location: %s", final_path) return final_path, True @@ -477,12 +489,12 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): folder_path, filename_base = get_file_path_from_template(template_context, "playlist_path") if folder_path and filename_base: final_path = os.path.join(transfer_dir, folder_path, filename_base + file_ext) - os.makedirs(os.path.join(transfer_dir, folder_path), exist_ok=True) + _ensure_dir(os.path.join(transfer_dir, folder_path), exist_ok=True) return final_path, True playlist_name_sanitized = sanitize_filename(playlist_name) playlist_dir = os.path.join(transfer_dir, playlist_name_sanitized) - os.makedirs(playlist_dir, exist_ok=True) + _ensure_dir(playlist_dir, exist_ok=True) artist_name_sanitized = sanitize_filename(template_context["artist"]) track_name_sanitized = sanitize_filename(track_name) new_filename = f"{artist_name_sanitized} - {track_name_sanitized}{file_ext}" @@ -579,10 +591,10 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): if total_discs > 1 and not user_controls_disc: disc_folder = f"{disc_label} {disc_number}" final_path = os.path.join(transfer_dir, folder_path, disc_folder, filename_base + file_ext) - os.makedirs(os.path.join(transfer_dir, folder_path, disc_folder), exist_ok=True) + _ensure_dir(os.path.join(transfer_dir, folder_path, disc_folder), exist_ok=True) else: final_path = os.path.join(transfer_dir, folder_path, filename_base + file_ext) - os.makedirs(os.path.join(transfer_dir, folder_path), exist_ok=True) + _ensure_dir(os.path.join(transfer_dir, folder_path), exist_ok=True) return final_path, True artist_name_sanitized = sanitize_filename(template_context["albumartist"]) @@ -592,7 +604,7 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): album_dir = os.path.join(artist_dir, album_folder_name) if total_discs > 1: album_dir = os.path.join(album_dir, f"{disc_label} {disc_number}") - os.makedirs(album_dir, exist_ok=True) + _ensure_dir(album_dir, exist_ok=True) final_track_name_sanitized = sanitize_filename(clean_track_name) new_filename = f"{track_number:02d} - {final_track_name_sanitized}{file_ext}" return os.path.join(album_dir, new_filename), True @@ -629,10 +641,10 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): if filename_base: if folder_path: final_path = os.path.join(transfer_dir, folder_path, filename_base + file_ext) - os.makedirs(os.path.join(transfer_dir, folder_path), exist_ok=True) + _ensure_dir(os.path.join(transfer_dir, folder_path), exist_ok=True) else: final_path = os.path.join(transfer_dir, filename_base + file_ext) - os.makedirs(transfer_dir, exist_ok=True) + _ensure_dir(transfer_dir, exist_ok=True) return final_path, True artist_name_sanitized = sanitize_filename(template_context["artist"]) @@ -640,6 +652,6 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): artist_dir = os.path.join(transfer_dir, artist_name_sanitized) single_folder_name = f"{artist_name_sanitized} - {final_track_name_sanitized}" single_dir = os.path.join(artist_dir, single_folder_name) - os.makedirs(single_dir, exist_ok=True) + _ensure_dir(single_dir, exist_ok=True) new_filename = f"{final_track_name_sanitized}{file_ext}" return os.path.join(single_dir, new_filename), True diff --git a/core/library_reorganize.py b/core/library_reorganize.py index af795789..25adcda4 100644 --- a/core/library_reorganize.py +++ b/core/library_reorganize.py @@ -989,7 +989,12 @@ def preview_album_reorganize( album_info = _build_album_info(context) try: spotify_artist = context['spotify_artist'] - new_full, _ok = build_final_path_fn(context, spotify_artist, album_info, file_ext) + # Dry run: compute the destination path WITHOUT creating the folder. + # Previously this physically created the album dir during preview, + # leaving empty folders all over the library (#767). + new_full, _ok = build_final_path_fn( + context, spotify_artist, album_info, file_ext, create_dirs=False + ) item['new_path'] = ( os.path.relpath(new_full, transfer_dir) if transfer_dir and new_full and new_full.startswith(transfer_dir) diff --git a/tests/imports/test_import_paths.py b/tests/imports/test_import_paths.py index ec94881d..ee4fd1b6 100644 --- a/tests/imports/test_import_paths.py +++ b/tests/imports/test_import_paths.py @@ -17,6 +17,84 @@ def test_sanitize_filename_replaces_illegal_characters(): assert import_paths.sanitize_filename("AUX.txt").startswith("_") +# ── #767: dry-run path build must not create folders ────────────────────── + +def _album_path_config(tmp_path): + return _Config({ + "soulseek.transfer_path": str(tmp_path / "Transfer"), + "file_organization.enabled": True, + "file_organization.templates": { + "album_path": "$albumartist/$albumartist - $album/$track - $title", + "single_path": "$artist/$artist - $title", + }, + "file_organization.collab_artist_mode": "first", + "file_organization.disc_label": "Disc", + }) + + +def _album_context(): + return { + "artist": {"name": "Lenka"}, + "album": {"name": "Lenka", "id": "album-1", "release_date": "2008-01-01", + "total_tracks": 12, "album_type": "album", "artists": [{"name": "Lenka"}]}, + "track_info": {"name": "The Show", "id": "t1", "track_number": 1, + "disc_number": 1, "artists": [{"name": "Lenka"}]}, + "original_search_result": {"title": "The Show", "clean_title": "The Show", + "clean_album": "Lenka", "clean_artist": "Lenka", + "artists": [{"name": "Lenka"}]}, + "source": "deezer", "is_album_download": False, + } + + +def test_create_dirs_false_does_not_create_folders(monkeypatch, tmp_path): + monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _album_path_config(tmp_path)) + monkeypatch.setattr(import_paths, "_get_album_tracks_for_source", lambda *a: None) + + final_path, created = import_paths.build_final_path_for_track( + _album_context(), {"name": "Lenka"}, + {"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1}, + ".flac", create_dirs=False, + ) + + # Path is still computed correctly... + assert created is True + assert final_path == str(tmp_path / "Transfer" / "Lenka" / "Lenka - Lenka" / "01 - The Show.flac") + # ...but NOTHING was written to disk — not even the Transfer root. + assert not (tmp_path / "Transfer").exists() + + +def test_create_dirs_true_still_creates_folders(monkeypatch, tmp_path): + # The download/import flow must keep working (default behavior unchanged). + monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _album_path_config(tmp_path)) + monkeypatch.setattr(import_paths, "_get_album_tracks_for_source", lambda *a: None) + + final_path, created = import_paths.build_final_path_for_track( + _album_context(), {"name": "Lenka"}, + {"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1}, + ".flac", # create_dirs defaults True + ) + + assert created is True + assert (tmp_path / "Transfer" / "Lenka" / "Lenka - Lenka").is_dir() + + +def test_create_dirs_false_and_true_yield_identical_path(monkeypatch, tmp_path): + monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _album_path_config(tmp_path)) + monkeypatch.setattr(import_paths, "_get_album_tracks_for_source", lambda *a: None) + + dry, _ = import_paths.build_final_path_for_track( + _album_context(), {"name": "Lenka"}, + {"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1}, + ".flac", create_dirs=False, + ) + live, _ = import_paths.build_final_path_for_track( + _album_context(), {"name": "Lenka"}, + {"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1}, + ".flac", create_dirs=True, + ) + assert dry == live + + def test_build_simple_download_destination_uses_album_folder(monkeypatch, tmp_path): config = _Config({"soulseek.transfer_path": str(tmp_path / "Transfer")}) monkeypatch.setattr(import_paths, "_get_config_manager", lambda: config) diff --git a/tests/test_library_reorganize_orchestrator.py b/tests/test_library_reorganize_orchestrator.py index e4045b9f..0b068a7a 100644 --- a/tests/test_library_reorganize_orchestrator.py +++ b/tests/test_library_reorganize_orchestrator.py @@ -1163,9 +1163,10 @@ def test_keeps_album_sidecars_when_a_track_failed_to_move(monkeypatch, tmpdirs): # --- preview function (shared planning with the orchestrator) ----------- -def _fake_path_builder(context, spotify_artist, _album_info, file_ext): +def _fake_path_builder(context, spotify_artist, _album_info, file_ext, **_kw): """Stand-in for `_build_final_path_for_track`. Inserts Disc N/ when - total_discs > 1 — same convention the real builder uses.""" + total_discs > 1 — same convention the real builder uses. Accepts + **_kw so the preview's create_dirs=False kwarg (#767) passes through.""" album = context['spotify_album']['name'] artist = spotify_artist['name'] track_info = context['track_info'] @@ -1180,7 +1181,7 @@ def _fake_path_builder(context, spotify_artist, _album_info, file_ext): return '/'.join(parts), True -def _path_builder_album_vs_single(context, spotify_artist, album_info, file_ext): +def _path_builder_album_vs_single(context, spotify_artist, album_info, file_ext, **_kw): """Stand-in that emulates the real `_build_final_path_for_track` branch on `album_info.get('is_album')`. ALBUM mode produces an album folder with disc subfolder + numbered file; SINGLE mode