From 85097554e9496ef3f6a0127f2aa5c89f1d7dff25 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Wed, 7 Sep 2016 12:47:52 +0200 Subject: [PATCH 1/3] Add hook implementation for succeeding hook tests --- libimagstore/src/store.rs | 106 +++++++++++++++++++++++++++++++++++++- 1 file changed, 105 insertions(+), 1 deletion(-) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index e8efd29c..9b90f89e 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -2154,7 +2154,7 @@ mod store_tests { use super::Store; - fn get_store() -> Store { + pub fn get_store() -> Store { Store::new(PathBuf::from("/"), None).unwrap() } @@ -2275,3 +2275,107 @@ mod store_tests { } +#[cfg(test)] +mod store_hook_tests { + use super::store_tests::get_store; + + mod succeeding_hook { + use hook::Hook; + use hook::accessor::HookDataAccessor; + use hook::accessor::HookDataAccessorProvider; + use hook::position::HookPosition; + + use self::accessor::SucceedingHookAccessor as DHA; + + use toml::Value; + + #[derive(Debug)] + pub struct SucceedingHook { + position: HookPosition, + accessor: DHA, + } + + impl SucceedingHook { + + pub fn new(pos: HookPosition) -> SucceedingHook { + SucceedingHook { position: pos.clone(), accessor: DHA::new(pos) } + } + + } + + impl Hook for SucceedingHook { + fn name(&self) -> &'static str { "testhook_succeeding" } + fn set_config(&mut self, _: &Value) { } + } + + impl HookDataAccessorProvider for SucceedingHook { + + fn accessor(&self) -> HookDataAccessor { + use hook::position::HookPosition as HP; + use hook::accessor::HookDataAccessor as HDA; + + match self.position { + HP::StoreUnload | + HP::PreCreate | + HP::PreRetrieve | + HP::PreDelete | + HP::PostDelete => HDA::StoreIdAccess(&self.accessor), + HP::PostCreate | + HP::PostRetrieve | + HP::PreUpdate | + HP::PostUpdate => HDA::MutableAccess(&self.accessor), + } + } + + } + + pub mod accessor { + use hook::result::HookResult; + use hook::accessor::MutableHookDataAccessor; + use hook::accessor::NonMutableHookDataAccessor; + use hook::accessor::StoreIdAccessor; + use hook::position::HookPosition; + use store::FileLockEntry; + use storeid::StoreId; + + #[derive(Debug)] + pub struct SucceedingHookAccessor(HookPosition); + + impl SucceedingHookAccessor { + + pub fn new(position: HookPosition) -> SucceedingHookAccessor { + SucceedingHookAccessor(position) + } + + } + + impl StoreIdAccessor for SucceedingHookAccessor { + + fn access(&self, id: &StoreId) -> HookResult<()> { + Ok(()) + } + + } + + impl MutableHookDataAccessor for SucceedingHookAccessor { + + fn access_mut(&self, fle: &mut FileLockEntry) -> HookResult<()> { + Ok(()) + } + + } + + impl NonMutableHookDataAccessor for SucceedingHookAccessor { + + fn access(&self, fle: &FileLockEntry) -> HookResult<()> { + Ok(()) + } + + } + + } + + } + +} + From 83ebe88022e848273b0ae2765cef854a9d8ed167 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Sun, 18 Sep 2016 19:49:50 +0200 Subject: [PATCH 2/3] Add PreCreate hook test --- libimagstore/src/store.rs | 53 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 9b90f89e..997f45b5 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -2277,7 +2277,6 @@ mod store_tests { #[cfg(test)] mod store_hook_tests { - use super::store_tests::get_store; mod succeeding_hook { use hook::Hook; @@ -2377,5 +2376,57 @@ mod store_hook_tests { } + use std::path::PathBuf; + + use hook::position::HookPosition as HP; + use storeid::StoreId; + use store::Store; + + use self::succeeding_hook::SucceedingHook; + + fn get_store_with_config() -> Store { + use toml::Parser; + + let cfg = Parser::new(mini_config()).parse().unwrap(); + println!("Config parsed: {:?}", cfg); + Store::new(PathBuf::from("/"), Some(cfg.get("store").cloned().unwrap())).unwrap() + } + + fn mini_config() -> &'static str { + r#" +[store] +store-unload-hook-aspects = [ "test" ] +pre-create-hook-aspects = [ "test" ] +post-create-hook-aspects = [ "test" ] +pre-move-hook-aspects = [ "test" ] +post-move-hook-aspects = [ "test" ] +pre-retrieve-hook-aspects = [ "test" ] +post-retrieve-hook-aspects = [ "test" ] +pre-update-hook-aspects = [ "test" ] +post-update-hook-aspects = [ "test" ] +pre-delete-hook-aspects = [ "test" ] +post-delete-hook-aspects = [ "test" ] + +[store.aspects.test] +parallel = false +mutable_hooks = true + +[store.hooks.testhook_succeeding] +aspect = "test" + "# + } + + #[test] + fn test_pre_create() { + let mut store = get_store_with_config(); + let pos = HP::PreCreate; + let hook = SucceedingHook::new(pos.clone()); + + assert!(store.register_hook(pos, "test", Box::new(hook)).map_err(|e| println!("{:?}", e)).is_ok()); + + let pb = StoreId::new_baseless(PathBuf::from("test")).unwrap(); + assert!(store.create(pb.clone()).is_ok()); + } + } From 72a95ee5daeb6a19a49408a1c84df95dfcd90d8c Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Mon, 19 Sep 2016 21:00:45 +0200 Subject: [PATCH 3/3] Rewrite config validity checker to return Result<()> And add more detailed error kinds for config errors --- libimagstore/src/configuration.rs | 139 +++++++++++++++++------------- libimagstore/src/error.rs | 13 +++ libimagstore/src/store.rs | 4 +- 3 files changed, 93 insertions(+), 63 deletions(-) diff --git a/libimagstore/src/configuration.rs b/libimagstore/src/configuration.rs index ff94a1a3..c450000c 100644 --- a/libimagstore/src/configuration.rs +++ b/libimagstore/src/configuration.rs @@ -1,5 +1,9 @@ use toml::Value; +use libimagerror::into::IntoError; + +use store::Result; + /// Check whether the configuration is valid for the store /// /// The passed `Value` _must be_ the `[store]` sub-tree of the configuration. Otherwise this will @@ -42,32 +46,42 @@ use toml::Value; /// You have been warned! /// /// -pub fn config_is_valid(config: &Option) -> bool { +pub fn config_is_valid(config: &Option) -> Result<()> { use std::collections::BTreeMap; - use std::io::Write; - use std::io::stderr; + use error::StoreErrorKind as SEK; if config.is_none() { - return true; + return Ok(()); } - fn has_key_with_string_ary(v: &BTreeMap, key: &str) -> bool { + /// Check whether the config has a key with a string array. + /// The `key` is the key which is checked + /// The `kind` is the error kind which is used as `cause` if there is an error, so we can + /// indicate via error type which key is missing + fn has_key_with_string_ary(v: &BTreeMap, key: &str, + kind: SEK) -> Result<()> { v.get(key) - .map_or_else(|| { - write!(stderr(), "Required key '{}' is not in store config", key).ok(); - false - }, |t| match *t { - Value::Array(ref a) => a.iter().all(|elem| { - match *elem { - Value::String(_) => true, - _ => false, - } - }), - _ => { - write!(stderr(), "Key '{}' in store config should contain an array", key) - .ok(); - false - } + .ok_or_else(|| { + warn!("Required key '{}' is not in store config", key); + SEK::ConfigKeyMissingError.into_error_with_cause(Box::new(kind.into_error())) + }) + .and_then(|t| match *t { + Value::Array(ref a) => { + a.iter().fold(Ok(()), |acc, elem| { + acc.and_then(|_| { + if is_match!(*elem, Value::String(_)) { + Ok(()) + } else { + let cause = Box::new(kind.into_error()); + Err(SEK::ConfigTypeError.into_error_with_cause(cause)) + } + }) + }) + }, + _ => { + warn!("Key '{}' in store config should contain an array", key); + Err(SEK::ConfigTypeError.into_error_with_cause(Box::new(kind.into_error()))) + } }) } @@ -82,59 +96,64 @@ pub fn config_is_valid(config: &Option) -> bool { section: &str, key: &str, f: F) - -> bool + -> Result<()> where F: Fn(&Value) -> bool { store_config.get(section) // The store config has the section `section` - .map_or_else(|| { - write!(stderr(), "Store config expects section '{}' to be present, but isn't.", - section).ok(); - false - }, |section_table| { - match *section_table { // which is - Value::Table(ref section_table) => // a table - section_table - .iter() // which has values, - .all(|(inner_key, cfg)| { // and all of these values - match *cfg { - Value::Table(ref hook_config) => { // are tables - hook_config.get(key) // with a key - // fullfilling this constraint - .map_or(false, |hook_aspect| f(&hook_aspect)) - }, - _ => { - write!(stderr(), "Store config expects '{}' to be in '{}.{}', but isn't.", - key, section, inner_key).ok(); - false + .ok_or_else(|| { + warn!("Store config expects section '{}' to be present, but isn't.", section); + SEK::ConfigKeyMissingError.into_error() + }) + .and_then(|section_table| match *section_table { // which is + Value::Table(ref section_table) => // a table + section_table.iter().fold(Ok(()), |acc, (inner_key, cfg)| { + acc.and_then(|_| { + match *cfg { + Value::Table(ref hook_config) => { // are tables + // with a key + let hook_aspect_is_valid = try!(hook_config.get(key) + .map(|hook_aspect| f(&hook_aspect)) + .ok_or(SEK::ConfigKeyMissingError.into_error()) + ); + + if !hook_aspect_is_valid { + Err(SEK::ConfigTypeError.into_error()) + } else { + Ok(()) } + }, + _ => { + warn!("Store config expects '{}' to be in '{}.{}', but isn't.", + key, section, inner_key); + Err(SEK::ConfigKeyMissingError.into_error()) } - }), - _ => { - write!(stderr(), "Store config expects '{}' to be a Table, but isn't.", - section).ok(); - false - } + } + }) + }), + _ => { + warn!("Store config expects '{}' to be a Table, but isn't.", section); + Err(SEK::ConfigTypeError.into_error()) } }) } match *config { Some(Value::Table(ref t)) => { - has_key_with_string_ary(t, "store-unload-hook-aspects") && + try!(has_key_with_string_ary(t, "store-unload-hook-aspects", SEK::ConfigKeyUnloadAspectsError)); - has_key_with_string_ary(t, "pre-create-hook-aspects") && - has_key_with_string_ary(t, "post-create-hook-aspects") && - has_key_with_string_ary(t, "pre-retrieve-hook-aspects") && - has_key_with_string_ary(t, "post-retrieve-hook-aspects") && - has_key_with_string_ary(t, "pre-update-hook-aspects") && - has_key_with_string_ary(t, "post-update-hook-aspects") && - has_key_with_string_ary(t, "pre-delete-hook-aspects") && - has_key_with_string_ary(t, "post-delete-hook-aspects") && + try!(has_key_with_string_ary(t, "pre-create-hook-aspects", SEK::ConfigKeyPreCreateAspectsError)); + try!(has_key_with_string_ary(t, "post-create-hook-aspects", SEK::ConfigKeyPostCreateAspectsError)); + try!(has_key_with_string_ary(t, "pre-retrieve-hook-aspects", SEK::ConfigKeyPreRetrieveAspectsError)); + try!(has_key_with_string_ary(t, "post-retrieve-hook-aspects", SEK::ConfigKeyPostRetrieveAspectsError)); + try!(has_key_with_string_ary(t, "pre-update-hook-aspects", SEK::ConfigKeyPreUpdateAspectsError)); + try!(has_key_with_string_ary(t, "post-update-hook-aspects", SEK::ConfigKeyPostUpdateAspectsError)); + try!(has_key_with_string_ary(t, "pre-delete-hook-aspects", SEK::ConfigKeyPreDeleteAspectsError)); + try!(has_key_with_string_ary(t, "post-delete-hook-aspects", SEK::ConfigKeyPostDeleteAspectsError)); // The section "hooks" has maps which have a key "aspect" which has a value of type // String - check_all_inner_maps_have_key_with(t, "hooks", "aspect", - |asp| is_match!(asp, &Value::String(_))) && + try!(check_all_inner_maps_have_key_with(t, "hooks", "aspect", + |asp| is_match!(asp, &Value::String(_)))); // The section "aspects" has maps which have a key "parllel" which has a value of type // Boolean @@ -142,8 +161,8 @@ pub fn config_is_valid(config: &Option) -> bool { |asp| is_match!(asp, &Value::Boolean(_))) } _ => { - write!(stderr(), "Store config is no table").ok(); - false + warn!("Store config is no table"); + Err(SEK::ConfigTypeError.into_error()) }, } } diff --git a/libimagstore/src/error.rs b/libimagstore/src/error.rs index fe0455c1..80c10a0c 100644 --- a/libimagstore/src/error.rs +++ b/libimagstore/src/error.rs @@ -6,6 +6,19 @@ pub struct CustomErrorData {} generate_custom_error_types!(StoreError, StoreErrorKind, CustomErrorData, ConfigurationError => "Store Configuration Error", + ConfigTypeError => "Store configuration type error", + ConfigKeyMissingError => "Configuration Key missing", + + ConfigKeyUnloadAspectsError => "Config Key 'store-unload-hook-aspects' caused an error", + ConfigKeyPreCreateAspectsError => "Config Key 'pre-create-hook-aspects' caused an error", + ConfigKeyPostCreateAspectsError => "Config Key 'post-create-hook-aspects' caused an error", + ConfigKeyPreRetrieveAspectsError => "Config Key 'pre-retrieve-hook-aspect' caused an error", + ConfigKeyPostRetrieveAspectsError => "Config Key 'post-retrieve-hook-aspec' caused an error", + ConfigKeyPreUpdateAspectsError => "Config Key 'pre-update-hook-aspects' caused an error", + ConfigKeyPostUpdateAspectsError => "Config Key 'post-update-hook-aspects' caused an error", + ConfigKeyPreDeleteAspectsError => "Config Key 'pre-delete-hook-aspects' caused an error", + ConfigKeyPostDeleteAspectsError => "Config Key 'post-delete-hook-aspects' caused an error", + CreateStoreDirDenied => "Creating store directory implicitely denied", FileError => "File Error", IoError => "IO Error", diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 997f45b5..c4a041e5 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -213,9 +213,7 @@ impl Store { use configuration::*; debug!("Validating Store configuration"); - if !config_is_valid(&store_config) { - return Err(SE::new(SEK::ConfigurationError, None)); - } + let _ = try!(config_is_valid(&store_config).map_err_into(SEK::ConfigurationError)); debug!("Building new Store object"); if !location.exists() {