From 1de9476c6391fb957e19d4e8b68f8a2a06cb2ec1 Mon Sep 17 00:00:00 2001 From: laughingman7743 Date: Sat, 24 Oct 2026 14:46:42 +0900 Subject: [PATCH 1/2] Translate delete-marker versions to FileNotFoundError and fix two S3Core edge cases (GH-1338) - A 405 response with the x-amz-delete-marker: true header, from HeadObject or GetObject of a version that is a delete marker, is translated to FileNotFoundError, so exists() returns False for it. Other 405 responses still map to EPERM. - S3Core.part_ranges() returns no ranges for an empty object instead of raising ValueError. - S3Core.call() with request_kwargs resolves the operation by the callable's __name__ and sends a callable without one, such as a functools.partial, without request_kwargs instead of raising AttributeError. Co-Authored-By: Claude Opus 5.5 --- docs/filesystem.md | 1 + pyathena/filesystem/s3_core.py | 11 ++++-- pyathena/filesystem/s3_errors.py | 13 +++++-- tests/pyathena/filesystem/test_s3_core.py | 43 +++++++++++++++++++++++ 4 files changed, 63 insertions(+), 5 deletions(-) diff --git a/docs/filesystem.md b/docs/filesystem.md index 813831860..a4e279500 100644 --- a/docs/filesystem.md +++ b/docs/filesystem.md @@ -243,6 +243,7 @@ operations raise natural errors instead of botocore's `ClientError`: | S3 error | Python exception | | --- | --- | | `404` / `NoSuchKey` / `NoSuchBucket` | `FileNotFoundError` | +| `405` of a version that is a delete marker (`x-amz-delete-marker: true`) | `FileNotFoundError` | | `403` / `AccessDenied` | `PermissionError` | | `BucketAlreadyExists` / `BucketAlreadyOwnedByYou` | `FileExistsError` | | `PreconditionFailed` of an `If-None-Match` condition | `FileExistsError` | diff --git a/pyathena/filesystem/s3_core.py b/pyathena/filesystem/s3_core.py index 737d2e066..12d9ab61f 100644 --- a/pyathena/filesystem/s3_core.py +++ b/pyathena/filesystem/s3_core.py @@ -620,7 +620,9 @@ def call(self, method: str | Callable[..., Any], **request) -> dict[str, Any]: """Send a request with the retry policy and translate its errors. ``request_kwargs`` that the operation accepts are added; a parameter - of the request takes precedence. + of the request takes precedence. The operation is identified by the + name of the method, so a callable whose ``__name__`` is not that of a + client method, such as a ``functools.partial``, is sent without them. Args: method: The name of the client method, or the method itself. @@ -641,7 +643,8 @@ def call(self, method: str | Callable[..., Any], **request) -> dict[str, Any]: """ func = getattr(self._client, method) if isinstance(method, str) else method if self._request_kwargs: - request = {**self.operation_params(func.__name__, self._request_kwargs), **request} + name = getattr(func, "__name__", "") + request = {**self.operation_params(name, self._request_kwargs), **request} try: response = retry_api_call(func, config=self._retry_config, logger=_logger, **request) except BaseException as e: @@ -1115,10 +1118,12 @@ def part_ranges(self, size: int, block_size: int) -> list[tuple[int, int]]: Returns: The ``(start, end)`` byte ranges, with an exclusive end, that - cover the whole object in order. + cover the whole object in order; empty for an empty object. """ block_size = max(block_size, math.ceil(size / self.MULTIPART_UPLOAD_MAX_PARTS)) starts = list(range(0, size, block_size)) + if not starts: + return [] if len(starts) > 1 and size - starts[-1] < self.MULTIPART_UPLOAD_MIN_PART_SIZE: starts.pop() if size - starts[-1] > self.MULTIPART_UPLOAD_MAX_PART_SIZE: diff --git a/pyathena/filesystem/s3_errors.py b/pyathena/filesystem/s3_errors.py index f77623501..d7f167fdd 100644 --- a/pyathena/filesystem/s3_errors.py +++ b/pyathena/filesystem/s3_errors.py @@ -35,7 +35,9 @@ class S3ClientError: by its HTTP status code; if neither is recognized, a generic ``OSError`` with the original error message is used. A ``PreconditionFailed`` error of an ``If-None-Match`` condition, from a write that must not replace an - existing object, is mapped to ``FileExistsError``. + existing object, is mapped to ``FileExistsError``. A ``405`` error with + the ``x-amz-delete-marker: true`` header, from a request for a version + that is a delete marker, is mapped to ``FileNotFoundError``. Example: >>> try: @@ -108,14 +110,21 @@ def __init__(self, error: botocore.exceptions.ClientError) -> None: self._code: str = str(error_info.get("Code", "")) self._message: str = str(error_info.get("Message", error)) self._condition: str = str(error_info.get("Condition", "")) - status_code = error.response.get("ResponseMetadata", {}).get("HTTPStatusCode") + response_metadata = error.response.get("ResponseMetadata", {}) + status_code = response_metadata.get("HTTPStatusCode") self._http_status_code: int | None = int(status_code) if status_code is not None else None + headers = response_metadata.get("HTTPHeaders", {}) + self._delete_marker: bool = headers.get("x-amz-delete-marker") == "true" self._os_error: OSError = self._translate() def _translate(self) -> OSError: if self._code == "PreconditionFailed" and self._condition == "If-None-Match": # A conditional write (IfNoneMatch="*") found an existing object. return FileExistsError(self._message) + if self._http_status_code == 405 and self._delete_marker: + # HeadObject/GetObject of a version that is a delete marker. + # https://docs.aws.amazon.com/AmazonS3/latest/userguide/DeleteMarker.html + return FileNotFoundError(self._message) exception = self._ERROR_CODE_TO_EXCEPTION.get(self._code) if exception: return exception(self._message) diff --git a/tests/pyathena/filesystem/test_s3_core.py b/tests/pyathena/filesystem/test_s3_core.py index f91b2b0a9..8ed75d107 100644 --- a/tests/pyathena/filesystem/test_s3_core.py +++ b/tests/pyathena/filesystem/test_s3_core.py @@ -6,6 +6,7 @@ # SPDX-License-Identifier: MIT import copy +import functools import gc import io import re @@ -119,6 +120,46 @@ def test_call_translates_errors(self, status, code, error): assert isinstance(e.value.__cause__, botocore.exceptions.ClientError) assert e.value.__cause__.response["Error"]["Code"] == code + @pytest.mark.parametrize( + ("method", "code", "message"), + [ + # HEAD responses have no body, so the code is the status code. + ("head_object", "405", "Method Not Allowed"), + ( + "get_object", + "MethodNotAllowed", + "The specified method is not allowed against this resource.", + ), + ], + ) + def test_delete_marker_version_is_not_found(self, method, code, message): + # GH-1338: a version that is a delete marker is answered with 405 and + # the x-amz-delete-marker header, and does not exist as an object. + core, stubber = _make_core() + stubber.add_client_error( + method, + service_error_code=code, + service_message=message, + http_status_code=405, + response_meta={"HTTPHeaders": {"x-amz-delete-marker": "true"}}, + expected_params={"Bucket": "bucket", "Key": "key", "VersionId": "marker"}, + ) + with stubber, pytest.raises(FileNotFoundError, match=message): + getattr(core, method)(S3Path("bucket", "key", "marker")) + + def test_call_callable_without_name(self): + # GH-1338: request_kwargs are selected by the name of the client + # method, so a callable without one is sent without them. + core, stubber = _make_core(request_kwargs={"RequestPayer": "requester"}) + stubber.add_response( + "head_object", {"ContentLength": 1}, expected_params={"Bucket": "bucket", "Key": "key"} + ) + with stubber: + response = core.call( + functools.partial(core.client.head_object, Bucket="bucket"), Key="key" + ) + assert response["ContentLength"] == 1 + @pytest.mark.parametrize("translated", [True, False]) def test_call_failure_frees_request(self, translated): # GH-1186: the frames of the retries and of botocore below call() in @@ -1082,6 +1123,8 @@ def test_upload_part_uses_creation_algorithm_and_identity(self): @pytest.mark.parametrize( ("size", "block_size", "ranges"), [ + # GH-1338: an empty object has no ranges. + (0, 5 * 2**20, []), # A single range. (5 * 2**20, 5 * 2**20, [(0, 5 * 2**20)]), # The size is an exact multiple of the block size. From 0b9aa8f6fe41f7b1f523e0f4953bbc88a8c848fb Mon Sep 17 00:00:00 2001 From: laughingman7743 Date: Sat, 24 Oct 2026 15:56:26 +0900 Subject: [PATCH 2/2] Resolve partials in S3Core.call() and move the delete-marker test (GH-1338) - A functools.partial of a client method is sent with the request_kwargs of the method it wraps; its bound keywords take precedence as parameters of the request. Dropping them silently would turn a requester-pays request into an unexplained 403. - part_ranges() returns [] for size 0 only, so a negative size still raises; the multipart writer no longer special-cases size 0. - The delete-marker translation is tested next to the other mappings in test_s3_errors.py. Co-Authored-By: Claude Opus 5.5 --- pyathena/filesystem/s3_core.py | 19 ++++--- pyathena/filesystem/s3_writer.py | 2 - tests/pyathena/filesystem/test_s3_core.py | 55 ++++++++------------- tests/pyathena/filesystem/test_s3_errors.py | 23 ++++++++- 4 files changed, 55 insertions(+), 44 deletions(-) diff --git a/pyathena/filesystem/s3_core.py b/pyathena/filesystem/s3_core.py index 12d9ab61f..84b98d94b 100644 --- a/pyathena/filesystem/s3_core.py +++ b/pyathena/filesystem/s3_core.py @@ -10,6 +10,7 @@ from __future__ import annotations import contextlib +import functools import logging import math import traceback @@ -621,8 +622,10 @@ def call(self, method: str | Callable[..., Any], **request) -> dict[str, Any]: ``request_kwargs`` that the operation accepts are added; a parameter of the request takes precedence. The operation is identified by the - name of the method, so a callable whose ``__name__`` is not that of a - client method, such as a ``functools.partial``, is sent without them. + name of the method; a ``functools.partial`` is identified by the + method it wraps, and its bound keywords are parameters of the request. + A callable whose ``__name__`` is not that of a client method is sent + without them. Args: method: The name of the client method, or the method itself. @@ -643,8 +646,12 @@ def call(self, method: str | Callable[..., Any], **request) -> dict[str, Any]: """ func = getattr(self._client, method) if isinstance(method, str) else method if self._request_kwargs: - name = getattr(func, "__name__", "") - request = {**self.operation_params(name, self._request_kwargs), **request} + target, bound = func, set(request) + while isinstance(target, functools.partial): + bound.update(target.keywords) + target = target.func + params = self.operation_params(getattr(target, "__name__", ""), self._request_kwargs) + request.update({k: v for k, v in params.items() if k not in bound}) try: response = retry_api_call(func, config=self._retry_config, logger=_logger, **request) except BaseException as e: @@ -1120,10 +1127,10 @@ def part_ranges(self, size: int, block_size: int) -> list[tuple[int, int]]: The ``(start, end)`` byte ranges, with an exclusive end, that cover the whole object in order; empty for an empty object. """ + if size == 0: + return [] block_size = max(block_size, math.ceil(size / self.MULTIPART_UPLOAD_MAX_PARTS)) starts = list(range(0, size, block_size)) - if not starts: - return [] if len(starts) > 1 and size - starts[-1] < self.MULTIPART_UPLOAD_MIN_PART_SIZE: starts.pop() if size - starts[-1] > self.MULTIPART_UPLOAD_MAX_PART_SIZE: diff --git a/pyathena/filesystem/s3_writer.py b/pyathena/filesystem/s3_writer.py index 8b256c465..be5defa14 100644 --- a/pyathena/filesystem/s3_writer.py +++ b/pyathena/filesystem/s3_writer.py @@ -179,8 +179,6 @@ def iter_copy_parts( raise ValueError("size must be nonnegative.") if first_part_number < 1: raise ValueError("part_number must be at least 1.") - if size == 0: - return ranges = self._core.part_ranges(size, self._core.MULTIPART_UPLOAD_MAX_PART_SIZE) for part_number, range_ in enumerate(ranges, first_part_number): self._check_part_number(part_number) diff --git a/tests/pyathena/filesystem/test_s3_core.py b/tests/pyathena/filesystem/test_s3_core.py index 8ed75d107..3c4e2a3f0 100644 --- a/tests/pyathena/filesystem/test_s3_core.py +++ b/tests/pyathena/filesystem/test_s3_core.py @@ -120,44 +120,29 @@ def test_call_translates_errors(self, status, code, error): assert isinstance(e.value.__cause__, botocore.exceptions.ClientError) assert e.value.__cause__.response["Error"]["Code"] == code - @pytest.mark.parametrize( - ("method", "code", "message"), - [ - # HEAD responses have no body, so the code is the status code. - ("head_object", "405", "Method Not Allowed"), - ( - "get_object", - "MethodNotAllowed", - "The specified method is not allowed against this resource.", - ), - ], - ) - def test_delete_marker_version_is_not_found(self, method, code, message): - # GH-1338: a version that is a delete marker is answered with 405 and - # the x-amz-delete-marker header, and does not exist as an object. - core, stubber = _make_core() - stubber.add_client_error( - method, - service_error_code=code, - service_message=message, - http_status_code=405, - response_meta={"HTTPHeaders": {"x-amz-delete-marker": "true"}}, - expected_params={"Bucket": "bucket", "Key": "key", "VersionId": "marker"}, - ) - with stubber, pytest.raises(FileNotFoundError, match=message): - getattr(core, method)(S3Path("bucket", "key", "marker")) - - def test_call_callable_without_name(self): - # GH-1338: request_kwargs are selected by the name of the client - # method, so a callable without one is sent without them. - core, stubber = _make_core(request_kwargs={"RequestPayer": "requester"}) + def test_call_partial(self): + # GH-1338: a functools.partial of a client method has no __name__; it + # is sent with the request_kwargs of the method it wraps, and its + # bound keywords take precedence as parameters of the request. + core, stubber = _make_core( + request_kwargs={"RequestPayer": "requester", "ExpectedBucketOwner": "111122223333"} + ) stubber.add_response( - "head_object", {"ContentLength": 1}, expected_params={"Bucket": "bucket", "Key": "key"} + "head_object", + {"ContentLength": 1}, + expected_params={ + "Bucket": "bucket", + "Key": "key", + "RequestPayer": "requester", + "ExpectedBucketOwner": "444455556666", + }, + ) + head_object = functools.partial( + core.client.head_object, Bucket="bucket", ExpectedBucketOwner="444455556666" ) with stubber: - response = core.call( - functools.partial(core.client.head_object, Bucket="bucket"), Key="key" - ) + response = core.call(head_object, Key="key") + stubber.assert_no_pending_responses() assert response["ContentLength"] == 1 @pytest.mark.parametrize("translated", [True, False]) diff --git a/tests/pyathena/filesystem/test_s3_errors.py b/tests/pyathena/filesystem/test_s3_errors.py index 68940cd81..dd278da6c 100644 --- a/tests/pyathena/filesystem/test_s3_errors.py +++ b/tests/pyathena/filesystem/test_s3_errors.py @@ -13,12 +13,15 @@ from pyathena.filesystem.s3_errors import S3ClientError -def _client_error(code, message="error message", status_code=None, condition=None): +def _client_error(code, message="error message", status_code=None, condition=None, headers=None): error_response = {"Error": {"Code": code, "Message": message}} if condition is not None: error_response["Error"]["Condition"] = condition if status_code is not None: error_response["ResponseMetadata"] = {"HTTPStatusCode": status_code} + if headers is not None: + # botocore lowercases the names of the response headers. + error_response.setdefault("ResponseMetadata", {})["HTTPHeaders"] = headers return botocore.exceptions.ClientError(error_response, "TestOperation") @@ -109,3 +112,21 @@ def test_os_error_precondition_failed(self, condition, expected): if expected is OSError: assert actual.errno == errno.EINVAL assert "error message" in str(actual) + + @pytest.mark.parametrize( + "code", + [ + # HEAD responses have no body, so the code is the status code. + "405", + "MethodNotAllowed", + ], + ) + def test_os_error_delete_marker(self, code): + # GH-1338: HeadObject/GetObject of a version that is a delete marker + # is answered with 405 and the x-amz-delete-marker header, and the + # version does not exist as an object. Other 405 errors map to EPERM. + actual = S3ClientError( + _client_error(code, status_code=405, headers={"x-amz-delete-marker": "true"}) + ).os_error + assert type(actual) is FileNotFoundError + assert "error message" in str(actual)