refactor: make GcpFileUploader implement FileUploader

Co-authored-by: Sébastien Fauvel <[email protected]>

mithril-aggregator/src/dependency_injection/builder.rs

@@ -462,9 +462,11 @@ impl DependenciesBuilder {
                         })?;
 
                     Ok(Arc::new(RemoteSnapshotUploader::new(
-                        Box::new(GcpFileUploader::new(bucket.clone(), logger.clone())),
-                        bucket,
-                        self.configuration.snapshot_use_cdn_domain,
+                        Box::new(GcpFileUploader::new(
+                            bucket,
+                            self.configuration.snapshot_use_cdn_domain,
+                            logger.clone(),
+                        )),
                         logger,
                     )))
                 }

mithril-aggregator/src/file_uploaders/remote_file_uploader.rs

@@ -6,30 +6,20 @@ use mithril_common::logging::LoggerExtensions;
 use mithril_common::StdResult;
 
 use crate::file_uploaders::{FileLocation, FileUploader};
-use crate::tools::RemoteFileUploader;
 
-/// GCPSnapshotUploader is a snapshot uploader working using Google Cloud Platform services
+/// RemoteSnapshotUploader is a snapshot uploader working using Google Cloud Platform services
 pub struct RemoteSnapshotUploader {
-    bucket: String,
-    file_uploader: Box<dyn RemoteFileUploader>,
-    use_cdn_domain: bool,
+    file_uploader: Box<dyn FileUploader>,
     logger: Logger,
 }
 
 impl RemoteSnapshotUploader {
-    /// GCPSnapshotUploader factory
-    pub fn new(
-        file_uploader: Box<dyn RemoteFileUploader>,
-        bucket: String,
-        use_cdn_domain: bool,
-        logger: Logger,
-    ) -> Self {
+    /// RemoteSnapshotUploader factory
+    pub fn new(file_uploader: Box<dyn FileUploader>, logger: Logger) -> Self {
         let logger = logger.new_with_component_name::<Self>();
-        debug!(logger, "New GCPSnapshotUploader created");
+        debug!(logger, "New RemoteSnapshotUploader created");
         Self {
-            bucket,
             file_uploader,
-            use_cdn_domain,
             logger,
         }
     }
@@ -38,18 +28,7 @@ impl RemoteSnapshotUploader {
 #[async_trait]
 impl FileUploader for RemoteSnapshotUploader {
     async fn upload(&self, snapshot_filepath: &Path) -> StdResult<FileLocation> {
-        let archive_name = snapshot_filepath.file_name().unwrap().to_str().unwrap();
-        let location = if self.use_cdn_domain {
-            format!("https://{}/{}", self.bucket, archive_name)
-        } else {
-            format!(
-                "https://storage.googleapis.com/{}/{}",
-                self.bucket, archive_name
-            )
-        };
-
-        debug!(self.logger, "Uploading snapshot to remote storage"; "location" => &location);
-        self.file_uploader.upload_file(snapshot_filepath).await?;
+        let location = self.file_uploader.upload(snapshot_filepath).await?;
         debug!(self.logger, "Snapshot upload to remote storage completed"; "location" => &location);
 
         Ok(location)
@@ -59,71 +38,43 @@ impl FileUploader for RemoteSnapshotUploader {
 #[cfg(test)]
 mod tests {
     use anyhow::anyhow;
+    use mockall::predicate::eq;
     use std::path::Path;
 
-    use crate::file_uploaders::FileUploader;
+    use crate::file_uploaders::{FileUploader, MockFileUploader};
     use crate::test_tools::TestLogger;
-    use crate::tools::MockRemoteFileUploader;
 
     use super::RemoteSnapshotUploader;
 
     #[tokio::test]
-    async fn test_upload_snapshot_not_using_cdn_domain_ok() {
-        let use_cdn_domain = false;
-        let mut file_uploader = MockRemoteFileUploader::new();
-        file_uploader.expect_upload_file().returning(|_| Ok(()));
-        let snapshot_uploader = RemoteSnapshotUploader::new(
-            Box::new(file_uploader),
-            "cardano-testnet".to_string(),
-            use_cdn_domain,
-            TestLogger::stdout(),
-        );
-        let snapshot_filepath = Path::new("test/snapshot.xxx.tar.gz");
-        let expected_location =
-            "https://storage.googleapis.com/cardano-testnet/snapshot.xxx.tar.gz".to_string();
-
-        let location = snapshot_uploader
-            .upload(snapshot_filepath)
-            .await
-            .expect("remote upload should not fail");
-
-        assert_eq!(expected_location, location);
-    }
-
-    #[tokio::test]
-    async fn test_upload_snapshot_using_cdn_domain_ok() {
-        let use_cdn_domain = true;
-        let mut file_uploader = MockRemoteFileUploader::new();
-        file_uploader.expect_upload_file().returning(|_| Ok(()));
-        let snapshot_uploader = RemoteSnapshotUploader::new(
-            Box::new(file_uploader),
-            "cdn.mithril.network".to_string(),
-            use_cdn_domain,
-            TestLogger::stdout(),
-        );
-        let snapshot_filepath = Path::new("test/snapshot.xxx.tar.gz");
+    async fn upload_call_uploader_and_return_location() {
+        let mut file_uploader = MockFileUploader::new();
+        file_uploader
+            .expect_upload()
+            .with(eq(Path::new("test/snapshot.xxx.tar.gz")))
+            .times(1)
+            .returning(|_| Ok("https://cdn.mithril.network/snapshot.xxx.tar.gz".to_string()));
+        let snapshot_uploader =
+            RemoteSnapshotUploader::new(Box::new(file_uploader), TestLogger::stdout());
+        let filepath = Path::new("test/snapshot.xxx.tar.gz");
         let expected_location = "https://cdn.mithril.network/snapshot.xxx.tar.gz".to_string();
 
         let location = snapshot_uploader
-            .upload(snapshot_filepath)
+            .upload(filepath)
             .await
             .expect("remote upload should not fail");
 
         assert_eq!(expected_location, location);
     }
 
     #[tokio::test]
-    async fn test_upload_snapshot_ko() {
-        let mut file_uploader = MockRemoteFileUploader::new();
+    async fn upload_return_error_when_uploader_error() {
+        let mut file_uploader = MockFileUploader::new();
         file_uploader
-            .expect_upload_file()
+            .expect_upload()
             .returning(|_| Err(anyhow!("unexpected error")));
-        let snapshot_uploader = RemoteSnapshotUploader::new(
-            Box::new(file_uploader),
-            "".to_string(),
-            false,
-            TestLogger::stdout(),
-        );
+        let snapshot_uploader =
+            RemoteSnapshotUploader::new(Box::new(file_uploader), TestLogger::stdout());
         let snapshot_filepath = Path::new("test/snapshot.xxx.tar.gz");
 
         let result = snapshot_uploader

mithril-aggregator/src/tools/mod.rs

@@ -12,15 +12,12 @@ pub use certificates_hash_migrator::CertificatesHashMigrator;
 pub use digest_helpers::extract_digest_from_path;
 pub use era::EraTools;
 pub use genesis::{GenesisTools, GenesisToolsDependency};
-pub use remote_file_uploader::{GcpFileUploader, RemoteFileUploader};
+pub use remote_file_uploader::GcpFileUploader;
 pub use signer_importer::{
     CExplorerSignerRetriever, SignersImporter, SignersImporterPersister, SignersImporterRetriever,
 };
 pub use single_signature_authenticator::*;
 
-#[cfg(test)]
-pub use remote_file_uploader::MockRemoteFileUploader;
-
 /// Downcast the error to the specified error type and check if the error satisfies the condition.
 pub(crate) fn downcast_check<E>(
     error: &mithril_common::StdError,

mithril-aggregator/src/tools/remote_file_uploader.rs

@@ -11,33 +11,41 @@ use tokio_util::{codec::BytesCodec, codec::FramedRead};
 use mithril_common::logging::LoggerExtensions;
 use mithril_common::StdResult;
 
-/// RemoteFileUploader represents a remote file uploader interactor
-#[cfg_attr(test, mockall::automock)]
-#[async_trait]
-pub trait RemoteFileUploader: Sync + Send {
-    /// Upload a snapshot
-    async fn upload_file(&self, filepath: &Path) -> StdResult<()>;
-}
+use crate::file_uploaders::FileLocation;
+use crate::FileUploader;
 
 /// GcpFileUploader represents a Google Cloud Platform file uploader interactor
 pub struct GcpFileUploader {
     bucket: String,
+    use_cdn_domain: bool,
     logger: Logger,
 }
 
 impl GcpFileUploader {
     /// GcpFileUploader factory
-    pub fn new(bucket: String, logger: Logger) -> Self {
+    pub fn new(bucket: String, use_cdn_domain: bool, logger: Logger) -> Self {
         Self {
             bucket,
+            use_cdn_domain,
             logger: logger.new_with_component_name::<Self>(),
         }
     }
+
+    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
+            )
+        }
+    }
 }
 
 #[async_trait]
-impl RemoteFileUploader for GcpFileUploader {
-    async fn upload_file(&self, filepath: &Path) -> StdResult<()> {
+impl FileUploader for GcpFileUploader {
+    async fn upload(&self, filepath: &Path) -> StdResult<FileLocation> {
         if env::var("GOOGLE_APPLICATION_CREDENTIALS_JSON").is_err() {
             return Err(anyhow!(
                 "Missing GOOGLE_APPLICATION_CREDENTIALS_JSON environment variable".to_string()
@@ -85,6 +93,48 @@ impl RemoteFileUploader for GcpFileUploader {
 
         info!(self.logger, "Updated acl for {filename}");
 
-        Ok(())
+        Ok(self.get_location(filename))
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::test_tools::TestLogger;
+
+    use super::*;
+
+    #[tokio::test]
+    async fn get_location_not_using_cdn_domain_return_google_api_uri() {
+        let use_cdn_domain = false;
+
+        let file_uploader = GcpFileUploader::new(
+            "cardano-testnet".to_string(),
+            use_cdn_domain,
+            TestLogger::stdout(),
+        );
+        let filename = "snapshot.xxx.tar.gz";
+        let expected_location =
+            "https://storage.googleapis.com/cardano-testnet/snapshot.xxx.tar.gz".to_string();
+
+        let location = file_uploader.get_location(filename);
+
+        assert_eq!(expected_location, location);
+    }
+
+    #[tokio::test]
+    async fn get_location_using_cdn_domain_return_cdn_in_uri() {
+        let use_cdn_domain = true;
+
+        let file_uploader = GcpFileUploader::new(
+            "cdn.mithril.network".to_string(),
+            use_cdn_domain,
+            TestLogger::stdout(),
+        );
+        let filename = "snapshot.xxx.tar.gz";
+        let expected_location = "https://cdn.mithril.network/snapshot.xxx.tar.gz".to_string();
+
+        let location = file_uploader.get_location(filename);
+
+        assert_eq!(expected_location, location);
     }
 }