From aa739dc253e3766abbe655cbaf94d45de5325994 Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Fri, 21 Oct 2022 17:05:35 +0200 Subject: [PATCH 01/13] first stab (unpolished) --- e2e/tests-dfx/assetscanister.bash | 16 +++ .../frontend/ic-asset/src/asset_config.rs | 115 +++++++++++++----- src/canisters/frontend/ic-asset/src/sync.rs | 8 +- 3 files changed, 109 insertions(+), 30 deletions(-) diff --git a/e2e/tests-dfx/assetscanister.bash b/e2e/tests-dfx/assetscanister.bash index 15e55becc6..086976b9f4 100644 --- a/e2e/tests-dfx/assetscanister.bash +++ b/e2e/tests-dfx/assetscanister.bash @@ -539,3 +539,19 @@ CHERRIES" "$stdout" assert_not_match "etag: my-etag" assert_match "etag: \"[a-z0-9]{64}\"" } + +@test "asset configuration via .ic-assets.json5 - detect unused config" { + install_asset assetscanister + + dfx_start + + echo '[ + { + "match": "nevermatchme", + "cache": { "max_age": 2000 } + } + ]' > src/e2e_project_frontend/assets/.ic-assets.json5 + + dfx deploy + assert_contains "Unused" +} diff --git a/src/canisters/frontend/ic-asset/src/asset_config.rs b/src/canisters/frontend/ic-asset/src/asset_config.rs index d4265136dd..2f0ca7663a 100644 --- a/src/canisters/frontend/ic-asset/src/asset_config.rs +++ b/src/canisters/frontend/ic-asset/src/asset_config.rs @@ -4,40 +4,74 @@ use globset::{Glob, GlobMatcher}; use serde::{Deserialize, Serialize}; use serde_json::Value; use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, fs, + hash::{Hash,Hasher}, path::{Path, PathBuf}, - sync::Arc, + sync::{Arc, Mutex}, }; pub(crate) const ASSETS_CONFIG_FILENAME_JSON: &str = ".ic-assets.json"; pub(crate) const ASSETS_CONFIG_FILENAME_JSON5: &str = ".ic-assets.json5"; pub(crate) type HeadersConfig = HashMap; -type ConfigMap = HashMap>; +type ConfigNode = Arc>; +type ConfigMap = HashMap; -#[derive(Deserialize, Serialize, Debug, Default, Clone, PartialEq, Eq)] +#[derive(Deserialize, Serialize, Debug, Default, Clone, PartialEq, Eq, Hash)] pub(crate) struct CacheConfig { pub(crate) max_age: Option, } -#[derive(Derivative)] +#[derive(Derivative, Clone)] #[derivative(Debug)] -struct AssetConfigRule { +pub struct AssetConfigRule { #[derivative(Debug(format_with = "fmt_glob_field"))] r#match: GlobMatcher, cache: Option, headers: Maybe, ignore: Option, + used: bool, + origin: PathBuf, +} +impl PartialEq for AssetConfigRule { +fn eq(&self, other: &Self) -> bool { + self.r#match.glob() == other.r#match.glob() + && self.origin == other.origin + && self.ignore == other.ignore + && self.cache == other.cache + && self.headers == other.headers + } +} +impl Eq for AssetConfigRule {} +impl Hash for AssetConfigRule { + fn hash(&self, state: &mut H) { + self.r#match.glob().hash(state); + self.origin.hash(state); + self.ignore.hash(state); + self.cache.hash(state); + self.headers.hash(state); + } } -#[derive(Deserialize, Debug)] +#[derive(Deserialize, Debug, Clone, Eq, PartialEq)] enum Maybe { Null, Absent, Value(T), } +impl Hash for Maybe { + fn hash(&self, state: &mut H) { + match self { + Maybe::Null => "null".hash(state), + Maybe::Absent => "absent".hash(state), + Maybe::Value(v) => v.iter().for_each(|v| v.hash(state)), + } + } +} + + fn fmt_glob_field( field: &GlobMatcher, formatter: &mut std::fmt::Formatter, @@ -54,7 +88,7 @@ impl AssetConfigRule { } } -#[derive(Debug)] +#[derive(Debug, Clone)] pub(crate) struct AssetSourceDirectoryConfiguration { config_map: ConfigMap, } @@ -68,7 +102,7 @@ pub(crate) struct AssetConfig { #[derive(Debug, Default)] struct AssetConfigTreeNode { - pub parent: Option>, + pub parent: Option, pub rules: Vec, } @@ -84,7 +118,7 @@ impl AssetSourceDirectoryConfiguration { Ok(Self { config_map }) } - pub(crate) fn get_asset_config(&self, canonical_path: &Path) -> anyhow::Result { + pub(crate) fn get_asset_config(&mut self, canonical_path: &Path) -> anyhow::Result { let parent_dir = canonical_path.parent().with_context(|| { format!( "unable to get the parent directory for asset path: {:?}", @@ -100,8 +134,28 @@ impl AssetSourceDirectoryConfiguration { parent_dir ) })? - .get_config(canonical_path)) + .lock().unwrap().get_config(canonical_path)) } + + pub(crate) fn get_unused_configs(&self) -> HashSet { + self.config_map + .iter() + .flat_map(|(_,y)| { + y + .clone() + .lock() + .unwrap() + .rules + .iter() + .filter_map(|xxx| + if !xxx.used { + Some(xxx.clone()) + } else { + None + }).collect::>() + }).collect::<_>() + } + } #[derive(Deserialize)] @@ -166,15 +220,17 @@ impl AssetConfigRule { cache, headers, ignore, + used: false, + origin: config_file_parent_dir.to_path_buf(), }) } } impl AssetConfigTreeNode { fn load( - parent: Option>, + parent: Option, dir: &Path, - configs: &mut HashMap>, + configs: &mut ConfigMap, ) -> anyhow::Result<()> { let config_path: Option; match ( @@ -213,7 +269,7 @@ impl AssetConfigTreeNode { let parent_ref = match parent { Some(p) if rules.is_empty() => p, - _ => Arc::new(Self { parent, rules }), + _ => Arc::new(Mutex::new(Self { parent, rules })), }; configs.insert(dir.to_path_buf(), parent_ref.clone()); @@ -227,15 +283,18 @@ impl AssetConfigTreeNode { Ok(()) } - fn get_config(&self, canonical_path: &Path) -> AssetConfig { + fn get_config(&mut self, canonical_path: &Path) -> AssetConfig { let base_config = match &self.parent { - Some(parent) => parent.get_config(canonical_path), + Some(parent) => parent.clone().lock().unwrap().get_config(canonical_path), None => AssetConfig::default(), }; self.rules - .iter() + .iter_mut() .filter(|rule| rule.applies(canonical_path)) - .fold(base_config, |acc, x| acc.merge(x)) + .fold(base_config, |acc , x| { + x.used = true; + acc.merge(x) + }) } } @@ -326,7 +385,7 @@ mod with_tempdir { let assets_temp_dir = create_temporary_assets_directory(Some(cfg), 7).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir)?; + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir)?; for f in ["nested/the-thing.txt", "nested/deep/the-next-thing.toml"] { assert_eq!( assets_config.get_asset_config(assets_dir.join(f).as_path())?, @@ -367,7 +426,7 @@ mod with_tempdir { let assets_temp_dir = create_temporary_assets_directory(cfg, 7).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir)?; + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir)?; for f in ["nested/the-thing.txt", "nested/deep/the-next-thing.toml"] { assert_eq!( assets_config.get_asset_config(assets_dir.join(f).as_path())?, @@ -442,7 +501,7 @@ mod with_tempdir { )])); let assets_temp_dir = create_temporary_assets_directory(cfg, 1).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir)?; + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir)?; let parsed_asset_config = assets_config.get_asset_config(assets_dir.join("index.html").as_path())?; let expected_asset_config = AssetConfig { @@ -515,7 +574,7 @@ mod with_tempdir { let assets_temp_dir = create_temporary_assets_directory(cfg, 7).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let assets_config = dbg!(AssetSourceDirectoryConfiguration::load(&assets_dir))?; + let mut assets_config = dbg!(AssetSourceDirectoryConfiguration::load(&assets_dir))?; for f in [ "index.html", "js/index.js", @@ -567,7 +626,7 @@ mod with_tempdir { )])); let assets_temp_dir = create_temporary_assets_directory(cfg, 0).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir)?; + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir)?; assert_eq!( assets_config.get_asset_config(assets_dir.join("index.html").as_path())?, AssetConfig { @@ -589,7 +648,7 @@ mod with_tempdir { ])); let assets_temp_dir = create_temporary_assets_directory(cfg, 0).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); assert_eq!( assets_config.err().unwrap().to_string(), format!( @@ -608,7 +667,7 @@ mod with_tempdir { let cfg = Some(HashMap::from([("".to_string(), "[[[{{{".to_string())])); let assets_temp_dir = create_temporary_assets_directory(cfg, 0).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); assert_eq!( assets_config.err().unwrap().to_string(), format!( @@ -633,7 +692,7 @@ mod with_tempdir { )])); let assets_temp_dir = create_temporary_assets_directory(cfg, 0).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); assert_eq!( assets_config.err().unwrap().to_string(), format!( @@ -652,7 +711,7 @@ mod with_tempdir { let cfg = Some(HashMap::new()); let assets_temp_dir = create_temporary_assets_directory(cfg, 0).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir)?; + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir)?; assert_eq!( assets_config.get_asset_config(assets_dir.join("doesnt.exists").as_path())?, AssetConfig::default() @@ -678,7 +737,7 @@ mod with_tempdir { ) .unwrap(); - let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); + let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); assert_eq!( assets_config.err().unwrap().to_string(), format!( diff --git a/src/canisters/frontend/ic-asset/src/sync.rs b/src/canisters/frontend/ic-asset/src/sync.rs index 26e096adb8..5ce311b7b3 100644 --- a/src/canisters/frontend/ic-asset/src/sync.rs +++ b/src/canisters/frontend/ic-asset/src/sync.rs @@ -73,13 +73,14 @@ fn gather_asset_descriptors(dirs: &[&Path]) -> anyhow::Result anyhow::Result Date: Fri, 21 Oct 2022 17:18:48 +0200 Subject: [PATCH 02/13] fmt+clippy --- .../frontend/ic-asset/src/asset_config.rs | 57 +++++++++---------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/src/canisters/frontend/ic-asset/src/asset_config.rs b/src/canisters/frontend/ic-asset/src/asset_config.rs index 2f0ca7663a..7437e6b19f 100644 --- a/src/canisters/frontend/ic-asset/src/asset_config.rs +++ b/src/canisters/frontend/ic-asset/src/asset_config.rs @@ -6,7 +6,7 @@ use serde_json::Value; use std::{ collections::{HashMap, HashSet}, fs, - hash::{Hash,Hasher}, + hash::{Hash, Hasher}, path::{Path, PathBuf}, sync::{Arc, Mutex}, }; @@ -35,12 +35,12 @@ pub struct AssetConfigRule { origin: PathBuf, } impl PartialEq for AssetConfigRule { -fn eq(&self, other: &Self) -> bool { + fn eq(&self, other: &Self) -> bool { self.r#match.glob() == other.r#match.glob() - && self.origin == other.origin - && self.ignore == other.ignore - && self.cache == other.cache - && self.headers == other.headers + && self.origin == other.origin + && self.ignore == other.ignore + && self.cache == other.cache + && self.headers == other.headers } } impl Eq for AssetConfigRule {} @@ -71,7 +71,6 @@ impl Hash for Maybe { } } - fn fmt_glob_field( field: &GlobMatcher, formatter: &mut std::fmt::Formatter, @@ -118,7 +117,10 @@ impl AssetSourceDirectoryConfiguration { Ok(Self { config_map }) } - pub(crate) fn get_asset_config(&mut self, canonical_path: &Path) -> anyhow::Result { + pub(crate) fn get_asset_config( + &mut self, + canonical_path: &Path, + ) -> anyhow::Result { let parent_dir = canonical_path.parent().with_context(|| { format!( "unable to get the parent directory for asset path: {:?}", @@ -134,28 +136,25 @@ impl AssetSourceDirectoryConfiguration { parent_dir ) })? - .lock().unwrap().get_config(canonical_path)) + .lock() + .unwrap() + .get_config(canonical_path)) } pub(crate) fn get_unused_configs(&self) -> HashSet { - self.config_map + self.config_map .iter() - .flat_map(|(_,y)| { - y - .clone() + .flat_map(|(_, y)| { + y.clone() .lock() .unwrap() .rules .iter() - .filter_map(|xxx| - if !xxx.used { - Some(xxx.clone()) - } else { - None - }).collect::>() - }).collect::<_>() + .filter_map(|xxx| if !xxx.used { Some(xxx.clone()) } else { None }) + .collect::>() + }) + .collect::<_>() } - } #[derive(Deserialize)] @@ -227,11 +226,7 @@ impl AssetConfigRule { } impl AssetConfigTreeNode { - fn load( - parent: Option, - dir: &Path, - configs: &mut ConfigMap, - ) -> anyhow::Result<()> { + fn load(parent: Option, dir: &Path, configs: &mut ConfigMap) -> anyhow::Result<()> { let config_path: Option; match ( dir.join(ASSETS_CONFIG_FILENAME_JSON).exists(), @@ -291,7 +286,7 @@ impl AssetConfigTreeNode { self.rules .iter_mut() .filter(|rule| rule.applies(canonical_path)) - .fold(base_config, |acc , x| { + .fold(base_config, |acc, x| { x.used = true; acc.merge(x) }) @@ -648,7 +643,7 @@ mod with_tempdir { ])); let assets_temp_dir = create_temporary_assets_directory(cfg, 0).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); + let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); assert_eq!( assets_config.err().unwrap().to_string(), format!( @@ -667,7 +662,7 @@ mod with_tempdir { let cfg = Some(HashMap::from([("".to_string(), "[[[{{{".to_string())])); let assets_temp_dir = create_temporary_assets_directory(cfg, 0).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); + let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); assert_eq!( assets_config.err().unwrap().to_string(), format!( @@ -692,7 +687,7 @@ mod with_tempdir { )])); let assets_temp_dir = create_temporary_assets_directory(cfg, 0).unwrap(); let assets_dir = assets_temp_dir.path().canonicalize()?; - let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); + let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); assert_eq!( assets_config.err().unwrap().to_string(), format!( @@ -737,7 +732,7 @@ mod with_tempdir { ) .unwrap(); - let mut assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); + let assets_config = AssetSourceDirectoryConfiguration::load(&assets_dir); assert_eq!( assets_config.err().unwrap().to_string(), format!( From d5f9be9608ab174e0c66405babed45c19c2612ec Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Fri, 21 Oct 2022 19:43:51 +0200 Subject: [PATCH 03/13] fix e2e --- e2e/tests-dfx/assetscanister.bash | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/tests-dfx/assetscanister.bash b/e2e/tests-dfx/assetscanister.bash index 086976b9f4..38fe94ed80 100644 --- a/e2e/tests-dfx/assetscanister.bash +++ b/e2e/tests-dfx/assetscanister.bash @@ -552,6 +552,6 @@ CHERRIES" "$stdout" } ]' > src/e2e_project_frontend/assets/.ic-assets.json5 - dfx deploy - assert_contains "Unused" + assert_command dfx deploy + assert_contains "WARN: Unused config: AssetConfigRule" } From 887aa47e890a7198b90ed836a17c8137af1ea2ea Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Wed, 26 Oct 2022 22:07:31 +0200 Subject: [PATCH 04/13] implement Eric's suggestions (+refactor): - added `origin: PathBuf` field to `AssetConfigTreeNode` to help aggregate rules coming from the same config file - implemented deduping for rules additionally: - refactor: moved all functions needed for de/serializaton and pretty-printing into separate module, leaving core business logic more easily visually separatble from rest of code - added comments for main data structures and functions - made core data structures and methods public (change from pub(crate)) in case someone would want to use ic_asset::asset_config outside of dfx --- e2e/tests-dfx/assetscanister.bash | 2 +- .../frontend/ic-asset/src/asset_config.rs | 338 +++++++++++------- src/canisters/frontend/ic-asset/src/sync.rs | 23 +- 3 files changed, 216 insertions(+), 147 deletions(-) diff --git a/e2e/tests-dfx/assetscanister.bash b/e2e/tests-dfx/assetscanister.bash index 38fe94ed80..38cff4ac07 100644 --- a/e2e/tests-dfx/assetscanister.bash +++ b/e2e/tests-dfx/assetscanister.bash @@ -553,5 +553,5 @@ CHERRIES" "$stdout" ]' > src/e2e_project_frontend/assets/.ic-assets.json5 assert_command dfx deploy - assert_contains "WARN: Unused config: AssetConfigRule" + assert_match "WARNING: 1 unmatched configuration in" } diff --git a/src/canisters/frontend/ic-asset/src/asset_config.rs b/src/canisters/frontend/ic-asset/src/asset_config.rs index 7437e6b19f..229d4af184 100644 --- a/src/canisters/frontend/ic-asset/src/asset_config.rs +++ b/src/canisters/frontend/ic-asset/src/asset_config.rs @@ -1,12 +1,10 @@ use anyhow::{bail, Context}; use derivative::Derivative; -use globset::{Glob, GlobMatcher}; +use globset::GlobMatcher; use serde::{Deserialize, Serialize}; -use serde_json::Value; use std::{ - collections::{HashMap, HashSet}, + collections::HashMap, fs, - hash::{Hash, Hasher}, path::{Path, PathBuf}, sync::{Arc, Mutex}, }; @@ -14,71 +12,57 @@ use std::{ pub(crate) const ASSETS_CONFIG_FILENAME_JSON: &str = ".ic-assets.json"; pub(crate) const ASSETS_CONFIG_FILENAME_JSON5: &str = ".ic-assets.json5"; +/// A final piece of metadata assigned to the asset +#[derive(Debug, Default, PartialEq, Eq, Serialize, Clone)] +pub struct AssetConfig { + pub(crate) cache: Option, + pub(crate) headers: Option, + pub(crate) ignore: Option, +} + pub(crate) type HeadersConfig = HashMap; -type ConfigNode = Arc>; -type ConfigMap = HashMap; -#[derive(Deserialize, Serialize, Debug, Default, Clone, PartialEq, Eq, Hash)] +#[derive(Deserialize, Serialize, Debug, Default, Clone, PartialEq, Eq)] pub(crate) struct CacheConfig { pub(crate) max_age: Option, } -#[derive(Derivative, Clone)] -#[derivative(Debug)] +/// A single configuration object, from `.ic-assets.json` config file +#[derive(Derivative, Clone, Serialize)] +#[derivative(Debug, PartialEq)] pub struct AssetConfigRule { - #[derivative(Debug(format_with = "fmt_glob_field"))] + #[derivative( + Debug(format_with = "rule_utils::glob_fmt"), + PartialEq(compare_with = "rule_utils::glob_cmp") + )] + #[serde(serialize_with = "rule_utils::glob_serialize")] r#match: GlobMatcher, + #[serde(skip_serializing_if = "Option::is_none")] cache: Option, + #[serde( + serialize_with = "rule_utils::headers_serialize", + skip_serializing_if = "Maybe::is_absent" + )] headers: Maybe, + #[serde(skip_serializing_if = "Option::is_none")] ignore: Option, + #[serde(skip_serializing)] used: bool, - origin: PathBuf, -} -impl PartialEq for AssetConfigRule { - fn eq(&self, other: &Self) -> bool { - self.r#match.glob() == other.r#match.glob() - && self.origin == other.origin - && self.ignore == other.ignore - && self.cache == other.cache - && self.headers == other.headers - } -} -impl Eq for AssetConfigRule {} -impl Hash for AssetConfigRule { - fn hash(&self, state: &mut H) { - self.r#match.glob().hash(state); - self.origin.hash(state); - self.ignore.hash(state); - self.cache.hash(state); - self.headers.hash(state); - } } -#[derive(Deserialize, Debug, Clone, Eq, PartialEq)] +#[derive(Deserialize, Debug, Clone, PartialEq, Eq)] enum Maybe { Null, Absent, Value(T), } -impl Hash for Maybe { - fn hash(&self, state: &mut H) { - match self { - Maybe::Null => "null".hash(state), - Maybe::Absent => "absent".hash(state), - Maybe::Value(v) => v.iter().for_each(|v| v.hash(state)), - } +impl Default for Maybe { + fn default() -> Self { + Self::Absent } } -fn fmt_glob_field( - field: &GlobMatcher, - formatter: &mut std::fmt::Formatter, -) -> Result<(), std::fmt::Error> { - formatter.write_str(field.glob().glob())?; - Ok(()) -} - impl AssetConfigRule { fn applies(&self, canonical_path: &Path) -> bool { // TODO: better dot files/dirs handling, awaiting upstream changes: @@ -87,27 +71,28 @@ impl AssetConfigRule { } } -#[derive(Debug, Clone)] -pub(crate) struct AssetSourceDirectoryConfiguration { - config_map: ConfigMap, -} +type ConfigNode = Arc>; +type ConfigMap = HashMap; -#[derive(Debug, Default, PartialEq, Eq, Serialize, Clone)] -pub(crate) struct AssetConfig { - pub(crate) cache: Option, - pub(crate) headers: Option, - pub(crate) ignore: Option, +/// The main public interface for aggregating `.ic-assets.json` files +/// nested in directories. Each sub/directory will be represented +/// as `AssetConfigTreeNode`. +#[derive(Debug)] +pub struct AssetSourceDirectoryConfiguration { + config_map: ConfigMap, } +/// A directory or subdirectory with assets. #[derive(Debug, Default)] struct AssetConfigTreeNode { pub parent: Option, pub rules: Vec, + pub origin: PathBuf, } impl AssetSourceDirectoryConfiguration { /// Constructs config tree for assets directory. - pub(crate) fn load(root_dir: &Path) -> anyhow::Result { + pub fn load(root_dir: &Path) -> anyhow::Result { if !root_dir.has_root() { bail!("root_dir paramenter is expected to be canonical path") } @@ -117,7 +102,8 @@ impl AssetSourceDirectoryConfiguration { Ok(Self { config_map }) } - pub(crate) fn get_asset_config( + /// Fetches the configuration for the asset. + pub fn get_asset_config( &mut self, canonical_path: &Path, ) -> anyhow::Result { @@ -141,91 +127,45 @@ impl AssetSourceDirectoryConfiguration { .get_config(canonical_path)) } - pub(crate) fn get_unused_configs(&self) -> HashSet { - self.config_map - .iter() - .flat_map(|(_, y)| { - y.clone() - .lock() - .unwrap() - .rules - .iter() - .filter_map(|xxx| if !xxx.used { Some(xxx.clone()) } else { None }) - .collect::>() - }) - .collect::<_>() - } -} - -#[derive(Deserialize)] -#[serde(deny_unknown_fields)] -struct InterimAssetConfigRule { - r#match: String, - cache: Option, - #[serde(default, deserialize_with = "deser_headers")] - headers: Maybe, - ignore: Option, -} - -impl Default for Maybe { - fn default() -> Self { - Self::Absent - } -} + /// Returns a collection of unused configuration objects from all `.ic-assets.json` files + pub fn get_unused_configs(&self) -> HashMap> { + let mut hm = HashMap::new(); + // aggregate + for (_fake_origin, node) in &self.config_map { + let config_node = &node.lock().unwrap(); -fn deser_headers<'de, D>(deserializer: D) -> Result, D::Error> -where - D: serde::Deserializer<'de>, -{ - match serde_json::value::Value::deserialize(deserializer)? { - Value::Object(v) => Ok(Maybe::Value( - v.into_iter() - .map(|(k, v)| (k, v.to_string().trim_matches('"').to_string())) - .collect::>(), - )), - Value::Null => Ok(Maybe::Null), - _ => Err(serde::de::Error::custom( - "wrong data format for field `headers` (only map or null are allowed)", - )), - } -} + let origin = config_node.origin.clone(); -impl AssetConfigRule { - fn from_interim( - InterimAssetConfigRule { - r#match, - cache, - headers, - ignore, - }: InterimAssetConfigRule, - config_file_parent_dir: &Path, - ) -> anyhow::Result { - let glob = Glob::new( - config_file_parent_dir - .join(&r#match) - .to_str() - .with_context(|| { - format!( - "cannot combine {} and {} into a string (to be later used as a glob pattern)", - config_file_parent_dir.display(), - r#match - ) - })?, - ) - .with_context(|| format!("{} is not a valid glob pattern", r#match))?.compile_matcher(); - - Ok(Self { - r#match: glob, - cache, - headers, - ignore, - used: false, - origin: config_file_parent_dir.to_path_buf(), - }) + for rule in config_node.rules.clone() { + if !rule.used { + hm.entry(origin.clone()) + .and_modify(|v: &mut Vec| v.push(rule.clone())) + .or_insert(vec![rule.clone()]); + } + } + } + // dedup and remove full path from "match" field + for (path, rules) in hm.iter_mut() { + rules.sort_by_key(|v| v.r#match.glob().to_string()); + rules.dedup(); + for mut rule in rules { + rule.r#match = globset::Glob::new( + &rule + .r#match + .glob() + .to_string() + .replace(path.to_str().unwrap(), ""), + ) + .unwrap() + .compile_matcher(); + } + } + hm } } impl AssetConfigTreeNode { + /// Constructs config tree for assets directory in a recursive fashion. fn load(parent: Option, dir: &Path, configs: &mut ConfigMap) -> anyhow::Result<()> { let config_path: Option; match ( @@ -241,7 +181,6 @@ impl AssetConfigTreeNode { )) } (true, false) => config_path = Some(dir.join(ASSETS_CONFIG_FILENAME_JSON)), - (false, true) => config_path = Some(dir.join(ASSETS_CONFIG_FILENAME_JSON5)), (false, false) => config_path = None, } @@ -250,7 +189,7 @@ impl AssetConfigTreeNode { let content = fs::read_to_string(&config_path).with_context(|| { format!("unable to read config file: {}", config_path.display()) })?; - let interim_rules: Vec = json5::from_str(&content) + let interim_rules: Vec = json5::from_str(&content) .with_context(|| { format!( "malformed JSON asset config file: {}", @@ -264,7 +203,11 @@ impl AssetConfigTreeNode { let parent_ref = match parent { Some(p) if rules.is_empty() => p, - _ => Arc::new(Mutex::new(Self { parent, rules })), + _ => Arc::new(Mutex::new(Self { + parent, + rules, + origin: dir.to_path_buf(), + })), }; configs.insert(dir.to_path_buf(), parent_ref.clone()); @@ -278,6 +221,8 @@ impl AssetConfigTreeNode { Ok(()) } + /// Fetches asset config in a recursive fashion. + /// Marks config rules as *used*, whenever the rule' glob patter matched querried file. fn get_config(&mut self, canonical_path: &Path) -> AssetConfig { let base_config = match &self.parent { Some(parent) => parent.clone().lock().unwrap().get_config(canonical_path), @@ -312,6 +257,121 @@ impl AssetConfig { } } +/// This module contains various utilities needed for serialization/deserialization +/// and pretty-printing of the main data struct (i.e. AssetConfigRule). +mod rule_utils { + use super::{AssetConfigRule, CacheConfig, HeadersConfig, Maybe}; + use anyhow::Context; + use globset::{Glob, GlobMatcher}; + use serde::{Deserialize, Serializer}; + use serde_json::Value; + use std::collections::HashMap; + use std::fmt; + use std::path::Path; + + pub(super) fn glob_cmp(a: &GlobMatcher, b: &GlobMatcher) -> bool { + a.glob() == b.glob() + } + + pub(super) fn glob_fmt( + field: &GlobMatcher, + formatter: &mut fmt::Formatter, + ) -> Result<(), fmt::Error> { + formatter.write_str(field.glob().glob())?; + Ok(()) + } + + pub(super) fn glob_serialize(bytes: &GlobMatcher, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&bytes.glob().to_string()) + } + + pub(super) fn headers_serialize( + bytes: &super::Maybe, + serializer: S, + ) -> Result + where + S: Serializer, + { + match bytes { + super::Maybe::Null => serializer.serialize_str("{}"), + super::Maybe::Value(v) => { + serializer.serialize_str(&serde_json::to_string_pretty(v).unwrap()) + } + super::Maybe::Absent => unreachable!(), // this option is already skipped via `skip_serialization_with` + } + } + + fn headers_deserialize<'de, D>(deserializer: D) -> Result, D::Error> + where + D: serde::Deserializer<'de>, + { + match serde_json::value::Value::deserialize(deserializer)? { + Value::Object(v) => Ok(Maybe::Value( + v.into_iter() + .map(|(k, v)| (k, v.to_string().trim_matches('"').to_string())) + .collect::>(), + )), + Value::Null => Ok(Maybe::Null), + _ => Err(serde::de::Error::custom( + "wrong data format for field `headers` (only map or null are allowed)", + )), + } + } + + impl Maybe { + pub(super) fn is_absent(&self) -> bool { + matches!(*self, Self::Absent) + } + } + + #[derive(Deserialize)] + #[serde(deny_unknown_fields)] + pub(super) struct InterimAssetConfigRule { + r#match: String, + cache: Option, + #[serde(default, deserialize_with = "headers_deserialize")] + headers: Maybe, + ignore: Option, + } + + impl AssetConfigRule { + pub(super) fn from_interim( + InterimAssetConfigRule { + r#match, + cache, + headers, + ignore, + }: InterimAssetConfigRule, + config_file_parent_dir: &Path, + ) -> anyhow::Result { + let glob = Glob::new( + config_file_parent_dir + .join(&r#match) + .to_str() + .with_context(|| { + format!( + "cannot combine {} and {} into a string (to be later used as a glob pattern)", + config_file_parent_dir.display(), + r#match + ) + })?, + ) + .with_context(|| format!("{} is not a valid glob pattern", r#match))?.compile_matcher(); + + Ok(Self { + r#match: glob, + cache, + headers, + ignore, + used: false, + }) + } + } +} + #[cfg(test)] mod with_tempdir { diff --git a/src/canisters/frontend/ic-asset/src/sync.rs b/src/canisters/frontend/ic-asset/src/sync.rs index 5ce311b7b3..d4f3941e19 100644 --- a/src/canisters/frontend/ic-asset/src/sync.rs +++ b/src/canisters/frontend/ic-asset/src/sync.rs @@ -5,7 +5,6 @@ use crate::asset_config::{ AssetConfig, AssetSourceDirectoryConfiguration, ASSETS_CONFIG_FILENAME_JSON, }; use crate::params::CanisterCallParams; - use crate::operations::{ create_new_assets, delete_obsolete_assets, set_encodings, unset_obsolete_encodings, }; @@ -74,13 +73,12 @@ fn gather_asset_descriptors(dirs: &[&Path]) -> anyhow::Result anyhow::Result>(); + + for e in entries { let source = e.path().canonicalize().with_context(|| { format!( "unable to canonicalize the path when gathering asset descriptors: {}", @@ -124,8 +124,17 @@ fn gather_asset_descriptors(dirs: &[&Path]) -> anyhow::Result 1 { "s" } else { "" }, + path=config_path.display() + ); + for rule in rules { + println!("{}", serde_json::to_string_pretty(&rule).unwrap()); + } } } Ok(asset_descriptors.into_values().collect()) From 2e37bb790517c4cc4fe00089b18f5fabd80a3c2e Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Wed, 26 Oct 2022 22:10:22 +0200 Subject: [PATCH 05/13] fmt --- src/canisters/frontend/ic-asset/src/asset_config.rs | 5 +---- src/canisters/frontend/ic-asset/src/sync.rs | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/canisters/frontend/ic-asset/src/asset_config.rs b/src/canisters/frontend/ic-asset/src/asset_config.rs index 229d4af184..8932cfcdb7 100644 --- a/src/canisters/frontend/ic-asset/src/asset_config.rs +++ b/src/canisters/frontend/ic-asset/src/asset_config.rs @@ -103,10 +103,7 @@ impl AssetSourceDirectoryConfiguration { } /// Fetches the configuration for the asset. - pub fn get_asset_config( - &mut self, - canonical_path: &Path, - ) -> anyhow::Result { + pub fn get_asset_config(&mut self, canonical_path: &Path) -> anyhow::Result { let parent_dir = canonical_path.parent().with_context(|| { format!( "unable to get the parent directory for asset path: {:?}", diff --git a/src/canisters/frontend/ic-asset/src/sync.rs b/src/canisters/frontend/ic-asset/src/sync.rs index d4f3941e19..2c872ba8e4 100644 --- a/src/canisters/frontend/ic-asset/src/sync.rs +++ b/src/canisters/frontend/ic-asset/src/sync.rs @@ -4,10 +4,10 @@ use crate::asset_canister::protocol::{AssetDetails, BatchOperationKind}; use crate::asset_config::{ AssetConfig, AssetSourceDirectoryConfiguration, ASSETS_CONFIG_FILENAME_JSON, }; -use crate::params::CanisterCallParams; use crate::operations::{ create_new_assets, delete_obsolete_assets, set_encodings, unset_obsolete_encodings, }; +use crate::params::CanisterCallParams; use crate::plumbing::{make_project_assets, AssetDescriptor, ProjectAsset}; use anyhow::{bail, Context}; use ic_utils::Canister; From 6ebd09bf12d994425507662fe5ab32ca2770b361 Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Wed, 26 Oct 2022 22:12:41 +0200 Subject: [PATCH 06/13] wording --- src/canisters/frontend/ic-asset/src/asset_config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/canisters/frontend/ic-asset/src/asset_config.rs b/src/canisters/frontend/ic-asset/src/asset_config.rs index 8932cfcdb7..965c5b8d70 100644 --- a/src/canisters/frontend/ic-asset/src/asset_config.rs +++ b/src/canisters/frontend/ic-asset/src/asset_config.rs @@ -255,7 +255,7 @@ impl AssetConfig { } /// This module contains various utilities needed for serialization/deserialization -/// and pretty-printing of the main data struct (i.e. AssetConfigRule). +/// and pretty-printing of the `AssetConfigRule` data structure. mod rule_utils { use super::{AssetConfigRule, CacheConfig, HeadersConfig, Maybe}; use anyhow::Context; From f72f3f6a7a68e2ae6b0ed6ea624d0f62f31d9903 Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Thu, 27 Oct 2022 14:56:10 +0200 Subject: [PATCH 07/13] fix message prinint + extend e2e tests --- e2e/tests-dfx/assetscanister.bash | 61 ++++++++++++++++++- .../frontend/ic-asset/src/asset_config.rs | 30 ++++----- 2 files changed, 76 insertions(+), 15 deletions(-) diff --git a/e2e/tests-dfx/assetscanister.bash b/e2e/tests-dfx/assetscanister.bash index 38cff4ac07..88c8b2d51f 100644 --- a/e2e/tests-dfx/assetscanister.bash +++ b/e2e/tests-dfx/assetscanister.bash @@ -545,13 +545,72 @@ CHERRIES" "$stdout" dfx_start + mkdir src/e2e_project_frontend/assets/somedir + touch src/e2e_project_frontend/assets/somedir/upload-me.txt echo '[ { "match": "nevermatchme", "cache": { "max_age": 2000 } } ]' > src/e2e_project_frontend/assets/.ic-assets.json5 + echo '[ + { + "match": "upload-me.txt", + "headers": { "key": "value" } + }, + { + "match": "nevermatchme", + "headers": {}, + "ignore": false + }, + { + "match": "nevermatchmetoo", + "headers": null, + "ignore": false + }, + { + "match": "non-matcher", + "headers": {"x-header": "x-value"}, + "ignore": false + }, + { + "match": "/thanks-for-not-stripping-forward-slash", + "headers": {"x-header": "x-value"}, + "ignore": false + } + ]' > src/e2e_project_frontend/assets/somedir/.ic-assets.json5 assert_command dfx deploy - assert_match "WARNING: 1 unmatched configuration in" + assert_match 'WARNING: 1 unmatched configuration .*/src/e2e_project_frontend/assets/.ic-assets.json config file:' + assert_contains '{ + "match": "nevermatchme", + "cache": { + "max_age": 2000 + } +}' + assert_match 'WARNING: 4 unmatched configurations .*/src/e2e_project_frontend/assets/somedir/.ic-assets.json config file:' + assert_contains '{ + "match": "nevermatchme", + "headers": {}, + "ignore": false +} +{ + "match": "nevermatchmetoo", + "headers": {}, + "ignore": false +} +{ + "match": "non-matcher", + "headers": { + "x-header": "x-value" + }, + "ignore": false +} +{ + "match": "/thanks-for-not-stripping-forward-slash", + "headers": { + "x-header": "x-value" + }, + "ignore": false +}' } diff --git a/src/canisters/frontend/ic-asset/src/asset_config.rs b/src/canisters/frontend/ic-asset/src/asset_config.rs index 965c5b8d70..e91dc40ad5 100644 --- a/src/canisters/frontend/ic-asset/src/asset_config.rs +++ b/src/canisters/frontend/ic-asset/src/asset_config.rs @@ -146,15 +146,12 @@ impl AssetSourceDirectoryConfiguration { rules.sort_by_key(|v| v.r#match.glob().to_string()); rules.dedup(); for mut rule in rules { - rule.r#match = globset::Glob::new( - &rule - .r#match - .glob() - .to_string() - .replace(path.to_str().unwrap(), ""), - ) - .unwrap() - .compile_matcher(); + let prefix_path = format!("{}/", path.display()); + let original_glob = rule.r#match.glob().to_string().replace(&prefix_path, ""); + let original_glob = globset::Glob::new(&original_glob) + .unwrap() + .compile_matcher(); + rule.r#match = original_glob; } } hm @@ -286,16 +283,21 @@ mod rule_utils { } pub(super) fn headers_serialize( - bytes: &super::Maybe, + headers: &super::Maybe, serializer: S, ) -> Result where S: Serializer, { - match bytes { - super::Maybe::Null => serializer.serialize_str("{}"), - super::Maybe::Value(v) => { - serializer.serialize_str(&serde_json::to_string_pretty(v).unwrap()) + use serde::ser::SerializeMap; + match headers { + super::Maybe::Null => serializer.serialize_map(Some(0))?.end(), + super::Maybe::Value(hm) => { + let mut map = serializer.serialize_map(Some(hm.len()))?; + for (k, v) in hm { + map.serialize_entry(k, &v)?; + } + map.end() } super::Maybe::Absent => unreachable!(), // this option is already skipped via `skip_serialization_with` } From a57ab404e9723aed086a28978736f254caa40e83 Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Thu, 27 Oct 2022 14:59:36 +0200 Subject: [PATCH 08/13] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08f567dd8a..305f0ce514 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ## DFX +### feat(frontend-canister): add warning if config is provided in `.ic-assets.json` but not used + ### fix(frontend-canister): Allow overwriting default HTTP Headers for assets in frontend canister Allows to overwrite `Content-Type`, `Content-Encoding`, and `Cache-Control` HTTP headers with custom values via `.ic-assets.json5` config file. Example `.ic-assets.json5` file: From 0c9d21bcebf57d61a0e51bdddc5f15033f11cd4b Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Thu, 27 Oct 2022 15:03:00 +0200 Subject: [PATCH 09/13] ate a word --- e2e/tests-dfx/assetscanister.bash | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/tests-dfx/assetscanister.bash b/e2e/tests-dfx/assetscanister.bash index 88c8b2d51f..56e71d039a 100644 --- a/e2e/tests-dfx/assetscanister.bash +++ b/e2e/tests-dfx/assetscanister.bash @@ -581,14 +581,14 @@ CHERRIES" "$stdout" ]' > src/e2e_project_frontend/assets/somedir/.ic-assets.json5 assert_command dfx deploy - assert_match 'WARNING: 1 unmatched configuration .*/src/e2e_project_frontend/assets/.ic-assets.json config file:' + assert_match 'WARNING: 1 unmatched configuration in .*/src/e2e_project_frontend/assets/.ic-assets.json config file:' assert_contains '{ "match": "nevermatchme", "cache": { "max_age": 2000 } }' - assert_match 'WARNING: 4 unmatched configurations .*/src/e2e_project_frontend/assets/somedir/.ic-assets.json config file:' + assert_match 'WARNING: 4 unmatched configurations in .*/src/e2e_project_frontend/assets/somedir/.ic-assets.json config file:' assert_contains '{ "match": "nevermatchme", "headers": {}, From c48c930f099277175eadf09ed009919e85a95cd5 Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Thu, 27 Oct 2022 15:42:23 +0200 Subject: [PATCH 10/13] lint --- src/canisters/frontend/ic-asset/src/asset_config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/canisters/frontend/ic-asset/src/asset_config.rs b/src/canisters/frontend/ic-asset/src/asset_config.rs index e91dc40ad5..9bd48ef6e8 100644 --- a/src/canisters/frontend/ic-asset/src/asset_config.rs +++ b/src/canisters/frontend/ic-asset/src/asset_config.rs @@ -128,7 +128,7 @@ impl AssetSourceDirectoryConfiguration { pub fn get_unused_configs(&self) -> HashMap> { let mut hm = HashMap::new(); // aggregate - for (_fake_origin, node) in &self.config_map { + for node in self.config_map.values() { let config_node = &node.lock().unwrap(); let origin = config_node.origin.clone(); @@ -137,7 +137,7 @@ impl AssetSourceDirectoryConfiguration { if !rule.used { hm.entry(origin.clone()) .and_modify(|v: &mut Vec| v.push(rule.clone())) - .or_insert(vec![rule.clone()]); + .or_insert_with(|| vec![rule.clone()]); } } } From 89dea755dfa000c3f5d62619d4e8f553673ec389 Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Thu, 27 Oct 2022 18:17:14 +0200 Subject: [PATCH 11/13] fix e2e test for ubuntu --- e2e/tests-dfx/assetscanister.bash | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/e2e/tests-dfx/assetscanister.bash b/e2e/tests-dfx/assetscanister.bash index 56e71d039a..cddde95e67 100644 --- a/e2e/tests-dfx/assetscanister.bash +++ b/e2e/tests-dfx/assetscanister.bash @@ -605,8 +605,9 @@ CHERRIES" "$stdout" "x-header": "x-value" }, "ignore": false -} -{ +}' + # splitting this up into two checks, because the order is different on macos vs ubuntu + assert_contains '{ "match": "/thanks-for-not-stripping-forward-slash", "headers": { "x-header": "x-value" From 5ed531f24bbfd972f6e8e92c6f35b3af0d8a040c Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Thu, 27 Oct 2022 18:57:14 +0200 Subject: [PATCH 12/13] reconsidered :) --- src/canisters/frontend/ic-asset/src/asset_config.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/canisters/frontend/ic-asset/src/asset_config.rs b/src/canisters/frontend/ic-asset/src/asset_config.rs index 9bd48ef6e8..311b76a260 100644 --- a/src/canisters/frontend/ic-asset/src/asset_config.rs +++ b/src/canisters/frontend/ic-asset/src/asset_config.rs @@ -147,11 +147,14 @@ impl AssetSourceDirectoryConfiguration { rules.dedup(); for mut rule in rules { let prefix_path = format!("{}/", path.display()); - let original_glob = rule.r#match.glob().to_string().replace(&prefix_path, ""); - let original_glob = globset::Glob::new(&original_glob) - .unwrap() - .compile_matcher(); - rule.r#match = original_glob; + let modified_glob = rule.r#match.glob().to_string(); + let original_glob = &modified_glob.strip_prefix(&prefix_path); + if let Some(og) = original_glob { + let original_glob = globset::Glob::new(og) + .unwrap() + .compile_matcher(); + rule.r#match = original_glob; + } } } hm From ea588cc7a1968edb52ea7ab897e9fc50839420aa Mon Sep 17 00:00:00 2001 From: Marcin Nowak-Liebiediew Date: Thu, 27 Oct 2022 19:21:47 +0200 Subject: [PATCH 13/13] fmt --- src/canisters/frontend/ic-asset/src/asset_config.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/canisters/frontend/ic-asset/src/asset_config.rs b/src/canisters/frontend/ic-asset/src/asset_config.rs index 311b76a260..1b4d0e5813 100644 --- a/src/canisters/frontend/ic-asset/src/asset_config.rs +++ b/src/canisters/frontend/ic-asset/src/asset_config.rs @@ -150,9 +150,7 @@ impl AssetSourceDirectoryConfiguration { let modified_glob = rule.r#match.glob().to_string(); let original_glob = &modified_glob.strip_prefix(&prefix_path); if let Some(og) = original_glob { - let original_glob = globset::Glob::new(og) - .unwrap() - .compile_matcher(); + let original_glob = globset::Glob::new(og).unwrap().compile_matcher(); rule.r#match = original_glob; } }