Skip to content

Commit 9a486c7

Browse files
kushaMark BirgerCopilot
authored
fix: Deep-merge nested data documents in Engine::add_data (microsoft#760)
* Deep-merge nested data documents in Engine::add_data 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> * docs(value): clarify Value::merge conflict wording 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> * perf(value): avoid deep-cloning RHS set during merge union 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> * fix(engine): make add_data atomic on merge conflict Now that Value::merge recurses, a conflict in a later nested key was reported only after earlier keys of the same document had already been written into the live init_data, leaving the engine partially mutated on a rejected add_data. Add a read-only Value::check_mergeable that mirrors merge's conflict rule (objects deep-merge, sets union, equal values no-op, anything else conflicts) and run it in add_data before merging. On conflict nothing is mutated, so add_data is all-or-nothing. The check allocates nothing and never copies the data spine, preserving merge's in-place uniquely-owned fast path (no candidate copy of the data document). Adds regression tests for a partial object-leaf conflict and a partial set-union conflict. Reported by a maintainer on microsoft#760. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * test(engine): add array atomicity regression for add_data Arrays are atomic leaves, so a differing array at a shared path is a conflict. The new key sorts before the conflicting array key, so a naive in-place merge would leak the new key before hitting the conflict. This test locks in that add_data rejects the whole call and leaves data untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix: make add_data atomic under allocator memory limits On �llocator-memory-limits builds, Value::merge runs the limit check *after* inserting each key, so an add_data whose merge trips the limit mid-way left the data document partially mutated. check_mergeable only models semantic conflicts, not limit failures, so the validate-then-merge precheck couldn't cover this failure mode. Use a build-split strategy in �dd_data: - default builds: keep the zero-copy validate-then-merge fast path (a conflict is the only way the merge can fail). - allocator-memory-limits builds: merge into a candidate copy and commit only on success, making both conflict and limit failures transactional. Value is Rc/copy-on-write, so only touched subtrees are cloned. check_mergeable is now cfg-gated to the default build to avoid dead code. Tests (allocator-memory-limits build): add a partial-merge atomicity test (limit trips mid-merge, data must be untouched) and a candidate-copy conflict-atomicity test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix: separate strict rule-output merge from data-document deep-merge microsoft#760 made Value::merge recursive so Engine::add_data deep-merges nested data documents. But that same method also backs rule materialization, where recursion is wrong: two rule definitions producing different outputs for one path must conflict (OPA complete-rule semantics), not silently combine. Split the two behaviors: - Value::merge is strict and shallow again (as pre-microsoft#760): a key on both sides must be equal or it conflicts; used for rule outputs. - Value::deep_merge is the recursive data-document merge behind add_data; check_mergeable validates it up front without allocating, so the default build merges in place instead of cloning a candidate. Also fix zero-arg functions (f() := ...): route their materialization through strict equality via a new RuleValueMerge selector, so disjoint outputs ({a:1} vs {b:2}) conflict as OPA does while prefix scaffolding (a.foo + a.bar) still combines. Add a 14-case interpreter conformance matrix (multiple_outputs.yaml) covering functions, static/dynamic partial objects, and ref-heads, matched against OPA v1.2.0. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * perf(value): make deep_merge acquire mutable access lazily deep_merge's object arm called Rc::make_mut on the target map up front, cloning a shared map's spine even when the merge changed nothing (a no-op subset re-add) or conflicted before any mutation. Decide each incoming key from a read-only probe (skip / insert / recurse / conflict) and take Rc::make_mut only when a key actually mutates, so no-op and conflict merges leave shared maps untouched. Behavior is unchanged: the equality short-circuit that previously ran inside the recursive call now runs in the probe, and conflicts bail with the same message. Add value tests asserting Rc::ptr_eq is preserved across no-op subset, equal-nested-object, and first-key-conflict merges. OPA conformance unchanged (3021 pass / 651 fail, byte-identical). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * feat(value): bound deep_merge recursion depth to prevent stack-overflow DoS deep_merge and check_mergeable recursed unbounded on object/set nesting. A Value built without serde_json's parse-time recursion limit (the Python and Ruby native bindings, or programmatic construction) could therefore drive add_data into a stack overflow -- an uncatchable abort that poisons every engine in an FFI process. Thread a depth counter through both functions and bail past MAX_MERGE_DEPTH (128, matching serde_json's default) so over-deep data fails with a clean Err. In the default build check_mergeable trips first, keeping add_data atomic; the guard in deep_merge covers the allocator-memory-limits build and any disjoint-then-overlapping merge. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs(changelog): note strict zero-arg function conflict and add_data depth limit Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Mark Birger <markbirger@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 9838b25 commit 9a486c7

8 files changed

Lines changed: 1113 additions & 17 deletions

File tree

CHANGELOG.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
66

77
## [Unreleased]
88

9+
### Fixed
10+
11+
- `Engine::add_data` now deep-merges nested data documents instead of only merging top-level keys. Adding `{ "a": { "x": 1 } }` followed by `{ "a": { "y": 2 } }` now yields `{ "a": { "x": 1, "y": 2 } }` (matching OPA's data-document merge). Nested sets under a shared key are unioned. Only genuine leaf conflicts (the same path holding two different values) are reported as errors.
12+
- A zero-arg function producing two different complete values (e.g. `f() := { "a": 1 }` and `f() := { "b": 2 }`) is now reported as a conflict, matching OPA's complete-rule semantics, instead of silently combining the outputs.
13+
14+
### Security
15+
16+
- `Engine::add_data` now rejects data nested beyond 128 levels instead of risking a stack overflow on adversarially deep input.
17+
918
## [0.10.1](https://github.com/microsoft/regorus/compare/regorus-v0.10.0...regorus-v0.10.1) - 2026-05-22
1019

1120
### Fixed

src/engine.rs

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -434,7 +434,13 @@ impl Engine {
434434

435435
/// Add data document.
436436
///
437-
/// The specified data document is merged into existing data document.
437+
/// The specified data document is deep-merged into the existing data document. Nested
438+
/// objects are merged recursively (matching OPA's data-document merge), so adding
439+
/// `{ "a": { "x": 1 } }` and then `{ "a": { "y": 2 } }` yields `{ "a": { "x": 1, "y": 2 } }`.
440+
/// A conflict — the same path holding two different values — is an error.
441+
///
442+
/// The merge is atomic: if any conflict is detected (including one deep in a nested
443+
/// document), the call fails and the existing data document is left unchanged.
438444
///
439445
/// ```
440446
/// # use regorus::*;
@@ -453,9 +459,13 @@ impl Engine {
453459
/// // Merge { "z" : 3 }. Conflict error.
454460
/// assert!(engine.add_data(Value::from_json_str(r#"{ "z" : 3 }"#)?).is_err());
455461
///
462+
/// // Nested objects are deep-merged. Merge { "y" : { "a" : 10 } } then { "y" : { "b" : 20 } }.
463+
/// assert!(engine.add_data(Value::from_json_str(r#"{ "y" : { "a" : 10 } }"#)?).is_ok());
464+
/// assert!(engine.add_data(Value::from_json_str(r#"{ "y" : { "b" : 20 } }"#)?).is_ok());
465+
///
456466
/// assert_eq!(
457467
/// engine.eval_query("data".to_string(), false)?.result[0].expressions[0].value,
458-
/// Value::from_json_str(r#"{ "x": 1, "y": {}, "z": 2}"#)?
468+
/// Value::from_json_str(r#"{ "x": 1, "y": { "a": 10, "b": 20 }, "z": 2}"#)?
459469
/// );
460470
/// # Ok(())
461471
/// # }
@@ -464,8 +474,29 @@ impl Engine {
464474
if data.as_object().is_err() {
465475
bail!("data must be object");
466476
}
467-
self.prepared = false;
468-
self.interpreter.get_init_data_mut().merge(data)
477+
478+
// add_data is all-or-nothing; the atomic strategy differs by build because the failure
479+
// modes do: a conflict (same path, differing values) is possible everywhere, an
480+
// allocator-limit failure mid-merge only under `allocator-memory-limits`.
481+
#[cfg(not(feature = "allocator-memory-limits"))]
482+
{
483+
// Conflict is the only failure mode; `check_mergeable` catches it up front without
484+
// allocating, so validate then deep-merge in place (zero-copy fast path).
485+
self.interpreter.get_init_data().check_mergeable(&data)?;
486+
self.prepared = false;
487+
self.interpreter.get_init_data_mut().deep_merge(data)
488+
}
489+
#[cfg(feature = "allocator-memory-limits")]
490+
{
491+
// A limit failure can strike mid-merge and can't be predicted, so merge into a
492+
// candidate and commit only on success. `Value` is copy-on-write, so only touched
493+
// subtrees are cloned.
494+
let mut candidate = self.interpreter.get_init_data().clone();
495+
candidate.deep_merge(data)?;
496+
*self.interpreter.get_init_data_mut() = candidate;
497+
self.prepared = false;
498+
Ok(())
499+
}
469500
}
470501

471502
/// Get the data document.

src/interpreter.rs

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,17 @@ enum FunctionModifier {
6060
Value(Value),
6161
}
6262

63+
/// How [`Interpreter::update_data`] merges a rule's value into the data document.
64+
#[derive(Debug, Clone, Copy)]
65+
enum RuleValueMerge {
66+
/// Shallow-merge keeping disjoint keys, so rules sharing a path prefix scaffold into one
67+
/// object (`a.foo` + `a.bar` → one `a`) instead of conflicting.
68+
Combine,
69+
/// Complete-rule semantics: existing value must be absent or exactly equal, else conflict.
70+
/// Used for zero-arg function outputs (`f() := …`), which OPA treats like complete rules.
71+
Strict,
72+
}
73+
6374
type RuleValues = BTreeMap<Vec<Value>, (Value, Ref<Expr>)>;
6475

6576
#[derive(Debug)]
@@ -3408,6 +3419,23 @@ impl Interpreter {
34083419
}
34093420
}
34103421

3422+
/// Materialize a complete-rule value: the existing value must be absent or *exactly equal*
3423+
/// to `new`, else it is a conflict.
3424+
///
3425+
/// Unlike the shallow [`Self::merge_rule_value`], differing outputs conflict instead of
3426+
/// combining — `f() := {"a": 1}` and `f() := {"b": 2}` conflict — matching OPA's semantics
3427+
/// for zero-arg functions.
3428+
fn merge_rule_value_strict(span: &Span, value: &mut Value, new: Value) -> Result<()> {
3429+
if *value == Value::Undefined {
3430+
*value = new;
3431+
Ok(())
3432+
} else if *value == new {
3433+
Ok(())
3434+
} else {
3435+
Err(span.error("rules should not produce multiple outputs."))
3436+
}
3437+
}
3438+
34113439
pub fn get_path_string(refr: &Expr, document: Option<&str>) -> Result<String> {
34123440
let mut comps = vec![];
34133441
let mut expr_opt = Some(refr);
@@ -3663,14 +3691,18 @@ impl Interpreter {
36633691
_refr: &Expr,
36643692
path: &[&str],
36653693
value: Value,
3694+
merge: RuleValueMerge,
36663695
) -> Result<()> {
36673696
if value == Value::Undefined {
36683697
return Ok(());
36693698
}
36703699
// Ensure that path is created.
36713700
let vref = Self::make_or_get_value_mut(&mut self.data, path)?;
36723701
if Self::get_value_chained(self.init_data.clone(), path) == Value::Undefined {
3673-
Self::merge_rule_value(span, vref, value)
3702+
match merge {
3703+
RuleValueMerge::Strict => Self::merge_rule_value_strict(span, vref, value),
3704+
RuleValueMerge::Combine => Self::merge_rule_value(span, vref, value),
3705+
}
36743706
} else {
36753707
// Retain specified value.
36763708
Ok(())
@@ -3778,7 +3810,13 @@ impl Interpreter {
37783810
// `a` is created as an empty object.
37793811
if let Some((_, prefix)) = path.split_last() {
37803812
if !prefix.is_empty() {
3781-
self.update_data(span, refr, prefix, Value::new_object())?;
3813+
self.update_data(
3814+
span,
3815+
refr,
3816+
prefix,
3817+
Value::new_object(),
3818+
RuleValueMerge::Combine,
3819+
)?;
37823820
}
37833821
}
37843822

@@ -3790,7 +3828,13 @@ impl Interpreter {
37903828
};
37913829

37923830
let value = self.eval_rule_bodies(ctx, span, rule_body)?;
3793-
self.update_data(refr.span(), refr, &path[..], value)?;
3831+
self.update_data(
3832+
refr.span(),
3833+
refr,
3834+
&path[..],
3835+
value,
3836+
RuleValueMerge::Strict,
3837+
)?;
37943838
}
37953839
}
37963840
}
@@ -4037,6 +4081,7 @@ impl Interpreter {
40374081
rule_refr,
40384082
&prefix_path,
40394083
Value::new_object(),
4084+
RuleValueMerge::Combine,
40404085
)?;
40414086
}
40424087
}

0 commit comments

Comments
 (0)