diff --git a/project_docs/BATCH_DIRECTORY_MOVE_V1_DESIGN.md b/project_docs/BATCH_DIRECTORY_MOVE_V1_DESIGN.md new file mode 100644 index 0000000..ffb0e7c --- /dev/null +++ b/project_docs/BATCH_DIRECTORY_MOVE_V1_DESIGN.md @@ -0,0 +1,190 @@ +# Batch Directory Move v1 + +## 1. Scope + +Doel van batch directory move v1 is het gecontroleerd ondersteunen van verplaatsingen voor meerdere geselecteerde items via de bestaande move/task-flow, zonder de huidige file-flow te destabiliseren. + +Aanbevolen v1-scope: +- alleen same-root batch move +- meerdere directories ondersteunen +- gemengde selectie van files + directories ondersteunen, maar alleen binnen dezelfde root +- geen cross-root batch directory move +- geen rollback +- geen recursive copy-delete move voor directories +- geen partial rename-semantiek in batchmodus + +Niet in scope: +- cross-root batch directory move +- batch move met meerdere roots tegelijk +- batch move met conflictoplossing UI +- cancel/retry +- rollback bij gedeeltelijk succes +- hernoemen per item binnen batch-popup + +## 2. Selectiesituaties + +### Meerdere directories +- Toegestaan in v1, mits alle geselecteerde directories binnen dezelfde root vallen. +- Bestemming is de current path van het inactieve paneel. +- Elk item krijgt als doelpad: `destination_base + / + item.name`. + +### Meerdere files + directories gemengd +- Toegestaan in v1, mits alle geselecteerde items binnen dezelfde root vallen. +- Files en directories worden in dezelfde batch verwerkt. +- Elk item gebruikt dezelfde destination-map semantiek. + +### 1 directory + meerdere files +- Valt onder gemengde selectie en is dus toegestaan onder dezelfde same-root-regels. + +### Niet toegestaan in v1 +- selectie die items uit verschillende roots combineert +- selectie waarbij voor een directory het doelpad in de eigen subtree valt +- selectie met symlink-source-items +- selectie waarbij een doelpad al bestaat + +## 3. Same-root versus cross-root + +Aanbevolen v1-keuze: +- batch directory move blijft beperkt tot same-root +- cross-root batch directory move wordt expliciet geblokkeerd + +Motivatie: +- same-root kan native rename/move gebruiken en sluit aan op de huidige directory move v1 +- cross-root voor directories vereist recursieve copy + delete en introduceert veel meer partial-failure-risico +- gemengde batch over roots maakt progress, foutafhandeling en rollback aanzienlijk complexer + +Geblokkeerde melding in v1: +- `Cross-root batch directory move is not supported in v1` + +## 4. Taskmodel + +Aanbevolen richting: +- één task voor de hele batch + +Motivatie: +- sluit beter aan op de gebruikersactie: één batch-confirmatie, één batchresultaat +- voorkomt een explosie van losse tasks bij grote selecties +- maakt batchprogress en foutsamenvatting eenvoudiger zichtbaar + +Progress: +- `done_items` / `total_items` zijn leidend +- `total_items` = aantal geselecteerde top-level items in de batch +- `done_items` telt per succesvol verplaatst top-level item op +- `done_bytes` / `total_bytes` blijven `null` in v1 voor batch directory move + - ook bij gemengde selectie is het verstandig in v1 item-gebaseerde progress leidend te houden + +Failure-semantiek: +- fail-fast binnen de batchtask +- bij eerste runtime-fout stopt verdere verwerking +- task eindigt als `failed` +- reeds verplaatste items blijven verplaatst +- geen rollback in v1 + +Partial completion model: +- partial completion wordt impliciet zichtbaar via `done_items < total_items` +- `failed_item` bevat het item waarop de batch stopte +- geen aparte `partial_completed` status in v1 + +## 5. Backend-impact + +Hergebruik: +- bestaande move-validatie voor individuele file/directory move +- bestaande task persistence en read-endpoints +- bestaande same-root native move in filesystem adapter +- bestaande path_guard containment en symlink-afwijzing + +Waarschijnlijke wijzigingen: +- move service: batchvalidatie en task-creatie voor meerdere items +- task runner: batch move worker +- filesystem adapter: hergebruik van bestaande file move en directory move primitives +- task repository: waarschijnlijk geen schemawijziging nodig als `done_items`, `total_items`, `current_item`, `failed_item` al volstaan + +Belangrijk semantisch behoud: +- rename voor exact 1 item blijft apart via bestaande rename-flow/F6-popupbeslislogica +- batchmodus is altijd move, nooit rename + +## 6. UI-impact + +Gedrag voor Move-knop en F6: +- bij meerdere geselecteerde items mag de bestaande batch move-confirmatie worden hergebruikt +- die confirmatie moet duidelijk tonen: + - aantal geselecteerde items + - destination-map + - dat batch same-root vereist voor directories +- voor gemengde selectie moet dezelfde batch-confirmatie bruikbaar blijven + +Benodigde meldingen: +- `Cross-root batch directory move is not supported in v1` +- `Batch move requires all selected items to be in the same root` +- `Destination already exists for one or more items` +- `Destination cannot be inside source` +- `Source must not be a symlink` + +Aanbeveling: +- hergebruik bestaande batch confirm-popup +- geen extra popup-type toevoegen in v1 +- alleen de validatietekst en submit-logica uitbreiden + +## 7. Security en validatie + +Per item moeten minimaal deze checks gelden: +- source ligt binnen whitelist +- destination-parent ligt binnen whitelist +- source is geen symlink +- destination bestaat nog niet +- directory destination ligt niet in subtree van source +- same-root verplicht voor directories in batch v1 + +Gemengde foutgevallen: +- als selectie meerdere roots bevat: directe validatiefout vóór task-creatie +- als één item ongeldig is: hele batchcreatie afwijzen vóór task-creatie +- geen "best effort" pre-validatie waarbij geldige items toch alvast starten + +Containment: +- path_guard blijft leidend voor alle bron- en doelpaden +- geen vrije pathconstructie buiten bestaande root mapping + +## 8. Teststrategie + +Golden tests: +- batch same-root directories success +- batch same-root mixed files + directories success +- batch cross-root directories blocked +- batch mixed-root selection blocked +- batch destination exists blocked +- batch destination inside source blocked +- batch symlink source blocked +- batch task failed shape bij runtime io_error + +Regressietests: +- exacte 1 file move-flow blijft ongewijzigd +- exacte 1 directory same-root move blijft werken +- batch file-only move blijft werken +- F6 rename/move voor exact 1 item blijft semantisch gelijk + +Securitytests: +- traversal in één van de source paths +- traversal in destination base +- symlink directory source +- symlink file source binnen gemengde selectie + +Runtime edge cases: +- eerste item succeeds, tweede faalt -> task failed met `done_items == 1` +- destination conflict ontdekt vóór task-creatie +- empty selection blijft no-op in UI, geen backend-aanroep + +## 9. Aanbeveling + +Aanbevolen v1-richting met laag regressierisico: +- ondersteun alleen same-root batch move +- ondersteun gemengde selectie van files + directories alleen als alle items in dezelfde root zitten +- gebruik één task per batch +- gebruik item-based progress (`done_items` / `total_items`) +- fail-fast zonder rollback +- hergebruik bestaande batch move-confirmatie in de UI + +Waarom deze richting: +- bouwt rechtstreeks voort op de huidige same-root directory move v1 en batch file move +- beperkt backendcomplexiteit tot validatie + batchrunner, zonder cross-root recursieve directorylogica +- houdt de UI voorspelbaar: één batchactie, één task, één resultaat +- minimaliseert regressierisico voor de bestaande single-item file-flow en F6-semantiek diff --git a/project_docs/DIRECTORY_MOVE_V1_DESIGN.md b/project_docs/DIRECTORY_MOVE_V1_DESIGN.md new file mode 100644 index 0000000..1c33931 --- /dev/null +++ b/project_docs/DIRECTORY_MOVE_V1_DESIGN.md @@ -0,0 +1,308 @@ +# DIRECTORY_MOVE_V1_DESIGN.md + +## 1. Scope + +Doel van Directory Move v1 is om directory-verplaatsing toe te voegen binnen het bestaande move/task/securitymodel, zonder het huidige veiligheidsniveau of de voorspelbaarheid van de backend te verzwakken. + +In scope voor v1: +- directory move als expliciete uitbreiding van de bestaande move-operatie +- task-based uitvoering +- conflictcontrole op destination +- beveiligde padvalidatie via bestaand `path_guard` +- duidelijke foutstatussen bij ongeldige of gevaarlijke moves + +Out of scope voor v1: +- geen rollbackmechanisme +- geen cancel/retry +- geen gedeeltelijke recovery van half-voltooide cross-root moves +- geen symlink-following buiten whitelist +- geen speciale merge- of overwrite-semantiek +- geen multi-select directory move als aparte batch-semantiek buiten bestaande sequentiële task-aanmaak + +--- + +## 2. Same-root versus cross-root + +### Directory move binnen dezelfde root + +Dit is de veiligste en simpelste stap. + +Kenmerken: +- kan meestal native via `rename()`/`move` op directoryniveau +- snel en atomair binnen hetzelfde filesystem/root +- geen inhoudelijke recursieve file copy nodig +- veel minder kans op partial failure + +### Directory move tussen verschillende roots + +Dit is aanzienlijk complexer. + +Kenmerken: +- vereist recursieve copy van directory tree +- daarna delete van source tree +- progressmeting en foutafhandeling worden lastiger +- partial failure is realistischer +- rollback is ingewikkeld en in v1 ongewenst + +### Aanbevolen scopekeuze + +Aanbevolen v1: +- **alleen same-root directory move ondersteunen** +- **cross-root directory move nog niet ondersteunen** + +Reden: +- sluit goed aan op bestaand taskmodel zonder grote complexiteitsexplosie +- beperkt regressierisico +- voorkomt een half-robuste recursieve copy+delete-implementatie zonder rollback + +--- + +## 3. Semantiek + +### Wanneer is het een echte move + +Directory move is van toepassing als: +- source een directory is +- destination een volledig doelpad is +- destination nog niet bestaat + +Binnen same-root: +- de operatie gebruikt native rename/move op directoryniveau +- dit geldt zowel voor verplaatsen naar andere parent als voor directory move binnen dezelfde root met andere naam + +### Destination bestaat al + +Gedrag v1: +- destination mag niet bestaan +- response: `already_exists` + +Geen merge in v1. + +### Nested destinations + +Verboden gevallen: +- source naar een child van zichzelf +- source naar een pad in eigen subtree + +Voorbeeld: +- source: `/Volumes/8TB/Shows` +- destination: `/Volumes/8TB/Shows/Archive/Shows` + +Dit moet direct geblokkeerd worden. + +Aanbevolen fout: +- `invalid_request` met duidelijke boodschap zoals `Destination cannot be inside source` + +### Verplaatsen van map in zichzelf + +Ook verboden: +- destination exact gelijk aan source +- destination onder source subtree + +Dit moet vóór task-creatie worden afgewezen. + +--- + +## 4. Taskmodel + +Directory move blijft task-based, ook voor same-root moves. + +### Progressmeting + +Voor same-root native directory move is echte byte-progress meestal niet beschikbaar. + +Aanbevolen v1: +- gebruik vooral `done_items` / `total_items` +- voor same-root kan dat minimaal zijn: + - `total_items = 1` + - `done_items = 0 -> 1` +- `done_bytes` / `total_bytes` mogen `null` blijven in v1 voor same-root directory move + +### Fail-fast gedrag + +Aanbevolen v1: +- fail-fast +- zodra de move-operatie faalt, task -> `failed` + +### Partial failure + +Voor same-root native directory move is partial failure veel minder waarschijnlijk. +- of de rename slaagt +- of de rename faalt + +Dus v1-model: +- geen expliciete partial-successstatus +- eindstatus blijft: + - `completed` + - `failed` + +### Rollback + +Geen rollback in v1. + +Voor same-root native rename is dat meestal niet nodig. +Voor cross-root zou rollback wel relevant worden, maar die scope wordt niet aanbevolen voor v1. + +--- + +## 5. Security + +### Symlinkgedrag in directory trees + +Aanbevolen v1-beleid: +- source directory zelf mag geen symlink zijn +- bij same-root native rename wordt de tree niet recursief gevolgd of gekopieerd, dus child-symlinks hoeven niet actief gevolgd te worden voor uitvoering +- toch blijft containmentcontrole op source en destination verplicht + +### Traversal en containment + +- bron- en destination-pad blijven via bestaand `path_guard` +- destination binnen whitelist verplicht +- destination mag niet buiten root escapen +- nested destination-in-source moet expliciet extra gevalideerd worden + +### PathGuard-impact + +Bestaand `path_guard` blijft basis voor: +- whitelistvalidatie +- traversalblokkade +- symlink containment check op bron/destination-resolutie + +Aanvullend nodig voor directory move: +- helper of servicecheck om te bepalen of destination in subtree van source ligt + +### Escapes buiten whitelist voorkomen + +Bij same-root-only v1: +- containment blijft relatief eenvoudig +- geen recursieve copy over child nodes nodig +- dus ook minder attack surface dan cross-root recursieve tree copy + +--- + +## 6. Backend-impact + +Waarschijnlijk te wijzigen delen: +- `webui/backend/app/services/move_task_service.py` +- `webui/backend/app/tasks_runner.py` +- `webui/backend/app/fs/filesystem_adapter.py` +- mogelijk `webui/backend/app/api/schemas.py` alleen als taskdetail-documentatie wordt aangescherpt +- mogelijk kleine aanvulling in `path_guard.py` of move-service validatiehelpers + +### Bestaande move-logica die hergebruikt kan worden + +Herbruikbaar: +- task-creatie +- repository-persistency +- statusflow `queued -> running -> completed/failed` +- destination-exists checks +- parent directory validation +- algemene error mapping + +Nieuw voor directory move: +- source typecheck moet directory toestaan in same-root-case +- same-root task runner moet directory rename kunnen uitvoeren +- nested-destination-validatie + +### Rename binnen zelfde parent blijft apart + +Ja. + +Aanbevolen scheiding: +- `rename` endpoint blijft aparte, directe single-path operatie +- `move` blijft task-based voor echte moves +- ook als een same-root directory move technisch op rename lijkt, blijft het semantisch onderdeel van `move` + +Dat houdt UI- en API-rollen duidelijk. + +--- + +## 7. UI-impact + +### Move-knop en F6 bij exact 1 directory + +Aanbevolen gedrag: +- F6 / Move-knop blijven dezelfde popupflow gebruiken +- exact 1 directory geselecteerd: + - zelfde parent + andere naam -> huidige `rename` route blijft toegestaan + - andere parent binnen dezelfde root -> task-based directory move toegestaan in v1 + - cross-root destination -> blokkeren met duidelijke melding, bijvoorbeeld: + - `Cross-root directory move is not supported in v1` + +### Multi-select met directories + +Aanbevolen v1: +- geen gemengde halfslimme batchflow +- als multi-select directories bevat: + - alleen toestaan als alle geselecteerde directories voldoen aan same-root move-semantiek, of + - eenvoudiger en veiliger voor v1: nog blokkeren met duidelijke melding + +Aanbevolen eerste stap: +- **multi-select met directories nog blokkeren** +- melding: + - `Batch directory move is not supported in v1` + +Reden: +- beperkt scope en regressierisico +- houdt UI-flow voorspelbaar + +--- + +## 8. Teststrategie + +### Golden tests + +Toe te voegen voor move-API: +- same-root directory move success +- directory destination exists -> `already_exists` +- directory source not found +- directory source is symlink -> blokkade +- nested destination blocked +- exact same source/destination blocked +- cross-root directory move blocked + +### Regressietests + +- file move success same-root blijft werken +- file move success cross-root blijft werken als huidige scope dat al ondersteunt +- rename endpoint blijft ongewijzigd +- browse en delete blijven ongewijzigd + +### Securitytests + +- traversal source +- traversal destination +- symlink source rejection +- destination inside source rejection +- destination outside whitelist rejection + +### Runtime edge cases + +- rename/move van lege directory +- rename/move van directory met inhoud binnen same-root +- permission failure -> `io_error` +- destination parent ontbreekt -> bestaande foutmapping + +--- + +## 9. Aanbeveling + +Aanbevolen v1-richting: +- **alleen same-root directory move ondersteunen** +- **cross-root directory move nog niet ondersteunen** + +Korte motivatie: +- laagste complexiteit +- beste aansluiting op bestaand taskmodel +- minimale partial-failure kans +- veel kleiner regressie- en securityrisico dan recursieve cross-root tree copy+delete + +Praktische v1-scope: +- exact 1 directory +- same-root move task-based +- nested destination geblokkeerd +- destination exists geblokkeerd +- cross-root directory move expliciet geweigerd +- batch directory move nog niet + +Dat is de veiligste eerste stap met duidelijke semantiek en goede testbaarheid. diff --git a/webui/backend/app/__pycache__/tasks_runner.cpython-313.pyc b/webui/backend/app/__pycache__/tasks_runner.cpython-313.pyc index d152c9d..b700bb7 100644 Binary files a/webui/backend/app/__pycache__/tasks_runner.cpython-313.pyc and b/webui/backend/app/__pycache__/tasks_runner.cpython-313.pyc differ diff --git a/webui/backend/app/db/__pycache__/task_repository.cpython-313.pyc b/webui/backend/app/db/__pycache__/task_repository.cpython-313.pyc index c1bb4be..94db469 100644 Binary files a/webui/backend/app/db/__pycache__/task_repository.cpython-313.pyc and b/webui/backend/app/db/__pycache__/task_repository.cpython-313.pyc differ diff --git a/webui/backend/app/db/task_repository.py b/webui/backend/app/db/task_repository.py index a0d26c8..fa870f8 100644 --- a/webui/backend/app/db/task_repository.py +++ b/webui/backend/app/db/task_repository.py @@ -125,39 +125,62 @@ class TaskRepository: ).fetchall() return [self._to_dict(row) for row in rows] - def mark_running(self, task_id: str, done_bytes: int, total_bytes: int | None, current_item: str | None) -> None: + def mark_running( + self, + task_id: str, + done_bytes: int | None = None, + total_bytes: int | None = None, + done_items: int | None = None, + total_items: int | None = None, + current_item: str | None = None, + ) -> None: started_at = self._now_iso() with self._connection() as conn: conn.execute( """ UPDATE tasks - SET status = ?, started_at = ?, done_bytes = ?, total_bytes = ?, current_item = ? + SET status = ?, started_at = ?, done_bytes = ?, total_bytes = ?, done_items = ?, total_items = ?, current_item = ? WHERE id = ? """, - ("running", started_at, done_bytes, total_bytes, current_item, task_id), + ("running", started_at, done_bytes, total_bytes, done_items, total_items, current_item, task_id), ) - def update_progress(self, task_id: str, done_bytes: int, total_bytes: int | None, current_item: str | None) -> None: + def update_progress( + self, + task_id: str, + done_bytes: int | None = None, + total_bytes: int | None = None, + done_items: int | None = None, + total_items: int | None = None, + current_item: str | None = None, + ) -> None: with self._connection() as conn: conn.execute( """ UPDATE tasks - SET done_bytes = ?, total_bytes = ?, current_item = ? + SET done_bytes = ?, total_bytes = ?, done_items = ?, total_items = ?, current_item = ? WHERE id = ? """, - (done_bytes, total_bytes, current_item, task_id), + (done_bytes, total_bytes, done_items, total_items, current_item, task_id), ) - def mark_completed(self, task_id: str, done_bytes: int | None, total_bytes: int | None) -> None: + def mark_completed( + self, + task_id: str, + done_bytes: int | None = None, + total_bytes: int | None = None, + done_items: int | None = None, + total_items: int | None = None, + ) -> None: finished_at = self._now_iso() with self._connection() as conn: conn.execute( """ UPDATE tasks - SET status = ?, finished_at = ?, done_bytes = ?, total_bytes = ? + SET status = ?, finished_at = ?, done_bytes = ?, total_bytes = ?, done_items = ?, total_items = ? WHERE id = ? """, - ("completed", finished_at, done_bytes, total_bytes, task_id), + ("completed", finished_at, done_bytes, total_bytes, done_items, total_items, task_id), ) def mark_failed( @@ -168,16 +191,29 @@ class TaskRepository: failed_item: str | None, done_bytes: int | None, total_bytes: int | None, + done_items: int | None = None, + total_items: int | None = None, ) -> None: finished_at = self._now_iso() with self._connection() as conn: conn.execute( """ UPDATE tasks - SET status = ?, finished_at = ?, error_code = ?, error_message = ?, failed_item = ?, done_bytes = ?, total_bytes = ? + SET status = ?, finished_at = ?, error_code = ?, error_message = ?, failed_item = ?, done_bytes = ?, total_bytes = ?, done_items = ?, total_items = ? WHERE id = ? """, - ("failed", finished_at, error_code, error_message, failed_item, done_bytes, total_bytes, task_id), + ( + "failed", + finished_at, + error_code, + error_message, + failed_item, + done_bytes, + total_bytes, + done_items, + total_items, + task_id, + ), ) def _ensure_schema(self) -> None: diff --git a/webui/backend/app/fs/__pycache__/filesystem_adapter.cpython-313.pyc b/webui/backend/app/fs/__pycache__/filesystem_adapter.cpython-313.pyc index 830ceb9..47f54d9 100644 Binary files a/webui/backend/app/fs/__pycache__/filesystem_adapter.cpython-313.pyc and b/webui/backend/app/fs/__pycache__/filesystem_adapter.cpython-313.pyc differ diff --git a/webui/backend/app/fs/filesystem_adapter.py b/webui/backend/app/fs/filesystem_adapter.py index 4d431be..87ac3a4 100644 --- a/webui/backend/app/fs/filesystem_adapter.py +++ b/webui/backend/app/fs/filesystem_adapter.py @@ -38,6 +38,9 @@ class FilesystemAdapter: def move_file(self, source: str, destination: str) -> None: Path(source).rename(Path(destination)) + def move_directory(self, source: str, destination: str) -> None: + Path(source).rename(Path(destination)) + def is_directory_empty(self, path: Path) -> bool: return not any(path.iterdir()) diff --git a/webui/backend/app/services/__pycache__/move_task_service.cpython-313.pyc b/webui/backend/app/services/__pycache__/move_task_service.cpython-313.pyc index dec6bc1..e06c762 100644 Binary files a/webui/backend/app/services/__pycache__/move_task_service.cpython-313.pyc and b/webui/backend/app/services/__pycache__/move_task_service.cpython-313.pyc differ diff --git a/webui/backend/app/services/move_task_service.py b/webui/backend/app/services/move_task_service.py index 1bf30b9..6ccec9d 100644 --- a/webui/backend/app/services/move_task_service.py +++ b/webui/backend/app/services/move_task_service.py @@ -1,5 +1,7 @@ from __future__ import annotations +from pathlib import Path + from backend.app.api.errors import AppError from backend.app.api.schemas import TaskCreateResponse from backend.app.db.task_repository import TaskRepository @@ -20,14 +22,16 @@ class MoveTaskService: if lexical_source.is_symlink(): raise AppError( code="type_conflict", - message="Source must be a regular file", + message="Source must not be a symlink", status_code=409, details={"path": source}, ) - if not resolved_source.absolute.is_file(): + source_is_file = resolved_source.absolute.is_file() + source_is_directory = resolved_source.absolute.is_dir() + if not source_is_file and not source_is_directory: raise AppError( code="type_conflict", - message="Source must be a file", + message="Unsupported source path type", status_code=409, details={"path": source}, ) @@ -41,6 +45,14 @@ class MoveTaskService: ) self._map_directory_validation(parent_relative) + if source_is_directory and resolved_destination.absolute == resolved_source.absolute: + raise AppError( + code="invalid_request", + message="Destination must differ from source", + status_code=400, + details={"path": source, "destination": destination}, + ) + if resolved_destination.absolute.exists(): raise AppError( code="already_exists", @@ -49,6 +61,36 @@ class MoveTaskService: details={"path": resolved_destination.relative}, ) + same_root = resolved_source.alias == resolved_destination.alias + + if source_is_directory: + if not same_root: + raise AppError( + code="invalid_request", + message="Cross-root directory move is not supported in v1", + status_code=400, + details={"path": source, "destination": destination}, + ) + if self._is_nested_destination(resolved_source.absolute, resolved_destination.absolute): + raise AppError( + code="invalid_request", + message="Destination cannot be inside source", + status_code=400, + details={"path": source, "destination": destination}, + ) + + task = self._repository.create_task( + operation="move", + source=resolved_source.relative, + destination=resolved_destination.relative, + ) + self._runner.enqueue_move_directory( + task_id=task["id"], + source=str(resolved_source.absolute), + destination=str(resolved_destination.absolute), + ) + return TaskCreateResponse(task_id=task["id"], status=task["status"]) + total_bytes = int(resolved_source.absolute.stat().st_size) task = self._repository.create_task( operation="move", @@ -56,7 +98,6 @@ class MoveTaskService: destination=resolved_destination.relative, ) - same_root = resolved_source.alias == resolved_destination.alias self._runner.enqueue_move_file( task_id=task["id"], source=str(resolved_source.absolute), @@ -79,3 +120,11 @@ class MoveTaskService: details=exc.details, ) raise + + @staticmethod + def _is_nested_destination(source: Path, destination: Path) -> bool: + try: + destination.relative_to(source) + return True + except ValueError: + return False diff --git a/webui/backend/app/tasks_runner.py b/webui/backend/app/tasks_runner.py index 8b3c0e3..2111370 100644 --- a/webui/backend/app/tasks_runner.py +++ b/webui/backend/app/tasks_runner.py @@ -35,6 +35,14 @@ class TaskRunner: ) thread.start() + def enqueue_move_directory(self, task_id: str, source: str, destination: str) -> None: + thread = threading.Thread( + target=self._run_move_directory, + args=(task_id, source, destination), + daemon=True, + ) + thread.start() + def _run_copy_file(self, task_id: str, source: str, destination: str, total_bytes: int) -> None: self._repository.mark_running( task_id=task_id, @@ -123,3 +131,28 @@ class TaskRunner: done_bytes=progress["done"], total_bytes=total_bytes, ) + + def _run_move_directory(self, task_id: str, source: str, destination: str) -> None: + self._repository.mark_running( + task_id=task_id, + done_items=0, + total_items=1, + current_item=source, + ) + + try: + self._filesystem.move_directory(source=source, destination=destination) + self._repository.mark_completed( + task_id=task_id, + done_items=1, + total_items=1, + ) + except OSError as exc: + self._repository.mark_failed( + task_id=task_id, + error_code="io_error", + error_message=str(exc), + failed_item=source, + done_items=0, + total_items=1, + ) diff --git a/webui/backend/data/tasks.db b/webui/backend/data/tasks.db index 83e172a..f33208e 100644 Binary files a/webui/backend/data/tasks.db and b/webui/backend/data/tasks.db differ diff --git a/webui/backend/tests/golden/__pycache__/test_api_move_golden.cpython-313.pyc b/webui/backend/tests/golden/__pycache__/test_api_move_golden.cpython-313.pyc index 4c8f2f0..0dda84c 100644 Binary files a/webui/backend/tests/golden/__pycache__/test_api_move_golden.cpython-313.pyc and b/webui/backend/tests/golden/__pycache__/test_api_move_golden.cpython-313.pyc differ diff --git a/webui/backend/tests/golden/__pycache__/test_ui_smoke_golden.cpython-313.pyc b/webui/backend/tests/golden/__pycache__/test_ui_smoke_golden.cpython-313.pyc index b6847a3..f9f49eb 100644 Binary files a/webui/backend/tests/golden/__pycache__/test_ui_smoke_golden.cpython-313.pyc and b/webui/backend/tests/golden/__pycache__/test_ui_smoke_golden.cpython-313.pyc differ diff --git a/webui/backend/tests/golden/test_api_move_golden.py b/webui/backend/tests/golden/test_api_move_golden.py index 4d211fd..1d083e5 100644 --- a/webui/backend/tests/golden/test_api_move_golden.py +++ b/webui/backend/tests/golden/test_api_move_golden.py @@ -96,6 +96,33 @@ class MoveApiGoldenTest(unittest.TestCase): self.assertTrue((self.root1 / "moved.txt").exists()) self.assertFalse(src.exists()) + def test_move_directory_success_same_root_and_completed(self) -> None: + src_dir = self.root1 / "source-dir" + src_dir.mkdir() + (src_dir / "nested.txt").write_text("hello", encoding="utf-8") + target_parent = self.root1 / "target-parent" + target_parent.mkdir() + + response = self._request( + "POST", + "/api/files/move", + {"source": "storage1/source-dir", "destination": "storage1/target-parent/moved-dir"}, + ) + + self.assertEqual(response.status_code, 202) + body = response.json() + self.assertEqual(body["status"], "queued") + + detail = self._wait_task(body["task_id"]) + self.assertEqual(detail["status"], "completed") + self.assertEqual(detail["done_items"], 1) + self.assertEqual(detail["total_items"], 1) + self.assertIsNone(detail["done_bytes"]) + self.assertIsNone(detail["total_bytes"]) + self.assertTrue((self.root1 / "target-parent" / "moved-dir").is_dir()) + self.assertTrue((self.root1 / "target-parent" / "moved-dir" / "nested.txt").exists()) + self.assertFalse(src_dir.exists()) + def test_move_success_cross_root_create_task_shape_and_completed(self) -> None: src = self.root1 / "source.txt" src.write_text("hello", encoding="utf-8") @@ -116,6 +143,19 @@ class MoveApiGoldenTest(unittest.TestCase): self.assertTrue((self.root2 / "moved.txt").exists()) self.assertFalse(src.exists()) + def test_move_directory_cross_root_blocked(self) -> None: + src_dir = self.root1 / "source-dir" + src_dir.mkdir() + + response = self._request( + "POST", + "/api/files/move", + {"source": "storage1/source-dir", "destination": "storage2/source-dir"}, + ) + + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json()["error"]["code"], "invalid_request") + def test_move_source_not_found(self) -> None: response = self._request( "POST", @@ -126,13 +166,24 @@ class MoveApiGoldenTest(unittest.TestCase): self.assertEqual(response.status_code, 404) self.assertEqual(response.json()["error"]["code"], "path_not_found") - def test_move_source_is_directory_type_conflict(self) -> None: + def test_move_directory_source_not_found(self) -> None: + response = self._request( + "POST", + "/api/files/move", + {"source": "storage1/missing-dir", "destination": "storage1/out-dir"}, + ) + + self.assertEqual(response.status_code, 404) + self.assertEqual(response.json()["error"]["code"], "path_not_found") + + def test_move_source_is_directory_type_conflict_for_file_destination_parent(self) -> None: (self.root1 / "dir").mkdir() + (self.root1 / "out.txt").write_text("x", encoding="utf-8") response = self._request( "POST", "/api/files/move", - {"source": "storage1/dir", "destination": "storage1/out.txt"}, + {"source": "storage1/dir", "destination": "storage1/out.txt/child"}, ) self.assertEqual(response.status_code, 409) @@ -151,6 +202,19 @@ class MoveApiGoldenTest(unittest.TestCase): self.assertEqual(response.status_code, 409) self.assertEqual(response.json()["error"]["code"], "already_exists") + def test_move_directory_destination_exists_already_exists(self) -> None: + (self.root1 / "source-dir").mkdir() + (self.root1 / "target-dir").mkdir() + + response = self._request( + "POST", + "/api/files/move", + {"source": "storage1/source-dir", "destination": "storage1/target-dir"}, + ) + + self.assertEqual(response.status_code, 409) + self.assertEqual(response.json()["error"]["code"], "already_exists") + def test_move_traversal_source(self) -> None: response = self._request( "POST", @@ -173,6 +237,33 @@ class MoveApiGoldenTest(unittest.TestCase): self.assertEqual(response.status_code, 403) self.assertEqual(response.json()["error"]["code"], "path_traversal_detected") + def test_move_directory_destination_inside_source_blocked(self) -> None: + src_dir = self.root1 / "source-dir" + src_dir.mkdir() + (src_dir / "child").mkdir() + + response = self._request( + "POST", + "/api/files/move", + {"source": "storage1/source-dir", "destination": "storage1/source-dir/child/moved-dir"}, + ) + + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json()["error"]["code"], "invalid_request") + + def test_move_directory_same_source_destination_blocked(self) -> None: + src_dir = self.root1 / "source-dir" + src_dir.mkdir() + + response = self._request( + "POST", + "/api/files/move", + {"source": "storage1/source-dir", "destination": "storage1/source-dir"}, + ) + + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json()["error"]["code"], "invalid_request") + def test_move_source_symlink_rejected(self) -> None: target = self.root1 / "real.txt" target.write_text("x", encoding="utf-8") @@ -188,6 +279,22 @@ class MoveApiGoldenTest(unittest.TestCase): self.assertEqual(response.status_code, 409) self.assertEqual(response.json()["error"]["code"], "type_conflict") + def test_move_directory_source_symlink_rejected(self) -> None: + target = self.root1 / "real-dir" + target.mkdir() + (target / "nested.txt").write_text("x", encoding="utf-8") + link = self.root1 / "dir-link" + link.symlink_to(target, target_is_directory=True) + + response = self._request( + "POST", + "/api/files/move", + {"source": "storage1/dir-link", "destination": "storage1/out-dir"}, + ) + + self.assertEqual(response.status_code, 409) + self.assertEqual(response.json()["error"]["code"], "type_conflict") + def test_move_runtime_io_error_failed_task_shape(self) -> None: src = self.root1 / "source.txt" src.write_text("hello", encoding="utf-8") diff --git a/webui/backend/tests/golden/test_ui_smoke_golden.py b/webui/backend/tests/golden/test_ui_smoke_golden.py index e735a93..92cf661 100644 --- a/webui/backend/tests/golden/test_ui_smoke_golden.py +++ b/webui/backend/tests/golden/test_ui_smoke_golden.py @@ -82,6 +82,10 @@ class UiSmokeGoldenTest(unittest.TestCase): self.assertTrue((static_root / "style.css").exists()) app_js = (static_root / "app.js").read_text(encoding="utf-8") self.assertIn('currentPath: "/Volumes"', app_js) + self.assertIn('Cross-root directory move is not supported in v1', app_js) + self.assertIn('Batch directory move is not supported in v1', app_js) + self.assertIn("function rootKeyFromPath(path)", app_js) + self.assertIn("function isNestedPath(sourcePath, destinationPath)", app_js) app_js_url = app.url_path_for("ui", path="/app.js") style_css_url = app.url_path_for("ui", path="/style.css") diff --git a/webui/backend/tests/unit/__pycache__/test_move_task_service.cpython-313.pyc b/webui/backend/tests/unit/__pycache__/test_move_task_service.cpython-313.pyc new file mode 100644 index 0000000..3ebd16b Binary files /dev/null and b/webui/backend/tests/unit/__pycache__/test_move_task_service.cpython-313.pyc differ diff --git a/webui/backend/tests/unit/test_move_task_service.py b/webui/backend/tests/unit/test_move_task_service.py new file mode 100644 index 0000000..20d77c7 --- /dev/null +++ b/webui/backend/tests/unit/test_move_task_service.py @@ -0,0 +1,27 @@ +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[3])) + +from backend.app.services.move_task_service import MoveTaskService + + +class MoveTaskServiceTest(unittest.TestCase): + def test_is_nested_destination_true_for_child_path(self) -> None: + source = Path("/tmp/source") + destination = source / "child" / "target" + + self.assertTrue(MoveTaskService._is_nested_destination(source, destination)) + + def test_is_nested_destination_false_for_sibling_path(self) -> None: + source = Path("/tmp/source") + destination = Path("/tmp/other/target") + + self.assertFalse(MoveTaskService._is_nested_destination(source, destination)) + + +if __name__ == "__main__": + unittest.main() diff --git a/webui/html/app.js b/webui/html/app.js index 8c7cd42..36aa2a4 100644 --- a/webui/html/app.js +++ b/webui/html/app.js @@ -259,6 +259,30 @@ function baseName(path) { return index >= 0 ? path.slice(index + 1) : path; } +function rootKeyFromPath(path) { + const normalized = (path || "").trim(); + if (!normalized) { + return null; + } + if (normalized.startsWith("/")) { + const segments = normalized.split("/").filter(Boolean); + if (segments.length < 2) { + return normalized; + } + return `/${segments[0]}/${segments[1]}`; + } + return normalized.split("/")[0]; +} + +function isNestedPath(sourcePath, destinationPath) { + const source = (sourcePath || "").replace(/\/+$/, ""); + const destination = (destinationPath || "").replace(/\/+$/, ""); + if (!source || !destination) { + return false; + } + return destination.startsWith(`${source}/`); +} + function renderBreadcrumbs(pane, path) { const nav = document.getElementById(`${pane}-breadcrumbs`); nav.innerHTML = ""; @@ -892,6 +916,12 @@ function showDirectoryMoveNotSupported() { setStatus(message); } +function showBatchDirectoryMoveNotSupported() { + const message = "Batch directory move is not supported in v1"; + setError("actions-error", message); + setStatus(message); +} + function resetRenameMoveState() { renameMoveState = { source: null, @@ -965,7 +995,7 @@ function openF6Flow() { return openRenameMovePopup(); } if (selectedItems.some((item) => item.kind !== "file")) { - showDirectoryMoveNotSupported(); + showBatchDirectoryMoveNotSupported(); return true; } return openBatchMovePopup(selectedItems); @@ -991,9 +1021,17 @@ async function submitRenameMovePopup() { elements.error.textContent = "Destination must differ from source"; return; } - if (source.kind === "directory" && destinationParent !== sourceParent) { - elements.error.textContent = "Directory move is not supported in v1"; - return; + if (source.kind === "directory") { + if (isNestedPath(source.path, destination)) { + elements.error.textContent = "Destination cannot be inside source"; + return; + } + if (destinationParent !== sourceParent) { + if (rootKeyFromPath(destination) !== rootKeyFromPath(source.path)) { + elements.error.textContent = "Cross-root directory move is not supported in v1"; + return; + } + } } try {