Added initial config store #249

jaytaph · 2023-11-10T12:18:27Z

Initial attempt on setting up a general config store.

It can serve from multiple backends (jsonfile, memory, sqlite).

jaytaph · 2023-11-10T12:18:59Z

src/config.rs

+    description: String,
+}
+
+trait StorageAdapter {


Any adapter that implements this can be used as a storage engine for the configuration store.

jaytaph · 2023-11-10T12:19:26Z

src/config.rs

+/// Configuration store 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<String, Setting>,


Settings are only the setting values.. they are not stored in SettingsInfo, which is only the template data for the configuration

jaytaph · 2023-11-10T12:19:49Z

src/config.rs

+    /// A hashmap of all setting descriptions
+    settings_info: HashMap<String, SettingInfo>,
+    /// Keys of all settings so we can iterate keys easily
+    setting_keys: Vec<String>,


I'm not sure if we need this, but if we need to iterate all keys, we need to store them in a vec as well.

jaytaph · 2023-11-10T12:20:32Z

src/config.rs

+
+        // preload the settings if requested
+        if preload {
+            let all_settings = store.storage.get_all_settings();


We might want to preload all settings from a storage engine, or lazy load them when needed. It depends a bit on the latency of the storage engine (ie: remote configuration would take a long time)

jaytaph · 2023-11-10T12:21:00Z

src/config.rs

+    }
+
+    /// Returns a list of keys that mathces the given search string (can use *)
+    fn find_setting_keys(&self, search: &str) -> Vec<String> {


This allows you to use "dns.resolv*" and it will match every key that starts with this. Easy for filtering

jaytaph · 2023-11-10T12:21:37Z

src/config.rs

+
+        // Panic if we can't find the setting, and we don't have a default
+        if default.is_none() {
+            panic!("Setting {} not found", key);


What should we do if we cannot find the config, and there is no default given?

Should we even use a default, as we have defaults in the settingsinfo per config setting anyway

Changed this into returning the default value as defined in the setting info for that key. This means we do not pass a default value when getting the key.

jaytaph · 2023-11-10T12:22:58Z

src/config/settings.rs

+    //   m:foo,bar,baz
+
+    /// Converts a string to a setting or None when the string is invalid
+    pub(crate) fn from_string(p0: &str) -> Option<Setting> {


A setting can have different types. bools, ints, strings etc. This tries to accomodate that

jaytaph · 2023-11-10T19:34:49Z

src/config/settings.json

@@ -0,0 +1,46 @@
+{


This config file is imported into the engine lib binary, but it allows for a more flexible setup than incode configuration.

jaytaph · 2023-11-10T19:36:13Z

src/config.rs

+        default.unwrap()
+    }
+
+    pub fn set(&mut self, key: &str, value: Setting) {


We do not do a check to see if the setting type is actually allowed.

For instance, you can set a bool (b:true) on a uint typed setting.

this has been fixed as well.. it will check the type you add against the type of the default setting.

jaytaph · 2023-11-11T12:51:00Z

src/config/settings.rs

+    Map(Vec<String>),
+}
+
+impl Serialize for Setting {


serialize / deserialize are needed for using serde_json in the json-adapter. It will convert a Setting directory to a string

jaytaph · 2023-11-11T12:52:28Z

src/config/storage/json_storage.rs

+
+impl StorageAdapter for JsonStorageAdapter {
+    fn get_setting(&self, key: &str) -> Option<Setting> {
+        match self.elements.get(key) {


basically the json adapter reads the whole json during init, and not during every get_setting call.. This would create a secondary layer of caching and doesn't reflect any changes that are done on disk. We might want to reread the whole json on each json file, OR do something smarter like checking access/modify time

jaytaph · 2023-11-11T12:52:50Z

src/config/storage/json_storage.rs

+
+        let json = serde_json::to_string_pretty(&self.elements).expect("failed to serialize");
+
+        file.set_len(0).expect("failed to truncate file");


we truncate the current file, since this is the file that is guarded by the mutex

jaytaph · 2023-11-11T12:53:38Z

src/config/storage/json_storage.rs

+
+        let mut adapter = JsonStorageAdapter {
+            path: path.to_string(),
+            file_mutex: Arc::new(Mutex::new(file)),


A mutex is used so we can easily use this is a multi-threaded environment. But we should lock at get/set_setting, not in the read/write functions.

emwalker

LGTM with suggestions.

src/config.rs

emwalker · 2023-11-11T17:40:08Z

src/config.rs

+
+impl ConfigStore {
+    /// Creates a new store with the given storage adapter and preloads the store if needed
+    pub fn new(storage: Box<dyn StorageAdapter>, preload: bool) -> Self {


I suspect we can avoid using Box<dyn ...> if we replace it with a generic struct parameter. But in this specific case I doubt it would make a difference, since this is not (presumably) critical path code.

emwalker · 2023-11-11T17:42:53Z

src/bin/config-store.rs

+            }
+        }
+    }
+}


I would add this line to the Makefile for this command:

diff --git a/Makefile b/Makefile index e25ba44..7a362d3 100644 --- a/Makefile +++ b/Makefile @@ -36,6 +36,7 @@ test_fmt: test_commands: cargo run --bin html5-parser-test >/dev/null cargo run --bin parser-test >/dev/null + cargo run --bin config-store list >/dev/null help: ## Display available commands echo "Available make commands:"

I would also add settings.db to .gitignore or put it under a tmp/ directory that is ignored:

diff --git a/.gitignore b/.gitignore index 5ce6f87..0a3c97f 100644 --- a/.gitignore +++ b/.gitignore @@ -5,4 +5,5 @@ target/ *.swp* # Local testing -local/ \ No newline at end of file +local/ +settings.db

src/bin/config-store.rs

src/config.rs

src/config/settings.rs

Co-authored-by: Eric Walker <[email protected]>

This reverts commit b58cdd0.

emwalker · 2023-11-13T13:06:52Z

src/bin/config-store.rs

@@ -101,11 +101,11 @@ fn main() {
            }
        }
        Commands::Set { key, value } => {
-            store.set(&key, Setting::from_str(&value).expect("incorrect value"));
+            store.set(&key, Setting::from_string(&value).expect("incorrect value"));


Curious why this was reverted. The name from_str is common in Rust, while the name from_string looks like someone didn't know about from_str.

@emwalker this is due to an warning that is emitted by rust itself... it clashes with a standard from_str that is found in a ::std trait and could cause conflicts

Ah, gotcha. Perhaps that means we should be implementing that trait instead of defining from_string. It would not be hard.

impl FromStr for Setting { fn from_str(value: &str) -> Self { todo!() } }

Added initial config store

f3205af

jaytaph requested review from emwalker, Kiyoshika, neuodev and a team November 10, 2023 12:18

jaytaph commented Nov 10, 2023

View reviewed changes

jaytaph added 2 commits November 10, 2023 18:12

Added config-store binary and some adjustments on the adapters

bdcc6ca

clippy / fmt

66b0164

jaytaph force-pushed the config branch from e6b7c34 to 66b0164 Compare November 10, 2023 19:33

jaytaph commented Nov 10, 2023

View reviewed changes

Making json adapapter work

312f270

jaytaph marked this pull request as ready for review November 11, 2023 12:50

jaytaph commented Nov 11, 2023

View reviewed changes

clippy/fmt fix

26fd251

emwalker approved these changes Nov 11, 2023

View reviewed changes

emwalker and others added 3 commits November 11, 2023 11:41

Replace Box<dyn ...> with a struct parameter

b58cdd0

Multiple suggestions implemented

371256b

Co-authored-by: Eric Walker <[email protected]>

Revert "Replace Box<dyn ...> with a struct parameter"

1580d32

This reverts commit b58cdd0.

jaytaph force-pushed the config branch from ac863c3 to 1580d32 Compare November 13, 2023 08:40

fixed refactoring

385a6a1

Fixed fmt issues

701149a

jaytaph merged commit 9930d4b into main Nov 13, 2023
4 checks passed

jaytaph deleted the config branch November 13, 2023 08:55

emwalker reviewed Nov 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added initial config store #249

Added initial config store #249

jaytaph commented Nov 10, 2023

jaytaph Nov 10, 2023

jaytaph Nov 10, 2023

jaytaph Nov 10, 2023

jaytaph Nov 10, 2023

jaytaph Nov 10, 2023

jaytaph Nov 10, 2023

jaytaph Nov 11, 2023

jaytaph Nov 10, 2023

jaytaph Nov 10, 2023

jaytaph Nov 10, 2023

jaytaph Nov 11, 2023

jaytaph Nov 11, 2023

jaytaph Nov 11, 2023

jaytaph Nov 11, 2023

jaytaph Nov 11, 2023 •

edited

Loading

emwalker left a comment

emwalker Nov 11, 2023

emwalker Nov 11, 2023

emwalker Nov 11, 2023

emwalker Nov 13, 2023

jaytaph Nov 13, 2023

emwalker Nov 13, 2023


		let json = serde_json::to_string_pretty(&self.elements).expect("failed to serialize");

		file.set_len(0).expect("failed to truncate file");

Added initial config store #249

Added initial config store #249

Conversation

jaytaph commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaytaph Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

emwalker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaytaph Nov 11, 2023 •

edited

Loading