From 85001ce71f1ec023ecb6e491db2924f6c0d2c4dd Mon Sep 17 00:00:00 2001 From: tenuous-guidance <105654822+tenuous-guidance@users.noreply.github.com> Date: Tue, 31 Mar 2026 12:45:26 +0100 Subject: [PATCH 1/2] fix(serde_json): value merging was incorrect when not present in a file --- confik/CHANGELOG.md | 3 ++ confik/src/helpers.rs | 93 +++++++++++++++++++++++++++++++++++++ confik/src/third_party.rs | 9 ++-- confik/tests/third_party.rs | 90 ++++++++++++----------------------- 4 files changed, 132 insertions(+), 63 deletions(-) diff --git a/confik/CHANGELOG.md b/confik/CHANGELOG.md index be51bb0..5a16f8a 100644 --- a/confik/CHANGELOG.md +++ b/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 `::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). diff --git a/confik/src/helpers.rs b/confik/src/helpers.rs index 6cbf19c..c66cd40 100644 --- a/confik/src/helpers.rs +++ b/confik/src/helpers.rs @@ -371,3 +371,96 @@ 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 { + #[default] + Unset, + Set(T), +} + +impl<'de, T> Deserialize<'de> for MergingUnsetBuilder +where + T: Deserialize<'de>, +{ + fn deserialize(deserializer: D) -> Result + 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` inside, and then handle the `Option` yourself. This trait allows that to be delegated to [`MergingUnsetBuilder`]. +/// ``` +/// use confik::{ +/// helpers::{MergingUnsetBuilder, MergingWithUnset}, +/// Configuration, Error, UnexpectedSecret, +/// }; +/// use serde::Deserialize; +/// +/// #[derive(Debug, Deserialize)] +/// struct AdditiveMerge(usize); +/// +/// impl Configuration for AdditiveMerge { +/// type Builder = MergingUnsetBuilder; +/// } +/// +/// impl MergingWithUnset for AdditiveMerge { +/// type Target = Self; +/// +/// fn merge(self, other: Self) -> Self { +/// Self(self.0 + other.0) +/// } +/// +/// fn try_build(self) -> Result { +/// Ok(self) +/// } +/// +/// fn contains_non_secret_data(&self) -> Result { +/// Ok(true) +/// } +/// } +/// ``` +pub trait MergingWithUnset { + type Target; + + fn merge(self, other: Self) -> Self; + fn try_build(self) -> Result; + fn contains_non_secret_data(&self) -> Result; +} + +impl ConfigurationBuilder for MergingUnsetBuilder +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 { + match self { + Self::Unset => Err(Error::MissingValue(MissingValue::default())), + Self::Set(s) => s.try_build(), + } + } + + fn contains_non_secret_data(&self) -> Result { + match self { + Self::Unset => Ok(false), + Self::Set(s) => s.contains_non_secret_data(), + } + } +} diff --git a/confik/src/third_party.rs b/confik/src/third_party.rs index dacf3f4..f84258d 100644 --- a/confik/src/third_party.rs +++ b/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; } - impl ConfigurationBuilder for Value { + impl MergingWithUnset for Value { type Target = Self; fn merge(self, other: Self) -> Self { diff --git a/confik/tests/third_party.rs b/confik/tests/third_party.rs index cabada2..795224f 100644 --- a/confik/tests/third_party.rs +++ b/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 })); } } } From 15d174a537b851c11874436c5fbe9e753bc50439 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Tue, 31 Mar 2026 20:41:38 +0100 Subject: [PATCH 2/2] docs: update changelog --- confik/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/confik/CHANGELOG.md b/confik/CHANGELOG.md index 5a16f8a..fcdd57f 100644 --- a/confik/CHANGELOG.md +++ b/confik/CHANGELOG.md @@ -3,7 +3,7 @@ ## 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 +- Fix `serde_json::Value` merge when the first parsed config doesn't have any value set. ## 0.15.10