Skip to content

Conversation

@durch
Copy link
Owner

@durch durch commented Dec 9, 2025

This change is Reviewable

Summary by CodeRabbit

  • New Features

    • Added support for bucket lifecycle configuration operations.
  • Bug Fixes

    • Fixed single-byte range request handling in object retrieval.
  • Chores

    • Patch releases for aws-creds, aws-region, and s3 packages.

✏️ Tip: You can customize this high-level summary in your review settings.

durch and others added 4 commits December 9, 2025 22:21
Ranges are closed, so start <= end is correct. This allows single-byte
reads where start == end, which was previously rejected by the assertion.

Added tests for single-byte range reads.

Closes #434
Remove incorrect removal of x-amz-content-sha256 header which caused
signature validation to fail.

Co-authored-by: ZzIsGod1019 <[email protected]>
Closes #435
@coderabbitai
Copy link

coderabbitai bot commented Dec 9, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

This PR bumps versions across three crates (aws-creds 0.39.0→0.39.1, aws-region 0.28.0→0.28.1, s3 0.37.0→0.37.1) and adjusts S3 range read validation to permit single-byte reads. It also adds lifecycle configuration tests, updates serde trait derivations, and modifies header handling in the PutBucketLifecycle request path.

Changes

Cohort / File(s) Summary
Version Bumps
aws-creds/Cargo.toml, aws-region/Cargo.toml, s3/Cargo.toml
Crate version increments (patch-level) and updated internal dependency versions in s3 to reference the new aws-creds and aws-region releases.
Range Read Validation
s3/src/bucket.rs, s3/src/request/request_trait.rs
Relaxed range assertions from start < end to start <= end in get_object_range and get_object_range_to_writer methods to allow single-byte reads; removed "x-amz-content-sha256" header removal in PutBucketLifecycle request path.
Test Enhancements
s3/src/bucket.rs
Added test cases for single-byte range reads, introduced new test_bucket_lifecycle integration test exercising lifecycle configuration put/get/delete, and updated test imports for lifecycle and CORS types.
Serde Type Enhancements
s3/src/serde_types.rs
Added Default trait to Expiration and LifecycleFilter struct derive macros.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Range assertion changes in bucket.rs and the header handling modification in request_trait.rs are straightforward logic tweaks; verify the range boundary conditions (start == end) behave as intended.
  • Lifecycle test additions are CRUD-style operations; check test structure and assertion correctness.
  • Version bumps and derive additions are routine and low-risk.

Poem

🐰 A patch release hops along the way,
Single bytes now read in S3's play.
Lifecycle configs dance with tests so bright,
Defaults bloom in structs, all feels just right! 🌱

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 0.37.1

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9393c22 and 54eba17.

📒 Files selected for processing (6)
  • aws-creds/Cargo.toml (1 hunks)
  • aws-region/Cargo.toml (1 hunks)
  • s3/Cargo.toml (2 hunks)
  • s3/src/bucket.rs (10 hunks)
  • s3/src/request/request_trait.rs (0 hunks)
  • s3/src/serde_types.rs (2 hunks)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@brownian-motion-v0
Copy link

Brownian Motion (Brass)

Recommendation: Proceed

Summary: PR fixes range checks and improves lifecycle handling with tests.
Risk: Medium · Confidence: 80%

Highlights

  • Good test coverage
  • Clear commit history

Unknowns

  • No linked issue

Next actions

  • Keep: range check fix and tests for single-byte reads
  • Drop: unnecessary removal of x-amz-content-sha256 header
  • Add: documentation for lifecycle changes

Reflection questions

  • What core assumption underpins this PR's approach?
  • How does this change align with the project's longer-term goals?
  • Could there be a simpler way to achieve the primary objective here?

@brownian-motion-v0
Copy link

Brownian Motion (Brass)

Recommendation: Refactor

Summary: PR addresses range checks but adds complexity with minimal benefit.
Risk: Medium · Confidence: 70%

Highlights

  • Good test coverage
  • Clear commit history

Unknowns

  • No linked issue
  • Missing performance metrics

Next actions

  • Keep: Tests for single-byte range reads
  • Drop: Unnecessary complexity in range checks
  • Add: Documentation on the reasoning behind changes

Reflection questions

  • What core assumption underpins this PR's approach?
  • How does this change align with the project's longer-term goals?
  • Could there be a simpler way to achieve the primary objective here?

@durch durch merged commit d9769aa into master Dec 9, 2025
3 of 4 checks passed
@durch durch deleted the 0.37.1 branch December 9, 2025 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants