Skip to content

Conversation

@uckelman-sf
Copy link

@uckelman-sf uckelman-sf commented Oct 21, 2025

The check in the ranged read functions is correct for a half-open range, but off-by-one for the closed ranges that the read functions are expecting. This prevents single-byte reads from working, because in a single-byte read, start == end. (Yes, single byte reads from S3 are stupid, but they should still work.)

This PR fixes the range checks.


This change is Reviewable

Summary by CodeRabbit

  • Bug Fixes
    • Adjusted S3 object range request validation to accept requests where start and end values are equal, enabling zero-length range retrieval.

@coderabbitai
Copy link

coderabbitai bot commented Oct 21, 2025

Walkthrough

Range validation logic in GetObjectRange paths is relaxed from requiring start < end to permitting start <= end when end is specified. This adjustment allows zero-length or equal-range requests to proceed, affecting only the pre-check condition rather than downstream command construction.

Changes

Cohort / File(s) Summary
Range validation relaxation
s3/src/bucket.rs
Modified boundary condition check in three GetObjectRange paths from start < end to start <= end, allowing equal or zero-length range requests

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 Hop closer, let ranges be free!
No more strict bounds on equality,
Start equals end? That's fine today,
Zero-length dreams now have their way! 🌾

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Ranges are closed, so the check should be start <= end, not start < end" directly and specifically describes the main change in the pull request. It clearly identifies what was changed (the comparison operator in range validation from < to <=), provides conceptual context (ranges are closed semantics), and explains the purpose of the change. The title is concise, avoids vague language, and would allow a teammate reviewing history to immediately understand the core intent of the changeset. The change is accurately represented in the file modifications adjusting range validation across three GetObjectRange paths.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 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 d88c5f4 and f1406a0.

📒 Files selected for processing (1)
  • s3/src/bucket.rs (3 hunks)
🔇 Additional comments (4)
s3/src/bucket.rs (4)

1188-1190: Correct fix for closed-range semantics.

The change from start < end to start <= end correctly implements closed-range validation. Since the function documentation states "Gets specified inclusive byte range" and S3's Range header uses closed ranges (e.g., bytes=0-0 for a single byte), allowing start == end enables valid single-byte reads.


1249-1251: Consistent fix applied to async implementation.

The validation change is correctly applied to the async variant of get_object_range_to_writer, maintaining consistency across the codebase.


1266-1268: Consistent fix applied to sync implementation.

The validation change is correctly applied to the sync variant of get_object_range_to_writer, ensuring all range read paths support single-byte reads.


3183-3187: Consider adding test coverage for single-byte reads.

The existing test validates a multi-byte range read. To prevent regression of the bug this PR fixes, consider adding a test case that specifically validates single-byte reads where start == end.

Example test case to add:

// Test single-byte read (start == end)
let response_data = bucket
    .get_object_range(s3_path, 100, Some(100))
    .await
    .unwrap();
assert_eq!(response_data.status_code(), 206);
assert_eq!(test[100..101].to_vec(), response_data.as_slice());
assert_eq!(response_data.as_slice().len(), 1);

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.

@uckelman-sf
Copy link
Author

The AI-generated code review is wrong. The change does not permit zero-length requests---end is inclusive, so when start == end the length is 1.

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.

1 participant