Validate repository URL eagerly

CHANGELOG.adoc

@@ -2,6 +2,12 @@
 
 == 0.6.0 - unreleased
 
+:262: https://github.com/stackabletech/agent/pull/262[#262]
+
+=== Changed
+* Lazy validation of repository URLs changed to eager validation
+  ({262}).
+
 == 0.5.0 - 2021-07-26
 
 :224: https://github.com/stackabletech/agent/pull/224[#224]

src/provider/repository/stackablerepository.rs

@@ -18,7 +18,7 @@ use crate::provider::repository::repository_spec::Repository;
 
 #[derive(Debug, Clone)]
 pub struct StackableRepoProvider {
-    base_url: Url,
+    metadata_url: Url,
     pub name: String,
     content: Option<RepositoryContent>,
 }
@@ -51,14 +51,18 @@ struct StackablePackage {
 }
 
 impl StackableRepoProvider {
-    // This is only used in a test case and hence warned about as dead code
-    #[allow(dead_code)]
-    pub fn new(name: String, base_url: String) -> Result<StackableRepoProvider, StackableError> {
-        let base_url = Url::parse(&base_url)?;
+    pub fn new(name: &str, base_url: &Url) -> Result<StackableRepoProvider, StackableError> {
+        let mut metadata_url = base_url.to_owned();
+
+        metadata_url
+            .path_segments_mut()
+            .map_err(|_| StackableError::RepositoryConversionError)?
+            .pop_if_empty()
+            .push("metadata.json");
 
         Ok(StackableRepoProvider {
-            base_url,
-            name,
+            metadata_url,
+            name: String::from(name),
             content: None,
         })
     }
@@ -69,7 +73,7 @@ impl StackableRepoProvider {
     ) -> Result<bool, StackableError> {
         debug!(
             "Starting metadata refresh for repository of type {} at location {}",
-            "StackableRepo", self.base_url
+            "StackableRepo", self.metadata_url
         );
         let package = package.into();
         let metadata = self.get_repo_metadata().await?;
@@ -119,17 +123,13 @@ impl StackableRepoProvider {
 
     async fn get_repo_metadata(&mut self) -> Result<RepositoryContent, StackableError> {
         trace!("entering get_repo_metadata");
-        let mut metadata_url = self.base_url.clone();
-
-        // path_segments_mut returns () in an error case, not sure how to handle this
-        metadata_url
-            .path_segments_mut()
-            .expect("")
-            .push("metadata.json");
 
-        debug!("Retrieving repository metadata from {}", metadata_url);
+        debug!("Retrieving repository metadata from {}", self.metadata_url);
 
-        let repo_data = reqwest::get(metadata_url).await?.json::<RepoData>().await?;
+        let repo_data = reqwest::get(self.metadata_url.clone())
+            .await?
+            .json::<RepoData>()
+            .await?;
 
         debug!("Got repository metadata: {:?}", repo_data);
 
@@ -170,7 +170,7 @@ impl StackableRepoProvider {
             // return it unchanged
             return Ok(path);
         }
-        let resolved_path = self.base_url.join(&path)?;
+        let resolved_path = self.metadata_url.join(&path)?;
         Ok(resolved_path.as_str().to_string())
     }
 }
@@ -185,16 +185,18 @@ impl TryFrom<&Repository> for StackableRepoProvider {
     type Error = StackableError;
 
     fn try_from(value: &Repository) -> Result<Self, Self::Error> {
-        let properties: HashMap<String, String> = value.clone().spec.properties;
-        let path = properties.get("url");
-        if let Some(valid_path) = path {
-            return Ok(StackableRepoProvider {
-                name: Meta::name(value),
-                base_url: Url::parse(valid_path)?,
-                content: None,
-            });
-        }
-        Err(StackableError::RepositoryConversionError)
+        let name = Meta::name(value);
+
+        let base_url = value
+            .spec
+            .properties
+            .get("url")
+            .and_then(|url| Url::parse(url).ok())
+            .ok_or(StackableError::RepositoryConversionError)?;
+
+        let stackable_repo_provider = StackableRepoProvider::new(&name, &base_url)?;
+
+        Ok(stackable_repo_provider)
     }
 }
 
@@ -214,15 +216,51 @@ impl Hash for StackableRepoProvider {
 
 #[cfg(test)]
 mod tests {
-    use crate::provider::repository::repository_spec::{Repository, RepositorySpec};
-    use crate::provider::repository::stackablerepository::StackableRepoProvider;
-    use std::collections::HashMap;
-    use std::convert::TryFrom;
+    use super::*;
+
+    use crate::provider::repository::repository_spec::RepositorySpec;
+
+    #[test]
+    fn stackable_repo_provider_should_be_created_from_a_valid_url_with_a_trailing_slash() {
+        let actual =
+            StackableRepoProvider::new("test", &Url::parse("http://localhost:8000/repo/").unwrap())
+                .unwrap();
+
+        assert_eq!(
+            Url::parse("http://localhost:8000/repo/metadata.json").unwrap(),
+            actual.metadata_url
+        );
+        assert_eq!(String::from("test"), actual.name);
+        assert!(actual.content.is_none());
+    }
+
+    #[test]
+    fn stackable_repo_provider_should_be_created_from_a_valid_url_without_a_trailing_slash() {
+        let actual =
+            StackableRepoProvider::new("test", &Url::parse("http://localhost:8000/repo").unwrap())
+                .unwrap();
+
+        assert_eq!(
+            Url::parse("http://localhost:8000/repo/metadata.json").unwrap(),
+            actual.metadata_url
+        );
+        assert_eq!(String::from("test"), actual.name);
+        assert!(actual.content.is_none());
+    }
+
+    #[test]
+    fn stackable_repo_provider_should_not_be_created_from_an_url_which_cannot_be_a_base() {
+        assert!(StackableRepoProvider::new(
+            "test",
+            &Url::parse("mailto:info@stackable.de").unwrap()
+        )
+        .is_err());
+    }
 
     #[test]
     fn test_url_functions() {
         let repo =
-            StackableRepoProvider::new(String::from("test"), String::from("http://localhost:8000"))
+            StackableRepoProvider::new("test", &Url::parse("http://localhost:8000").unwrap())
                 .unwrap();
 
         // Check that a relative URL is correctly resolved against the repo's baseurl
@@ -256,8 +294,8 @@ mod tests {
         let converted_repo = StackableRepoProvider::try_from(&test_repo_crd).unwrap();
         assert_eq!(converted_repo.name, "test");
         assert_eq!(
-            converted_repo.base_url.as_str(),
-            "http://monitoring.stackable.demo:8000/"
+            converted_repo.metadata_url.as_str(),
+            "http://monitoring.stackable.demo:8000/metadata.json"
         );
     }
 }