Global config store #267

jaytaph · 2023-11-19T21:19:32Z

The config store is now usuable as a global instance through macros (in the same spirit as you can use warn! info! etc from the logger).

You can use config!(string, "my.key") to get the given key or it will return a default value when the key is not found. There is always the possibily to do checks on the actual store:

   let exist = config::config_store().has("does.this.key.exist")

This setup makes it possible for any user agent to set their own configuration store:

fn main() {
  // Set the config through `config_store_write()` instead of `config_store()`
  config::config_store_write().set_storage(new SqliteStorage("config.db"));

  // do the rest
}

src/config.rs

src/config/settings.rs

src/config.rs

configuration store.

jaytaph · 2023-11-22T15:08:03Z

src/bin/config-store.rs

-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;


The name "store" was a bit ambiguous. They are actually storage adapters since we already have a config store.

Maybe it would make sense to reexport the StorageAdapters in gosub_engine::config::storage, so you can import them directly with

use gosub_engine::config::storage::*

Then you could also make your json memory and sqlite module private

@Sharktheone Can you guve an example on what you mean?

I mean, doing this in storage.rs

mod json; mod memory; mod sqlite; pub use json::*; pub use memory::*; pub use sqlite::*;

Then you can do this

use gosub_engine::config::storage::*

and still use JsonStorageAdapter and SqliteStorageAdapter. This makes things cleaner. Most of the time when you import one of the storage atapters, you most likely need all.

jaytaph · 2023-11-22T15:09:02Z

src/config.rs

+/// 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 {


Send + Sync are important here, otherwise we cannot share them with other threads. We must make sure the adapters are thread-aware.

jaytaph · 2023-11-22T15:09:33Z

src/config.rs

+    fn all(&self) -> crate::types::Result<HashMap<String, Setting>>;
+}
+
+lazy_static! {


This is static reference, but through lazy_static! we can actually initialize it just like regular variables.

jaytaph · 2023-11-22T15:10:12Z

src/config.rs

+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<ConfigStore> = RwLock::new(ConfigStore::default());


RwLock since we can have multiple reads (for instance, https://github.com/gosub-browser/gosub-engine/pull/267/files#diff-d55130faaf1118028faf388a306896363c166c769c0bab6bdff2b23051659575R101)

jaytaph · 2023-11-22T15:10:28Z

src/config.rs

+/// 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()


this returns a regular read locked store

jaytaph · 2023-11-22T15:11:11Z

src/config.rs

+    };
+}
+
+#[allow(clippy::crate_in_macro_def)]


clippy wants the macro to use $crate instead of crate, but i think this is correct. We want THIS crate, not the crate where the macro is invoked from.

I guess then you could use gosub_engine instead of crate

no, it won't work. It gives an unknown crate issue. I'll leave it for now

jaytaph · 2023-11-22T15:11:46Z

src/config.rs

