library reorganize: add a rename-only executor (#875) — core + tests
#875 (tsoulard/Tacobell444): the reorganize job runs the FULL download post-processing on every track — copy to staging, re-tag, quality + AcoustID checks, then move. So it fails on the same checks as downloads, is slow (a full copy per file on a NAS, not a rename), and re-touches EVERY file even when only its name changes (Tacobell's "2 of 14 previewed but all 14 modified"). This adds the rename-only path users actually want for "just fix the filenames": move each file to the path the current naming scheme dictates and nothing else — no copy, no re-tag, no checks. The tags are already correct; only the on-disk filename/folder layout changes (their hardware DAP sorts by filename). Design (additive — the full flow is byte-for-byte untouched): - preview_album_reorganize gains current_path_abs / new_path_abs (additive fields; existing trimmed display paths unchanged) so the executor acts on EXACTLY what the preview computed — apply can never disagree with what the user saw. - reorganize_album_rename_only: consumes the preview (injected preview_fn for testability), and for each track that's matched + actually changing + non-colliding, renames in place and updates the SoulSync DB directly (authoritative — we just did the move, no need to round-trip a server scan). unchanged tracks are SKIPPED — that's the fix for "every file got modified". - _rename_track_in_place: os.rename with a cross-device (EXDEV) fallback to shutil.move, creates the dest dir, carries sibling-format files (.flac+.opus) along, and refuses to overwrite a different existing file (never silent data loss). 11 new tests incl. the headline regression (changed → moved + DB updated, unchanged → untouched), collision/unmatched skip, overwrite-refusal, sibling carry, cross-device, stop, cleanup. 207 reorganize/library tests green, ruff clean. Endpoint flag + modal + post-rename server scan next.
This commit is contained in:
BoulderBadgeDad
parent
34adb6fb32
commit
92c101e300
2 changed files with 321 additions and 1 deletions
|
|
@ -26,6 +26,7 @@ without a source ID are reported back to the caller and skipped
|
|||
entirely.
|
||||
"""
|
||||
|
||||
import errno
|
||||
import os
|
||||
import re
|
||||
import shutil
|
||||
|
|
@ -34,7 +35,7 @@ import time
|
|||
import uuid
|
||||
from concurrent.futures import FIRST_COMPLETED, ThreadPoolExecutor, wait
|
||||
from dataclasses import dataclass, field
|
||||
from typing import Any, Callable, Dict, List, Optional, Set
|
||||
from typing import Any, Callable, Dict, List, Optional, Set, Tuple
|
||||
|
||||
# Per-album track concurrency. Matches the download workers' per-batch
|
||||
# concurrency (3) so reorganize feels comparable to a fresh download.
|
||||
|
|
@ -1101,6 +1102,12 @@ def preview_album_reorganize(
|
|||
'track_number': track.get('track_number', 0),
|
||||
'current_path': _trim_to_transfer(db_path, resolved, transfer_dir),
|
||||
'new_path': '',
|
||||
# Absolute on-disk paths (additive). `current_path`/`new_path` above are
|
||||
# display-trimmed; these carry the real paths so the rename-only executor
|
||||
# acts on EXACTLY what the preview computed — no separate path logic that
|
||||
# could drift from what the user saw (#875).
|
||||
'current_path_abs': resolved or '',
|
||||
'new_path_abs': '',
|
||||
'file_exists': resolved is not None,
|
||||
'unchanged': False,
|
||||
'collision': False,
|
||||
|
|
@ -1148,6 +1155,7 @@ def preview_album_reorganize(
|
|||
new_full, _ok = build_final_path_fn(
|
||||
context, spotify_artist, album_info, file_ext, create_dirs=False
|
||||
)
|
||||
item['new_path_abs'] = new_full or ''
|
||||
item['new_path'] = (
|
||||
os.path.relpath(new_full, transfer_dir)
|
||||
if transfer_dir and new_full and new_full.startswith(transfer_dir)
|
||||
|
|
@ -1807,6 +1815,150 @@ def reorganize_album(
|
|||
return summary
|
||||
|
||||
|
||||
def _rename_track_in_place(current_abs: str, new_abs: str) -> Tuple[bool, Optional[str]]:
|
||||
"""Move ONE file from ``current_abs`` to ``new_abs`` in place — no copy, no re-tag,
|
||||
no post-processing. Creates the destination folder, carries sibling-format files
|
||||
(e.g. a lossy ``.opus`` alongside the ``.flac``) along with the renamed stem, and
|
||||
falls back to a cross-device move when the rename crosses a filesystem boundary.
|
||||
|
||||
Refuses to overwrite a DIFFERENT existing file at the destination (returns an error
|
||||
instead) — never silent data loss. Returns ``(ok, error_message)``.
|
||||
"""
|
||||
try:
|
||||
if current_abs and not os.path.exists(current_abs):
|
||||
return False, 'source file no longer on disk'
|
||||
same = os.path.normpath(current_abs) == os.path.normpath(new_abs)
|
||||
if os.path.exists(new_abs) and not same:
|
||||
return False, 'destination already exists'
|
||||
os.makedirs(os.path.dirname(new_abs), exist_ok=True)
|
||||
# Carry sibling-format audio to the same destination with the renamed stem —
|
||||
# mirrors _finalize_track so lossy-copy pairs don't get orphaned.
|
||||
for sibling_src in _find_sibling_audio_files(current_abs):
|
||||
_move_sibling_to_destination(sibling_src, new_abs)
|
||||
try:
|
||||
os.rename(current_abs, new_abs)
|
||||
except OSError as e:
|
||||
if getattr(e, 'errno', None) == errno.EXDEV:
|
||||
shutil.move(current_abs, new_abs) # crosses a filesystem boundary
|
||||
else:
|
||||
raise
|
||||
return True, None
|
||||
except Exception as e:
|
||||
return False, str(e)
|
||||
|
||||
|
||||
def reorganize_album_rename_only(
|
||||
*,
|
||||
album_id: str,
|
||||
db,
|
||||
transfer_dir: str,
|
||||
resolve_file_path_fn: Callable[[Optional[str]], Optional[str]],
|
||||
build_final_path_fn: Callable,
|
||||
update_track_path_fn: Optional[Callable[[object, str], None]] = None,
|
||||
cleanup_empty_dir_fn: Optional[Callable[[str], None]] = None,
|
||||
on_progress: Optional[Callable[[dict], None]] = None,
|
||||
primary_source: Optional[str] = None,
|
||||
strict_source: bool = False,
|
||||
metadata_source: str = 'api',
|
||||
stop_check: Optional[Callable[[], bool]] = None,
|
||||
preview_fn: Optional[Callable] = None,
|
||||
) -> dict:
|
||||
"""RENAME-ONLY reorganize (#875): move each track's file to the path the current
|
||||
naming scheme dictates, and nothing else — no copy-to-staging, no re-tag, no
|
||||
quality/AcoustID checks.
|
||||
|
||||
It acts on EXACTLY what :func:`preview_album_reorganize` computed (injected via
|
||||
``preview_fn`` for testability), so the apply can never disagree with what the user
|
||||
saw, and ONLY files whose path actually changes are touched — files marked
|
||||
``unchanged`` are skipped, which is what keeps a rename from rewriting the whole
|
||||
album (the #875 complaint). Tags and audio are left byte-for-byte alone.
|
||||
|
||||
Returns the same summary shape as :func:`reorganize_album`.
|
||||
"""
|
||||
preview_fn = preview_fn or preview_album_reorganize
|
||||
summary = {
|
||||
'status': 'completed', 'source': None, 'total': 0,
|
||||
'moved': 0, 'skipped': 0, 'failed': 0, 'errors': [],
|
||||
}
|
||||
|
||||
def _emit(**updates):
|
||||
if on_progress is None:
|
||||
return
|
||||
try:
|
||||
on_progress(updates)
|
||||
except Exception as e:
|
||||
logger.debug("[Reorganize/rename] progress emit failed: %s", e)
|
||||
|
||||
preview = preview_fn(
|
||||
album_id=album_id, db=db, transfer_dir=transfer_dir,
|
||||
resolve_file_path_fn=resolve_file_path_fn,
|
||||
build_final_path_fn=build_final_path_fn,
|
||||
primary_source=primary_source, strict_source=strict_source,
|
||||
metadata_source=metadata_source,
|
||||
)
|
||||
summary['source'] = preview.get('source')
|
||||
if not preview.get('success'):
|
||||
summary['status'] = preview.get('status', 'error')
|
||||
return summary
|
||||
|
||||
tracks = preview.get('tracks', [])
|
||||
summary['total'] = len(tracks)
|
||||
src_dirs_touched: Set[str] = set()
|
||||
|
||||
for t in tracks:
|
||||
if stop_check and stop_check():
|
||||
break
|
||||
title = t.get('title', 'Unknown')
|
||||
_emit(current_track=title)
|
||||
|
||||
# Skip anything that isn't a real, changing move. `unchanged` is the key one —
|
||||
# it's why a rename no longer rewrites files whose name didn't change.
|
||||
if (not t.get('matched') or t.get('unchanged')
|
||||
or t.get('collision') or not t.get('new_path_abs')):
|
||||
summary['skipped'] += 1
|
||||
_emit(skipped=summary['skipped'])
|
||||
continue
|
||||
|
||||
current_abs = t.get('current_path_abs')
|
||||
new_abs = t.get('new_path_abs')
|
||||
ok, err = _rename_track_in_place(current_abs, new_abs)
|
||||
if not ok:
|
||||
summary['failed'] += 1
|
||||
summary['errors'].append({
|
||||
'track_id': t.get('track_id'), 'title': title,
|
||||
'error': err or 'rename failed',
|
||||
})
|
||||
_emit(failed=summary['failed'], errors=list(summary['errors']))
|
||||
continue
|
||||
|
||||
# File is at its new home — update the DB directly (authoritative; no need to
|
||||
# round-trip through a server scan to learn what we just did). On DB failure the
|
||||
# file still moved; a library scan reconciles it, so we don't fail the track.
|
||||
if update_track_path_fn:
|
||||
try:
|
||||
update_track_path_fn(t.get('track_id'), new_abs)
|
||||
except Exception as db_err:
|
||||
logger.warning(
|
||||
"[Reorganize/rename] DB path update failed for %s: %s "
|
||||
"(file moved to %s; a scan will reconcile)",
|
||||
t.get('track_id'), db_err, new_abs,
|
||||
)
|
||||
if current_abs:
|
||||
src_dirs_touched.add(os.path.dirname(current_abs))
|
||||
summary['moved'] += 1
|
||||
_emit(moved=summary['moved'],
|
||||
processed=summary['moved'] + summary['skipped'] + summary['failed'])
|
||||
|
||||
if cleanup_empty_dir_fn:
|
||||
for src_dir in src_dirs_touched:
|
||||
try:
|
||||
cleanup_empty_dir_fn(src_dir)
|
||||
except Exception as e:
|
||||
logger.debug("[Reorganize/rename] cleanup of %s failed: %s", src_dir, e)
|
||||
|
||||
return summary
|
||||
|
||||
|
||||
def _find_artist_dir(dest_path: str, transfer_dir: str) -> Optional[str]:
|
||||
"""Walk up from ``dest_path`` until the parent equals ``transfer_dir``;
|
||||
the directory at that point is the artist folder. Returns None if
|
||||
|
|
|
|||
168
tests/test_reorganize_rename_only.py
Normal file
168
tests/test_reorganize_rename_only.py
Normal file
|
|
@ -0,0 +1,168 @@
|
|||
"""Rename-only reorganize (#875): move files to the current naming scheme with NO
|
||||
copy / re-tag / post-processing. The headline guarantee is that it acts on exactly
|
||||
what the preview computed and ONLY touches files whose path actually changes — files
|
||||
the preview marked `unchanged` are left alone (the "every file got modified" bug).
|
||||
"""
|
||||
|
||||
import os
|
||||
|
||||
from core.library_reorganize import (
|
||||
_rename_track_in_place,
|
||||
reorganize_album_rename_only,
|
||||
)
|
||||
|
||||
|
||||
# ── _rename_track_in_place ──
|
||||
|
||||
def test_rename_moves_file_and_creates_dest_dir(tmp_path):
|
||||
src = tmp_path / "old" / "01 - Song.flac"
|
||||
src.parent.mkdir(parents=True)
|
||||
src.write_bytes(b"audio")
|
||||
dst = tmp_path / "new" / "Song - Artist.flac"
|
||||
|
||||
ok, err = _rename_track_in_place(str(src), str(dst))
|
||||
assert ok and err is None
|
||||
assert dst.exists() and dst.read_bytes() == b"audio"
|
||||
assert not src.exists()
|
||||
|
||||
|
||||
def test_rename_refuses_to_overwrite_a_different_file(tmp_path):
|
||||
src = tmp_path / "a.flac"
|
||||
src.write_bytes(b"source")
|
||||
dst = tmp_path / "b.flac"
|
||||
dst.write_bytes(b"someone else") # a DIFFERENT existing file
|
||||
|
||||
ok, err = _rename_track_in_place(str(src), str(dst))
|
||||
assert not ok and "exists" in err
|
||||
assert src.exists() and dst.read_bytes() == b"someone else" # nothing destroyed
|
||||
|
||||
|
||||
def test_rename_missing_source_errors(tmp_path):
|
||||
ok, err = _rename_track_in_place(str(tmp_path / "gone.flac"), str(tmp_path / "x.flac"))
|
||||
assert not ok and "no longer on disk" in err
|
||||
|
||||
|
||||
def test_rename_same_path_is_noop_ok(tmp_path):
|
||||
f = tmp_path / "x.flac"
|
||||
f.write_bytes(b"a")
|
||||
ok, err = _rename_track_in_place(str(f), str(f))
|
||||
assert ok and f.exists()
|
||||
|
||||
|
||||
def test_rename_carries_sibling_format_file(tmp_path):
|
||||
# lossy-copy pair: canonical .flac + sibling .opus in the same folder
|
||||
src = tmp_path / "old" / "01 - Song.flac"
|
||||
src.parent.mkdir(parents=True)
|
||||
src.write_bytes(b"flac")
|
||||
sib = tmp_path / "old" / "01 - Song.opus"
|
||||
sib.write_bytes(b"opus")
|
||||
dst = tmp_path / "new" / "Song.flac"
|
||||
|
||||
ok, _ = _rename_track_in_place(str(src), str(dst))
|
||||
assert ok
|
||||
assert dst.exists()
|
||||
assert (tmp_path / "new" / "Song.opus").exists() # sibling came along, renamed stem
|
||||
|
||||
|
||||
# ── reorganize_album_rename_only (fake preview injected) ──
|
||||
|
||||
def _fake_preview(tracks, *, success=True, status="planned", source="deezer"):
|
||||
def _preview(**_kw):
|
||||
return {"success": success, "status": status, "source": source, "tracks": tracks}
|
||||
return _preview
|
||||
|
||||
|
||||
def _run(tracks, *, update=None, cleanup=None, stop=None, **preview_kw):
|
||||
return reorganize_album_rename_only(
|
||||
album_id="A1", db=None, transfer_dir="/x",
|
||||
resolve_file_path_fn=lambda p: p,
|
||||
build_final_path_fn=lambda *a, **k: (None, True),
|
||||
update_track_path_fn=update,
|
||||
cleanup_empty_dir_fn=cleanup,
|
||||
stop_check=stop,
|
||||
preview_fn=_fake_preview(tracks, **preview_kw),
|
||||
)
|
||||
|
||||
|
||||
def test_moves_changed_and_skips_unchanged(tmp_path):
|
||||
"""THE regression: a changed track moves + DB updates; an `unchanged` track is
|
||||
left completely alone (not re-touched). This is the #875 fix in one test."""
|
||||
src = tmp_path / "old" / "01 - A.flac"
|
||||
src.parent.mkdir(parents=True)
|
||||
src.write_bytes(b"a")
|
||||
new = tmp_path / "new" / "A - Artist.flac"
|
||||
keep = tmp_path / "keep" / "B.flac"
|
||||
keep.parent.mkdir(parents=True)
|
||||
keep.write_bytes(b"b")
|
||||
|
||||
updates = []
|
||||
summary = _run(
|
||||
[
|
||||
{"track_id": "t1", "title": "A", "matched": True, "unchanged": False,
|
||||
"collision": False, "current_path_abs": str(src), "new_path_abs": str(new)},
|
||||
{"track_id": "t2", "title": "B", "matched": True, "unchanged": True,
|
||||
"collision": False, "current_path_abs": str(keep), "new_path_abs": str(keep)},
|
||||
],
|
||||
update=lambda tid, path: updates.append((tid, path)),
|
||||
)
|
||||
|
||||
assert summary["moved"] == 1 and summary["skipped"] == 1 and summary["failed"] == 0
|
||||
assert new.exists() and not src.exists() # changed track moved
|
||||
assert keep.exists() and keep.read_bytes() == b"b" # unchanged: untouched
|
||||
assert updates == [("t1", str(new))] # DB updated ONLY for the moved one
|
||||
|
||||
|
||||
def test_collision_and_unmatched_are_skipped(tmp_path):
|
||||
summary = _run([
|
||||
{"track_id": "c", "title": "C", "matched": True, "unchanged": False,
|
||||
"collision": True, "current_path_abs": "/a", "new_path_abs": "/b"},
|
||||
{"track_id": "u", "title": "U", "matched": False, "unchanged": False,
|
||||
"collision": False, "current_path_abs": "/a", "new_path_abs": "/b"},
|
||||
])
|
||||
assert summary["skipped"] == 2 and summary["moved"] == 0 and summary["failed"] == 0
|
||||
|
||||
|
||||
def test_failed_rename_is_counted_not_fatal(tmp_path):
|
||||
src = tmp_path / "a.flac"
|
||||
src.write_bytes(b"src")
|
||||
dst = tmp_path / "taken.flac"
|
||||
dst.write_bytes(b"occupied") # forces "destination already exists"
|
||||
summary = _run([
|
||||
{"track_id": "t", "title": "T", "matched": True, "unchanged": False,
|
||||
"collision": False, "current_path_abs": str(src), "new_path_abs": str(dst)},
|
||||
])
|
||||
assert summary["failed"] == 1 and summary["moved"] == 0
|
||||
assert summary["errors"] and summary["errors"][0]["track_id"] == "t"
|
||||
assert src.exists() and dst.read_bytes() == b"occupied" # nothing lost
|
||||
|
||||
|
||||
def test_preview_failure_returns_its_status():
|
||||
summary = _run([], success=False, status="no_source_id")
|
||||
assert summary["status"] == "no_source_id"
|
||||
assert summary["moved"] == 0
|
||||
|
||||
|
||||
def test_stop_check_aborts_early(tmp_path):
|
||||
src = tmp_path / "a.flac"
|
||||
src.write_bytes(b"a")
|
||||
summary = _run(
|
||||
[{"track_id": "t", "title": "T", "matched": True, "unchanged": False,
|
||||
"collision": False, "current_path_abs": str(src), "new_path_abs": str(tmp_path / "b.flac")}],
|
||||
stop=lambda: True,
|
||||
)
|
||||
assert summary["moved"] == 0 # aborted before processing
|
||||
assert src.exists()
|
||||
|
||||
|
||||
def test_cleanup_called_for_emptied_source_dirs(tmp_path):
|
||||
src = tmp_path / "old" / "01 - A.flac"
|
||||
src.parent.mkdir(parents=True)
|
||||
src.write_bytes(b"a")
|
||||
cleaned = []
|
||||
_run(
|
||||
[{"track_id": "t1", "title": "A", "matched": True, "unchanged": False,
|
||||
"collision": False, "current_path_abs": str(src),
|
||||
"new_path_abs": str(tmp_path / "new" / "A.flac")}],
|
||||
cleanup=lambda d: cleaned.append(d),
|
||||
)
|
||||
assert str(tmp_path / "old") in cleaned
|
||||
Loading…
Reference in a new issue