feat: add json patch support - #442
Conversation
|
Mats Willemsen (@ma-ts) Thanks for the contribution. Do also add I happened to see that you have also created feature request in the Azure Policy repo. We are working on some interesting projects involving Azure Policy and Regorus. Hence, I'm curious whether you are already using Regorus. |
|
Hi Anand Krishnamoorthi (@anakrish), I will do that! And yes, all of this is part of a big policy overhaul in our organisation, and actually we are already using it in some services. I am soon going to be open sourcing a project as well to provide a Rust-based ingress controller that uses Regorus and integrates natively with OIDC, and has special support for Entra ID |
Anand Krishnamoorthi (anakrish)
left a comment
There was a problem hiding this comment.
https://docs.rs/serde_json/latest/serde_json/fn.from_value.html and https://docs.rs/serde_json/latest/serde_json/fn.to_value.html can be used to avoid explicit serialization to strings.
| ensure_args_count(span, name, params, args, 2)?; | ||
|
|
||
| let object_str = args[0].to_json_str()?; | ||
| let mut object: serde_json::Value = serde_json::from_str(&object_str) |
There was a problem hiding this comment.
| let mut object: serde_json::Value = serde_json::from_str(&object_str) | |
| let mut object: serde_json::Value = serde_json::to_value(&args[0]) |
| ensure_array(name, ¶ms[1], args[1].clone())?; | ||
|
|
||
| let patches_str = args[1].to_json_str()?; | ||
| let patches_json: serde_json::Value = serde_json::from_str(&patches_str) |
There was a problem hiding this comment.
| let patches_json: serde_json::Value = serde_json::from_str(&patches_str) | |
| let patches_json: serde_json::Value = serde_json::to_value(&args[1]) |
|
|
||
| match json_patch::patch(&mut object, &patch) { | ||
| Ok(_) => { | ||
| let result_str = serde_json::to_string(&object) |
There was a problem hiding this comment.
| let result_str = serde_json::to_string(&object) | |
| let value : regorus::Value = serde_json::from_value&object) |
|
Hey Mats Willemsen (@ma-ts), just a gentle ping on this. |
…ody fix (picks up #442) (#776) * feat: add json patch support Implements the json.patch builtin (RFC6902) via the json-patch crate, behind the optional jsonpatch feature. Rebased on top of current main. Fixes: #95 Originally: #442 * feat: implement json.patch natively (Rego set support, review fixes) Per anakrish's review on #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. * fix: run every body of a multi-body partial-object/set rule 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 (#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). * fix: address json.patch CI and review feedback * fix: align json.patch behavior with OPA * fix: match OPA set replace semantics --------- Co-authored-by: Mats Willemsen <mats.willemsen@ah.nl>
|
superceded by #776 |
Add json.patch builtin support
Fixes: #95
Implements the
json.patchbuiltin function according to the OPA specification, enabling RFC6902 JSON Patch operations on objects.Changes
json-patchcrate v4.0.0 with optionaljsonpatchfeaturejson.patch(object, patches)function that applies JSON Patch operations atomicallyFeatures
Usage