Repository navigation
Document that copies of a delete-marker source version raise OSError(EINVAL) (GH-1346) - #1348
laughingman7743 wants to merge 3 commits into
Conversation
…EINVAL) (GH-1346) CopyObject and UploadPartCopy whose source names a delete-marker version are answered with 400 InvalidRequest, without the x-amz-delete-marker header, so S3Core.copy_object() and upload_part_copy() raise OSError with EINVAL rather than FileNotFoundError. The filesystem's copies look up the source first and raise FileNotFoundError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
plan_multipart_copy() looks up the source with HeadObject first and raises FileNotFoundError for a delete-marker version, so the class docstring names copy_object() and upload_part_copy() instead of every copy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- The filesystem paragraph names cp_file() only: copy() and mv() follow fsspec, which skips a missing source with on_error="ignore" (the recursive default) and drops a missing version from expand_path(). - copy_object() and upload_part_copy() list FileNotFoundError for every missing resource they can report, measured for a removed source version and a missing source key, and EINVAL for an invalid request, which a malformed version ID also gives. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| invalidate the filesystem's cache: call | ||
| version that it reads, and caches nothing. `copy_object()` and `upload_part_copy()` with | ||
| a source version that is a delete marker raise `OSError` with `EINVAL`, while the | ||
| filesystem's `cp_file()` raises `FileNotFoundError` for it, as for a missing file. |
There was a problem hiding this comment.
Self-review round one (behavior of the documented paths, simplicity), base 1a6a0d6c3f137d47bbe96e274ecd467e016fcb38. The initial pass covered heads 5addecda926178912127e3b6049cfbe5f8e91209 and 247bf10687ffcaca9f41d37350fea88cd78f8383 (my own pass, /code-review high, /simplify). Result: FINDINGS, repaired in 247bf106 and d3ae3a9c.
Finding 1 (docs/filesystem.md:403 at 247bf10). The paragraph said that copy() and mv() raise FileNotFoundError for a delete-marker source version. They do not always:
- fsspec's
copy()useson_error="ignore"by default withrecursive=True, so the source is skipped without an error. expand_path()keeps a version path only ifexists()(s3.pyaround 934,s3_async.pyaround 825), so with several sources the delete-marker version is dropped.
Repair: only cp_file() is named ("as for a missing file"). copy() and mv() then follow fsspec's missing-source handling. /simplify's wording suggestion is folded in.
| or multipart upload, or a missing object or version that an operation | ||
| reads, raises ``FileNotFoundError``, and a denied request | ||
| ``PermissionError``. | ||
| ``PermissionError``. :meth:`copy_object` and :meth:`upload_part_copy` |
There was a problem hiding this comment.
Round one, finding 2 (my pass, at 5addecd). The class docstring said "a copy whose source names a delete-marker version" raises EINVAL. plan_multipart_copy() looks up the source with HeadObject first and raises FileNotFoundError instead. Repair (247bf10): the docstring names copy_object and upload_part_copy. In d3ae3a9c the trailing "as S3 rejects the request as invalid" was cut as a restatement (/simplify).
Not taken: removing the sentence and relying on the method Raises sections (/simplify, altitude). The class docstring's general "a missing version that an operation reads raises FileNotFoundError" is the statement #1346 found contradicted, so it keeps the exception next to it.
| Raises: | ||
| ValueError: If the upload has no bucket, key, or upload ID, or the | ||
| source has no key. | ||
| FileNotFoundError: If the upload, or the source object or version, |
There was a problem hiding this comment.
Round one, finding 3. upload_part_copy gained the EINVAL entry but no FileNotFoundError entry, unlike copy_object (altitude, /code-review). Before adding it, a live probe on a temporary versioned bucket (deleted afterwards) measured UploadPartCopy: removed source version 404 NoSuchVersion, missing source key 404 NoSuchKey. Repair: FileNotFoundError: If the upload, or the source object or version, does not exist. The missing upload follows the existing NoSuchUpload mapping.
| Raises: | ||
| ValueError: If the source or the destination has no key, or the | ||
| destination has a version ID, which a write cannot replace. | ||
| FileNotFoundError: If a bucket, or the source object or version, |
There was a problem hiding this comment.
Round one, finding 4 (/code-review). Two inaccuracies in the new copy_object Raises entries:
FileNotFoundErrornamed only the source, but a missing destination bucket (NoSuchBucket) maps to it too.- The
EINVALentry read as if it identified a delete marker, but a malformed version ID gives the same 400 →EINVAL(measured: CopyObjectInvalidRequest).
Repair: "If a bucket, or the source object or version, does not exist", and "With EINVAL for an invalid request, such as a source version that is a delete marker". upload_part_copy was changed the same way.
Deferred or rejected:
cp_file's Raises lacksFileNotFoundError: this predates the PR, and the new prose no longer claims more thancp_filealready does.- An error-table row for the 400 (reuse, altitude): the table lists PyAthena's own translations, and this case falls under "Others".
copy_object_annotation: not listed, since it uses GetObjectAnnotation and is unmeasured.- A test of the documented contract: the generic 400 →
EINVALis pinned bytest_os_error_by_http_status_code[400]. A message-specific translation was declined in Document that S3Core copies of a delete-marker source version raise OSError(EINVAL) #1346.
| version that it reads, and caches nothing. Requests sent through `fs.core` do not | ||
| invalidate the filesystem's cache: call | ||
| version that it reads, and caches nothing. `copy_object()` and `upload_part_copy()` with | ||
| a source version that is a delete marker raise `OSError` with `EINVAL`, while the |
There was a problem hiding this comment.
Self-review round two (claims, documentation reader, existing callers, evidence), base 1a6a0d6c3f137d47bbe96e274ecd467e016fcb38, head d3ae3a9ce8b101673332a7af18bc68d49af5ea98. Result: FINDINGS (PR description wording), corrected. No change to tracked files.
Claims checked:
- CopyObject and UploadPartCopy with a delete-marker source version get 400
InvalidRequestwithout the header. Measured live on 10/27 (temporary versioned bucket, deleted afterwards), andS3Core.copy_object()raisedOSError [Errno 22]at 1a6a0d6. FileNotFoundErrorfor a missing bucket, upload, source object or source version. Measured for both operations: removed source version404 NoSuchVersion, missing source key404 NoSuchKey. The missing upload and bucket follow the existingNoSuchUpload/NoSuchBucketmappings (s3_errors.py).EINVALfor an invalid request, such as a delete-marker source. A generic 400 maps toEINVAL(_HTTP_STATUS_CODE_TO_ERRNO, pinned bytest_os_error_by_http_status_code[400]). A malformed version ID gives the same (measured CopyObjectInvalidRequest), which is why the entry no longer reads as identifying a delete marker.- The filesystem's
cp_file()raisesFileNotFoundErrorfor it. Measured live (sync). The aio_copy_filecalls_info()first (s3_async.py:576); read in the code only. - Why
copy()/mv()are not named (PR body). The first wording attributed both to fsspec.mv()is PyAthena's own (s3.py:1356), and theexpand_path()drop applies only withrecursive=True(s3.py:934). Corrected. I checked offline with a scriptedS3Core: one delete-marker source raisesFileNotFoundErrorfromcopy/mvwith and withoutrecursive. With several sources it is dropped silently. - The docs render, with no new warnings.
uv run sphinx-build docs <tmp>at d3ae3a9: 173 warnings, the same asorigin/master.just docs buildbuilds committed refs only.just docs lint: 0 errors.
Documentation reader: the error-translation table (docs/filesystem.md:239-251) and the S3ClientError docstring are unaffected, since the 400 is not a PyAthena-specific translation. No other page states copy errors.
Existing callers: no behavior change, since the PR touches only docstrings and Markdown.
| does not exist. | ||
| OSError: With ``EINVAL`` for an invalid request, such as a source | ||
| version that is a delete marker. | ||
| """ |
There was a problem hiding this comment.
Independent review (relayed result), static only.
- Reviewer: Codex CLI 0.164.0, model
gpt-6-astra, reasoning effortmax,codex exec --sandbox read-only. It did not author the change. Session01a178d5-cee4-77e3-a1f5-a6c4283b97f4. - Scope: base
1a6a0d6c3f137d47bbe96e274ecd467e016fcb38, headd3ae3a9ce8b101673332a7af18bc68d49af5ea98, run on a detached snapshot of the head. The prompt supplied the measured S3 facts as evidence, without the PR description, commit messages, or prior findings. The reviewer was not allowed to edit, build, run tests, or use GitHub or the network. - Covered: the full diff (class docstring, both copy-method Raises sections, the user-docs paragraph);
s3_errors.pytranslation; sync lookup,cp_file,_copy_file,copy,mvandexpand_path; aio_copy_file; the surrounding filesystem docs, the error translation table, and the Google-style docstring rules.
Result: CLEAN. No findings were introduced, and no pre-existing actionable issue was found in the reviewed context. The reviewer reports that:
- Both core copy methods raise
OSError(EINVAL)for a delete-marker source version, andcp_file()raisesFileNotFoundErrorfrom its lookup, as does the aio path. - Missing keys, removed versions, and an unversioned key whose current version is a delete marker remain
FileNotFoundErrorcases. The malformed-version result fits the "invalid request" wording. - The 405 row of the table does not contradict the copies' 400.
- The added prose states behavior without rationale asides and does not overpromise for
copy()/mv().
The snapshot and the PR worktree were unchanged after the review.
WHAT
Documents how a copy whose source names a delete-marker version fails. Before this change, the
S3Coredocstring anddocs/filesystem.mdimpliedFileNotFoundErrorfor it.S3Coreclass docstring:copy_object()andupload_part_copy()with a source version that is a delete marker raiseOSErrorwithEINVAL.S3Core.copy_object()andS3Core.upload_part_copy()Raises:FileNotFoundErrorfor a missing bucket, upload, source object or source version, andOSError(EINVAL) for an invalid request, such as a source version that is a delete marker.docs/filesystem.md(S3Coresection): the same forcopy_object()andupload_part_copy(), and that the filesystem'scp_file()raisesFileNotFoundErrorfor it, as for a missing file.copy()andmv()are not named. Withrecursive=True,expand_path()drops a version path that does not exist (s3.py:934), soFileNotFoundErroris raised only when nothing else matches, and fsspec'scopy()defaults toon_error="ignore". With one delete-marker source, both raiseFileNotFoundErrorwith or withoutrecursive(checked offline with a scriptedS3Core).No code change. The 400 response carries neither the
x-amz-delete-markerheader nor an error code specific to delete markers; only its message identifies the case, so it is documented rather than translated.WHY
Closes #1346. Follow-up of #1338 (part of #1327).
TEST
Tested at d3ae3a9.
--profile pyathena), at 1a6a0d6; the bucket and every version were deleted afterwards:CopySource.VersionId= a delete marker:400 InvalidRequest, nox-amz-delete-markerheader, "The source of a copy request may not specifically refer to a delete marker by version id."S3Core.copy_object()raisedOSError [Errno 22].404 NoSuchVersion. CopyObject without a version ID when the current version is a delete marker:404 NoSuchKey. Both are translated toFileNotFoundError.404 NoSuchVersion, a missing source key404 NoSuchKey, and a key whose current version is a delete marker404 NoSuchKeywithx-amz-delete-marker: true. All three translate toFileNotFoundError. A malformed version ID gives 400 (InvalidRequestfor CopyObject), which translates toEINVAL.S3FileSystem.cp_file()of the delete-marker version raisedFileNotFoundErrorfrom itsinfo()lookup. The aio_copy_filelooks up the source with_info()in the same way (s3_async.py:576); that path was read in the code but not run live.just lint: passed.just docs lint: 0 errors.uv run sphinx-build docs <tmp>of the working tree: at d3ae3a9 the new text renders infilesystem.htmlandapi/filesystem.html. 173 warnings, the same count asorigin/master.just docs build(sphinx-multiversion) builds only committed refs, so it does not show uncommitted changes.🤖 Generated with Claude Code