From 3b49ac8280b267fc554ce694570317ea5f7f0d93 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 2 Jun 2026 10:32:06 -0700 Subject: [PATCH] Fix #767: Library Organizer dry run no longer creates folders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reorganize preview (dry run) was physically creating destination album folders, littering the library with empty dirs and making "changes" before the user ever hit Apply. Cause: preview_album_reorganize calls build_final_path_for_track purely to COMPUTE the destination path string — but that shared helper has 9 os.makedirs side effects (it's also the live download/import path builder, where creating the dir is correct). So computing the preview path created "Lenka (Expanded Edition)/" on disk. Fix: build_final_path_for_track gains create_dirs=True; all 9 makedirs now route through a gated helper. The reorganize PREVIEW passes create_dirs=False, so a dry run computes the exact destination path with zero filesystem side effects. Everything else keeps the default True: - the download/import post-process flow (still writes files into the dir), - retag, - the reorganize APPLY path — verified it goes through post_process_fn (the real pipeline → build_final_path_for_track with create_dirs=True), so live moves still create their destination dirs. The gate only silences the dry run. Tests: tests/imports/test_import_paths.py — create_dirs=False computes the correct path (matching the reported "01 - The Show.flac") but writes NOTHING to disk (not even the Transfer root); create_dirs=True still creates folders; both yield an identical path. Updated two reorganize-orchestrator test doubles to accept the new kwarg. 148 reorganize/paths/retag/pipeline tests pass. Does NOT fix the second half of #767 (Expanded Edition picked over the standard album). That is NOT a reorganizer bug: the library album row was linked to the deluxe release at enrichment time (its stored spotify_album_id/itunes_album_id/ deezer_id points at "Lenka (Expanded Edition)"), and the reorganizer faithfully reorganizes to whatever the album is linked to. The real fix is in album enrichment's edition preference — tracked separately. --- core/imports/paths.py | 34 +++++--- core/library_reorganize.py | 7 +- tests/imports/test_import_paths.py | 78 +++++++++++++++++++ tests/test_library_reorganize_orchestrator.py | 7 +- 4 files changed, 111 insertions(+), 15 deletions(-) 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