From e1e3cf462037171fbac2b67026ffc8fde1193645 Mon Sep 17 00:00:00 2001 From: GatewayJ <835269233@qq.com> Date: Tue, 15 Sep 2026 15:33:46 +0800 Subject: [PATCH] fix(s3): render null write version IDs consistently (#7904) --- .../src/replication_extension_test.rs | 9 +-- docs/architecture/s3-compatibility-matrix.md | 2 +- rustfs/src/app/object/copy.rs | 74 ++++++++++++++++++- rustfs/src/app/object/get.rs | 4 +- rustfs/src/app/object/head.rs | 2 +- rustfs/src/app/object/put.rs | 2 +- rustfs/src/app/object/shared.rs | 16 +++- rustfs/src/storage/ecfs.rs | 2 +- rustfs/src/storage/s3_api/mod.rs | 2 +- 9 files changed, 97 insertions(+), 16 deletions(-) diff --git a/crates/e2e_test/src/replication_extension_test.rs b/crates/e2e_test/src/replication_extension_test.rs index 1eddab705..a22c2e399 100644 --- a/crates/e2e_test/src/replication_extension_test.rs +++ b/crates/e2e_test/src/replication_extension_test.rs @@ -3875,11 +3875,10 @@ async fn test_bucket_replication_replicates_directory_marker_in_versioned_bucket .body(ByteStream::from_static(body)) .send() .await?; - assert!( - put.version_id() - .is_none_or(|id| id == "null" || id == uuid::Uuid::nil().to_string()), - "a directory marker is the null version even in a versioned bucket: {:?}", - put.version_id() + assert_eq!( + put.version_id(), + Some("null"), + "a directory marker must expose its null version without leaking the internal nil UUID" ); wait_for_source_replication_status(&source_client, source_bucket, marker_key, "COMPLETED", false).await?; diff --git a/docs/architecture/s3-compatibility-matrix.md b/docs/architecture/s3-compatibility-matrix.md index 05b57c074..83ffc522d 100644 --- a/docs/architecture/s3-compatibility-matrix.md +++ b/docs/architecture/s3-compatibility-matrix.md @@ -104,7 +104,7 @@ Object keys are stored as file-system paths under each drive (`{drive}/{bucket}/ | Behavior | RustFS | AWS S3 | Why | |---|---|---|---| | Object key with a `.` or `..` path segment, or an empty segment (`//`), such as `a//b/./c/../d` | `400 InvalidArgument` (`check_object_args` in `crates/ecstore/src/bucket/utils.rs`, mirroring MinIO `IsValidObjectPrefix`) | Accepted as an opaque key | A `..` segment would resolve to a parent directory and `.`/`//` segments would alias other keys on disk; encoding them would change the MinIO-compatible on-disk format. | -| Directory marker (key ending in `/`, with or without a body) in a versioned bucket | Stored as the null version: `PutObject`/`HeadObject` report version id `00000000-0000-0000-0000-000000000000`, `ListObjectVersions` reports `null`, and a later PUT of the same key overwrites in place (`put_opts` in `rustfs/src/storage/options.rs`, mirroring MinIO `putOpts`: "for directory objects skip creating new versions") | A real version id per PUT, with a version history | The marker only exists to make an empty prefix listable; keeping a history for it would leave hidden versions behind every prefix delete. Replication still copies the marker as its null version (`test_bucket_replication_replicates_directory_marker_in_versioned_bucket` in `crates/e2e_test/src/replication_extension_test.rs`). | +| Directory marker (key ending in `/`, with or without a body) in a versioned bucket | Stored as the null version: `PutObject`, `CopyObject`, `HeadObject`, and `ListObjectVersions` report version id `null`, and a later PUT of the same key overwrites in place (`put_opts` in `rustfs/src/storage/options.rs`, mirroring MinIO `putOpts`: "for directory objects skip creating new versions") | A real version id per PUT, with a version history | The marker only exists to make an empty prefix listable; keeping a history for it would leave hidden versions behind every prefix delete. Replication still copies the marker as its null version (`test_bucket_replication_replicates_directory_marker_in_versioned_bucket` in `crates/e2e_test/src/replication_extension_test.rs`). | ## Update Rule diff --git a/rustfs/src/app/object/copy.rs b/rustfs/src/app/object/copy.rs index 97666465b..614fc1b85 100644 --- a/rustfs/src/app/object/copy.rs +++ b/rustfs/src/app/object/copy.rs @@ -826,7 +826,7 @@ impl DefaultObjectUsecase { S3Error::with_message(S3ErrorCode::InternalError, format!("copy object commit owner task failed: {err}")) })??; - let raw_dest_version = oi.version_id.map(|v| v.to_string()); + let raw_dest_version = s3_response_version_id(oi.version_id); let dest_version = if dest_versioned { raw_dest_version } else { None }; // Echo the source version that was copied via x-amz-copy-source-version-id (issue #4976). @@ -1327,6 +1327,78 @@ mod tests { .expect("expected-current copy test bucket should be removed"); } + #[tokio::test] + #[serial_test::serial] + async fn execute_copy_object_renders_null_directory_destination_version() { + use crate::app::storage_api::test::contract::bucket::{BucketOperations as _, DeleteBucketOptions, MakeBucketOptions}; + + let store = crate::app::gating_test_env::shared_gating_ecstore().await; + if current_app_context().is_none() { + crate::app::runtime_sources::install_test_app_context(Arc::clone(&store)).await; + } + let ambient = current_app_context().expect("directory copy test requires an AppContext"); + let context = Arc::new(AppContext::new(Arc::clone(&store), ambient.iam(), ambient.kms())); + let bucket = format!("copy-null-dir-version-{}", Uuid::new_v4()); + let source = "source.bin"; + let destination = "directory/"; + store + .make_bucket( + &bucket, + &MakeBucketOptions { + versioning_enabled: true, + ..Default::default() + }, + ) + .await + .expect("versioned directory copy test bucket should be created"); + + let mut source_reader = PutObjReader::from_vec(b"copy source".to_vec()); + store + .put_object( + &bucket, + source, + &mut source_reader, + &ObjectOptions { + versioned: true, + ..Default::default() + }, + ) + .await + .expect("copy source should be written"); + + let input = CopyObjectInput::builder() + .copy_source(CopySource::Bucket { + bucket: bucket.clone().into(), + key: source.to_string().into(), + version_id: None, + }) + .bucket(bucket.clone()) + .key(destination.to_string()) + .build() + .expect("directory copy input should build"); + let response = DefaultObjectUsecase::with_context(Some(context)) + .execute_copy_object(build_request(input, Method::PUT)) + .await + .expect("copying to a directory marker should succeed"); + + assert_eq!( + response.output.version_id.as_deref(), + Some(NULL_VERSION_ID), + "CopyObject must expose the null destination version without leaking the internal nil UUID" + ); + + store + .delete_bucket( + &bucket, + &DeleteBucketOptions { + force: true, + ..Default::default() + }, + ) + .await + .expect("directory copy test bucket should be removed"); + } + #[tokio::test] async fn execute_copy_object_allows_self_copy_of_null_version() { // A "null" source version id is a restore of the null version, not a no-op self-copy. diff --git a/rustfs/src/app/object/get.rs b/rustfs/src/app/object/get.rs index 70c85520e..441626d38 100644 --- a/rustfs/src/app/object/get.rs +++ b/rustfs/src/app/object/get.rs @@ -3692,7 +3692,7 @@ impl DefaultObjectUsecase { let checksums = Self::build_get_object_checksums(&info, &req.headers, part_number, rs.as_ref())?; record_get_object_s3_handler_stage_duration(GET_OBJECT_STAGE_CHECKSUM_HEADERS, checksum_headers_start); - let output_version_id = read_response_version_id(info.version_id); + let output_version_id = s3_response_version_id(info.version_id); // x-amz-restore: extract from object metadata let restore = info.user_defined.get(X_AMZ_RESTORE.as_str()).and_then(|v| { @@ -4430,7 +4430,7 @@ impl DefaultObjectUsecase { None }; - let version_id = read_response_version_id(info.version_id); + let version_id = s3_response_version_id(info.version_id); let output = GetObjectAttributesOutput { checksum, diff --git a/rustfs/src/app/object/head.rs b/rustfs/src/app/object/head.rs index ae208e20e..81bde22fe 100644 --- a/rustfs/src/app/object/head.rs +++ b/rustfs/src/app/object/head.rs @@ -553,7 +553,7 @@ impl DefaultObjectUsecase { last_modified, e_tag: info.etag.map(|etag| to_s3s_etag(&etag)), metadata: filter_object_metadata(&metadata_map), - version_id: read_response_version_id(info.version_id), + version_id: s3_response_version_id(info.version_id), server_side_encryption, sse_customer_algorithm, sse_customer_key_md5, diff --git a/rustfs/src/app/object/put.rs b/rustfs/src/app/object/put.rs index c74c3289a..c279862fb 100644 --- a/rustfs/src/app/object/put.rs +++ b/rustfs/src/app/object/put.rs @@ -1356,7 +1356,7 @@ impl DefaultObjectUsecase { }; let committed = self.put_object_core(write, body, start_time).await?; - let raw_version = committed.obj_info.version_id.map(|v| v.to_string()); + let raw_version = s3_response_version_id(committed.obj_info.version_id); let put_version = if committed.put_versioned { raw_version } else { None }; let e_tag = committed.obj_info.etag.clone().map(|etag| to_s3s_etag(&etag)); diff --git a/rustfs/src/app/object/shared.rs b/rustfs/src/app/object/shared.rs index 9e1265ace..62c7095ec 100644 --- a/rustfs/src/app/object/shared.rs +++ b/rustfs/src/app/object/shared.rs @@ -32,9 +32,10 @@ pub(super) const LOG_COMPONENT_APP: &str = "app"; pub(super) const LOG_SUBSYSTEM_OBJECT: &str = "object"; -/// Encode the resolved local read identity. Storage distinguishes a null -/// version from an unversioned object by returning a nil UUID instead of None. -pub(crate) fn read_response_version_id(version_id: Option) -> Option { +/// Encode a resolved object version identity for an S3 response. Storage +/// distinguishes a null version from an unversioned object by returning a nil +/// UUID instead of None. +pub(crate) fn s3_response_version_id(version_id: Option) -> Option { version_id.map(|id| { if id.is_nil() { NULL_VERSION_ID.to_owned() @@ -1114,6 +1115,15 @@ mod tests { }; use std::sync::Arc; + #[test] + fn s3_response_version_id_distinguishes_absent_null_and_uuid_versions() { + let version = Uuid::parse_str("9341ae04-d4ce-468c-a4e1-6501d58cd6b7").unwrap(); + + assert_eq!(s3_response_version_id(None), None); + assert_eq!(s3_response_version_id(Some(Uuid::nil())).as_deref(), Some(NULL_VERSION_ID)); + assert_eq!(s3_response_version_id(Some(version)), Some(version.to_string())); + } + #[test] fn delete_marker_read_headers_round_trip_uuid_and_null_errors() { let uuid = Uuid::parse_str("9341ae04-d4ce-468c-a4e1-6501d58cd6b7").unwrap(); diff --git a/rustfs/src/storage/ecfs.rs b/rustfs/src/storage/ecfs.rs index 0a940a178..170b74de6 100644 --- a/rustfs/src/storage/ecfs.rs +++ b/rustfs/src/storage/ecfs.rs @@ -1233,7 +1233,7 @@ impl S3 for FS { histogram!("rustfs_object_tagging_operation_duration_seconds", "operation" => "get").record(duration.as_secs_f64()); Ok(S3Response::new(GetObjectTaggingOutput { tag_set, - version_id: s3_api::read_response_version_id(info.version_id), + version_id: s3_api::s3_response_version_id(info.version_id), })) } diff --git a/rustfs/src/storage/s3_api/mod.rs b/rustfs/src/storage/s3_api/mod.rs index f29f88e68..672320cbc 100644 --- a/rustfs/src/storage/s3_api/mod.rs +++ b/rustfs/src/storage/s3_api/mod.rs @@ -17,7 +17,7 @@ //! This file intentionally starts as skeleton-only. Behavior remains in place //! until each helper is moved with dedicated small refactor steps. -pub(crate) use crate::app::object::read_response_version_id; +pub(crate) use crate::app::object::s3_response_version_id; use crate::app::{ bucket_usecase::DefaultBucketUsecase, multipart_usecase::DefaultMultipartUsecase, object_usecase::DefaultObjectUsecase, };