Skip to content

Comments

minio-rs: refactored all functions#145

Merged
donatello merged 15 commits intominio:masterfrom
HJLebbink:refactor-remaining
Apr 23, 2025
Merged

minio-rs: refactored all functions#145
donatello merged 15 commits intominio:masterfrom
HJLebbink:refactor-remaining

Conversation

@HJLebbink
Copy link
Member

refactored stat_object
refactored select_object_content
refactor get_presigned_object_url
refactor get_presigned_policy_form_data
refactored upload-part-copy

@HJLebbink HJLebbink requested a review from Copilot April 1, 2025 20:18
@HJLebbink HJLebbink added the enhancement Used in release doc generation label Apr 1, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors several S3 builder functions to simplify request construction and improve consistency. Key changes include:

  • Changing the ToS3Request implementations to consume self instead of taking a reference.
  • Replacing manual header and query parameter construction with an insert helper function.
  • Updating module re-exports and import paths to reflect newly refactored functions.

Reviewed Changes

Copilot reviewed 122 out of 122 changed files in this pull request and generated no comments.

File Description
src/s3/builders/* Refactored multiple builder functions to consume self and use the insert helper
src/s3/builders.rs Updated module re-exports to include new functions like get_presigned_object_url
common/src/example.rs Adjusted import paths to use the builders module for PostPolicy
benches/s3/common_benches.rs Added explicit type annotation for request for clarity
Comments suppressed due to low confidence (2)

src/s3/builders/get_bucket_tags.rs:76

  • Changing the function signature to consume self instead of using &self alters builder reusability. Please confirm that the intended design is for these builder instances to be used only once.
fn to_s3request(self) -> Result<S3Request, Error> {

src/s3/builders/get_bucket_lifecycle.rs:42

  • The region value is wrapped in an Option using Some(region) here, while in other implementations the region is passed directly as self.region. Please verify that the types and handling for region are consistent across all builders.
.region(Some(region))

@HJLebbink HJLebbink requested a review from donatello April 1, 2025 20:22
refactored select_object_content

refactor get_presigned_object_url

refactor get_presigned_policy_form_data

refactored upload-part-copy
@HJLebbink HJLebbink force-pushed the refactor-remaining branch from 8b7a739 to 6a15d89 Compare April 1, 2025 22:00
@HJLebbink HJLebbink force-pushed the refactor-remaining branch 7 times, most recently from 7b21771 to 1b661fb Compare April 2, 2025 12:17
@HJLebbink HJLebbink force-pushed the refactor-remaining branch 2 times, most recently from 0bc2c74 to 944859d Compare April 3, 2025 09:50
@HJLebbink HJLebbink requested a review from Copilot April 6, 2025 09:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 188 out of 188 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

common/src/test_context.rs:27

  • Ensure that all branches in TestContext's constructor consistently wrap the Client instance in an Arc. The diff only shows updates for one branch (the test branch), so verify that the non-test branch also creates an Arc to avoid type inconsistencies.
pub client: Arc<Client>,

@HJLebbink
Copy link
Member Author

@donatello I noticed a few functional differences between the various builders. I've tried to keep changes to a minimum, but this one really stands out and feels like a bug.

Some functions (like get_bucket_encryption) use the region provided by the user directly when sending an S3Request.

impl ToS3Request for GetBucketEncryption {
    fn to_s3request(self) -> Result<S3Request, Error> {
        check_bucket_name(&self.bucket, true)?;

        Ok(S3Request::new(self.client, Method::GET)
            .region(self.region)   // HERE
            .bucket(Some(self.bucket))
            .query_params(insert(self.extra_query_params, "encryption"))
            .headers(self.extra_headers.unwrap_or_default()))
    }
}

Whereas other functions (like get_bucket_lifecycle) use the region provided by the user to look up a cached region (and if it is not cached, makes a s3API get_region call)

impl ToS3Request for GetBucketLifecycle {
    fn to_s3request(self) -> Result<S3Request, Error> {
        check_bucket_name(&self.bucket, true)?;

        let region: String = self.client.get_region_cached(&self.bucket, &self.region)?;

        Ok(S3Request::new(self.client, Method::GET)
            .region(Some(region))
            .bucket(Some(self.bucket))
            .query_params(insert(self.extra_query_params, "lifecycle"))
            .headers(self.extra_headers.unwrap_or_default()))
    }
}

I reckon that for calls to minio it probably doesn't matter, but since we should be S3 compatible, I assume we need to do the cached lookup in all the builders? Right, or can we just drop the cached lookup accross the board?

@HJLebbink HJLebbink force-pushed the refactor-remaining branch 2 times, most recently from 4a590cc to 300557c Compare April 6, 2025 17:38
@HJLebbink HJLebbink force-pushed the refactor-remaining branch from 300557c to 562d52c Compare April 8, 2025 15:11
@HJLebbink HJLebbink self-assigned this Apr 8, 2025
@HJLebbink HJLebbink changed the title refactored remaining functions minio-rs: refactored all functions Apr 8, 2025
@HJLebbink HJLebbink force-pushed the refactor-remaining branch from 562d52c to 8278940 Compare April 8, 2025 18:50
@HJLebbink HJLebbink force-pushed the refactor-remaining branch from 8278940 to e49e230 Compare April 8, 2025 19:02
@HJLebbink HJLebbink force-pushed the refactor-remaining branch from 9226043 to 774c9cd Compare April 11, 2025 13:26
@HJLebbink HJLebbink force-pushed the refactor-remaining branch 5 times, most recently from 87a495a to 4651cf6 Compare April 13, 2025 19:47
@HJLebbink HJLebbink force-pushed the refactor-remaining branch from 4651cf6 to 4cccbe1 Compare April 13, 2025 19:50
@HJLebbink HJLebbink force-pushed the refactor-remaining branch from c48e61c to c4888d6 Compare April 16, 2025 13:52
@HJLebbink HJLebbink force-pushed the refactor-remaining branch 3 times, most recently from 355b8fc to 262a7c0 Compare April 16, 2025 20:51
@HJLebbink HJLebbink force-pushed the refactor-remaining branch from 262a7c0 to fd4c725 Compare April 16, 2025 21:48
@HJLebbink HJLebbink force-pushed the refactor-remaining branch from e1b41b6 to 78ebd60 Compare April 18, 2025 16:12
@HJLebbink HJLebbink force-pushed the refactor-remaining branch from 448436e to e9e2a97 Compare April 23, 2025 13:16
@donatello donatello requested a review from Copilot April 23, 2025 17:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors several functions and benchmark setups to improve consistency and code clarity across the repository. Key changes include refactoring the CleanupGuard constructor and updating various builder function calls in the benchmarks to use client clones and the new method signatures; additional minor dependency updates in Cargo.toml and new example additions support these refactors.

Reviewed Changes

Copilot reviewed 213 out of 213 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
common/src/cleanup_guard.rs Refactored CleanupGuard::new to accept a Client by value and a bucket name as Into; error logging now silently suppressed in Drop.
benches/s3/common_benches.rs Updated CleanupGuard instantiation to use client.clone() as required by the refactored API.
benches/s3/* Updated several benchmark functions to use the new builder patterns and incorporated skip_express_mode checks.
Cargo.toml Minor version updates to dependencies and addition of a new example for the append_object feature.
.github/workflows/rust.yml Updated workflow step naming for running S3 tests.

@donatello donatello merged commit 58d9203 into minio:master Apr 23, 2025
2 checks passed
@HJLebbink HJLebbink deleted the refactor-remaining branch April 23, 2025 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Used in release doc generation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants