fix: Deep-merge nested data documents in Engine::add_data

[kusha]

Summary

Engine::add_data performed a shallow merge. Adding a nested object under a key that already existed would either replace the existing subtree wholesale or raise a spurious generated multiple times conflict, 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::merge now recurses into nested objects, so keys from both sides are preserved — matching OPA's data-document merge semantics (internal/merge/merge.go).
• Nested sets are unioned on merge. This is a regorus extension (OPA data documents are JSON and cannot contain sets), but it keeps set behavior consistent between top-level and nested keys.
• Genuine leaf conflicts (the same path holding two different scalar values) still error. Equal values remain a no-op, which the shared rule-evaluation path (Interpreter::merge_rule_value) relies on, since a rule may legally produce the same value more than once.
• Engine::add_data doctest and CHANGELOG.md updated to reflect the deep-merge behavior.

The with data.x as ... modifier is unaffected: it replaces the targeted subtree directly and does not go through Value::merge.

Tests

Added to src/tests/interpreter/mod.rs:

• Object deep-merge (single- and multi-level), leaf-conflict errors, object-vs-scalar conflict errors, equal-leaf no-op.
• Set union (top-level and nested), equal-set no-op.
• with data.x modifier replace semantics (nested replace preserves siblings; whole-subtree replace).
• Rules reading deep-merged base data, and rule values coexisting with merged base data.

Validation

• cargo test --lib: all passing.
• cargo test --test opa: pass/fail counts identical to main — no conformance regression.
• cargo xtask pre-commit (rustfmt + clippy -Dwarnings): clean.

[Copilot]

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::merge to 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_data docs/doctest and CHANGELOG.md to 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.

[kusha]

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).

[anakrish]

🐛 add_data is not atomic — a failed nested merge partially mutates engine data

Now that Value::merge recurses, a conflict in a later nested key is reported only after earlier keys from the same document have already been written into the engine's data. Because add_data merges directly into the live init_data (engine.rs:468 → get_init_data_mut().merge(data)), a rejected add_data call can leave the engine in a partially-mutated state.

Repro (fails today):

