fix: percent-encode storage resource paths on the wire (#124)

aidial_client/helpers/storage_resource.py

@@ -1,6 +1,6 @@
 from pathlib import PurePosixPath
 from typing import Literal, cast, get_args
-from urllib.parse import urljoin, urlparse
+from urllib.parse import quote, unquote, urljoin, urlparse, urlsplit
 
 from aidial_client._compatibility.pydantic_v1 import BaseModel
 from aidial_client._constants import API_PREFIX
@@ -12,6 +12,22 @@
 StorageResourceType = Literal["files", "conversations", "prompts"]
 
 
+def percent_encode_resource_url(url: str) -> str:
+    """
+    Percent-encode each path segment so reserved characters (space, ``#``,
+    ``?``, ``[`` …) reach DIAL Core encoded instead of making it answer 500.
+    Segments are decoded first, so a decoded path (``my file.txt``) and an
+    already-encoded one (``my%20file.txt``, as returned by the API) converge
+    without double-encoding. Absolute URLs come from the API already encoded and
+    are returned untouched.
+    """
+    if urlsplit(url).netloc:
+        return url
+
+    segments = url.split("/")
+    return "/".join(quote(unquote(seg), safe="") for seg in segments)
+
+
 def _is_directory(s: str) -> bool:
     return s[-1] == "/"
 
@@ -153,6 +169,15 @@ def get_api_path(self, url: str) -> str:
         """
         return self.get_storage_resource(url).api_path
 
+    def get_encoded_api_path(self, url: str) -> str:
+        """
+        Relative api path with every segment percent-encoded for the wire.
+
+        Encodes before parsing so reserved characters (notably ``#`` and ``?``,
+        which ``urlparse`` would otherwise drop as fragment/query) survive.
+        """
+        return self.get_api_path(percent_encode_resource_url(url))
+
     def get_display_name(self, url: str) -> str:
         """
         Get the display name of the resource from the URL
@@ -164,7 +189,9 @@ def _prepare_download_request(
         url: str | PurePosixPath,
         etag_if_match: str | None,
     ) -> tuple[FinalRequestOptions, str]:
-        storage_resource = self.get_storage_resource(str(url))
+        storage_resource = self.get_storage_resource(
+            percent_encode_resource_url(str(url))
+        )
 
         if storage_resource.filename is None:
             raise InvalidDialURLError("URL points to a directory, not a file")
@@ -179,4 +206,5 @@ def _prepare_download_request(
             ),
         )
 
-        return options, storage_resource.filename
+        # api_path is percent-encoded; return a human-readable filename.
+        return options, unquote(storage_resource.filename)

aidial_client/resources/files.py

@@ -25,6 +25,19 @@
 from aidial_client.types.metadata import FileItem, FileMetadata
 
 
+def _move_copy_body(
+    resource: DialStorageResourceMixin,
+    source: str | PurePosixPath,
+    destination: str | PurePosixPath,
+    overwrite: bool,
+) -> dict[str, object]:
+    return {
+        "sourceUrl": resource.get_encoded_api_path(str(source)),
+        "destinationUrl": resource.get_encoded_api_path(str(destination)),
+        "overwrite": overwrite,
+    }
+
+
 def _files_error_processor(
     http_status_error: httpx.HTTPStatusError,
 ) -> DialException | None:
