Skip to content

Conversation

@vandry
Copy link

@vandry vandry commented Nov 26, 2025

Traits are defined which backends must implement:

  • The HTTP client tower::Service interface.
  • The ResponseBody that the backend must return.

Minimal new dependencies are taken, notably http-body but not the whole tower crate, only the small crate with the trait definition.

All 3 built-in backends, Reqwest, Surf, and Atto, implement the necessary traits. Exactly one of them is compiled depending on feature flags and exposed as DefaultBackend so that everything works the same as before by default.

Future work will be 2 refactor some functionality such as retries and the fail-on-err feature as tower::Layer middleware.

This is a somewhat breaking change and so will require a semver bump but I tried to keep it as unintrusive as possible.


This change is Reviewable

Summary by CodeRabbit

Release Notes

  • Breaking Changes

    • Updated Bucket type and related public method signatures; code using Bucket will require updates.
    • Added new error type variant for invalid HTTP status codes.
  • New Features

    • Added new builder methods for fluent configuration of bucket settings.
    • Enhanced error handling with improved status code validation.

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

The RequestImpl type alias for 1 of the 3 backends is now referenced
in only one place, a new Bucket::make_request method which everything
else that previously called RequestImpl::new now calls.

The constructors for each of these backends are sync, as constructors
should be. This will make it easier to change to a different backend
factory API. The only reason these were async anyway is so that
credentials_refresh could be called, but make_request can just do that.
This will make it easier to offer pluggable backends in the future.
The unification of all 3 backends under http::Request left essentially
3 copies of the same unit tests. Deduplicate those and move them to
request_trait.rs because that's the substance of what they were really
testing. In their place add new tests for each backend that test the
request translation from http::Request to the backend's own request
representation.
This will make it easier to offer pluggable backends in the future.
This is a breaking change. What used to we expressed as:
  bucket.with_request_timeout(...)
  bucket.request_timeout()
  bucket.set_dangereous_config(...)
  bucket.set_proxy(...)
is now written as:
  bucket.with_backend(bucket.backend().with_request_timeout(...)?)
  bucket.backend().request_timeout()
  bucket.with_backend(bucket.backend().with_dangereous_config(...)?)
  bucket.with_backend(bucket.backend().with_proxy(...)?)

Bucket no longer contains any assumptions about backends except that
they accept http::Request and return http::Response.
This hides the details of s3::request::Request inside
Bucket::make_request() in preparation for replacing the backend API with
one better suited to pluggable backends.
The next change needs to split the Bucket type into different impl
blocks with different generics. This textually moves the some of the
methods so that they are grouped to fit together into the impl blocks
they will need to belong to. Nothing is added or removed.

This change exists only to make the diff of the next change easier
to follow.
let backend = something that impls tower::Service<http::Request<...>>;
let bucket = Bucket::new(...)?.with_backend(backend);

There is still plenty to do, for example there is no API yet to
create a bucket with a non-default backend, only get an existing
bucket. But this is the basic feature.
@coderabbitai
Copy link

coderabbitai bot commented Nov 26, 2025

Walkthrough

This pull request introduces a pluggable backend abstraction for HTTP requests in the S3 crate. The Bucket type is generalized to Bucket<B> with a Backend trait, replacing per-runtime concrete implementations. New HTTP dependencies are added, backend types (SurfBackend, AttoBackend, ReqwestBackend) are refactored as Service implementations, and request construction is unified through generic helper functions and trait bounds.

Changes

