From 225d42c0a3f7ed957780834d10ba66f4e7346a91 Mon Sep 17 00:00:00 2001 From: ysaito1001 Date: Tue, 29 Oct 2024 15:11:01 -0500 Subject: [PATCH] Add integration tests for uploading multiple objects --- aws-s3-transfer-manager/Cargo.toml | 7 +- aws-s3-transfer-manager/src/error.rs | 2 +- .../src/operation/upload_objects/input.rs | 2 - .../src/operation/upload_objects/output.rs | 2 +- .../src/operation/upload_objects/worker.rs | 40 +--- .../test-common/Cargo.toml | 8 + .../test-common/src/lib.rs | 50 +++++ .../tests/download_objects_test.rs | 2 +- .../tests/upload_objects_test.rs | 191 ++++++++++++++++++ 9 files changed, 257 insertions(+), 47 deletions(-) create mode 100644 aws-s3-transfer-manager/test-common/Cargo.toml create mode 100644 aws-s3-transfer-manager/test-common/src/lib.rs create mode 100644 aws-s3-transfer-manager/tests/upload_objects_test.rs diff --git a/aws-s3-transfer-manager/Cargo.toml b/aws-s3-transfer-manager/Cargo.toml index a247fe1..4871628 100644 --- a/aws-s3-transfer-manager/Cargo.toml +++ b/aws-s3-transfer-manager/Cargo.toml @@ -36,12 +36,13 @@ clap = { version = "4.5.7", default-features = false, features = ["derive", "std console-subscriber = "0.4.0" http-02x = { package = "http", version = "0.2.9" } http-body-1x = { package = "http-body", version = "1" } -tracing-subscriber = { version = "0.3.18", features = ["env-filter"] } -tempfile = "3.12.0" fastrand = "2.1.1" futures-test = "0.3.30" -tower-test = "0.4.0" +tempfile = "3.12.0" +test-common = { path = "./test-common" } tokio-test = "0.4.4" +tower-test = "0.4.0" +tracing-subscriber = { version = "0.3.18", features = ["env-filter"] } [target.'cfg(not(target_env = "msvc"))'.dev-dependencies] jemallocator = "0.5.4" diff --git a/aws-s3-transfer-manager/src/error.rs b/aws-s3-transfer-manager/src/error.rs index b5bf421..f4e3d0a 100644 --- a/aws-s3-transfer-manager/src/error.rs +++ b/aws-s3-transfer-manager/src/error.rs @@ -21,7 +21,7 @@ pub struct Error { } /// General categories of transfer errors. -#[derive(Debug, Clone)] +#[derive(Clone, Debug, Eq, PartialEq)] #[non_exhaustive] pub enum ErrorKind { /// Operation input validation issues diff --git a/aws-s3-transfer-manager/src/operation/upload_objects/input.rs b/aws-s3-transfer-manager/src/operation/upload_objects/input.rs index 2aae28f..44ce39f 100644 --- a/aws-s3-transfer-manager/src/operation/upload_objects/input.rs +++ b/aws-s3-transfer-manager/src/operation/upload_objects/input.rs @@ -8,8 +8,6 @@ use aws_smithy_types::error::operation::BuildError; use std::path::{Path, PathBuf}; -// TODO - docs and examples on the interaction of key_prefix & delimiter - /// Input type for uploading multiple objects #[non_exhaustive] #[derive(Clone, Debug)] diff --git a/aws-s3-transfer-manager/src/operation/upload_objects/output.rs b/aws-s3-transfer-manager/src/operation/upload_objects/output.rs index ecd784c..c8ee21a 100644 --- a/aws-s3-transfer-manager/src/operation/upload_objects/output.rs +++ b/aws-s3-transfer-manager/src/operation/upload_objects/output.rs @@ -36,7 +36,7 @@ impl UploadObjectsOutput { self.failed_transfers.as_deref().unwrap_or_default() } - /// The number of bytes successfully transferred (downloaded) + /// The number of bytes successfully transferred (uploaded) pub fn total_bytes_transferred(&self) -> u64 { self.total_bytes_transferred } diff --git a/aws-s3-transfer-manager/src/operation/upload_objects/worker.rs b/aws-s3-transfer-manager/src/operation/upload_objects/worker.rs index 0491e80..c9c7bf4 100644 --- a/aws-s3-transfer-manager/src/operation/upload_objects/worker.rs +++ b/aws-s3-transfer-manager/src/operation/upload_objects/worker.rs @@ -235,9 +235,7 @@ mod tests { use crate::operation::upload_objects::UploadObjectsInputBuilder; use std::collections::BTreeMap; use std::error::Error as _; - use std::fs; - use std::io::Write; - use tempfile::{tempdir, TempDir}; + use test_common::create_test_dir; #[cfg(target_family = "unix")] #[test] @@ -302,42 +300,6 @@ mod tests { ); } - #[cfg(target_family = "unix")] - fn create_test_dir( - recursion_root: Option<&str>, - files: Vec<(&str, usize)>, - inaccessible_dir_relative_paths: &[&str], - ) -> TempDir { - let temp_dir = match recursion_root { - Some(root) => TempDir::with_prefix(root).unwrap(), - None => tempdir().unwrap(), - }; - - // Create the directory structure and files - for (path, size) in files { - let full_path = temp_dir.path().join(path); - let parent = full_path.parent().unwrap(); - - // Create the parent directories if they don't exist - fs::create_dir_all(parent).unwrap(); - - // Create the .jpg file with the specified size - let mut file = fs::File::create(&full_path).unwrap(); - file.write_all(&vec![0; size]).unwrap(); // Writing `size` byte - } - - // Set the directories in `inaccessible_dir_relative_paths` to be inaccessible, - // which will in turn render the files within those directories inaccessible - for dir_relative_path in inaccessible_dir_relative_paths { - let dir_path = temp_dir.path().join(*dir_relative_path); - let mut permissions = fs::metadata(&dir_path).unwrap().permissions(); - std::os::unix::fs::PermissionsExt::set_mode(&mut permissions, 0o000); // No permissions for anyone - fs::set_permissions(dir_path, permissions).unwrap(); - } - - temp_dir - } - async fn exercise_list_directory_contents( input: UploadObjectsInput, ) -> (BTreeMap, Vec) { diff --git a/aws-s3-transfer-manager/test-common/Cargo.toml b/aws-s3-transfer-manager/test-common/Cargo.toml new file mode 100644 index 0000000..9a2827e --- /dev/null +++ b/aws-s3-transfer-manager/test-common/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "test-common" +version = "0.1.0" +edition = "2021" +publish = false + +[dependencies] +tempfile = "3.12.0" \ No newline at end of file diff --git a/aws-s3-transfer-manager/test-common/src/lib.rs b/aws-s3-transfer-manager/test-common/src/lib.rs new file mode 100644 index 0000000..97c3a3e --- /dev/null +++ b/aws-s3-transfer-manager/test-common/src/lib.rs @@ -0,0 +1,50 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#![cfg(target_family = "unix")] + +use std::{fs, io::Write}; +use tempfile::{tempdir, TempDir}; + +/// Create a directory structure rooted at `recursion_root`, containing files with sizes +/// specified in `files` +/// +/// For testing purposes, certain directories (and all files within them) can be made +/// inaccessible by providing `inaccessible_dir_relative_paths`, which should be relative +/// to `recursion_root`. +pub fn create_test_dir( + recursion_root: Option<&str>, + files: Vec<(&str, usize)>, + inaccessible_dir_relative_paths: &[&str], +) -> TempDir { + let temp_dir = match recursion_root { + Some(root) => TempDir::with_prefix(root).unwrap(), + None => tempdir().unwrap(), + }; + + // Create the directory structure and files + for (path, size) in files { + let full_path = temp_dir.path().join(path); + let parent = full_path.parent().unwrap(); + + // Create the parent directories if they don't exist + fs::create_dir_all(parent).unwrap(); + + // Create the .jpg file with the specified size + let mut file = fs::File::create(&full_path).unwrap(); + file.write_all(&vec![0; size]).unwrap(); // Writing `size` byte + } + + // Set the directories in `inaccessible_dir_relative_paths` to be inaccessible, + // which will in turn render the files within those directories inaccessible + for dir_relative_path in inaccessible_dir_relative_paths { + let dir_path = temp_dir.path().join(*dir_relative_path); + let mut permissions = fs::metadata(&dir_path).unwrap().permissions(); + std::os::unix::fs::PermissionsExt::set_mode(&mut permissions, 0o000); // No permissions for anyone + fs::set_permissions(dir_path, permissions).unwrap(); + } + + temp_dir +} \ No newline at end of file diff --git a/aws-s3-transfer-manager/tests/download_objects_test.rs b/aws-s3-transfer-manager/tests/download_objects_test.rs index 862eb30..ba0508f 100644 --- a/aws-s3-transfer-manager/tests/download_objects_test.rs +++ b/aws-s3-transfer-manager/tests/download_objects_test.rs @@ -401,5 +401,5 @@ async fn test_destination_dir_not_valid() { .unwrap_err(); let err_str = format!("{}", DisplayErrorContext(err)); - assert!(err_str.contains("is not a directory")); + assert!(err_str.contains("target is not a directory")); } diff --git a/aws-s3-transfer-manager/tests/upload_objects_test.rs b/aws-s3-transfer-manager/tests/upload_objects_test.rs new file mode 100644 index 0000000..84a47fc --- /dev/null +++ b/aws-s3-transfer-manager/tests/upload_objects_test.rs @@ -0,0 +1,191 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#![cfg(target_family = "unix")] + +use aws_s3_transfer_manager::{error::ErrorKind, types::FailedTransferPolicy}; +use aws_sdk_s3::{ + error::DisplayErrorContext, + operation::{ + complete_multipart_upload::CompleteMultipartUploadOutput, + create_multipart_upload::CreateMultipartUploadOutput, upload_part::UploadPartOutput, + }, + Client, +}; +use aws_smithy_mocks_experimental::{mock, mock_client, RuleMode}; +use test_common::create_test_dir; + +// Create an S3 client with mock behavior configured +// +// We intentionally avoid being specific about the expected input and output for mocks, +// as long as the execution of uploading multiple objects completes successfully. +// Setting expectations that are too precise can lead to brittle tests. +fn mock_s3_client(bucket_name: String) -> Client { + let upload_id = "test-upload-id".to_owned(); + + let create_mpu = mock!(aws_sdk_s3::Client::create_multipart_upload).then_output({ + let upload_id = upload_id.clone(); + move || { + CreateMultipartUploadOutput::builder() + .upload_id(upload_id.clone()) + .build() + } + }); + + let upload_part = mock!(aws_sdk_s3::Client::upload_part) + .match_requests({ + let upload_id = upload_id.clone(); + move |input| { + input.upload_id.as_ref() == Some(&upload_id) && input.bucket() == Some(&bucket_name) + } + }) + .then_output(|| UploadPartOutput::builder().build()); + + let complete_mpu = mock!(aws_sdk_s3::Client::complete_multipart_upload) + .match_requests({ + let upload_id = upload_id.clone(); + move |r| r.upload_id.as_ref() == Some(&upload_id) + }) + .then_output(|| CompleteMultipartUploadOutput::builder().build()); + + mock_client!( + aws_sdk_s3, + RuleMode::MatchAny, + &[create_mpu, upload_part, complete_mpu] + ) +} + +#[tokio::test] +async fn test_successful_multiple_objects_upload() { + let recursion_root = "test"; + let files = vec![ + ("sample.jpg", 1), + ("photos/2022/January/sample.jpg", 1), + ("photos/2022/February/sample1.jpg", 1), + ("photos/2022/February/sample2.jpg", 1), + ("photos/2022/February/sample3.jpg", 1), + ]; + let test_dir = create_test_dir(Some(&recursion_root), files.clone(), &[]); + + let bucket_name = "test-bucket"; + let config = aws_s3_transfer_manager::Config::builder() + .client(mock_s3_client(bucket_name.to_owned())) + .build(); + let sut = aws_s3_transfer_manager::Client::new(config); + + let handle = sut + .upload_objects() + .bucket(bucket_name) + .source(test_dir.path()) + .recursive(true) + .send() + .await + .unwrap(); + + let output = handle.join().await.unwrap(); + assert_eq!(5, output.objects_uploaded()); + assert!(output.failed_transfers().is_empty()); + assert_eq!(5, output.total_bytes_transferred()); +} + +#[tokio::test] +async fn test_failed_upload_policy_continue() { + let recursion_root = "test"; + let files = vec![ + ("sample.jpg", 1), + ("photos/2022/January/sample.jpg", 1), + ("photos/2022/February/sample1.jpg", 1), + ("photos/2022/February/sample2.jpg", 1), + ("photos/2022/February/sample3.jpg", 1), + ]; + // Make all files inaccessible under `photos/2022/February` + let inaccessible_dir_relative_path = "photos/2022/February"; + let test_dir = create_test_dir( + Some(&recursion_root), + files.clone(), + &[inaccessible_dir_relative_path], + ); + + let bucket_name = "test-bucket"; + let config = aws_s3_transfer_manager::Config::builder() + .client(mock_s3_client(bucket_name.to_owned())) + .build(); + let sut = aws_s3_transfer_manager::Client::new(config); + + let handle = sut + .upload_objects() + .bucket(bucket_name) + .source(test_dir.path()) + .recursive(true) + .failure_policy(FailedTransferPolicy::Continue) + .send() + .await + .unwrap(); + + let output = handle.join().await.unwrap(); + assert_eq!(2, output.objects_uploaded()); + assert_eq!(1, output.failed_transfers().len()); // Cannot traverse inaccessible dir to count how many files are in it + assert_eq!(2, output.total_bytes_transferred()); +} + +/// Fail when source is not a directory +#[tokio::test] +async fn test_source_dir_not_valid() { + let source = tempfile::NamedTempFile::new().unwrap(); + + let bucket_name = "test-bucket"; + let config = aws_s3_transfer_manager::Config::builder() + .client(mock_s3_client(bucket_name.to_owned())) + .build(); + let sut = aws_s3_transfer_manager::Client::new(config); + + let err = sut + .upload_objects() + .bucket(bucket_name) + .source(source.path()) + .send() + .await + .unwrap_err(); + + assert_eq!(&ErrorKind::InputInvalid, err.kind()); + assert!(format!("{}", DisplayErrorContext(err)).contains("is not a directory")); +} + +#[tokio::test] +async fn test_error_when_custom_delimiter_appears_in_filename() { + let recursion_root = "test"; + let files = vec![ + ("sample.jpg", 1), + ("photos/2022-January/sample.jpg", 1), + ("photos/2022-February/sample1.jpg", 1), + ("photos/2022-February/sample2.jpg", 1), + ("photos/2022-February/sample3.jpg", 1), + ]; + let test_dir = create_test_dir(Some(&recursion_root), files.clone(), &[]); + + let bucket_name = "test-bucket"; + let config = aws_s3_transfer_manager::Config::builder() + .client(mock_s3_client(bucket_name.to_owned())) + .build(); + let sut = aws_s3_transfer_manager::Client::new(config); + + // Getting `handle` is ok, i.e., tasks should be spawned from `UploadObjects::orchestrate` + let handle = sut + .upload_objects() + .bucket(bucket_name) + .source(test_dir.path()) + .recursive(true) + .delimiter("-") + .send() + .await + .unwrap(); + + // But unwrapping it should fail + let err = handle.join().await.unwrap_err(); + + assert_eq!(&ErrorKind::InputInvalid, err.kind()); + assert!(format!("{}", DisplayErrorContext(err)) + .contains("a custom delimiter `-` should not appear")); +}