fix(s3): render null write version IDs consistently (#7904)

This commit is contained in:
GatewayJ
2026-09-15 15:33:46 +08:00
committed by GitHub
parent 3999544ece
commit e1e3cf4620
9 changed files with 97 additions and 16 deletions
@@ -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?;
+1 -1
View File
@@ -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
+73 -1
View File
@@ -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.
+2 -2
View File
@@ -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,
+1 -1
View File
@@ -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,
+1 -1
View File
@@ -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));
+13 -3
View File
@@ -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<Uuid>) -> Option<String> {
/// 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<Uuid>) -> Option<String> {
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();
+1 -1
View File
@@ -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),
}))
}
+1 -1
View File
@@ -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,
};