fix(serde_json): value merging was incorrect when not present in a file

Author: tenuous-guidance

confik/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## Unreleased
 
+- Add a new helper `MergingUnsetBuilder` and trait `MergingWithUnset`, for aiding in building of custom `Configuration` implementations. See the docs on `MergingWithUnset` for a worked example.
+- Fix `serde_json::Value` merge when the first parsed config doesn't have any value set
+
 ## 0.15.10
 
 - Add `#[confik(struct_default)]` for struct fields: when no configuration data is present for that field, its value is taken from the same-named field on `<Self as Default>::default()`. Can be combined with `#[confik(skip)]` (in place of an explicit `#[confik(default)]`). Cannot be combined with `#[confik(default)]` on the same field, and is not supported on enum variant fields (the configuration type must remain a struct).

confik/src/helpers.rs

@@ -371,3 +371,90 @@ where
         }
     }
 }
+
+/// Implementation helper for building a type that supports merging in itself, but needs to represent unset.
+///
+/// See [`MergingWithUnset`] for more info.
+#[derive(Debug, Default, Clone)]
+pub enum MergingUnsetBuilder<T> {
+    #[default]
+    Unset,
+    Set(T),
+}
+
+impl<'de, T> Deserialize<'de> for MergingUnsetBuilder<T>
+where
+    T: Deserialize<'de>,
+{
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        T::deserialize(deserializer).map(Self::Set)
+    }
+}
+
+/// A helper trait for building [`Configuration`], with automated handling of unset merges and builds.
+///
+/// This trait duplicated the definitions of [Configuration], any set value(s) delegate to these functions, with unset values being handled by [`MergingUnsetBuilder`] itself.
+///
+/// For an example `AdditiveMerge` type, you would otherwise need to have a custom builder with `Option<usize>` inside, and then handle the `Option` yourself. This trait allows that to be delegated to [`MergingUnsetBuilder`].
+/// ```
+/// #[derive(Debug, Deserialize)]
+/// struct AdditiveMerge(usize);
+///
+/// impl Configuration for AdditiveMerge {
+///     type Builder = MergingUnsetBuilder<Self>;
+/// }
+///
+/// impl MergingWithUnset for AdditiveMerge {
+///     type Target = Self;
+///
+///     fn merge(self, other: Self) -> Self {
+///         self.0 + other.0
+///     }
+///
+///     fn try_build(self) -> Result<Self::Target, Error> {
+///         Ok(self)
+///     }
+///
+///     fn contains_non_secret_data(&self) -> Result<bool, UnexpectedSecret> {
+///         Ok(true)
+///     }
+/// }
+/// ```
+pub trait MergingWithUnset {
+    type Target;
+
+    fn merge(self, other: Self) -> Self;
+    fn try_build(self) -> Result<Self::Target, Error>;
+    fn contains_non_secret_data(&self) -> Result<bool, UnexpectedSecret>;
+}
+
+impl<T> ConfigurationBuilder for MergingUnsetBuilder<T>
+where
+    T: MergingWithUnset + DeserializeOwned,
+{
+    type Target = T::Target;
+
+    fn merge(self, other: Self) -> Self {
+        match (self, other) {
+            (Self::Unset, merged) | (merged, Self::Unset) => merged,
+            (Self::Set(s), Self::Set(o)) => Self::Set(s.merge(o)),
+        }
+    }
+
+    fn try_build(self) -> Result<Self::Target, Error> {
+        match self {
+            Self::Unset => Err(Error::MissingValue(MissingValue::default())),
+            Self::Set(s) => s.try_build(),
+        }
+    }
+
+    fn contains_non_secret_data(&self) -> Result<bool, UnexpectedSecret> {
+        match self {
+            Self::Unset => Ok(false),
+            Self::Set(s) => s.contains_non_secret_data(),
+        }
+    }
+}

confik/src/third_party.rs

