diff --git a/CHANGELOG.md b/CHANGELOG.md index 78dfa43..7cd7a56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- **`get_file_info` and `delete_files` correctly handle multi-path inputs on real DSM 7.x** (#77) — closes #68. DSM 7.x's `SYNO.FileStation.List getinfo` and `SYNO.FileStation.Delete start` do **not** honor the documented comma-joined multi-path format, even on v2: a request with `path=/a,/b` is treated as a single literal path. For `getinfo` this surfaces as one synthetic record whose `path` field IS the literal comma-joined string (the handler's `len(files) == 1` branch then renders it as a single info card). For `delete_files` this surfaces as a successful task that actually no-ops on every input, returning `[+] Deleted N item(s)` with all paths listed but none removed. The round-1 hypothesis (pin `getinfo` to v2 to dodge a v3 quirk) was disproven by vdsm CI on DSM 7.2.2 — the comma-joined-as-single-path symptom reproduces on v2 too. Fix matches the user's documented workaround on #68: **one DSM call per input path**. Both tools now iterate `paths` and issue per-path requests, aggregating results into the same response shape callers already expect (single info card for one path, table for multiple; per-share recycle-bin messaging unchanged for delete). Trade-off is N round-trips for N paths, which is fine for typical small-N usage and trivially correct. Refactor extracted `_delete_one_path` from `delete_files` so the per-path async-task pattern (start → poll → stop in `try`/`finally`) lives in one place; `get_file_info` simply loops `client.request` since it's synchronous. Bumped `tests/conftest.py` `SYNO.FileStation.List max_version` from 2 to 3 to match DSM 7.x reality so future regression tests don't get fooled by a max-resolves-to-2 default. New `TestGetFileInfo::test_multipath_uses_per_path_serial_calls` asserts (a) N requests for N paths, (b) each request carries a single path with no commas, (c) all pinned to v2, (d) results aggregate correctly. New `TestMultiPathDelete` integration test (re-exported in `tests/vdsm/test_vdsm_integration.py`) creates two folders, deletes both in one multi-path call, verifies via `list_files` that both are actually gone — would have caught the original #68 regression before it shipped if it had existed in v0.5.0. 550 unit tests pass at 96.13% coverage. - **`delete_files` now reports recycle-bin status correctly per share** (#73) — closes #37. Pre-fix, `modules/filestation/__init__.py:238` constructed `recycle_status: dict[str, bool] = {}` and never populated it. The closure-shared dict flowed into `delete_files`, where `operations.py:386-398` fell through to the `# Assume recycle bin by default` branch for every share, so users with `#recycle` *disabled* on a share saw `"Recycle bin is enabled on /{share} — files can be recovered..."` after a delete that had actually removed the data permanently. New lazy probe `ensure_recycle_status(client, share, recycle_status)` in `modules/filestation/helpers.py` lives off the cache: a missing share triggers `SYNO.FileStation.List` on `/{share}/#recycle` with `limit=0`. Success → `True`, DSM 408 → `False`, 105 (permission denied) or any other DSM error → `True` + WARNING log (preserves prior optimistic-default behavior so messaging stays consistent and the operator sees the diagnostic). Cached in-place so repeated deletes against the same share don't re-probe. Wired into `delete_files` and `list_recycle_bin` (the two correctness-sensitive paths). `list_shares` left alone — it renders whatever's already cached, kept cheap. **Self-correct on observation**: when `list_recycle_bin` sees DSM behavior contradicting the cached value (cached `True` but the actual list returns 408 → flip to `False`; or vice-versa on an unexpected success), the helper updates the cache in place and logs at INFO so subsequent `delete_files` calls in the same session see the corrected state without waiting for re-auth invalidation. **Invalidate-on-reauth**: new `AuthManager.add_on_reauth_callback` API + dispatch loop in `_re_authenticate`; new `SharedClientManager.subscribe_on_reauth` proxy that queues callbacks before the AuthManager is lazily created and flushes them on first `get_client`. Filestation `register()` subscribes `recycle_status.clear` so admin-side toggles to `#recycle` between sessions are picked up after the next session-error-driven re-auth. Eighteen new tests (9 helper + 3 auth + 3 server + 1 delete-files lazy-probe + 2 list_recycle_bin self-correct); 546 passing total at 96.10% coverage. Persistence across server restarts (binding the closure dict to `ServerState.recycle_bin_status`) is intentionally out of scope — that field exists on the model but `load_state`/`save_state` aren't currently wired up at all; treating that as a separate follow-up since plumbing it touches a much larger surface than #37 alone needs. vdsm integration test (verifying real DSM recycle-on/recycle-off shares produce correct messaging end-to-end) is also a follow-up — `synoshare --setopt` recycle-toggle reliability on DSM 7.2.x is unproven (PR #23 reverted a similar setopt for share creation), so unit tests carry the regression load until that's verified. ### Added diff --git a/src/mcp_synology/modules/filestation/metadata.py b/src/mcp_synology/modules/filestation/metadata.py index 8fd35b7..341a296 100644 --- a/src/mcp_synology/modules/filestation/metadata.py +++ b/src/mcp_synology/modules/filestation/metadata.py @@ -16,7 +16,6 @@ synology_error_response, ) from mcp_synology.modules.filestation.helpers import ( - escape_multi_path, normalize_path, ) @@ -46,31 +45,62 @@ async def get_file_info( paths: list[str], additional: list[str] | None = None, ) -> str: - """Get detailed metadata for specific files or folders.""" + """Get detailed metadata for specific files or folders. + + Closes #68 (the `get_file_info` half). DSM 7.x's + `SYNO.FileStation.List getinfo` does not honor the documented + comma-joined multi-path format on v2 — verified against vdsm DSM + 7.2.2 in CI: a request with `path=/a,/b` returns a single synthetic + record whose `path` field IS the literal `/a,/b` string. v3 has the + same problem (verified by the round-1 v2-pin attempt that didn't + fix the bug). The user's documented workaround on #68 was to loop + one path at a time, and that's what this function now does + internally: one DSM call per input path, results aggregated into + the response shape callers already expect (single-info card for one + path, table for multiple). Trade-off is N round-trips for N paths, + which is fine for the typical small-N usage and trivially correct. + """ if additional is None: additional = ["real_path", "size", "owner", "time", "perm"] - normalized = [normalize_path(p) for p in paths] - path_param = escape_multi_path(normalized) - - try: - data = await client.request( - "SYNO.FileStation.List", - "getinfo", - params={ - "path": path_param, - "additional": '["' + '","'.join(additional) + '"]', - }, + if not paths: + error_response( + ErrorCode.NOT_FOUND, + "Get file info failed: No paths provided.", + retryable=False, + param="paths", + value=paths, + suggestion="Pass at least one path.", ) - except SynologyError as e: - synology_error_response("Get file info", e) - files = data.get("files", []) + normalized = [normalize_path(p) for p in paths] + + # Pin to v2 for parallelism with Delete / CopyMove / Search. v3 doesn't + # buy us anything for getinfo and adds risk of further multi-path quirks. + getinfo_version = min(2, client.negotiate_version("SYNO.FileStation.List", max_version=2)) + additional_param = '["' + '","'.join(additional) + '"]' + + files: list[dict[str, Any]] = [] + for p in normalized: + try: + data = await client.request( + "SYNO.FileStation.List", + "getinfo", + version=getinfo_version, + params={ + "path": p, + "additional": additional_param, + }, + ) + except SynologyError as e: + synology_error_response("Get file info", e) + per_path_files = data.get("files", []) + if per_path_files: + files.extend(per_path_files) if len(files) == 1: return _format_single_info(files[0]) - # Multiple files: table format if not files: error_response( ErrorCode.NOT_FOUND, diff --git a/src/mcp_synology/modules/filestation/operations.py b/src/mcp_synology/modules/filestation/operations.py index 95375ee..e7b1ed0 100644 --- a/src/mcp_synology/modules/filestation/operations.py +++ b/src/mcp_synology/modules/filestation/operations.py @@ -285,28 +285,26 @@ async def _copy_move( return "\n".join(lines) -async def delete_files( +async def _delete_one_path( client: DsmClient, + path: str, *, - paths: list[str], - recursive: bool = True, - recycle_bin_status: dict[str, bool] | None = None, - timeout: float = 120.0, -) -> str: - """Delete files or folders.""" - normalized = [normalize_path(p) for p in paths] - path_param = escape_multi_path(normalized) - - # Pin to version 2 — v3 uses JSON request format with different parameter encoding. - delete_version = min(2, client.negotiate_version("SYNO.FileStation.Delete", max_version=2)) + recursive: bool, + delete_version: int, + timeout: float, +) -> None: + """Delete a single path via DSM async-task pattern. + Raises ToolError (via error_response/synology_error_response) on failure; + returns None on success. Always stops the background task on exit. + """ try: start_data = await client.request( "SYNO.FileStation.Delete", "start", version=delete_version, params={ - "path": path_param, + "path": path, "recursive": str(recursive).lower(), }, ) @@ -315,7 +313,6 @@ async def delete_files( taskid = start_data.get("taskid", "") - # Poll for completion. Use try/finally to ensure task is always stopped. elapsed = 0.0 interval = 0.5 status: dict[str, Any] = {} @@ -335,7 +332,7 @@ async def delete_files( poll_error = e break - logger.debug("Delete status: %s", status) + logger.debug("Delete status (%s): %s", path, status) if status.get("finished", False): break @@ -358,21 +355,17 @@ async def delete_files( if timed_out: error_response( ErrorCode.TIMEOUT, - f"Delete files failed: timed out after {timeout}s.", + f"Delete files failed: timed out after {timeout}s on path: {path}", retryable=True, param="timeout", value=timeout, suggestion="The operation may still be running on the NAS.", ) - # Check for errors in the completed task if "error" in status: err = status["error"] err_code = err.get("code", 0) if isinstance(err, dict) else err - err_path = status.get("path", "") - # Route err_code through error_from_code so callers see a specific - # envelope (e.g. permission_denied, not_found) matching the synchronous - # error paths in this module. Unknown codes fall back to DSM_ERROR. + err_path = status.get("path", path) mapped = error_from_code(err_code, "SYNO.FileStation.Delete") error_response( mapped.error_code, @@ -384,6 +377,54 @@ async def delete_files( ), ) + +async def delete_files( + client: DsmClient, + *, + paths: list[str], + recursive: bool = True, + recycle_bin_status: dict[str, bool] | None = None, + timeout: float = 120.0, +) -> str: + """Delete files or folders. + + Closes #68 (the `delete_files` half). DSM 7.x's + `SYNO.FileStation.Delete start` does not honor the documented + comma-joined multi-path format on v2 — the user reported that + `delete_files(paths=[a, b, ..., l])` returns success but no paths + are actually deleted, while single-path calls work. The verified + parallel symptom on `get_file_info` (vdsm CI proves a v2-style + `path=/a,/b` returns one synthetic record with the literal + comma-joined string as its `path`) makes the same root cause near + certain on Delete. Fix matches the user's documented workaround: + one DSM Delete task per input path. Per-path serial means N + round-trips for N paths, which is fine for typical small-N usage + and trivially correct. + """ + if not paths: + error_response( + ErrorCode.NOT_FOUND, + "Delete files failed: No paths provided.", + retryable=False, + param="paths", + value=paths, + suggestion="Pass at least one path.", + ) + + normalized = [normalize_path(p) for p in paths] + + # Pin to v2 — v3 uses JSON request format with different parameter encoding. + delete_version = min(2, client.negotiate_version("SYNO.FileStation.Delete", max_version=2)) + + for p in normalized: + await _delete_one_path( + client, + p, + recursive=recursive, + delete_version=delete_version, + timeout=timeout, + ) + # Determine recycle bin status per share. Probes lazily on first observation # via `ensure_recycle_status` so the dict stays empty until we actually need # the answer; pre-#37 the dict was never populated and every share was diff --git a/tests/conftest.py b/tests/conftest.py index b4651da..cca383f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -43,7 +43,11 @@ def make_api_cache() -> dict[str, ApiInfoEntry]: return { "SYNO.API.Auth": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=7), "SYNO.FileStation.Info": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=2), - "SYNO.FileStation.List": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=2), + # max_version=3 matches DSM 7.x (where #68 surfaced); production code + # MUST pin to v2 explicitly via `negotiate_version(..., max_version=2)` + # because v3 reinterprets multi-path semantics and silently breaks + # comma-joined `path` queries. + "SYNO.FileStation.List": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=3), "SYNO.FileStation.Search": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=2), "SYNO.FileStation.DirSize": ApiInfoEntry(path="entry.cgi", min_version=1, max_version=2), "SYNO.FileStation.CreateFolder": ApiInfoEntry( diff --git a/tests/modules/filestation/test_metadata.py b/tests/modules/filestation/test_metadata.py index 3251b69..fa49cb2 100644 --- a/tests/modules/filestation/test_metadata.py +++ b/tests/modules/filestation/test_metadata.py @@ -88,6 +88,86 @@ async def test_error(self, mock_client: DsmClient) -> None: assert body["status"] == "error" assert body["error"]["code"] == "not_found" + @respx.mock + async def test_multipath_uses_per_path_serial_calls(self, mock_client: DsmClient) -> None: + """Closes #68 (the get_file_info half): DSM 7.x's + `SYNO.FileStation.List getinfo` doesn't honor the documented + comma-joined multi-path format on v2 (vdsm 7.2.2 verified — the + comma-joined string lands as a literal single path, returning one + synthetic record). Production therefore issues ONE DSM call per + input path. Test asserts (a) N requests for N paths, (b) each + request carries a single path (no commas), (c) all pinned to + v2, (d) results aggregate into the table-format response. + """ + captured: list[dict[str, str]] = [] + per_path_files = { + "/video/a.mkv": { + "name": "a.mkv", + "path": "/video/a.mkv", + "isdir": False, + "additional": {"size": 1, "time": {"mtime": 1710000000}}, + }, + "/video/b.srt": { + "name": "b.srt", + "path": "/video/b.srt", + "isdir": False, + "additional": {"size": 1, "time": {"mtime": 1710000000}}, + }, + } + + def side_effect(request: httpx.Request) -> httpx.Response: + params = dict(request.url.params) + captured.append(params) + requested_path = params.get("path", "") + return httpx.Response( + 200, + json={ + "success": True, + "data": {"files": [per_path_files[requested_path]]}, + }, + ) + + respx.get(f"{BASE_URL}/webapi/entry.cgi").mock(side_effect=side_effect) + result = await get_file_info(mock_client, paths=["/video/a.mkv", "/video/b.srt"]) + + # Two paths → two DSM calls. + assert len(captured) == 2, f"expected two DSM calls (one per path), got {len(captured)}" + for params in captured: + assert params["api"] == "SYNO.FileStation.List" + assert params["method"] == "getinfo" + assert params["version"] == "2", ( + f"expected version=2, got version={params['version']!r}" + ) + assert "," not in params["path"], ( + f"each request must carry a single path (no comma-joined multipath), " + f"got path={params['path']!r}" + ) + # Each input path appears in exactly one request. + sent_paths = sorted(p["path"] for p in captured) + assert sent_paths == ["/video/a.mkv", "/video/b.srt"] + # And the aggregated response renders both files in tabular form. + assert "a.mkv" in result + assert "b.srt" in result + # No comma-joined string in the result — that would indicate a regression + # back to the single-call form that #68 surfaced. + assert "/video/a.mkv,/video/b.srt" not in result + + async def test_empty_paths_list_returns_not_found(self, mock_client: DsmClient) -> None: + """Empty paths list short-circuits to a not_found error before any + DSM call is attempted. Defensive guard added with the per-path + serial refactor — without it, the for-loop would simply do nothing + and the downstream `if not files:` branch would fire with a less + precise message about missing file information. + """ + with pytest.raises(ToolError) as exc_info: + await get_file_info(mock_client, paths=[]) + body = json.loads(str(exc_info.value)) + assert body["status"] == "error" + assert body["error"]["code"] == "not_found" + assert "No paths provided" in body["error"]["message"] + assert body["error"]["param"] == "paths" + assert body["error"]["value"] == [] + @respx.mock async def test_empty_files_list_returns_not_found(self, mock_client: DsmClient) -> None: """getinfo succeeds but returns no files → not_found. diff --git a/tests/modules/filestation/test_operations.py b/tests/modules/filestation/test_operations.py index 273a9b6..1adfcae 100644 --- a/tests/modules/filestation/test_operations.py +++ b/tests/modules/filestation/test_operations.py @@ -219,6 +219,21 @@ async def test_delete_multiple_shares(self, mock_client: DsmClient) -> None: assert "enabled" in result assert "NOT enabled" in result + async def test_delete_empty_paths_list_returns_not_found(self, mock_client: DsmClient) -> None: + """Empty paths list short-circuits to a not_found error before any + DSM call is attempted. Defensive guard added with the per-path + serial refactor — without it, the per-path for-loop would simply + do nothing and return an empty success message. + """ + with pytest.raises(ToolError) as exc_info: + await delete_files(mock_client, paths=[]) + body = json.loads(str(exc_info.value)) + assert body["status"] == "error" + assert body["error"]["code"] == "not_found" + assert "No paths provided" in body["error"]["message"] + assert body["error"]["param"] == "paths" + assert body["error"]["value"] == [] + @respx.mock async def test_delete_lazily_probes_when_share_missing_from_cache( self, mock_client: DsmClient diff --git a/tests/test_integration.py b/tests/test_integration.py index fcb2798..8476c95 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -462,13 +462,38 @@ async def test_get_file_info_share(self, nas_client: Any) -> None: logger.info("get_file_info(%s):\n%s", paths["existing_share"], result) async def test_get_file_info_multiple_paths(self, nas_client: Any) -> None: - """Get info about multiple paths at once.""" + """Get info about multiple paths at once. + + Closes #68 (the get_file_info half): pre-fix, `metadata.py` did not + pin to v2 of `SYNO.FileStation.List`, so on real DSM 7.x (which + advertises v3) the comma-joined `path` was treated as a single + literal path and the response collapsed to one synthetic record + with the comma-joined string as its `path`. The handler's + `len(files) == 1` branch then rendered it as a single info card. + Asserting BOTH paths appear in the rendered output catches that + regression — a single-record collapse can only show one of them. + """ client, _, _, paths = _unpack(nas_client) - result = await get_file_info( - client, - paths=[paths["existing_share"], paths["writable_folder"]], - ) + a = paths["existing_share"] + b = paths["writable_folder"] + # Skip when the test config happens to alias both to the same path. + if a == b: + pytest.skip("existing_share and writable_folder are the same path") + result = await get_file_info(client, paths=[a, b]) logger.info("get_file_info(multiple):\n%s", result) + # Tabular output for >1 file uses the table headers; single-record + # output uses "File Info:" followed by the path. Assert tabular shape. + assert "File Info" in result + # Both paths must appear individually — the comma-joined form + # `,` would only appear if we'd regressed to v3 semantics. + comma_joined = f"{a},{b}" + assert comma_joined not in result, ( + "v2 pin regression: response contains the comma-joined path string, " + "indicating DSM treated the input as a single literal path. " + f"Got: {result!r}" + ) + assert a in result + assert b in result async def test_get_file_info_invalid_path(self, nas_client: Any) -> None: """Get info about a non-existent path — should not crash. @@ -644,6 +669,85 @@ async def test_10_verify_deleted(self, nas_client: Any) -> None: logger.info("Verified %s is deleted", self._TEST_DIR) +# --------------------------------------------------------------------------- +# Multi-path regression coverage (closes #68) +# --------------------------------------------------------------------------- + + +@pytest.mark.integration +class TestMultiPathDelete: + """Closes #68 (the delete_files half). + + The user reported that ``delete_files(paths=[a, b, ..., l])`` returned + ``[+] Deleted N item(s)`` listing every input — but none were actually + removed on the NAS. Single-element calls worked. Symptom was a + silent-no-op masquerading as success. + + This test creates two real folders, calls ``delete_files`` with both in + a single multi-path invocation, then asserts via ``list_files`` that + BOTH are gone. A pre-fix code path that no-ops on multi-path would fail + at the post-delete listing check, not at the delete call itself. + """ + + _MULTI_DIR = "_integration_test_multipath" + _A = "alpha" + _B = "bravo" + + async def test_01_setup_multipath_dirs(self, nas_client: Any) -> None: + """Create two sibling folders to delete in one call.""" + client, _, config, paths = _unpack(nas_client) + _skip_unless_write(config) + + base = f"{paths['writable_folder']}/{self._MULTI_DIR}" + for sub in (self._A, self._B): + try: + await create_folder(client, paths=[f"{base}/{sub}"]) + except ToolError as e: + body = json.loads(str(e)) + assert body["error"]["code"] == "already_exists", f"Unexpected error: {e}" + + async def test_02_multipath_delete_actually_deletes(self, nas_client: Any) -> None: + """Delete both folders in a single multi-path call. Verify by listing.""" + client, _, config, paths = _unpack(nas_client) + _skip_unless_write(config) + + base = f"{paths['writable_folder']}/{self._MULTI_DIR}" + a_path = f"{base}/{self._A}" + b_path = f"{base}/{self._B}" + + # Pre-condition: both folders should exist before the multi-path delete. + pre = await list_files(client, path=base) + assert self._A in pre, f"setup failed: {self._A} not present pre-delete" + assert self._B in pre, f"setup failed: {self._B} not present pre-delete" + + result = await delete_files(client, paths=[a_path, b_path], recursive=True) + logger.info("multi-path delete result:\n%s", result) + + # Post-condition: BOTH folders must actually be gone. The reported + # bug (#68) was that delete returned "[+] Deleted 2 item(s)" with + # both listed but the entries remained. A passing test here proves + # the multi-path code path actually deletes on real DSM. + post = await list_files(client, path=base) + assert self._A not in post, ( + f"#68 regression: delete returned success but {self._A!r} still present.\n" + f"Post-delete listing:\n{post}" + ) + assert self._B not in post, ( + f"#68 regression: delete returned success but {self._B!r} still present.\n" + f"Post-delete listing:\n{post}" + ) + + async def test_03_cleanup(self, nas_client: Any) -> None: + """Remove the parent directory we created (best effort).""" + import contextlib + + client, _, config, paths = _unpack(nas_client) + _skip_unless_write(config) + base = f"{paths['writable_folder']}/{self._MULTI_DIR}" + with contextlib.suppress(ToolError): + await delete_files(client, paths=[base], recursive=True) + + # --------------------------------------------------------------------------- # Recycle bin # --------------------------------------------------------------------------- diff --git a/tests/vdsm/test_vdsm_integration.py b/tests/vdsm/test_vdsm_integration.py index 168cf33..9ea29ea 100644 --- a/tests/vdsm/test_vdsm_integration.py +++ b/tests/vdsm/test_vdsm_integration.py @@ -18,6 +18,7 @@ TestFileTransfers, TestListing, TestMetadata, + TestMultiPathDelete, TestRecycleBin, TestResourceUsage, TestSearch, @@ -38,6 +39,7 @@ "TestFileTransfers", "TestListing", "TestMetadata", + "TestMultiPathDelete", "TestRecycleBin", "TestResourceUsage", "TestSearch",