Cohort / File(s) Summary
Dependency Updates
s3/Cargo.toml
Added optional dependencies: http-body (1.0.1), http-body-util (0.1.3), tower-service (0.3.3); updated with-async-std and with-tokio features to include new dependencies.
Core Bucket API Generalization
s3/src/bucket.rs, s3/src/bucket_ops.rs
Introduced generic parameter B to Bucket struct and CreateBucketResponse; updated public constructors, builders (with_path_style, with_extra_headers, with_backend), and request execution pathways to use Backend abstraction; adjusted method signatures and return types to default to DefaultBackend.
Backend Abstraction Layer
s3/src/request/backend.rs, s3/src/request/mod.rs
Introduced new backend.rs module defining Backend trait with feature-conditional implementations for async and sync modes; added DefaultBackend alias, BackendRequestBody type alias, SyncService trait for sync builds, and DEFAULT_REQUEST_TIMEOUT constant; exposed module in mod.rs.
Request Infrastructure Refactor
s3/src/request/request_trait.rs
Removed public Request trait; introduced ResponseBody trait abstraction, BuildHelper for signature construction, and generic helper functions build_request(), build_presigned(), and response_data* variants; refactored request/response handling to support both async and sync backends.
Async-Std Backend
s3/src/request/async_std_backend.rs
Replaced SurfRequest struct with new SurfBackend implementing Service trait; introduced SurfBody implementing http_body::Body; added http_request_to_surf_request converter; updated response handling and tests.
Blocking Backend
s3/src/request/blocking.rs
Removed AttoRequest struct; introduced AttoBackend with request_timeout configuration and SyncService implementation; added http_request_to_atto_request converter; refactored response handling and tests.
Tokio Backend
s3/src/request/tokio_backend.rs
Replaced ReqwestRequest with new ReqwestBackend struct implementing Service trait; made client() function private; added configuration methods (with_request_timeout, with_proxy, with_dangereous_config); updated tests and module structure.
Error Handling
s3/src/error.rs
Made Http variant unconditional (removed #[cfg(feature = "with-tokio")]); added new InvalidStatusCode variant with #[from]; implemented From<std::convert::Infallible> conversion.
Request Type Updates
s3/src/put_object_request.rs, s3/src/post_policy.rs
Parameterized PutObjectRequest and PutObjectStreamRequest with generic B: Backend; updated bucket field types to &Bucket<B>; updated method signatures for presigning and execution; updated PostPolicy::build() and PostPolicy::sign() with generic Bucket<B> parameters; adjusted test helpers and fixtures.
Utilities and Testing
s3/src/utils/mod.rs, s3/src/utils/testing.rs
Added public testing module with new AlwaysFailBackend test type; added crate-private service_ready module with Ready future-like type for Service readiness polling (non-sync builds only).

Sequence Diagram

sequenceDiagram
    actor Client
    participant Bucket
    participant Backend as Backend Trait
    participant HTTPImpl as HTTP Implementation<br/>(Surf/Reqwest/Atto)
    
    Note over Client,HTTPImpl: Old Flow: Concrete per-runtime handling
    Client->>Bucket: make_request()
    Bucket->>Bucket: RequestImpl (Surf/Reqwest/Atto)
    Bucket->>HTTPImpl: Direct runtime call
    HTTPImpl-->>Bucket: Response
    
    Note over Client,HTTPImpl: New Flow: Generic Backend abstraction
    Client->>Bucket: make_request()
    Bucket->>Backend: call(http::Request)
    Backend->>HTTPImpl: Service::call()
    HTTPImpl-->>Backend: http::Response
    Backend-->>Bucket: Response via ResponseBody trait
    Bucket-->>Client: Result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Trait abstraction correctness: Verify Backend trait bounds are sound for both async (tower_service::Service) and sync (SyncService) implementations, including error propagation via Into<S3Error>.
  • Generic parameter threading: Ensure generic B: Backend parameters are correctly threaded through Bucket<B>, request types, and public APIs; check that bounds on impl blocks are complete and consistent.
  • Backend implementation compliance: Validate that SurfBackend, AttoBackend, and ReqwestBackend correctly implement Service/SyncService and properly convert incoming requests via the new helper functions.
  • Request/response handling: Confirm that build_request(), build_presigned(), and response_data*() functions handle all command variants and edge cases (etag extraction, body consumption, streaming).
  • Feature flag consistency: Verify conditional compilation (with-tokio, with-async-std, sync) is correctly applied across backends, error handling, and utilities without gaps or conflicts.
  • DefaultBackend resolution: Check that feature-gated DefaultBackend aliases resolve to the correct backend implementation for each build configuration.

Poem

🐰 A backend abstraction, so clean and so neat,
Generics and traits make the architecture complete,
Service, Surf, Reqwest, and Atto unite,
Request handling unified—the code smells just right!

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 PR title 'Make the HTTP client backend used by Bucket pluggable' directly and clearly describes the primary change: introducing a pluggable backend abstraction for HTTP clients in the Bucket type. The title accurately reflects the main architectural shift from concrete per-runtime RequestImpl types to a generic Backend trait-based design.
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

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
s3/src/utils/mod.rs (1)

291-293: Typo: "get-Type" should be "Content-Type".

This appears to be a pre-existing bug where the header name is incorrectly specified as "get-Type" instead of "Content-Type" in the sync backend's HeadObjectResult conversion.

             content_type: headers
-                .get("get-Type")
+                .get("Content-Type")
                 .map(|v| v.to_str().unwrap_or_default().to_string()),
🧹 Nitpick comments (7)
s3/src/request/async_std_backend.rs (2)

36-40: Consider returning Result instead of panicking on invalid headers.

The expect() calls will panic if header name/value parsing fails. While this is unlikely in practice, returning an error would be more robust for a library.

-    for (name, value) in request.headers().iter() {
-        builder = builder.header(
-            HeaderName::from_bytes(AsRef::<[u8]>::as_ref(&name).to_vec())
-                .expect("Could not parse heaeder name"),
-            HeaderValue::from_bytes(AsRef::<[u8]>::as_ref(&value).to_vec())
-                .expect("Could not parse header value"),
-        );
-    }
+    for (name, value) in request.headers().iter() {
+        let header_name = HeaderName::from_bytes(AsRef::<[u8]>::as_ref(&name).to_vec())
+            .map_err(|e| S3Error::Surf(format!("Invalid header name: {}", e)))?;
+        let header_value = HeaderValue::from_bytes(AsRef::<[u8]>::as_ref(&value).to_vec())
+            .map_err(|e| S3Error::Surf(format!("Invalid header value: {}", e)))?;
+        builder = builder.header(header_name, header_value);
+    }

Also note the typo: "heaeder" → "header".


109-110: Consider moving struct definition before its impl block.

The SurfBackend struct is defined after its Service impl block. While valid Rust, placing the struct definition before its implementations is more conventional and improves readability.

s3/src/put_object_request.rs (1)

114-125: Inconsistent with_header signatures between builder types.

PutObjectRequest::with_header takes key: &str (Line 114) while PutObjectStreamRequest::with_header takes key: K where K: Into<http::HeaderName> (Line 250). This API inconsistency may confuse users expecting the same interface.

Consider aligning both methods to use the more flexible K: Into<http::HeaderName> signature, or both to use &str for consistency.

-    pub fn with_header<V>(mut self, key: &str, value: V) -> Result<Self, S3Error>
+    pub fn with_header<K, V>(mut self, key: K, value: V) -> Result<Self, S3Error>
     where
+        K: TryInto<HeaderName>,
+        K::Error: Into<S3Error>,
         V: AsRef<str>,
     {
-        let header_name = HeaderName::from_bytes(key.as_bytes())?;
+        let header_name = key.try_into().map_err(Into::into)?;

Also applies to: 250-261

s3/src/bucket.rs (4)

183-235: Consider harmonizing return types across builder methods.

The builder methods have inconsistent return types:

  • with_path_style returns Box<Self>
  • with_extra_headers / with_extra_query return Result<Self, S3Error> but never fail
  • with_listobjects_v1 returns Self

The Result wrappers in with_extra_headers and with_extra_query are infallible — consider returning Self directly to simplify the API, unless error handling is planned for a future change.


529-565: Note the asymmetric return types between new and new_public.

new returns Box<Self> while new_public returns Self. This may be intentional for backwards compatibility, but it creates an inconsistent API surface. Consider documenting why this asymmetry exists or harmonizing in a future release.


482-513: Consider extracting shared logic between create and create_with_path_style.

Both methods share nearly identical code for the RUST_S3_SKIP_LOCATION_CONSTRAINT check and bucket creation. Consider extracting a common helper to reduce duplication.

async fn create_bucket_inner(
    bucket: Box<Bucket<DefaultBackend>>,
    config: BucketConfiguration,
) -> Result<CreateBucketResponse<DefaultBackend>, S3Error> {
    let command = Command::CreateBucket { config };
    let response = bucket.make_request("", command).await?;
    let data = response_data(response, false).await?;
    let response_text = data.as_str()?;
    Ok(CreateBucketResponse {
        bucket,
        response_text: response_text.to_string(),
        response_code: data.status_code(),
    })
}

4034-4036: Typo in method name: with_dangereous_config.

The method name contains a typo ("dangereous" instead of "dangerous"). If this exists in the backend implementation, consider fixing it as a separate change. Otherwise, this is a minor issue to note.

#!/bin/bash
# Check if the typo exists in the backend implementation
rg -n "dangereous" --type rust
📜 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 5a48df0.

📒 Files selected for processing (14)
  • s3/Cargo.toml (2 hunks)
  • s3/src/bucket.rs (60 hunks)
  • s3/src/bucket_ops.rs (1 hunks)
  • s3/src/error.rs (2 hunks)
  • s3/src/post_policy.rs (6 hunks)
  • s3/src/put_object_request.rs (3 hunks)
  • s3/src/request/async_std_backend.rs (1 hunks)
  • s3/src/request/backend.rs (1 hunks)
  • s3/src/request/blocking.rs (1 hunks)
  • s3/src/request/mod.rs (1 hunks)
  • s3/src/request/request_trait.rs (29 hunks)
  • s3/src/request/tokio_backend.rs (3 hunks)
  • s3/src/utils/mod.rs (2 hunks)
  • s3/src/utils/testing.rs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
s3/src/request/mod.rs (1)
s3/src/bucket.rs (1)
  • backend (159-161)
s3/src/request/tokio_backend.rs (2)
s3/src/request/async_std_backend.rs (2)
  • poll_ready (79-81)
  • call (83-106)
s3/src/request/backend.rs (1)
  • call (27-27)
s3/src/request/backend.rs (4)
s3/src/utils/testing.rs (2)
  • call (17-23)
  • call (31-35)
s3/src/request/async_std_backend.rs (1)
  • call (83-106)
s3/src/request/blocking.rs (1)
  • call (45-63)
s3/src/request/tokio_backend.rs (2)
  • call (67-83)
  • call (187-200)
s3/src/post_policy.rs (1)
s3/src/bucket.rs (1)
  • new (529-540)
s3/src/utils/testing.rs (1)
s3/src/request/backend.rs (1)
  • call (27-27)
s3/src/request/async_std_backend.rs (2)
s3/src/request/request_trait.rs (1)
  • headers (114-116)
s3/src/request/backend.rs (1)
  • call (27-27)
s3/src/put_object_request.rs (2)
s3/src/bucket.rs (3)
  • backend (159-161)
  • new (529-540)
  • response (444-446)
s3/src/request/request_trait.rs (2)
  • response_data (254-296)
  • new (74-80)
s3/src/request/blocking.rs (3)
s3/src/request/tokio_backend.rs (4)
  • request_timeout (104-106)
  • call (67-83)
  • call (187-200)
  • with_request_timeout (93-102)
s3/src/request/backend.rs (1)
  • call (27-27)
s3/src/request/async_std_backend.rs (1)
  • call (83-106)
s3/src/bucket.rs (3)
s3/src/request/request_trait.rs (6)
  • build_presigned (1004-1017)
  • response_data (254-296)
  • new (74-80)
  • status_code (102-104)
  • head (281-294)
  • from (68-70)
s3/src/request/backend.rs (1)
  • call (27-27)
s3/src/utils/testing.rs (2)
  • call (17-23)
  • call (31-35)
🔇 Additional comments (35)
s3/src/error.rs (2)

47-48: LGTM!

The InvalidStatusCode variant is properly added to handle errors from http::StatusCode::from_u16() when converting backend responses.


78-82: LGTM!

The From<Infallible> impl enables seamless error propagation with the ? operator when working with services that have Error = Infallible. Using unreachable!() is the idiomatic approach since Infallible can never be instantiated.

s3/src/request/mod.rs (1)

3-3: LGTM!

The backend module is correctly exposed unconditionally to provide the pluggable backend abstraction layer that all feature configurations can depend on.

s3/src/utils/testing.rs (1)

1-36: LGTM!

AlwaysFailBackend is a well-designed test utility that provides a deterministic failure response (HTTP 418) for both async and sync backends. The conditional compilation ensures it implements the correct service trait based on the feature flag.

s3/src/bucket_ops.rs (1)

234-244: LGTM!

The generic parameter B on CreateBucketResponse<B> correctly propagates the backend type from Bucket<B> without requiring trait bounds, since the struct only holds the bucket and the success() method doesn't interact with the backend.

s3/src/utils/mod.rs (2)

433-465: LGTM!

The Ready future is a well-implemented lightweight alternative to tower::util::ServiceExt::ready, avoiding a full dependency on the tower crate. The implementation correctly:

  • Polls Service::poll_ready once per poll
  • Panics defensively if polled after completion (standard Future contract)
  • Returns a mutable reference to the service on success

1-2: I'm unable to execute the verification scripts due to repository clone failures in the sandbox environment. However, I can provide analysis based on Rust language semantics and the context provided in the review comment.

Based on the code snippet and review context:

  1. The #[cfg(test)] pattern is correct: In Rust, a #[cfg(test)] module declaration at module level is a standard pattern for test-only utilities. The module is only compiled during test builds, and visibility rules apply within that test scope.

  2. Cross-module imports work: When s3/src/post_policy.rs test code imports crate::utils::testing::AlwaysFailBackend, both the import statement and the testing module are compiled under #[cfg(test)], so this works correctly.

  3. Feature combinations: The concern about different feature combinations is valid - the tests should compile with different runtime features (tokio, async-std, sync). However, this would require actual compilation to verify.

Since I cannot execute the cargo test verification in the sandbox, the verification request cannot be fully completed. However, the pattern itself is sound and follows Rust best practices.

The #[cfg(test)] visibility for cross-module test imports follows standard Rust patterns and should work correctly. However, actual compilation verification across feature combinations (tokio-native-tls, async-std-native-tls, sync-native-tls) would need to be performed locally or in CI to definitively confirm.

s3/src/post_policy.rs (2)

61-64: LGTM!

The build and sign methods are correctly generalized over the backend type B. Since these methods only access bucket metadata (name, region, credentials) and don't perform HTTP requests, no trait bounds on B are required.

Also applies to: 113-113


465-480: LGTM!

Test helper functions are properly updated to use AlwaysFailBackend via the .with_backend() pattern, ensuring tests don't accidentally make real HTTP requests.

Also applies to: 482-497

s3/Cargo.toml (1)

58-59: All dependency versions are current.

The verification confirms that the added dependencies are at their latest stable versions:

  • http-body v1.0.1 is the latest (July 12, 2024)
  • http-body-util v0.1.3 is the latest (March 11, 2025)
  • tower-service v0.3.3 is the latest (as of November 26, 2025)

The dependency additions are appropriately gated behind optional features and do not introduce outdated or vulnerable versions.

s3/src/request/async_std_backend.rs (1)

74-107: Based on my research, I've uncovered important information about the Send bound concern. Let me provide the rewritten review:


Verify Send bound is actually satisfied—surf::Request is not Send.

The Future declares + Send, but surf::Request (the concrete Request<NativeClient> type) contains non-Send internals. If http_request_to_surf_request returns a surf::Request and request.send() holds that request across the .await point, the resulting future will be !Send, violating the trait bound and causing compilation to fail.

Ensure either:

  • http_request_to_surf_request converts the request to owned, Send-compatible types before returning, or
  • The request is consumed immediately in request.send() without being held across await points with non-Send state.
s3/src/put_object_request.rs (1)

178-194: LGTM!

The execute method cleanly delegates to the bucket's make_request infrastructure and correctly uses response_data(response, true) to extract the ETag for PUT operations.

s3/src/request/tokio_backend.rs (1)

67-83: LGTM!

The call implementation properly converts the request, handles the fail-on-err feature flag, and includes the response body in the error for debugging. Good use of into_owned() to ensure the body is owned before conversion.

s3/src/request/blocking.rs (3)

57-62: LGTM!

The response reconstruction correctly extracts status, headers, and body from the attohttpc response. The unwrap() on headers_mut() is safe here since the builder was just created with only a status.


79-82: LGTM!

The Result return type maintains API consistency with other backends like ReqwestBackend, even though this implementation is infallible.


18-24: The review comment is incorrect. Session methods mutate in place and do not return values.

The attohttpc::Session::header() and Session::timeout() methods both have return type () (unit type). They take &mut self and mutate the session in place, so the return values (units) are correctly discarded. The headers and timeout are properly applied; there is no bug here. The review comment conflates this with RequestBuilder, which uses the builder pattern and returns Self for chaining.

s3/src/request/request_trait.rs (4)

199-230: Well-designed sealed trait pattern for ResponseBody.

The ResponseBody trait correctly uses a sealed pattern (unused_trait_should_have_only_blanket_impl) to prevent external implementations while providing a blanket impl for all compatible types. The async and sync variants are properly feature-gated.


253-296: LGTM!

The response_data function correctly handles both ETag extraction for PUT operations and normal body consumption. The TODO comment (Lines 270-271) appropriately documents the technical debt for future refactoring.


965-1001: LGTM!

The build_request function properly orchestrates request construction: method mapping, header generation via BuildHelper, URL construction with query handling, and body assembly. Clean and well-structured.


1276-1294: LGTM!

The testawait helper provides a minimal spinning executor that's independent of tokio/async-std, enabling tests that don't need full runtime features. The progress check (Line 1285-1287) prevents infinite loops in case of bugs.

s3/src/request/backend.rs (3)

30-52: LGTM!

The async Backend trait correctly leverages higher-ranked trait bounds (for<'a>) to allow handling requests with arbitrary lifetimes. The Clone + Send + Sync bounds are appropriate for async contexts where the backend may be shared across tasks. The blanket implementation enables any compatible tower_service::Service to automatically be a Backend.


21-28: LGTM!

SyncService provides a clean synchronous counterpart to tower_service::Service, appropriate for blocking backends. The trait is simple and focused.


6-11: Based on my verification through web search, I've confirmed that the underlying concern about Rust's feature flag behavior is technically correct: Cargo features are additive (unified), meaning multiple features can be enabled simultaneously. When that happens, having multiple #[cfg(feature = "...")] blocks with the same target name (like DefaultBackend) will produce duplicate definitions, causing Rust compilation error E0592.

However, I cannot verify whether:

  1. The codebase implements safeguards (e.g., compile_error! guards, module-based separation)
  2. Cargo.toml enforces mutual exclusivity at the package level
  3. CI/tests prevent this scenario from occurring

Since the repository clone failed and I cannot inspect the actual implementation details to confirm whether mitigations are in place, the verification remains inconclusive.


Verify feature flag mutual exclusivity for DefaultBackend.

Three separate #[cfg] blocks define DefaultBackend for different features (with-async-std, sync, with-tokio). Cargo features are additive by default, so enabling multiple features simultaneously would cause duplicate definitions, resulting in a Rust compilation error (E0592). Verify that either:

  • A compile_error! guard prevents incompatible features from being enabled together, or
  • Cargo.toml enforces feature-level mutual exclusivity constraints, or
  • The feature design has been refactored to handle additive features safely.
s3/src/bucket.rs (12)

40-54: LGTM!

The new imports properly support the pluggable backend abstraction. The Cow type for request bodies, backend traits, and response handling utilities are all correctly imported.


132-181: LGTM!

The generic Bucket<B> design is clean. The with_backend method correctly clones all bucket configuration while allowing backend substitution, enabling composable backend customization (e.g., adding timeouts as shown in tests).


237-273: LGTM!

The exec_request methods properly abstract over the backend trait. The async variant correctly polls for service readiness before invoking the backend, while the sync variant assumes immediate readiness. Request cloning inside the retry loop is necessary and Cow<'_, [u8]> makes this efficient for borrowed payloads.


275-280: LGTM!

The 7-day (604800 seconds) maximum is correct per AWS S3 presigned URL limits.


1283-1296: LGTM!

The stream handling correctly uses http_body_util::BodyExt to convert the response body to a data stream. The error mapping properly transforms the body error type into S3Error.


2064-2074: LGTM!

The head_object implementation correctly discards the body (HEAD responses have no body) and extracts headers to construct the result.


3002-3061: LGTM!

Test helper functions are correctly updated to use the new Box<Bucket<DefaultBackend>> return type, consistent with the Bucket::new signature change.


4090-4110: Good test coverage for the pluggable backend feature.

This test effectively validates the core capability: injecting a custom backend that returns controlled responses. The AlwaysFailBackend returning 418 (I'm a teapot) is a clever way to verify the custom backend is being used.


644-651: LGTM!

The presign_post implementation correctly clones and boxes the bucket for the PostPolicy::sign method, which requires ownership of the bucket.


3068-3072: LGTM!

The generic test function signatures allow testing with any backend implementation, which is excellent for verifying custom backend behavior. The trait bounds correctly constrain the backend to produce compatible responses.


1590-1590: LGTM!

The FuturesUnordered import is correctly placed within the function scope where it's used for bounded parallel multipart uploads.


3943-3965: The review comment is incorrect. SurfBackend (async-std) intentionally does not support with_request_timeout.

The web search confirms that with_request_timeout is only honored by the attohttpc and hyper backends. The surf backend (used with with-async-std feature) has its own timeout mechanism via surf::Config::set_timeout(). The test's feature gate #[cfg(any(feature = "with-tokio", feature = "sync"))] correctly excludes async-std because SurfBackend lacks the with_request_timeout method—not due to an oversight, but by design. Enabling this test for async-std would be incorrect and would likely fail at runtime.

Comment on lines +1298 to +1302
fn fake_credentials() -> Credentials {
let access_key = "AKIAIOSFODNN7EXAMPLE";
let secert_key = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY";
Credentials::new(Some(access_key), Some(secert_key), None, None, None).unwrap()
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Typo in variable name: secert_keysecret_key.

Minor typo in the test helper that doesn't affect functionality but reduces code clarity.

     fn fake_credentials() -> Credentials {
         let access_key = "AKIAIOSFODNN7EXAMPLE";
-        let secert_key = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY";
-        Credentials::new(Some(access_key), Some(secert_key), None, None, None).unwrap()
+        let secret_key = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY";
+        Credentials::new(Some(access_key), Some(secret_key), None, None, None).unwrap()
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn fake_credentials() -> Credentials {
let access_key = "AKIAIOSFODNN7EXAMPLE";
let secert_key = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY";
Credentials::new(Some(access_key), Some(secert_key), None, None, None).unwrap()
}
fn fake_credentials() -> Credentials {
let access_key = "AKIAIOSFODNN7EXAMPLE";
let secret_key = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY";
Credentials::new(Some(access_key), Some(secret_key), None, None, None).unwrap()
}
🤖 Prompt for AI Agents
In s3/src/request/request_trait.rs around lines 1298 to 1302, there's a typo in
the test helper variable name `secert_key`; rename it to `secret_key` and update
any references within the function to use the corrected identifier so the code
is clearer (no behavior change).

{
type Response = http::Response<reqwest::Body>;
type Error = S3Error;
type Future = Pin<Box<dyn Future<Output = Result<Self::Response, S3Error>> + Send>>;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Missing Future trait import.

Line 61 uses Future in the type alias, but there's no use std::future::Future; import visible. This will cause a compilation error.

Add the missing import at the top of the file:

 use std::pin::Pin;
 use std::task::{Context, Poll};
 use std::time::Duration;
+use std::future::Future;
 use tower_service::Service;

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In s3/src/request/tokio_backend.rs around line 61 the type alias references
Future but the file is missing the import; add the missing import line `use
std::future::Future;` near the other use statements at the top of the file so
the type alias `type Future = Pin<Box<dyn Future<Output = Result<Self::Response,
S3Error>> + Send>>;` compiles.

Comment on lines +136 to 155
/// .set_dangereous_config(true, true)?;
/// # Ok(())
/// # }
/// ```
#[cfg(any(feature = "tokio-native-tls", feature = "tokio-rustls-tls"))]
pub fn with_dangereous_config(
&self,
accept_invalid_certs: bool,
accept_invalid_hostnames: bool,
) -> Result<Self, S3Error> {
let client_options = ClientOptions {
accept_invalid_certs,
accept_invalid_hostnames,
..self.client_options.clone()
};
Ok(Self {
http_client: client(&client_options)?,
client_options,
})
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Typo in method name: dangereousdangerous.

The method with_dangereous_config (Line 141) and the docstring reference to set_dangereous_config (Line 136) contain a typo. Since this is a public API, consider fixing before release.

-    /// let bucket = Bucket::new("my-bucket", Region::from_str("us-east-1")?, Credentials::default()?)?
-    ///     .set_dangereous_config(true, true)?;
+    /// let bucket = Bucket::new("my-bucket", Region::from_str("us-east-1")?, Credentials::default()?)?
+    ///     .set_dangerous_config(true, true)?;
     /// # Ok(())
     /// # }
     /// ```
     #[cfg(any(feature = "tokio-native-tls", feature = "tokio-rustls-tls"))]
-    pub fn with_dangereous_config(
+    pub fn with_dangerous_config(
🤖 Prompt for AI Agents
In s3/src/request/tokio_backend.rs around lines 136 to 155, the public API
method and doc reference use the typoed name "dangereous" instead of
"dangerous"; rename the method to with_dangerous_config and update the docstring
examples to set_dangerous_config (and any other text occurrences) to match;
update all internal and public call sites to the new name, and to avoid breaking
existing users consider adding a small deprecated wrapper function named
with_dangereous_config that calls the new with_dangerous_config and is annotated
with #[deprecated] (plus update tests and documentation).

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