* 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 #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 #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 #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
#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-#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>
Partial object rules with dynamic keys (e.g. `violations[k] if { ... }`)
only produced a single entry instead of collecting all bindings. Two
independent bugs caused this:
1. Interpreter: the early-return optimization in eval_output_expr_in_loop
checked whether the rule_ref was constant but never verified whether
the key expression was also constant. A variable key like `k` was
treated as constant output, causing the loop to exit after the first
iteration. Fixed by gating early-return on key_expr constness.
2. RVM: compute_rule_type incorrectly classified `p[k] if { ... }` as
PartialSet instead of PartialObject. OPA v1 semantics define this
form as a partial object (key -> true). Fixed the classification and
added compiler error guards for patterns the RVM codegen cannot yet
handle (constant keys, nested bracket keys), ensuring graceful
fallback to the interpreter.
The OPA test harness now skips RVM validation per-case when partial
object compiler errors are raised, rather than blanket-skipping entire
folders. This preserves RVM coverage for unrelated tests in the same
folders.
Closes#712
Co-authored-by: Anand Krishnamoorthi <anakrish@microsoft.com>
Handle undefined values correctly in ordered-else. Previously an undefined value
in one of the blocks could cause the entire rule to evaluate to undefined.
Handle undefined values correctly in generic rule refs to prevent them from
propagating to output.
Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>
Previously we used to rely on whether there was a value in the
data document for a given rule path. This approach cannot handle
the case of evaluating a.b when a.b.c has been evaluated but
a.b.d has not been evaluated. Upon evaluating a.b.c, the data document
will already have a value of a.b even though a.b.d has not yet
been evaluated.
Hence we need to keep track of evaluated rules separately.
Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>
Mark Birger