diff --git a/aidial_client/helpers/storage_resource.py b/aidial_client/helpers/storage_resource.py index 27c9501..4702595 100644 --- a/aidial_client/helpers/storage_resource.py +++ b/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) diff --git a/aidial_client/resources/files.py b/aidial_client/resources/files.py index ce3de02..2ba4e8f 100644 --- a/aidial_client/resources/files.py +++ b/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, ) diff --git a/aidial_client/resources/metadata.py b/aidial_client/resources/metadata.py index acf460a..86faf2a 100644 --- a/aidial_client/resources/metadata.py +++ b/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}), ), ) diff --git a/aidial_client/resources/prompts.py b/aidial_client/resources/prompts.py index 94b1c2a..6e9130b 100644 --- a/aidial_client/resources/prompts.py +++ b/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)), ) diff --git a/aidial_client/resources/resource_permissions.py b/aidial_client/resources/resource_permissions.py index 494aa41..939dd3c 100644 --- a/aidial_client/resources/resource_permissions.py +++ b/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, diff --git a/tests/resources/files/test_download.py b/tests/resources/files/test_download.py index d5320d3..439bc38 100644 --- a/tests/resources/files/test_download.py +++ b/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] = [] diff --git a/tests/resources/files/test_metadata.py b/tests/resources/files/test_metadata.py index 8c41751..4601b82 100644 --- a/tests/resources/files/test_metadata.py +++ b/tests/resources/files/test_metadata.py @@ -86,6 +86,30 @@ def test_get_metadata(): assert r.next_token == "next-page-token" # noqa: S105 +def test_get_metadata_encodes_reserved_characters(): + captured: list[httpx.Request] = [] + client = _make_capturing_client(captured) + + client.files.get_metadata(url="files/test-bucket/c#py (1)?.md") + + assert captured[0].url.raw_path.decode() == ( + "/v1/metadata/files/test-bucket/c%23py%20%281%29%3F.md" + ) + + +def test_low_level_metadata_get_encodes_reserved_characters(): + # The generic client.metadata.get(...) is also used directly (bypassing + # files.get_metadata), so it must encode the path too. + captured: list[httpx.Request] = [] + client = _make_capturing_client(captured) + + client.metadata.get("files", "files/test-bucket/c#py (1)?.md") + + assert captured[0].url.raw_path.decode() == ( + "/v1/metadata/files/test-bucket/c%23py%20%281%29%3F.md" + ) + + def test_get_metadata_sends_pagination_params(): captured: list[httpx.Request] = [] client = _make_capturing_client(captured) diff --git a/tests/resources/files/test_move_copy.py b/tests/resources/files/test_move_copy.py index f517db8..d2733f5 100644 --- a/tests/resources/files/test_move_copy.py +++ b/tests/resources/files/test_move_copy.py @@ -103,6 +103,70 @@ def test_accepts_pureposixpath_and_absolute_urls(method: str): assert body["destinationUrl"] == "files/test-bucket/final/file.txt" +# Every reserved character must reach DIAL Core percent-encoded, otherwise +# `new URI(...)` on the server throws and it answers 500. `#` and `?` also must +# survive (a naive urlparse would drop them as fragment/query). +RESERVED_CASES = [ + ("my file.txt", "my%20file.txt"), + ("50% off.txt", "50%25%20off.txt"), + ("a#b.txt", "a%23b.txt"), + ("a?b.txt", "a%3Fb.txt"), + ("a&b.txt", "a%26b.txt"), + ("tag[1].txt", "tag%5B1%5D.txt"), + ("naïve.txt", "na%C3%AFve.txt"), +] +parametrize_reserved = pytest.mark.parametrize("raw, encoded", RESERVED_CASES) + + +@parametrize_method +@parametrize_reserved +def test_encodes_reserved_characters_in_body( + method: str, raw: str, encoded: str +): + captured: list[httpx.Request] = [] + client = _make_capturing_client(captured) + + getattr(client.files, method)( + source=f"files/test-bucket/{raw}", + destination="files/test-bucket/dst.txt", + ) + + assert _body(captured[0])["sourceUrl"] == f"files/test-bucket/{encoded}" + + +@parametrize_method +@parametrize_reserved +@pytest.mark.asyncio +async def test_async_encodes_reserved_characters_in_body( + method: str, raw: str, encoded: str +): + captured: list[httpx.Request] = [] + client = _make_async_capturing_client(captured) + + await getattr(client.files, method)( + source=f"files/test-bucket/{raw}", + destination="files/test-bucket/dst.txt", + ) + + assert _body(captured[0])["sourceUrl"] == f"files/test-bucket/{encoded}" + + +@parametrize_method +def test_already_encoded_input_is_not_double_encoded(method: str): + captured: list[httpx.Request] = [] + client = _make_capturing_client(captured) + + # A url as returned by the API (percent-encoded) must round-trip unchanged. + getattr(client.files, method)( + source="files/test-bucket/my%20file.txt", + destination="files/test-bucket/50%25%20off.txt", + ) + + body = _body(captured[0]) + assert body["sourceUrl"] == "files/test-bucket/my%20file.txt" + assert body["destinationUrl"] == "files/test-bucket/50%25%20off.txt" + + @parametrize_method @pytest.mark.parametrize( "bad_arg",