From 2bf91fab09faab26563bb130309b00f44baa0f11 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 22 Dec 2017 11:49:20 +0100 Subject: [PATCH 1/4] Enhance error types --- lib/core/libimagstore/src/configuration.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/core/libimagstore/src/configuration.rs b/lib/core/libimagstore/src/configuration.rs index d239d9dd..b1b1974e 100644 --- a/lib/core/libimagstore/src/configuration.rs +++ b/lib/core/libimagstore/src/configuration.rs @@ -23,11 +23,11 @@ use store::Result; use error::StoreError as SE; use error::StoreErrorKind as SEK; -use toml_query::read::TomlValueReadExt; - /// Checks whether the store configuration has a key "implicit-create" which maps to a boolean /// value. If that key is present, the boolean is returned, otherwise false is returned. pub fn config_implicit_store_create_allowed(config: &Option) -> Result { + use toml_query::read::TomlValueReadExt; + let key = "store.implicit-create"; if let Some(ref t) = *config { From 20a552f527d6ab007123cf8594c8d8625798ca88 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 22 Dec 2017 11:49:32 +0100 Subject: [PATCH 2/4] Refactor header checking to use toml-query --- lib/core/libimagstore/src/store.rs | 67 +++++++++++++----------------- 1 file changed, 30 insertions(+), 37 deletions(-) diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs index 86bd08f4..b2e9beba 100644 --- a/lib/core/libimagstore/src/store.rs +++ b/lib/core/libimagstore/src/store.rs @@ -32,10 +32,10 @@ use std::fmt::Debug; use std::fmt::Error as FMTError; use toml::Value; -use toml::value::Table; use glob::glob; use walkdir::WalkDir; use walkdir::Iter as WalkDirIter; +use toml_query::read::TomlValueReadExt; use error::{StoreError as SE, StoreErrorKind as SEK}; use error::ResultExt; @@ -1067,10 +1067,7 @@ pub trait Header { impl Header for Value { fn verify(&self) -> Result<()> { - match *self { - Value::Table(ref t) => verify_header(&t), - _ => Err(SE::from_kind(SEK::HeaderTypeFailure)), - } + verify_header(self) } fn parse(s: &str) -> Result { @@ -1079,7 +1076,6 @@ impl Header for Value { from_str(s) .map_err(From::from) .and_then(verify_header_consistency) - .map(Value::Table) } fn default_header() -> Value { @@ -1098,16 +1094,16 @@ impl Header for Value { } -fn verify_header_consistency(t: Table) -> Result { +fn verify_header_consistency(t: Value) -> Result { verify_header(&t).chain_err(|| SEK::HeaderInconsistency).map(|_| t) } -fn verify_header(t: &Table) -> Result<()> { - if !has_main_section(t) { +fn verify_header(t: &Value) -> Result<()> { + if !has_main_section(t)? { Err(SE::from_kind(SEK::MissingMainSection)) - } else if !has_imag_version_in_main_section(t) { + } else if !has_imag_version_in_main_section(t)? { Err(SE::from_kind(SEK::MissingVersionInfo)) - } else if !has_only_tables(t) { + } else if !has_only_tables(t)? { debug!("Could not verify that it only has tables in its base table"); Err(SE::from_kind(SEK::NonTableInBaseTable)) } else { @@ -1115,33 +1111,30 @@ fn verify_header(t: &Table) -> Result<()> { } } -fn has_only_tables(t: &Table) -> bool { +fn has_only_tables(t: &Value) -> Result { debug!("Verifying that table has only tables"); - t.iter().all(|(_, x)| is_match!(*x, Value::Table(_))) -} - -fn has_main_section(t: &Table) -> bool { - t.contains_key("imag") && is_match!(t.get("imag"), Some(&Value::Table(_))) -} - -fn has_imag_version_in_main_section(t: &Table) -> bool { - use semver::Version; - - match *t.get("imag").unwrap() { - Value::Table(ref sec) => { - sec.get("version") - .and_then(|v| { - match *v { - Value::String(ref s) => Some(Version::parse(&s[..]).is_ok()), - _ => Some(false), - } - }) - .unwrap_or(false) - } - _ => false, + match *t { + Value::Table(ref tab) => Ok(tab.iter().all(|(_, x)| is_match!(*x, Value::Table(_)))), + _ => Err(SE::from_kind(SEK::HeaderTypeFailure)), } } +fn has_main_section(t: &Value) -> Result { + t.read("imag")? + .ok_or(SE::from_kind(SEK::ConfigKeyMissingError("imag"))) + .map(Value::is_table) +} + +fn has_imag_version_in_main_section(t: &Value) -> Result { + use toml_query::read::TomlValueReadExt; + + t.read("imag.version")? + .ok_or(SE::from_kind(SEK::ConfigKeyMissingError("imag.version")))? + .as_str() + .map(|s| ::semver::Version::parse(s).is_ok()) + .ok_or(SE::from_kind(SEK::ConfigTypeError("imag.version", "String"))) +} + #[cfg(test)] mod test { @@ -1184,7 +1177,7 @@ mod test { let mut map = BTreeMap::new(); map.insert("imag".into(), Value::Table(BTreeMap::new())); - assert!(!has_imag_version_in_main_section(&map)); + assert!(has_imag_version_in_main_section(&map).is_err()); } #[test] @@ -1194,7 +1187,7 @@ mod test { sub.insert("version".into(), Value::String("0.0.0".into())); map.insert("imag".into(), Value::Table(sub)); - assert!(has_imag_version_in_main_section(&map)); + assert!(has_imag_version_in_main_section(&map)?); } #[test] @@ -1204,7 +1197,7 @@ mod test { sub.insert("version".into(), Value::Boolean(false)); map.insert("imag".into(), Value::Table(sub)); - assert!(!has_imag_version_in_main_section(&map)); + assert!(has_imag_version_in_main_section(&map).is_err()); } #[test] From 35410aaa2e00516bb28d5a4da74e9913b037e0a5 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 22 Dec 2017 12:01:58 +0100 Subject: [PATCH 3/4] Refactor: Remove unneccessary functions --- lib/core/libimagstore/src/store.rs | 48 +++++++++++++----------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs index b2e9beba..66c5b042 100644 --- a/lib/core/libimagstore/src/store.rs +++ b/lib/core/libimagstore/src/store.rs @@ -1067,7 +1067,16 @@ pub trait Header { impl Header for Value { fn verify(&self) -> Result<()> { - verify_header(self) + if !has_main_section(self)? { + Err(SE::from_kind(SEK::MissingMainSection)) + } else if !has_imag_version_in_main_section(self)? { + Err(SE::from_kind(SEK::MissingVersionInfo)) + } else if !has_only_tables(self)? { + debug!("Could not verify that it only has tables in its base table"); + Err(SE::from_kind(SEK::NonTableInBaseTable)) + } else { + Ok(()) + } } fn parse(s: &str) -> Result { @@ -1075,7 +1084,7 @@ impl Header for Value { from_str(s) .map_err(From::from) - .and_then(verify_header_consistency) + .and_then(|h: Value| h.verify().map(|_| h)) } fn default_header() -> Value { @@ -1094,23 +1103,6 @@ impl Header for Value { } -fn verify_header_consistency(t: Value) -> Result { - verify_header(&t).chain_err(|| SEK::HeaderInconsistency).map(|_| t) -} - -fn verify_header(t: &Value) -> Result<()> { - if !has_main_section(t)? { - Err(SE::from_kind(SEK::MissingMainSection)) - } else if !has_imag_version_in_main_section(t)? { - Err(SE::from_kind(SEK::MissingVersionInfo)) - } else if !has_only_tables(t)? { - debug!("Could not verify that it only has tables in its base table"); - Err(SE::from_kind(SEK::NonTableInBaseTable)) - } else { - Ok(()) - } -} - fn has_only_tables(t: &Value) -> Result { debug!("Verifying that table has only tables"); match *t { @@ -1142,9 +1134,9 @@ mod test { use std::collections::BTreeMap; use storeid::StoreId; + use store::Header; use store::has_main_section; use store::has_imag_version_in_main_section; - use store::verify_header_consistency; use toml::Value; @@ -1153,7 +1145,7 @@ mod test { let mut map = BTreeMap::new(); map.insert("imag".into(), Value::Table(BTreeMap::new())); - assert!(has_main_section(&map)); + assert!(has_main_section(&Value::Table(map)).unwrap()); } #[test] @@ -1169,7 +1161,7 @@ mod test { let mut map = BTreeMap::new(); map.insert("not_imag".into(), Value::Boolean(false)); - assert!(!has_main_section(&map)); + assert!(has_main_section(&Value::Table(map)).is_err()); } #[test] @@ -1177,7 +1169,7 @@ mod test { let mut map = BTreeMap::new(); map.insert("imag".into(), Value::Table(BTreeMap::new())); - assert!(has_imag_version_in_main_section(&map).is_err()); + assert!(has_imag_version_in_main_section(&Value::Table(map)).is_err()); } #[test] @@ -1187,7 +1179,7 @@ mod test { sub.insert("version".into(), Value::String("0.0.0".into())); map.insert("imag".into(), Value::Table(sub)); - assert!(has_imag_version_in_main_section(&map)?); + assert!(has_imag_version_in_main_section(&Value::Table(map)).unwrap()); } #[test] @@ -1197,7 +1189,7 @@ mod test { sub.insert("version".into(), Value::Boolean(false)); map.insert("imag".into(), Value::Table(sub)); - assert!(has_imag_version_in_main_section(&map).is_err()); + assert!(has_imag_version_in_main_section(&Value::Table(map)).is_err()); } #[test] @@ -1212,7 +1204,7 @@ mod test { header.insert("imag".into(), sub); - assert!(verify_header_consistency(header).is_ok()); + assert!(Value::Table(header).verify().is_ok()); } #[test] @@ -1227,7 +1219,7 @@ mod test { header.insert("imag".into(), sub); - assert!(!verify_header_consistency(header).is_ok()); + assert!(!Value::Table(header).verify().is_ok()); } @@ -1243,7 +1235,7 @@ mod test { header.insert("imag".into(), sub); - assert!(verify_header_consistency(header).is_ok()); + assert!(Value::Table(header).verify().is_ok()); } static TEST_ENTRY : &'static str = "--- From 9128d048664e087aa6439b5e5c7b0891e7250615 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 22 Dec 2017 12:02:07 +0100 Subject: [PATCH 4/4] Remove test This test is not applicable anymore because it tests (and tested) the wrong thing. It was to check whether the function failed because the "imag" key contained the wrong type, but this is not tested by that function. The function only checks whether the "imag" key is present. --- lib/core/libimagstore/src/store.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs index 66c5b042..5b08226f 100644 --- a/lib/core/libimagstore/src/store.rs +++ b/lib/core/libimagstore/src/store.rs @@ -1148,14 +1148,6 @@ mod test { assert!(has_main_section(&Value::Table(map)).unwrap()); } - #[test] - fn test_imag_invalid_section_type() { - let mut map = BTreeMap::new(); - map.insert("imag".into(), Value::Boolean(false)); - - assert!(!has_main_section(&map)); - } - #[test] fn test_imag_abscent_main_section() { let mut map = BTreeMap::new();