-
Notifications
You must be signed in to change notification settings - Fork 233
add custom_headers parameter to initiate_multipart_upload #446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
fix: command PutBucketLifecycle failed(SignatureDoesNotMatch)
📝 WalkthroughWalkthroughThe PR adds support for custom HTTP headers in multipart upload initiation flows. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
s3/src/bucket.rs (2)
1857-1877: Breaking API change: Consider backward compatibility.Adding
custom_headers: Option<HeaderMap>as a required parameter to the publicinitiate_multipart_uploadmethod is a breaking change for existing callers. Consider whether this warrants a semver major version bump.Additionally, as per coding guidelines, public APIs should include documentation examples. This method currently lacks doc comments demonstrating the new
custom_headersparameter usage.Example documentation to add
/// Initiate multipart upload to s3. /// /// # Example: /// /// ```no_run /// use s3::bucket::Bucket; /// use s3::creds::Credentials; /// use http::HeaderMap; /// use http::header::HeaderName; /// use anyhow::Result; /// /// # #[tokio::main] /// # async fn main() -> Result<()> { /// let bucket = Bucket::new("my-bucket", "us-east-1".parse()?, Credentials::default()?)?; /// /// // Without custom headers /// let response = bucket.initiate_multipart_upload("/path/to/file", "application/octet-stream", None).await?; /// /// // With custom headers (e.g., Cache-Control, x-amz-meta-*) /// let mut headers = HeaderMap::new(); /// headers.insert( /// HeaderName::from_static("cache-control"), /// "public, max-age=3600".parse().unwrap(), /// ); /// let response = bucket.initiate_multipart_upload("/path/to/file", "application/octet-stream", Some(headers)).await?; /// # Ok(()) /// # } /// ``` #[maybe_async::async_impl] pub async fn initiate_multipart_upload(
1802-1809: Inconsistency: Sync streaming path doesn't support custom headers.The sync
_put_object_stream_with_content_typemethod always passesNoneforcustom_headers, while the async path (_put_object_stream_with_content_type_and_headers) properly propagates custom headers. Consider adding a sync equivalent that supports custom headers for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
s3/src/bucket.rss3/src/command.rss3/src/request/request_trait.rs
🧰 Additional context used
📓 Path-based instructions (2)
s3/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use existing error types from
s3/src/error.rsrather than defining new error types
Files:
s3/src/command.rss3/src/request/request_trait.rss3/src/bucket.rs
{s3,aws-region,aws-creds}/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
{s3,aws-region,aws-creds}/src/**/*.rs: Follow the async/sync abstraction pattern usingmaybe_asyncmacros for runtime-agnostic implementations
Mark integration tests with#[ignore]if they require AWS credentials
Provide documentation examples for all public APIs
Files:
s3/src/command.rss3/src/request/request_trait.rss3/src/bucket.rs
🧬 Code graph analysis (1)
s3/src/request/request_trait.rs (2)
s3/src/signing.rs (1)
headers(141-144)s3/src/request/tokio_backend.rs (1)
headers(124-135)
🔇 Additional comments (6)
s3/src/command.rs (2)
135-138: LGTM! Consistent with existing patterns.The addition of
custom_headers: Option<HeaderMap>toInitiateMultipartUploadmirrors the existing pattern inPutObjectandPresignPutvariants, providing a consistent API surface.
262-262: LGTM! Correct pattern matching update.Using
..to ignore the newcustom_headersfield while extractingcontent_typeis the idiomatic approach.s3/src/request/request_trait.rs (1)
699-706: LGTM! Header merging correctly follows the established pattern.The custom headers for
InitiateMultipartUploadare merged using the same approach asPutObject(lines 691-697). Headers are inserted before the host header and authorization calculation, ensuring they are properly included in the AWS signature.s3/src/bucket.rs (3)
1864-1867: LGTM! Command construction correctly includes custom_headers.The
Command::InitiateMultipartUploadis properly constructed with bothcontent_typeandcustom_headersfields.
1879-1899: Sync implementation mirrors async correctly.The sync variant properly accepts and passes
custom_headersto the command, maintaining consistency with the async implementation.
1667-1668: LGTM! Custom headers correctly propagated in async multipart flow.The
custom_headersparameter is properly forwarded toinitiate_multipart_upload, enabling per-request header customization for multipart uploads.
This PR adds support for custom HTTP headers in the
initiate_multipart_uploadmethod, allowing users to pass custom headers (such as cache-control, metadata, storage class, etc.) when initiating multipart uploads to S3.This feature aligns the
initiate_multipart_uploadAPI with the existingput_object_with_headersfunctionality, providing a consistent interface for setting custom headers across different upload methods.This change is
Summary by CodeRabbit
New Features
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.