settings: Introduce PRESERVED_KEYS to write default values (#15474)

This adds the optional `PRESERVED_KEYS` constant to the `Settings`
trait,
which allows users of the trait to specify which keys should be written
to
the settings file, even if their current value matches the default
value.

That's useful for tagged settings that have, for example, a `"version"`
field
that should always be present in the user settings file, so we can then
reparse
the user settings based on the version.

Co-Authored-By: Thorsten <thorsten@zed.dev>

Release Notes:

- N/A

---------

Co-authored-by: Thorsten <thorsten@zed.dev>

Cargo.lock

@@ -439,6 +439,7 @@ dependencies = [
  "semantic_index",
  "serde",
  "serde_json",
+ "serde_json_lenient",
  "settings",
  "similar",
  "smol",

crates/assistant/Cargo.toml

@@ -85,5 +85,6 @@ language = { workspace = true, features = ["test-support"] }
 log.workspace = true
 project = { workspace = true, features = ["test-support"] }
 rand.workspace = true
+serde_json_lenient.workspace = true
 text = { workspace = true, features = ["test-support"] }
 unindent.workspace = true

crates/assistant/src/assistant_settings.rs

@@ -456,6 +456,8 @@ pub struct LegacyAssistantSettingsContent {
 impl Settings for AssistantSettings {
     const KEY: Option<&'static str> = Some("assistant");
 
+    const PRESERVED_KEYS: Option<&'static [&'static str]> = Some(&["version"]);
+
     type FileContent = AssistantSettingsContent;
 
     fn load(
@@ -497,103 +499,70 @@ fn merge<T>(target: &mut T, value: Option<T>) {
     }
 }
 
-// #[cfg(test)]
+#[cfg(test)]
-// mod tests {
+mod tests {
-//     use gpui::{AppContext, UpdateGlobal};
+    use gpui::{ReadGlobal, TestAppContext};
-//     use settings::SettingsStore;
 
-//     use super::*;
+    use super::*;
 
-//     #[gpui::test]
+    #[gpui::test]
-//     fn test_deserialize_assistant_settings(cx: &mut AppContext) {
+    async fn test_deserialize_assistant_settings_with_version(cx: &mut TestAppContext) {
-//         let store = settings::SettingsStore::test(cx);
+        let fs = fs::FakeFs::new(cx.executor().clone());
-//         cx.set_global(store);
+        fs.create_dir(paths::settings_file().parent().unwrap())
+            .await
+            .unwrap();
 
-//         // Settings default to gpt-4-turbo.
+        cx.update(|cx| {
-//         AssistantSettings::register(cx);
+            let test_settings = settings::SettingsStore::test(cx);
-//         assert_eq!(
+            cx.set_global(test_settings);
-//             AssistantSettings::get_global(cx).provider,
+            AssistantSettings::register(cx);
-//             AssistantProvider::OpenAi {
+        });
-//                 model: OpenAiModel::FourOmni,
-//                 api_url: open_ai::OPEN_AI_API_URL.into(),
-//                 low_speed_timeout_in_seconds: None,
-//                 available_models: Default::default(),
-//             }
-//         );
 
-//         // Ensure backward-compatibility.
+        cx.update(|cx| {
-//         SettingsStore::update_global(cx, |store, cx| {
+            assert!(!AssistantSettings::get_global(cx).using_outdated_settings_version);
-//             store
+            assert_eq!(
-//                 .set_user_settings(
+                AssistantSettings::get_global(cx).default_model,
-//                     r#"{
+                AssistantDefaultModel {
-//                         "assistant": {
+                    provider: "openai".into(),
-//                             "openai_api_url": "test-url",
+                    model: "gpt-4o".into(),
-//                         }
+                }
-//                     }"#,
+            );
-//                     cx,
+        });
-//                 )
-//                 .unwrap();
-//         });
-//         assert_eq!(
-//             AssistantSettings::get_global(cx).provider,
-//             AssistantProvider::OpenAi {
-//                 model: OpenAiModel::FourOmni,
-//                 api_url: "test-url".into(),
-//                 low_speed_timeout_in_seconds: None,
-//                 available_models: Default::default(),
-//             }
-//         );
-//         SettingsStore::update_global(cx, |store, cx| {
-//             store
-//                 .set_user_settings(
-//                     r#"{
-//                         "assistant": {
-//                             "default_open_ai_model": "gpt-4-0613"
-//                         }
-//                     }"#,
-//                     cx,
-//                 )
-//                 .unwrap();
-//         });
-//         assert_eq!(
-//             AssistantSettings::get_global(cx).provider,
-//             AssistantProvider::OpenAi {
-//                 model: OpenAiModel::Four,
-//                 api_url: open_ai::OPEN_AI_API_URL.into(),
-//                 low_speed_timeout_in_seconds: None,
-//                 available_models: Default::default(),
-//             }
-//         );
 
-//         // The new version supports setting a custom model when using zed.dev.
+        cx.update(|cx| {
-//         SettingsStore::update_global(cx, |store, cx| {
+            settings::SettingsStore::global(cx).update_settings_file::<AssistantSettings>(
-//             store
+                fs.clone(),
-//                 .set_user_settings(
+                |settings, _| {
-//                     r#"{
+                    *settings = AssistantSettingsContent::Versioned(
-//                         "assistant": {
+                        VersionedAssistantSettingsContent::V2(AssistantSettingsContentV2 {
-//                             "version": "1",
+                            default_model: Some(AssistantDefaultModel {
-//                             "provider": {
+                                provider: "test-provider".into(),
-//                                 "name": "zed.dev",
+                                model: "gpt-99".into(),
-//                                 "default_model": {
+                            }),
-//                                     "custom": {
+                            enabled: None,
-//                                         "name": "custom-provider"
+                            button: None,
-//                                     }
+                            dock: None,
-//                                 }
+                            default_width: None,
-//                             }
+                            default_height: None,
-//                         }
+                        }),
-//                     }"#,
+                    )
-//                     cx,
+                },
-//                 )
+            );
-//                 .unwrap();
+        });
-//         });
+
-//         assert_eq!(
+        cx.run_until_parked();
-//             AssistantSettings::get_global(cx).provider,
+
-//             AssistantProvider::ZedDotDev {
+        let raw_settings_value = fs.load(paths::settings_file()).await.unwrap();
-//                 model: CloudModel::Custom {
+        assert!(raw_settings_value.contains(r#""version": "2""#));
-//                     name: "custom-provider".into(),
+
-//                     max_tokens: None
+        #[derive(Debug, Deserialize)]
-//                 }
+        struct AssistantSettingsTest {
-//             }
+            assistant: AssistantSettingsContent,
-//         );
+        }
-//     }
+
-// }
+        let assistant_settings: AssistantSettingsTest =
+            serde_json_lenient::from_str(&raw_settings_value).unwrap();
+
+        assert!(!assistant_settings.assistant.is_version_outdated());
+    }
+}

crates/settings/src/settings_store.rs

@@ -28,6 +28,14 @@ pub trait Settings: 'static + Send + Sync {
     /// from the root object.
     const KEY: Option<&'static str>;
 
+    /// The name of the keys in the [`FileContent`] that should always be written to
+    /// a settings file, even if their value matches the default value.
+    ///
+    /// This is useful for tagged [`FileContent`]s where the tag is a "version" field
+    /// that should always be persisted, even if the current user settings match the
+    /// current version of the settings.
+    const PRESERVED_KEYS: Option<&'static [&'static str]> = None;
+
     /// The type that is stored in an individual JSON file.
     type FileContent: Clone + Default + Serialize + DeserializeOwned + JsonSchema;
 
@@ -407,6 +415,8 @@ impl SettingsStore {
     ) -> Vec<(Range<usize>, String)> {
         let setting_type_id = TypeId::of::<T>();
 
+        let preserved_keys = T::PRESERVED_KEYS.unwrap_or_default();
+
         let setting = self
             .setting_values
             .get(&setting_type_id)
@@ -436,6 +446,7 @@ impl SettingsStore {
             tab_size,
             &old_value,
             &new_value,
+            preserved_keys,
             &mut edits,
         );
         edits
@@ -874,6 +885,7 @@ fn update_value_in_json_text<'a>(
     tab_size: usize,
     old_value: &'a serde_json::Value,
     new_value: &'a serde_json::Value,
+    preserved_keys: &[&str],
     edits: &mut Vec<(Range<usize>, String)>,
 ) {
     // If the old and new values are both objects, then compare them key by key,
@@ -891,6 +903,7 @@ fn update_value_in_json_text<'a>(
                 tab_size,
                 old_sub_value,
                 new_sub_value,
+                preserved_keys,
                 edits,
             );
             key_path.pop();
@@ -904,12 +917,17 @@ fn update_value_in_json_text<'a>(
                     tab_size,
                     &serde_json::Value::Null,
                     new_sub_value,
+                    preserved_keys,
                     edits,
                 );
             }
             key_path.pop();
         }
-    } else if old_value != new_value {
+    } else if key_path
+        .last()
+        .map_or(false, |key| preserved_keys.contains(&key))
+        || old_value != new_value
+    {
         let mut new_value = new_value.clone();
         if let Some(new_object) = new_value.as_object_mut() {
             new_object.retain(|_, v| !v.is_null());