@@ -476,13 +476,16 @@ mod ahash {
 mod serde_json {
     use serde_json::Value;
 
-    use crate::{Configuration, ConfigurationBuilder};
+    use crate::{
+        helpers::{MergingUnsetBuilder, MergingWithUnset},
+        Configuration,
+    };
 
     impl Configuration for Value {
-        type Builder = Self;
+        type Builder = MergingUnsetBuilder<Self>;
     }
 
-    impl ConfigurationBuilder for Value {
+    impl MergingWithUnset for Value {
         type Target = Self;
 
         fn merge(self, other: Self) -> Self {

confik/tests/third_party.rs

@@ -157,102 +157,72 @@ mod bigdecimal {
 
 #[cfg(feature = "serde_json")]
 mod serde_json {
-    use confik::ConfigurationBuilder as _;
-    use serde_json::{json, Value};
-
-    #[test]
-    fn merge_left_type_wins_over_right_type() {
-        // When the two sides have different types, the left always wins
-        assert_eq!(Value::Null.merge(json!({"key": "value"})), Value::Null);
-        assert_eq!(json!(true).merge(json!([1, 2, 3])), json!(true));
-        assert_eq!(json!(42).merge(json!({"key": "value"})), json!(42));
-        assert_eq!(json!("hello").merge(json!(99)), json!("hello"));
-        assert_eq!(json!([1, 2]).merge(json!({"key": "value"})), json!([1, 2]));
-        assert_eq!(
-            json!({"key": "value"}).merge(json!([1, 2])),
-            json!({"key": "value"})
-        );
-    }
-
-    #[test]
-    fn merge_arrays_are_concatenated() {
-        assert_eq!(json!([1, 2]).merge(json!([3, 4])), json!([1, 2, 3, 4]));
-    }
-
-    #[test]
-    fn merge_objects_combine_disjoint_keys() {
-        assert_eq!(
-            json!({"a": 1}).merge(json!({"b": 2})),
-            json!({"a": 1, "b": 2})
-        );
-    }
-
-    #[test]
-    fn try_build_returns_value_unchanged() {
-        let value = json!({"key": "value", "num": 42});
-        assert_eq!(value.clone().try_build().unwrap(), value);
-    }
-
-    #[test]
-    fn contains_non_secret_data() {
-        for value in [Value::Null, json!(""), json!([]), json!({})] {
-            assert!(!value.contains_non_secret_data().unwrap());
-        }
-        for value in [
-            json!(true),
-            json!(42),
-            json!("hello"),
-            json!([1, 2, 3]),
-            json!({"key": "value"}),
-        ] {
-            assert!(value.contains_non_secret_data().unwrap());
-        }
-    }
-
     #[cfg(feature = "toml")]
     mod toml {
         use confik::{Configuration, TomlSource};
         use serde_json::{json, Value};
 
         #[derive(Configuration)]
-        struct Config {
+        struct Root {
+            leaf: Leaf,
+        }
+
+        #[derive(Configuration)]
+        struct Leaf {
             data: Value,
         }
 
         #[test]
         fn value_loads_from_toml() {
             let toml = r#"
-                [data]
+                [leaf.data]
                 key = "hello"
                 num = 42
             "#;
 
-            let config = Config::builder()
+            let config = Root::builder()
                 .override_with(TomlSource::new(toml))
                 .try_build()
                 .unwrap();
 
-            assert_eq!(config.data, json!({"key": "hello", "num": 42}));
+            assert_eq!(config.leaf.data, json!({"key": "hello", "num": 42}));
         }
 
         #[test]
         fn objects_merged_from_multiple_sources() {
             let toml_1 = r#"
-                [data]
+                [leaf.data]
                 item_1 = 1
             "#;
             let toml_2 = r#"
-                [data]
+                [leaf.data]
+                item_2 = 2
+            "#;
+
+            let config = Root::builder()
+                .override_with(TomlSource::new(toml_1))
+                .override_with(TomlSource::new(toml_2))
+                .try_build()
+                .unwrap();
+
+            assert_eq!(config.leaf.data, json!({"item_1": 1, "item_2": 2}));
+        }
+
+        #[test]
+        fn object_merged_missing_in_first_source() {
+            let toml_1 = r#"
+                [leaf.data]
                 item_2 = 2
             "#;
+            let toml_2 = "[leaf]";
 
-            let config = Config::builder()
+            let config = Root::builder()
                 .override_with(TomlSource::new(toml_1))
                 .override_with(TomlSource::new(toml_2))
                 .try_build()
                 .unwrap();
 
-            assert_eq!(config.data, json!({"item_1": 1, "item_2": 2}));
+            assert_eq!(config.leaf.data, json!({ "item_2": 2 }));
         }
     }
 }