Allow registering additional settings after loading global settings

Max Brunsfeld created

Change summary

crates/settings/src/settings_store.rs | 226 +++++++++++++++++-----------
1 file changed, 133 insertions(+), 93 deletions(-)

Detailed changes

crates/settings/src/settings_store.rs 🔗

@@ -1,12 +1,10 @@
 use anyhow::{anyhow, Result};
 use collections::{hash_map, BTreeMap, HashMap, HashSet};
 use schemars::JsonSchema;
-use serde::{de::DeserializeOwned, Serialize};
-use serde_json::value::RawValue;
+use serde::{de::DeserializeOwned, Deserialize as _, Serialize};
 use smallvec::SmallVec;
 use std::{
     any::{type_name, Any, TypeId},
-    cmp::Ordering,
     fmt::Debug,
     mem,
     path::Path,
@@ -18,13 +16,13 @@ use util::{merge_non_null_json_value_into, ResultExt as _};
 ///
 /// Settings can be loaded from a combination of multiple JSON files.
 pub trait Setting: 'static + Debug {
-    /// The name of a field within the JSON file from which this setting should
+    /// The name of a key within the JSON file from which this setting should
     /// be deserialized. If this is `None`, then the setting will be deserialized
     /// from the root object.
-    const FIELD_NAME: Option<&'static str> = None;
+    const KEY: Option<&'static str> = None;
 
     /// The type that is stored in an individual JSON file.
-    type FileContent: DeserializeOwned + JsonSchema;
+    type FileContent: Serialize + DeserializeOwned + JsonSchema;
 
     /// The logic for combining together values from one or more JSON files into the
     /// final value for this setting.
@@ -52,9 +50,8 @@ pub trait Setting: 'static + Debug {
 /// A set of strongly-typed setting values defined via multiple JSON files.
 #[derive(Default)]
 pub struct SettingsStore {
-    setting_keys: Vec<(Option<&'static str>, TypeId)>,
     setting_values: HashMap<TypeId, Box<dyn AnySettingValue>>,
-    default_deserialized_settings: DeserializedSettingMap,
+    default_deserialized_settings: Option<DeserializedSettingMap>,
     user_deserialized_settings: Option<DeserializedSettingMap>,
     local_deserialized_settings: BTreeMap<Arc<Path>, DeserializedSettingMap>,
     changed_setting_types: HashSet<TypeId>,
@@ -67,8 +64,9 @@ struct SettingValue<T> {
 }
 
 trait AnySettingValue: Debug {
+    fn key(&self) -> Option<&'static str>;
     fn setting_type_name(&self) -> &'static str;
-    fn deserialize_setting(&self, json: &str) -> Result<DeserializedSetting>;
+    fn deserialize_setting(&self, json: &serde_json::Value) -> Result<DeserializedSetting>;
     fn load_setting(
         &self,
         default_value: &DeserializedSetting,
@@ -81,29 +79,40 @@ trait AnySettingValue: Debug {
 
 struct DeserializedSetting(Box<dyn Any>);
 
-type DeserializedSettingMap = HashMap<TypeId, DeserializedSetting>;
+struct DeserializedSettingMap {
+    untyped: serde_json::Value,
+    typed: HashMap<TypeId, DeserializedSetting>,
+}
 
 impl SettingsStore {
     /// Add a new type of setting to the store.
-    ///
-    /// This should be done before any settings are loaded.
     pub fn register_setting<T: Setting>(&mut self) {
-        let type_id = TypeId::of::<T>();
+        let setting_type_id = TypeId::of::<T>();
 
-        let entry = self.setting_values.entry(type_id);
+        let entry = self.setting_values.entry(setting_type_id);
         if matches!(entry, hash_map::Entry::Occupied(_)) {
             panic!("duplicate setting type: {}", type_name::<T>());
         }
-        entry.or_insert(Box::new(SettingValue::<T> {
+        let setting_value = entry.or_insert(Box::new(SettingValue::<T> {
             global_value: None,
             local_values: Vec::new(),
         }));
 
-        match self
-            .setting_keys
-            .binary_search_by_key(&T::FIELD_NAME, |e| e.0)
-        {
-            Ok(ix) | Err(ix) => self.setting_keys.insert(ix, (T::FIELD_NAME, type_id)),
+        if let Some(default_settings) = self.default_deserialized_settings.as_mut() {
+            Self::load_setting_in_map(setting_type_id, setting_value, default_settings);
+
+            let mut user_values_stack = Vec::new();
+            if let Some(user_settings) = self.user_deserialized_settings.as_mut() {
+                Self::load_setting_in_map(setting_type_id, setting_value, user_settings);
+                if let Some(user_value) = user_settings.typed.get(&setting_type_id) {
+                    user_values_stack = vec![user_value];
+                }
+            }
+            if let Some(default_deserialized_value) = default_settings.typed.get(&setting_type_id) {
+                setting_value.set_global_value(
+                    setting_value.load_setting(default_deserialized_value, &user_values_stack),
+                );
+            }
         }
     }
 
@@ -124,19 +133,18 @@ impl SettingsStore {
     ///
     /// The string should contain a JSON object with a default value for every setting.
     pub fn set_default_settings(&mut self, default_settings_content: &str) -> Result<()> {
-        self.default_deserialized_settings = self.load_setting_map(default_settings_content)?;
-        if self.default_deserialized_settings.len() != self.setting_keys.len() {
+        let deserialized_setting_map = self.load_setting_map(default_settings_content)?;
+        if deserialized_setting_map.typed.len() != self.setting_values.len() {
             return Err(anyhow!(
                 "default settings file is missing fields: {:?}",
-                self.setting_keys
+                self.setting_values
                     .iter()
-                    .filter(|(_, type_id)| !self
-                        .default_deserialized_settings
-                        .contains_key(type_id))
+                    .filter(|(type_id, _)| !deserialized_setting_map.typed.contains_key(type_id))
                     .map(|(name, _)| *name)
                     .collect::<Vec<_>>()
             ));
         }
+        self.default_deserialized_settings = Some(deserialized_setting_map);
         self.recompute_values(false, None, None);
         Ok(())
     }
@@ -175,17 +183,17 @@ impl SettingsStore {
     ) {
         // Identify all of the setting types that have changed.
         let new_settings_map = if let Some(changed_path) = changed_local_path {
-            &self.local_deserialized_settings.get(changed_path).unwrap()
+            self.local_deserialized_settings.get(changed_path)
         } else if user_settings_changed {
-            self.user_deserialized_settings.as_ref().unwrap()
+            self.user_deserialized_settings.as_ref()
         } else {
-            &self.default_deserialized_settings
+            self.default_deserialized_settings.as_ref()
         };
         self.changed_setting_types.clear();
-        self.changed_setting_types.extend(new_settings_map.keys());
-        if let Some(previous_settings_map) = old_settings_map {
-            self.changed_setting_types
-                .extend(previous_settings_map.keys());
+        for map in [old_settings_map.as_ref(), new_settings_map] {
+            if let Some(map) = map {
+                self.changed_setting_types.extend(map.typed.keys());
+            }
         }
 
         // Reload the global and local values for every changed setting.
@@ -197,18 +205,26 @@ impl SettingsStore {
             // load function.
             user_values_stack.clear();
             if let Some(user_settings) = &self.user_deserialized_settings {
-                if let Some(user_value) = user_settings.get(setting_type_id) {
+                if let Some(user_value) = user_settings.typed.get(setting_type_id) {
                     user_values_stack.push(&user_value);
                 }
             }
 
+            let default_deserialized_value = if let Some(value) = self
+                .default_deserialized_settings
+                .as_ref()
+                .and_then(|map| map.typed.get(setting_type_id))
+            {
+                value
+            } else {
+                continue;
+            };
+
             // If the global settings file changed, reload the global value for the field.
             if changed_local_path.is_none() {
-                let global_value = setting_value.load_setting(
-                    &self.default_deserialized_settings[setting_type_id],
-                    &user_values_stack,
+                setting_value.set_global_value(
+                    setting_value.load_setting(default_deserialized_value, &user_values_stack),
                 );
-                setting_value.set_global_value(global_value);
             }
 
             // Reload the local values for the setting.
@@ -223,7 +239,7 @@ impl SettingsStore {
                 }
 
                 // Ignore recomputing settings for any path that hasn't customized that setting.
-                let Some(deserialized_value) = deserialized_values.get(setting_type_id) else {
+                let Some(deserialized_value) = deserialized_values.typed.get(setting_type_id) else {
                     continue;
                 };
 
@@ -240,7 +256,7 @@ impl SettingsStore {
                     }
 
                     if let Some(preceding_deserialized_value) =
-                        preceding_deserialized_values.get(setting_type_id)
+                        preceding_deserialized_values.typed.get(setting_type_id)
                     {
                         user_values_stack.push(&*preceding_deserialized_value);
                     }
@@ -248,11 +264,10 @@ impl SettingsStore {
                 user_values_stack.push(&*deserialized_value);
 
                 // Load the local value for the field.
-                let local_value = setting_value.load_setting(
-                    &self.default_deserialized_settings[setting_type_id],
-                    &user_values_stack,
+                setting_value.set_local_value(
+                    path.clone(),
+                    setting_value.load_setting(default_deserialized_value, &user_values_stack),
                 );
-                setting_value.set_local_value(path.clone(), local_value);
             }
         }
     }
@@ -261,57 +276,42 @@ impl SettingsStore {
     ///
     /// Returns an error if the string doesn't contain a valid JSON object.
     fn load_setting_map(&self, json: &str) -> Result<DeserializedSettingMap> {
-        let mut map = DeserializedSettingMap::default();
-        let settings_content_by_key: BTreeMap<&str, &RawValue> = serde_json::from_str(json)?;
-        let mut setting_types_by_key = self.setting_keys.iter().peekable();
-
-        // Load all of the fields that don't have a key.
-        while let Some((setting_key, setting_type_id)) = setting_types_by_key.peek() {
-            if setting_key.is_some() {
-                break;
-            }
-            setting_types_by_key.next();
-            if let Some(deserialized_value) = self
-                .setting_values
-                .get(setting_type_id)
-                .unwrap()
-                .deserialize_setting(json)
-                .log_err()
-            {
-                map.insert(*setting_type_id, deserialized_value);
-            }
+        let mut map = DeserializedSettingMap {
+            typed: HashMap::default(),
+            untyped: serde_json::from_str(json)?,
+        };
+        for (setting_type_id, setting_value) in self.setting_values.iter() {
+            Self::load_setting_in_map(*setting_type_id, setting_value, &mut map);
         }
+        Ok(map)
+    }
 
-        // For each key in the file, load all of the settings that belong to that key.
-        for (key, key_content) in settings_content_by_key {
-            while let Some((setting_key, setting_type_id)) = setting_types_by_key.peek() {
-                let setting_key = setting_key.expect("setting names are ordered");
-                match setting_key.cmp(key) {
-                    Ordering::Less => {
-                        setting_types_by_key.next();
-                        continue;
-                    }
-                    Ordering::Greater => break,
-                    Ordering::Equal => {
-                        if let Some(deserialized_value) = self
-                            .setting_values
-                            .get(setting_type_id)
-                            .unwrap()
-                            .deserialize_setting(key_content.get())
-                            .log_err()
-                        {
-                            map.insert(*setting_type_id, deserialized_value);
-                        }
-                        setting_types_by_key.next();
-                    }
-                }
+    fn load_setting_in_map(
+        setting_type_id: TypeId,
+        setting_value: &Box<dyn AnySettingValue>,
+        map: &mut DeserializedSettingMap,
+    ) {
+        let value = if let Some(setting_key) = setting_value.key() {
+            if let Some(value) = map.untyped.get(setting_key) {
+                value
+            } else {
+                return;
             }
+        } else {
+            &map.untyped
+        };
+
+        if let Some(deserialized_value) = setting_value.deserialize_setting(&value).log_err() {
+            map.typed.insert(setting_type_id, deserialized_value);
         }
-        Ok(map)
     }
 }
 
 impl<T: Setting> AnySettingValue for SettingValue<T> {
+    fn key(&self) -> Option<&'static str> {
+        T::KEY
+    }
+
     fn setting_type_name(&self) -> &'static str {
         type_name::<T>()
     }
@@ -329,8 +329,8 @@ impl<T: Setting> AnySettingValue for SettingValue<T> {
         Box::new(T::load(default_value, &values))
     }
 
-    fn deserialize_setting(&self, json: &str) -> Result<DeserializedSetting> {
-        let value = serde_json::from_str::<T::FileContent>(json)?;
+    fn deserialize_setting(&self, json: &serde_json::Value) -> Result<DeserializedSetting> {
+        let value = T::FileContent::deserialize(json)?;
         Ok(DeserializedSetting(Box::new(value)))
     }
 
@@ -380,7 +380,7 @@ mod tests {
     use serde_derive::Deserialize;
 
     #[test]
-    fn test_settings_store() {
+    fn test_settings_store_basic() {
         let mut store = SettingsStore::default();
         store.register_setting::<UserSettings>();
         store.register_setting::<TurboSetting>();
@@ -517,6 +517,46 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_setting_store_load_before_register() {
+        let mut store = SettingsStore::default();
+        store
+            .set_default_settings(
+                r#"{
+                    "turbo": true,
+                    "user": {
+                        "name": "John Doe",
+                        "age": 30,
+                        "staff": false
+                    },
+                    "key1": "x
+                }"#,
+            )
+            .unwrap();
+        store.set_user_settings(r#"{ "turbo": false }"#).unwrap();
+        store.register_setting::<UserSettings>();
+        store.register_setting::<TurboSetting>();
+
+        assert_eq!(store.get::<TurboSetting>(None), &TurboSetting(false));
+        assert_eq!(
+            store.get::<UserSettings>(None),
+            &UserSettings {
+                name: "John Doe".to_string(),
+                age: 30,
+                staff: false,
+            }
+        );
+
+        store.register_setting::<MultiKeySettings>();
+        assert_eq!(
+            store.get::<MultiKeySettings>(None),
+            &MultiKeySettings {
+                key1: "x".into(),
+                key2: String::new(),
+            }
+        );
+    }
+
     #[derive(Debug, PartialEq, Deserialize)]
     struct UserSettings {
         name: String,
@@ -532,7 +572,7 @@ mod tests {
     }
 
     impl Setting for UserSettings {
-        const FIELD_NAME: Option<&'static str> = Some("user");
+        const KEY: Option<&'static str> = Some("user");
         type FileContent = UserSettingsJson;
 
         fn load(default_value: &UserSettingsJson, user_values: &[&UserSettingsJson]) -> Self {
@@ -544,7 +584,7 @@ mod tests {
     struct TurboSetting(bool);
 
     impl Setting for TurboSetting {
-        const FIELD_NAME: Option<&'static str> = Some("turbo");
+        const KEY: Option<&'static str> = Some("turbo");
         type FileContent = Option<bool>;
 
         fn load(default_value: &Option<bool>, user_values: &[&Option<bool>]) -> Self {
@@ -597,7 +637,7 @@ mod tests {
     }
 
     impl Setting for JournalSettings {
-        const FIELD_NAME: Option<&'static str> = Some("journal");
+        const KEY: Option<&'static str> = Some("journal");
 
         type FileContent = JournalSettingsJson;