From 806cf4a4a08786a8602e6a2a8d0f19f80dc5ab7a Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Mon, 16 Aug 2021 17:07:32 +0200 Subject: [PATCH] Validate repository URL eagerly The validation of the repository URL is already performed when a StackableRepoProvider is created and not only when the repository content is requested. --- CHANGELOG.adoc | 6 + .../repository/stackablerepository.rs | 108 ++++++++++++------ 2 files changed, 79 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 3d82bc2f..9c6fc587 100644 --- a/CHANGELOG.adoc +++ b/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] diff --git a/src/provider/repository/stackablerepository.rs b/src/provider/repository/stackablerepository.rs index cc07f4d6..e666dbd6 100644 --- a/src/provider/repository/stackablerepository.rs +++ b/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, } @@ -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 { - let base_url = Url::parse(&base_url)?; + pub fn new(name: &str, base_url: &Url) -> Result { + 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 { 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 { 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::().await?; + let repo_data = reqwest::get(self.metadata_url.clone()) + .await? + .json::() + .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 { - let properties: HashMap = 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" ); } }