From be14d50ca7d446bad40381a971459db3e2f73348 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Tue, 7 Jan 2025 21:48:43 +0800 Subject: [PATCH 1/2] fix(services/s3): List with deleted should contain latest Signed-off-by: Xuanwo --- core/src/services/s3/lister.rs | 41 +++++++++++++++++-------------- core/tests/behavior/async_list.rs | 11 ++++++++- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/core/src/services/s3/lister.rs b/core/src/services/s3/lister.rs index dd4359499d2..d0f56f42c9e 100644 --- a/core/src/services/s3/lister.rs +++ b/core/src/services/s3/lister.rs @@ -222,28 +222,31 @@ impl oio::PageList for S3ObjectVersionsLister { ctx.entries.push_back(de); } - if self.args.versions() { - for version_object in output.version { - let mut path = build_rel_path(&self.core.root, &version_object.key); - if path.is_empty() { - path = "/".to_owned(); - } + for version_object in output.version { + // Skip if users are neither requesting all versions, nor the object is the latest version. + if !(self.args.versions() || version_object.is_latest) { + continue; + } - let mut meta = Metadata::new(EntryMode::from_path(&path)); - meta.set_version(&version_object.version_id); - meta.set_is_current(version_object.is_latest); - meta.set_content_length(version_object.size); - meta.set_last_modified(parse_datetime_from_rfc3339( - version_object.last_modified.as_str(), - )?); - if let Some(etag) = version_object.etag { - meta.set_etag(&etag); - meta.set_content_md5(etag.trim_matches('"')); - } + let mut path = build_rel_path(&self.core.root, &version_object.key); + if path.is_empty() { + path = "/".to_owned(); + } - let entry = oio::Entry::new(&path, meta); - ctx.entries.push_back(entry); + let mut meta = Metadata::new(EntryMode::from_path(&path)); + meta.set_version(&version_object.version_id); + meta.set_is_current(version_object.is_latest); + meta.set_content_length(version_object.size); + meta.set_last_modified(parse_datetime_from_rfc3339( + version_object.last_modified.as_str(), + )?); + if let Some(etag) = version_object.etag { + meta.set_etag(&etag); + meta.set_content_md5(etag.trim_matches('"')); } + + let entry = oio::Entry::new(&path, meta); + ctx.entries.push_back(entry); } if self.args.deleted() { diff --git a/core/tests/behavior/async_list.rs b/core/tests/behavior/async_list.rs index 3e0822d8ab6..506a7fab2a9 100644 --- a/core/tests/behavior/async_list.rs +++ b/core/tests/behavior/async_list.rs @@ -610,10 +610,19 @@ pub async fn test_list_files_with_deleted(op: Operator) -> Result<()> { let file_name = TEST_FIXTURE.new_file_path(); let file_path = format!("{}{}", parent, file_name); op.write(file_path.as_str(), "1").await?; + + // List with deleted should include self too. + let ds = op.list_with(&file_path).deleted(true).await?; + assert_eq!( + ds.len(), + 1, + "list with deleted should current active and only have one" + ); + op.write(file_path.as_str(), "2").await?; op.delete(file_path.as_str()).await?; - // This file has been deleted + // This file has been deleted, list with deleted should contain its versions and delete marker. let mut ds = op.list_with(&file_path).deleted(true).await?; ds.retain(|de| de.path() == file_path && de.metadata().is_deleted()); From 03785be374e88289589afb274c04e3704847f42b Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Tue, 7 Jan 2025 22:10:14 +0800 Subject: [PATCH 2/2] Address comments Signed-off-by: Xuanwo --- core/src/services/s3/lister.rs | 5 ++++- core/tests/behavior/async_list.rs | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/core/src/services/s3/lister.rs b/core/src/services/s3/lister.rs index d0f56f42c9e..f280e10e40f 100644 --- a/core/src/services/s3/lister.rs +++ b/core/src/services/s3/lister.rs @@ -223,7 +223,10 @@ impl oio::PageList for S3ObjectVersionsLister { } for version_object in output.version { - // Skip if users are neither requesting all versions, nor the object is the latest version. + // `list` must be additive, so we need to include the latest version object + // even if `versions` is not enabled. + // + // Here we skip all non-latest version objects if `versions` is not enabled. if !(self.args.versions() || version_object.is_latest) { continue; } diff --git a/core/tests/behavior/async_list.rs b/core/tests/behavior/async_list.rs index 506a7fab2a9..e59c07af79f 100644 --- a/core/tests/behavior/async_list.rs +++ b/core/tests/behavior/async_list.rs @@ -616,7 +616,7 @@ pub async fn test_list_files_with_deleted(op: Operator) -> Result<()> { assert_eq!( ds.len(), 1, - "list with deleted should current active and only have one" + "list with deleted should contain current active file version" ); op.write(file_path.as_str(), "2").await?;