+        crate::config::config_store().set($key, Setting::SInt($val))
+    };
+    (map $key:expr, $val:expr) => {
+        crate::config::config_store().set($key, Setting::Map($val))


we can do "set" even though we have an immutable reference. This is due to internal mutability through the RefCell<>

jaytaph · 2023-11-22T15:12:28Z

src/config.rs

+    /// 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<std::cell::RefCell<HashMap<String, Setting>>>,


A mutex to share the config store between threads
A RefCell to allow us to mutate from a immutable config store

jaytaph · 2023-11-22T15:13:11Z

src/config.rs

            for (key, value) in all_settings {
-                store.settings.insert(key, value);
+                self.settings


lock the mutex,
unwrap the mutex guard,
borrow the refcel as mutable reference
and insert the key/value into the hashmap

jaytaph · 2023-11-22T15:13:33Z

src/config.rs

        if !self.has(key) {
-            panic!("Setting {} not found", key);
+            return None;


This was a panic, but we just return None.. it's up to the caller to deal with it

I think this doesn't make sense at all. I mean we're asking self.settings if we have the key and lower we get the key from self.settings and if it doesn't has the setting we search it in self.storage, but we can't get to that point. Am I missing something? I am a bit confused ride now.

This might indeed be false. I'll check

fixed this. This flow was not correct after some refactoring here and there.. surprised it still passed the tests

jaytaph · 2023-11-22T15:14:01Z

src/config.rs

-        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) {


If the key is not found in the current settings, we load it from the storage, and save it in the settings for the next time.

jaytaph · 2023-11-22T15:14:51Z

src/config/storage/json.rs


 pub struct JsonStorageAdapter {
    path: String,
-    file_mutex: Arc<Mutex<File>>,
-    elements: HashMap<String, Setting>,
+    elements: Mutex<HashMap<String, Setting>>,


Mutex to guard against multiple threads. This needs to be fixed better when saving the json

jaytaph · 2023-11-22T15:15:20Z

src/config/storage/json.rs

-        self.elements.get(key).cloned()
+impl StorageAdapter for JsonStorageAdapter {
+    fn get(&self, key: &str) -> Option<Setting> {
+        let lock = self.elements.lock().unwrap();


We read during a lock. This is fast enough. I don't even know if we really need it here.

jaytaph · 2023-11-22T15:16:07Z

src/config/storage/sqlite.rs

@@ -22,14 +23,18 @@ impl TryFrom<&String> for SqliteStorageAdapter {
        )";
        conn.execute(query)?;

-        Ok(SqliteStorageAdapter { connection: conn })
+        Ok(SqliteStorageAdapter {
+            connection: Mutex::new(conn),


Connections from Sqlite cannot be shared between threads (the compiler wont let us). Instead, we guard it with a mutex and lock whenever we need to read/write.

Sharktheone

I would remove all the unwraps and at leased replace them with a expect but better would be to just us a Result for the function

Sharktheone · 2023-11-22T16:01:13Z

src/bin/config-store.rs

-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;


Maybe it would make sense to reexport the StorageAdapters in gosub_engine::config::storage, so you can import them directly with

use gosub_engine::config::storage::*

Then you could also make your json memory and sqlite module private

Sharktheone · 2023-11-22T16:15:09Z

src/config.rs

+/// 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()


Hmm, i don't like the unwrap here. I also don't really like them in the config-store.rs

you need to unwrap the guard before you can use it. I'm open for suggestions on better ways

Maybe return a Result which you can just use the ? on in functions that also return a Result. Replacing it with a expect is also already better

Sharktheone · 2023-11-22T16:18:07Z

src/bin/config-store.rs

You have a - in the filename, I've seen this also in the other files in the bin directory now. Everywhere we have underscores and not dashes in the file names. I guess going for consistency is probably good. I'd generally avoid dashes/spaces and so on in file names, I just use a underscore for it.

I personally like dashes in binaries and options :)

Same here — I prefer dashes to underscores in binary executable names. E.g., in Linux, the executable name is google-chrome. (For binary executables, even better is a short name with no dashes or underscores.)

Sharktheone · 2023-11-22T16:24:02Z

src/config.rs

+/// 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")


it might make sense to panic when a config value isn't found. I mean, the config keys are static anyway, and so if something isn't found, this most likely is an issue in the code. Finding out, that the issue is a typo in the name can be quite difficult, especially when the two characters look similar.

If the config key is not found, we have a default value in the json configuration. There might be a chance that we will be using user defined keys later on (for instance for plugins).

Ah, yeah... then it makes definitely sense to not do this.

this flow was a bit off. This should be fixed now:

get:

if key exists in store: return key

if key exists in storage: save key in store and return key

if key info exists: return default value from info

set:

if key info exist: set in store and set in storage (persist)

Later on, we can register keys to the configuration by registering an settings_info struct. From that point on, it should work exactly like it works with our "global" configuration keys. I'm not sure if this works the way i envision it, but it allows plugins to actually register their own keys (only in the user.* or plugin.* namespace, for instance)

Maybe we can do dynamic keys as well.. but i'm not 100% sure how that would work

Sharktheone · 2023-11-22T16:27:25Z

src/config.rs

+    };
+}
+
+#[allow(clippy::crate_in_macro_def)]


I guess then you could use gosub_engine instead of crate

Sharktheone · 2023-11-22T16:40:26Z

src/config.rs

        if !self.has(key) {
-            panic!("Setting {} not found", key);
+            return None;


I think this doesn't make sense at all. I mean we're asking self.settings if we have the key and lower we get the key from self.settings and if it doesn't has the setting we search it in self.storage, but we can't get to that point. Am I missing something? I am a bit confused ride now.

Sharktheone · 2023-11-22T16:44:41Z

src/config.rs

        if !self.has(key) {
-            panic!("key not found");
+            warn!("config: Setting {} is not known", key);
+            return;


Here it's the same, i guess the self.has method should look in self.storage and not in self.settings

Sharktheone · 2023-11-22T16:58:21Z

src/config/settings.rs

+
+        match self {
+            Setting::Map(values) => values.clone(),
+            _ => Vec::new(),


Maybe it'd make sense to not just return a empty vec, but push the contents of the values

Suggested change

_ => Vec::new(),

other => vec![other.to_string()],

yeah, makes sense I guess

Sharktheone · 2023-11-22T22:36:56Z

src/config/settings.rs

@@ -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<Setting> {
-        let (key_type, key_value) = key.split_once(':').unwrap();
+        let (key_type, key_value) = key.split_once(':').expect("");


something that says, what has failed in the expect would be nice, else it is more or less the same as unwrap

Sharktheone · 2023-11-22T22:39:39Z

src/dns.rs

-            Ok(_) => {}
-            Err(_) => {}
-        }
+        let _ = SimpleLogger::new().init();


When i think about this, I'm not sure if we want to to this here. It maybe would make sense to use the SimpleLogger instead of our own and invoke the logger init somewhere in the main fn. The other option I'd see, to upgrade our logger and use this instead. I guess it makes sense to only have one logger.

Kiyoshika · 2023-11-23T15:04:26Z

src/config/settings.rs

+            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,


For to_sint and to_uint, should the string convert to the actual integer representation or leave it as bool here?

If there was a setting s:123 stored as a string and we tried converting it to its integer representation then it would return 1 which may be unexpected.

I'm not sure what cases if/when that would ever come up, but something to consider

Of course, we shouldn't store integer settings as strings, but.. :P

Makes sense to me, to do that.

Sharktheone

LGTM. The only thing i noticed is the unwraps but i think they are okay for now. I also don't really have a better solution that is convenient. I might look into that at some point. I maybe add a todo on tasks project

Sharktheone · 2023-11-23T15:16:22Z

src/config.rs

+            warn!(
+                "config: Setting {} is of different type than setting expects",
+                key
+            );
+            return;


I guess we could try to convert the setting here to the expected type. (e.g int to string)

Sharktheone · 2023-11-23T15:17:58Z

src/config/settings.rs

+            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,


Makes sense to me, to do that.

jaytaph marked this pull request as draft November 19, 2023 21:19

jaytaph commented Nov 19, 2023

View reviewed changes