From fd2f4c5bdd3f571cdad76924deb2af6a0410043b Mon Sep 17 00:00:00 2001 From: Joshua Thijssen Date: Sun, 19 Nov 2023 22:18:33 +0100 Subject: [PATCH 1/3] Implemented config! and config_set! macros and using a global configuration store. --- Cargo.lock | 17 ++ Cargo.toml | 2 + src/bin/config-store.rs | 27 +- src/config.rs | 264 ++++++++++++++---- src/config/settings.rs | 148 ++++++++++ src/config/storage.rs | 6 +- .../storage/{json_storage.rs => json.rs} | 45 +-- src/config/storage/memory.rs | 36 +++ src/config/storage/memory_storage.rs | 32 --- .../storage/{sqlite_storage.rs => sqlite.rs} | 29 +- src/dns.rs | 6 +- 11 files changed, 475 insertions(+), 137 deletions(-) rename src/config/storage/{json_storage.rs => json.rs} (65%) create mode 100644 src/config/storage/memory.rs delete mode 100644 src/config/storage/memory_storage.rs rename src/config/storage/{sqlite_storage.rs => sqlite.rs} (68%) diff --git a/Cargo.lock b/Cargo.lock index 24ed4027f..1bc031a5c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -557,9 +557,11 @@ dependencies = [ "serde", "serde_derive", "serde_json", + "shared_singleton", "simple_logger", "sqlite", "test-case", + "testing_logger", "textwrap", "thiserror", "typed-arena", @@ -1280,6 +1282,12 @@ dependencies = [ "serde", ] +[[package]] +name = "shared_singleton" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e4949e4d0e0c2336f9587c5a30bf005f3b29f3a29b8bec72ca28866c3cfcf9e" + [[package]] name = "simple_logger" version = "4.2.0" @@ -1437,6 +1445,15 @@ dependencies = [ "test-case-core", ] +[[package]] +name = "testing_logger" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6d92b727cb45d33ae956f7f46b966b25f1bc712092aeef9dba5ac798fc89f720" +dependencies = [ + "log", +] + [[package]] name = "textwrap" version = "0.16.0" diff --git a/Cargo.toml b/Cargo.toml index c0ea606e6..8bd57f45d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -56,6 +56,8 @@ log = "0.4.20" domain-lookup-tree = "0.1" hickory-resolver = "0.24.0" simple_logger = "4.2.0" +shared_singleton = "0.1.0" +testing_logger = "0.1.1" [dev-dependencies] criterion = { version = "0.5", features = ["html_reports"] } diff --git a/src/bin/config-store.rs b/src/bin/config-store.rs index 64b67a062..242d43092 100644 --- a/src/bin/config-store.rs +++ b/src/bin/config-store.rs @@ -1,10 +1,11 @@ use anyhow::anyhow; use clap::{Parser, Subcommand}; use derive_more::Display; +use gosub_engine::config; use gosub_engine::config::settings::Setting; -use gosub_engine::config::storage::json_storage::JsonStorageAdapter; -use gosub_engine::config::storage::sqlite_storage::SqliteStorageAdapter; -use gosub_engine::config::{ConfigStore, Store}; +use gosub_engine::config::storage::json::JsonStorageAdapter; +use gosub_engine::config::storage::sqlite::SqliteStorageAdapter; +use gosub_engine::config::StorageAdapter; use std::str::FromStr; #[derive(Debug, Parser)] @@ -74,22 +75,22 @@ struct GlobalOpts { fn main() -> anyhow::Result<()> { let args = Cli::parse(); - let storage_box: Box = match args.global_opts.engine { + let storage: Box = match args.global_opts.engine { Engine::Sqlite => Box::new(SqliteStorageAdapter::try_from(&args.global_opts.path)?), Engine::Json => Box::new(JsonStorageAdapter::try_from(&args.global_opts.path)?), }; - let mut store = ConfigStore::from_storage(storage_box, true)?; + config::config_store_write().set_storage(storage); match args.command { Commands::View { key } => { - if !store.has(&key) { + if !config::config_store().has(&key) { println!("Key not found"); return Ok(()); } - let info = store.get_info(&key).unwrap(); - let value = store.get(&key); + let info = config::config_store().get_info(&key).unwrap(); + let value = config::config_store().get(&key).unwrap(); println!("Key : {}", key); println!("Current Value : {}", value); @@ -97,17 +98,17 @@ fn main() -> anyhow::Result<()> { println!("Description : {}", info.description); } Commands::List => { - for key in store.find("*") { - let value = store.get(&key); + for key in config::config_store().find("*") { + let value = config::config_store().get(&key).unwrap(); println!("{:40}: {}", key, value); } } Commands::Set { key, value } => { - store.set(&key, Setting::from_str(&value).expect("incorrect value")); + config::config_store().set(&key, Setting::from_str(&value).expect("incorrect value")); } Commands::Search { key } => { - for key in store.find(&key) { - let value = store.get(&key); + for key in config::config_store().find(&key) { + let value = config::config_store().get(&key).unwrap(); println!("{:40}: {}", key, value); } } diff --git a/src/config.rs b/src/config.rs index ffa80f7b8..af3fbaf14 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2,16 +2,118 @@ pub mod settings; pub mod storage; use crate::config::settings::{Setting, SettingInfo}; -use crate::types::Result; +use crate::config::storage::memory::MemoryStorageAdapter; +use lazy_static::lazy_static; +use log::warn; use serde_derive::Deserialize; use serde_json::Value; use std::collections::HashMap; use std::mem; use std::str::FromStr; +use std::sync::RwLock; use wildmatch::WildMatch; +/// Settings are stored in a json file, but this is included in the binary for mostly easy editting. const SETTINGS_JSON: &str = include_str!("./config/settings.json"); +/// StoreAdapter is the interface for storing and retrieving settings +/// This can be used to storage settings in a database, json file, etc +/// Note that we need to implement Send so we can send the storage adapter +/// to other threads. +pub trait StorageAdapter: Send + Sync { + /// Retrieves a setting from the storage + fn get(&self, key: &str) -> Option; + + /// Stores a given setting to the storage. Note that "self" is self and not "mut self". We need to be able + /// to storage settings in a non-mutable way. That is mostly possible it seems with a mutex lock that we + /// can get mutable. + fn set(&self, key: &str, value: Setting); + + /// Retrieves all the settings in the storage in one go. This is used for preloading the settings + /// into the ConfigStore and is more performant normally than calling get_setting manually for each + /// setting. + fn all(&self) -> crate::types::Result>; +} + +lazy_static! { + // Initial config store will have a memory storage adapter. It will save within the session, but not + // persist this on disk. + static ref CONFIG_STORE: RwLock = RwLock::new(ConfigStore::default()); +} + +/// Returns a reference to the config store, which is locked by a mutex. +/// Any callers of the config store can just do config::config_store().get("dns.local_resolver.enabled") +pub fn config_store() -> std::sync::RwLockReadGuard<'static, ConfigStore> { + CONFIG_STORE.read().unwrap() +} + +pub fn config_store_write() -> std::sync::RwLockWriteGuard<'static, ConfigStore> { + CONFIG_STORE.write().unwrap() +} + +/// These macro's can be used to simplify the calls to the config store. You can simply do: +/// +/// let enabled = config!(bool "dns.local_resolver.enabled").unwrap(); +/// config_set!(bool "dns.local_resolver.enabled", false); +/// +/// Note that when you cannot find the key, it will return a default value. This is not always +/// what you want, but you can test for existence of the key with config_store().has("key") +#[allow(clippy::crate_in_macro_def)] +#[macro_export] +macro_rules! config { + (string $key:expr) => { + match crate::config::config_store().get($key) { + Some(setting) => setting.to_string(), + None => String::new(), + } + }; + (bool $key:expr) => { + match crate::config::config_store().get($key) { + Some(setting) => setting.to_bool(), + None => false, + } + }; + (uint $key:expr) => { + match crate::config::config_store().get($key) { + Some(setting) => setting.to_uint(), + None => 0, + } + }; + (sint $key:expr) => { + match crate::config::config_store().get($key) { + Some(setting) => setting.to_sint(), + None => 0, + } + }; + (map $key:expr) => { + match crate::config::config_store().get($key) { + Some(setting) => setting.to_map(), + None => std::collections::HashMap::new(), + } + }; +} + +#[allow(clippy::crate_in_macro_def)] +#[macro_export] +macro_rules! config_set { + (string $key:expr, $val:expr) => { + crate::config::config_store().set($key, Setting::String($val)) + }; + (bool $key:expr, $val:expr) => { + crate::config::config_store().set($key, Setting::Bool($val)) + }; + (uint $key:expr, $val:expr) => { + crate::config::config_store().set($key, Setting::UInt($val)) + }; + (sint $key:expr, $val:expr) => { + crate::config::config_store().set($key, Setting::SInt($val)) + }; + (map $key:expr, $val:expr) => { + crate::config::config_store().set($key, Setting::Map($val)) + }; +} + +/// JsonEntry is used for parsing the settings.json file #[derive(Debug, Deserialize)] struct JsonEntry { key: String, @@ -21,58 +123,59 @@ struct JsonEntry { description: String, } -/// StorageAdapter is the interface for storing and retrieving settings -/// This can be used to store settings in a database, json file, etc -pub trait Store { - /// Retrieves a setting from the storage - fn get_setting(&self, key: &str) -> Option; - /// Stores a given setting to the storage - fn set_setting(&mut self, key: &str, value: Setting); - /// Retrieves all the settings in the storage in one go. This is used for preloading the settings - /// into the ConfigStore and is more performant normally than calling get_setting manually for each - /// setting. - fn get_all_settings(&self) -> Result>; -} - -/// Configuration store is the place where the gosub engine can find all configurable options +/// Configuration storage is the place where the gosub engine can find all configurable options pub struct ConfigStore { /// A hashmap of all settings so we can search o(1) time - settings: HashMap, + /// The mutex allows to share between multiple threads, + /// The refcell allows us to use mutable references in a non-mutable way (ie: settings can be + /// stored while doing a immutable get()) + settings: std::sync::Mutex>>, /// A hashmap of all setting descriptions, default values and type information settings_info: HashMap, /// Keys of all settings so we can iterate keys easily setting_keys: Vec, /// The storage adapter used for persisting and loading keys - storage: Box, + storage: Box, } -impl ConfigStore { - /// Creates a new store with the given storage adapter and preloads the store if needed - pub fn from_storage(storage: Box, preload: bool) -> Result { +impl Default for ConfigStore { + fn default() -> Self { let mut store = ConfigStore { - settings: HashMap::new(), + settings: std::sync::Mutex::new(std::cell::RefCell::new(HashMap::new())), settings_info: HashMap::new(), setting_keys: Vec::new(), - storage, + storage: Box::new(MemoryStorageAdapter::new()), }; - // Populate the settings from the json file - store.populate_settings()?; + // Populate the store with the default settings. They may be overwritten by the storage + // as soon as one is added with config::config_store()::set_storage() + let _ = store.populate_default_settings(); + store + } +} + +impl ConfigStore { + /// Sets a new storage engine and updates all settings in the config store according to what + /// is written in the storage. Note that it will overwrite any current settings in the config + /// store. Take this into consideration when using this function to switch storage engines. + pub fn set_storage(&mut self, storage: Box) { + self.storage = storage; - // preload the settings if requested - if preload { - let all_settings = store.storage.get_all_settings()?; + // Find all keys, and add them to the configuration store + if let Ok(all_settings) = self.storage.all() { for (key, value) in all_settings { - store.settings.insert(key, value); + self.settings + .lock() + .unwrap() + .borrow_mut() + .insert(key, value); } } - - Ok(store) } - /// Returns true when the store knows about the given key + /// Returns true when the storage knows about the given key pub fn has(&self, key: &str) -> bool { - self.settings.contains_key(key) + self.settings.lock().unwrap().borrow().contains_key(key) } /// Returns a list of keys that matches the given search string (can use ? and *) for search @@ -97,46 +200,60 @@ impl ConfigStore { } /// Returns the setting with the given key. If the setting is not found in the current - /// store, it will load the key from the storage. If the key is still not found, it will + /// storage, it will load the key from the storage. If the key is still not found, it will /// return the default value for the given key. Note that if the key is not found and no /// default value is specified, this function will panic. - pub fn get(&mut self, key: &str) -> Setting { + pub fn get(&self, key: &str) -> Option { if !self.has(key) { - panic!("Setting {} not found", key); + return None; } - if let Some(setting) = self.settings.get(key) { - return setting.clone(); + if let Some(setting) = self.settings.lock().unwrap().borrow().get(key) { + return Some(setting.clone()); } // Setting not found, try and load it from the storage adapter - if let Some(setting) = self.storage.get_setting(key) { - self.settings.insert(key.to_string(), setting.clone()); - return setting.clone(); + if let Some(setting) = self.storage.get(key) { + self.settings + .lock() + .unwrap() + .borrow_mut() + .insert(key.to_string(), setting.clone()); + return Some(setting.clone()); } // Return the default value for the setting when nothing is found let info = self.settings_info.get(key).unwrap(); - info.default.clone() + Some(info.default.clone()) } /// Sets the given setting to the given value. Will persist the setting to the /// storage. - pub fn set(&mut self, key: &str, value: Setting) { + pub fn set(&self, key: &str, value: Setting) { if !self.has(key) { - panic!("key not found"); + warn!("config: Setting {} is not known", key); + return; } + let info = self.settings_info.get(key).unwrap(); if mem::discriminant(&info.default) != mem::discriminant(&value) { - panic!("value is of different type than setting expects") + warn!( + "config: Setting {} is of different type than setting expects", + key + ); + return; } - self.settings.insert(key.to_string(), value.clone()); - self.storage.set_setting(key, value); + self.settings + .lock() + .unwrap() + .borrow_mut() + .insert(key.to_string(), value.clone()); + self.storage.set(key, value); } - /// Populates the settings in the store from the settings.json file - fn populate_settings(&mut self) -> Result<()> { + /// Populates the settings in the storage from the settings.json file + fn populate_default_settings(&mut self) -> crate::types::Result<()> { let json_data: Value = serde_json::from_str(SETTINGS_JSON).expect("Failed to parse settings.json"); @@ -158,7 +275,11 @@ impl ConfigStore { self.setting_keys.push(key.clone()); self.settings_info.insert(key.clone(), info.clone()); - self.settings.insert(key.clone(), info.default.clone()); + self.settings + .lock() + .unwrap() + .borrow_mut() + .insert(key.clone(), info.default.clone()); } } } @@ -170,28 +291,53 @@ impl ConfigStore { #[cfg(test)] mod test { use super::*; - use crate::config::storage::memory_storage::MemoryStorageAdapter; + use storage::memory::MemoryStorageAdapter; #[test] fn config_store() { - let mut store = - ConfigStore::from_storage(Box::new(MemoryStorageAdapter::new()), true).unwrap(); - let setting = store.get("dns.local_resolver.enabled"); + config_store_write().set_storage(Box::new(MemoryStorageAdapter::new())); + + let setting = crate::config::config_store() + .get("dns.local_resolver.enabled") + .unwrap(); assert_eq!(setting, Setting::Bool(true)); - store.set("dns.local_resolver.enabled", Setting::Bool(false)); - let setting = store.get("dns.local_resolver.enabled"); + config_store_write().set("dns.local_resolver.enabled", Setting::Bool(false)); + let setting = crate::config::config_store() + .get("dns.local_resolver.enabled") + .unwrap(); assert_eq!(setting, Setting::Bool(false)); } #[test] - #[should_panic] fn invalid_setting() { - let mut store = - ConfigStore::from_storage(Box::new(MemoryStorageAdapter::new()), true).unwrap(); - store.set( + testing_logger::setup(); + + testing_logger::validate(|captured_logs| { + assert_eq!(captured_logs.len(), 0); + }); + + config_store_write().set( "dns.local_resolver.enabled", Setting::String("wont accept strings".into()), ); + + testing_logger::validate(|captured_logs| { + assert_eq!(captured_logs.len(), 1); + assert_eq!(captured_logs[0].level, log::Level::Warn); + }); + } + + #[test] + fn macro_usage() { + config_store_write().set_storage(Box::new(MemoryStorageAdapter::new())); + + config_set!(uint "dns.cache.max_entries", 9432); + let max_entries = config!(uint "dns.cache.max_entries"); + assert_eq!(max_entries, 9432); + + config_set!(string "this.key.doesnt.exist", "yesitdoes".into()); + let s = config!(string "this.key.doesnt.exist"); + assert_eq!(s, ""); } } diff --git a/src/config/settings.rs b/src/config/settings.rs index df5526363..7534b29f5 100644 --- a/src/config/settings.rs +++ b/src/config/settings.rs @@ -1,5 +1,6 @@ use crate::types::{Error, Result}; use core::fmt::Display; +use log::warn; use serde::{Deserialize, Deserializer, Serialize, Serializer}; use std::str::FromStr; @@ -14,6 +15,93 @@ pub enum Setting { Map(Vec), } +impl Setting { + pub fn to_bool(&self) -> bool { + if !matches!(self, Setting::Bool(_)) { + warn!("setting is not a boolean"); + } + + match self { + Setting::Bool(value) => *value, + Setting::SInt(value) => *value != 0, + Setting::UInt(value) => *value != 0, + Setting::String(value) => is_bool_value(value), + Setting::Map(values) => !values.is_empty(), + } + } + + pub fn to_sint(&self) -> isize { + if !matches!(self, Setting::SInt(_)) { + warn!("setting is not an signed integer"); + } + + match self { + Setting::SInt(value) => *value, + Setting::UInt(value) => *value as isize, + Setting::Bool(value) => *value as isize, + Setting::String(value) => is_bool_value(value) as isize, + Setting::Map(values) => values.len() as isize, + } + } + + pub fn to_uint(&self) -> usize { + if !matches!(self, Setting::UInt(_)) { + warn!("setting is not an unsigned integer"); + } + + match self { + Setting::UInt(value) => *value, + Setting::SInt(value) => *value as usize, + Setting::Bool(value) => *value as usize, + Setting::String(value) => is_bool_value(value) as usize, + Setting::Map(values) => values.len(), + } + } + + #[allow(clippy::inherent_to_string_shadow_display)] + pub fn to_string(&self) -> String { + if !matches!(self, Setting::String(_)) { + warn!("setting is not a string"); + } + + match self { + Setting::SInt(value) => value.to_string(), + Setting::UInt(value) => value.to_string(), + Setting::String(value) => value.clone(), + Setting::Bool(value) => value.to_string(), + Setting::Map(values) => { + let mut result = String::new(); + for value in values { + result.push_str(value); + result.push(','); + } + result.pop(); + result + } + } + } + + pub fn to_map(&self) -> Vec { + if !matches!(self, Setting::Map(_)) { + warn!("setting is not a map"); + } + + match self { + Setting::Map(values) => values.clone(), + _ => Vec::new(), + } + } +} + +fn is_bool_value(s: &str) -> bool { + let us = s.to_uppercase(); + if ["YES", "ON", "TRUE", "1"].contains(&us.as_str()) { + return true; + } + + false +} + impl Serialize for Setting { fn serialize(&self, serializer: S) -> std::result::Result where @@ -122,21 +210,46 @@ mod test { fn setting() { let s = Setting::from_str("b:true").unwrap(); assert_eq!(s, Setting::Bool(true)); + assert!(s.to_bool()); + assert_eq!(1, s.to_sint()); + assert_eq!(1, s.to_uint()); + assert_eq!("true", s.to_string()); + assert_eq!(Vec::::new(), s.to_map()); let s = Setting::from_str("i:-1").unwrap(); assert_eq!(s, Setting::SInt(-1)); + assert!(s.to_bool()); + assert_eq!(-1, s.to_sint()); + assert_eq!(18446744073709551615, s.to_uint()); + assert_eq!("-1", s.to_string()); + assert_eq!(Vec::::new(), s.to_map()); let s = Setting::from_str("i:1").unwrap(); assert_eq!(s, Setting::SInt(1)); + assert!(s.to_bool()); + assert_eq!(1, s.to_sint()); + assert_eq!(1, s.to_uint()); + assert_eq!("1", s.to_string()); + assert_eq!(Vec::::new(), s.to_map()); let s = Setting::from_str("s:hello world").unwrap(); assert_eq!(s, Setting::String("hello world".into())); + assert!(!s.to_bool()); + assert_eq!(0, s.to_sint()); + assert_eq!(0, s.to_uint()); + assert_eq!("hello world", s.to_string()); + assert_eq!(Vec::::new(), s.to_map()); let s = Setting::from_str("m:foo,bar,baz").unwrap(); assert_eq!( s, Setting::Map(vec!["foo".into(), "bar".into(), "baz".into()]) ); + assert!(s.to_bool()); + assert_eq!(3, s.to_sint()); + assert_eq!(3, s.to_uint()); + assert_eq!("foo,bar,baz", s.to_string()); + assert_eq!(vec!["foo", "bar", "baz"], s.to_map()); let s = Setting::from_str("notexist:true"); assert!(matches!(s, Err(Error::Config(_)))); @@ -149,5 +262,40 @@ mod test { let s = Setting::from_str("u:-1"); assert!(matches!(s, Err(Error::Config(_)))); + + let s = Setting::from_str("s:true").unwrap(); + assert!(s.to_bool()); + assert_eq!(1, s.to_sint()); + assert_eq!(1, s.to_uint()); + + let s = Setting::from_str("s:false").unwrap(); + assert!(!s.to_bool()); + assert_eq!(0, s.to_sint()); + assert_eq!(0, s.to_uint()); + + let s = Setting::from_str("s:1").unwrap(); + assert!(s.to_bool()); + assert_eq!(1, s.to_sint()); + assert_eq!(1, s.to_uint()); + + let s = Setting::from_str("s:0").unwrap(); + assert!(!s.to_bool()); + assert_eq!(0, s.to_sint()); + assert_eq!(0, s.to_uint()); + + let s = Setting::from_str("s:on").unwrap(); + assert!(s.to_bool()); + assert_eq!(1, s.to_sint()); + assert_eq!(1, s.to_uint()); + + let s = Setting::from_str("s:yes").unwrap(); + assert!(s.to_bool()); + assert_eq!(1, s.to_sint()); + assert_eq!(1, s.to_uint()); + + let s = Setting::from_str("s:off").unwrap(); + assert!(!s.to_bool()); + assert_eq!(0, s.to_sint()); + assert_eq!(0, s.to_uint()); } } diff --git a/src/config/storage.rs b/src/config/storage.rs index 7af2025e9..65d6c4a33 100644 --- a/src/config/storage.rs +++ b/src/config/storage.rs @@ -1,3 +1,3 @@ -pub mod json_storage; -pub mod memory_storage; -pub mod sqlite_storage; +pub mod json; +pub mod memory; +pub mod sqlite; diff --git a/src/config/storage/json_storage.rs b/src/config/storage/json.rs similarity index 65% rename from src/config/storage/json_storage.rs rename to src/config/storage/json.rs index 8a55e5d7a..76884d6fd 100644 --- a/src/config/storage/json_storage.rs +++ b/src/config/storage/json.rs @@ -1,5 +1,5 @@ use crate::config::settings::Setting; -use crate::config::Store; +use crate::config::StorageAdapter; use crate::types::{Error, Result}; use log::warn; use serde_json::Value; @@ -7,19 +7,18 @@ use std::collections::HashMap; use std::fs; use std::fs::File; use std::io::{Read, Seek, Write}; -use std::sync::{Arc, Mutex}; +use std::sync::Mutex; pub struct JsonStorageAdapter { path: String, - file_mutex: Arc>, - elements: HashMap, + elements: Mutex>, } impl TryFrom<&String> for JsonStorageAdapter { type Error = Error; fn try_from(path: &String) -> Result { - let file = match fs::metadata(path) { + let _ = match fs::metadata(path) { Ok(metadata) => { if !metadata.is_file() { panic!("json file is not a regular file") @@ -43,8 +42,7 @@ impl TryFrom<&String> for JsonStorageAdapter { let mut adapter = JsonStorageAdapter { path: path.to_string(), - file_mutex: Arc::new(Mutex::new(file)), - elements: HashMap::new(), + elements: Mutex::new(HashMap::new()), }; adapter.read_file(); @@ -53,26 +51,31 @@ impl TryFrom<&String> for JsonStorageAdapter { } } -impl Store for JsonStorageAdapter { - fn get_setting(&self, key: &str) -> Option { - self.elements.get(key).cloned() +impl StorageAdapter for JsonStorageAdapter { + fn get(&self, key: &str) -> Option { + let lock = self.elements.lock().unwrap(); + lock.get(key).cloned() } - fn set_setting(&mut self, key: &str, value: Setting) { - self.elements.insert(key.to_string(), value); + fn set(&self, key: &str, value: Setting) { + let mut lock = self.elements.lock().unwrap(); + lock.insert(key.to_string(), value); - self.write_file() + // self.write_file() } - fn get_all_settings(&self) -> Result> { - Ok(self.elements.clone()) + fn all(&self) -> Result> { + let lock = self.elements.lock().unwrap(); + + Ok(lock.clone()) } } impl JsonStorageAdapter { /// Read whole json file and stores the data into self.elements fn read_file(&mut self) { - let mut file = self.file_mutex.lock().expect("Mutex lock failed"); + // @TODO: We should have some kind of OS file lock here + let mut file = File::open(&self.path).expect("failed to open json file"); let mut buf = String::new(); _ = file.read_to_string(&mut buf); @@ -80,11 +83,13 @@ impl JsonStorageAdapter { let parsed_json: Value = serde_json::from_str(&buf).expect("Failed to parse json"); if let Value::Object(settings) = parsed_json { - self.elements = HashMap::new(); + self.elements = Mutex::new(HashMap::new()); + + let mut lock = self.elements.lock().unwrap(); for (key, value) in settings.iter() { match serde_json::from_value(value.clone()) { Ok(setting) => { - self.elements.insert(key.clone(), setting); + lock.insert(key.clone(), setting); } Err(err) => { warn!("problem reading setting from json: {err}"); @@ -97,7 +102,9 @@ impl JsonStorageAdapter { /// Write the self.elements hashmap back to the file by truncating the file and writing the /// data again. fn write_file(&mut self) { - let mut file = self.file_mutex.lock().expect("Mutex lock failed"); + // @TODO: We need some kind of OS lock file here. We should protect against concurrent threads but also + // against concurrent processes. + let mut file = File::open(&self.path).expect("failed to open json file"); let json = serde_json::to_string_pretty(&self.elements).expect("failed to serialize"); diff --git a/src/config/storage/memory.rs b/src/config/storage/memory.rs new file mode 100644 index 000000000..4f86ad92c --- /dev/null +++ b/src/config/storage/memory.rs @@ -0,0 +1,36 @@ +use crate::config::settings::Setting; +use crate::config::StorageAdapter; +use crate::types::Result; +use std::collections::HashMap; +use std::sync::Mutex; + +#[derive(Default)] +pub struct MemoryStorageAdapter { + settings: Mutex>, +} + +impl MemoryStorageAdapter { + pub fn new() -> Self { + MemoryStorageAdapter { + settings: Mutex::new(HashMap::new()), + } + } +} + +impl StorageAdapter for MemoryStorageAdapter { + fn get(&self, key: &str) -> Option { + let lock = self.settings.lock().unwrap(); + let v = lock.get(key); + v.cloned() + } + + fn set(&self, key: &str, value: Setting) { + let mut lock = self.settings.lock().unwrap(); + lock.insert(key.to_string(), value); + } + + fn all(&self) -> Result> { + let lock = self.settings.lock().unwrap(); + Ok(lock.clone()) + } +} diff --git a/src/config/storage/memory_storage.rs b/src/config/storage/memory_storage.rs deleted file mode 100644 index b40d9df3b..000000000 --- a/src/config/storage/memory_storage.rs +++ /dev/null @@ -1,32 +0,0 @@ -use crate::config::settings::Setting; -use crate::config::Store; -use crate::types::Result; -use std::collections::HashMap; - -#[derive(Default)] -pub struct MemoryStorageAdapter { - store: HashMap, -} - -impl MemoryStorageAdapter { - pub fn new() -> Self { - MemoryStorageAdapter { - store: HashMap::new(), - } - } -} - -impl Store for MemoryStorageAdapter { - fn get_setting(&self, key: &str) -> Option { - let v = self.store.get(key); - v.cloned() - } - - fn set_setting(&mut self, key: &str, value: Setting) { - self.store.insert(key.to_string(), value); - } - - fn get_all_settings(&self) -> Result> { - Ok(self.store.clone()) - } -} diff --git a/src/config/storage/sqlite_storage.rs b/src/config/storage/sqlite.rs similarity index 68% rename from src/config/storage/sqlite_storage.rs rename to src/config/storage/sqlite.rs index 046ca175a..b96ce53af 100644 --- a/src/config/storage/sqlite_storage.rs +++ b/src/config/storage/sqlite.rs @@ -1,12 +1,13 @@ use crate::config::settings::Setting; -use crate::config::Store; +use crate::config::StorageAdapter; use crate::types::{Error, Result}; use log::warn; use std::collections::HashMap; use std::str::FromStr; +use std::sync::Mutex; pub struct SqliteStorageAdapter { - connection: sqlite::Connection, + connection: Mutex, } impl TryFrom<&String> for SqliteStorageAdapter { @@ -22,14 +23,18 @@ impl TryFrom<&String> for SqliteStorageAdapter { )"; conn.execute(query)?; - Ok(SqliteStorageAdapter { connection: conn }) + Ok(SqliteStorageAdapter { + connection: Mutex::new(conn), + }) } } -impl Store for SqliteStorageAdapter { - fn get_setting(&self, key: &str) -> Option { +impl StorageAdapter for SqliteStorageAdapter { + fn get(&self, key: &str) -> Option { + let db_lock = self.connection.lock().unwrap(); + let query = "SELECT * FROM settings WHERE key = :key"; - let mut statement = self.connection.prepare(query).unwrap(); + let mut statement = db_lock.prepare(query).unwrap(); statement.bind((":key", key)).unwrap(); match Setting::from_str(key) { @@ -41,9 +46,11 @@ impl Store for SqliteStorageAdapter { } } - fn set_setting(&mut self, key: &str, value: Setting) { + fn set(&self, key: &str, value: Setting) { + let db_lock = self.connection.lock().unwrap(); + let query = "INSERT OR REPLACE INTO settings (key, value) VALUES (:key, :value)"; - let mut statement = self.connection.prepare(query).unwrap(); + let mut statement = db_lock.prepare(query).unwrap(); statement.bind((":key", key)).unwrap(); statement .bind((":value", value.to_string().as_str())) @@ -52,9 +59,11 @@ impl Store for SqliteStorageAdapter { statement.next().unwrap(); } - fn get_all_settings(&self) -> Result> { + fn all(&self) -> Result> { + let db_lock = self.connection.lock().unwrap(); + let query = "SELECT * FROM settings"; - let mut statement = self.connection.prepare(query).unwrap(); + let mut statement = db_lock.prepare(query).unwrap(); let mut settings = HashMap::new(); while let sqlite::State::Row = statement.next().unwrap() { diff --git a/src/dns.rs b/src/dns.rs index e17ff0393..f2cfc97ad 100644 --- a/src/dns.rs +++ b/src/dns.rs @@ -163,7 +163,11 @@ mod test { #[test] fn resolver() { - SimpleLogger::new().init().unwrap(); + // Add simple logger, if not possible, that's fine too + match SimpleLogger::new().init() { + Ok(_) => {} + Err(_) => {} + } let mut dns = Dns::new(); From 66335814d4837a7c87394cf207122742076df896 Mon Sep 17 00:00:00 2001 From: Joshua Thijssen Date: Wed, 22 Nov 2023 21:27:48 +0100 Subject: [PATCH 2/3] added suggestions --- src/bin/config-store.rs | 3 +-- src/config.rs | 33 +++++++++++++++++++-------------- src/config/settings.rs | 4 ++-- src/config/storage.rs | 10 +++++++--- src/dns.rs | 5 +---- 5 files changed, 30 insertions(+), 25 deletions(-) diff --git a/src/bin/config-store.rs b/src/bin/config-store.rs index 242d43092..1e00f06cf 100644 --- a/src/bin/config-store.rs +++ b/src/bin/config-store.rs @@ -3,8 +3,7 @@ use clap::{Parser, Subcommand}; use derive_more::Display; use gosub_engine::config; use gosub_engine::config::settings::Setting; -use gosub_engine::config::storage::json::JsonStorageAdapter; -use gosub_engine::config::storage::sqlite::SqliteStorageAdapter; +use gosub_engine::config::storage::*; use gosub_engine::config::StorageAdapter; use std::str::FromStr; diff --git a/src/config.rs b/src/config.rs index af3fbaf14..c4a442b19 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2,7 +2,7 @@ pub mod settings; pub mod storage; use crate::config::settings::{Setting, SettingInfo}; -use crate::config::storage::memory::MemoryStorageAdapter; +use crate::config::storage::MemoryStorageAdapter; use lazy_static::lazy_static; use log::warn; use serde_derive::Deserialize; @@ -204,10 +204,6 @@ impl ConfigStore { /// return the default value for the given key. Note that if the key is not found and no /// default value is specified, this function will panic. pub fn get(&self, key: &str) -> Option { - if !self.has(key) { - return None; - } - if let Some(setting) = self.settings.lock().unwrap().borrow().get(key) { return Some(setting.clone()); } @@ -223,19 +219,27 @@ impl ConfigStore { } // Return the default value for the setting when nothing is found - let info = self.settings_info.get(key).unwrap(); - Some(info.default.clone()) + if let Some(info) = self.settings_info.get(key) { + return Some(info.default.clone()); + } + + // At this point we haven't found the key in the store, we haven't found it in storage, and we + // don't have a default value. This is a programming error, so we panic. + panic!("config: Setting {} is not known", key); } /// Sets the given setting to the given value. Will persist the setting to the - /// storage. + /// storage. Note that the setting MUST have a settings-info entry, otherwise + /// this function will not store the setting. pub fn set(&self, key: &str, value: Setting) { - if !self.has(key) { - warn!("config: Setting {} is not known", key); - return; - } + let info = match self.settings_info.get(key) { + Some(info) => info, + None => { + warn!("config: Setting {} is not known", key); + return; + } + }; - let info = self.settings_info.get(key).unwrap(); if mem::discriminant(&info.default) != mem::discriminant(&value) { warn!( "config: Setting {} is of different type than setting expects", @@ -249,6 +253,7 @@ impl ConfigStore { .unwrap() .borrow_mut() .insert(key.to_string(), value.clone()); + self.storage.set(key, value); } @@ -291,7 +296,7 @@ impl ConfigStore { #[cfg(test)] mod test { use super::*; - use storage::memory::MemoryStorageAdapter; + use storage::MemoryStorageAdapter; #[test] fn config_store() { diff --git a/src/config/settings.rs b/src/config/settings.rs index 7534b29f5..9b35853e2 100644 --- a/src/config/settings.rs +++ b/src/config/settings.rs @@ -88,7 +88,7 @@ impl Setting { match self { Setting::Map(values) => values.clone(), - _ => Vec::new(), + other => vec![other.to_string()], } } } @@ -155,7 +155,7 @@ impl FromStr for Setting { /// Converts a string to a setting or None when the string is invalid fn from_str(key: &str) -> Result { - let (key_type, key_value) = key.split_once(':').unwrap(); + let (key_type, key_value) = key.split_once(':').expect(""); let setting = match key_type { "b" => Setting::Bool( diff --git a/src/config/storage.rs b/src/config/storage.rs index 65d6c4a33..2b5d008be 100644 --- a/src/config/storage.rs +++ b/src/config/storage.rs @@ -1,3 +1,7 @@ -pub mod json; -pub mod memory; -pub mod sqlite; +mod json; +mod memory; +mod sqlite; + +pub use json::*; +pub use memory::*; +pub use sqlite::*; diff --git a/src/dns.rs b/src/dns.rs index f2cfc97ad..8c721f9b4 100644 --- a/src/dns.rs +++ b/src/dns.rs @@ -164,10 +164,7 @@ mod test { #[test] fn resolver() { // Add simple logger, if not possible, that's fine too - match SimpleLogger::new().init() { - Ok(_) => {} - Err(_) => {} - } + let _ = SimpleLogger::new().init(); let mut dns = Dns::new(); From eb3cfc85ffda0b98c854d0f9407d1ed7a0f2f814 Mon Sep 17 00:00:00 2001 From: Joshua Thijssen Date: Thu, 23 Nov 2023 15:42:58 +0100 Subject: [PATCH 3/3] fixed last issues --- src/config.rs | 4 ++++ src/config/settings.rs | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/config.rs b/src/config.rs index c4a442b19..eb796a2ee 100644 --- a/src/config.rs +++ b/src/config.rs @@ -340,7 +340,11 @@ mod test { config_set!(uint "dns.cache.max_entries", 9432); let max_entries = config!(uint "dns.cache.max_entries"); assert_eq!(max_entries, 9432); + } + #[test] + #[should_panic] + fn macro_usage_with_panic() { config_set!(string "this.key.doesnt.exist", "yesitdoes".into()); let s = config!(string "this.key.doesnt.exist"); assert_eq!(s, ""); diff --git a/src/config/settings.rs b/src/config/settings.rs index 9b35853e2..800eeb896 100644 --- a/src/config/settings.rs +++ b/src/config/settings.rs @@ -214,7 +214,7 @@ mod test { assert_eq!(1, s.to_sint()); assert_eq!(1, s.to_uint()); assert_eq!("true", s.to_string()); - assert_eq!(Vec::::new(), s.to_map()); + assert_eq!(vec!("true"), s.to_map()); let s = Setting::from_str("i:-1").unwrap(); assert_eq!(s, Setting::SInt(-1)); @@ -222,7 +222,7 @@ mod test { assert_eq!(-1, s.to_sint()); assert_eq!(18446744073709551615, s.to_uint()); assert_eq!("-1", s.to_string()); - assert_eq!(Vec::::new(), s.to_map()); + assert_eq!(vec!("-1"), s.to_map()); let s = Setting::from_str("i:1").unwrap(); assert_eq!(s, Setting::SInt(1)); @@ -230,7 +230,7 @@ mod test { assert_eq!(1, s.to_sint()); assert_eq!(1, s.to_uint()); assert_eq!("1", s.to_string()); - assert_eq!(Vec::::new(), s.to_map()); + assert_eq!(vec!("1"), s.to_map()); let s = Setting::from_str("s:hello world").unwrap(); assert_eq!(s, Setting::String("hello world".into())); @@ -238,7 +238,7 @@ mod test { assert_eq!(0, s.to_sint()); assert_eq!(0, s.to_uint()); assert_eq!("hello world", s.to_string()); - assert_eq!(Vec::::new(), s.to_map()); + assert_eq!(vec!("hello world"), s.to_map()); let s = Setting::from_str("m:foo,bar,baz").unwrap(); assert_eq!(