refactor(storage): extract list object versions helpers (#1830)

rustfs/src/storage/ecfs.rs

@@ -23,7 +23,9 @@ use crate::storage::head_prefix::{head_prefix_not_found_message, probe_prefix_ha
 use crate::storage::helper::OperationHelper;
 use crate::storage::options::{filter_object_metadata, get_content_sha256};
 use crate::storage::readers::InMemoryAsyncReader;
-use crate::storage::s3_api::bucket::{build_list_objects_output, build_list_objects_v2_output};
+use crate::storage::s3_api::bucket::{
+    build_list_object_versions_output, build_list_objects_output, build_list_objects_v2_output, parse_list_object_versions_params,
+};
 use crate::storage::s3_api::common::rustfs_owner;
 use crate::storage::s3_api::multipart::{
     build_list_multipart_uploads_output, build_list_parts_output, parse_list_multipart_uploads_params, parse_list_parts_params,
@@ -3597,12 +3599,8 @@ impl S3 for FS {
             ..
         } = req.input;
 
-        let prefix = prefix.unwrap_or_default();
+        let (prefix, delimiter, key_marker, version_id_marker, max_keys) =
-        let max_keys = max_keys.unwrap_or(1000);
+            parse_list_object_versions_params(prefix, delimiter, key_marker, version_id_marker, max_keys)?;
-
-        let key_marker = key_marker.filter(|v| !v.is_empty());
-        let version_id_marker = version_id_marker.filter(|v| !v.is_empty());
-        let delimiter = delimiter.filter(|v| !v.is_empty());
 
         let store = get_validated_store(&bucket).await?;
 
@@ -3611,63 +3609,7 @@ impl S3 for FS {
             .await
             .map_err(ApiError::from)?;
 
-        let objects: Vec<ObjectVersion> = object_infos
+        let output = build_list_object_versions_output(object_infos, bucket, prefix, delimiter, max_keys);
-            .objects
-            .iter()
-            .filter(|v| !v.name.is_empty() && !v.delete_marker)
-            .map(|v| {
-                ObjectVersion {
-                    key: Some(v.name.to_owned()),
-                    last_modified: v.mod_time.map(Timestamp::from),
-                    size: Some(v.size),
-                    version_id: Some(v.version_id.map(|v| v.to_string()).unwrap_or_else(|| "null".to_string())),
-                    is_latest: Some(v.is_latest),
-                    e_tag: v.etag.clone().map(|etag| to_s3s_etag(&etag)),
-                    storage_class: v.storage_class.clone().map(ObjectVersionStorageClass::from),
-                    ..Default::default() // TODO: another fields
-                }
-            })
-            .collect();
-
-        let common_prefixes = object_infos
-            .prefixes
-            .into_iter()
-            .map(|v| CommonPrefix { prefix: Some(v) })
-            .collect();
-
-        let delete_markers = object_infos
-            .objects
-            .iter()
-            .filter(|o| o.delete_marker)
-            .map(|o| DeleteMarkerEntry {
-                key: Some(o.name.clone()),
-                version_id: Some(o.version_id.map(|v| v.to_string()).unwrap_or_else(|| "null".to_string())),
-                is_latest: Some(o.is_latest),
-                last_modified: o.mod_time.map(Timestamp::from),
-                ..Default::default()
-            })
-            .collect::<Vec<_>>();
-
-        // Only set next_key_marker and next_version_id_marker if they have values, per AWS S3 API spec
-        // boto3 expects them to be strings or omitted, not None or empty strings
-        let next_key_marker = object_infos.next_marker.filter(|v| !v.is_empty());
-        let next_version_id_marker = object_infos.next_version_idmarker.filter(|v| !v.is_empty());
-
-        let output = ListObjectVersionsOutput {
-            is_truncated: Some(object_infos.is_truncated),
-            // max_keys should be the requested maximum number of keys, not the actual count returned
-            // Per AWS S3 API spec, this field represents the maximum number of keys that can be returned in the response
-            max_keys: Some(max_keys),
-            delimiter,
-            name: Some(bucket),
-            prefix: Some(prefix),
-            common_prefixes: Some(common_prefixes),
-            versions: Some(objects),
-            delete_markers: Some(delete_markers),
-            next_key_marker,
-            next_version_id_marker,
-            ..Default::default()
-        };
 
         Ok(s3_response(output))
     }

rustfs/src/storage/s3_api/bucket.rs

@@ -13,12 +13,16 @@
 // limitations under the License.
 
 use rustfs_ecstore::client::object_api_utils::to_s3s_etag;
-use rustfs_ecstore::store_api::ListObjectsV2Info;
+use rustfs_ecstore::store_api::{ListObjectVersionsInfo, ListObjectsV2Info};
 use s3s::dto::{
-    CommonPrefix, EncodingType, ListObjectsOutput, ListObjectsV2Output, Object, ObjectStorageClass, Owner, Timestamp,
+    CommonPrefix, DeleteMarkerEntry, EncodingType, ListObjectVersionsOutput, ListObjectsOutput, ListObjectsV2Output, Object,
+    ObjectStorageClass, ObjectVersion, ObjectVersionStorageClass, Owner, Timestamp,
 };
+use s3s::{S3Error, S3ErrorCode};
 use urlencoding::encode;
 
+pub(crate) type ListObjectVersionsParams = (String, Option<String>, Option<String>, Option<String>, i32);
+
 pub(crate) fn build_list_objects_output(v2: ListObjectsV2Output, request_marker: Option<String>) -> ListObjectsOutput {
     let next_marker = calculate_next_marker(&v2);
 
@@ -41,6 +45,86 @@ pub(crate) fn build_list_objects_output(v2: ListObjectsV2Output, request_marker:
     }
 }
 
+pub(crate) fn parse_list_object_versions_params(
+    prefix: Option<String>,
+    delimiter: Option<String>,
+    key_marker: Option<String>,
+    version_id_marker: Option<String>,
+    max_keys: Option<i32>,
+) -> Result<ListObjectVersionsParams, S3Error> {
+    let prefix = prefix.unwrap_or_default();
+    let delimiter = delimiter.filter(|v| !v.is_empty());
+    let key_marker = key_marker.filter(|v| !v.is_empty());
+    let version_id_marker = version_id_marker.filter(|v| !v.is_empty());
+    let max_keys = max_keys.unwrap_or(1000);
+    if max_keys < 0 {
+        return Err(S3Error::with_message(S3ErrorCode::InvalidArgument, "Invalid max keys".to_string()));
+    }
+
+    Ok((prefix, delimiter, key_marker, version_id_marker, max_keys))
+}
+
+pub(crate) fn build_list_object_versions_output(
+    object_infos: ListObjectVersionsInfo,
+    bucket: String,
+    prefix: String,
+    delimiter: Option<String>,
+    max_keys: i32,
+) -> ListObjectVersionsOutput {
+    let versions: Vec<ObjectVersion> = object_infos
+        .objects
+        .iter()
+        .filter(|v| !v.name.is_empty() && !v.delete_marker)
+        .map(|v| ObjectVersion {
+            key: Some(v.name.to_owned()),
+            last_modified: v.mod_time.map(Timestamp::from),
+            size: Some(v.size),
+            version_id: Some(v.version_id.map(|id| id.to_string()).unwrap_or_else(|| "null".to_string())),
+            is_latest: Some(v.is_latest),
+            e_tag: v.etag.clone().map(|etag| to_s3s_etag(&etag)),
+            storage_class: v.storage_class.clone().map(ObjectVersionStorageClass::from),
+            ..Default::default()
+        })
+        .collect();
+
+    let delete_markers: Vec<DeleteMarkerEntry> = object_infos
+        .objects
+        .iter()
+        .filter(|o| o.delete_marker)
+        .map(|o| DeleteMarkerEntry {
+            key: Some(o.name.to_owned()),
+            version_id: Some(o.version_id.map(|id| id.to_string()).unwrap_or_else(|| "null".to_string())),
+            is_latest: Some(o.is_latest),
+            last_modified: o.mod_time.map(Timestamp::from),
+            ..Default::default()
+        })
+        .collect();
+
+    let common_prefixes: Vec<CommonPrefix> = object_infos
+        .prefixes
+        .into_iter()
+        .map(|v| CommonPrefix { prefix: Some(v) })
+        .collect();
+
+    // Only return markers when they are non-empty to preserve S3 client compatibility.
+    let next_key_marker = object_infos.next_marker.filter(|v| !v.is_empty());
+    let next_version_id_marker = object_infos.next_version_idmarker.filter(|v| !v.is_empty());
+
+    ListObjectVersionsOutput {
+        is_truncated: Some(object_infos.is_truncated),
+        max_keys: Some(max_keys),
+        delimiter,
+        name: Some(bucket),
+        prefix: Some(prefix),
+        common_prefixes: Some(common_prefixes),
+        versions: Some(versions),
+        delete_markers: Some(delete_markers),
+        next_key_marker,
+        next_version_id_marker,
+        ..Default::default()
+    }
+}
+
 #[allow(clippy::too_many_arguments)]
 pub(crate) fn build_list_objects_v2_output(
     object_infos: ListObjectsV2Info,
@@ -168,9 +252,14 @@ fn calculate_next_marker(v2: &ListObjectsV2Output) -> Option<String> {
 
 #[cfg(test)]
 mod tests {
-    use super::{build_list_objects_output, build_list_objects_v2_output};
+    use super::{
-    use rustfs_ecstore::store_api::{ListObjectsV2Info, ObjectInfo};
+        build_list_object_versions_output, build_list_objects_output, build_list_objects_v2_output,
+        parse_list_object_versions_params,
+    };
+    use rustfs_ecstore::store_api::{ListObjectVersionsInfo, ListObjectsV2Info, ObjectInfo};
+    use s3s::S3ErrorCode;
     use s3s::dto::{CommonPrefix, EncodingType, ListObjectsV2Output, Object};
+    use uuid::Uuid;
 
     #[test]
     fn test_list_objects_marker_echoes_request_value() {
@@ -298,6 +387,93 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_parse_list_object_versions_params_defaults_and_filters_empty_values() {
+        let (prefix, delimiter, key_marker, version_id_marker, max_keys) =
+            parse_list_object_versions_params(None, Some(String::new()), Some(String::new()), None, None)
+                .expect("parse should succeed");
+
+        assert_eq!(prefix, String::new());
+        assert_eq!(delimiter, None);
+        assert_eq!(key_marker, None);
+        assert_eq!(version_id_marker, None);
+        assert_eq!(max_keys, 1000);
+    }
+
+    #[test]
+    fn test_parse_list_object_versions_params_rejects_negative_max_keys() {
+        let err = parse_list_object_versions_params(None, None, None, None, Some(-1))
+            .expect_err("negative max_keys should be rejected");
+
+        assert_eq!(*err.code(), S3ErrorCode::InvalidArgument);
+    }
+
+    #[test]
+    fn test_build_list_object_versions_output_maps_versions_delete_markers_and_markers() {
+        let object_infos = ListObjectVersionsInfo {
+            is_truncated: true,
+            next_marker: Some(String::new()),
+            next_version_idmarker: Some("next-version-id".to_string()),
+            objects: vec![
+                ObjectInfo {
+                    name: "obj-a".to_string(),
+                    size: 10,
+                    etag: Some("etag-a".to_string()),
+                    storage_class: Some("STANDARD".to_string()),
+                    version_id: Some(Uuid::nil()),
+                    is_latest: true,
+                    ..Default::default()
+                },
+                ObjectInfo {
+                    name: "obj-delete-marker".to_string(),
+                    delete_marker: true,
+                    is_latest: false,
+                    ..Default::default()
+                },
+                ObjectInfo {
+                    name: String::new(),
+                    delete_marker: false,
+                    ..Default::default()
+                },
+            ],
+            prefixes: vec!["photos/".to_string()],
+        };
+
+        let output = build_list_object_versions_output(
+            object_infos,
+            "bucket-a".to_string(),
+            "prefix-a".to_string(),
+            Some("/".to_string()),
+            123,
+        );
+
+        assert_eq!(output.is_truncated, Some(true));
+        assert_eq!(output.max_keys, Some(123));
+        assert_eq!(output.name, Some("bucket-a".to_string()));
+        assert_eq!(output.prefix, Some("prefix-a".to_string()));
+        assert_eq!(output.delimiter, Some("/".to_string()));
+        assert_eq!(output.next_key_marker, None);
+        assert_eq!(output.next_version_id_marker, Some("next-version-id".to_string()));
+
+        let versions = output.versions.unwrap_or_default();
+        assert_eq!(versions.len(), 1);
+        assert_eq!(versions[0].key, Some("obj-a".to_string()));
+        assert_eq!(versions[0].version_id, Some(Uuid::nil().to_string()));
+
+        let delete_markers = output.delete_markers.unwrap_or_default();
+        assert_eq!(delete_markers.len(), 1);
+        assert_eq!(delete_markers[0].key, Some("obj-delete-marker".to_string()));
+        assert_eq!(delete_markers[0].version_id, Some("null".to_string()));
+
+        let prefixes = output.common_prefixes.unwrap_or_default();
+        assert_eq!(
+            prefixes,
+            vec![CommonPrefix {
+                prefix: Some("photos/".to_string())
+            }]
+        );
+    }
+
     fn object_info(name: &str) -> ObjectInfo {
         ObjectInfo {
             name: name.to_string(),