fix: Deep-merge nested data documents in Engine::add_data#760
Open
kusha wants to merge 3 commits into
Open
Conversation
add_data previously performed a shallow merge: adding a nested object under a key that already existed either replaced the whole subtree or errored on a spurious conflict, instead of merging the trees. This makes Engine::add_data (and the shared Value::merge) recurse into nested objects so keys from both sides are preserved, matching OPA's data-document merge semantics. Nested sets are unioned as a regorus extension (OPA data is JSON and has no sets). Genuine leaf conflicts (same path, two different scalar values) still error; equal values remain a no-op, which the shared rule-evaluation path relies on. Adds tests for object deep-merge, set union, leaf/type conflicts, and interaction with the 'with data.x' modifier. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
kusha
force-pushed
the
markbirger-microsoft-data-merge-bug-analysis
branch
from
749d710 to
efff5c4
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes Engine::add_data to follow OPA-style deep-merge semantics for nested objects by making Value::merge recursive for object values (and unioning nested sets as a regorus extension). This aligns repeated add_data calls with expected data-document composition behavior without spurious conflicts or subtree replacement.
Changes:
- Update
Value::mergeto recursively merge nested objects and union nested sets, while still erroring on genuine leaf conflicts (and tolerating equal-values as a no-op). - Add interpreter tests covering deep-merge behavior, conflicts, set union, and
with data.* as ...replacement semantics. - Update
Engine::add_datadocs/doctest andCHANGELOG.mdto document the new merge behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/value/mod.rs | Implements recursive object merge and nested set union in Value::merge, updating merge semantics used by Engine::add_data. |
| src/tests/interpreter/mod.rs | Adds tests to validate deep-merge behavior, conflicts, set union, and with data.* replacement behavior. |
| src/engine.rs | Updates Engine::add_data documentation/doctest to reflect deep-merge behavior and conflict rules. |
| CHANGELOG.md | Notes the deep-merge behavior change in the Unreleased section. |
Comment on lines
+1397
to
+1404
| let both_mergeable = matches!( | ||
| (&*existing, v), | ||
| (Value::Object(_), Value::Object(_)) | ||
| | (Value::Set(_), Value::Set(_)) | ||
| ); | ||
| if both_mergeable { | ||
| existing.merge(v.clone())?; | ||
| } else if *existing != *v { |
Contributor
Author
There was a problem hiding this comment.
Optimized. The (Set, Set) arm no longer calls Rc::make_mut(new). It now mem::takes the RHS and Rc::try_unwraps it: elements are moved out when the set is uniquely owned, and only the per-element Rc handles are cloned when it's shared (the v.clone() path).
Copilot review on microsoft#760 noted the doc comment called non-mergeable variants 'non-container values', which is misleading since arrays are containers yet still conflict unless equal. Reword to describe a conflict as any differing pair that is not both objects or both sets (e.g. unequal scalars or arrays). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
When unioning sets in Value::merge, the RHS set is often shared: the object arm recurses via existing.merge(v.clone()), which bumps the incoming set's Rc refcount. The old Rc::make_mut(new) then structurally deep-cloned the entire RHS BTreeSet just to drain it via append and immediately discard the copy. Move the elements out when the RHS set is uniquely owned, and otherwise clone only the per-element Rc handles into the destination. The union result is identical (BTreeSet dedups), but no throwaway set is allocated on the nested-merge path exercised by add_data deep-merge. Addresses a Copilot review comment on microsoft#760. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Engine::add_dataperformed a shallow merge. Adding a nested object under a key that already existed would either replace the existing subtree wholesale or raise a spuriousgenerated multiple timesconflict, instead of deep-merging the two data trees.
rust engine.add_data(Value::from_json_str(r#"{ "y": { "a": 10 } }"#)?)?; engine.add_data(Value::from_json_str(r#"{ "y": { "b": 20 } }"#)?)?; // previously errored / dropped "a" // data["y"] is now { "a": 10, "b": 20 } Changes
Value::mergenow recurses into nested objects, so keys from both sides are preserved — matching OPA's data-document merge semantics (internal/merge/merge.go).Interpreter::merge_rule_value) relies on, since a rule may legally produce the same value more than once.Engine::add_datadoctest andCHANGELOG.mdupdated to reflect the deep-merge behavior.The
with data.x as ...modifier is unaffected: it replaces the targeted subtree directly and does not go throughValue::merge.Tests
Added to
src/tests/interpreter/mod.rs:with data.xmodifier replace semantics (nested replace preserves siblings; whole-subtree replace).Validation
cargo test --lib: all passing.cargo test --test opa: pass/fail counts identical tomain— no conformance regression.cargo xtask pre-commit(rustfmt + clippy-Dwarnings): clean.