From 4c7595951ce773444bb5f79a6b23128a3174607d Mon Sep 17 00:00:00 2001 From: sfauvel Date: Wed, 11 Dec 2024 15:58:13 +0100 Subject: [PATCH] refactor: Use a FileUri struct instead of a String for the location --- .../cardano_immutable_files_full.rs | 16 ++++++---- .../src/file_uploaders/dumb_uploader.rs | 16 +++++----- .../src/file_uploaders/gcp_uploader.rs | 30 +++++++++---------- .../src/file_uploaders/interface.rs | 11 +++++-- .../src/file_uploaders/local_uploader.rs | 10 +++---- mithril-aggregator/src/file_uploaders/mod.rs | 2 +- 6 files changed, 47 insertions(+), 38 deletions(-) diff --git a/mithril-aggregator/src/artifact_builder/cardano_immutable_files_full.rs b/mithril-aggregator/src/artifact_builder/cardano_immutable_files_full.rs index 61424f6410..49a0548808 100644 --- a/mithril-aggregator/src/artifact_builder/cardano_immutable_files_full.rs +++ b/mithril-aggregator/src/artifact_builder/cardano_immutable_files_full.rs @@ -5,9 +5,7 @@ use slog::{debug, warn, Logger}; use std::sync::Arc; use thiserror::Error; -use crate::{ - file_uploaders::FileLocation, snapshotter::OngoingSnapshot, FileUploader, Snapshotter, -}; +use crate::{file_uploaders::FileUri, snapshotter::OngoingSnapshot, FileUploader, Snapshotter}; use super::ArtifactBuilder; use mithril_common::logging::LoggerExtensions; @@ -88,7 +86,7 @@ impl CardanoImmutableFilesFullArtifactBuilder { async fn upload_snapshot_archive( &self, ongoing_snapshot: &OngoingSnapshot, - ) -> StdResult> { + ) -> StdResult> { debug!(self.logger, ">> upload_snapshot_archive"); let location = self .snapshot_uploader @@ -157,7 +155,12 @@ impl ArtifactBuilder for CardanoImmutableFilesFullArt })?; let snapshot = self - .create_snapshot(beacon, &ongoing_snapshot, snapshot_digest, locations) + .create_snapshot( + beacon, + &ongoing_snapshot, + snapshot_digest, + locations.into_iter().map(Into::into).collect(), + ) .await?; Ok(snapshot) @@ -173,7 +176,7 @@ mod tests { use mithril_common::{entities::CompressionAlgorithm, test_utils::fake_data}; use crate::{ - file_uploaders::MockFileUploader, test_tools::TestLogger, DumbUploader, DumbSnapshotter, + file_uploaders::MockFileUploader, test_tools::TestLogger, DumbSnapshotter, DumbUploader, }; use super::*; @@ -211,6 +214,7 @@ mod tests { let remote_locations = vec![dumb_snapshot_uploader .get_last_upload() .unwrap() + .map(Into::into) .expect("A snapshot should have been 'uploaded'")]; let artifact_expected = Snapshot::new( snapshot_digest.to_owned(), diff --git a/mithril-aggregator/src/file_uploaders/dumb_uploader.rs b/mithril-aggregator/src/file_uploaders/dumb_uploader.rs index 6f3ff18392..0ad7fef61b 100644 --- a/mithril-aggregator/src/file_uploaders/dumb_uploader.rs +++ b/mithril-aggregator/src/file_uploaders/dumb_uploader.rs @@ -3,14 +3,14 @@ use async_trait::async_trait; use mithril_common::StdResult; use std::{path::Path, sync::RwLock}; -use super::{FileLocation, FileUploader}; +use crate::file_uploaders::{FileUploader, FileUri}; /// Dummy uploader for test purposes. /// /// It actually does NOT upload any file but remembers the last file it /// was asked to upload. This is intended to by used by integration tests. pub struct DumbUploader { - last_uploaded: RwLock>, + last_uploaded: RwLock>, } impl DumbUploader { @@ -22,13 +22,13 @@ impl DumbUploader { } /// Return the last upload that was triggered. - pub fn get_last_upload(&self) -> StdResult> { + pub fn get_last_upload(&self) -> StdResult> { let value = self .last_uploaded .read() .map_err(|e| anyhow!("Error while saving filepath location: {e}"))?; - Ok(value.as_ref().map(|v| v.to_string())) + Ok(value.as_ref().map(Clone::clone)) } } @@ -41,13 +41,13 @@ impl Default for DumbUploader { #[async_trait] impl FileUploader for DumbUploader { /// Upload a file - async fn upload(&self, filepath: &Path) -> StdResult { + async fn upload(&self, filepath: &Path) -> StdResult { let mut value = self .last_uploaded .write() .map_err(|e| anyhow!("Error while saving filepath location: {e}"))?; - let location = filepath.to_string_lossy().to_string(); + let location = FileUri(filepath.to_string_lossy().to_string()); *value = Some(location.clone()); Ok(location) @@ -69,9 +69,9 @@ mod tests { .upload(Path::new("/tmp/whatever")) .await .expect("uploading with a dumb uploader should not fail"); - assert_eq!(res, "/tmp/whatever".to_string()); + assert_eq!(res, FileUri("/tmp/whatever".to_string())); assert_eq!( - Some("/tmp/whatever".to_string()), + Some(FileUri("/tmp/whatever".to_string())), uploader .get_last_upload() .expect("getting dumb uploader last value after a fake download should not fail") diff --git a/mithril-aggregator/src/file_uploaders/gcp_uploader.rs b/mithril-aggregator/src/file_uploaders/gcp_uploader.rs index 2ffdd4c257..a7f2824b2d 100644 --- a/mithril-aggregator/src/file_uploaders/gcp_uploader.rs +++ b/mithril-aggregator/src/file_uploaders/gcp_uploader.rs @@ -6,13 +6,11 @@ use cloud_storage::{ }; use slog::{info, Logger}; use std::{env, path::Path}; -use tokio_util::{codec::BytesCodec, codec::FramedRead}; +use tokio_util::codec::{BytesCodec, FramedRead}; -use mithril_common::logging::LoggerExtensions; -use mithril_common::StdResult; +use mithril_common::{logging::LoggerExtensions, StdResult}; -use crate::file_uploaders::FileLocation; -use crate::FileUploader; +use crate::{file_uploaders::FileUri, FileUploader}; /// GcpUploader represents a Google Cloud Platform file uploader interactor pub struct GcpUploader { @@ -31,21 +29,21 @@ impl GcpUploader { } } - fn get_location(&self, filename: &str) -> String { - if self.use_cdn_domain { - format!("https://{}/{}", self.bucket, filename) - } else { - format!( - "https://storage.googleapis.com/{}/{}", - self.bucket, filename - ) + fn get_location(&self, filename: &str) -> FileUri { + let mut uri = vec![]; + if !self.use_cdn_domain { + uri.push("storage.googleapis.com"); } + uri.push(&self.bucket); + uri.push(filename); + + FileUri(format!("https://{}", uri.join("/"))) } } #[async_trait] impl FileUploader for GcpUploader { - async fn upload(&self, filepath: &Path) -> StdResult { + async fn upload(&self, filepath: &Path) -> StdResult { if env::var("GOOGLE_APPLICATION_CREDENTIALS_JSON").is_err() { return Err(anyhow!( "Missing GOOGLE_APPLICATION_CREDENTIALS_JSON environment variable".to_string() @@ -118,7 +116,7 @@ mod tests { let location = file_uploader.get_location(filename); - assert_eq!(expected_location, location); + assert_eq!(FileUri(expected_location), location); } #[tokio::test] @@ -135,6 +133,6 @@ mod tests { let location = file_uploader.get_location(filename); - assert_eq!(expected_location, location); + assert_eq!(FileUri(expected_location), location); } } diff --git a/mithril-aggregator/src/file_uploaders/interface.rs b/mithril-aggregator/src/file_uploaders/interface.rs index 34eb21913e..924d2122ed 100644 --- a/mithril-aggregator/src/file_uploaders/interface.rs +++ b/mithril-aggregator/src/file_uploaders/interface.rs @@ -2,12 +2,19 @@ use async_trait::async_trait; use mithril_common::StdResult; use std::path::Path; -pub type FileLocation = String; +#[derive(Debug, PartialEq, Clone)] +pub struct FileUri(pub String); + +impl From for String { + fn from(file_uri: FileUri) -> Self { + file_uri.0 + } +} /// FileUploader represents a file uploader interactor #[cfg_attr(test, mockall::automock)] #[async_trait] pub trait FileUploader: Sync + Send { /// Upload a file - async fn upload(&self, filepath: &Path) -> StdResult; + async fn upload(&self, filepath: &Path) -> StdResult; } diff --git a/mithril-aggregator/src/file_uploaders/local_uploader.rs b/mithril-aggregator/src/file_uploaders/local_uploader.rs index 2d85692199..b6a2c7933f 100644 --- a/mithril-aggregator/src/file_uploaders/local_uploader.rs +++ b/mithril-aggregator/src/file_uploaders/local_uploader.rs @@ -6,7 +6,7 @@ use std::path::{Path, PathBuf}; use mithril_common::logging::LoggerExtensions; use mithril_common::StdResult; -use crate::file_uploaders::{FileLocation, FileUploader}; +use crate::file_uploaders::{FileUploader, FileUri}; use crate::http_server; use crate::tools; @@ -36,7 +36,7 @@ impl LocalUploader { #[async_trait] impl FileUploader for LocalUploader { - async fn upload(&self, filepath: &Path) -> StdResult { + async fn upload(&self, filepath: &Path) -> StdResult { let archive_name = filepath.file_name().unwrap().to_str().unwrap(); let target_path = &self.target_location.join(archive_name); tokio::fs::copy(filepath, target_path) @@ -54,7 +54,7 @@ impl FileUploader for LocalUploader { ); debug!(self.logger, "File 'uploaded' to local storage"; "location" => &location); - Ok(location) + Ok(FileUri(location)) } } @@ -65,7 +65,7 @@ mod tests { use std::path::{Path, PathBuf}; use tempfile::tempdir; - use crate::file_uploaders::FileUploader; + use crate::file_uploaders::{FileUploader, FileUri}; use crate::http_server; use crate::test_tools::TestLogger; @@ -103,7 +103,7 @@ mod tests { .await .expect("local upload should not fail"); - assert_eq!(expected_location, location); + assert_eq!(FileUri(expected_location), location); } #[tokio::test] diff --git a/mithril-aggregator/src/file_uploaders/mod.rs b/mithril-aggregator/src/file_uploaders/mod.rs index 8ec7ef0f70..31cf085fac 100644 --- a/mithril-aggregator/src/file_uploaders/mod.rs +++ b/mithril-aggregator/src/file_uploaders/mod.rs @@ -5,7 +5,7 @@ mod local_uploader; pub use dumb_uploader::*; pub use gcp_uploader::GcpUploader; -pub use interface::FileLocation; +pub use interface::FileUri; pub use interface::FileUploader; pub use local_uploader::LocalUploader;