From ccc88c6845ea4cee2bd59f8595a5d77235bf40c6 Mon Sep 17 00:00:00 2001 From: chengwudi1 Date: Sat, 22 Aug 2026 21:09:22 +0800 Subject: [PATCH] fix(storage): close upload file handle when the request fails --- src/storage/src/storage3/_async/file_api.py | 53 +++++----- src/storage/src/storage3/_sync/file_api.py | 53 +++++----- src/storage/tests/_async/test_file_api.py | 102 ++++++++++++++++++++ src/storage/tests/_sync/test_file_api.py | 102 ++++++++++++++++++++ 4 files changed, 262 insertions(+), 48 deletions(-) create mode 100644 src/storage/tests/_async/test_file_api.py create mode 100644 src/storage/tests/_sync/test_file_api.py diff --git a/src/storage/src/storage3/_async/file_api.py b/src/storage/src/storage3/_async/file_api.py index 474486f1..85d0e9ec 100644 --- a/src/storage/src/storage3/_async/file_api.py +++ b/src/storage/src/storage3/_async/file_api.py @@ -65,31 +65,36 @@ async def _request( **kwargs: Any, ) -> Response: try: - url_path = self._base_url.joinpath(*path).with_query(query_params) - headers = headers or dict() - headers.update(self._headers) - response = await self._client.request( - method, - str(url_path), - headers=headers, - json=json, - files=files, - **kwargs, - ) - response.raise_for_status() - except HTTPStatusError as exc: try: - resp = exc.response.json() - raise StorageApiError( - resp["message"], resp["error"], resp["statusCode"] - ) from exc - except KeyError as err: - message = f"Unable to parse error message: {resp.text}" - raise StorageApiError(message, "InternalError", 400) from err - - # close the resource before returning the response - if files and "file" in files and isinstance(files["file"][1], BufferedReader): - files["file"][1].close() + url_path = self._base_url.joinpath(*path).with_query(query_params) + headers = headers or dict() + headers.update(self._headers) + response = await self._client.request( + method, + str(url_path), + headers=headers, + json=json, + files=files, + **kwargs, + ) + response.raise_for_status() + except HTTPStatusError as exc: + try: + resp = exc.response.json() + raise StorageApiError( + resp["message"], resp["error"], resp["statusCode"] + ) from exc + except KeyError as err: + message = f"Unable to parse error message: {resp.text}" + raise StorageApiError(message, "InternalError", 400) from err + finally: + # close the resource before returning the response + if ( + files + and "file" in files + and isinstance(files["file"][1], BufferedReader) + ): + files["file"][1].close() return response diff --git a/src/storage/src/storage3/_sync/file_api.py b/src/storage/src/storage3/_sync/file_api.py index 1bc22a09..c3e94c6b 100644 --- a/src/storage/src/storage3/_sync/file_api.py +++ b/src/storage/src/storage3/_sync/file_api.py @@ -65,31 +65,36 @@ def _request( **kwargs: Any, ) -> Response: try: - url_path = self._base_url.joinpath(*path).with_query(query_params) - headers = headers or dict() - headers.update(self._headers) - response = self._client.request( - method, - str(url_path), - headers=headers, - json=json, - files=files, - **kwargs, - ) - response.raise_for_status() - except HTTPStatusError as exc: try: - resp = exc.response.json() - raise StorageApiError( - resp["message"], resp["error"], resp["statusCode"] - ) from exc - except KeyError as err: - message = f"Unable to parse error message: {resp.text}" - raise StorageApiError(message, "InternalError", 400) from err - - # close the resource before returning the response - if files and "file" in files and isinstance(files["file"][1], BufferedReader): - files["file"][1].close() + url_path = self._base_url.joinpath(*path).with_query(query_params) + headers = headers or dict() + headers.update(self._headers) + response = self._client.request( + method, + str(url_path), + headers=headers, + json=json, + files=files, + **kwargs, + ) + response.raise_for_status() + except HTTPStatusError as exc: + try: + resp = exc.response.json() + raise StorageApiError( + resp["message"], resp["error"], resp["statusCode"] + ) from exc + except KeyError as err: + message = f"Unable to parse error message: {resp.text}" + raise StorageApiError(message, "InternalError", 400) from err + finally: + # close the resource before returning the response + if ( + files + and "file" in files + and isinstance(files["file"][1], BufferedReader) + ): + files["file"][1].close() return response diff --git a/src/storage/tests/_async/test_file_api.py b/src/storage/tests/_async/test_file_api.py new file mode 100644 index 00000000..39344cc2 --- /dev/null +++ b/src/storage/tests/_async/test_file_api.py @@ -0,0 +1,102 @@ +from io import BufferedReader +from pathlib import Path +from typing import Generator +from unittest.mock import AsyncMock, MagicMock, Mock, mock_open, patch + +import pytest +from httpx import Headers, HTTPStatusError, Request, Response +from storage3._async.file_api import AsyncBucketProxy +from storage3.exceptions import StorageApiError +from yarl import URL + + +@pytest.fixture +def mock_client() -> AsyncMock: + return AsyncMock(headers=Headers()) + + +@pytest.fixture +def file_api(mock_client: AsyncMock) -> AsyncBucketProxy: + return AsyncBucketProxy( + id="bucket", + _base_url=URL("http://example.com"), + _headers=Headers(), + _client=mock_client, + ) + + +class _MalformedErrorBody(dict): + """An error body missing message/error/statusCode, carrying .text like httpx responses.""" + + text = "mock error body without error fields" + + +def _error_response() -> Mock: + response = Mock(spec=Response) + response.json.return_value = _MalformedErrorBody() + response.raise_for_status = Mock( + side_effect=HTTPStatusError( + "Conflict", + request=Mock(spec=Request), + response=response, + ) + ) + return response + + +def _success_response() -> Mock: + response = Mock(spec=Response) + response.raise_for_status = Mock() + response.json.return_value = {"Key": "file.txt"} + return response + + +def _make_file(tmp_path, name="data.bin", size=1024) -> Path: + file_path = tmp_path / name + file_path.write_bytes(b"x" * size) + return file_path + + +@pytest.fixture +def opened_handle() -> Generator[Mock, None, None]: + """A builtins.open mock whose returned handle passes the BufferedReader check.""" + m = mock_open(read_data=b"x" * 1024) + m.return_value = MagicMock(spec=BufferedReader) + with patch("builtins.open", m) as patched: + yield patched + + +async def test_upload_closes_file_handle_on_error( + mock_client: AsyncMock, file_api: AsyncBucketProxy, tmp_path, opened_handle +) -> None: + """A handle storage3 opened itself must be closed even when the upload fails (#1575).""" + mock_client.request.return_value = _error_response() + + with pytest.raises(StorageApiError): + await file_api.upload("file.txt", str(_make_file(tmp_path))) + + opened_handle.return_value.close.assert_called_once() + + +async def test_upload_closes_file_handle_on_success( + mock_client: AsyncMock, file_api: AsyncBucketProxy, tmp_path, opened_handle +) -> None: + """Regression: the success path must keep closing the handle it opened.""" + mock_client.request.return_value = _success_response() + + response = await file_api.upload("file.txt", str(_make_file(tmp_path))) + + opened_handle.return_value.close.assert_called_once() + assert response.fullPath == "file.txt" + + +async def test_update_closes_file_handle_on_error( + mock_client: AsyncMock, file_api: AsyncBucketProxy, tmp_path, opened_handle +) -> None: + """update() shares _upload_or_update, so it is affected the same way.""" + mock_client.request.return_value = _error_response() + + with pytest.raises(StorageApiError): + await file_api.update("file.txt", str(_make_file(tmp_path))) + + opened_handle.return_value.close.assert_called_once() diff --git a/src/storage/tests/_sync/test_file_api.py b/src/storage/tests/_sync/test_file_api.py new file mode 100644 index 00000000..b0d11028 --- /dev/null +++ b/src/storage/tests/_sync/test_file_api.py @@ -0,0 +1,102 @@ +from io import BufferedReader +from pathlib import Path +from typing import Generator +from unittest.mock import MagicMock, Mock, mock_open, patch + +import pytest +from httpx import Headers, HTTPStatusError, Request, Response +from storage3._sync.file_api import SyncBucketProxy +from storage3.exceptions import StorageApiError +from yarl import URL + + +@pytest.fixture +def mock_client() -> Mock: + return Mock(headers=Headers()) + + +@pytest.fixture +def file_api(mock_client: Mock) -> SyncBucketProxy: + return SyncBucketProxy( + id="bucket", + _base_url=URL("http://example.com"), + _headers=Headers(), + _client=mock_client, + ) + + +class _MalformedErrorBody(dict): + """An error body missing message/error/statusCode, carrying .text like httpx responses.""" + + text = "mock error body without error fields" + + +def _error_response() -> Mock: + response = Mock(spec=Response) + response.json.return_value = _MalformedErrorBody() + response.raise_for_status = Mock( + side_effect=HTTPStatusError( + "Conflict", + request=Mock(spec=Request), + response=response, + ) + ) + return response + + +def _success_response() -> Mock: + response = Mock(spec=Response) + response.raise_for_status = Mock() + response.json.return_value = {"Key": "file.txt"} + return response + + +def _make_file(tmp_path, name="data.bin", size=1024) -> Path: + file_path = tmp_path / name + file_path.write_bytes(b"x" * size) + return file_path + + +@pytest.fixture +def opened_handle() -> Generator[Mock, None, None]: + """A builtins.open mock whose returned handle passes the BufferedReader check.""" + m = mock_open(read_data=b"x" * 1024) + m.return_value = MagicMock(spec=BufferedReader) + with patch("builtins.open", m) as patched: + yield patched + + +def test_upload_closes_file_handle_on_error( + mock_client: Mock, file_api: SyncBucketProxy, tmp_path, opened_handle +) -> None: + """A handle storage3 opened itself must be closed even when the upload fails (#1575).""" + mock_client.request.return_value = _error_response() + + with pytest.raises(StorageApiError): + file_api.upload("file.txt", str(_make_file(tmp_path))) + + opened_handle.return_value.close.assert_called_once() + + +def test_upload_closes_file_handle_on_success( + mock_client: Mock, file_api: SyncBucketProxy, tmp_path, opened_handle +) -> None: + """Regression: the success path must keep closing the handle it opened.""" + mock_client.request.return_value = _success_response() + + response = file_api.upload("file.txt", str(_make_file(tmp_path))) + + opened_handle.return_value.close.assert_called_once() + assert response.fullPath == "file.txt" + + +def test_update_closes_file_handle_on_error( + mock_client: Mock, file_api: SyncBucketProxy, tmp_path, opened_handle +) -> None: + """update() shares _upload_or_update, so it is affected the same way.""" + mock_client.request.return_value = _error_response() + + with pytest.raises(StorageApiError): + file_api.update("file.txt", str(_make_file(tmp_path))) + + opened_handle.return_value.close.assert_called_once()