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

[tenuous-guidance]

I kind of want to replace OptionBuilder with this approach too, but it's a pretty big breaking change as unset won't default to None anymore without an explicit default annotation.

This is a positive in that you can require an explicit null if you want one (currently not possible to do), but also an easy to miss breakage.

Summary by CodeRabbit

• New Features

  • Added builder utilities to support richer merging behavior for custom configuration types.

• Bug Fixes

  • Fixed JSON/TOML merging when a first source has no value, ensuring correct composition across sources.

• Documentation

  • Updated changelog with entries for the new helper abstractions and the merging behavior fix.

• Tests

  • Refactored and extended merge tests to cover nested structures and missing-first-source scenarios.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 56c97511-03ad-43a6-a5d1-725ec6dd4532

📥 Commits

Reviewing files that changed from the base of the PR and between 85001ce and 15d174a.

📒 Files selected for processing (1)

• confik/CHANGELOG.md

---

📝 Walkthrough

Walkthrough

Adds a helper builder abstraction (MergingUnsetBuilder + MergingWithUnset), refactors serde_json::Value to use it, updates tests to a nested Root { leaf { data } } structure, and records changelog entries including a fix for merging when the first parsed source is missing values.

Changes

Cohort / File(s)	Summary
Changelog confik/CHANGELOG.md	Added Unreleased entries documenting MergingUnsetBuilder, MergingWithUnset, and a fix for serde_json::Value merging when the first source is unset.
Helper abstraction confik/src/helpers.rs	Added pub enum MergingUnsetBuilder<T> (Unset/Set(T)), Deserialize impl, pub trait MergingWithUnset (assoc Target, merge, try_build, contains_non_secret_data), and ConfigurationBuilder impl for MergingUnsetBuilder<T>.
serde_json integration confik/src/third_party.rs	Changed Configuration/builder usage for serde_json::Value to helpers::MergingUnsetBuilder<serde_json::Value> and implemented helpers::MergingWithUnset for serde_json::Value, preserving prior merge/build semantics with corrected unset handling.
Tests confik/tests/third_party.rs	Removed older serde_json-specific tests; refactored TOML/JSON tests to use nested Root { leaf { data } } and added a merge test covering missing first-source object behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• fix(serde_json): value merging was incorrect when not present in a file #390: Adds MergingUnsetBuilder and MergingWithUnset and switches serde_json::Value to use them (same code-level intent).
• feat(third_party): add support for serde_json::Value #374: Prior modifications to serde_json::Value configuration support and merge implementations that are refactored here.

Suggested reviewers

• robjtede

Poem

🐰 I hop through fields of Set and Unset bright,
I patch the gaps where first sources came up light,
I coax two JSONs to merge without a fight,
A nibble here, a nudge there—now they fit just right. 🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: correcting serde_json Value merging behavior when values are absent from configuration files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sj-fix

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@confik/src/helpers.rs`:
- Around line 396-400: Replace the manual impl Default for
MergingUnsetBuilder<T> with a derived Default: add Default to the enum's derive
list and mark the Unset variant with #[default]; specifically, update the
MergingUnsetBuilder definition (the enum with variant Unset) to #[derive(...,
Default)] and annotate Unset with #[default] so the compiler/Clippy no longer
flags the manual impl.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 64fad18b-0dc7-40fb-bfd5-08b6d0ee02c4

📥 Commits

Reviewing files that changed from the base of the PR and between b780764 and df63ed8.

📒 Files selected for processing (4)

• confik/CHANGELOG.md
• confik/src/helpers.rs
• confik/src/third_party.rs
• confik/tests/third_party.rs

[coderabbitai]

🧹 Nitpick comments (1)

confik/tests/third_party.rs (1)

211-226: Consider clarifying the test name or adding a comment.

This test validates the PR's core fix, but the name object_merged_missing_in_first_source could be confusing given confik's merge semantics. Based on learnings, override sources are reduced in reverse order — so toml_2 (last added) is the "first" source in the merge chain (higher priority), and it's the one missing data. Meanwhile, toml_1 (first added) is lower priority but provides the value.

Consider either:

1. Renaming to object_merged_missing_in_higher_priority_source, or
2. Adding a brief comment explaining the merge order

💡 Suggested comment clarification

 #[test]
 fn object_merged_missing_in_first_source() {
+    // toml_2 is higher priority (last override) but lacks `data` field.
+    // Verifies that MergingUnsetBuilder falls back to toml_1's value.
     let toml_1 = r#"
         [leaf.data]
         item_2 = 2
     "#;
     let toml_2 = "[leaf]";

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@confik/tests/third_party.rs` around lines 211 - 226, The test name
object_merged_missing_in_first_source is misleading given confik's reverse
reduction of override sources; update the test to either rename the function to
object_merged_missing_in_higher_priority_source or add a one-line comment above
the test explaining that override sources passed via
Root::builder().override_with(...) are reduced in reverse order (so toml_2 is
higher priority and missing data while toml_1 supplies it), referencing the test
symbols toml_1, toml_2, Root::builder(), override_with, try_build, and
config.leaf.data so future readers understand the merge semantics.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@confik/tests/third_party.rs`:
- Around line 211-226: The test name object_merged_missing_in_first_source is
misleading given confik's reverse reduction of override sources; update the test
to either rename the function to object_merged_missing_in_higher_priority_source
or add a one-line comment above the test explaining that override sources passed
via Root::builder().override_with(...) are reduced in reverse order (so toml_2
is higher priority and missing data while toml_1 supplies it), referencing the
test symbols toml_1, toml_2, Root::builder(), override_with, try_build, and
config.leaf.data so future readers understand the merge semantics.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 04acf80c-d15e-4ce6-aea8-9b55df09c01c

📥 Commits

Reviewing files that changed from the base of the PR and between df63ed8 and 85001ce.

📒 Files selected for processing (4)

• confik/CHANGELOG.md
• confik/src/helpers.rs
• confik/src/third_party.rs
• confik/tests/third_party.rs

✅ Files skipped from review due to trivial changes (1)

• confik/CHANGELOG.md

🚧 Files skipped from review as they are similar to previous changes (2)

• confik/src/third_party.rs
• confik/src/helpers.rs

[robjtede]

makes sense to me, we can chat about the wider behavioral change elsewhere