From 162955d20deaea83981ecf7881eb434645e7092c Mon Sep 17 00:00:00 2001 From: OverHash <46231745+OverHash@users.noreply.github.com> Date: Wed, 14 Dec 2022 23:17:23 +1300 Subject: [PATCH 1/5] Implement anti-duplicate parsing for config.tools During deserialization, ensures that there are no duplicate entries in config.tools. By default, serde will overwrite duplicate entries when deserializing to a HashMap. In almost all cases, a user would be incorrectly doing so accidentally, so we now throw an error when parsing the configuration file. --- src/config.rs | 91 ++++++++++++++++++++++++++++++++++++++++++++++++--- src/main.rs | 2 +- 2 files changed, 87 insertions(+), 6 deletions(-) diff --git a/src/config.rs b/src/config.rs index c5650ee..a245207 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,7 +1,14 @@ -use std::{collections::HashMap, env, fmt}; +use std::{ + collections::HashMap, + env, fmt, + ops::{Deref, DerefMut}, +}; use semver::VersionReq; -use serde::{Deserialize, Serialize}; +use serde::{ + de::{self, MapAccess, Visitor}, + Deserialize, Serialize, +}; use crate::{ ci_string::CiString, error::ForemanError, fs, paths::ForemanPaths, tool_provider::Provider, @@ -67,20 +74,43 @@ impl fmt::Display for ToolSpec { } } +#[derive(Debug, Serialize)] +pub struct ConfigFileTools(HashMap); + +impl ConfigFileTools { + pub fn new() -> ConfigFileTools { + Self(HashMap::new()) + } +} + +impl Deref for ConfigFileTools { + type Target = HashMap; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for ConfigFileTools { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + #[derive(Debug, Serialize, Deserialize)] pub struct ConfigFile { - pub tools: HashMap, + pub tools: ConfigFileTools, } impl ConfigFile { pub fn new() -> Self { Self { - tools: HashMap::new(), + tools: ConfigFileTools::new(), } } fn fill_from(&mut self, other: ConfigFile) { - for (tool_name, tool_source) in other.tools { + for (tool_name, tool_source) in other.tools.0 { self.tools.entry(tool_name).or_insert(tool_source); } } @@ -141,6 +171,46 @@ impl fmt::Display for ConfigFile { } } +struct ConfigFileVisitor; + +impl<'de> Visitor<'de> for ConfigFileVisitor { + type Value = ConfigFileTools; + + fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + formatter.write_str("a map with non-duplicate keys") + } + + fn visit_map(self, mut map: A) -> Result + where + A: MapAccess<'de>, + { + let mut tools = HashMap::new(); + + while let Some((key, value)) = map.next_entry()? { + if tools.contains_key(&key) { + // item already existed inside the config + // throw an error as this is unlikely to be the users intention + return Err(de::Error::custom(format!("duplicate tool `{key}`"))); + } + + tools.insert(key, value); + } + + Ok(ConfigFileTools(tools)) + } +} + +impl<'de> Deserialize<'de> for ConfigFileTools { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let tools = deserializer.deserialize_map(ConfigFileVisitor)?; + + Ok(tools) + } +} + #[cfg(test)] mod test { use super::*; @@ -189,6 +259,17 @@ mod test { .unwrap(); assert_eq!(gitlab, new_gitlab("user/repo", version("0.1.0"))); } + + #[test] + fn duplicate_tools() { + let err = toml::from_str::( + r#"tool = { github = "user/repo", version = "0.1.0" } + tool = { github = "user2/repo2", version = "0.2.0" }"#, + ) + .unwrap_err(); + + assert_eq!(err.to_string(), "duplicate tool `tool` at line 1 column 1"); + } } #[test] diff --git a/src/main.rs b/src/main.rs index 0eae111..d452c17 100644 --- a/src/main.rs +++ b/src/main.rs @@ -197,7 +197,7 @@ fn actual_main(paths: ForemanPaths) -> ForemanResult<()> { let mut cache = ToolCache::load(&paths)?; - for (tool_alias, tool_spec) in &config.tools { + for (tool_alias, tool_spec) in config.tools.iter() { let providers = ToolProvider::new(&paths); cache.download_if_necessary(tool_spec, &providers)?; add_self_alias(tool_alias, &paths.bin_dir())?; From b745481a075f81a9a2cc7d97d6fd552ceb3d9936 Mon Sep 17 00:00:00 2001 From: OverHash <46231745+OverHash@users.noreply.github.com> Date: Thu, 15 Dec 2022 10:58:36 +1300 Subject: [PATCH 2/5] Improve duplicate tool error Co-authored-by: oltrep <34498770+oltrep@users.noreply.github.com> --- src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index a245207..f613142 100644 --- a/src/config.rs +++ b/src/config.rs @@ -190,7 +190,7 @@ impl<'de> Visitor<'de> for ConfigFileVisitor { if tools.contains_key(&key) { // item already existed inside the config // throw an error as this is unlikely to be the users intention - return Err(de::Error::custom(format!("duplicate tool `{key}`"))); + return Err(de::Error::custom(format!("duplicate tool name `{key}` found"))); } tools.insert(key, value); From f8bdc6bde2962bb2f9d2cf42b9e84235a114e989 Mon Sep 17 00:00:00 2001 From: OverHash <46231745+OverHash@users.noreply.github.com> Date: Thu, 15 Dec 2022 11:18:40 +1300 Subject: [PATCH 3/5] Add more test cases for duplicate tool definitions --- src/config.rs | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/src/config.rs b/src/config.rs index f613142..a975109 100644 --- a/src/config.rs +++ b/src/config.rs @@ -190,7 +190,9 @@ impl<'de> Visitor<'de> for ConfigFileVisitor { if tools.contains_key(&key) { // item already existed inside the config // throw an error as this is unlikely to be the users intention - return Err(de::Error::custom(format!("duplicate tool name `{key}` found"))); + return Err(de::Error::custom(format!( + "duplicate tool name `{key}` found" + ))); } tools.insert(key, value); @@ -268,7 +270,34 @@ mod test { ) .unwrap_err(); - assert_eq!(err.to_string(), "duplicate tool `tool` at line 1 column 1"); + assert_eq!( + err.to_string(), + "duplicate tool name `tool` found at line 1 column 1" + ); + + let err = toml::from_str::( + r#"tool_a = { github = "user/a", version = "0.1.0" } + tool_b = { github = "user/b", version = "0.2.0" } + tool_a = { gitlab = "user/c", version = "0.3.0" }"#, + ) + .unwrap_err(); + + assert_eq!( + err.to_string(), + "duplicate tool name `tool_a` found at line 1 column 1" + ); + + let err = toml::from_str::( + r#"tool_b = { github = "user/b", version = "0.1.0" } + tool_a = { github = "user/a", version = "0.2.0" } + tool_a = { gitlab = "user/c", version = "0.3.0" }"#, + ) + .unwrap_err(); + + assert_eq!( + err.to_string(), + "duplicate tool name `tool_a` found at line 1 column 1" + ); } } From 090743e973234c2b5fa470e8647e8d71418139c9 Mon Sep 17 00:00:00 2001 From: OverHash <46231745+OverHash@users.noreply.github.com> Date: Tue, 7 Feb 2023 11:27:00 +1300 Subject: [PATCH 4/5] Bump Rust CI to 1.58 Allows the support of named parameters, which is required for the anti-duplicate config --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0da09c8..339cd0b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -27,7 +27,7 @@ jobs: strategy: matrix: - rust_version: [stable, "1.53.0"] + rust_version: [stable, "1.58.0"] steps: - uses: actions/checkout@v2 From 7bdbf535ee02f4e1ceee0be08c164bfe23f1ce0e Mon Sep 17 00:00:00 2001 From: OverHash <46231745+OverHash@users.noreply.github.com> Date: Tue, 7 Feb 2023 11:30:06 +1300 Subject: [PATCH 5/5] Add anti-duplicate tool definition to CHANGELOG.md --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6704ae0..ba77e12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # Foreman Changelog +## Unreleased +- Errors when duplicate tools are defined in `foreman.toml` ([#65](https://github.com/Roblox/foreman/pull/65)) + ## 1.0.6 (2022-09-28) - Support `macos-aarch64` as an artifact name for Apple Silicon (arm64) binaries ([#60](https://github.com/Roblox/foreman/pull/60))