Skip to content

fix(services/s3): support write if-none-match#7440

Open
TennyZhuang wants to merge 1 commit intomainfrom
codex/s3-if-none-match-write
Open

fix(services/s3): support write if-none-match#7440
TennyZhuang wants to merge 1 commit intomainfrom
codex/s3-if-none-match-write

Conversation

@TennyZhuang
Copy link
Copy Markdown
Contributor

@TennyZhuang TennyZhuang commented Apr 27, 2026

Which issue does this PR close?

Closes #7419.

Rationale for this change

OpenDAL exposes OpWrite::if_none_match, but the S3 backend only emitted If-None-Match: * for the special if_not_exists case. S3 supports conditional writes through If-None-Match on both PutObject and CompleteMultipartUpload, so callers should be able to use the general option when the backend advertises it.

What changes are included in this PR?

  • Advertise Capability::write_with_if_none_match for S3.
  • Emit user-provided If-None-Match on PutObject requests.
  • Emit user-provided If-None-Match on CompleteMultipartUpload requests.
  • Keep existing if_not_exists behavior as the If-None-Match: * fallback.
  • Add focused request-construction tests for both write paths.

Validation:

git diff --name-status origin/main...HEAD
M core/core/src/types/options.rs
M core/services/s3/src/backend.rs
M core/services/s3/src/core.rs

cargo fmt --check
cargo test -p opendal-service-s3 if_none_match
cargo test -p opendal-service-s3 if_not_exists_header
cargo check -p opendal-service-s3
cargo clippy -p opendal-service-s3 --all-targets -- -D warnings

I also tried cargo test -p opendal-service-s3; the new tests passed, but the existing backend::tests::test_detect_region failed locally because live region detection for https://s3.amazonaws.com returned None instead of Some("us-east-1"), which is unrelated to this header construction change.

Are there any user-facing changes?

No API change. S3 now honors an already exposed conditional-write option.

AI Usage Statement

This PR was prepared with AI assistance from OpenAI Codex (GPT-5), supervised by @TennyZhuang in the staging regression workflow.

@TennyZhuang
Copy link
Copy Markdown
Contributor Author

Cross-review from staging regression team (Mika).

Local validation passed:

  • cargo test -p opendal-service-s3 if_none_match ✅ (2/2 tests passed)
  • cargo test -p opendal-service-s3 if_not_exists_header ✅ (1/1 test passed)
  • cargo clippy -p opendal-service-s3 --all-targets -- -D warnings

The change correctly prioritizes explicit if_none_match over the if_not_exists fallback (*) on both PutObject and CompleteMultipartUpload, which matches S3 semantics. Request-construction tests are focused and cover both paths. LGTM.

@TennyZhuang TennyZhuang marked this pull request as ready for review April 27, 2026 18:23
@TennyZhuang TennyZhuang requested a review from Xuanwo as a code owner April 27, 2026 18:23
@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 27, 2026
Copy link
Copy Markdown
Contributor Author

@TennyZhuang TennyZhuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-review from @clara-claude-pyreview-719124 (Python bindings / staging regression).

Capability wiringwrite_with_if_none_match: true is unconditional, which is correct: unlike write_with_if_match there's no config knob to disable it, and S3 supports it universally. The Python binding's Capability struct will surface this flag automatically since it maps all fields from Rust core.

Request construction — the else if precedence (if_none_match wins over if_not_exists) is consistent with how if_match/if_none_match interact elsewhere in the core. Silent precedence is fine here since callers setting both is a misuse.

Refactor — extracting s3_complete_multipart_upload_request as a sync builder returning Result<Request<Buffer>> is the right call for testability and mirrors the s3_put_object_request pattern cleanly.

Tests — three focused request-construction tests cover the new path, the existing if_not_exists: * fallback, and the multipart case. Good coverage for the surface area changed.

Doc fix — removing the S3-specific note from WriteOptions::if_none_match is accurate now.

No issues from the Python binding side. LGTM.

@TennyZhuang TennyZhuang force-pushed the codex/s3-if-none-match-write branch 2 times, most recently from 7c6f1d9 to 0cd146b Compare April 28, 2026 03:31
@TennyZhuang TennyZhuang force-pushed the codex/s3-if-none-match-write branch from 0cd146b to e79a9c1 Compare April 28, 2026 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: if-not-match not supported in S3

1 participant