engine.add_data(Value::from_json_str(r#"{ "a": { "z": 1 } }"#)?)?;
// `m` sorts before `z`, so `a.m` is inserted, then `a.z` (1 vs 3) conflicts and bails.
assert!(engine.add_data(Value::from_json_str(r#"{ "a": { "m": 2, "z": 3 } }"#)?).is_err());
// BUG: engine data is now { "a": { "m": 2, "z": 1 } } — the rejected `m` leaked in.

The merge doc says a conflict "is an error", but callers reasonably expect an errored add_data to be a no-op (all-or-nothing).

Suggested fix — merge into a candidate copy and commit only on success. Value is Rc-backed with copy-on-write, so this only deep-copies the paths actually touched by the merge (the untouched subtrees stay shared):

pub fn add_data(&mut self, data: Value) -> Result<()> {
    if data.as_object().is_err() {
        bail!("data must be object");
    }
    // Merge into a candidate copy so a mid-merge conflict cannot leave the
    // data document partially mutated: add_data is all-or-nothing.
    let mut candidate = self.interpreter.get_init_data().clone();
    candidate.merge(data)?;
    *self.interpreter.get_init_data_mut() = candidate;
    self.prepared = false;
    Ok(())
}

(Cost is O(number of top-level keys) copy-on-write per call; acceptable for correctness. A more surgical rollback is possible but far more complex.)

Suggested regression test (add to src/tests/interpreter/mod.rs):

#[test]
fn test_add_data_failed_merge_is_atomic() -> Result<()> {
    let mut engine = Engine::new();

    engine.add_data(Value::from_json_str(r#"{ "a" : { "z" : 1 } }"#)?)?;

    // Mixes a new key `m` with a conflicting leaf `z` (1 vs 3). The whole call
    // must fail AND leave existing data untouched — `m` must not leak in.
    assert!(engine
        .add_data(Value::from_json_str(r#"{ "a" : { "m" : 2, "z" : 3 } }"#)?)
        .is_err());

    assert_eq!(
        engine.get_data(),
        Value::from_json_str(r#"{ "a" : { "z" : 1 } }"#)?
    );

    Ok(())
}

Note this pre-dates the PR for top-level keys, but the new recursion widens the window to nested paths, so it's worth closing here.

_{🤖 Found during automated review (deep-review skill), verified manually.}

[kusha]

Thanks @anakrish , good catch. Fixed.

I went with validate-then-merge instead of candidate-copy. Candidate-copy costs even on success: after eval, init_data is shared (cloned into with_document["data"] and self.data), so clone() + merge forces an Rc::make_mut COW of the touched spine on every add, and discards the in-place fast path when init_data is uniquely owned. Accumulating under one subtree becomes O(N²).

self.interpreter.get_init_data().check_mergeable(&data)?;
self.prepared = false;
self.interpreter.get_init_data_mut().merge(data)

The read-only check is O(overlap), allocates nothing, and short-circuits at absent keys (new-key adds never walk the payload), so the happy path stays zero-copy and in-place. check_mergeable mirrors merge's conflict rule exactly, so they can't drift. Added your repro plus set and array variants.

[anakrish]

🔁 Second-round review (post-update)

Thanks for the atomicity fix — the conflict path is now correctly all-or-nothing. Re-reviewing the update surfaced two new issues plus some hardening items. Findings below are ordered by severity; the two most important are #1 and #2.

---

1. 🔴 HIGH — Atomicity is still broken for the allocator-limit failure mode

check_mergeable mirrors merge's semantic conflict rules, but merge can also fail at enforce_limit_anyhow() after it has already mutated:

map.insert(k.clone(), v.clone());
enforce_limit_anyhow()?;   // <-- fails AFTER the insert

So on an allocator-memory-limits build, an add_data that passes check_mergeable (e.g. a pure insertion of a large nested doc) can insert some keys, then return Err at the limit — leaving the data document partially merged. This directly contradicts the newly-added doc guarantee:

The merge is atomic: if any conflict is detected … the existing data document is left unchanged.

Suggested fix — merge into a candidate copy and commit only on success. Value is Rc-backed with copy-on-write, so this only deep-copies the paths the merge actually touches, and it makes every error mode (conflict and limit) transactional — which also lets check_mergeable be removed:

pub fn add_data(&mut self, data: Value) -> Result<()> {
    if data.as_object().is_err() {
        bail!("data must be object");
    }
    // All-or-nothing: merge into a candidate, commit only on success. Covers both
    // semantic conflicts AND allocator-limit failures. Rc copy-on-write means only
    // the touched subtrees are cloned.
    let mut candidate = self.interpreter.get_init_data().clone();
    candidate.merge(data)?;
    *self.interpreter.get_init_data_mut() = candidate;
    self.prepared = false;
    Ok(())
}

Regression test (needs the allocator-memory-limits config): set a memory limit just above current usage, add_data a large nested doc that trips the limit mid-merge, assert the call errors and engine.get_data() is unchanged.

---

2. 🟠 MEDIUM — Zero-arg function conflicts now silently deep-merge instead of erroring

Value::merge is shared with rule/function materialization via Interpreter::merge_rule_value. Complete rules are safe (their conflict is caught independently in update_rule_value, interpreter.rs:1697), but zero-arg functions materialize through update_data → merge_rule_value → Value::merge (interpreter.rs:3785, 3673, 3404), so they inherit the new deep-merge. Verified empirically on this branch:

Policy	before this PR	this PR
f() := {"a":{"x":1}} / f() := {"a":{"y":2}}	Err (conflict)	{"a":{"x":1,"y":2}} — silently merged ⚠️
p := {"a":{"x":1}} / p := {"a":{"y":2}} (complete rule)	Err	Err (unaffected ✅)

An authoring mistake that used to be a hard error now silently produces a merged value. It's narrow (zero-arg functions + a shared key whose values are both objects/sets), but it's a silent evaluation-semantics change. (Note: disjoint object keys already merged pre-PR, so that specific case is not a regression.)

Suggested fix — don't let the data-document deep-merge leak into rule materialization. Either give add_data its own deep_merge and keep Value::merge as the strict equality-or-conflict primitive for merge_rule_value, or gate the recursive arm behind a flag/param that only add_data sets.

Regression test (add to src/tests/interpreter/mod.rs):

#[test]
fn test_zero_arg_function_object_conflict_errors() -> Result<()> {
    let mut engine = Engine::new();
    engine.add_policy(
        "p.rego".to_string(),
        r#"
package test
f() := {"a": {"x": 1}}
f() := {"a": {"y": 2}}
"#
        .to_string(),
    )?;
    // Two zero-arg definitions producing objects that share a key must conflict,
    // NOT silently deep-merge.
    assert!(engine
        .eval_query("data.test.f".to_string(), false)
        .is_err());
    Ok(())
}

---

3. 🟡 MEDIUM — Unconditional Rc::make_mut(map) clones large shared objects with no limit check

The object arm now clones the map up front, before it knows whether any mutation is needed:

(Value::Object(map), Value::Object(new)) => {
    let map = Rc::make_mut(map);   // clones the whole map if shared
    for (k, v) in new.iter() { ... }

If a caller holds a snapshot (shared Rc, e.g. via get_data()) and the incoming doc only overlaps equal values, this deep-clones a potentially large object while performing no insertion — so the object-arm enforce_limit_anyhow() (which lives only in the None insert branch) never runs, and memory can exceed the configured limit undetected. The old code called Rc::make_mut only on the insert path.

Suggested fix — inspect with map.get(k) first and only Rc::make_mut(map) when an insert or a mutating recursion is actually required; or run enforce_limit_anyhow() immediately after any Rc::make_mut that may clone.

---

4. 🟡 MEDIUM/LOW — Recursive merge/precheck has no depth guard (now doubled)

Both merge and check_mergeable recurse one stack frame per nesting level, and check_mergeable runs first. A deeply nested, programmatically-constructed Value (JSON is capped ~128 by serde, but the Value API is not) added via add_data can overflow the stack → process abort instead of a recoverable Result.

Suggested fix — thread a depth counter with a conservative cap (returning an error past it), or rewrite the object traversal iteratively with an explicit stack. A regression test after adding the cap: assert an over-deep document is rejected rather than crashing.

---

5. 🔵 LOW — Doc wording + test coverage

• The "atomic / all-or-nothing" doc wording is broader than the implementation until Code from github.com/anakrish/rego-rs #1 is fixed; otherwise narrow it to "conflict handling is atomic".
• Test gaps to close alongside the fixes above: (a) allocator-limit atomicity (Code from github.com/anakrish/rego-rs #1), (b) zero-arg-function object conflict (Update license #2), (c) recursion-depth rejection (Improvements to github workflow #4).

---

_{🤖 Automated second-round review (code-review + deep-review skills); findings #1 and #2 verified manually by building and running targeted tests on this branch.}

[kusha]

@anakrish I've addressed all five findings. Two of them (#1 and #2) I approached slightly differently than suggested; flagging those so you can tell me if you'd prefer the narrower/simpler version.

1. 🔴 Allocator-limit atomicity

Fixed, but with a build-split rather than unconditionally switching to the candidate copy:

• allocator-memory-limits build: exactly your suggestion — merge into a candidate clone and commit only on success, so both conflict and limit failures are transactional.
• Default build: a read-only check_mergeable pre-scan, then deep-merge in place. With the feature off, enforce_limit_anyhow() is a no-op, so a semantic conflict is the only failure mode and is fully determined by the inputs — the pre-scan guarantees atomicity without cloning the touched subtrees on every add_data.

So check_mergeable survives only as the default-build fast path; the candidate copy owns atomicity wherever a limit can trip mid-merge. It's a perf-vs-simplicity call and I lean slightly toward keeping the zero-copy path.

Tests (tests/memory_limits.rs): add_data_memory_limit_partial_merge_is_atomic (limit trips mid-merge → call errors and get_data() is unchanged) and add_data_conflict_is_atomic_on_allocator_build.

2. 🟠 Zero-arg function conflicts

Fixed by separating the two merges, as you suggested: Value::merge is now the strict, shallow equality-or-conflict primitive (used by rule/function materialization), and a new Value::deep_merge is the recursive one used only by add_data. Rule materialization routes through an explicit RuleValueMerge { Combine, Strict } policy instead of inheriting the recursive arm.

One scope note: I took zero-arg functions all the way to OPA v1.2.0 complete-rule semantics — so any two differing outputs conflict, including disjoint top-level keys (f() := {"a":1} + f() := {"b":2} now errors). You noted that disjoint case wasn't a pre-PR regression; I made it conflict anyway to match OPA rather than leave a known divergence. If you'd prefer to keep this PR minimal and only restore the nested-object error (leaving the pre-existing disjoint-key merge as-is), I can scope it back.

Coverage lives in a conformance matrix (tests/interpreter/cases/rule/multiple_outputs.yaml, 14 cases cross-checked against OPA v1.2.0) rather than a single unit test — it includes fn_nested_object_conflict_no_deep_merge (your exact scenario) and fn_toplevel_disjoint_conflict, plus static/dynamic partial-object and ref-head cases.

3. 🟡 Unconditional Rc::make_mut

Fixed as suggested. The object arm now does a read-only map.get(k) probe that classifies each key (skip / insert / recurse / conflict) and only calls Rc::make_mut when a key actually mutates. No-op re-adds and pre-mutation conflicts no longer clone a shared map, and enforce_limit_anyhow() runs on each insert/recurse. Added three Rc::ptr_eq tests asserting no clone on a no-op subset, an equal nested object, and a first-key conflict.

4. 🟡 Recursion depth guard

Fixed via a depth counter threaded through both deep_merge and check_mergeable, bailing past MAX_MERGE_DEPTH = 128 (to match serde_json's default, so it never rejects input the JSON parser would accept). Regression tests build an over-deep Value iteratively and assert both functions return Err instead of overflowing.

5. 🔵 Doc wording + test coverage

add_data's doc now describes the per-build atomicity precisely instead of a blanket "atomic". The three test gaps are closed: (a) allocator-limit atomicity, (b) zero-arg-function conflict, (c) recursion-depth rejection.