Refactor ObjectClient.get_object to use an GetObjectParams parameter

mountpoint-s3-client/examples/client_benchmark.rs

@@ -9,7 +9,7 @@ use futures::StreamExt;
 use mountpoint_s3_client::config::{EndpointConfig, S3ClientConfig};
 use mountpoint_s3_client::mock_client::throughput_client::ThroughputMockClient;
 use mountpoint_s3_client::mock_client::{MockClientConfig, MockObject};
-use mountpoint_s3_client::types::ETag;
+use mountpoint_s3_client::types::{ETag, GetObjectParams};
 use mountpoint_s3_client::{ObjectClient, S3CrtClient};
 use mountpoint_s3_crt::common::rust_log_adapter::RustLogAdapter;
 use tracing_subscriber::fmt::Subscriber;
@@ -46,7 +46,7 @@ fn run_benchmark(
                 scope.spawn(|| {
                     futures::executor::block_on(async move {
                         let mut request = client
-                            .get_object(bucket, key, None, None)
+                            .get_object(bucket, key, &GetObjectParams::new())
                             .await
                             .expect("couldn't create get request");
                         let mut request = pin!(request);

mountpoint-s3-client/examples/download.rs

@@ -4,6 +4,7 @@ use std::sync::{Arc, Mutex};
 use clap::{Arg, Command};
 use futures::StreamExt;
 use mountpoint_s3_client::config::{EndpointConfig, S3ClientConfig};
+use mountpoint_s3_client::types::GetObjectParams;
 use mountpoint_s3_client::{ObjectClient, S3CrtClient};
 use mountpoint_s3_crt::common::rust_log_adapter::RustLogAdapter;
 use regex::Regex;
@@ -58,7 +59,7 @@ fn main() {
     let last_offset_clone = Arc::clone(&last_offset);
     futures::executor::block_on(async move {
         let mut request = client
-            .get_object(bucket, key, range, None)
+            .get_object(bucket, key, &GetObjectParams::new().range(range))
             .await
             .expect("couldn't create get request");
         loop {

mountpoint-s3-client/src/failure_client.rs

@@ -16,10 +16,10 @@ use pin_project::pin_project;
 
 use crate::object_client::{
     CopyObjectError, CopyObjectParams, CopyObjectResult, DeleteObjectError, DeleteObjectResult, ETag, GetBodyPart,
-    GetObjectAttributesError, GetObjectAttributesResult, GetObjectError, GetObjectRequest, HeadObjectError,
-    HeadObjectParams, HeadObjectResult, ListObjectsError, ListObjectsResult, ObjectAttribute, ObjectClient,
-    ObjectClientError, ObjectClientResult, ObjectMetadata, PutObjectError, PutObjectParams, PutObjectRequest,
-    PutObjectResult, PutObjectSingleParams, UploadReview,
+    GetObjectAttributesError, GetObjectAttributesResult, GetObjectError, GetObjectParams, GetObjectRequest,
+    HeadObjectError, HeadObjectParams, HeadObjectResult, ListObjectsError, ListObjectsResult, ObjectAttribute,
+    ObjectClient, ObjectClientError, ObjectClientResult, ObjectMetadata, PutObjectError, PutObjectParams,
+    PutObjectRequest, PutObjectResult, PutObjectSingleParams, UploadReview,
 };
 
 // Wrapper for injecting failures into a get stream or a put request
@@ -123,17 +123,16 @@ where
         &self,
         bucket: &str,
         key: &str,
-        range: Option<Range<u64>>,
-        if_match: Option<ETag>,
+        params: &GetObjectParams,
     ) -> ObjectClientResult<Self::GetObjectRequest, GetObjectError, Self::ClientError> {
         let wrapper = (self.get_object_cb)(
             &mut *self.state.lock().unwrap(),
             bucket,
             key,
-            range.clone(),
-            if_match.clone(),
+            params.range.clone(),
+            params.if_match.clone(),
         )?;
-        let request = self.client.get_object(bucket, key, range, if_match).await?;
+        let request = self.client.get_object(bucket, key, params).await?;
         Ok(FailureGetRequest {
             state: wrapper.state,
             result_fn: wrapper.result_fn,
@@ -486,7 +485,7 @@ mod tests {
 
         let fail_set = HashSet::from([2, 4, 5]);
         for i in 1..=6 {
-            let r = fail_client.get_object(bucket, key, None, None).await;
+            let r = fail_client.get_object(bucket, key, &GetObjectParams::new()).await;
             if fail_set.contains(&i) {
                 assert!(r.is_err());
             } else {

mountpoint-s3-client/src/lib.rs

@@ -21,7 +21,7 @@
 //!
 //! let client = S3CrtClient::new(Default::default()).expect("client construction failed");
 //!
-//! let response = client.get_object("my-bucket", "my-key", None, None).await.expect("get_object failed");
+//! let response = client.get_object("my-bucket", "my-key", &GetObjectParams::new().await.expect("get_object failed"));
 //! let body = response.map_ok(|(offset, body)| body.to_vec()).try_concat().await.expect("body streaming failed");
 //! # }
 //! ```
@@ -73,9 +73,9 @@ pub mod config {
 pub mod types {
     pub use super::object_client::{
         Checksum, ChecksumAlgorithm, ChecksumMode, CopyObjectParams, CopyObjectResult, DeleteObjectResult, ETag,
-        GetBodyPart, GetObjectAttributesParts, GetObjectAttributesResult, GetObjectRequest, HeadObjectParams,
-        HeadObjectResult, ListObjectsResult, ObjectAttribute, ObjectClientResult, ObjectInfo, ObjectPart,
-        PutObjectParams, PutObjectResult, PutObjectSingleParams, PutObjectTrailingChecksums, RestoreStatus,
+        GetBodyPart, GetObjectAttributesParts, GetObjectAttributesResult, GetObjectParams, GetObjectRequest,
+        HeadObjectParams, HeadObjectResult, ListObjectsResult, ObjectAttribute, ObjectClientResult, ObjectInfo,
+        ObjectPart, PutObjectParams, PutObjectResult, PutObjectSingleParams, PutObjectTrailingChecksums, RestoreStatus,
         UploadChecksum, UploadReview, UploadReviewPart,
     };
 }

mountpoint-s3-client/src/mock_client.rs

@@ -5,7 +5,6 @@
 use std::borrow::Cow;
 use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
 use std::fmt::Write;
-use std::ops::Range;
 use std::pin::Pin;
 use std::sync::{Arc, RwLock};
 use std::task::{Context, Poll};
@@ -28,10 +27,11 @@ use crate::error_metadata::{ClientErrorMetadata, ProvideErrorMetadata};
 use crate::object_client::{
     Checksum, ChecksumAlgorithm, ChecksumMode, CopyObjectError, CopyObjectParams, CopyObjectResult, DeleteObjectError,
     DeleteObjectResult, ETag, GetBodyPart, GetObjectAttributesError, GetObjectAttributesParts,
-    GetObjectAttributesResult, GetObjectError, GetObjectRequest, HeadObjectError, HeadObjectParams, HeadObjectResult,
-    ListObjectsError, ListObjectsResult, ObjectAttribute, ObjectClient, ObjectClientError, ObjectClientResult,
-    ObjectInfo, ObjectMetadata, ObjectPart, PutObjectError, PutObjectParams, PutObjectRequest, PutObjectResult,
-    PutObjectSingleParams, PutObjectTrailingChecksums, RestoreStatus, UploadChecksum, UploadReview, UploadReviewPart,
+    GetObjectAttributesResult, GetObjectError, GetObjectParams, GetObjectRequest, HeadObjectError, HeadObjectParams,
+    HeadObjectResult, ListObjectsError, ListObjectsResult, ObjectAttribute, ObjectClient, ObjectClientError,
+    ObjectClientResult, ObjectInfo, ObjectMetadata, ObjectPart, PutObjectError, PutObjectParams, PutObjectRequest,
+    PutObjectResult, PutObjectSingleParams, PutObjectTrailingChecksums, RestoreStatus, UploadChecksum, UploadReview,
+    UploadReviewPart,
 };
 
 mod leaky_bucket;
@@ -660,10 +660,9 @@ impl ObjectClient for MockClient {
         &self,
         bucket: &str,
         key: &str,
-        range: Option<Range<u64>>,
-        if_match: Option<ETag>,
+        params: &GetObjectParams,
     ) -> ObjectClientResult<Self::GetObjectRequest, GetObjectError, Self::ClientError> {
-        trace!(bucket, key, ?range, ?if_match, "GetObject");
+        trace!(bucket, key, ?params.range, ?params.if_match, "GetObject");
         self.inc_op_count(Operation::GetObject);
 
         if bucket != self.config.bucket {
@@ -673,13 +672,13 @@ impl ObjectClient for MockClient {
         let objects = self.objects.read().unwrap();
 
         if let Some(object) = objects.get(key) {
-            if let Some(etag_match) = if_match {
-                if etag_match != object.etag {
+            if let Some(etag_match) = params.if_match.as_ref() {
+                if etag_match != &object.etag {
                     return Err(ObjectClientError::ServiceError(GetObjectError::PreconditionFailed));
                 }
             }
 
-            let (next_offset, length) = if let Some(range) = range {
+            let (next_offset, length) = if let Some(range) = params.range.as_ref() {
                 if range.start >= object.len() as u64 || range.end > object.len() as u64 {
                     return mock_client_error(format!("invalid range, length={}", object.len()));
                 }
@@ -1050,6 +1049,7 @@ mod tests {
     use futures::{pin_mut, StreamExt};
     use rand::{Rng, RngCore, SeedableRng};
     use rand_chacha::ChaChaRng;
+    use std::ops::Range;
     use test_case::test_case;
 
     use super::*;
@@ -1089,7 +1089,7 @@ mod tests {
         client.add_object(key, object);
 
         let mut get_request = client
-            .get_object("test_bucket", key, range.clone(), None)
+            .get_object("test_bucket", key, &GetObjectParams::new().range(range.clone()))
             .await
             .expect("should not fail");
 
@@ -1143,7 +1143,7 @@ mod tests {
         client.add_object(key, MockObject::from_bytes(&body, ETag::for_tests()));
 
         let get_request = client
-            .get_object("test_bucket", key, range.clone(), None)
+            .get_object("test_bucket", key, &GetObjectParams::new().range(range.clone()))
             .await
             .expect("should not fail");
         pin_mut!(get_request);
@@ -1191,33 +1191,45 @@ mod tests {
         client.add_object("key1", body[..].into());
 
         assert!(matches!(
-            client.get_object("wrong_bucket", "key1", None, None).await,
+            client.get_object("wrong_bucket", "key1", &GetObjectParams::new()).await,
             Err(ObjectClientError::ServiceError(GetObjectError::NoSuchBucket))
         ));
 
         assert!(matches!(
-            client.get_object("test_bucket", "wrong_key", None, None).await,
+            client
+                .get_object("test_bucket", "wrong_key", &GetObjectParams::new())
+                .await,
             Err(ObjectClientError::ServiceError(GetObjectError::NoSuchKey))
         ));
 
         assert_client_error!(
-            client.get_object("test_bucket", "key1", Some(0..2001), None).await,
+            client
+                .get_object("test_bucket", "key1", &GetObjectParams::new().range(Some(0..2001)))
+                .await,
             "invalid range, length=2000"
         );
         assert_client_error!(
-            client.get_object("test_bucket", "key1", Some(2000..2000), None).await,
+            client
+                .get_object("test_bucket", "key1", &GetObjectParams::new().range(Some(2000..2000)))
+                .await,
             "invalid range, length=2000"
         );
         assert_client_error!(
-            client.get_object("test_bucket", "key1", Some(500..2001), None).await,
+            client
+                .get_object("test_bucket", "key1", &GetObjectParams::new().range(Some(500..2001)))
+                .await,
             "invalid range, length=2000"
         );
         assert_client_error!(
-            client.get_object("test_bucket", "key1", Some(5000..2001), None).await,
+            client
+                .get_object("test_bucket", "key1", &GetObjectParams::new().range(Some(5000..2001)))
+                .await,
             "invalid range, length=2000"
         );
         assert_client_error!(
-            client.get_object("test_bucket", "key1", Some(5000..1), None).await,
+            client
+                .get_object("test_bucket", "key1", &GetObjectParams::new().range(Some(5000..1)))
+                .await,
             "invalid range, length=2000"
         );
     }
@@ -1245,7 +1257,7 @@ mod tests {
         client.add_object(key, MockObject::from_bytes(&expected_body, ETag::for_tests()));
 
         let mut get_request = client
-            .get_object("test_bucket", key, Some(range.clone()), None)
+            .get_object("test_bucket", key, &GetObjectParams::new().range(Some(range.clone())))
             .await
             .expect("should not fail");
 
@@ -1282,7 +1294,7 @@ mod tests {
             .expect("Should not fail");
 
         client
-            .get_object(bucket, dst_key, None, None)
+            .get_object(bucket, dst_key, &GetObjectParams::new())
             .await
             .expect("get_object should succeed");
     }
@@ -1706,7 +1718,7 @@ mod tests {
         put_request.complete().await.expect("put_object failed");
 
         let mut get_request = client
-            .get_object("test_bucket", "key1", None, None)
+            .get_object("test_bucket", "key1", &GetObjectParams::new())
             .await
             .expect("get_object failed");
 
@@ -1739,7 +1751,7 @@ mod tests {
             .expect("put_object failed");
 
         let get_request = client
-            .get_object("test_bucket", "key1", None, None)
+            .get_object("test_bucket", "key1", &GetObjectParams::new())
             .await
             .expect("get_object failed");
 

mountpoint-s3-client/src/mock_client/throughput_client.rs

@@ -1,4 +1,3 @@
-use std::ops::Range;
 use std::pin::Pin;
 use std::task::{Context, Poll};
 use std::time::Duration;
@@ -14,10 +13,11 @@ use crate::mock_client::{
     MockClient, MockClientConfig, MockClientError, MockGetObjectRequest, MockObject, MockPutObjectRequest,
 };
 use crate::object_client::{
-    CopyObjectError, CopyObjectParams, CopyObjectResult, DeleteObjectError, DeleteObjectResult, ETag, GetBodyPart,
-    GetObjectAttributesError, GetObjectAttributesResult, GetObjectError, GetObjectRequest, HeadObjectError,
-    HeadObjectParams, HeadObjectResult, ListObjectsError, ListObjectsResult, ObjectAttribute, ObjectClient,
-    ObjectClientResult, ObjectMetadata, PutObjectError, PutObjectParams, PutObjectResult, PutObjectSingleParams,
+    CopyObjectError, CopyObjectParams, CopyObjectResult, DeleteObjectError, DeleteObjectResult, GetBodyPart,
+    GetObjectAttributesError, GetObjectAttributesResult, GetObjectError, GetObjectParams, GetObjectRequest,
+    HeadObjectError, HeadObjectParams, HeadObjectResult, ListObjectsError, ListObjectsResult, ObjectAttribute,
+    ObjectClient, ObjectClientResult, ObjectMetadata, PutObjectError, PutObjectParams, PutObjectResult,
+    PutObjectSingleParams,
 };
 
 /// A [MockClient] that rate limits overall download throughput to simulate a target network
@@ -148,10 +148,9 @@ impl ObjectClient for ThroughputMockClient {
         &self,
         bucket: &str,
         key: &str,
-        range: Option<Range<u64>>,
-        if_match: Option<ETag>,
+        params: &GetObjectParams,
     ) -> ObjectClientResult<Self::GetObjectRequest, GetObjectError, Self::ClientError> {
-        let request = self.inner.get_object(bucket, key, range, if_match).await?;
+        let request = self.inner.get_object(bucket, key, params).await?;
         let rate_limiter = self.rate_limiter.clone();
         Ok(ThroughputGetObjectRequest { request, rate_limiter })
     }
@@ -219,6 +218,7 @@ mod tests {
     use futures::StreamExt;
 
     use crate::mock_client::MockObject;
+    use crate::types::ETag;
 
     use super::*;
 
@@ -245,7 +245,10 @@ mod tests {
                 let start = Instant::now();
                 let num_bytes = block_on(async move {
                     let mut num_bytes = 0;
-                    let mut get = client.get_object("test_bucket", "testfile", None, None).await.unwrap();
+                    let mut get = client
+                        .get_object("test_bucket", "testfile", &GetObjectParams::new())
+                        .await
+                        .unwrap();
                     while let Some(part) = get.next().await {
                         let (_offset, part) = part.unwrap();
                         num_bytes += part.len();

mountpoint-s3-client/src/object_client.rs

@@ -78,8 +78,7 @@ pub trait ObjectClient {
         &self,
         bucket: &str,
         key: &str,
-        range: Option<Range<u64>>,
-        if_match: Option<ETag>,
+        params: &GetObjectParams,
     ) -> ObjectClientResult<Self::GetObjectRequest, GetObjectError, Self::ClientError>;
 
     /// List the objects in a bucket under a given prefix
@@ -184,6 +183,33 @@ pub enum GetObjectError {
     PreconditionFailed,
 }
 
+/// Parameters to a [`get_object`](ObjectClient::get_object) request
+#[derive(Debug, Default, Clone)]
+#[non_exhaustive]
+pub struct GetObjectParams {
+    pub range: Option<Range<u64>>,
+    pub if_match: Option<ETag>,
+}
+
+impl GetObjectParams {
+    /// Create a default [GetObjectParams].
+    pub fn new() -> Self {
+        Self::default()
+    }
+
+    /// Set the range retrieved by the GetObject request
+    pub fn range(mut self, value: Option<Range<u64>>) -> Self {
+        self.range = value;
+        self
+    }
+
+    /// Set the required etag on the object
+    pub fn if_match(mut self, value: Option<ETag>) -> Self {
+        self.if_match = value;
+        self
+    }
+}
+
 /// Result of a [`list_objects`](ObjectClient::list_objects) request
 #[derive(Debug)]
 #[non_exhaustive]

mountpoint-s3-client/src/s3_crt_client.rs

@@ -1281,12 +1281,9 @@ impl ObjectClient for S3CrtClient {
         &self,
         bucket: &str,
         key: &str,
-        range: Option<Range<u64>>,
-        if_match: Option<ETag>,
-        // TODO: If more arguments are added to get object, make a request struct having those arguments
-        // along with bucket and key.
+        params: &GetObjectParams,
     ) -> ObjectClientResult<Self::GetObjectRequest, GetObjectError, Self::ClientError> {
-        self.get_object(bucket, key, range, if_match)
+        self.get_object(bucket, key, params)
     }
 
     async fn list_objects(