diff --git a/libs/remote_storage/src/s3_bucket.rs b/libs/remote_storage/src/s3_bucket.rs index 43d818dfb9..37a6bf23e8 100644 --- a/libs/remote_storage/src/s3_bucket.rs +++ b/libs/remote_storage/src/s3_bucket.rs @@ -200,13 +200,17 @@ impl S3Bucket { ) } - fn relative_path_to_s3_object(&self, path: &RemotePath) -> String { - let mut full_path = self.prefix_in_bucket.clone().unwrap_or_default(); - for segment in path.0.iter() { - full_path.push(REMOTE_STORAGE_PREFIX_SEPARATOR); - full_path.push_str(segment.to_str().unwrap_or_default()); + pub fn relative_path_to_s3_object(&self, path: &RemotePath) -> String { + assert_eq!(std::path::MAIN_SEPARATOR, REMOTE_STORAGE_PREFIX_SEPARATOR); + let path_string = path + .get_path() + .to_string_lossy() + .trim_end_matches(REMOTE_STORAGE_PREFIX_SEPARATOR) + .to_string(); + match &self.prefix_in_bucket { + Some(prefix) => prefix.clone() + "/" + &path_string, + None => path_string, } - full_path } async fn download_object(&self, request: GetObjectRequest) -> Result { @@ -427,10 +431,12 @@ impl RemoteStorage for S3Bucket { } async fn download(&self, from: &RemotePath) -> Result { + // if prefix is not none then download file `prefix/from` + // if prefix is none then download file `from` self.download_object(GetObjectRequest { bucket: self.bucket_name.clone(), key: self.relative_path_to_s3_object(from), - ..GetObjectRequest::default() + range: None, }) .await } @@ -523,3 +529,63 @@ impl RemoteStorage for S3Bucket { Ok(()) } } + +#[cfg(test)] +mod tests { + use std::num::NonZeroUsize; + use std::path::Path; + + use crate::{RemotePath, S3Bucket, S3Config}; + + #[test] + fn relative_path() { + let all_paths = vec!["", "some/path", "some/path/"]; + let all_paths: Vec = all_paths + .iter() + .map(|x| RemotePath::new(Path::new(x)).expect("bad path")) + .collect(); + let prefixes = [ + None, + Some(""), + Some("test/prefix"), + Some("test/prefix/"), + Some("/test/prefix/"), + ]; + let expected_outputs = vec![ + vec!["", "some/path", "some/path"], + vec!["/", "/some/path", "/some/path"], + vec![ + "test/prefix/", + "test/prefix/some/path", + "test/prefix/some/path", + ], + vec![ + "test/prefix/", + "test/prefix/some/path", + "test/prefix/some/path", + ], + vec![ + "test/prefix/", + "test/prefix/some/path", + "test/prefix/some/path", + ], + ]; + + for (prefix_idx, prefix) in prefixes.iter().enumerate() { + let config = S3Config { + bucket_name: "bucket".to_owned(), + bucket_region: "region".to_owned(), + prefix_in_bucket: prefix.map(str::to_string), + endpoint: None, + concurrency_limit: NonZeroUsize::new(100).unwrap(), + max_keys_per_list_response: Some(5), + }; + let storage = S3Bucket::new(&config).expect("remote storage init"); + for (test_path_idx, test_path) in all_paths.iter().enumerate() { + let result = storage.relative_path_to_s3_object(test_path); + let expected = expected_outputs[prefix_idx][test_path_idx]; + assert_eq!(result, expected); + } + } + } +} diff --git a/libs/remote_storage/tests/test_real_s3.rs b/libs/remote_storage/tests/test_real_s3.rs index 0917bab39c..982c01a9be 100644 --- a/libs/remote_storage/tests/test_real_s3.rs +++ b/libs/remote_storage/tests/test_real_s3.rs @@ -19,7 +19,7 @@ static LOGGING_DONE: OnceCell<()> = OnceCell::new(); const ENABLE_REAL_S3_REMOTE_STORAGE_ENV_VAR_NAME: &str = "ENABLE_REAL_S3_REMOTE_STORAGE"; -const BASE_PREFIX: &str = "test/"; +const BASE_PREFIX: &str = "test"; /// Tests that S3 client can list all prefixes, even if the response come paginated and requires multiple S3 queries. /// Uses real S3 and requires [`ENABLE_REAL_S3_REMOTE_STORAGE_ENV_VAR_NAME`] and related S3 cred env vars specified. diff --git a/test_runner/fixtures/neon_fixtures.py b/test_runner/fixtures/neon_fixtures.py index 5e4553f52d..fb78d69d67 100644 --- a/test_runner/fixtures/neon_fixtures.py +++ b/test_runner/fixtures/neon_fixtures.py @@ -542,7 +542,7 @@ class S3Storage: access_key: str secret_key: str endpoint: Optional[str] = None - prefix_in_bucket: Optional[str] = None + prefix_in_bucket: Optional[str] = "" def access_env_vars(self) -> Dict[str, str]: return {