feat: json.patch builtin with Rego set support + partial-rule multi-body fix (picks up #442) - #776
Conversation
Implements the json.patch builtin (RFC6902) via the json-patch crate, behind the optional jsonpatch feature. Rebased on top of current main. Fixes: microsoft#95 Originally: microsoft#442
Per anakrish's review on microsoft#442: Value already implements Serialize/ Deserialize directly, so round-tripping through to_json_str()/ from_str() was unnecessary double-serialization -- first pass fixed that (serde_json::to_value()/from_value() instead). Wiring v0/jsonpatch and v1/jsonpatch into tests/opa.passing (per the same review thread, so OPA's own jsonpatch suite runs in CI) surfaced a real gap: OPA's json.patch operates on the Rego value directly and special-cases Set (a set member is addressed by value, there is no JSON equivalent -- see OPA's internal/edittree). The json-patch crate only understands plain JSON, so every set-typed test case (add/ remove/move on a Rego set, e.g. {"a","b","c"}) silently degraded to array-index semantics and returned Undefined. This replaces the json-patch/jsonptr/thiserror dependency with a native implementation over regorus::Value, mirroring OPA's own semantics (github.com/open-policy-agent/opa v1/topdown/json.go + internal/edittree/edittree.go @ v1.2.0): - path parsing: leading '/' optional (OPA-specific relaxation), array-form paths carry raw (unescaped, non-string-only) segments - object: key lookup; array: numeric/'-'-append index; set: lookup and insert by value equality - add/remove/replace/move/copy/test composed from two primitives (functional insert/remove), same as OPA's EditTree-based apply - any patch-application failure yields Undefined unconditionally (matching builtinJSONPatch, which never hard-errors on a bad patch, independent of strict-builtin-errors) v1/jsonpatch: 7/7 OPA suite cases pass, now registered in tests/opa.passing. v0/jsonpatch: 6/7 -- the remaining failure (json_patch_tests, the OPA-authored batch-comparison rule) reproduces independent of json.patch: a v0-only bug where a partial-object rule with multiple bodies drops entries once more than one package contributes to the same iterated key (data.<pkg>[p]...), only visible at the corpus's scale. Minimal repro available on request. Left v0/jsonpatch out of opa.passing pending that separate fix.
@microsoft-github-policy-service agree company="Nitra" |
Investigating why v0/jsonpatch's OPA-authored batch test
(json_patch_tests) still failed after the previous commit surfaced a
second, unrelated interpreter bug: eval_rule_bodies broke out of its
body loop as soon as one body produced a value, so for a partial
(object/set) rule with multiple bodies -- e.g.
passed[k] = t {
t := items[k]
not t.err
} {
t := items[k]
t.err
}
-- once the first body matched *any* key, later bodies were never
even attempted, silently dropping every key only the later bodies
would have produced. Reproduces independent of v0 and of json.patch
(also breaks a v1 `else`-chained partial rule); minimal repro added
inline in the commit for reference, not as a test file since it
duplicates existing coverage patterns.
Fix: for partial rules only (ctx.is_set || ctx.key_expr.is_some()),
don't break after a successful body, and carry the accumulator
(Context::value / Context::rule_value) forward across bodies instead
of discarding it when moving to the next body -- both are required;
either alone still drops results. Complete rules and functions keep
the original first-body-wins (`else`) semantics unchanged.
Old-style stacked bodies with no `else` keyword reuse the rule head's
output expression, but `RuleBody::assign` is `None` for them (the
parser never populates it outside of an explicit `else = ...`
clause), so a body recovered by this fix that doesn't happen to be
the first can still bind the wrong output value (defaults to boolean
`true`). Threading the head's expression into those bodies turned out
to require reusing an `Expr` node across two `RuleBody`s, which trips
an `eidx`-uniqueness invariant elsewhere in the compiler (loop
hoisting table lookups are keyed by `eidx`) -- fixing that is a
separate, riskier change and is not needed for the json.patch
regression this was chasing (which only depends on *key presence*,
not the bound value). Left as a known follow-up.
RVM has an analogous, already-tracked gap for multi-body partial
object rules (microsoft#665); tests/opa.rs now skips the RVM cross-check for
jsonpatch/json_patch_tests specifically, same pattern already used
for other known RVM gaps in this file.
v0/jsonpatch: 7/7, now back in tests/opa.passing.
Full tests/opa.passing regression run: 2871/2875 (the 4 failures are
a pre-existing, unrelated gap -- `test.sleep` is not implemented;
confirmed via A/B against this same commit with this change reverted,
identical failures either way).
There was a problem hiding this comment.
Pull request overview
Adds first-class json.patch support aligned with OPA semantics (including Rego set behavior) and fixes an interpreter correctness bug where multi-body partial rules could drop keys. It also expands CI coverage by registering OPA’s jsonpatch compliance suites.
Changes:
- Implement
json.patchnatively overregorus::Value(feature-gated) and add an interpreter fixture for the builtin. - Fix interpreter
eval_rule_bodiesto accumulate results across bodies for partial (object/set) rules. - Register
v0/jsonpatchandv1/jsonpatchintests/opa.passingand skip RVM validation for the known multi-body partial-object gap.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/opa.rs | Skips RVM cross-check for OPA’s jsonpatch/json_patch_tests due to known RVM limitation. |
| tests/opa.passing | Enables OPA jsonpatch compliance suites in CI (v0/jsonpatch, v1/jsonpatch). |
| tests/interpreter/cases/builtins/objects/json.patch.yaml | Adds interpreter fixture coverage for json.patch (currently object/array-focused). |
| src/interpreter.rs | Fixes partial-rule multi-body evaluation to accumulate contributions across bodies. |
| src/builtins/objects.rs | Registers and implements feature-gated json.patch over Value with OPA-like path/collection semantics. |
| Cargo.toml | Introduces jsonpatch feature and includes it in full-opa. |
|
Vitalii Tverdokhlib (@vitaliytv) Thank you for taking this on. Can you address the test failure and see if the copilot review comments make sense? Overall, the PR looks good to me. |
Thank you! done |
Anand Krishnamoorthi (anakrish)
left a comment
There was a problem hiding this comment.
Automated review: inline findings from independent repository code-review and deep-review skill runs. Each issue was retained after an adversarial verification pass.
|
Vitalii Tverdokhlib (@vitaliytv) I did one more review focusing on corner cases. After those are addressed, it should be good to go. Thanks! |
Thank you! Done. |
Review summary — Approve (mergeable), with tracked follow-upsI reviewed this PR using the repo's Verification (local,
The I've filed the non-blocking follow-ups as #781 — the plan is to merge this PR and address them in a follow-up PR, which is a reasonable approach since none affects correctness of default evaluation. Findings (all tracked in #781)
Suggested before merge (optional, low effort)The two items most worth doing before RVM suspendable/serialized-program modes are relied upon are #1 (suspendable parity) and #2 ( Nice work — the native rewrite over |
|
Vitalii Tverdokhlib (@vitaliytv) thank you for your contribution! I will go ahead an merge this PR. I've created an issue to track some outstanding issues that the deep-review found. |
70e9b16
into
microsoft:main
Summary
Picks up #442 (
json.patchbuiltin) by Mats Willemsen (@ma-ts), which stalled on review feedback. Addresses both requests from Anand Krishnamoorthi (@anakrish)'s review:to_json_str()/from_str()—Valuealready implementsSerialize/Deserializedirectly.v0/jsonpatchandv1/jsonpatchintests/opa.passingso OPA's own jsonpatch compliance suite runs in CI.Doing (2) surfaced a real gap: OPA's
json.patchoperates on the Rego value directly and special-casesset(a set member is addressed by value — there's no JSON equivalent; see OPA'sinternal/edittree). Thejson-patchcrate only understands plain JSON, so every set-typed case (add/remove/moveon a Rego{"a","b","c"}) silently degraded to array-index semantics and returnedUndefinedinstead of the expected result.Given that, this PR replaces the
json-patch/jsonptr/thiserrordependency with a native implementation overregorus::Value, mirroring OPA's own semantics (github.com/open-policy-agent/opav1/topdown/json.go+internal/edittree/edittree.go@v1.2.0):/is optional (OPA-specific relaxation of RFC6901), array-form paths carry raw (unescaped, non-string-only) segments.object: key lookup.array: numeric index /"-"append.set: lookup and insert by value equality.add/remove/replace/move/copy/testare composed from two primitives (functional insert/remove) — same decomposition as OPA's EditTree-basedapplyPatches.Undefinedunconditionally, matchingbuiltinJSONPatch, which never hard-errors on a bad patch regardless ofstrict-builtin-errors.Second commit: a real (unrelated) interpreter bug found along the way
Wiring
v0/jsonpatchintoopa.passinginitially still failed on OPA's ownjson_patch_testsbatch-comparison rule, for a reason that had nothing to do withjson.patchitself. Minimal repro (no jsonpatch involved):Expected
{"a": {...}, "b": {...}}, got only{"b": {...}}.Root cause in
eval_rule_bodies(src/interpreter.rs): the multi-body loop breaks as soon as one body produces a value. That's correctelsesemantics for a complete rule/function (first matching body wins), but for a partial (object/set) rule, different bodies can legitimately contribute different keys — once body 1 matched something, body 2 (which owns a different key) was never even attempted, silently dropping it. Fix: for partial rules only (ctx.is_set || ctx.key_expr.is_some()), evaluate every body and carry the accumulator (Context::value/Context::rule_value) forward between them instead of discarding it — both are needed; either alone still loses results. Complete rules and functions keep the original first-body-wins behavior untouched.Known follow-up, out of scope here: for old-style stacked bodies with no
elsekeyword, a body recovered by this fix (that isn't the first) still binds the wrong output value when it doesn't explicitly restate the assignment, becauseRuleBody::assignisNonefor those and the parser never threads the rule head's output expression into them. I attempted to fix that at parse time (cloning the head'sRuleAssigninto each stacked body), but that reuses oneExprnode'seidxacross multipleRuleBodys and trips aneidx-uniqueness invariant used by the loop-hoisting table elsewhere in the compiler ("expression with eidx N already exists") — a materially bigger, riskier change. Not needed for the regression this PR chases, since the OPA batch-comparison rule only depends on key presence (not passed_cases[k]), not the bound value. Happy to open a separate issue/PR for it if useful.RVM has an analogous, already-tracked gap for multi-body partial-object rules (#665, still open) —
tests/opa.rsnow skips the RVM cross-check forjsonpatch/json_patch_testsspecifically, same pattern already used elsewhere in that file for other known RVM gaps.Test plan
cargo clippy --all-targets --all-features -- -D warnings(incl.ffi/java/python/wasm/rubybindings) — cleancargo fmt --check— cleantests/interpreter/cases/builtins/objects/json.patch.yaml, from feat: add json patch support #442, all 6 RFC6902 ops) — 128/128 passingcargo test --test opa -- v0/jsonpatch v1/jsonpatch): 7/7 both — both now registered intests/opa.passingtests/opa.passingregression run (everything the suite already covers, ~2875 cases across both commits) — 0 failuresAttribution
First commit is Mats Willemsen (@ma-ts)'s original patch, cherry-picked as-is (rebased onto current
main) to preserve authorship. Second and third commits are the rework and the interpreter fix described above.