From 1b99064b0092ba9b6c20b16ecf4b2b6e4525a022 Mon Sep 17 00:00:00 2001 From: Aiden Fujiwara Date: Mon, 11 Sep 2023 14:30:30 -0700 Subject: [PATCH 1/2] manual deserialization of toml into toolspec struct --- src/config.rs | 296 ++++++++++++++---- src/error.rs | 19 +- src/tool_cache.rs | 2 +- src/tool_provider/mod.rs | 3 +- .../install_invalid_tool_configuration.snap | 7 +- 5 files changed, 261 insertions(+), 66 deletions(-) diff --git a/src/config.rs b/src/config.rs index e9cdf15..ebf6f93 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,81 +1,197 @@ -use std::{collections::BTreeMap, env, fmt}; - +use crate::{ + ci_string::CiString, + error::{ConfigFileParseError, ConfigFileParseResult, ForemanError}, + fs, + paths::ForemanPaths, + tool_provider::Provider, +}; use semver::VersionReq; use serde::{Deserialize, Serialize}; - -use crate::{ - ci_string::CiString, error::ForemanError, fs, paths::ForemanPaths, tool_provider::Provider, +use std::{ + collections::{BTreeMap, HashMap}, + env, fmt, }; - +use toml::Value; #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -#[serde(untagged)] -pub enum ToolSpec { - Github { - // alias to `source` for backward compatibility - #[serde(alias = "source")] - github: String, - version: VersionReq, - }, - Gitlab { - gitlab: String, - version: VersionReq, - }, +pub struct ToolSpec { + host: String, + path: String, + version: VersionReq, + protocol: Protocol, +} +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub enum Protocol { + Github, + Gitlab, } impl ToolSpec { - pub fn cache_key(&self) -> CiString { - match self { - ToolSpec::Github { github, .. } => CiString(github.clone()), - ToolSpec::Gitlab { gitlab, .. } => CiString(format!("gitlab@{}", gitlab)), + pub fn from_value( + value: &Value, + host_map: &HashMap, + ) -> ConfigFileParseResult { + if let Value::Table(mut map) = value.clone() { + let version_value = + map.remove("version") + .ok_or_else(|| ConfigFileParseError::Tool { + tool: value.to_string(), + })?; + let version_str = version_value + .as_str() + .ok_or_else(|| ConfigFileParseError::Tool { + tool: value.to_string(), + })?; + let version = + VersionReq::parse(version_str).map_err(|_| ConfigFileParseError::Tool { + tool: value.to_string(), + })?; + + let (path_val, host_source) = host_map + .iter() + .find_map(|(potential_host, host_source)| { + if let Some(path) = map.remove(potential_host) { + Some((path, host_source)) + } else { + None + } + }) + .ok_or_else(|| ConfigFileParseError::Tool { + tool: value.to_string(), + })?; + + // Extraneous fields should in a tool spec definition should not be allowed + if !map.is_empty() { + return Err(ConfigFileParseError::Tool { + tool: value.to_string(), + }); + } + + let host = host_source.source.to_string(); + let path = path_val + .as_str() + .ok_or_else(|| ConfigFileParseError::Tool { + tool: value.to_string(), + })? + .to_string(); + + let protocol = host_source.protocol.clone(); + + Ok(Self { + host, + path, + version, + protocol, + }) + } else { + Err(ConfigFileParseError::Tool { + tool: value.to_string(), + }) } } - pub fn source(&self) -> &str { - match self { - ToolSpec::Github { github: source, .. } | ToolSpec::Gitlab { gitlab: source, .. } => { - source - } + pub fn cache_key(&self) -> CiString { + match self.protocol { + Protocol::Github => CiString(format!("{}", self.path)), + Protocol::Gitlab => CiString(format!("gitlab@{}", self.path)), } } + pub fn source(&self) -> String { + let provider = match self.protocol { + Protocol::Github => "github.com", + Protocol::Gitlab => "gitlab.com", + }; + + format!("{}/{}", provider, self.path) + } + + pub fn path(&self) -> &str { + self.path.as_str() + } + pub fn version(&self) -> &VersionReq { - match self { - ToolSpec::Github { version, .. } | ToolSpec::Gitlab { version, .. } => version, - } + &self.version } pub fn provider(&self) -> Provider { - match self { - ToolSpec::Github { .. } => Provider::Github, - ToolSpec::Gitlab { .. } => Provider::Gitlab, + match self.protocol { + Protocol::Github => Provider::Github, + Protocol::Gitlab => Provider::Gitlab, } } } impl fmt::Display for ToolSpec { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!( - f, - "{}.com/{}@{}", - match self { - ToolSpec::Github { .. } => "github", - ToolSpec::Gitlab { .. } => "gitlab", - }, - self.source(), - self.version(), - ) + write!(f, "{}@{}", self.source(), self.version()) } } #[derive(Debug, Serialize, Deserialize)] pub struct ConfigFile { pub tools: BTreeMap, + pub hosts: HashMap, +} + +#[derive(Debug, Serialize, Deserialize, PartialEq)] +pub struct Host { + source: String, + protocol: Protocol, +} + +impl Host { + pub fn new(source: String, protocol: Protocol) -> Self { + Self { source, protocol } + } } impl ConfigFile { - pub fn new() -> Self { + pub fn new_with_defaults() -> Self { Self { tools: BTreeMap::new(), + hosts: HashMap::from([ + ( + "source".to_string(), + Host::new("https://github.com".to_string(), Protocol::Github), + ), + ( + "github".to_string(), + Host::new("https://github.com".to_string(), Protocol::Github), + ), + ( + "gitlab".to_string(), + Host::new("https://gitlab.com".to_string(), Protocol::Gitlab), + ), + ]), + } + } + + pub fn from_value(value: Value) -> ConfigFileParseResult { + let mut config = ConfigFile::new_with_defaults(); + + if let Value::Table(top_level) = &value { + if let Some(tools) = &top_level.get("tools") { + if let Value::Table(tools) = tools { + for (tool, toml) in tools { + let tool_spec = + ToolSpec::from_value(&toml, &config.hosts).map_err(|_| { + ConfigFileParseError::Tool { + tool: value.to_string(), + } + })?; + config.tools.insert(tool.to_owned(), tool_spec); + } + } + } else { + return Err(ConfigFileParseError::MissingField { + field: "tools".to_string(), + }); + } + Ok(config) + } else { + Err(ConfigFileParseError::Tool { + tool: value.to_string(), + }) } } @@ -83,10 +199,14 @@ impl ConfigFile { for (tool_name, tool_source) in other.tools { self.tools.entry(tool_name).or_insert(tool_source); } + + for (host_name, host_source) in other.hosts { + self.hosts.entry(host_name).or_insert(host_source); + } } pub fn aggregate(paths: &ForemanPaths) -> Result { - let mut config = ConfigFile::new(); + let mut config = ConfigFile::new_with_defaults(); let base_dir = env::current_dir().map_err(|err| { ForemanError::io_error_with_context( @@ -102,11 +222,9 @@ impl ConfigFile { if let Some(contents) = fs::try_read(&config_path)? { let config_source = toml::from_slice(&contents) .map_err(|err| ForemanError::config_parsing(&config_path, err.to_string()))?; - log::debug!( - "aggregating content from config file at {}", - config_path.display() - ); - config.fill_from(config_source); + let new_config = ConfigFile::from_value(config_source) + .map_err(|err| ForemanError::config_parsing(&config_path, err.to_string()))?; + config.fill_from(new_config); } if let Some(parent) = current_dir.parent() { @@ -120,11 +238,13 @@ impl ConfigFile { if let Some(contents) = fs::try_read(&home_config_path)? { let config_source = toml::from_slice(&contents) .map_err(|err| ForemanError::config_parsing(&home_config_path, err.to_string()))?; + let new_config = ConfigFile::from_value(config_source) + .map_err(|err| ForemanError::config_parsing(&home_config_path, err.to_string()))?; log::debug!( "aggregating content from config file at {}", home_config_path.display() ); - config.fill_from(config_source); + config.fill_from(new_config); } Ok(config) @@ -146,16 +266,20 @@ mod test { use super::*; fn new_github>(github: S, version: VersionReq) -> ToolSpec { - ToolSpec::Github { - github: github.into(), - version, + ToolSpec { + host: "https://github.com".to_string(), + path: github.into(), + version: version, + protocol: Protocol::Github, } } - fn new_gitlab>(github: S, version: VersionReq) -> ToolSpec { - ToolSpec::Gitlab { - gitlab: github.into(), - version, + fn new_gitlab>(gitlab: S, version: VersionReq) -> ToolSpec { + ToolSpec { + host: "https://gitlab.com".to_string(), + path: gitlab.into(), + version: version, + protocol: Protocol::Gitlab, } } @@ -163,32 +287,82 @@ mod test { VersionReq::parse(string).unwrap() } + fn default_host() -> HashMap { + HashMap::from([ + ( + "source".to_string(), + Host::new("https://github.com".to_string(), Protocol::Github), + ), + ( + "github".to_string(), + Host::new("https://github.com".to_string(), Protocol::Github), + ), + ( + "gitlab".to_string(), + Host::new("https://gitlab.com".to_string(), Protocol::Gitlab), + ), + ]) + } + mod deserialization { use super::*; #[test] fn github_from_source_field() { - let github: ToolSpec = + let value: Value = toml::from_str(&[r#"source = "user/repo""#, r#"version = "0.1.0""#].join("\n")) .unwrap(); + let github = ToolSpec::from_value(&value, &default_host()).unwrap(); + + dbg!("{github}"); assert_eq!(github, new_github("user/repo", version("0.1.0"))); } #[test] fn github_from_github_field() { - let github: ToolSpec = + let value: Value = toml::from_str(&[r#"github = "user/repo""#, r#"version = "0.1.0""#].join("\n")) .unwrap(); + let github = ToolSpec::from_value(&value, &default_host()).unwrap(); assert_eq!(github, new_github("user/repo", version("0.1.0"))); } #[test] fn gitlab_from_gitlab_field() { - let gitlab: ToolSpec = + let value: Value = toml::from_str(&[r#"gitlab = "user/repo""#, r#"version = "0.1.0""#].join("\n")) .unwrap(); + let gitlab = ToolSpec::from_value(&value, &default_host()).unwrap(); assert_eq!(gitlab, new_gitlab("user/repo", version("0.1.0"))); } + + #[test] + fn extraneous_fields_tools() { + let value: Value = toml::from_str( + &[ + r#"github = "Roblox/rotriever""#, + r#"path = "Roblox/rotriever""#, + r#"version = "0.5.4""#, + ] + .join("\n"), + ) + .unwrap(); + + let artifactory = ToolSpec::from_value(&value, &default_host()).unwrap_err(); + assert_eq!( + artifactory, + ConfigFileParseError::Tool { + tool: [ + r#"github = "Roblox/rotriever""#, + r#"path = "Roblox/rotriever""#, + r#"version = "0.5.4""#, + r#""#, + ] + .join("\n") + .to_string() + } + ) + } } #[test] diff --git a/src/error.rs b/src/error.rs index 4bc9c6e..12ddb2c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -5,7 +5,7 @@ use semver::Version; use crate::config::{ConfigFile, ToolSpec}; pub type ForemanResult = Result; - +pub type ConfigFileParseResult = Result; #[derive(Debug)] pub enum ForemanError { IO { @@ -73,6 +73,12 @@ pub enum ForemanError { }, } +#[derive(Debug, PartialEq)] +pub enum ConfigFileParseError { + MissingField { field: String }, + Tool { tool: String }, +} + impl ForemanError { pub fn io_error_with_context>(source: io::Error, message: S) -> Self { Self::IO { @@ -312,6 +318,17 @@ impl fmt::Display for ForemanError { } } +impl fmt::Display for ConfigFileParseError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::MissingField { field } => write!(f, "missing field `{}`", field), + Self::Tool { tool } => { + write!(f, "data is not properly formatted for tool:\n\n{}", tool) + } + } + } +} + const FOREMAN_CONFIG_HELP: &str = r#"A Foreman configuration file looks like this: [tools] # list the tools you want to install under this header diff --git a/src/tool_cache.rs b/src/tool_cache.rs index 56ebfa9..3379fce 100644 --- a/src/tool_cache.rs +++ b/src/tool_cache.rs @@ -107,7 +107,7 @@ impl ToolCache { log::info!("Downloading {}", tool); let provider = providers.get(&tool.provider()); - let releases = provider.get_releases(tool.source())?; + let releases = provider.get_releases(tool.path())?; // Filter down our set of releases to those that are valid versions and // have release assets for our current platform. diff --git a/src/tool_provider/mod.rs b/src/tool_provider/mod.rs index 8383720..98ab790 100644 --- a/src/tool_provider/mod.rs +++ b/src/tool_provider/mod.rs @@ -3,11 +3,10 @@ mod gitlab; use std::{collections::HashMap, fmt}; +use crate::{error::ForemanResult, paths::ForemanPaths}; use github::GithubProvider; use gitlab::GitlabProvider; -use crate::{error::ForemanResult, paths::ForemanPaths}; - pub trait ToolProviderImpl: fmt::Debug { fn get_releases(&self, repo: &str) -> ForemanResult>; diff --git a/tests/snapshots/install_invalid_tool_configuration.snap b/tests/snapshots/install_invalid_tool_configuration.snap index fd6f21b..c11017a 100644 --- a/tests/snapshots/install_invalid_tool_configuration.snap +++ b/tests/snapshots/install_invalid_tool_configuration.snap @@ -4,7 +4,12 @@ assertion_line: 101 expression: content --- -unable to parse Foreman configuration file (at {{CWD}}foreman.toml): data did not match any variant of untagged enum ToolSpec for key `tools.tool` at line 2 column 1 +unable to parse Foreman configuration file (at {{CWD}}foreman.toml): data is not properly formatted for tool: + +[tools.tool] +invalid = "roblox/tooling" +version = "0.0.0" + A Foreman configuration file looks like this: From 8b2a2a3306a062ef1f654605b0324602e4f16df4 Mon Sep 17 00:00:00 2001 From: Aiden Fujiwara Date: Tue, 12 Sep 2023 09:45:35 -0700 Subject: [PATCH 2/2] nits --- src/config.rs | 57 ++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/src/config.rs b/src/config.rs index ebf6f93..8458c59 100644 --- a/src/config.rs +++ b/src/config.rs @@ -6,20 +6,23 @@ use crate::{ tool_provider::Provider, }; use semver::VersionReq; -use serde::{Deserialize, Serialize}; use std::{ collections::{BTreeMap, HashMap}, env, fmt, }; use toml::Value; -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] + +const GITHUB: &'static str = "https://github.com"; +const GITLAB: &'static str = "https://gitlab.com"; + +#[derive(Debug, Clone, PartialEq)] pub struct ToolSpec { host: String, path: String, version: VersionReq, protocol: Protocol, } -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq)] pub enum Protocol { Github, Gitlab, @@ -59,7 +62,7 @@ impl ToolSpec { tool: value.to_string(), })?; - // Extraneous fields should in a tool spec definition should not be allowed + // Extraneous fields in a tool spec definition should not be allowed if !map.is_empty() { return Err(ConfigFileParseError::Tool { tool: value.to_string(), @@ -127,21 +130,24 @@ impl fmt::Display for ToolSpec { } } -#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug)] pub struct ConfigFile { pub tools: BTreeMap, pub hosts: HashMap, } -#[derive(Debug, Serialize, Deserialize, PartialEq)] +#[derive(Debug, PartialEq)] pub struct Host { source: String, protocol: Protocol, } impl Host { - pub fn new(source: String, protocol: Protocol) -> Self { - Self { source, protocol } + pub fn new>(source: S, protocol: Protocol) -> Self { + Self { + source: source.into(), + protocol, + } } } @@ -150,18 +156,9 @@ impl ConfigFile { Self { tools: BTreeMap::new(), hosts: HashMap::from([ - ( - "source".to_string(), - Host::new("https://github.com".to_string(), Protocol::Github), - ), - ( - "github".to_string(), - Host::new("https://github.com".to_string(), Protocol::Github), - ), - ( - "gitlab".to_string(), - Host::new("https://gitlab.com".to_string(), Protocol::Gitlab), - ), + ("source".to_string(), Host::new(GITHUB, Protocol::Github)), + ("github".to_string(), Host::new(GITHUB, Protocol::Github)), + ("gitlab".to_string(), Host::new(GITLAB, Protocol::Gitlab)), ]), } } @@ -267,7 +264,7 @@ mod test { fn new_github>(github: S, version: VersionReq) -> ToolSpec { ToolSpec { - host: "https://github.com".to_string(), + host: GITHUB.to_string(), path: github.into(), version: version, protocol: Protocol::Github, @@ -276,7 +273,7 @@ mod test { fn new_gitlab>(gitlab: S, version: VersionReq) -> ToolSpec { ToolSpec { - host: "https://gitlab.com".to_string(), + host: GITLAB.to_string(), path: gitlab.into(), version: version, protocol: Protocol::Gitlab, @@ -287,19 +284,19 @@ mod test { VersionReq::parse(string).unwrap() } - fn default_host() -> HashMap { + fn default_hosts() -> HashMap { HashMap::from([ ( "source".to_string(), - Host::new("https://github.com".to_string(), Protocol::Github), + Host::new(GITHUB.to_string(), Protocol::Github), ), ( "github".to_string(), - Host::new("https://github.com".to_string(), Protocol::Github), + Host::new(GITHUB.to_string(), Protocol::Github), ), ( "gitlab".to_string(), - Host::new("https://gitlab.com".to_string(), Protocol::Gitlab), + Host::new(GITLAB.to_string(), Protocol::Gitlab), ), ]) } @@ -312,7 +309,7 @@ mod test { let value: Value = toml::from_str(&[r#"source = "user/repo""#, r#"version = "0.1.0""#].join("\n")) .unwrap(); - let github = ToolSpec::from_value(&value, &default_host()).unwrap(); + let github = ToolSpec::from_value(&value, &default_hosts()).unwrap(); dbg!("{github}"); assert_eq!(github, new_github("user/repo", version("0.1.0"))); @@ -323,7 +320,7 @@ mod test { let value: Value = toml::from_str(&[r#"github = "user/repo""#, r#"version = "0.1.0""#].join("\n")) .unwrap(); - let github = ToolSpec::from_value(&value, &default_host()).unwrap(); + let github = ToolSpec::from_value(&value, &default_hosts()).unwrap(); assert_eq!(github, new_github("user/repo", version("0.1.0"))); } @@ -332,7 +329,7 @@ mod test { let value: Value = toml::from_str(&[r#"gitlab = "user/repo""#, r#"version = "0.1.0""#].join("\n")) .unwrap(); - let gitlab = ToolSpec::from_value(&value, &default_host()).unwrap(); + let gitlab = ToolSpec::from_value(&value, &default_hosts()).unwrap(); assert_eq!(gitlab, new_gitlab("user/repo", version("0.1.0"))); } @@ -348,7 +345,7 @@ mod test { ) .unwrap(); - let artifactory = ToolSpec::from_value(&value, &default_host()).unwrap_err(); + let artifactory = ToolSpec::from_value(&value, &default_hosts()).unwrap_err(); assert_eq!( artifactory, ConfigFileParseError::Tool {