From 38b56df4067d985001cc6fe84423a63bd50ad17a Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 22 Dec 2017 11:20:30 +0100 Subject: [PATCH 1/3] Add new dependency: toml-query --- lib/core/libimagstore/Cargo.toml | 1 + lib/core/libimagstore/src/error.rs | 1 + lib/core/libimagstore/src/lib.rs | 1 + 3 files changed, 3 insertions(+) diff --git a/lib/core/libimagstore/Cargo.toml b/lib/core/libimagstore/Cargo.toml index 0fce876d..cba27897 100644 --- a/lib/core/libimagstore/Cargo.toml +++ b/lib/core/libimagstore/Cargo.toml @@ -33,6 +33,7 @@ serde = "1" serde_json = "1" serde_derive = "1" error-chain = "0.11" +toml-query = "0.4" libimagerror = { version = "0.5.0", path = "../../../lib/core/libimagerror" } libimagutil = { version = "0.5.0", path = "../../../lib/etc/libimagutil" } diff --git a/lib/core/libimagstore/src/error.rs b/lib/core/libimagstore/src/error.rs index fec24d9e..f04b2617 100644 --- a/lib/core/libimagstore/src/error.rs +++ b/lib/core/libimagstore/src/error.rs @@ -30,6 +30,7 @@ error_chain! { Io(::std::io::Error); TomlDeserError(::toml::de::Error); GlobPatternError(::glob::PatternError); + TomlQueryError(::toml_query::error::Error); } errors { diff --git a/lib/core/libimagstore/src/lib.rs b/lib/core/libimagstore/src/lib.rs index 3b665a5e..4ce4ccfb 100644 --- a/lib/core/libimagstore/src/lib.rs +++ b/lib/core/libimagstore/src/lib.rs @@ -48,6 +48,7 @@ extern crate walkdir; extern crate serde_json; #[macro_use] extern crate serde_derive; #[macro_use] extern crate error_chain; +extern crate toml_query; extern crate libimagerror; extern crate libimagutil; From 174d8d76e97bc0c30809fc58ae93185d2770ab57 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 22 Dec 2017 11:20:53 +0100 Subject: [PATCH 2/3] Remove configuration member, reduce configuration We only need the configuration to check whether creating the store directory is actually allowed. --- lib/core/libimagstore/src/configuration.rs | 51 ++++++---------------- lib/core/libimagstore/src/error.rs | 8 ++-- lib/core/libimagstore/src/store.rs | 27 +++--------- 3 files changed, 22 insertions(+), 64 deletions(-) diff --git a/lib/core/libimagstore/src/configuration.rs b/lib/core/libimagstore/src/configuration.rs index dc60311d..9b855d13 100644 --- a/lib/core/libimagstore/src/configuration.rs +++ b/lib/core/libimagstore/src/configuration.rs @@ -21,48 +21,23 @@ use toml::Value; use store::Result; use error::StoreError as SE; +use error::StoreErrorKind as SEK; -/// Check whether the configuration is valid for the store -pub fn config_is_valid(config: &Option) -> Result<()> { - use error::StoreErrorKind as SEK; - - if config.is_none() { - return Ok(()); - } - - match *config { - Some(Value::Table(_)) => Ok(()), - _ => { - warn!("Store config is no table"); - Err(SE::from_kind(SEK::ConfigTypeError)) - }, - } -} +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<&Value>) -> bool { - config.map(|t| { - match *t { - Value::Table(ref t) => { - match t.get("implicit-create") { - Some(&Value::Boolean(b)) => b, - Some(_) => { - warn!("Key 'implicit-create' does not contain a Boolean value"); - false - } - None => { - warn!("Key 'implicit-create' in store configuration missing"); - false - }, - } - } - _ => { - warn!("Store configuration seems to be no Table"); - false - }, - } - }).unwrap_or(false) +pub fn config_implicit_store_create_allowed(config: Option<&Value>) -> Result { + let key = "implicit-create"; + + if let Some(t) = config { + t.read(key)? + .ok_or(SE::from_kind(SEK::ConfigKeyMissingError(key)))? + .as_bool() + .ok_or(SE::from_kind(SEK::ConfigTypeError(key, "boolean"))) + } else { + Ok(false) + } } #[cfg(test)] diff --git a/lib/core/libimagstore/src/error.rs b/lib/core/libimagstore/src/error.rs index f04b2617..2bb71726 100644 --- a/lib/core/libimagstore/src/error.rs +++ b/lib/core/libimagstore/src/error.rs @@ -40,14 +40,14 @@ error_chain! { display("Store Configuration Error") } - ConfigTypeError { + ConfigTypeError(key: &'static str, expected: &'static str) { description("Store configuration type error") - display("Store configuration type error") + display("Store configuration type error at '{}', expected {}", key, expected) } - ConfigKeyMissingError { + ConfigKeyMissingError(key: &'static str) { description("Configuration Key missing") - display("Configuration Key missing") + display("Configuration Key missing: '{}'", key) } VersionError { diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs index 860d9682..c42b902d 100644 --- a/lib/core/libimagstore/src/store.rs +++ b/lib/core/libimagstore/src/store.rs @@ -204,11 +204,6 @@ impl Drop for StoreEntry { pub struct Store { location: PathBuf, - /// - /// Configuration object of the store - /// - configuration: Option, - /// /// Internal Path->File cache map /// @@ -228,10 +223,8 @@ impl Store { /// Create a new Store object /// - /// This opens a Store in `location` using the configuration from `store_config` (if absent, it - /// uses defaults). - /// - /// If the configuration is not valid, this fails. + /// This opens a Store in `location`. The store_config is used to check whether creating the + /// store implicitely is allowed. /// /// If the location does not exist, creating directories is by default denied and the operation /// fails, if not configured otherwise. @@ -243,7 +236,7 @@ impl Store { /// /// - On success: Store object /// - pub fn new(location: PathBuf, store_config: Option) -> Result { + pub fn new(location: PathBuf, store_config: Option<&Value>) -> Result { let backend = Box::new(FSFileAbstraction::new()); Store::new_with_backend(location, store_config, backend) } @@ -253,16 +246,13 @@ impl Store { /// /// Do not use directly, only for testing purposes. pub fn new_with_backend(location: PathBuf, - store_config: Option, + store_config: Option<&Value>, backend: Box) -> Result { use configuration::*; - debug!("Validating Store configuration"); - let _ = config_is_valid(&store_config).chain_err(|| SEK::ConfigurationError)?; - debug!("Building new Store object"); if !location.exists() { - if !config_implicit_store_create_allowed(store_config.as_ref()) { + if !config_implicit_store_create_allowed(store_config)? { return Err(SE::from_kind(SEK::CreateStoreDirDenied)) .chain_err(|| SEK::FileError) .chain_err(|| SEK::IoError); @@ -279,7 +269,6 @@ impl Store { let store = Store { location: location.clone(), - configuration: store_config, entries: Arc::new(RwLock::new(HashMap::new())), backend: backend, }; @@ -319,11 +308,6 @@ impl Store { .map(|_| self.backend = backend) } - /// Get the store configuration - pub fn config(&self) -> Option<&Value> { - self.configuration.as_ref() - } - /// Creates the Entry at the given location (inside the entry) /// /// # Return value @@ -786,7 +770,6 @@ impl Debug for Store { write!(fmt, " --- Store ---\n")?; write!(fmt, "\n")?; write!(fmt, " - location : {:?}\n", self.location)?; - write!(fmt, " - configuration : {:?}\n", self.configuration)?; write!(fmt, "\n")?; write!(fmt, "Entries:\n")?; write!(fmt, "{:?}", self.entries)?; From 0ed636bb06d3cf748377f53556adeae136f995b8 Mon Sep 17 00:00:00 2001 From: Matthias Beyer Date: Fri, 22 Dec 2017 11:24:04 +0100 Subject: [PATCH 3/3] Refactor libimagrt+libimagstore to pass whole configuration object Before we extracted the store configuration from the configuration toml::Value object and passed it to the store. This is unecessary overhead. Now we pass the whole configuration object and let the store extract the required values. --- lib/core/libimagrt/src/runtime.rs | 15 ++------------- lib/core/libimagstore/src/configuration.rs | 16 +++++++++------- lib/core/libimagstore/src/store.rs | 12 ++++++------ lib/domain/libimagtimetrack/src/iter/mod.rs | 2 +- lib/entry/libimagentrycategory/src/register.rs | 2 +- lib/entry/libimagentrydatetime/src/datetime.rs | 2 +- lib/entry/libimagentrygps/src/entry.rs | 2 +- lib/entry/libimagentrylink/src/external.rs | 2 +- lib/entry/libimagentrylink/src/internal.rs | 2 +- lib/entry/libimagentrymarkdown/src/processor.rs | 2 +- 10 files changed, 24 insertions(+), 33 deletions(-) diff --git a/lib/core/libimagrt/src/runtime.rs b/lib/core/libimagrt/src/runtime.rs index 42f5d5ba..9f46feb3 100644 --- a/lib/core/libimagrt/src/runtime.rs +++ b/lib/core/libimagrt/src/runtime.rs @@ -24,7 +24,6 @@ use std::process::exit; pub use clap::App; use toml::Value; -use toml_query::read::TomlValueReadExt; use clap::{Arg, ArgMatches}; use log; @@ -138,22 +137,12 @@ impl<'a> Runtime<'a> { debug!("RTP path = {:?}", rtp); debug!("Store path = {:?}", storepath); - let store_config = match config { - Some(ref c) => c.read("store").chain_err(|| RuntimeErrorKind::Instantiate)?.cloned(), - None => None, - }; - - if matches.is_present(Runtime::arg_debugging_name()) { - debug!("Config: {:?}\n", config); - debug!("Store-config: {:?}\n", store_config); - } - let store_result = if cli_app.use_inmemory_fs() { Store::new_with_backend(storepath, - store_config, + &config, Box::new(InMemoryFileAbstraction::new())) } else { - Store::new(storepath, store_config) + Store::new(storepath, &config) }; store_result.map(|store| { diff --git a/lib/core/libimagstore/src/configuration.rs b/lib/core/libimagstore/src/configuration.rs index 9b855d13..d239d9dd 100644 --- a/lib/core/libimagstore/src/configuration.rs +++ b/lib/core/libimagstore/src/configuration.rs @@ -27,10 +27,10 @@ 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<&Value>) -> Result { - let key = "implicit-create"; +pub fn config_implicit_store_create_allowed(config: &Option) -> Result { + let key = "store.implicit-create"; - if let Some(t) = config { + if let Some(ref t) = *config { t.read(key)? .ok_or(SE::from_kind(SEK::ConfigKeyMissingError(key)))? .as_bool() @@ -47,31 +47,33 @@ mod tests { #[test] fn test_implicit_store_create_allowed_no_toml() { - assert!(!config_implicit_store_create_allowed(None)); + assert!(!config_implicit_store_create_allowed(&None).unwrap()); } #[test] fn test_implicit_store_create_allowed_toml_empty() { let config = toml_from_str("").unwrap(); - assert!(!config_implicit_store_create_allowed(Some(config).as_ref())); + assert!(config_implicit_store_create_allowed(&Some(config)).is_err()); } #[test] fn test_implicit_store_create_allowed_toml_false() { let config = toml_from_str(r#" + [store] implicit-create = false "#).unwrap(); - assert!(!config_implicit_store_create_allowed(Some(config).as_ref())); + assert!(!config_implicit_store_create_allowed(&Some(config)).unwrap()); } #[test] fn test_implicit_store_create_allowed_toml_true() { let config = toml_from_str(r#" + [store] implicit-create = true "#).unwrap(); - assert!(config_implicit_store_create_allowed(Some(config).as_ref())); + assert!(config_implicit_store_create_allowed(&Some(config)).unwrap()); } } diff --git a/lib/core/libimagstore/src/store.rs b/lib/core/libimagstore/src/store.rs index c42b902d..86bd08f4 100644 --- a/lib/core/libimagstore/src/store.rs +++ b/lib/core/libimagstore/src/store.rs @@ -236,7 +236,7 @@ impl Store { /// /// - On success: Store object /// - pub fn new(location: PathBuf, store_config: Option<&Value>) -> Result { + pub fn new(location: PathBuf, store_config: &Option) -> Result { let backend = Box::new(FSFileAbstraction::new()); Store::new_with_backend(location, store_config, backend) } @@ -246,7 +246,7 @@ impl Store { /// /// Do not use directly, only for testing purposes. pub fn new_with_backend(location: PathBuf, - store_config: Option<&Value>, + store_config: &Option, backend: Box) -> Result { use configuration::*; @@ -1293,7 +1293,7 @@ mod store_tests { pub fn get_store() -> Store { let backend = Box::new(InMemoryFileAbstraction::new()); - Store::new_with_backend(PathBuf::from("/"), None, backend).unwrap() + Store::new_with_backend(PathBuf::from("/"), &None, backend).unwrap() } #[test] @@ -1346,7 +1346,7 @@ mod store_tests { let backend = StdIoFileAbstraction::new(&mut input, output.clone(), mapper).unwrap(); let backend = Box::new(backend); - Store::new_with_backend(PathBuf::from("/"), None, backend).unwrap() + Store::new_with_backend(PathBuf::from("/"), &None, backend).unwrap() }; for n in 1..100 { @@ -1619,7 +1619,7 @@ mod store_tests { let backend = InMemoryFileAbstraction::new(); let backend = Box::new(backend); - Store::new_with_backend(PathBuf::from("/"), None, backend).unwrap() + Store::new_with_backend(PathBuf::from("/"), &None, backend).unwrap() }; for n in 1..100 { @@ -1694,7 +1694,7 @@ mod store_tests { let backend = StdIoFileAbstraction::new(&mut input, output, mapper).unwrap(); let backend = Box::new(backend); - Store::new_with_backend(PathBuf::from("/"), None, backend).unwrap() + Store::new_with_backend(PathBuf::from("/"), &None, backend).unwrap() }; // Replacing the backend diff --git a/lib/domain/libimagtimetrack/src/iter/mod.rs b/lib/domain/libimagtimetrack/src/iter/mod.rs index 294f04c3..79720995 100644 --- a/lib/domain/libimagtimetrack/src/iter/mod.rs +++ b/lib/domain/libimagtimetrack/src/iter/mod.rs @@ -38,7 +38,7 @@ mod test { use libimagstore::file_abstraction::InMemoryFileAbstraction; let backend = Box::new(InMemoryFileAbstraction::new()); - Store::new_with_backend(PathBuf::from("/"), None, backend).unwrap() + Store::new_with_backend(PathBuf::from("/"), &None, backend).unwrap() } #[test] diff --git a/lib/entry/libimagentrycategory/src/register.rs b/lib/entry/libimagentrycategory/src/register.rs index af23d5bf..d5b40584 100644 --- a/lib/entry/libimagentrycategory/src/register.rs +++ b/lib/entry/libimagentrycategory/src/register.rs @@ -130,7 +130,7 @@ mod tests { pub fn get_store() -> Store { use libimagstore::store::InMemoryFileAbstraction; let backend = Box::new(InMemoryFileAbstraction::new()); - Store::new_with_backend(PathBuf::from("/"), None, backend).unwrap() + Store::new_with_backend(PathBuf::from("/"), &None, backend).unwrap() } #[test] diff --git a/lib/entry/libimagentrydatetime/src/datetime.rs b/lib/entry/libimagentrydatetime/src/datetime.rs index 692f1d53..efe90280 100644 --- a/lib/entry/libimagentrydatetime/src/datetime.rs +++ b/lib/entry/libimagentrydatetime/src/datetime.rs @@ -225,7 +225,7 @@ mod tests { pub fn get_store() -> Store { use libimagstore::store::InMemoryFileAbstraction; let backend = Box::new(InMemoryFileAbstraction::new()); - Store::new_with_backend(PathBuf::from("/"), None, backend).unwrap() + Store::new_with_backend(PathBuf::from("/"), &None, backend).unwrap() } #[test] diff --git a/lib/entry/libimagentrygps/src/entry.rs b/lib/entry/libimagentrygps/src/entry.rs index 3383e25d..3b689fd2 100644 --- a/lib/entry/libimagentrygps/src/entry.rs +++ b/lib/entry/libimagentrygps/src/entry.rs @@ -116,7 +116,7 @@ mod tests { fn get_store() -> Store { use libimagstore::file_abstraction::InMemoryFileAbstraction; let backend = Box::new(InMemoryFileAbstraction::new()); - Store::new_with_backend(PathBuf::from("/"), None, backend).unwrap() + Store::new_with_backend(PathBuf::from("/"), &None, backend).unwrap() } #[test] diff --git a/lib/entry/libimagentrylink/src/external.rs b/lib/entry/libimagentrylink/src/external.rs index c3eccfa6..2aab7747 100644 --- a/lib/entry/libimagentrylink/src/external.rs +++ b/lib/entry/libimagentrylink/src/external.rs @@ -421,7 +421,7 @@ mod tests { pub fn get_store() -> Store { use libimagstore::file_abstraction::InMemoryFileAbstraction; let backend = Box::new(InMemoryFileAbstraction::new()); - Store::new_with_backend(PathBuf::from("/"), None, backend).unwrap() + Store::new_with_backend(PathBuf::from("/"), &None, backend).unwrap() } diff --git a/lib/entry/libimagentrylink/src/internal.rs b/lib/entry/libimagentrylink/src/internal.rs index 1598feb9..d339ff8d 100644 --- a/lib/entry/libimagentrylink/src/internal.rs +++ b/lib/entry/libimagentrylink/src/internal.rs @@ -778,7 +778,7 @@ mod test { pub fn get_store() -> Store { use libimagstore::file_abstraction::InMemoryFileAbstraction; let backend = Box::new(InMemoryFileAbstraction::new()); - Store::new_with_backend(PathBuf::from("/"), None, backend).unwrap() + Store::new_with_backend(PathBuf::from("/"), &None, backend).unwrap() } #[test] diff --git a/lib/entry/libimagentrymarkdown/src/processor.rs b/lib/entry/libimagentrymarkdown/src/processor.rs index 943990a0..60952061 100644 --- a/lib/entry/libimagentrymarkdown/src/processor.rs +++ b/lib/entry/libimagentrymarkdown/src/processor.rs @@ -225,7 +225,7 @@ mod tests { pub fn get_store() -> Store { use libimagstore::file_abstraction::InMemoryFileAbstraction; let fs = InMemoryFileAbstraction::new(); - Store::new_with_backend(PathBuf::from("/"), None, Box::new(fs)).unwrap() + Store::new_with_backend(PathBuf::from("/"), &None, Box::new(fs)).unwrap() } #[test]