fix: merge bare folder into year-suffixed one instead of bailing
When the year-suffixed target folder already exists on disk, both
FolderNamingService and AnimeService.rename_folder_if_needed used to
silently bail out. The bare folder (e.g. 'Ultraman') was left next to
the year-suffixed one ('Ultraman (2019)'), producing the symptom
'reports series like Ultraman as added twice' — the DB has one row but
the filesystem has two folders holding the same content.
Fix: when the target already exists, merge the source's contents into
the target (target version wins on file conflicts; source copies are
removed so cleanup succeeds), remove the now-empty source directory,
update DB row + in-memory cache. Plain rename path is unchanged.
Also fixes a latent TypeError in rename_folder_if_needed where
self._directory (a str) was used with the '/' operator. Production
behavior was that any rename through that method raised and was
swallowed by the caller's try/except, leaving the bare folder
untouched. The new path builds Path objects from the string base.
Tests:
- Replaced test_skips_when_target_folder_already_exists (which
codified the bug) with three tests that cover the new merge
contract: clean merge, no-overwrite, and empty-source removal.
- Added tests/unit/test_rename_folder_if_needed.py covering the
same scenarios plus the str-directory regression. All seven go
red on the unfixed code and green with the fix.
Fixes the 'Ultraman' / 'Ultraman (2019)' duplicate-folder bug.
This commit is contained in:
@@ -1244,8 +1244,7 @@ class AnimeService:
|
||||
Returns:
|
||||
True if rename was performed, False if no rename needed or failed
|
||||
"""
|
||||
import os
|
||||
import shutil
|
||||
from pathlib import Path
|
||||
|
||||
if current_folder == target_folder:
|
||||
logger.debug(
|
||||
@@ -1254,8 +1253,9 @@ class AnimeService:
|
||||
)
|
||||
return False
|
||||
|
||||
current_path = self._directory / current_folder
|
||||
target_path = self._directory / target_folder
|
||||
base_dir = Path(self._directory)
|
||||
current_path = base_dir / current_folder
|
||||
target_path = base_dir / target_folder
|
||||
|
||||
if not current_path.exists():
|
||||
logger.debug(
|
||||
@@ -1265,15 +1265,54 @@ class AnimeService:
|
||||
return False
|
||||
|
||||
if target_path.exists():
|
||||
logger.warning(
|
||||
"Cannot rename folder for %s: target path already exists: %s",
|
||||
key,
|
||||
target_path
|
||||
# Target already exists — merge source into target instead of
|
||||
# bailing. Without this, a bare folder ('Naruto') next to the
|
||||
# year-suffixed one ('Naruto (2019)') would orphan the bare
|
||||
# folder forever, producing the "series added twice" symptom.
|
||||
try:
|
||||
summary = self._merge_folder_into_target(
|
||||
str(current_path), str(target_path)
|
||||
)
|
||||
except Exception as exc:
|
||||
logger.error(
|
||||
"Failed to merge %s -> %s for %s: %s",
|
||||
current_folder, target_folder, key, exc,
|
||||
)
|
||||
return False
|
||||
logger.info(
|
||||
"Merged folder %s -> %s for series %s (moved=%d skipped=%d removed_source=%s)",
|
||||
current_folder, target_folder, key,
|
||||
summary["moved"], summary["skipped"], summary["removed_source"],
|
||||
)
|
||||
return False
|
||||
|
||||
# Update in-memory cache
|
||||
if key in self._app.list.keyDict:
|
||||
self._app.list.keyDict[key].folder = target_folder
|
||||
logger.debug(
|
||||
"Updated in-memory cache folder for %s: %s",
|
||||
key, target_folder
|
||||
)
|
||||
|
||||
# Update database if session provided
|
||||
if db is not None:
|
||||
from src.server.database.service import AnimeSeriesService
|
||||
|
||||
# Look up series by key to get database ID
|
||||
series = await AnimeSeriesService.get_by_key(db, key)
|
||||
if series:
|
||||
await AnimeSeriesService.update(
|
||||
db, series_id=series.id, folder=target_folder
|
||||
)
|
||||
logger.debug(
|
||||
"Updated DB folder for %s: %s",
|
||||
key, target_folder
|
||||
)
|
||||
|
||||
return True
|
||||
|
||||
try:
|
||||
# Rename folder on disk
|
||||
import shutil
|
||||
shutil.move(str(current_path), str(target_path))
|
||||
logger.info(
|
||||
"Renamed folder for %s: %s -> %s",
|
||||
@@ -1317,6 +1356,83 @@ class AnimeService:
|
||||
)
|
||||
return False
|
||||
|
||||
@staticmethod
|
||||
def _merge_folder_into_target(source: str, target: str) -> dict:
|
||||
"""Merge a source folder's contents into an existing target folder.
|
||||
|
||||
Walks the source tree and moves every file into the matching path
|
||||
under the target. When a destination file already exists, the
|
||||
source copy is removed (the target version wins; we don't keep
|
||||
duplicates). When the source tree is fully consumed, the
|
||||
(now-empty) source directory is removed.
|
||||
|
||||
Both paths must be absolute and ``target`` must already exist on
|
||||
disk.
|
||||
|
||||
Returns a summary dict with ``moved`` (file count), ``skipped``
|
||||
(file count where target already had a copy), and
|
||||
``removed_source`` (bool).
|
||||
"""
|
||||
import os
|
||||
import shutil
|
||||
|
||||
if not os.path.isdir(source):
|
||||
return {"moved": 0, "skipped": 0, "removed_source": False}
|
||||
if not os.path.isdir(target):
|
||||
raise ValueError(f"target does not exist: {target}")
|
||||
|
||||
moved = 0
|
||||
skipped = 0
|
||||
for root, _dirs, files in os.walk(source):
|
||||
rel_root = os.path.relpath(root, source)
|
||||
dest_root = (
|
||||
target if rel_root == "."
|
||||
else os.path.join(target, rel_root)
|
||||
)
|
||||
os.makedirs(dest_root, exist_ok=True)
|
||||
for name in files:
|
||||
src_file = os.path.join(root, name)
|
||||
dest_file = os.path.join(dest_root, name)
|
||||
if os.path.exists(dest_file):
|
||||
# Target wins — never overwrite existing content.
|
||||
# Remove the orphaned source copy so cleanup below
|
||||
# can rmdir it.
|
||||
try:
|
||||
os.remove(src_file)
|
||||
except OSError as exc:
|
||||
logger.warning(
|
||||
"merge: could not remove duplicate %s: %s",
|
||||
src_file, exc,
|
||||
)
|
||||
skipped += 1
|
||||
logger.warning(
|
||||
"merge: skipping %s (target already has %s)",
|
||||
src_file, dest_file,
|
||||
)
|
||||
continue
|
||||
shutil.move(src_file, dest_file)
|
||||
moved += 1
|
||||
|
||||
# Try to remove the (now empty) source tree. Walk bottom-up so
|
||||
# leaf directories are removed before their parents.
|
||||
removed_source = False
|
||||
for root, dirs, files in os.walk(source, topdown=False):
|
||||
for d in dirs:
|
||||
try:
|
||||
os.rmdir(os.path.join(root, d))
|
||||
except OSError:
|
||||
pass
|
||||
try:
|
||||
os.rmdir(source)
|
||||
removed_source = True
|
||||
except OSError as exc:
|
||||
logger.warning(
|
||||
"merge: could not remove source directory %s: %s",
|
||||
source, exc,
|
||||
)
|
||||
|
||||
return {"moved": moved, "skipped": skipped, "removed_source": removed_source}
|
||||
|
||||
async def contains_in_db(self, key: str, db) -> bool:
|
||||
"""
|
||||
Check if a series with the given key exists in the database.
|
||||
|
||||
@@ -119,6 +119,79 @@ class FolderNamingService:
|
||||
|
||||
return await self._execute_rename(series, folder, target_folder)
|
||||
|
||||
@staticmethod
|
||||
def _merge_folder_into_target(source: str, target: str) -> dict:
|
||||
"""Merge a source folder's contents into an existing target folder.
|
||||
|
||||
Walks the source tree and moves every file into the matching path under
|
||||
the target. When a destination file already exists, the source copy is
|
||||
removed (the target version wins; we don't keep duplicates). When the
|
||||
source tree is fully consumed, the (now-empty) source directory is
|
||||
removed.
|
||||
|
||||
Both paths must be absolute and ``target`` must already exist on disk.
|
||||
|
||||
Returns a summary dict with ``moved`` (file count), ``skipped`` (file
|
||||
count where target already had a copy), and ``removed_source`` (bool).
|
||||
Caller is responsible for any DB / cache updates that depend on the
|
||||
outcome.
|
||||
"""
|
||||
if not os.path.isdir(source):
|
||||
return {"moved": 0, "skipped": 0, "removed_source": False}
|
||||
if not os.path.isdir(target):
|
||||
raise ValueError(f"target does not exist: {target}")
|
||||
|
||||
moved = 0
|
||||
skipped = 0
|
||||
for root, _dirs, files in os.walk(source):
|
||||
rel_root = os.path.relpath(root, source)
|
||||
dest_root = (
|
||||
target if rel_root == "."
|
||||
else os.path.join(target, rel_root)
|
||||
)
|
||||
os.makedirs(dest_root, exist_ok=True)
|
||||
for name in files:
|
||||
src_file = os.path.join(root, name)
|
||||
dest_file = os.path.join(dest_root, name)
|
||||
if os.path.exists(dest_file):
|
||||
# Target wins — never overwrite existing content. Remove
|
||||
# the orphaned source copy so cleanup below can rmdir it.
|
||||
try:
|
||||
os.remove(src_file)
|
||||
except OSError as exc:
|
||||
logger.warning(
|
||||
"merge: could not remove duplicate %s: %s",
|
||||
src_file, exc,
|
||||
)
|
||||
skipped += 1
|
||||
logger.warning(
|
||||
"merge: skipping %s (target already has %s)",
|
||||
src_file, dest_file,
|
||||
)
|
||||
continue
|
||||
shutil.move(src_file, dest_file)
|
||||
moved += 1
|
||||
|
||||
# Try to remove the (now empty) source tree. Walk bottom-up so leaf
|
||||
# directories are removed before their parents.
|
||||
removed_source = False
|
||||
for root, dirs, files in os.walk(source, topdown=False):
|
||||
for d in dirs:
|
||||
try:
|
||||
os.rmdir(os.path.join(root, d))
|
||||
except OSError:
|
||||
pass
|
||||
try:
|
||||
os.rmdir(source)
|
||||
removed_source = True
|
||||
except OSError as exc:
|
||||
logger.warning(
|
||||
"merge: could not remove source directory %s: %s",
|
||||
source, exc,
|
||||
)
|
||||
|
||||
return {"moved": moved, "skipped": skipped, "removed_source": removed_source}
|
||||
|
||||
async def _execute_rename(self, series, old_folder: str, target_folder: str) -> FolderRenameResult:
|
||||
key = series.key
|
||||
|
||||
@@ -132,16 +205,72 @@ class FolderNamingService:
|
||||
if not os.path.isdir(old_path):
|
||||
return FolderRenameResult(key=key, old_folder=old_folder, new_folder=None, success=False, skipped=False, reason="source folder does not exist on disk")
|
||||
|
||||
# If the target already exists, merge source into it instead of bailing.
|
||||
# A bare folder ('Naruto') sitting next to the year-suffixed one
|
||||
# ('Naruto (2019)') is how we get a series "added twice". Merging
|
||||
# makes the rename succeed and removes the orphan folder.
|
||||
if os.path.isdir(target_path):
|
||||
return FolderRenameResult(key=key, old_folder=old_folder, new_folder=None, success=False, skipped=False, reason="target folder already exists on disk")
|
||||
try:
|
||||
summary = self._merge_folder_into_target(old_path, target_path)
|
||||
except Exception as exc:
|
||||
logger.error(
|
||||
"Failed to merge %s -> %s for %s: %s",
|
||||
old_folder, target_folder, key, exc,
|
||||
)
|
||||
return FolderRenameResult(
|
||||
key=key, old_folder=old_folder, new_folder=None,
|
||||
success=False, skipped=False,
|
||||
reason=f"merge failed: {exc}",
|
||||
)
|
||||
logger.info(
|
||||
"Merged folder %s -> %s for series %s (moved=%d skipped=%d removed_source=%s)",
|
||||
old_folder, target_folder, key,
|
||||
summary["moved"], summary["skipped"], summary["removed_source"],
|
||||
)
|
||||
|
||||
# Update in-memory cache (best-effort)
|
||||
try:
|
||||
from src.server.utils.dependencies import get_series_app
|
||||
series_app = get_series_app()
|
||||
if key in series_app.list.keyDict:
|
||||
series_app.list.keyDict[key].folder = target_folder
|
||||
except Exception as exc:
|
||||
logger.warning("Failed to update in-memory cache for %s: %s", key, exc)
|
||||
|
||||
# Update database
|
||||
async with _get_db_session() as db:
|
||||
db_series = await AnimeSeriesService.get_by_key(db, key)
|
||||
if db_series:
|
||||
await AnimeSeriesService.update(db, series_id=db_series.id, folder=target_folder)
|
||||
logger.debug("Updated DB folder for %s to %s", key, target_folder)
|
||||
|
||||
# If source couldn't be removed (still had unexpected files) the
|
||||
# state is worse than the original orphan, so surface that as a
|
||||
# warning in the result while still reporting success.
|
||||
note = None
|
||||
if not summary["removed_source"]:
|
||||
note = (
|
||||
f"merged (moved={summary['moved']}, skipped={summary['skipped']}) "
|
||||
f"but source folder could not be removed"
|
||||
)
|
||||
elif summary["skipped"]:
|
||||
note = (
|
||||
f"merged (moved={summary['moved']}, "
|
||||
f"kept target copies for {summary['skipped']} file(s))"
|
||||
)
|
||||
return FolderRenameResult(
|
||||
key=key, old_folder=old_folder, new_folder=target_folder,
|
||||
success=True, skipped=False, reason=note,
|
||||
)
|
||||
|
||||
# Target doesn't exist — plain rename.
|
||||
try:
|
||||
shutil.move(old_path, target_path)
|
||||
logger.info("Renamed folder %s -> %s for series %s", old_folder, target_folder, key)
|
||||
|
||||
# Update in-memory cache
|
||||
try:
|
||||
from src.server.SeriesApp import get_series_app
|
||||
from src.server.utils.dependencies import get_series_app
|
||||
series_app = get_series_app()
|
||||
if key in series_app.list.keyDict:
|
||||
series_app.list.keyDict[key].folder = target_folder
|
||||
|
||||
Reference in New Issue
Block a user