From bfdd1715a747cf0f9750fe8d7b52dd8ddfc00387 Mon Sep 17 00:00:00 2001 From: Andrii Novikov Date: Fri, 17 Jul 2026 13:31:41 +0300 Subject: [PATCH] fix: encode file paths for DIAL Core requests DIAL Core 500s (URISyntaxException: Illegal character in path) when file URLs containing spaces or parens reach it verbatim - most visibly on copy/move, whose ops/resource JSON body bypasses HTTP-client URL encoding entirely. Establish a URL contract on DialFileService: decoded 'files/...' URLs in and out; percent-encoding is an internal egress concern. An idempotent quote(unquote(...)) accepts both spellings, and list_folder decodes Core's encoded item URLs so the file tools keep matching human-readable globs with no encoding awareness. StateHolder's file-cache key gets the same normalization so every cache user (DialFileService, FileLoaderService) shares one entry per file regardless of spelling. Fixes #447 --- src/quickapp/common/state_holder.py | 8 +- .../dial_core_services/dial_file_service.py | 57 +++++-- .../unit_tests/common/test_state_holder.py | 31 ++++ .../test_dial_file_service.py | 140 ++++++++++++++++++ .../test_find_files_tool.py | 13 ++ 5 files changed, 235 insertions(+), 14 deletions(-) create mode 100644 src/tests/unit_tests/common/test_state_holder.py diff --git a/src/quickapp/common/state_holder.py b/src/quickapp/common/state_holder.py index 4651c660..8180c898 100644 --- a/src/quickapp/common/state_holder.py +++ b/src/quickapp/common/state_holder.py @@ -1,6 +1,7 @@ import logging from hashlib import sha256 from typing import Any +from urllib.parse import quote, unquote from aidial_client.types.metadata import FileMetadata from aidial_sdk.chat_completion import Attachment, ToolCall @@ -54,4 +55,9 @@ def invalidate_file_data(self, url: str) -> None: @staticmethod def _get_file_key_by_url(url: str) -> str: - return sha256(url.encode('utf-8')).hexdigest() + # Normalize percent-encoding (unquote-then-quote is idempotent) so the + # encoded and decoded spellings of one URL share a single cache entry — + # the file cache is populated by several services (DialFileService, + # FileLoaderService) that may receive either spelling of the same file. + normalized = quote(unquote(url), safe="/") + return sha256(normalized.encode("utf-8")).hexdigest() diff --git a/src/quickapp/dial_core_services/dial_file_service.py b/src/quickapp/dial_core_services/dial_file_service.py index e6e09e91..97259b4d 100644 --- a/src/quickapp/dial_core_services/dial_file_service.py +++ b/src/quickapp/dial_core_services/dial_file_service.py @@ -2,6 +2,7 @@ from collections import deque from pathlib import PurePosixPath from typing import Literal, NoReturn +from urllib.parse import quote, unquote from aidial_client import AsyncDial from aidial_client._exception import DialException, ResourceNotFoundError @@ -17,6 +18,19 @@ logger = logging.getLogger(__name__) +def _encode(url: str) -> str: + """Percent-encode a 'files/...' URL path for the DIAL Core API. + + Unquote-then-quote makes this idempotent: a decoded input ('file name.pdf') + and an already-encoded one ('file%20name.pdf') both yield 'file%20name.pdf'. + Accepted edge case: a stored filename literally containing a valid '%XX' + sequence is indistinguishable from its encoded form and gets normalized — + this favors the overwhelmingly common case. `safe="/"` keeps path separators + (including a trailing '/', which distinguishes folders) intact. + """ + return quote(unquote(url), safe="/") + + class FolderEntry(BaseModel): model_config = ConfigDict(frozen=True) @@ -27,6 +41,13 @@ class FolderEntry(BaseModel): @inject class DialFileService: + """DIAL Core file API wrapper. + + URL contract: decoded (human-readable) 'files/...' URLs in and out. + Percent-encoding for the wire is an internal egress concern (`_encode`); + ingress URLs from Core (`list_folder` items) are decoded before being + returned, so callers never handle encoded spellings. + """ def __init__( self, @@ -63,12 +84,14 @@ async def _get_metadata(self, url: str) -> FileMetadata: pass pre-resolved relative 'files/...' URLs, so get_api_path normalization is moot. """ try: - return await self.__dial_client.metadata.get("files", url) + return await self.__dial_client.metadata.get("files", _encode(url)) except DialException as e: self._reraise_404_as_not_found(e) async def download_file(self, file_url: str) -> tuple[bytes, FileMetadata | None]: logger.debug(f"File url to download url:{file_url}") + # StateHolder normalizes percent-encoding in its cache keys, so encoded + # and decoded spellings of one file share a single entry. file_data = self.__state_holder.get_file_data(url=file_url) if file_data is not None: return file_data, self.__state_holder.get_file_metadata(file_url) @@ -80,7 +103,9 @@ async def download_file(self, file_url: str) -> tuple[bytes, FileMetadata | None raise ValueError( f"File size {size} exceeds the limit of {self.__content_size_limit} bytes." ) - file_data = await (await self.__dial_client.files.download(file_url)).aget_content() + file_data = await ( + await self.__dial_client.files.download(_encode(file_url)) + ).aget_content() self.__state_holder.store_file_data(file_url, file_data, metadata) except Exception as e: logger.error("Failed to download: %s", file_url, exc_info=True) @@ -127,18 +152,20 @@ async def _upload_text( if_none_match: Literal["*"] | None = None, if_match: str | None = None, ) -> str: - encoded = content.encode("utf-8") - filename = url.split("/")[-1] + content_bytes = content.encode("utf-8") + filename = unquote(url).split("/")[-1] metadata = await self.__dial_client.files.upload( - url=url, - file=(filename, encoded, content_type), + url=_encode(url), + file=(filename, content_bytes, content_type), etag_if_none_match=if_none_match, etag_if_match=if_match, ) - return metadata.url + # Core echoes the encoded URL back; return the decoded spelling per the + # class contract. + return unquote(metadata.url) async def delete(self, file_url: str) -> None: - await self.__dial_client.files.delete(file_url) + await self.__dial_client.files.delete(_encode(file_url)) self.invalidate_cache(file_url) def invalidate_cache(self, file_url: str) -> None: @@ -160,7 +187,9 @@ async def list_folder(self, folder_url: str, max_depth: int = 1) -> list[FolderE items: list[FileItem] = metadata.items or [] for item in items: is_folder = item.node_type == "FOLDER" - item_url = item.url + # Core returns percent-encoded item URLs; expose the decoded + # spelling per the class contract. + item_url = unquote(item.url) if is_folder and not item_url.endswith("/"): item_url = item_url + "/" results.append( @@ -175,16 +204,18 @@ async def list_folder(self, folder_url: str, max_depth: int = 1) -> list[FolderE return results async def copy(self, source_url: str, destination_url: str, overwrite: bool) -> None: + # copy_to/move_to send the URLs in an ops/resource JSON body that the + # HTTP client never encodes, so both must be encoded here (#447). await self.__dial_client.files.copy_to( - source=source_url, - destination=destination_url, + source=_encode(source_url), + destination=_encode(destination_url), overwrite=overwrite, ) async def move(self, source_url: str, destination_url: str, overwrite: bool) -> None: await self.__dial_client.files.move_to( - source=source_url, - destination=destination_url, + source=_encode(source_url), + destination=_encode(destination_url), overwrite=overwrite, ) self.invalidate_cache(source_url) diff --git a/src/tests/unit_tests/common/test_state_holder.py b/src/tests/unit_tests/common/test_state_holder.py new file mode 100644 index 00000000..79b1fcae --- /dev/null +++ b/src/tests/unit_tests/common/test_state_holder.py @@ -0,0 +1,31 @@ +from unittest.mock import MagicMock + +from quickapp.common.state_holder import StateHolder + + +class TestFileCacheKeyNormalization: + def test_encoded_and_decoded_spellings_share_one_entry(self): + holder = StateHolder() + metadata = MagicMock() + + holder.store_file_data("files/b/file name (1).pdf", b"data", metadata) + + assert holder.get_file_data(url="files/b/file%20name%20%281%29.pdf") == b"data" + assert holder.get_file_metadata("files/b/file%20name%20%281%29.pdf") is metadata + + def test_invalidate_accepts_either_spelling(self): + holder = StateHolder() + + holder.store_file_data("files/b/file name.pdf", b"data") + holder.invalidate_file_data("files/b/file%20name.pdf") + + assert holder.get_file_data(url="files/b/file name.pdf") is None + + def test_distinct_urls_keep_distinct_entries(self): + holder = StateHolder() + + holder.store_file_data("files/b/a.txt", b"a") + holder.store_file_data("files/b/b.txt", b"b") + + assert holder.get_file_data(url="files/b/a.txt") == b"a" + assert holder.get_file_data(url="files/b/b.txt") == b"b" diff --git a/src/tests/unit_tests/dial_core_services_tests/test_dial_file_service.py b/src/tests/unit_tests/dial_core_services_tests/test_dial_file_service.py index 94de3612..fb8fb4ba 100644 --- a/src/tests/unit_tests/dial_core_services_tests/test_dial_file_service.py +++ b/src/tests/unit_tests/dial_core_services_tests/test_dial_file_service.py @@ -29,6 +29,9 @@ def _make_mock_dial_client( mock_files = MagicMock() mock_files.download = AsyncMock(return_value=mock_download_result) mock_files.upload = AsyncMock(return_value=mock_metadata) + mock_files.delete = AsyncMock(return_value=None) + mock_files.copy_to = AsyncMock(return_value=None) + mock_files.move_to = AsyncMock(return_value=None) # The service fetches file/folder metadata via metadata.get (not files.get_metadata). mock_metadata_resource = MagicMock() @@ -233,6 +236,143 @@ async def test_subsequent_download_hits_network_after_invalidation(self): assert second == file_bytes_v2 +def _make_folder_item( + url: str, node_type: str = "ITEM", content_length: int | None = 5 +) -> MagicMock: + item = MagicMock() + item.node_type = node_type + item.url = url + item.content_length = content_length + return item + + +def _make_folder_metadata(items: list[MagicMock]) -> MagicMock: + metadata = MagicMock() + metadata.node_type = "FOLDER" + metadata.items = items + return metadata + + +class TestUrlEncoding: + @pytest.mark.asyncio + async def test_download_encodes_decoded_path_with_spaces_and_parens(self): + mock_client = _make_mock_dial_client() + svc = _make_service(dial_client=mock_client) + + await svc.download_file("files/b/Uno-Rules-PDF-Official-Rules-unorules.org_ (1).pdf") + + encoded = "files/b/Uno-Rules-PDF-Official-Rules-unorules.org_%20%281%29.pdf" + mock_client.metadata.get.assert_awaited_once_with("files", encoded) + mock_client.files.download.assert_awaited_once_with(encoded) + + @pytest.mark.asyncio + async def test_download_does_not_double_encode_encoded_input(self): + mock_client = _make_mock_dial_client() + svc = _make_service(dial_client=mock_client) + + await svc.download_file("files/b/file%20name.pdf") + + mock_client.files.download.assert_awaited_once_with("files/b/file%20name.pdf") + + @pytest.mark.asyncio + async def test_metadata_lookup_preserves_trailing_slash(self): + mock_client = _make_mock_dial_client() + mock_client.metadata.get = AsyncMock(return_value=_make_folder_metadata([])) + svc = _make_service(dial_client=mock_client) + + await svc.list_folder("files/b/my folder/") + + mock_client.metadata.get.assert_awaited_once_with("files", "files/b/my%20folder/") + + @pytest.mark.asyncio + async def test_delete_encodes_url(self): + mock_client = _make_mock_dial_client() + svc = _make_service(dial_client=mock_client) + + await svc.delete("files/b/report (final).txt") + + mock_client.files.delete.assert_awaited_once_with("files/b/report%20%28final%29.txt") + + @pytest.mark.asyncio + async def test_copy_encodes_source_and_destination(self): + mock_client = _make_mock_dial_client() + svc = _make_service(dial_client=mock_client) + + await svc.copy("files/b/src (1).pdf", "files/b/dst (1).pdf", overwrite=False) + + mock_client.files.copy_to.assert_awaited_once_with( + source="files/b/src%20%281%29.pdf", + destination="files/b/dst%20%281%29.pdf", + overwrite=False, + ) + + @pytest.mark.asyncio + async def test_move_encodes_source_and_destination(self): + mock_client = _make_mock_dial_client() + svc = _make_service(dial_client=mock_client) + + await svc.move("files/b/src (1).pdf", "files/b/dst (1).pdf", overwrite=True) + + mock_client.files.move_to.assert_awaited_once_with( + source="files/b/src%20%281%29.pdf", + destination="files/b/dst%20%281%29.pdf", + overwrite=True, + ) + + @pytest.mark.asyncio + async def test_upload_encodes_url_and_keeps_decoded_filename(self): + mock_client = _make_mock_dial_client(upload_url="files/b/my%20notes.txt") + svc = _make_service(dial_client=mock_client) + + result = await svc.write_file(url="files/b/my notes.txt", content="hi", overwrite=True) + + call_kwargs = mock_client.files.upload.call_args.kwargs + assert call_kwargs["url"] == "files/b/my%20notes.txt" + # Multipart filename stays human-readable. + assert call_kwargs["file"][0] == "my notes.txt" + # Core echoes the encoded URL; the service returns it decoded. + assert result == "files/b/my notes.txt" + + @pytest.mark.asyncio + async def test_cache_hit_across_encoded_and_decoded_spellings(self): + mock_client = _make_mock_dial_client(file_content=b"once") + svc = _make_service(dial_client=mock_client) + + first, _ = await svc.download_file("files/b/file name.pdf") + second, _ = await svc.download_file("files/b/file%20name.pdf") + + assert first == second == b"once" + mock_client.files.download.assert_awaited_once() + + @pytest.mark.asyncio + async def test_invalidate_cache_accepts_either_spelling(self): + mock_client = _make_mock_dial_client(file_content=b"v1") + svc = _make_service(dial_client=mock_client) + + await svc.download_file("files/b/file name.pdf") + svc.invalidate_cache("files/b/file%20name.pdf") + await svc.download_file("files/b/file name.pdf") + + assert mock_client.files.download.await_count == 2 + + @pytest.mark.asyncio + async def test_list_folder_returns_decoded_entry_urls(self): + mock_client = _make_mock_dial_client() + mock_client.metadata.get = AsyncMock( + return_value=_make_folder_metadata( + [ + _make_folder_item("files/b/My%20Report%20%281%29.pdf"), + _make_folder_item("files/b/sub%20dir", node_type="FOLDER", content_length=None), + ] + ) + ) + svc = _make_service(dial_client=mock_client) + + entries = await svc.list_folder("files/b/") + + assert [e.url for e in entries] == ["files/b/My Report (1).pdf", "files/b/sub dir/"] + + class TestGrantPermissions: @pytest.mark.asyncio async def test_grant_permissions_calls_client(self): diff --git a/src/tests/unit_tests/dial_files_tooling/test_find_files_tool.py b/src/tests/unit_tests/dial_files_tooling/test_find_files_tool.py index d7a8346d..ac65c7fe 100644 --- a/src/tests/unit_tests/dial_files_tooling/test_find_files_tool.py +++ b/src/tests/unit_tests/dial_files_tooling/test_find_files_tool.py @@ -113,6 +113,19 @@ async def test_non_root_folder_not_found_raises(self): assert "folder not found: missing/" in exc.value.message assert "appbucket" not in exc.value.message + @pytest.mark.asyncio + async def test_space_bearing_name_matches_human_readable_glob(self): + # DialFileService returns decoded entry URLs, so globs match the + # human-readable spelling — no percent-encoding leaks into matching. + entries = [ + _file("files/appbucket/Uno-Rules-PDF-Official-Rules-unorules.org_ (1).pdf"), + _file("files/appbucket/other.pdf"), + ] + tool = _make_tool(entries=entries) + result = await tool._run_in_stage_async(stage_wrapper=None, pattern="*(1).pdf") + assert "Uno-Rules-PDF-Official-Rules-unorules.org_ (1).pdf" in result.content + assert "other.pdf" not in result.content + @pytest.mark.asyncio async def test_no_matches_returns_no_files(self): tool = _make_tool(entries=[_file("files/appbucket/a.txt")])