From f707444ffff0f2338e44b7f41b71e9831ce21854 Mon Sep 17 00:00:00 2001 From: Henk-Jan Lebbink Date: Thu, 10 Jul 2025 13:14:10 +0200 Subject: [PATCH 1/3] Tests cleanup; cargo clippy fixes, minor doc updates --- Cargo.toml | 3 +- common/Cargo.toml | 1 + common/src/utils.rs | 18 +++- src/s3/builders/delete_objects.rs | 2 +- src/s3/client/delete_bucket.rs | 6 +- src/s3/client/delete_objects.rs | 2 +- src/s3/lifecycle_config.rs | 2 +- src/s3/response/get_bucket_policy.rs | 2 +- src/s3/response/get_object_prompt.rs | 2 +- src/s3/response/list_objects.rs | 14 +-- src/s3/segmented_bytes.rs | 2 +- src/s3/utils.rs | 39 +++++--- tests/test_bucket_create_delete.rs | 45 ++++++++- tests/test_object_delete.rs | 133 +++++++++++++++++++++++++++ tests/test_object_put.rs | 66 ++++--------- tests/test_object_remove.rs | 62 ------------- 16 files changed, 243 insertions(+), 156 deletions(-) create mode 100644 tests/test_object_delete.rs delete mode 100644 tests/test_object_remove.rs diff --git a/Cargo.toml b/Cargo.toml index a333ba3b..0e8335a7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,9 +41,10 @@ hmac = { version = "0.12.1", optional = true } hyper = { version = "1.6.0", features = ["full"] } lazy_static = "1.5.0" log = "0.4.27" -md5 = "0.7.0" +md5 = "0.8.0" multimap = "0.10.1" percent-encoding = "2.3.1" +url = "2.5.4" rand = { version = "0.8.5", features = ["small_rng"] } regex = "1.11.1" ring = { version = "0.17.14", optional = true, default-features = false, features = ["alloc"] } diff --git a/common/Cargo.toml b/common/Cargo.toml index 0cec1880..e5298a7e 100644 --- a/common/Cargo.toml +++ b/common/Cargo.toml @@ -14,6 +14,7 @@ chrono = "0.4.41" reqwest = "0.12.20" http = "1.3.1" futures = "0.3.31" +uuid = { version = "1.17.0", features = ["v4"] } [lib] name = "minio_common" diff --git a/common/src/utils.rs b/common/src/utils.rs index 3759bb23..cbc9debe 100644 --- a/common/src/utils.rs +++ b/common/src/utils.rs @@ -15,16 +15,24 @@ use http::{Response as HttpResponse, StatusCode}; use minio::s3::error::Error; -use rand::distributions::{Alphanumeric, DistString}; +use rand::distributions::Standard; +use rand::{Rng, thread_rng}; +use uuid::Uuid; pub fn rand_bucket_name() -> String { - Alphanumeric - .sample_string(&mut rand::thread_rng(), 8) - .to_lowercase() + format!("test-bucket-{}", Uuid::new_v4()) } pub fn rand_object_name() -> String { - Alphanumeric.sample_string(&mut rand::thread_rng(), 20) + format!("test-object-{}", Uuid::new_v4()) +} + +pub fn rand_object_name_utf8(len: usize) -> String { + let rng = thread_rng(); + rng.sample_iter::(Standard) + .filter(|c| !c.is_control()) + .take(len) + .collect() } pub async fn get_bytes_from_response(v: Result) -> bytes::Bytes { diff --git a/src/s3/builders/delete_objects.rs b/src/s3/builders/delete_objects.rs index bebc86e1..dd662b73 100644 --- a/src/s3/builders/delete_objects.rs +++ b/src/s3/builders/delete_objects.rs @@ -209,7 +209,7 @@ impl DeleteObjects { } /// Enable verbose mode (defaults to false). If enabled, the response will - /// include the keys of objects that were successfully deleted. Otherwise + /// include the keys of objects that were successfully deleted. Otherwise, /// only objects that encountered an error are returned. pub fn verbose_mode(mut self, verbose_mode: bool) -> Self { self.verbose_mode = verbose_mode; diff --git a/src/s3/client/delete_bucket.rs b/src/s3/client/delete_bucket.rs index c83af3f1..a2bcf41b 100644 --- a/src/s3/client/delete_bucket.rs +++ b/src/s3/client/delete_bucket.rs @@ -61,11 +61,9 @@ impl Client { let mut stream = self.list_objects(&bucket).to_stream().await; while let Some(items) = stream.next().await { + let object_names = items?.contents.into_iter().map(ObjectToDelete::from); let mut resp = self - .delete_objects_streaming( - &bucket, - items?.contents.into_iter().map(ObjectToDelete::from), - ) + .delete_objects_streaming(&bucket, object_names) .to_stream() .await; while let Some(item) = resp.next().await { diff --git a/src/s3/client/delete_objects.rs b/src/s3/client/delete_objects.rs index 51079ac8..7d6f421b 100644 --- a/src/s3/client/delete_objects.rs +++ b/src/s3/client/delete_objects.rs @@ -66,7 +66,7 @@ impl Client { /// Creates a [`DeleteObjectsStreaming`] request builder to delete a stream of objects from an S3 bucket. /// - /// to execute the request, call [`DeleteObjectsStreaming::to_stream()`](crate::s3::types::S3Api::send), + /// To execute the request, call [`DeleteObjectsStreaming::to_stream()`](crate::s3::types::S3Api::send), /// which returns a [`Result`] containing a [`DeleteObjectsResponse`](crate::s3::response::DeleteObjectsResponse). pub fn delete_objects_streaming, D: Into>( &self, diff --git a/src/s3/lifecycle_config.rs b/src/s3/lifecycle_config.rs index 8f358c7b..7443b7bb 100644 --- a/src/s3/lifecycle_config.rs +++ b/src/s3/lifecycle_config.rs @@ -480,5 +480,5 @@ impl LifecycleRule { fn parse_iso8601(date_str: &str) -> Result, Error> { chrono::DateTime::parse_from_rfc3339(date_str) .map(|dt| dt.with_timezone(&chrono::Utc)) - .map_err(|_| Error::XmlError(format!("Invalid date format: {}", date_str))) + .map_err(|_| Error::XmlError(format!("Invalid date format: {date_str}"))) } diff --git a/src/s3/response/get_bucket_policy.rs b/src/s3/response/get_bucket_policy.rs index 6f9bf348..2c1bd8d6 100644 --- a/src/s3/response/get_bucket_policy.rs +++ b/src/s3/response/get_bucket_policy.rs @@ -48,7 +48,7 @@ impl GetBucketPolicyResponse { /// for accessing the bucket and its contents. pub fn config(&self) -> Result<&str, Error> { std::str::from_utf8(&self.body).map_err(|e| { - Error::Utf8Error(format!("Failed to parse bucket policy as UTF-8: {}", e).into()) + Error::Utf8Error(format!("Failed to parse bucket policy as UTF-8: {e}").into()) }) } } diff --git a/src/s3/response/get_object_prompt.rs b/src/s3/response/get_object_prompt.rs index 9b7a370c..387d54f7 100644 --- a/src/s3/response/get_object_prompt.rs +++ b/src/s3/response/get_object_prompt.rs @@ -40,7 +40,7 @@ impl GetObjectPromptResponse { /// This method retrieves the content of the object as a UTF-8 encoded string. pub fn prompt_response(&self) -> Result<&str, Error> { std::str::from_utf8(&self.body).map_err(|e| { - Error::Utf8Error(format!("Failed to parse prompt_response as UTF-8: {}", e).into()) + Error::Utf8Error(format!("Failed to parse prompt_response as UTF-8: {e}").into()) }) } } diff --git a/src/s3/response/list_objects.rs b/src/s3/response/list_objects.rs index 49789194..78c59b56 100644 --- a/src/s3/response/list_objects.rs +++ b/src/s3/response/list_objects.rs @@ -10,18 +10,12 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! Response types for ListObjects APIs - use crate::impl_has_s3fields; +use crate::s3::error::Error; use crate::s3::response::a_response_traits::HasS3Fields; -use crate::s3::{ - error::Error, - types::{FromS3Response, ListEntry, S3Request}, - utils::{ - from_iso8601utc, parse_tags, urldecode, - xml::{Element, MergeXmlElements}, - }, -}; +use crate::s3::types::{FromS3Response, ListEntry, S3Request}; +use crate::s3::utils::xml::{Element, MergeXmlElements}; +use crate::s3::utils::{from_iso8601utc, parse_tags, urldecode}; use async_trait::async_trait; use bytes::{Buf, Bytes}; use reqwest::header::HeaderMap; diff --git a/src/s3/segmented_bytes.rs b/src/s3/segmented_bytes.rs index 1d548ee9..0f0dfc1c 100644 --- a/src/s3/segmented_bytes.rs +++ b/src/s3/segmented_bytes.rs @@ -80,7 +80,7 @@ impl SegmentedBytes { impl fmt::Display for SegmentedBytes { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match std::str::from_utf8(self.to_bytes().as_ref()) { - Ok(s) => write!(f, "{}", s), + Ok(s) => write!(f, "{s}"), Err(_) => Ok(()), // or: write!(f, "") } } diff --git a/src/s3/utils.rs b/src/s3/utils.rs index 2af03d5a..420775ae 100644 --- a/src/s3/utils.rs +++ b/src/s3/utils.rs @@ -255,14 +255,14 @@ pub fn check_bucket_name(bucket_name: impl AsRef, strict: bool) -> Result<( )); } if bucket_name_len < 3 { - return Err(Error::InvalidBucketName( - "bucket name cannot be less than 3 characters".into(), - )); + return Err(Error::InvalidBucketName(format!( + "bucket name ('{bucket_name}') cannot be less than 3 characters" + ))); } if bucket_name_len > 63 { - return Err(Error::InvalidBucketName( - "Bucket name cannot be greater than 63 characters".into(), - )); + return Err(Error::InvalidBucketName(format!( + "Bucket name ('{bucket_name}') cannot be greater than 63 characters" + ))); } lazy_static! { @@ -274,8 +274,8 @@ pub fn check_bucket_name(bucket_name: impl AsRef, strict: bool) -> Result<( } if IPV4_REGEX.is_match(bucket_name) { - return Err(Error::InvalidBucketName(String::from( - "bucket name cannot be an IP address", + return Err(Error::InvalidBucketName(format!( + "bucket name ('{bucket_name}') cannot be an IP address" ))); } @@ -288,12 +288,14 @@ pub fn check_bucket_name(bucket_name: impl AsRef, strict: bool) -> Result<( if strict { if !VALID_BUCKET_NAME_STRICT_REGEX.is_match(bucket_name) { return Err(Error::InvalidBucketName(format!( - "bucket name ('{bucket_name}') does not follow S3 standards strictly", + "bucket name ('{bucket_name}') does not follow S3 standards strictly, according to {}", + *VALID_BUCKET_NAME_STRICT_REGEX ))); } } else if !VALID_BUCKET_NAME_REGEX.is_match(bucket_name) { return Err(Error::InvalidBucketName(format!( - "bucket name ('{bucket_name}') does not follow S3 standards" + "bucket name ('{bucket_name}') does not follow S3 standards, according to {}", + *VALID_BUCKET_NAME_REGEX ))); } @@ -301,13 +303,20 @@ pub fn check_bucket_name(bucket_name: impl AsRef, strict: bool) -> Result<( } pub fn check_object_name(object_name: impl AsRef) -> Result<(), Error> { - if object_name.as_ref().is_empty() { - Err(Error::InvalidObjectName( + let object_name: &str = object_name.as_ref(); + let bucket_name_n_bytes = object_name.len(); + if bucket_name_n_bytes == 0 { + return Err(Error::InvalidObjectName( "object name cannot be empty".into(), - )) - } else { - Ok(()) + )); } + if bucket_name_n_bytes > 1024 { + return Err(Error::InvalidObjectName(format!( + "Bucket name ('{object_name}') cannot be greater than 1024 bytes" + ))); + } + + Ok(()) } /// Gets text value of given XML element for given tag. diff --git a/tests/test_bucket_create_delete.rs b/tests/test_bucket_create_delete.rs index 4e40058e..6eb4f3e4 100644 --- a/tests/test_bucket_create_delete.rs +++ b/tests/test_bucket_create_delete.rs @@ -15,11 +15,13 @@ use minio::s3::client::DEFAULT_REGION; use minio::s3::error::{Error, ErrorCode}; -use minio::s3::response::a_response_traits::{HasBucket, HasRegion}; -use minio::s3::response::{BucketExistsResponse, CreateBucketResponse, DeleteBucketResponse}; +use minio::s3::response::a_response_traits::{HasBucket, HasObject, HasRegion}; +use minio::s3::response::{ + BucketExistsResponse, CreateBucketResponse, DeleteBucketResponse, PutObjectContentResponse, +}; use minio::s3::types::S3Api; use minio_common::test_context::TestContext; -use minio_common::utils::rand_bucket_name; +use minio_common::utils::{rand_bucket_name, rand_object_name}; #[minio_macros::test(no_bucket)] async fn bucket_create(ctx: TestContext) { @@ -85,3 +87,40 @@ async fn bucket_delete(ctx: TestContext) { assert_eq!(resp.bucket(), bucket_name); assert_eq!(resp.region(), ""); } + +#[minio_macros::test(no_bucket)] +async fn bucket_delete_and_purge_1(ctx: TestContext) { + let bucket_name = rand_bucket_name(); + + // create a new bucket + let resp: CreateBucketResponse = ctx.client.create_bucket(&bucket_name).send().await.unwrap(); + assert_eq!(resp.bucket(), bucket_name); + assert_eq!(resp.region(), DEFAULT_REGION); + + // add some objects to the bucket + for _ in 0..5 { + let object_name = rand_object_name(); + let resp: PutObjectContentResponse = ctx + .client + .put_object_content(&bucket_name, &object_name, "Hello, World!") + .send() + .await + .unwrap(); + assert_eq!(resp.bucket(), bucket_name); + assert_eq!(resp.object(), object_name); + } + + // try to remove the bucket without purging, this should fail because the bucket is not empty + let resp: Result = + ctx.client.delete_bucket(&bucket_name).send().await; + + assert!(resp.is_err()); + + // try to remove the bucket with purging, this should succeed + let resp: DeleteBucketResponse = ctx + .client + .delete_and_purge_bucket(&bucket_name) + .await + .unwrap(); + assert_eq!(resp.bucket(), bucket_name); +} diff --git a/tests/test_object_delete.rs b/tests/test_object_delete.rs new file mode 100644 index 00000000..5aadcf22 --- /dev/null +++ b/tests/test_object_delete.rs @@ -0,0 +1,133 @@ +// MinIO Rust Library for Amazon S3 Compatible Cloud Storage +// Copyright 2025 MinIO, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use async_std::stream::StreamExt; +use minio::s3::builders::ObjectToDelete; +use minio::s3::response::a_response_traits::{HasBucket, HasObject}; +use minio::s3::response::{ + DeleteObjectResponse, DeleteObjectsResponse, DeleteResult, PutObjectContentResponse, +}; +use minio::s3::types::{S3Api, ToStream}; +use minio_common::test_context::TestContext; +use minio_common::utils::rand_object_name_utf8; + +async fn create_object( + ctx: &TestContext, + bucket_name: &str, + object_name: &str, +) -> PutObjectContentResponse { + let resp: PutObjectContentResponse = ctx + .client + .put_object_content(bucket_name, object_name, "hello world") + .send() + .await + .unwrap(); + assert_eq!(resp.bucket(), bucket_name); + assert_eq!(resp.object(), object_name); + resp +} + +#[minio_macros::test] +async fn delete_object(ctx: TestContext, bucket_name: String) { + let object_name = rand_object_name_utf8(20); + let _resp = create_object(&ctx, &bucket_name, &object_name).await; + + let resp: DeleteObjectResponse = ctx + .client + .delete_object(&bucket_name, &object_name) + .send() + .await + .unwrap(); + + assert_eq!(resp.bucket(), bucket_name); +} + +#[minio_macros::test] +async fn delete_object_with_whitespace(ctx: TestContext, bucket_name: String) { + let object_name = format!(" {}", rand_object_name_utf8(20)); + let _resp = create_object(&ctx, &bucket_name, &object_name).await; + + let resp: DeleteObjectResponse = ctx + .client + .delete_object(&bucket_name, &object_name) + .send() + .await + .unwrap(); + + assert_eq!(resp.bucket(), bucket_name); +} + +#[minio_macros::test] +async fn delete_objects(ctx: TestContext, bucket_name: String) { + const OBJECT_COUNT: usize = 3; + let mut names: Vec = Vec::new(); + for _ in 1..=OBJECT_COUNT { + let object_name = rand_object_name_utf8(20); + let _resp = create_object(&ctx, &bucket_name, &object_name).await; + names.push(object_name); + } + let del_items: Vec = names + .iter() + .map(|v| ObjectToDelete::from(v.as_str())) + .collect(); + + let resp: DeleteObjectsResponse = ctx + .client + .delete_objects::<&String, ObjectToDelete>(&bucket_name, del_items) + .verbose_mode(true) // Enable verbose mode to get detailed response + .send() + .await + .unwrap(); + + let deleted_names: Vec = resp.result().unwrap(); + assert_eq!(deleted_names.len(), OBJECT_COUNT); + for obj in deleted_names.iter() { + assert!(obj.is_deleted()); + } +} + +#[minio_macros::test] +async fn delete_objects_streaming(ctx: TestContext, bucket_name: String) { + const OBJECT_COUNT: usize = 3; + let mut names: Vec = Vec::new(); + for _ in 1..=OBJECT_COUNT { + let object_name = rand_object_name_utf8(20); + let _resp = create_object(&ctx, &bucket_name, &object_name).await; + names.push(object_name); + } + let del_items: Vec = names + .iter() + .map(|v| ObjectToDelete::from(v.as_str())) + .collect(); + + let mut resp = ctx + .client + .delete_objects_streaming(&bucket_name, del_items.into_iter()) + .verbose_mode(true) + .to_stream() + .await; + + let mut del_count = 0; + while let Some(item) = resp.next().await { + let res = item.unwrap(); + let del_result = res.result().unwrap(); + del_count += del_result.len(); + + for obj in del_result.into_iter() { + assert!(obj.is_deleted()); + } + } + assert_eq!(del_count, 3); +} diff --git a/tests/test_object_put.rs b/tests/test_object_put.rs index 83dfb0c6..a0d36d27 100644 --- a/tests/test_object_put.rs +++ b/tests/test_object_put.rs @@ -15,9 +15,8 @@ use http::header; use minio::s3::builders::{MIN_PART_SIZE, ObjectContent}; -use minio::s3::error::{Error, ErrorCode}; use minio::s3::response::a_response_traits::{ - HasBucket, HasEtagFromHeaders, HasIsDeleteMarker, HasObject, HasS3Fields, HasVersion, + HasBucket, HasEtagFromHeaders, HasIsDeleteMarker, HasObject, HasS3Fields, }; use minio::s3::response::{DeleteObjectResponse, PutObjectContentResponse, StatObjectResponse}; use minio::s3::types::S3Api; @@ -26,16 +25,13 @@ use minio_common::test_context::TestContext; use minio_common::utils::rand_object_name; use tokio::sync::mpsc; -#[minio_macros::test] -async fn put_object(ctx: TestContext, bucket_name: String) { - let object_name: String = rand_object_name(); - +async fn test_put_object(ctx: &TestContext, bucket_name: &str, object_name: &str) { let size = 16_u64; let resp: PutObjectContentResponse = ctx .client .put_object_content( - &bucket_name, - &object_name, + bucket_name, + object_name, ObjectContent::new_from_stream(RandSrc::new(size), Some(size)), ) .send() @@ -48,35 +44,26 @@ async fn put_object(ctx: TestContext, bucket_name: String) { let resp: StatObjectResponse = ctx .client - .stat_object(&bucket_name, &object_name) + .stat_object(bucket_name, object_name) .send() .await .unwrap(); assert_eq!(resp.bucket(), bucket_name); assert_eq!(resp.object(), object_name); assert_eq!(resp.size().unwrap(), size); +} - let resp: DeleteObjectResponse = ctx - .client - .delete_object(&bucket_name, &object_name) - .send() - .await - .unwrap(); - assert!(resp.version_id().is_none()); - - // Validate delete succeeded. - let resp: Result = ctx - .client - .stat_object(&bucket_name, &object_name) - .send() - .await; +/// Test putting an object into a bucket and verifying its existence. +#[minio_macros::test] +async fn put_object_1(ctx: TestContext, bucket_name: String) { + test_put_object(&ctx, &bucket_name, &rand_object_name()).await; +} - match resp.err().unwrap() { - Error::S3Error(er) => { - assert_eq!(er.code, ErrorCode::NoSuchKey) - } - e => panic!("Unexpected error {:?}", e), - } +/// Test putting an object with a name that contains special characters. +#[minio_macros::test] +async fn put_object_2(ctx: TestContext, bucket_name: String) { + test_put_object(&ctx, &bucket_name, "name with+spaces").await; + test_put_object(&ctx, &bucket_name, "name%20with%2Bspaces").await; } #[minio_macros::test] @@ -108,14 +95,6 @@ async fn put_object_multipart(ctx: TestContext, bucket_name: String) { assert_eq!(resp.bucket(), bucket_name); assert_eq!(resp.object(), object_name); assert_eq!(resp.size().unwrap(), size); - - let resp: DeleteObjectResponse = ctx - .client - .delete_object(&bucket_name, &object_name) - .send() - .await - .unwrap(); - assert_eq!(resp.version_id(), None); } #[minio_macros::test] @@ -192,14 +171,6 @@ async fn put_object_content_2(ctx: TestContext, bucket_name: String) { .unwrap(); assert_eq!(resp.size().unwrap(), *size); assert_eq!(resp.etag().unwrap(), etag); - - let resp: DeleteObjectResponse = ctx - .client - .delete_object(&bucket_name, &object_name) - .send() - .await - .unwrap(); - assert_eq!(resp.version_id(), None); } } @@ -247,11 +218,6 @@ async fn put_object_content_3(ctx: TestContext, bucket_name: String) { .unwrap(); assert_eq!(resp.size().unwrap(), sizes[idx]); assert_eq!(resp.etag().unwrap(), etag); - client - .delete_object(&test_bucket, &object_name) - .send() - .await - .unwrap(); idx += 1; } diff --git a/tests/test_object_remove.rs b/tests/test_object_remove.rs deleted file mode 100644 index 8f2cc109..00000000 --- a/tests/test_object_remove.rs +++ /dev/null @@ -1,62 +0,0 @@ -// MinIO Rust Library for Amazon S3 Compatible Cloud Storage -// Copyright 2025 MinIO, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -use async_std::stream::StreamExt; -use minio::s3::builders::ObjectToDelete; -use minio::s3::response::PutObjectContentResponse; -use minio::s3::response::a_response_traits::{HasBucket, HasObject}; -use minio::s3::types::ToStream; -use minio_common::test_context::TestContext; -use minio_common::utils::rand_object_name; - -#[minio_macros::test] -async fn remove_objects(ctx: TestContext, bucket_name: String) { - let mut names: Vec = Vec::new(); - for _ in 1..=3 { - let object_name = rand_object_name(); - let resp: PutObjectContentResponse = ctx - .client - .put_object_content(&bucket_name, &object_name, "") - .send() - .await - .unwrap(); - assert_eq!(resp.bucket(), bucket_name); - assert_eq!(resp.object(), object_name); - names.push(object_name); - } - let del_items: Vec = names - .iter() - .map(|v| ObjectToDelete::from(v.as_str())) - .collect(); - - let mut resp = ctx - .client - .delete_objects_streaming(&bucket_name, del_items.into_iter()) - .verbose_mode(true) - .to_stream() - .await; - - let mut del_count = 0; - while let Some(item) = resp.next().await { - let res = item.unwrap(); - let del_result = res.result().unwrap(); - del_count += del_result.len(); - - for obj in del_result.into_iter() { - assert!(obj.is_deleted()); - } - } - assert_eq!(del_count, 3); -} From b88fb35ccaa9f1d20e932c29ed87def10dc10ed5 Mon Sep 17 00:00:00 2001 From: Henk-Jan Lebbink Date: Thu, 10 Jul 2025 14:12:58 +0200 Subject: [PATCH 2/3] updated label checker workflow --- .github/workflows/label-checker.yaml | 36 ++++++++++----------- src/s3/client/delete_bucket.rs | 47 +++++++++++++++++++--------- tests/test_list_buckets.rs | 7 +---- tests/test_list_objects.rs | 18 +++++------ 4 files changed, 59 insertions(+), 49 deletions(-) diff --git a/.github/workflows/label-checker.yaml b/.github/workflows/label-checker.yaml index 129b8c3f..42bf8468 100644 --- a/.github/workflows/label-checker.yaml +++ b/.github/workflows/label-checker.yaml @@ -1,19 +1,17 @@ -name: Label Checker -on: - pull_request: - types: - - opened - - synchronize - - labeled - - unlabeled - -jobs: - - check_labels: - name: Check for labels - runs-on: ubuntu-latest - steps: - - uses: docker://agilepathway/pull-request-label-checker:latest - with: - one_of: highlight,breaking-change,security-fix,enhancement,bug - repo_token: ${{ secrets.GITHUB_TOKEN }} \ No newline at end of file + name: Label Checker + on: + pull_request: + types: + - opened + - synchronize + - labeled + - unlabeled + jobs: + check_labels: + name: Check for labels + runs-on: ubuntu-latest + steps: + - uses: docker://agilepathway/pull-request-label-checker:latest + with: + any_of: highlight,breaking-change,security-fix,enhancement,bug,cleanup-rewrite,regression-fix,codex + repo_token: ${{ secrets.GITHUB_TOKEN }} diff --git a/src/s3/client/delete_bucket.rs b/src/s3/client/delete_bucket.rs index a2bcf41b..65056738 100644 --- a/src/s3/client/delete_bucket.rs +++ b/src/s3/client/delete_bucket.rs @@ -57,33 +57,33 @@ impl Client { bucket: S, ) -> Result { let bucket: String = bucket.into(); - if self.is_minio_express().await { - let mut stream = self.list_objects(&bucket).to_stream().await; + let is_express = self.is_minio_express().await; + let mut stream = self + .list_objects(&bucket) + .include_versions(!is_express) + .recursive(true) + .to_stream() + .await; + + if is_express { while let Some(items) = stream.next().await { let object_names = items?.contents.into_iter().map(ObjectToDelete::from); let mut resp = self .delete_objects_streaming(&bucket, object_names) + .bypass_governance_mode(false) // Express does not support governance mode .to_stream() .await; + while let Some(item) = resp.next().await { let _resp: DeleteObjectsResponse = item?; } } } else { - let mut stream = self - .list_objects(&bucket) - .include_versions(true) - .recursive(true) - .to_stream() - .await; - while let Some(items) = stream.next().await { + let object_names = items?.contents.into_iter().map(ObjectToDelete::from); let mut resp = self - .delete_objects_streaming( - &bucket, - items?.contents.into_iter().map(ObjectToDelete::from), - ) + .delete_objects_streaming(&bucket, object_names) .bypass_governance_mode(true) .to_stream() .await; @@ -115,7 +115,8 @@ impl Client { } } } - let request: DeleteBucket = self.delete_bucket(bucket); + + let request: DeleteBucket = self.delete_bucket(&bucket); match request.send().await { Ok(resp) => Ok(resp), Err(Error::S3Error(e)) => { @@ -125,6 +126,24 @@ impl Client { body: Bytes::new(), headers: e.headers, }) + } else if e.code == ErrorCode::BucketNotEmpty { + // for convenience, add the first 5 documents that were are still in the bucket + let mut stream = self + .list_objects(&bucket) + .include_versions(!is_express) + .recursive(true) + .to_stream() + .await; + + let mut objs = Vec::new(); + while let Some(items) = stream.next().await { + objs.append(items?.contents.as_mut()); + if objs.len() >= 5 { + break; + } + } + println!("Bucket '{bucket}' is not empty. The first 5 objects are: {objs:?}"); + Err(Error::S3Error(e)) } else { Err(Error::S3Error(e)) } diff --git a/tests/test_list_buckets.rs b/tests/test_list_buckets.rs index 543f2384..f928fd79 100644 --- a/tests/test_list_buckets.rs +++ b/tests/test_list_buckets.rs @@ -38,12 +38,7 @@ async fn list_buckets(ctx: TestContext) { for bucket in resp.buckets().unwrap().iter() { if names.contains(&bucket.name) { count += 1; - } // else if bucket.name.len() == 8 { - // match ctx.client.delete_and_purge_bucket(&bucket.name).await { - // Ok(_) => println!("Deleted bucket: {}", bucket.name), - // Err(e) => println!("Failed to delete bucket {}: {}", bucket.name, e) - // } - //} + } } assert_eq!(guards.len(), N_BUCKETS); assert_eq!(count, N_BUCKETS); diff --git a/tests/test_list_objects.rs b/tests/test_list_objects.rs index 3708031d..74f5dae2 100644 --- a/tests/test_list_objects.rs +++ b/tests/test_list_objects.rs @@ -41,11 +41,9 @@ async fn list_objects( let is_express = ctx.client.is_minio_express().await; if is_express && !express { - println!("Skipping test because it is running in MinIO Express mode"); - return; + panic!("Skipping test because it is running in MinIO Express mode"); } else if !is_express && express { - println!("Skipping test because it is NOT running in MinIO Express mode"); - return; + panic!("Skipping test because it is NOT running in MinIO Express mode"); } let mut names_set_before: HashSet = HashSet::new(); @@ -97,29 +95,29 @@ async fn list_objects( assert_eq!(names_set_after, names_set_before); } -#[minio_macros::test] +#[minio_macros::test(skip_if_express)] async fn list_objects_v1_no_versions(ctx: TestContext, bucket_name: String) { list_objects(true, false, false, 5, 5, ctx, bucket_name).await; } -#[minio_macros::test] +#[minio_macros::test(skip_if_express)] async fn list_objects_v1_with_versions(ctx: TestContext, bucket_name: String) { list_objects(true, true, false, 5, 5, ctx, bucket_name).await; } -#[minio_macros::test] +#[minio_macros::test(skip_if_express)] async fn list_objects_v2_no_versions(ctx: TestContext, bucket_name: String) { list_objects(false, false, false, 5, 5, ctx, bucket_name).await; } -#[minio_macros::test] +#[minio_macros::test(skip_if_express)] async fn list_objects_v2_with_versions(ctx: TestContext, bucket_name: String) { list_objects(false, true, false, 5, 5, ctx, bucket_name).await; } /// Test for S3-Express: List objects with S3-Express are only supported with V2 API, without -/// versions, and yield unsorted results. -#[minio_macros::test] +/// versions, and yield results that need not be sorted. +#[minio_macros::test(skip_if_not_express)] async fn list_objects_express(ctx: TestContext, bucket_name: String) { list_objects(false, false, true, 5, 5, ctx, bucket_name).await; } From 7c90005a3d5538be72dff0b89ef3dab4b9d8fcc6 Mon Sep 17 00:00:00 2001 From: Henk-Jan Lebbink Date: Thu, 10 Jul 2025 22:00:18 +0200 Subject: [PATCH 3/3] more code review updates --- src/s3/client/delete_bucket.rs | 22 +++++++++++++------- src/s3/error.rs | 4 ++-- src/s3/response/bucket_exists.rs | 2 +- src/s3/response/delete_bucket_policy.rs | 2 +- src/s3/response/delete_bucket_replication.rs | 2 +- src/s3/response/get_bucket_encryption.rs | 5 ++++- src/s3/response/get_bucket_policy.rs | 2 +- src/s3/response/get_bucket_tagging.rs | 2 +- src/s3/response/get_object_retention.rs | 4 +++- src/s3/utils.rs | 8 +++---- tests/test_bucket_create_delete.rs | 4 ++-- tests/test_list_objects.rs | 6 ++++-- 12 files changed, 38 insertions(+), 25 deletions(-) diff --git a/src/s3/client/delete_bucket.rs b/src/s3/client/delete_bucket.rs index 65056738..e627177d 100644 --- a/src/s3/client/delete_bucket.rs +++ b/src/s3/client/delete_bucket.rs @@ -119,15 +119,16 @@ impl Client { let request: DeleteBucket = self.delete_bucket(&bucket); match request.send().await { Ok(resp) => Ok(resp), - Err(Error::S3Error(e)) => { - if e.code == ErrorCode::NoSuchBucket { + Err(Error::S3Error(mut e)) => { + if matches!(e.code, ErrorCode::NoSuchBucket) { Ok(DeleteBucketResponse { request: Default::default(), //TODO consider how to handle this body: Bytes::new(), headers: e.headers, }) - } else if e.code == ErrorCode::BucketNotEmpty { + } else if let ErrorCode::BucketNotEmpty(reason) = &e.code { // for convenience, add the first 5 documents that were are still in the bucket + // to the error message let mut stream = self .list_objects(&bucket) .include_versions(!is_express) @@ -136,13 +137,18 @@ impl Client { .await; let mut objs = Vec::new(); - while let Some(items) = stream.next().await { - objs.append(items?.contents.as_mut()); - if objs.len() >= 5 { - break; + while let Some(items_result) = stream.next().await { + if let Ok(items) = items_result { + objs.extend(items.contents); + if objs.len() >= 5 { + break; + } } + // else: silently ignore the error and keep looping } - println!("Bucket '{bucket}' is not empty. The first 5 objects are: {objs:?}"); + + let new_reason = format!("{reason}: found content: {objs:?}"); + e.code = ErrorCode::BucketNotEmpty(new_reason); Err(Error::S3Error(e)) } else { Err(Error::S3Error(e)) diff --git a/src/s3/error.rs b/src/s3/error.rs index aedfaf6b..6fc2339e 100644 --- a/src/s3/error.rs +++ b/src/s3/error.rs @@ -44,7 +44,7 @@ pub enum ErrorCode { ResourceConflict, AccessDenied, NotSupported, - BucketNotEmpty, + BucketNotEmpty(String), // String contains optional reason msg BucketAlreadyOwnedByYou, InvalidWriteOffset, @@ -75,7 +75,7 @@ impl ErrorCode { "resourceconflict" => ErrorCode::ResourceConflict, "accessdenied" => ErrorCode::AccessDenied, "notsupported" => ErrorCode::NotSupported, - "bucketnotempty" => ErrorCode::BucketNotEmpty, + "bucketnotempty" => ErrorCode::BucketNotEmpty("".to_string()), "bucketalreadyownedbyyou" => ErrorCode::BucketAlreadyOwnedByYou, "invalidwriteoffset" => ErrorCode::InvalidWriteOffset, diff --git a/src/s3/response/bucket_exists.rs b/src/s3/response/bucket_exists.rs index 829a86f7..8bc1e189 100644 --- a/src/s3/response/bucket_exists.rs +++ b/src/s3/response/bucket_exists.rs @@ -50,7 +50,7 @@ impl FromS3Response for BucketExistsResponse { body: resp.bytes().await?, exists: true, }), - Err(Error::S3Error(e)) if e.code == ErrorCode::NoSuchBucket => Ok(Self { + Err(Error::S3Error(e)) if matches!(e.code, ErrorCode::NoSuchBucket) => Ok(Self { request, headers: e.headers, body: Bytes::new(), diff --git a/src/s3/response/delete_bucket_policy.rs b/src/s3/response/delete_bucket_policy.rs index 9c20844f..2cdc71c9 100644 --- a/src/s3/response/delete_bucket_policy.rs +++ b/src/s3/response/delete_bucket_policy.rs @@ -48,7 +48,7 @@ impl FromS3Response for DeleteBucketPolicyResponse { headers: mem::take(resp.headers_mut()), body: resp.bytes().await?, }), - Err(Error::S3Error(e)) if e.code == ErrorCode::NoSuchBucketPolicy => Ok(Self { + Err(Error::S3Error(e)) if matches!(e.code, ErrorCode::NoSuchBucketPolicy) => Ok(Self { request, headers: e.headers, body: Bytes::new(), diff --git a/src/s3/response/delete_bucket_replication.rs b/src/s3/response/delete_bucket_replication.rs index 31ea8d95..ad530caa 100644 --- a/src/s3/response/delete_bucket_replication.rs +++ b/src/s3/response/delete_bucket_replication.rs @@ -49,7 +49,7 @@ impl FromS3Response for DeleteBucketReplicationResponse { body: resp.bytes().await?, }), Err(Error::S3Error(e)) - if e.code == ErrorCode::ReplicationConfigurationNotFoundError => + if matches!(e.code, ErrorCode::ReplicationConfigurationNotFoundError) => { Ok(Self { request, diff --git a/src/s3/response/get_bucket_encryption.rs b/src/s3/response/get_bucket_encryption.rs index bff28534..2c776a79 100644 --- a/src/s3/response/get_bucket_encryption.rs +++ b/src/s3/response/get_bucket_encryption.rs @@ -85,7 +85,10 @@ impl FromS3Response for GetBucketEncryptionResponse { body: resp.bytes().await?, }), Err(Error::S3Error(e)) - if e.code == ErrorCode::ServerSideEncryptionConfigurationNotFoundError => + if matches!( + e.code, + ErrorCode::ServerSideEncryptionConfigurationNotFoundError + ) => { Ok(Self { request, diff --git a/src/s3/response/get_bucket_policy.rs b/src/s3/response/get_bucket_policy.rs index 2c1bd8d6..cd77b88e 100644 --- a/src/s3/response/get_bucket_policy.rs +++ b/src/s3/response/get_bucket_policy.rs @@ -65,7 +65,7 @@ impl FromS3Response for GetBucketPolicyResponse { headers: mem::take(resp.headers_mut()), body: resp.bytes().await?, }), - Err(Error::S3Error(e)) if e.code == ErrorCode::NoSuchBucketPolicy => Ok(Self { + Err(Error::S3Error(e)) if matches!(e.code, ErrorCode::NoSuchBucketPolicy) => Ok(Self { request, headers: e.headers, body: Bytes::from_static("{}".as_ref()), diff --git a/src/s3/response/get_bucket_tagging.rs b/src/s3/response/get_bucket_tagging.rs index 10ec3c58..625c874d 100644 --- a/src/s3/response/get_bucket_tagging.rs +++ b/src/s3/response/get_bucket_tagging.rs @@ -54,7 +54,7 @@ impl FromS3Response for GetBucketTaggingResponse { headers: mem::take(resp.headers_mut()), body: resp.bytes().await?, }), - Err(Error::S3Error(e)) if e.code == ErrorCode::NoSuchTagSet => Ok(Self { + Err(Error::S3Error(e)) if matches!(e.code, ErrorCode::NoSuchTagSet) => Ok(Self { request, headers: e.headers, body: Bytes::new(), diff --git a/src/s3/response/get_object_retention.rs b/src/s3/response/get_object_retention.rs index 5fd27145..edde3377 100644 --- a/src/s3/response/get_object_retention.rs +++ b/src/s3/response/get_object_retention.rs @@ -83,7 +83,9 @@ impl FromS3Response for GetObjectRetentionResponse { headers: mem::take(resp.headers_mut()), body: resp.bytes().await?, }), - Err(Error::S3Error(e)) if e.code == ErrorCode::NoSuchObjectLockConfiguration => { + Err(Error::S3Error(e)) + if matches!(e.code, ErrorCode::NoSuchObjectLockConfiguration) => + { Ok(Self { request, headers: e.headers, diff --git a/src/s3/utils.rs b/src/s3/utils.rs index 420775ae..186337fd 100644 --- a/src/s3/utils.rs +++ b/src/s3/utils.rs @@ -304,15 +304,15 @@ pub fn check_bucket_name(bucket_name: impl AsRef, strict: bool) -> Result<( pub fn check_object_name(object_name: impl AsRef) -> Result<(), Error> { let object_name: &str = object_name.as_ref(); - let bucket_name_n_bytes = object_name.len(); - if bucket_name_n_bytes == 0 { + let object_name_n_bytes = object_name.len(); + if object_name_n_bytes == 0 { return Err(Error::InvalidObjectName( "object name cannot be empty".into(), )); } - if bucket_name_n_bytes > 1024 { + if object_name_n_bytes > 1024 { return Err(Error::InvalidObjectName(format!( - "Bucket name ('{object_name}') cannot be greater than 1024 bytes" + "Object name ('{object_name}') cannot be greater than 1024 bytes" ))); } diff --git a/tests/test_bucket_create_delete.rs b/tests/test_bucket_create_delete.rs index 6eb4f3e4..341771f1 100644 --- a/tests/test_bucket_create_delete.rs +++ b/tests/test_bucket_create_delete.rs @@ -43,7 +43,7 @@ async fn bucket_create(ctx: TestContext) { ctx.client.create_bucket(&bucket_name).send().await; match resp { Ok(_) => panic!("Bucket already exists, but was created again"), - Err(Error::S3Error(e)) if e.code == ErrorCode::BucketAlreadyOwnedByYou => { + Err(Error::S3Error(e)) if matches!(e.code, ErrorCode::BucketAlreadyOwnedByYou) => { // this is expected, as the bucket already exists } Err(e) => panic!("Unexpected error: {:?}", e), @@ -59,7 +59,7 @@ async fn bucket_delete(ctx: TestContext) { ctx.client.delete_bucket(&bucket_name).send().await; match resp { Ok(_) => panic!("Bucket does not exist, but was removed"), - Err(Error::S3Error(e)) if e.code == ErrorCode::NoSuchBucket => { + Err(Error::S3Error(e)) if matches!(e.code, ErrorCode::NoSuchBucket) => { // this is expected, as the bucket does not exist } Err(e) => panic!("Unexpected error: {:?}", e), diff --git a/tests/test_list_objects.rs b/tests/test_list_objects.rs index 74f5dae2..78be1a58 100644 --- a/tests/test_list_objects.rs +++ b/tests/test_list_objects.rs @@ -41,9 +41,11 @@ async fn list_objects( let is_express = ctx.client.is_minio_express().await; if is_express && !express { - panic!("Skipping test because it is running in MinIO Express mode"); + eprintln!("Skipping test because it is running in MinIO Express mode"); + return; } else if !is_express && express { - panic!("Skipping test because it is NOT running in MinIO Express mode"); + eprintln!("Skipping test because it is NOT running in MinIO Express mode"); + return; } let mut names_set_before: HashSet = HashSet::new();