refactor: Use a FileUri struct instead of a String for the location

input-output-hk · Dec 11, 2024 · eaa1fca · eaa1fca
1 parent 8d5e1c7
commit eaa1fca
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 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
@@ -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<Vec<FileLocation>> {
+    ) -> StdResult<Vec<FileUri>> {
         debug!(self.logger, ">> upload_snapshot_archive");
         let location = self
             .snapshot_uploader
@@ -157,7 +155,12 @@ impl ArtifactBuilder<CardanoDbBeacon, Snapshot> 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
@@ -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<Option<String>>,
+    last_uploaded: RwLock<Option<FileUri>>,
 }
 
 impl DumbUploader {
@@ -22,13 +22,13 @@ impl DumbUploader {
     }
 
     /// Return the last upload that was triggered.
-    pub fn get_last_upload(&self) -> StdResult<Option<String>> {
+    pub fn get_last_upload(&self) -> StdResult<Option<FileUri>> {
         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<FileLocation> {
+    async fn upload(&self, filepath: &Path) -> StdResult<FileUri> {
         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
@@ -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<FileLocation> {
+    async fn upload(&self, filepath: &Path) -> StdResult<FileUri> {
         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
@@ -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<FileUri> 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<FileLocation>;
+    async fn upload(&self, filepath: &Path) -> StdResult<FileUri>;
 }
diff --git 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::{FileUri, FileUploader};
 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<FileLocation> {
+    async fn upload(&self, filepath: &Path) -> StdResult<FileUri> {
         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::{FileUri, FileUploader};
     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
@@ -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;