@@ -54,7 +67,7 @@ def upload(
             cast_to=FileItem,
             options=FinalRequestOptions(
                 method="PUT",
-                url=urljoin(API_PREFIX, self.get_api_path(str(url))),
+                url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))),
                 files={"file": file},
                 headers=remove_none(
                     {
@@ -88,7 +101,7 @@ def delete(
             cast_to=NoneType,
             options=FinalRequestOptions(
                 method="DELETE",
-                url=urljoin(API_PREFIX, self.get_api_path(str(url))),
+                url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))),
                 headers=remove_none(
                     {
                         "If-Match": etag_if_match,
@@ -109,11 +122,7 @@ def move_to(
             options=FinalRequestOptions(
                 method="POST",
                 url=urljoin(API_PREFIX, "ops/resource/move"),
-                json_data={
-                    "sourceUrl": self.get_api_path(str(source)),
-                    "destinationUrl": self.get_api_path(str(destination)),
-                    "overwrite": overwrite,
-                },
+                json_data=_move_copy_body(self, source, destination, overwrite),
             ),
             on_http_error=_files_error_processor,
         )
@@ -129,11 +138,7 @@ def copy_to(
             options=FinalRequestOptions(
                 method="POST",
                 url=urljoin(API_PREFIX, "ops/resource/copy"),
-                json_data={
-                    "sourceUrl": self.get_api_path(str(source)),
-                    "destinationUrl": self.get_api_path(str(destination)),
-                    "overwrite": overwrite,
-                },
+                json_data=_move_copy_body(self, source, destination, overwrite),
             ),
             on_http_error=_files_error_processor,
         )
@@ -147,7 +152,7 @@ def get_metadata(
     ) -> FileMetadata:
         return self.metadata.get(
             resource="files",
-            relative_url=self.get_api_path(str(url)),
+            relative_url=self.get_encoded_api_path(str(url)),
             limit=limit,
             token=token,
         )
@@ -168,7 +173,7 @@ async def upload(
             cast_to=FileItem,
             options=FinalRequestOptions(
                 method="PUT",
-                url=urljoin(API_PREFIX, self.get_api_path(str(url))),
+                url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))),
                 files={"file": file},
                 headers=remove_none(
                     {
@@ -215,7 +220,7 @@ async def delete(
             cast_to=NoneType,
             options=FinalRequestOptions(
                 method="DELETE",
-                url=urljoin(API_PREFIX, self.get_api_path(str(url))),
+                url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))),
                 headers=remove_none(
                     {
                         "If-Match": etag_if_match,
@@ -236,11 +241,7 @@ async def move_to(
             options=FinalRequestOptions(
                 method="POST",
                 url=urljoin(API_PREFIX, "ops/resource/move"),
-                json_data={
-                    "sourceUrl": self.get_api_path(str(source)),
-                    "destinationUrl": self.get_api_path(str(destination)),
-                    "overwrite": overwrite,
-                },
+                json_data=_move_copy_body(self, source, destination, overwrite),
             ),
             on_http_error=_files_error_processor,
         )
@@ -256,11 +257,7 @@ async def copy_to(
             options=FinalRequestOptions(
                 method="POST",
                 url=urljoin(API_PREFIX, "ops/resource/copy"),
-                json_data={
-                    "sourceUrl": self.get_api_path(str(source)),
-                    "destinationUrl": self.get_api_path(str(destination)),
-                    "overwrite": overwrite,
-                },
+                json_data=_move_copy_body(self, source, destination, overwrite),
             ),
             on_http_error=_files_error_processor,
         )
@@ -274,7 +271,7 @@ async def get_metadata(
     ) -> FileMetadata:
         return await self.metadata.get(
             resource="files",
-            relative_url=self.get_api_path(str(url)),
+            relative_url=self.get_encoded_api_path(str(url)),
             limit=limit,
             token=token,
         )

aidial_client/resources/metadata.py

@@ -6,7 +6,10 @@
 from aidial_client._constants import METADATA_PREFIX
 from aidial_client._internal_types._http_request import FinalRequestOptions
 from aidial_client._utils._dict import remove_none
-from aidial_client.helpers.storage_resource import StorageResourceType
+from aidial_client.helpers.storage_resource import (
+    StorageResourceType,
+    percent_encode_resource_url,
+)
 from aidial_client.resources.base import AsyncResource, Resource
 from aidial_client.types.metadata import (
     ConversationMetadata,
@@ -71,7 +74,10 @@ def get(
             cast_to=_get_cast_to(resource),
             options=FinalRequestOptions(
                 method="GET",
-                url=urljoin(METADATA_PREFIX, relative_url),
+                url=urljoin(
+                    METADATA_PREFIX,
+                    percent_encode_resource_url(relative_url),
+                ),
                 params=remove_none({"limit": limit, "token": token}),
             ),
         )
@@ -120,7 +126,10 @@ async def get(
             cast_to=_get_cast_to(resource),
             options=FinalRequestOptions(
                 method="GET",
-                url=urljoin(METADATA_PREFIX, relative_url),
+                url=urljoin(
+                    METADATA_PREFIX,
+                    percent_encode_resource_url(relative_url),
+                ),
                 params=remove_none({"limit": limit, "token": token}),
             ),
         )

aidial_client/resources/prompts.py

@@ -56,7 +56,7 @@ def save(
             cast_to=PromptItem,
             options=FinalRequestOptions(
                 method="PUT",
-                url=urljoin(API_PREFIX, self.get_api_path(str(url))),
+                url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))),
                 json_data=_prompt_to_json(prompt),
                 headers=remove_none(
                     {
@@ -74,7 +74,7 @@ def get(self, url: str | PurePosixPath) -> Prompt:
             cast_to=Prompt,
             options=FinalRequestOptions(
                 method="GET",
-                url=urljoin(API_PREFIX, self.get_api_path(str(url))),
+                url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))),
             ),
             on_http_error=_prompts_error_processor,
         )
@@ -88,7 +88,7 @@ def delete(
             cast_to=NoneType,
             options=FinalRequestOptions(
                 method="DELETE",
-                url=urljoin(API_PREFIX, self.get_api_path(str(url))),
+                url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))),
                 headers=remove_none(
                     {
                         "If-Match": etag_if_match,
@@ -101,7 +101,7 @@ def delete(
     def get_metadata(self, url: str | PurePosixPath) -> PromptMetadata:
         return self.metadata.get(
             resource="prompts",
-            relative_url=self.get_api_path(str(url)),
+            relative_url=self.get_encoded_api_path(str(url)),
         )
 
 
@@ -120,7 +120,7 @@ async def save(
             cast_to=PromptItem,
             options=FinalRequestOptions(
                 method="PUT",
-                url=urljoin(API_PREFIX, self.get_api_path(str(url))),
+                url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))),
                 json_data=_prompt_to_json(prompt),
                 headers=remove_none(
                     {
@@ -138,7 +138,7 @@ async def get(self, url: str | PurePosixPath) -> Prompt:
             cast_to=Prompt,
             options=FinalRequestOptions(
                 method="GET",
-                url=urljoin(API_PREFIX, self.get_api_path(str(url))),
+                url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))),
             ),
             on_http_error=_prompts_error_processor,
         )
@@ -152,7 +152,7 @@ async def delete(
             cast_to=NoneType,
             options=FinalRequestOptions(
                 method="DELETE",
-                url=urljoin(API_PREFIX, self.get_api_path(str(url))),
+                url=urljoin(API_PREFIX, self.get_encoded_api_path(str(url))),
                 headers=remove_none(
                     {
                         "If-Match": etag_if_match,
@@ -165,5 +165,5 @@ async def delete(
     async def get_metadata(self, url: str | PurePosixPath) -> PromptMetadata:
         return await self.metadata.get(
             resource="prompts",
-            relative_url=self.get_api_path(str(url)),
+            relative_url=self.get_encoded_api_path(str(url)),
         )

aidial_client/resources/resource_permissions.py

@@ -1,5 +1,8 @@
 from aidial_client._internal_types._generic import NoneType
 from aidial_client._internal_types._http_request import FinalRequestOptions
+from aidial_client.helpers.storage_resource import (
+    percent_encode_resource_url,
+)
 from aidial_client.resources.base import AsyncResource, Resource
 
 _GRANT_URL = "v1/ops/resource/per-request-permissions/grant"
@@ -21,7 +24,10 @@ def grant(
                 url=_GRANT_URL,
                 json_data={
                     "resourcePermissions": [
-                        {"url": url, "permissions": permissions}
+                        {
+                            "url": percent_encode_resource_url(url),
+                            "permissions": permissions,
+                        }
                         for url in resources
                     ],
                     "receiver": receiver,
@@ -46,7 +52,10 @@ async def grant(
                 url=_GRANT_URL,
                 json_data={
                     "resourcePermissions": [
-                        {"url": url, "permissions": permissions}
+                        {
+                            "url": percent_encode_resource_url(url),
+                            "permissions": permissions,
+                        }
                         for url in resources
                     ],
                     "receiver": receiver,

tests/resources/files/test_download.py

@@ -1,14 +1,61 @@
 from typing import Any, cast
-from unittest.mock import AsyncMock
+from unittest.mock import AsyncMock, Mock
 
 import httpx
 import pytest
 
+from aidial_client import Dial
 from aidial_client._client import AsyncDial
 from aidial_client._exception import InvalidDialURLError
 from tests.client_mock import MockStreamIterator
 
 
+def _make_capturing_client(captured: list[httpx.Request]) -> Dial:
+    client = Dial(api_key="dummy", base_url="http://dial.core")
+
+    def send_mock(request: httpx.Request, **_: Any) -> httpx.Response:
+        captured.append(request)
+        return httpx.Response(status_code=200, request=request, content=b"x")
+
+    client._http_client._internal_http_client.send = cast(Any, send_mock)
+    client._get_my_bucket = cast(Any, Mock(return_value="test-bucket"))
+    return client
+
+
+@pytest.mark.parametrize(
+    "raw, encoded",
+    [
+        ("my file.txt", "my%20file.txt"),
+        ("a#b.txt", "a%23b.txt"),
+        ("a?b.txt", "a%3Fb.txt"),
+        ("tag[1].txt", "tag%5B1%5D.txt"),
+    ],
+)
+def test_download_encodes_url_and_decodes_filename(raw: str, encoded: str):
+    captured: list[httpx.Request] = []
+    client = _make_capturing_client(captured)
+
+    response = client.files.download(f"files/test-bucket/{raw}")
+
+    assert (
+        captured[0].url.raw_path.decode() == f"/v1/files/test-bucket/{encoded}"
+    )
+    assert response.filename == raw
+
+
+def test_download_accepts_already_encoded_url():
+    captured: list[httpx.Request] = []
+    client = _make_capturing_client(captured)
+
+    response = client.files.download("files/test-bucket/my%20file.txt")
+
+    assert (
+        captured[0].url.raw_path.decode()
+        == "/v1/files/test-bucket/my%20file.txt"
+    )
+    assert response.filename == "my file.txt"
+
+
 @pytest.mark.asyncio
 async def test_stream_download_async_streams_and_closes_response():
     captured_requests: list[httpx.Request] = []