Remove use of ObjectInfo in S3 client HeadObject response (#1058)

* Remove use of ObjectInfo in S3 client HeadObject response Signed-off-by: Daniel Carl Jones <[email protected]> * Change HeadObjectResult etag field from String to ETag Signed-off-by: Daniel Carl Jones <[email protected]> --------- Signed-off-by: Daniel Carl Jones <[email protected]>
awslabs · Oct 24, 2024 · 4dc8e7d · 4dc8e7d
1 parent d4a31ee
commit 4dc8e7d
Show file tree

Hide file tree

Showing 7 changed files with 66 additions and 55 deletions.
diff --git a/mountpoint-s3-client/CHANGELOG.md b/mountpoint-s3-client/CHANGELOG.md
@@ -1,5 +1,18 @@
 ## Unreleased
 
+### Other changes
+
+* No other changes.
+
+### Breaking changes
+
+* `HeadObjectResult` no longer contains an `ObjectInfo` struct.
+  Instead, it returns the object attributes as individual fields on the `HeadObjectResult`.
+  The entity tag field has also changed and is now of type `ETag` rather than `String`.
+  ([#1058](https://github.com/awslabs/mountpoint-s3/pull/1058))
+* `HeadObjectResult` no longer provides the bucket and key used in the original request.
+  ([#1058](https://github.com/awslabs/mountpoint-s3/pull/1058))
+
 ## v0.11.0 (October 17, 2024)
 
 ### Breaking changes

diff --git a/mountpoint-s3-client/src/mock_client.rs b/mountpoint-s3-client/src/mock_client.rs
@@ -687,15 +687,11 @@ impl ObjectClient for MockClient {
         let objects = self.objects.read().unwrap();
         if let Some(object) = objects.get(key) {
             Ok(HeadObjectResult {
-                bucket: bucket.to_string(),
-                object: ObjectInfo {
-                    key: key.to_string(),
-                    size: object.size as u64,
-                    last_modified: object.last_modified,
-                    etag: object.etag.as_str().to_string(),
-                    storage_class: object.storage_class.clone(),
-                    restore_status: object.restore_status,
-                },
+                size: object.size as u64,
+                last_modified: object.last_modified,
+                etag: object.etag.clone(),
+                storage_class: object.storage_class.clone(),
+                restore_status: object.restore_status,
             })
         } else {
             Err(ObjectClientError::ServiceError(HeadObjectError::NotFound))
@@ -1681,7 +1677,7 @@ mod tests {
 
         // head_object returns storage class
         let head_result = client.head_object(bucket, key).await.unwrap();
-        assert_eq!(head_result.object.storage_class.as_deref(), storage_class);
+        assert_eq!(head_result.storage_class.as_deref(), storage_class);
 
         // list_objects returns storage class
         let list_result = client.list_objects(bucket, None, "/", 1, "").await.unwrap();

diff --git a/mountpoint-s3-client/src/object_client.rs b/mountpoint-s3-client/src/object_client.rs
@@ -210,11 +210,28 @@ pub enum ListObjectsError {
 #[derive(Debug)]
 #[non_exhaustive]
 pub struct HeadObjectResult {
-    /// The name of the bcuket
-    pub bucket: String,
+    /// Size of the object in bytes.
+    ///
+    /// Refers to the `Content-Length` HTTP header for HeadObject.
+    pub size: u64,
+
+    /// The time this object was last modified.
+    pub last_modified: OffsetDateTime,
+
+    /// Entity tag of this object.
+    pub etag: ETag,
+
+    /// Storage class for this object.
+    ///
+    /// The value is optional because HeadObject does not return the storage class in its response
+    /// for objects in the S3 Standard storage class.
+    /// See examples in the
+    /// [Amazon S3 API Reference](https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html#API_HeadObject_Examples).
+    pub storage_class: Option<String>,
 
-    /// Object metadata
-    pub object: ObjectInfo,
+    /// Objects in flexible retrieval storage classes (such as GLACIER and DEEP_ARCHIVE) are only
+    /// accessible after restoration
+    pub restore_status: Option<RestoreStatus>,
 }
 
 /// Errors returned by a [`head_object`](ObjectClient::head_object) request

diff --git a/mountpoint-s3-client/src/s3_crt_client/head_object.rs b/mountpoint-s3-client/src/s3_crt_client/head_object.rs
@@ -11,9 +11,7 @@ use time::format_description::well_known::Rfc2822;
 use time::OffsetDateTime;
 use tracing::error;
 
-use crate::object_client::{
-    HeadObjectError, HeadObjectResult, ObjectClientError, ObjectClientResult, ObjectInfo, RestoreStatus,
-};
+use crate::object_client::{HeadObjectError, HeadObjectResult, ObjectClientError, ObjectClientResult, RestoreStatus};
 use crate::s3_crt_client::{S3CrtClient, S3Operation, S3RequestError};
 
 #[derive(Error, Debug)]
@@ -85,23 +83,23 @@ impl HeadObjectResult {
         Ok(Some(RestoreStatus::Restored { expiry: expiry.into() }))
     }
 
-    fn parse_from_hdr(bucket: String, key: String, headers: &Headers) -> Result<Self, ParseError> {
+    /// Parse from HeadObject headers
+    fn parse_from_hdr(headers: &Headers) -> Result<Self, ParseError> {
         let last_modified = OffsetDateTime::parse(&get_field(headers, "Last-Modified")?, &Rfc2822)
             .map_err(|e| ParseError::OffsetDateTime(e, "LastModified".into()))?;
         let size = u64::from_str(&get_field(headers, "Content-Length")?)
             .map_err(|e| ParseError::Int(e, "ContentLength".into()))?;
         let etag = get_field(headers, "Etag")?;
         let storage_class = get_optional_field(headers, "x-amz-storage-class")?;
         let restore_status = Self::parse_restore_status(headers)?;
-        let object = ObjectInfo {
-            key,
+        let result = HeadObjectResult {
             size,
             last_modified,
             storage_class,
             restore_status,
-            etag,
+            etag: etag.into(),
         };
-        Ok(HeadObjectResult { bucket, object })
+        Ok(result)
     }
 }
 
@@ -137,11 +135,7 @@ impl S3CrtClient {
                 span,
                 move |headers, _status| {
                     let mut header = header1.lock().unwrap();
-                    *header = Some(HeadObjectResult::parse_from_hdr(
-                        bucket.to_string(),
-                        key.to_string(),
-                        headers,
-                    ));
+                    *header = Some(HeadObjectResult::parse_from_hdr(headers));
                 },
                 |_, _| (),
                 move |result| {

diff --git a/mountpoint-s3-client/tests/head_object.rs b/mountpoint-s3-client/tests/head_object.rs
@@ -22,7 +22,7 @@ async fn test_head_object() {
     let sdk_client = get_test_sdk_client().await;
     let (bucket, prefix) = get_test_bucket_and_prefix("test_head_object");
 
-    let key = format!("{prefix}/hello");
+    let key = format!("{prefix}hello");
     let body = b"hello world!";
     sdk_client
         .put_object()
@@ -36,9 +36,11 @@ async fn test_head_object() {
     let client: S3CrtClient = get_test_client();
     let result = client.head_object(&bucket, &key).await.expect("head_object failed");
 
-    assert_eq!(result.bucket, bucket);
-    assert_eq!(result.object.key, key);
-    assert_eq!(result.object.size as usize, body.len());
+    assert_eq!(
+        result.size as usize,
+        body.len(),
+        "HeadObject reported size should match uploaded body length",
+    );
 }
 
 #[test_case("INTELLIGENT_TIERING")]
@@ -65,11 +67,9 @@ async fn test_head_object_storage_class(storage_class: &str) {
     let client: S3CrtClient = get_test_client();
     let result = client.head_object(&bucket, &key).await.expect("head_object failed");
 
-    assert_eq!(result.bucket, bucket);
-    assert_eq!(result.object.key, key);
-    assert_eq!(result.object.size as usize, body.len());
-    assert_eq!(result.object.storage_class.as_deref(), Some(storage_class));
-    assert!(result.object.restore_status.is_none());
+    assert_eq!(result.size as usize, body.len());
+    assert_eq!(result.storage_class.as_deref(), Some(storage_class));
+    assert!(result.restore_status.is_none());
 }
 
 #[tokio::test]
@@ -140,10 +140,8 @@ async fn test_head_object_restored() {
     let client: S3CrtClient = get_test_client();
     let result = client.head_object(&bucket, &key).await.expect("head_object failed");
 
-    assert_eq!(result.bucket, bucket);
-    assert_eq!(result.object.key, key);
     assert!(
-        result.object.restore_status.is_none(),
+        result.restore_status.is_none(),
         "object should become restored only after restoration"
     );
 
@@ -168,18 +166,14 @@ async fn test_head_object_restored() {
 
     let timeout = Duration::from_secs(300);
     let start = Instant::now();
-    let mut timeouted = true;
+    let mut timeout_exceeded = true;
     while start.elapsed() < timeout {
-        let object = client
-            .head_object(&bucket, &key)
-            .await
-            .expect("head_object failed")
-            .object;
-        if let Some(RestoreStatus::Restored { expiry: _ }) = object.restore_status {
-            timeouted = false;
+        let response = client.head_object(&bucket, &key).await.expect("head_object failed");
+        if let Some(RestoreStatus::Restored { expiry: _ }) = response.restore_status {
+            timeout_exceeded = false;
             break;
         }
         std::thread::sleep(Duration::from_secs(1));
     }
-    assert!(!timeouted, "timeouted while waiting for object become restored");
+    assert!(!timeout_exceeded, "timeouted while waiting for object become restored");
 }
diff --git a/mountpoint-s3/examples/prefetch_benchmark.rs b/mountpoint-s3/examples/prefetch_benchmark.rs
@@ -1,4 +1,3 @@
-use std::str::FromStr;
 use std::sync::atomic::{AtomicU64, Ordering};
 use std::sync::Arc;
 use std::thread;
@@ -10,7 +9,6 @@ use mountpoint_s3::mem_limiter::MemoryLimiter;
 use mountpoint_s3::object::ObjectId;
 use mountpoint_s3::prefetch::{default_prefetch, Prefetch, PrefetchResult};
 use mountpoint_s3_client::config::{EndpointConfig, S3ClientConfig};
-use mountpoint_s3_client::types::ETag;
 use mountpoint_s3_client::{ObjectClient, S3CrtClient};
 use mountpoint_s3_crt::common::rust_log_adapter::RustLogAdapter;
 use sysinfo::{RefreshKind, System};
@@ -113,8 +111,8 @@ fn main() {
     let mem_limiter = Arc::new(MemoryLimiter::new(client.clone(), max_memory_target));
 
     let head_object_result = block_on(client.head_object(bucket, key)).expect("HeadObject failed");
-    let size = head_object_result.object.size;
-    let etag = ETag::from_str(&head_object_result.object.etag).unwrap();
+    let size = head_object_result.size;
+    let etag = head_object_result.etag;
 
     for i in 0..iterations.unwrap_or(1) {
         let runtime = client.event_loop_group();

diff --git a/mountpoint-s3/src/superblock.rs b/mountpoint-s3/src/superblock.rs
@@ -699,8 +699,8 @@ impl SuperblockInner {
             select_biased! {
                 result = file_lookup => {
                     match result {
-                        Ok(HeadObjectResult { object, .. }) => {
-                            let stat = InodeStat::for_file(object.size as usize, object.last_modified, Some(object.etag.clone()), object.storage_class, object.restore_status, self.config.cache_config.file_ttl);
+                        Ok(HeadObjectResult { size, last_modified, restore_status ,etag, storage_class, .. }) => {
+                            let stat = InodeStat::for_file(size as usize, last_modified, Some(etag.as_str().to_string()), storage_class, restore_status, self.config.cache_config.file_ttl);
                             file_state = Some(stat);
                         }
                         // If the object is not found, might be a directory, so keep going
@@ -1276,7 +1276,6 @@ mod tests {
                         .head_object(bucket, file.inode.full_key())
                         .await
                         .expect("object should exist")
-                        .object
                         .last_modified;
                     assert_inode_stat!(file, InodeKind::File, modified_time, object_size);
                     assert_eq!(