From 2fadb16365168c5b98bd3463f578ed940a296bef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Sun, 15 Feb 2026 12:32:26 +0800 Subject: [PATCH] refactor(storage): extract list object versions helpers (#1830) --- rustfs/src/storage/ecfs.rs | 70 +---------- rustfs/src/storage/s3_api/bucket.rs | 184 +++++++++++++++++++++++++++- 2 files changed, 186 insertions(+), 68 deletions(-) diff --git a/rustfs/src/storage/ecfs.rs b/rustfs/src/storage/ecfs.rs index c99b74b9b..4ad210768 100644 --- a/rustfs/src/storage/ecfs.rs +++ b/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 max_keys = max_keys.unwrap_or(1000); - - 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 (prefix, delimiter, key_marker, version_id_marker, max_keys) = + parse_list_object_versions_params(prefix, delimiter, key_marker, version_id_marker, max_keys)?; let store = get_validated_store(&bucket).await?; @@ -3611,63 +3609,7 @@ impl S3 for FS { .await .map_err(ApiError::from)?; - let objects: Vec = 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(|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::>(); - - // 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() - }; + let output = build_list_object_versions_output(object_infos, bucket, prefix, delimiter, max_keys); Ok(s3_response(output)) } diff --git a/rustfs/src/storage/s3_api/bucket.rs b/rustfs/src/storage/s3_api/bucket.rs index ba73776c2..e9bcf86f5 100644 --- a/rustfs/src/storage/s3_api/bucket.rs +++ b/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, Option, Option, i32); + pub(crate) fn build_list_objects_output(v2: ListObjectsV2Output, request_marker: Option) -> 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, + delimiter: Option, + key_marker: Option, + version_id_marker: Option, + max_keys: Option, +) -> Result { + 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, + max_keys: i32, +) -> ListObjectVersionsOutput { + let versions: Vec = 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 = 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 = 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 { #[cfg(test)] mod tests { - use super::{build_list_objects_output, build_list_objects_v2_output}; - use rustfs_ecstore::store_api::{ListObjectsV2Info, ObjectInfo}; + use super::{ + 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(),