From df4807e19e002bfbd8bb6ae0acd652e543a9dd8e Mon Sep 17 00:00:00 2001 From: Mats Willemsen Date: Sat, 26 Jul 2025 08:38:38 +0200 Subject: [PATCH 1/6] 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: https://github.com/microsoft/regorus/issues/95 Originally: https://github.com/microsoft/regorus/pull/442 --- Cargo.toml | 3 + src/builtins/objects.rs | 51 ++++- .../cases/builtins/objects/json.patch.yaml | 212 ++++++++++++++++++ 3 files changed, 265 insertions(+), 1 deletion(-) create mode 100644 tests/interpreter/cases/builtins/objects/json.patch.yaml diff --git a/Cargo.toml b/Cargo.toml index d43ff2c7c..e99cb946f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,6 +40,7 @@ http = [] glob = ["dep:globset"] graph = [] jsonschema = ["dep:jsonschema"] +jsonpatch = ["dep:json-patch"] mimalloc = ["dep:mimalloc"] net = ["dep:ipnet"] no_std = ["lazy_static/spin_no_std"] @@ -63,6 +64,7 @@ full-opa = [ "hex", "http", "jsonschema", + "jsonpatch", "net", "opa-runtime", "regex", @@ -121,6 +123,7 @@ semver = {version = "1.0.28", optional = true, default-features = false } url = { version = "2.5.4", optional = true } uuid = { version = "1.22.0", default-features = false, features = ["v4", "fast-rng"], optional = true } jsonschema = { version = "0.48.5", default-features = false, optional = true } +json-patch = { version = "4.0.0", default-features = false, optional = true } chrono = { version = "0.4.44", optional = true } chrono-tz = { version = "0.10.1", optional = true } ipnet = { version = "2.12.0", optional = true, default-features = false } diff --git a/src/builtins/objects.rs b/src/builtins/objects.rs index b5fac7bad..b6d950bfc 100644 --- a/src/builtins/objects.rs +++ b/src/builtins/objects.rs @@ -32,6 +32,11 @@ pub fn register(m: &mut builtins::BuiltinsMap<&'static str, builtins::BuiltinFcn m.insert("json.match_schema", (json_match_schema, 2)); m.insert("json.verify_schema", (json_verify_schema, 1)); } + + #[cfg(feature = "jsonpatch")] + { + m.insert("json.patch", (json_patch, 2)); + } } fn json_filter_impl(v: &Value, filter: &Value) -> Result { @@ -469,7 +474,6 @@ fn json_match_schema( let name = "json.match_schema"; ensure_args_count(span, name, params, args, 2)?; - // The following is expected to succeed. let document: serde_json::Value = serde_json::from_str(&args[0].to_json_str()?) .map_err(|err| span.error(&format!("Failed to parse JSON: {err}")))?; @@ -487,3 +491,48 @@ fn json_match_schema( .to_vec(), )) } + +#[cfg(feature = "jsonpatch")] +fn json_patch(span: &Span, params: &[Ref], args: &[Value], strict: bool) -> Result { + let name = "json.patch"; + 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) + .map_err(|err| span.error(&format!("Failed to parse object as JSON: {err}")))?; + + 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) + .map_err(|err| span.error(&format!("Failed to parse patches as JSON: {err}")))?; + + let patch: json_patch::Patch = serde_json::from_value(patches_json).map_err(|err| { + if strict { + params[1] + .span() + .error(&format!("Invalid patch format: {err}")) + } else { + span.error(&format!("Invalid patch format: {err}")) + } + })?; + + match json_patch::patch(&mut object, &patch) { + Ok(_) => { + let result_str = serde_json::to_string(&object) + .map_err(|err| span.error(&format!("Failed to serialize patched object: {err}")))?; + Value::from_json_str(&result_str).map_err(|err| { + span.error(&format!( + "Failed to convert patched object back to Value: {err}" + )) + }) + } + Err(err) => { + if strict { + bail!(span.error(&format!("Failed to apply patch: {err}"))); + } else { + Ok(Value::Undefined) + } + } + } +} diff --git a/tests/interpreter/cases/builtins/objects/json.patch.yaml b/tests/interpreter/cases/builtins/objects/json.patch.yaml new file mode 100644 index 000000000..59097f48f --- /dev/null +++ b/tests/interpreter/cases/builtins/objects/json.patch.yaml @@ -0,0 +1,212 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +cases: + - note: basic add operation + data: {} + modules: + - | + package test + + obj = {"a": {"foo": 1}} + patches = [{"op": "add", "path": "/a/bar", "value": 2}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + a: + foo: 1 + bar: 2 + + - note: basic remove operation + data: {} + modules: + - | + package test + + obj = {"a": {"foo": 1, "bar": 2}} + patches = [{"op": "remove", "path": "/a/bar"}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + a: + foo: 1 + + - note: basic replace operation + data: {} + modules: + - | + package test + + obj = {"a": {"foo": 1}} + patches = [{"op": "replace", "path": "/a/foo", "value": 42}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + a: + foo: 42 + + - note: basic move operation + data: {} + modules: + - | + package test + + obj = {"a": {"foo": 1}, "b": {}} + patches = [{"op": "move", "from": "/a/foo", "path": "/b/foo"}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + a: {} + b: + foo: 1 + + - note: basic copy operation + data: {} + modules: + - | + package test + + obj = {"a": {"foo": 1}, "b": {}} + patches = [{"op": "copy", "from": "/a/foo", "path": "/b/foo"}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + a: + foo: 1 + b: + foo: 1 + + - note: basic test operation (successful) + data: {} + modules: + - | + package test + + obj = {"a": {"foo": 1}} + patches = [{"op": "test", "path": "/a/foo", "value": 1}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + a: + foo: 1 + + - note: multiple operations + data: {} + modules: + - | + package test + + obj = {"a": {"foo": 1}} + patches = [ + {"op": "add", "path": "/a/bar", "value": 2}, + {"op": "replace", "path": "/a/foo", "value": 42} + ] + result = json.patch(obj, patches) + query: data.test.result + want_result: + a: + foo: 42 + bar: 2 + + - note: array operations - add to array + data: {} + modules: + - | + package test + + obj = {"arr": [1, 2, 3]} + patches = [{"op": "add", "path": "/arr/1", "value": "inserted"}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + arr: [1, "inserted", 2, 3] + + - note: array operations - add to end of array + data: {} + modules: + - | + package test + + obj = {"arr": [1, 2, 3]} + patches = [{"op": "add", "path": "/arr/-", "value": 4}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + arr: [1, 2, 3, 4] + + - note: array operations - remove from array + data: {} + modules: + - | + package test + + obj = {"arr": [1, 2, 3]} + patches = [{"op": "remove", "path": "/arr/1"}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + arr: [1, 3] + + - note: nested object operations + data: {} + modules: + - | + package test + + obj = {"a": {"b": {"c": {"d": 1}}}} + patches = [{"op": "add", "path": "/a/b/c/e", "value": 2}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + a: + b: + c: + d: 1 + e: 2 + + - note: special characters in keys + data: {} + modules: + - | + package test + + obj = {"foo/bar": {"baz~": 1}} + patches = [{"op": "add", "path": "/foo~1bar/baz~0test", "value": 2}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + "foo/bar": + "baz~": 1 + "baz~test": 2 + + - note: empty object and array handling + data: {} + modules: + - | + package test + + obj = {} + patches = [{"op": "add", "path": "/newkey", "value": {"nested": []}}] + result = json.patch(obj, patches) + query: data.test.result + want_result: + newkey: + nested: [] + + - note: complex value types + data: {} + modules: + - | + package test + + obj = {"data": null} + patches = [ + {"op": "replace", "path": "/data", "value": {"bool": true, "num": 3.14, "str": "hello"}} + ] + result = json.patch(obj, patches) + query: data.test.result + want_result: + data: + bool: true + num: 3.14 + str: "hello" From a09a3e0cb710b5cb43d0fec565a06e4ab2156404 Mon Sep 17 00:00:00 2001 From: vitaliytv Date: Fri, 31 Jul 2026 21:42:12 +0300 Subject: [PATCH 2/6] 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.[p]...), only visible at the corpus's scale. Minimal repro available on request. Left v0/jsonpatch out of opa.passing pending that separate fix. --- Cargo.toml | 3 +- src/builtins/objects.rs | 332 ++++++++++++++++++++++++++++++++++++---- tests/opa.passing | 1 + 3 files changed, 302 insertions(+), 34 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index e99cb946f..fac58c3eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,7 +40,7 @@ http = [] glob = ["dep:globset"] graph = [] jsonschema = ["dep:jsonschema"] -jsonpatch = ["dep:json-patch"] +jsonpatch = [] mimalloc = ["dep:mimalloc"] net = ["dep:ipnet"] no_std = ["lazy_static/spin_no_std"] @@ -123,7 +123,6 @@ semver = {version = "1.0.28", optional = true, default-features = false } url = { version = "2.5.4", optional = true } uuid = { version = "1.22.0", default-features = false, features = ["v4", "fast-rng"], optional = true } jsonschema = { version = "0.48.5", default-features = false, optional = true } -json-patch = { version = "4.0.0", default-features = false, optional = true } chrono = { version = "0.4.44", optional = true } chrono-tz = { version = "0.10.1", optional = true } ipnet = { version = "2.12.0", optional = true, default-features = false } diff --git a/src/builtins/objects.rs b/src/builtins/objects.rs index b6d950bfc..0f0fc78f0 100644 --- a/src/builtins/objects.rs +++ b/src/builtins/objects.rs @@ -492,47 +492,315 @@ fn json_match_schema( )) } +// `json.patch` implements RFC6902 JSON Patch, extended (matching OPA's own +// behavior) to operate on Rego `object`/`array`/`set` values directly rather +// than on plain JSON. A generic serde-based JSON-Patch crate cannot express +// this: sets have no JSON equivalent (a set member is addressed *by value*, +// not by key/index), so patching has to know about `Value::Set` explicitly. +// The traversal/mutation rules below mirror OPA's `internal/edittree` +// (https://github.com/open-policy-agent/opa/blob/v1.2.0/internal/edittree/edittree.go): +// object -> key lookup, array -> index (numbers, numeric strings, or "-" for +// append), set -> membership lookup by value equality. + #[cfg(feature = "jsonpatch")] -fn json_patch(span: &Span, params: &[Ref], args: &[Value], strict: bool) -> Result { - let name = "json.patch"; - ensure_args_count(span, name, params, args, 2)?; +fn json_patch_parse_path(path: &Value) -> core::result::Result, String> { + match path { + // Per OPA: leading '/' is optional and stripped before splitting, so + // "/a/b" and "a/b" are equivalent. RFC6901 '~1'/'~0' escapes are + // unescaped in that order (must unescape ~1 before ~0). + Value::String(s) => { + if s.is_empty() { + return Ok(Vec::new()); + } + Ok(s.trim_start_matches('/') + .split('/') + .map(|part| Value::from(part.replace("~1", "/").replace("~0", "~"))) + .collect()) + } + // Array-form paths carry raw, unescaped segments (can be any Value, + // not just strings) -- used to address non-string set members. + Value::Array(items) => Ok(items.iter().cloned().collect()), + _ => Err("path must be a string or an array of path segments".into()), + } +} - let object_str = args[0].to_json_str()?; - let mut object: serde_json::Value = serde_json::from_str(&object_str) - .map_err(|err| span.error(&format!("Failed to parse object as JSON: {err}")))?; +/// Resolves a path segment to an array index. `append_ok` allows the index to +/// equal `len` (i.e. one-past-the-end, including `"-"`) -- only valid for the +/// final segment of an `add`/`insert`; every other use requires `idx < len`. +#[cfg(feature = "jsonpatch")] +fn json_patch_to_index( + len: usize, + seg: &Value, + append_ok: bool, +) -> core::result::Result { + let raw: i64 = match seg { + Value::Number(n) => n + .as_i64() + .ok_or_else(|| "invalid number type for indexing".to_string())?, + Value::String(s) if s.as_ref() == "-" => { + if !append_ok { + return Err("'-' index is not valid here".into()); + } + i64::try_from(len).map_err(|_| "array too large to index".to_string())? + } + Value::String(s) => { + if s.as_ref() != "0" && s.starts_with('0') { + return Err("leading zeros are not allowed in JSON paths".into()); + } + s.parse::() + .map_err(|_| "invalid string for indexing".to_string())? + } + _ => return Err("invalid type for indexing".into()), + }; + let idx = usize::try_from(raw).map_err(|_| format!("negative index: {raw}"))?; + let in_bounds = if append_ok { idx <= len } else { idx < len }; + if !in_bounds { + return Err(format!("index {idx} out of bounds for length {len}")); + } + Ok(idx) +} - ensure_array(name, ¶ms[1], args[1].clone())?; +/// Read-only path traversal (used for `from`/`test`). +#[cfg(feature = "jsonpatch")] +fn json_patch_get<'v>( + target: &'v Value, + path: &[Value], +) -> core::result::Result<&'v Value, String> { + let Some((head, rest)) = path.split_first() else { + return Ok(target); + }; + match target { + Value::Object(obj) => obj + .get(head) + .ok_or_else(|| format!("path {head} does not exist in object")) + .and_then(|child| json_patch_get(child, rest)), + Value::Array(arr) => { + let idx = json_patch_to_index(arr.len(), head, false)?; + json_patch_get(&arr[idx], rest) + } + Value::Set(set) => set + .get(head) + .ok_or_else(|| format!("path {head} does not exist in set")) + .and_then(|member| json_patch_get(member, rest)), + _ => Err(format!("expected composite type, found value: {target}")), + } +} - let patches_str = args[1].to_json_str()?; - let patches_json: serde_json::Value = serde_json::from_str(&patches_str) - .map_err(|err| span.error(&format!("Failed to parse patches as JSON: {err}")))?; +/// Functional insert: rebuilds the path from `target` down with `value` +/// placed at `path` (last segment inserted/overwritten; intermediate +/// segments must already exist). +#[cfg(feature = "jsonpatch")] +fn json_patch_insert( + target: &Value, + path: &[Value], + value: Value, +) -> core::result::Result { + let Some((head, rest)) = path.split_first() else { + return Ok(value); + }; + match target { + Value::Object(obj) => { + let mut new_obj = (**obj).clone(); + if rest.is_empty() { + new_obj.insert(head.clone(), value); + } else { + let child = obj + .get(head) + .ok_or_else(|| format!("path {head} does not exist in object"))?; + let new_child = json_patch_insert(child, rest, value)?; + new_obj.insert(head.clone(), new_child); + } + Ok(new_obj.into_value()) + } + Value::Array(arr) => { + if rest.is_empty() { + let idx = json_patch_to_index(arr.len(), head, true)?; + let mut new_arr = (**arr).clone(); + new_arr.insert(idx, value); + Ok(Value::from(new_arr)) + } else { + let idx = json_patch_to_index(arr.len(), head, false)?; + let new_child = json_patch_insert(&arr[idx], rest, value)?; + let mut new_arr = (**arr).clone(); + new_arr[idx] = new_child; + Ok(Value::from(new_arr)) + } + } + Value::Set(set) => { + if rest.is_empty() { + // Sets have no keys: the last path segment must equal the + // value being inserted (this is how OPA addresses set + // membership for `add`). + if head != &value { + return Err(format!( + "set key {head} does not equal value to be inserted {value}" + )); + } + let mut new_set = (**set).clone(); + new_set.insert(value); + Ok(Value::from(new_set)) + } else { + let member = set + .get(head) + .ok_or_else(|| format!("path {head} does not exist in set"))?; + let new_member = json_patch_insert(member, rest, value)?; + let mut new_set = (**set).clone(); + new_set.remove(head); + new_set.insert(new_member); + Ok(Value::from(new_set)) + } + } + _ => Err(format!("expected composite type, found value: {target}")), + } +} - let patch: json_patch::Patch = serde_json::from_value(patches_json).map_err(|err| { - if strict { - params[1] - .span() - .error(&format!("Invalid patch format: {err}")) - } else { - span.error(&format!("Invalid patch format: {err}")) +/// Functional remove: rebuilds the path from `target` down with the node at +/// `path` removed. Returns the rebuilt value and the value that was removed. +#[cfg(feature = "jsonpatch")] +fn json_patch_remove( + target: &Value, + path: &[Value], +) -> core::result::Result<(Value, Value), String> { + let Some((head, rest)) = path.split_first() else { + // Removing the root document itself is valid (OPA's EditTree just + // marks the node deleted). The placeholder new-document value is + // only ever observed by a following `add`/`insert` at the same + // (empty) path, which overwrites it outright -- see `replace`. + return Ok((Value::Null, target.clone())); + }; + match target { + Value::Object(obj) => { + if rest.is_empty() { + let mut new_obj = (**obj).clone(); + let removed = new_obj + .remove(head) + .ok_or_else(|| format!("path {head} does not exist in object"))?; + Ok((new_obj.into_value(), removed)) + } else { + let child = obj + .get(head) + .ok_or_else(|| format!("path {head} does not exist in object"))?; + let (new_child, removed) = json_patch_remove(child, rest)?; + let mut new_obj = (**obj).clone(); + new_obj.insert(head.clone(), new_child); + Ok((new_obj.into_value(), removed)) + } } - })?; - - match json_patch::patch(&mut object, &patch) { - Ok(_) => { - let result_str = serde_json::to_string(&object) - .map_err(|err| span.error(&format!("Failed to serialize patched object: {err}")))?; - Value::from_json_str(&result_str).map_err(|err| { - span.error(&format!( - "Failed to convert patched object back to Value: {err}" - )) - }) + Value::Array(arr) => { + let idx = json_patch_to_index(arr.len(), head, false)?; + if rest.is_empty() { + let mut new_arr = (**arr).clone(); + let removed = new_arr.remove(idx); + Ok((Value::from(new_arr), removed)) + } else { + let (new_child, removed) = json_patch_remove(&arr[idx], rest)?; + let mut new_arr = (**arr).clone(); + new_arr[idx] = new_child; + Ok((Value::from(new_arr), removed)) + } } - Err(err) => { - if strict { - bail!(span.error(&format!("Failed to apply patch: {err}"))); + Value::Set(set) => { + let member = set + .get(head) + .ok_or_else(|| format!("path {head} does not exist in set"))? + .clone(); + if rest.is_empty() { + let mut new_set = (**set).clone(); + new_set.remove(head); + Ok((Value::from(new_set), member)) } else { - Ok(Value::Undefined) + let (new_member, removed) = json_patch_remove(&member, rest)?; + let mut new_set = (**set).clone(); + new_set.remove(head); + new_set.insert(new_member); + Ok((Value::from(new_set), removed)) } } + _ => Err(format!("expected composite type, found value: {target}")), + } +} + +#[cfg(feature = "jsonpatch")] +fn json_patch_apply(target: &Value, ops: &[Value]) -> core::result::Result { + let mut current = target.clone(); + for op_value in ops { + let obj = match op_value { + Value::Object(o) => o, + _ => return Err( + "must be an array of JSON-Patch objects, but at least one element is not an object" + .into(), + ), + }; + + let get_field = |name: &str| -> core::result::Result<&Value, String> { + obj.get(&Value::from(name)) + .ok_or_else(|| format!("missing '{name}' attribute")) + }; + + let op = match get_field("op")? { + Value::String(s) => s.as_ref(), + _ => return Err("attribute 'op' must be a string".into()), + }; + + match op { + "add" => { + let path = json_patch_parse_path(get_field("path")?)?; + let value = get_field("value")?.clone(); + current = json_patch_insert(¤t, &path, value)?; + } + "remove" => { + let path = json_patch_parse_path(get_field("path")?)?; + let (new_current, _) = json_patch_remove(¤t, &path)?; + current = new_current; + } + "replace" => { + let path = json_patch_parse_path(get_field("path")?)?; + let value = get_field("value")?.clone(); + let (new_current, _) = json_patch_remove(¤t, &path)?; + current = json_patch_insert(&new_current, &path, value)?; + } + "move" => { + let from = json_patch_parse_path(get_field("from")?)?; + let path = json_patch_parse_path(get_field("path")?)?; + let (new_current, chunk) = json_patch_remove(¤t, &from)?; + current = json_patch_insert(&new_current, &path, chunk)?; + } + "copy" => { + let from = json_patch_parse_path(get_field("from")?)?; + let path = json_patch_parse_path(get_field("path")?)?; + let chunk = json_patch_get(¤t, &from)?.clone(); + current = json_patch_insert(¤t, &path, chunk)?; + } + "test" => { + let path = json_patch_parse_path(get_field("path")?)?; + let value = get_field("value")?; + let chunk = json_patch_get(¤t, &path)?; + if chunk != value { + return Err(format!( + "value from patch != expected value.\n\nExpected: {value}\n\nFound: {chunk}" + )); + } + } + other => return Err(format!("unrecognized op '{other}'")), + } + } + Ok(current) +} + +// Note: matching OPA's own `builtinJSONPatch`, any failure while applying the +// patch (bad path, missing attribute, failed `test`, ...) yields Undefined +// rather than a hard error -- this builtin never errors on a malformed patch, +// regardless of the `strict-builtin-errors` setting. +#[cfg(feature = "jsonpatch")] +fn json_patch(span: &Span, params: &[Ref], args: &[Value], _strict: bool) -> Result { + let name = "json.patch"; + ensure_args_count(span, name, params, args, 2)?; + ensure_array(name, ¶ms[1], args[1].clone())?; + + let ops = args[1].as_array()?; + + match json_patch_apply(&args[0], ops) { + Ok(patched) => Ok(patched), + Err(_) => Ok(Value::Undefined), } } diff --git a/tests/opa.passing b/tests/opa.passing index 439bef129..3f25c8f15 100644 --- a/tests/opa.passing +++ b/tests/opa.passing @@ -147,6 +147,7 @@ v1/intersection v1/jsonbuiltins v1/jsonfilter v1/jsonfilteridempotent +v1/jsonpatch v1/jsonremove v1/jsonremoveidempotent v1/jsonschema From f5ae80ca14ce4ab6326f4c07db5b18eb4fb46df4 Mon Sep 17 00:00:00 2001 From: vitaliytv Date: Sat, 1 Aug 2026 08:01:39 +0300 Subject: [PATCH 3/6] 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). --- src/interpreter.rs | 49 ++++++++++++++++++++++++++++++++++++++++------ tests/opa.passing | 1 + tests/opa.rs | 8 ++++++++ 3 files changed, 52 insertions(+), 6 deletions(-) diff --git a/src/interpreter.rs b/src/interpreter.rs index 36cc703ac..7f5daf053 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -3412,30 +3412,67 @@ impl Interpreter { ) -> Result { self.check_execution_time()?; let n_scopes = self.scopes.len(); + // Partial (object/set) rules: each body can contribute a *different* + // subset of keys/members (e.g. an `else`-chained or old-style + // multi-body rule where one body handles some keys and another body + // handles the rest). Every body must be tried and their + // contributions accumulated -- unlike complete rules/functions, + // where the first body to produce a value wins (`else` semantics) + // and later bodies must never run. + let is_partial = ctx.is_set || ctx.key_expr.is_some(); let result = if bodies.is_empty() { self.contexts.push(ctx.clone()); self.eval_output_expr() } else { let mut result = Ok(true); + let mut any_success = false; for (idx, body) in bodies.iter().enumerate() { if idx == 0 { self.contexts.push(ctx.clone()); } else { - self.contexts.pop(); + let popped = self + .contexts + .pop() + .ok_or_else(|| anyhow!("internal error: rule's context already popped"))?; + // Old-style stacked bodies (no `else`) carry the rule + // head's assign as their own `.assign` (see + // `Parser::parse_query_blocks`) precisely so this reuses + // the same output expression here; a bare `else`/ + // `else if` genuinely has no `.assign` and must default + // to the boolean-true output below, not the head's. let output_expr = body.assign.as_ref().map(|e| e.value.clone()); - self.contexts.push(Context { + let mut next_ctx = Context { output_expr, // value: Value::new_array(), // ..Context::default() ..ctx.clone() - }); + }; + if is_partial { + // Carry the accumulator forward so this body adds to + // (rather than replaces) what earlier bodies + // produced. + next_ctx.rule_value = popped.rule_value; + next_ctx.value = popped.value; + } + self.contexts.push(next_ctx); } result = self.eval_query(&body.query); - if matches!(&result, Ok(true) | Err(_)) { - break; + match &result { + Ok(true) => { + any_success = true; + if !is_partial { + break; + } + } + Err(_) => break, + Ok(false) => {} } } - result + if is_partial { + result.map(|_| any_success) + } else { + result + } }; let popped_ctx = match self.contexts.pop() { diff --git a/tests/opa.passing b/tests/opa.passing index 3f25c8f15..b1f030da1 100644 --- a/tests/opa.passing +++ b/tests/opa.passing @@ -40,6 +40,7 @@ v0/intersection v0/jsonbuiltins v0/jsonfilter v0/jsonfilteridempotent +v0/jsonpatch v0/jsonremove v0/jsonremoveidempotent v0/jsonschema diff --git a/tests/opa.rs b/tests/opa.rs index 79be338b0..1dfaa1bd2 100644 --- a/tests/opa.rs +++ b/tests/opa.rs @@ -433,6 +433,14 @@ fn run_opa_tests(opa_tests_dir: String, folders: &[String]) -> Result<()> { case.note ); skip_rvm_validation = true; + } else if case.note == "jsonpatch/json_patch_tests" { + // Interpreter now correctly runs every body of a multi-body + // partial-object rule (`passed_cases[k] = t { .. } { .. }`), + // matching OPA. RVM's partial-object-rule codegen doesn't + // yet handle multiple bodies contributing distinct keys -- + // tracked separately in + // https://github.com/microsoft/regorus/issues/665. + skip_rvm_validation = true; } else if case.note == "reachable_paths/cycle_1022_3" { // The OPA behavior is not well-defined. // See: https://github.com/open-policy-agent/opa/issues/5871 From 1acaee824e1e96a90c7e754bfa7572fbb4d066ad Mon Sep 17 00:00:00 2001 From: vitaliytv Date: Wed, 5 Aug 2026 11:35:23 +0300 Subject: [PATCH 4/6] fix: address json.patch CI and review feedback --- src/builtins/objects.rs | 75 ++++++++++++------- src/interpreter.rs | 9 +-- src/tests/interpreter/mod.rs | 5 ++ .../cases/builtins/objects/json.patch.yaml | 36 +++++++++ 4 files changed, 92 insertions(+), 33 deletions(-) diff --git a/src/builtins/objects.rs b/src/builtins/objects.rs index 0f0fc78f0..ec2d15aa1 100644 --- a/src/builtins/objects.rs +++ b/src/builtins/objects.rs @@ -721,68 +721,79 @@ fn json_patch_remove( } #[cfg(feature = "jsonpatch")] -fn json_patch_apply(target: &Value, ops: &[Value]) -> core::result::Result { +fn json_patch_apply(target: &Value, ops: &[Value]) -> Result { let mut current = target.clone(); for op_value in ops { let obj = match op_value { Value::Object(o) => o, - _ => return Err( + _ => bail!( "must be an array of JSON-Patch objects, but at least one element is not an object" - .into(), ), }; - let get_field = |name: &str| -> core::result::Result<&Value, String> { + let get_field = |name: &str| -> Result<&Value> { obj.get(&Value::from(name)) - .ok_or_else(|| format!("missing '{name}' attribute")) + .ok_or_else(|| anyhow::anyhow!("missing '{name}' attribute")) }; let op = match get_field("op")? { Value::String(s) => s.as_ref(), - _ => return Err("attribute 'op' must be a string".into()), + _ => bail!("attribute 'op' must be a string"), }; match op { "add" => { - let path = json_patch_parse_path(get_field("path")?)?; + let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; let value = get_field("value")?.clone(); - current = json_patch_insert(¤t, &path, value)?; + current = json_patch_insert(¤t, &path, value).map_err(anyhow::Error::msg)?; } "remove" => { - let path = json_patch_parse_path(get_field("path")?)?; - let (new_current, _) = json_patch_remove(¤t, &path)?; + let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; + let (new_current, _) = + json_patch_remove(¤t, &path).map_err(anyhow::Error::msg)?; current = new_current; } "replace" => { - let path = json_patch_parse_path(get_field("path")?)?; + let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; let value = get_field("value")?.clone(); - let (new_current, _) = json_patch_remove(¤t, &path)?; - current = json_patch_insert(&new_current, &path, value)?; + let (new_current, _) = + json_patch_remove(¤t, &path).map_err(anyhow::Error::msg)?; + current = + json_patch_insert(&new_current, &path, value).map_err(anyhow::Error::msg)?; } "move" => { - let from = json_patch_parse_path(get_field("from")?)?; - let path = json_patch_parse_path(get_field("path")?)?; - let (new_current, chunk) = json_patch_remove(¤t, &from)?; - current = json_patch_insert(&new_current, &path, chunk)?; + let from = json_patch_parse_path(get_field("from")?).map_err(anyhow::Error::msg)?; + let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; + let (new_current, chunk) = + json_patch_remove(¤t, &from).map_err(anyhow::Error::msg)?; + current = + json_patch_insert(&new_current, &path, chunk).map_err(anyhow::Error::msg)?; } "copy" => { - let from = json_patch_parse_path(get_field("from")?)?; - let path = json_patch_parse_path(get_field("path")?)?; - let chunk = json_patch_get(¤t, &from)?.clone(); - current = json_patch_insert(¤t, &path, chunk)?; + let from = json_patch_parse_path(get_field("from")?).map_err(anyhow::Error::msg)?; + let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; + let chunk = json_patch_get(¤t, &from) + .map_err(anyhow::Error::msg)? + .clone(); + current = json_patch_insert(¤t, &path, chunk).map_err(anyhow::Error::msg)?; } "test" => { - let path = json_patch_parse_path(get_field("path")?)?; + let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; let value = get_field("value")?; - let chunk = json_patch_get(¤t, &path)?; + let chunk = json_patch_get(¤t, &path).map_err(anyhow::Error::msg)?; if chunk != value { - return Err(format!( + bail!( "value from patch != expected value.\n\nExpected: {value}\n\nFound: {chunk}" - )); + ); } } - other => return Err(format!("unrecognized op '{other}'")), + other => bail!("unrecognized op '{other}'"), } + + // Each operation may rebuild and grow a user-controlled value. Check + // while applying the patch so allocation limits cannot be deferred + // until the whole patch list has been processed. + enforce_limit()?; } Ok(current) } @@ -799,8 +810,18 @@ fn json_patch(span: &Span, params: &[Ref], args: &[Value], _strict: bool) let ops = args[1].as_array()?; - match json_patch_apply(&args[0], ops) { + let patched = json_patch_apply(&args[0], ops); + match patched { Ok(patched) => Ok(patched), + // Resource-limit errors must propagate rather than look like an + // invalid patch, so callers cannot bypass configured limits. + Err(err) + if err + .downcast_ref::() + .is_some() => + { + Err(err) + } Err(_) => Ok(Value::Undefined), } } diff --git a/src/interpreter.rs b/src/interpreter.rs index 7f5daf053..ba6503e6a 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -3434,12 +3434,9 @@ impl Interpreter { .contexts .pop() .ok_or_else(|| anyhow!("internal error: rule's context already popped"))?; - // Old-style stacked bodies (no `else`) carry the rule - // head's assign as their own `.assign` (see - // `Parser::parse_query_blocks`) precisely so this reuses - // the same output expression here; a bare `else`/ - // `else if` genuinely has no `.assign` and must default - // to the boolean-true output below, not the head's. + // Each body provides its own assignment. A bare `else`/ + // `else if` has no `.assign` and therefore defaults to + // the boolean-true output below. let output_expr = body.assign.as_ref().map(|e| e.value.clone()); let mut next_ctx = Context { output_expr, diff --git a/src/tests/interpreter/mod.rs b/src/tests/interpreter/mod.rs index 981fa10ab..26d4aaa3e 100644 --- a/src/tests/interpreter/mod.rs +++ b/src/tests/interpreter/mod.rs @@ -747,6 +747,11 @@ fn yaml_test(file: &str) -> Result<()> { return Ok(()); } + #[cfg(not(feature = "jsonpatch"))] + if file.contains("json.patch.yaml") { + return Ok(()); + } + // Targets are supported only with azure_policy feature. #[cfg(not(feature = "azure_policy"))] if file.contains("target") { diff --git a/tests/interpreter/cases/builtins/objects/json.patch.yaml b/tests/interpreter/cases/builtins/objects/json.patch.yaml index 59097f48f..bd8782fc3 100644 --- a/tests/interpreter/cases/builtins/objects/json.patch.yaml +++ b/tests/interpreter/cases/builtins/objects/json.patch.yaml @@ -210,3 +210,39 @@ cases: bool: true num: 3.14 str: "hello" + + - note: set add addresses the member by value with an array-form path + data: {} + modules: + - | + package test + + obj = {"a", "b", "c"} + patches = [{"op": "add", "path": ["d"], "value": "d"}] + result = json.patch(obj, patches) + query: data.test.result == {"a", "b", "c", "d"} + want_result: true + + - note: set remove addresses the member by value with an array-form path + data: {} + modules: + - | + package test + + obj = {"a", "b", "c"} + patches = [{"op": "remove", "path": ["b"]}] + result = json.patch(obj, patches) + query: data.test.result == {"a", "c"} + want_result: true + + - note: set move removes and reinserts a member by value with array-form paths + data: {} + modules: + - | + package test + + obj = {"a", "b", "c"} + patches = [{"op": "move", "from": ["a"], "path": ["a"]}] + result = json.patch(obj, patches) + query: data.test.result == {"a", "b", "c"} + want_result: true From d758d3cba7579ed603ce55449f26a99cc929b215 Mon Sep 17 00:00:00 2001 From: Vitalii Tverdokhlib Date: Fri, 7 Aug 2026 12:09:06 +0500 Subject: [PATCH 5/6] fix: align json.patch behavior with OPA --- Cargo.toml | 1 + README.md | 1 - bindings/wasm/Cargo.toml | 1 + bindings/wasm/test.js | 18 + docs/builtins.md | 1 + src/ast.rs | 3 + src/builtins/json_patch.rs | 542 ++++++++++++++++++ src/builtins/mod.rs | 2 + src/builtins/objects.rs | 308 +--------- src/interpreter.rs | 32 +- src/languages/rego/compiler/mod.rs | 3 + src/languages/rego/compiler/program.rs | 11 + src/languages/rego/compiler/rules.rs | 23 +- src/parser.rs | 6 + src/rvm/program/types.rs | 14 + src/rvm/tests/vm.rs | 4 + src/rvm/vm/rules.rs | 23 +- .../cases/builtins/objects/json.patch.yaml | 173 ++++++ tests/memory_limits.rs | 26 + tests/opa.rs | 8 - .../rvm/rego/cases/partial_object_rules.yaml | 59 ++ tests/rvm/rego/mod.rs | 2 + 22 files changed, 928 insertions(+), 333 deletions(-) create mode 100644 src/builtins/json_patch.rs diff --git a/Cargo.toml b/Cargo.toml index fac58c3eb..c8b432eb8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -88,6 +88,7 @@ opa-no-std = [ "coverage", "graph", "hex", + "jsonpatch", "no_std", "opa-runtime", "regex", diff --git a/README.md b/README.md index cc5030a1c..4105f0b73 100644 --- a/README.md +++ b/README.md @@ -351,7 +351,6 @@ The following test suites don't pass fully due to missing builtins: - `globsmatch` - `graphql` - `invalidkeyerror` -- `jsonpatch` - `jwtbuiltins` - `jwtdecodeverify` - `jwtencodesign` diff --git a/bindings/wasm/Cargo.toml b/bindings/wasm/Cargo.toml index 1b674d411..8d530976a 100644 --- a/bindings/wasm/Cargo.toml +++ b/bindings/wasm/Cargo.toml @@ -25,6 +25,7 @@ default = [ "regorus/hex", "regorus/http", "regorus/jsonschema", + "regorus/jsonpatch", "regorus/net", "regorus/opa-runtime", "regorus/regex", diff --git a/bindings/wasm/test.js b/bindings/wasm/test.js index 04d59bca0..cfb90c9b4 100644 --- a/bindings/wasm/test.js +++ b/bindings/wasm/test.js @@ -92,6 +92,24 @@ console.log(report); report = engine.getCoverageReportPretty(); console.log(report); +// json.patch is available through the WASM default feature set. +{ +const patchEngine = new regorus.Engine(); +patchEngine.addPolicy('json-patch.rego', ` +package wasm_patch +import rego.v1 + +result := json.patch( + {"a": [1, 2]}, + [{"op": "replace", "path": "/a/1", "value": 9}], +) +`); +const patched = JSON.parse(patchEngine.evalRule('data.wasm_patch.result')); +if (JSON.stringify(patched) !== JSON.stringify({a: [1, 9]})) { + throw new Error(`Unexpected json.patch WASM result: ${JSON.stringify(patched)}`); +} +} + // RVM regular example { const policy = ` diff --git a/docs/builtins.md b/docs/builtins.md index 9eafd6e00..dddf124b9 100644 --- a/docs/builtins.md +++ b/docs/builtins.md @@ -69,6 +69,7 @@ In future, each builtin will be associated with a feature (many builtins could b |----------------------------------------------------------------------------------------------------------------------|--------------| | [json.filter](https://www.openpolicyagent.org/docs/latest/policy-reference/#builtin-object-jsonfilter) | _ | | [json.match_schema](https://www.openpolicyagent.org/docs/latest/policy-reference/#builtin-object-jsonmatch_schema) | `jsonschema` | + | [json.patch](https://www.openpolicyagent.org/docs/latest/policy-reference/#builtin-object-jsonpatch) | `jsonpatch` | | [json.remove](https://www.openpolicyagent.org/docs/latest/policy-reference/#builtin-object-jsonremove) | _ | | [json.verify_schema](https://www.openpolicyagent.org/docs/latest/policy-reference/#builtin-object-jsonverify_schema) | `jsonschema` | | [object.filter](https://www.openpolicyagent.org/docs/latest/policy-reference/#builtin-object-objectfilter) | _ | diff --git a/src/ast.rs b/src/ast.rs index a86584fd2..5cd61a310 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -390,6 +390,9 @@ pub struct RuleAssign { pub struct RuleBody { pub span: Span, pub assign: Option, + /// True when this body was introduced by an `else` clause. Unlike + /// independent legacy query blocks, else bodies are mutually exclusive. + pub is_else: bool, pub query: Ref, } diff --git a/src/builtins/json_patch.rs b/src/builtins/json_patch.rs new file mode 100644 index 000000000..17d005a19 --- /dev/null +++ b/src/builtins/json_patch.rs @@ -0,0 +1,542 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! OPA-compatible edit tree used by `json.patch`. +//! +//! The design follows OPA's `internal/edittree`: patch operations update an +//! intermediate tree and the final `Value` is rendered once. Arrays use a +//! `VecDeque`, making repeated edits at either end cheap instead of cloning +//! and shifting the complete source array for every operation. + +#![allow(clippy::pattern_type_mismatch)] + +use super::utils::enforce_limit; +use crate::number::Number; +use crate::value::Object; +use crate::Value; +use alloc::collections::{BTreeMap, BTreeSet, VecDeque}; +use alloc::string::ToString as _; +use alloc::vec::Vec; +use anyhow::{anyhow, bail, Result}; + +#[derive(Debug)] +enum EditNode { + Scalar(Value), + Object(BTreeMap), + Array(VecDeque), + // Sets are content-addressed. The rendered member is kept as the key and + // refreshed whenever a nested edit changes that member. + Set(BTreeMap), +} + +impl EditNode { + fn from_value(value: &Value) -> Result { + enforce_limit()?; + Ok(match value { + Value::Object(object) => { + let mut fields = BTreeMap::new(); + for (key, value) in object.iter() { + fields.insert(key.clone(), Self::from_value(value)?); + enforce_limit()?; + } + Self::Object(fields) + } + Value::Array(array) => { + let mut items = VecDeque::with_capacity(array.len()); + for value in array.iter() { + items.push_back(Self::from_value(value)?); + enforce_limit()?; + } + Self::Array(items) + } + Value::Set(set) => { + let mut members = BTreeMap::new(); + for value in set.iter() { + members.insert(value.clone(), Self::from_value(value)?); + enforce_limit()?; + } + Self::Set(members) + } + scalar => Self::Scalar(scalar.clone()), + }) + } + + fn render(&self) -> Result { + enforce_limit()?; + Ok(match self { + Self::Scalar(value) => value.clone(), + Self::Object(fields) => { + let mut object = Object::new(); + for (key, value) in fields { + object.insert(key.clone(), value.render()?); + enforce_limit()?; + } + Value::Object(crate::Rc::new(object)) + } + Self::Array(items) => { + let mut array = Vec::with_capacity(items.len()); + for value in items { + array.push(value.render()?); + enforce_limit()?; + } + Value::Array(crate::Rc::new(array)) + } + Self::Set(members) => { + let mut set = BTreeSet::new(); + for value in members.values() { + set.insert(value.render()?); + enforce_limit()?; + } + Value::Set(crate::Rc::new(set)) + } + }) + } + + fn get(&self, path: &[Value]) -> Result<&Self> { + let Some((head, rest)) = path.split_first() else { + return Ok(self); + }; + match self { + Self::Object(fields) => fields + .get(head) + .ok_or_else(|| anyhow!("path {head} does not exist in object"))? + .get(rest), + Self::Array(items) => { + let index = array_index(items.len(), head, false)?; + items + .get(index) + .ok_or_else(|| anyhow!("array index disappeared"))? + .get(rest) + } + Self::Set(members) => members + .get(head) + .ok_or_else(|| anyhow!("path {head} does not exist in set"))? + .get(rest), + Self::Scalar(value) => bail!("expected composite type, found value: {value}"), + } + } + + fn insert(&mut self, path: &[Value], value: EditNode) -> Result<()> { + enforce_limit()?; + let Some((head, rest)) = path.split_first() else { + *self = value; + return Ok(()); + }; + match self { + Self::Object(fields) => { + if rest.is_empty() { + fields.insert(head.clone(), value); + } else { + fields + .get_mut(head) + .ok_or_else(|| anyhow!("path {head} does not exist in object"))? + .insert(rest, value)?; + } + } + Self::Array(items) => { + let index = array_index(items.len(), head, rest.is_empty())?; + if rest.is_empty() { + items.insert(index, value); + } else { + items + .get_mut(index) + .ok_or_else(|| anyhow!("array index disappeared"))? + .insert(rest, value)?; + } + } + Self::Set(members) => { + if rest.is_empty() { + let rendered = value.render()?; + if head != &rendered { + bail!("set key {head} does not equal value to be inserted {rendered}"); + } + members.insert(rendered, value); + } else { + let mut member = members + .remove(head) + .ok_or_else(|| anyhow!("path {head} does not exist in set"))?; + member.insert(rest, value)?; + let new_key = member.render()?; + members.insert(new_key, member); + } + } + Self::Scalar(current) => { + bail!("expected composite type, found value: {current}") + } + } + enforce_limit()?; + Ok(()) + } + + fn remove(&mut self, path: &[Value]) -> Result { + enforce_limit()?; + let (head, rest) = path + .split_first() + .ok_or_else(|| anyhow!("cannot remove node without a path"))?; + let removed = match self { + Self::Object(fields) => { + if rest.is_empty() { + fields + .remove(head) + .ok_or_else(|| anyhow!("path {head} does not exist in object"))? + } else { + fields + .get_mut(head) + .ok_or_else(|| anyhow!("path {head} does not exist in object"))? + .remove(rest)? + } + } + Self::Array(items) => { + let index = array_index(items.len(), head, false)?; + if rest.is_empty() { + items + .remove(index) + .ok_or_else(|| anyhow!("array index disappeared"))? + } else { + items + .get_mut(index) + .ok_or_else(|| anyhow!("array index disappeared"))? + .remove(rest)? + } + } + Self::Set(members) => { + if rest.is_empty() { + members + .remove(head) + .ok_or_else(|| anyhow!("path {head} does not exist in set"))? + } else { + let mut member = members + .remove(head) + .ok_or_else(|| anyhow!("path {head} does not exist in set"))?; + let removed = member.remove(rest)?; + let new_key = member.render()?; + members.insert(new_key, member); + removed + } + } + Self::Scalar(current) => { + bail!("expected composite type, found value: {current}") + } + }; + enforce_limit()?; + Ok(removed) + } + + fn replace(&mut self, path: &[Value], value: EditNode) -> Result<()> { + enforce_limit()?; + let Some((head, rest)) = path.split_first() else { + *self = value; + return Ok(()); + }; + match self { + Self::Object(fields) => { + if rest.is_empty() { + let slot = fields + .get_mut(head) + .ok_or_else(|| anyhow!("path {head} does not exist in object"))?; + *slot = value; + } else { + fields + .get_mut(head) + .ok_or_else(|| anyhow!("path {head} does not exist in object"))? + .replace(rest, value)?; + } + } + Self::Array(items) => { + let index = array_index(items.len(), head, false)?; + let slot = items + .get_mut(index) + .ok_or_else(|| anyhow!("array index disappeared"))?; + if rest.is_empty() { + *slot = value; + } else { + slot.replace(rest, value)?; + } + } + Self::Set(members) => { + let mut member = members + .remove(head) + .ok_or_else(|| anyhow!("path {head} does not exist in set"))?; + if rest.is_empty() { + member = value; + } else { + member.replace(rest, value)?; + } + let new_key = member.render()?; + members.insert(new_key, member); + } + Self::Scalar(current) => { + bail!("expected composite type, found value: {current}") + } + } + enforce_limit()?; + Ok(()) + } +} + +struct EditTree { + root: Option, +} + +impl EditTree { + fn new(value: &Value) -> Result { + Ok(Self { + root: Some(EditNode::from_value(value)?), + }) + } + + fn root(&self) -> Result<&EditNode> { + self.root + .as_ref() + .ok_or_else(|| anyhow!("path does not exist in deleted document")) + } + + fn root_mut(&mut self) -> Result<&mut EditNode> { + self.root + .as_mut() + .ok_or_else(|| anyhow!("path does not exist in deleted document")) + } + + fn insert_value(&mut self, path: &[Value], value: &Value) -> Result<()> { + let node = EditNode::from_value(value)?; + if path.is_empty() { + self.root = Some(node); + } else { + self.root_mut()?.insert(path, node)?; + } + Ok(()) + } + + fn insert_node(&mut self, path: &[Value], node: EditNode) -> Result<()> { + if path.is_empty() { + self.root = Some(node); + } else { + self.root_mut()?.insert(path, node)?; + } + Ok(()) + } + + fn remove(&mut self, path: &[Value]) -> Result { + if path.is_empty() { + self.root + .take() + .ok_or_else(|| anyhow!("root is already deleted")) + } else { + self.root_mut()?.remove(path) + } + } + + fn replace_value(&mut self, path: &[Value], value: &Value) -> Result<()> { + let node = EditNode::from_value(value)?; + if path.is_empty() { + self.root = Some(node); + } else { + self.root_mut()?.replace(path, node)?; + } + Ok(()) + } + + fn render(self) -> Result { + match self.root { + Some(root) => root.render(), + None => Ok(Value::Undefined), + } + } +} + +pub(super) fn apply(target: &Value, operations: &[Value]) -> Result { + let mut tree = EditTree::new(target)?; + for operation in operations { + enforce_limit()?; + let object = match operation { + Value::Object(object) => object, + _ => bail!( + "must be an array of JSON-Patch objects, but at least one element is not an object" + ), + }; + let field = |name: &str| -> Result<&Value> { + object + .get(&Value::from(name)) + .ok_or_else(|| anyhow!("missing '{name}' attribute")) + }; + let operation_name = match field("op")? { + Value::String(name) => name.as_ref(), + _ => bail!("attribute 'op' must be a string"), + }; + + match operation_name { + "add" => { + let path = parse_path(field("path")?)?; + tree.insert_value(&path, field("value")?)?; + } + "remove" => { + let path = parse_path(field("path")?)?; + tree.remove(&path)?; + } + "replace" => { + let path = parse_path(field("path")?)?; + tree.replace_value(&path, field("value")?)?; + } + "move" => { + let from = parse_path(field("from")?)?; + let path = parse_path(field("path")?)?; + let node = tree.remove(&from)?; + tree.insert_node(&path, node)?; + } + "copy" => { + let from = parse_path(field("from")?)?; + let path = parse_path(field("path")?)?; + let value = tree.root()?.get(&from)?.render()?; + tree.insert_value(&path, &value)?; + } + "test" => { + let path = parse_path(field("path")?)?; + let actual = tree.root()?.get(&path)?.render()?; + let expected = field("value")?; + if &actual != expected { + bail!( + "value from patch != expected value.\n\nExpected: {expected}\n\nFound: {actual}" + ); + } + } + other => bail!("unrecognized op '{other}'"), + } + } + tree.render() +} + +fn parse_path(path: &Value) -> Result> { + match path { + Value::String(path) if path.is_empty() => Ok(Vec::new()), + Value::String(path) => Ok(path + .trim_start_matches('/') + .split('/') + .map(|part| Value::from(part.replace("~1", "/").replace("~0", "~"))) + .collect()), + Value::Array(parts) => Ok(parts.iter().cloned().collect()), + _ => bail!("path must be a string or an array of path segments"), + } +} + +fn array_index(length: usize, segment: &Value, append_ok: bool) -> Result { + let raw = match segment { + Value::Number(Number::UInt(value)) => { + i64::try_from(*value).map_err(|_| anyhow!("array index is too large"))? + } + Value::Number(Number::Int(value)) => *value, + Value::Number(Number::BigInt(value)) => value + .to_string() + .parse::() + .map_err(|_| anyhow!("array index is too large"))?, + Value::Number(Number::Float(_)) => bail!("array index must be an integer"), + Value::String(value) if value.as_ref() == "-" => { + if !append_ok { + bail!("'-' index is not valid here"); + } + i64::try_from(length).map_err(|_| anyhow!("array too large to index"))? + } + Value::String(value) => { + if value.as_ref() != "0" && value.starts_with('0') { + bail!("leading zeros are not allowed in JSON paths"); + } + value + .parse::() + .map_err(|_| anyhow!("invalid string for indexing"))? + } + _ => bail!("invalid type for indexing"), + }; + let index = usize::try_from(raw).map_err(|_| anyhow!("negative index: {raw}"))?; + let in_bounds = if append_ok { + index <= length + } else { + index < length + }; + if !in_bounds { + bail!("index {index} out of bounds for length {length}"); + } + Ok(index) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn operation(json: &str) -> Value { + Value::from_json_str(json).expect("valid patch operation") + } + + #[test] + fn repeated_front_removals_do_not_rebuild_the_array() { + const LENGTH: usize = 4096; + const REMOVALS: usize = 2048; + + let target = Value::from_array((0..LENGTH).map(Value::from).collect()); + let remove_front = operation(r#"{"op":"remove","path":"/0"}"#); + let operations = alloc::vec![remove_front; REMOVALS]; + + let result = apply(&target, &operations).expect("patch must succeed"); + let array = result.as_array().expect("result must be an array"); + assert_eq!(array.len(), LENGTH - REMOVALS); + assert_eq!(array.first(), Some(&Value::from(REMOVALS))); + // The immutable input is not modified if applying a patch succeeds or + // fails; the edit tree owns all intermediate state. + assert_eq!( + target.as_array().expect("input must be an array").len(), + LENGTH + ); + } + + #[test] + fn failed_patch_does_not_modify_the_input() { + let target = Value::from_json_str(r#"{"a":[1,2,3]}"#).expect("valid target"); + let original = target.clone(); + let operations = [ + operation(r#"{"op":"remove","path":"/a/0"}"#), + operation(r#"{"op":"remove","path":"/missing"}"#), + ]; + + apply(&target, &operations).expect_err("patch must fail"); + assert_eq!(target, original); + } + + #[test] + fn move_uses_post_removal_array_indexes() { + let target = Value::from_json_str(r#"["a","b","c","d"]"#).expect("valid target"); + let operations = [operation(r#"{"op":"move","from":"/1","path":"/3"}"#)]; + let expected = Value::from_json_str(r#"["a","c","d","b"]"#).expect("valid expected value"); + + assert_eq!( + apply(&target, &operations).expect("patch must succeed"), + expected + ); + } + + #[test] + fn deep_paths_survive_edit_and_render() { + const DEPTH: usize = 128; + let key = Value::from("k"); + let mut target = Value::from(0); + for _ in 0..DEPTH { + let mut object = Object::new(); + object.insert(key.clone(), target); + target = Value::Object(crate::Rc::new(object)); + } + let path = alloc::vec![key; DEPTH]; + + let mut tree = EditTree::new(&target).expect("tree construction must succeed"); + tree.replace_value(&path, &Value::from(9)) + .expect("replace must succeed"); + let rendered = tree.render().expect("render must succeed"); + + let mut leaf = &rendered; + for segment in &path { + leaf = match leaf { + Value::Object(object) => { + object.get(segment).expect("deep path must survive render") + } + other => panic!("expected object on deep path, found {other}"), + }; + } + assert_eq!(leaf, &Value::from(9)); + } +} diff --git a/src/builtins/mod.rs b/src/builtins/mod.rs index 2df2e5bcb..ee6eecc8f 100644 --- a/src/builtins/mod.rs +++ b/src/builtins/mod.rs @@ -32,6 +32,8 @@ mod http; #[cfg(feature = "net")] mod net; +#[cfg(feature = "jsonpatch")] +mod json_patch; pub mod numbers; mod objects; #[cfg(feature = "opa-runtime")] diff --git a/src/builtins/objects.rs b/src/builtins/objects.rs index ec2d15aa1..a52884c23 100644 --- a/src/builtins/objects.rs +++ b/src/builtins/objects.rs @@ -492,312 +492,6 @@ fn json_match_schema( )) } -// `json.patch` implements RFC6902 JSON Patch, extended (matching OPA's own -// behavior) to operate on Rego `object`/`array`/`set` values directly rather -// than on plain JSON. A generic serde-based JSON-Patch crate cannot express -// this: sets have no JSON equivalent (a set member is addressed *by value*, -// not by key/index), so patching has to know about `Value::Set` explicitly. -// The traversal/mutation rules below mirror OPA's `internal/edittree` -// (https://github.com/open-policy-agent/opa/blob/v1.2.0/internal/edittree/edittree.go): -// object -> key lookup, array -> index (numbers, numeric strings, or "-" for -// append), set -> membership lookup by value equality. - -#[cfg(feature = "jsonpatch")] -fn json_patch_parse_path(path: &Value) -> core::result::Result, String> { - match path { - // Per OPA: leading '/' is optional and stripped before splitting, so - // "/a/b" and "a/b" are equivalent. RFC6901 '~1'/'~0' escapes are - // unescaped in that order (must unescape ~1 before ~0). - Value::String(s) => { - if s.is_empty() { - return Ok(Vec::new()); - } - Ok(s.trim_start_matches('/') - .split('/') - .map(|part| Value::from(part.replace("~1", "/").replace("~0", "~"))) - .collect()) - } - // Array-form paths carry raw, unescaped segments (can be any Value, - // not just strings) -- used to address non-string set members. - Value::Array(items) => Ok(items.iter().cloned().collect()), - _ => Err("path must be a string or an array of path segments".into()), - } -} - -/// Resolves a path segment to an array index. `append_ok` allows the index to -/// equal `len` (i.e. one-past-the-end, including `"-"`) -- only valid for the -/// final segment of an `add`/`insert`; every other use requires `idx < len`. -#[cfg(feature = "jsonpatch")] -fn json_patch_to_index( - len: usize, - seg: &Value, - append_ok: bool, -) -> core::result::Result { - let raw: i64 = match seg { - Value::Number(n) => n - .as_i64() - .ok_or_else(|| "invalid number type for indexing".to_string())?, - Value::String(s) if s.as_ref() == "-" => { - if !append_ok { - return Err("'-' index is not valid here".into()); - } - i64::try_from(len).map_err(|_| "array too large to index".to_string())? - } - Value::String(s) => { - if s.as_ref() != "0" && s.starts_with('0') { - return Err("leading zeros are not allowed in JSON paths".into()); - } - s.parse::() - .map_err(|_| "invalid string for indexing".to_string())? - } - _ => return Err("invalid type for indexing".into()), - }; - let idx = usize::try_from(raw).map_err(|_| format!("negative index: {raw}"))?; - let in_bounds = if append_ok { idx <= len } else { idx < len }; - if !in_bounds { - return Err(format!("index {idx} out of bounds for length {len}")); - } - Ok(idx) -} - -/// Read-only path traversal (used for `from`/`test`). -#[cfg(feature = "jsonpatch")] -fn json_patch_get<'v>( - target: &'v Value, - path: &[Value], -) -> core::result::Result<&'v Value, String> { - let Some((head, rest)) = path.split_first() else { - return Ok(target); - }; - match target { - Value::Object(obj) => obj - .get(head) - .ok_or_else(|| format!("path {head} does not exist in object")) - .and_then(|child| json_patch_get(child, rest)), - Value::Array(arr) => { - let idx = json_patch_to_index(arr.len(), head, false)?; - json_patch_get(&arr[idx], rest) - } - Value::Set(set) => set - .get(head) - .ok_or_else(|| format!("path {head} does not exist in set")) - .and_then(|member| json_patch_get(member, rest)), - _ => Err(format!("expected composite type, found value: {target}")), - } -} - -/// Functional insert: rebuilds the path from `target` down with `value` -/// placed at `path` (last segment inserted/overwritten; intermediate -/// segments must already exist). -#[cfg(feature = "jsonpatch")] -fn json_patch_insert( - target: &Value, - path: &[Value], - value: Value, -) -> core::result::Result { - let Some((head, rest)) = path.split_first() else { - return Ok(value); - }; - match target { - Value::Object(obj) => { - let mut new_obj = (**obj).clone(); - if rest.is_empty() { - new_obj.insert(head.clone(), value); - } else { - let child = obj - .get(head) - .ok_or_else(|| format!("path {head} does not exist in object"))?; - let new_child = json_patch_insert(child, rest, value)?; - new_obj.insert(head.clone(), new_child); - } - Ok(new_obj.into_value()) - } - Value::Array(arr) => { - if rest.is_empty() { - let idx = json_patch_to_index(arr.len(), head, true)?; - let mut new_arr = (**arr).clone(); - new_arr.insert(idx, value); - Ok(Value::from(new_arr)) - } else { - let idx = json_patch_to_index(arr.len(), head, false)?; - let new_child = json_patch_insert(&arr[idx], rest, value)?; - let mut new_arr = (**arr).clone(); - new_arr[idx] = new_child; - Ok(Value::from(new_arr)) - } - } - Value::Set(set) => { - if rest.is_empty() { - // Sets have no keys: the last path segment must equal the - // value being inserted (this is how OPA addresses set - // membership for `add`). - if head != &value { - return Err(format!( - "set key {head} does not equal value to be inserted {value}" - )); - } - let mut new_set = (**set).clone(); - new_set.insert(value); - Ok(Value::from(new_set)) - } else { - let member = set - .get(head) - .ok_or_else(|| format!("path {head} does not exist in set"))?; - let new_member = json_patch_insert(member, rest, value)?; - let mut new_set = (**set).clone(); - new_set.remove(head); - new_set.insert(new_member); - Ok(Value::from(new_set)) - } - } - _ => Err(format!("expected composite type, found value: {target}")), - } -} - -/// Functional remove: rebuilds the path from `target` down with the node at -/// `path` removed. Returns the rebuilt value and the value that was removed. -#[cfg(feature = "jsonpatch")] -fn json_patch_remove( - target: &Value, - path: &[Value], -) -> core::result::Result<(Value, Value), String> { - let Some((head, rest)) = path.split_first() else { - // Removing the root document itself is valid (OPA's EditTree just - // marks the node deleted). The placeholder new-document value is - // only ever observed by a following `add`/`insert` at the same - // (empty) path, which overwrites it outright -- see `replace`. - return Ok((Value::Null, target.clone())); - }; - match target { - Value::Object(obj) => { - if rest.is_empty() { - let mut new_obj = (**obj).clone(); - let removed = new_obj - .remove(head) - .ok_or_else(|| format!("path {head} does not exist in object"))?; - Ok((new_obj.into_value(), removed)) - } else { - let child = obj - .get(head) - .ok_or_else(|| format!("path {head} does not exist in object"))?; - let (new_child, removed) = json_patch_remove(child, rest)?; - let mut new_obj = (**obj).clone(); - new_obj.insert(head.clone(), new_child); - Ok((new_obj.into_value(), removed)) - } - } - Value::Array(arr) => { - let idx = json_patch_to_index(arr.len(), head, false)?; - if rest.is_empty() { - let mut new_arr = (**arr).clone(); - let removed = new_arr.remove(idx); - Ok((Value::from(new_arr), removed)) - } else { - let (new_child, removed) = json_patch_remove(&arr[idx], rest)?; - let mut new_arr = (**arr).clone(); - new_arr[idx] = new_child; - Ok((Value::from(new_arr), removed)) - } - } - Value::Set(set) => { - let member = set - .get(head) - .ok_or_else(|| format!("path {head} does not exist in set"))? - .clone(); - if rest.is_empty() { - let mut new_set = (**set).clone(); - new_set.remove(head); - Ok((Value::from(new_set), member)) - } else { - let (new_member, removed) = json_patch_remove(&member, rest)?; - let mut new_set = (**set).clone(); - new_set.remove(head); - new_set.insert(new_member); - Ok((Value::from(new_set), removed)) - } - } - _ => Err(format!("expected composite type, found value: {target}")), - } -} - -#[cfg(feature = "jsonpatch")] -fn json_patch_apply(target: &Value, ops: &[Value]) -> Result { - let mut current = target.clone(); - for op_value in ops { - let obj = match op_value { - Value::Object(o) => o, - _ => bail!( - "must be an array of JSON-Patch objects, but at least one element is not an object" - ), - }; - - let get_field = |name: &str| -> Result<&Value> { - obj.get(&Value::from(name)) - .ok_or_else(|| anyhow::anyhow!("missing '{name}' attribute")) - }; - - let op = match get_field("op")? { - Value::String(s) => s.as_ref(), - _ => bail!("attribute 'op' must be a string"), - }; - - match op { - "add" => { - let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; - let value = get_field("value")?.clone(); - current = json_patch_insert(¤t, &path, value).map_err(anyhow::Error::msg)?; - } - "remove" => { - let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; - let (new_current, _) = - json_patch_remove(¤t, &path).map_err(anyhow::Error::msg)?; - current = new_current; - } - "replace" => { - let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; - let value = get_field("value")?.clone(); - let (new_current, _) = - json_patch_remove(¤t, &path).map_err(anyhow::Error::msg)?; - current = - json_patch_insert(&new_current, &path, value).map_err(anyhow::Error::msg)?; - } - "move" => { - let from = json_patch_parse_path(get_field("from")?).map_err(anyhow::Error::msg)?; - let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; - let (new_current, chunk) = - json_patch_remove(¤t, &from).map_err(anyhow::Error::msg)?; - current = - json_patch_insert(&new_current, &path, chunk).map_err(anyhow::Error::msg)?; - } - "copy" => { - let from = json_patch_parse_path(get_field("from")?).map_err(anyhow::Error::msg)?; - let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; - let chunk = json_patch_get(¤t, &from) - .map_err(anyhow::Error::msg)? - .clone(); - current = json_patch_insert(¤t, &path, chunk).map_err(anyhow::Error::msg)?; - } - "test" => { - let path = json_patch_parse_path(get_field("path")?).map_err(anyhow::Error::msg)?; - let value = get_field("value")?; - let chunk = json_patch_get(¤t, &path).map_err(anyhow::Error::msg)?; - if chunk != value { - bail!( - "value from patch != expected value.\n\nExpected: {value}\n\nFound: {chunk}" - ); - } - } - other => bail!("unrecognized op '{other}'"), - } - - // Each operation may rebuild and grow a user-controlled value. Check - // while applying the patch so allocation limits cannot be deferred - // until the whole patch list has been processed. - enforce_limit()?; - } - Ok(current) -} - // Note: matching OPA's own `builtinJSONPatch`, any failure while applying the // patch (bad path, missing attribute, failed `test`, ...) yields Undefined // rather than a hard error -- this builtin never errors on a malformed patch, @@ -810,7 +504,7 @@ fn json_patch(span: &Span, params: &[Ref], args: &[Value], _strict: bool) let ops = args[1].as_array()?; - let patched = json_patch_apply(&args[0], ops); + let patched = super::json_patch::apply(&args[0], ops); match patched { Ok(patched) => Ok(patched), // Resource-limit errors must propagate rather than look like an diff --git a/src/interpreter.rs b/src/interpreter.rs index ba6503e6a..fc0294891 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -3412,13 +3412,9 @@ impl Interpreter { ) -> Result { self.check_execution_time()?; let n_scopes = self.scopes.len(); - // Partial (object/set) rules: each body can contribute a *different* - // subset of keys/members (e.g. an `else`-chained or old-style - // multi-body rule where one body handles some keys and another body - // handles the rest). Every body must be tried and their - // contributions accumulated -- unlike complete rules/functions, - // where the first body to produce a value wins (`else` semantics) - // and later bodies must never run. + // Independent bodies of a partial (object/set) rule each contribute + // keys/members. Bodies introduced by `else`, however, are mutually + // exclusive even for partial rules and must short-circuit. let is_partial = ctx.is_set || ctx.key_expr.is_some(); let result = if bodies.is_empty() { self.contexts.push(ctx.clone()); @@ -3434,10 +3430,18 @@ impl Interpreter { .contexts .pop() .ok_or_else(|| anyhow!("internal error: rule's context already popped"))?; - // Each body provides its own assignment. A bare `else`/ - // `else if` has no `.assign` and therefore defaults to - // the boolean-true output below. - let output_expr = body.assign.as_ref().map(|e| e.value.clone()); + // An assignment on an else body overrides the rule-head + // value. Without one, OPA retains the head assignment. + let output_expr = body.assign.as_ref().map(|e| e.value.clone()).or_else(|| { + // Complete-rule `else if { ... }` has an implicit + // boolean result. Partial rules retain their head + // value so every independent body contributes it. + if is_partial { + ctx.output_expr.clone() + } else { + None + } + }); let mut next_ctx = Context { output_expr, // value: Value::new_array(), @@ -3457,7 +3461,11 @@ impl Interpreter { match &result { Ok(true) => { any_success = true; - if !is_partial { + let next_is_else = idx + .checked_add(1) + .and_then(|next_idx| bodies.get(next_idx)) + .is_some_and(|next| next.is_else); + if !is_partial || next_is_else { break; } } diff --git a/src/languages/rego/compiler/mod.rs b/src/languages/rego/compiler/mod.rs index 3dbca039b..8817d80d6 100644 --- a/src/languages/rego/compiler/mod.rs +++ b/src/languages/rego/compiler/mod.rs @@ -123,6 +123,8 @@ pub struct Compiler<'a> { rule_definitions: Vec>>, rule_definition_function_params: Vec>>>, rule_definition_destructuring_patterns: Vec>>, + /// Per-rule, per-definition marker for bodies introduced by `else`. + rule_definition_else_bodies: Vec>>, /// Per-rule, per-definition: the static value produced by this definition, /// or `None` if the value is dynamic or differs across else-branches. /// Used to compute `RuleInfo::early_exit_on_first_success`. @@ -164,6 +166,7 @@ impl<'a> Compiler<'a> { rule_definitions: Vec::new(), rule_definition_function_params: Vec::new(), rule_definition_destructuring_patterns: Vec::new(), + rule_definition_else_bodies: Vec::new(), rule_definition_static_values: Vec::new(), rule_types: Vec::new(), rule_function_param_count: Vec::new(), diff --git a/src/languages/rego/compiler/program.rs b/src/languages/rego/compiler/program.rs index ff39dce26..cfb0853b5 100644 --- a/src/languages/rego/compiler/program.rs +++ b/src/languages/rego/compiler/program.rs @@ -99,6 +99,17 @@ impl<'a> Compiler<'a> { }; rule_info.destructuring_blocks = destructuring_blocks; + rule_info.else_bodies = self + .rule_definition_else_bodies + .get(rule_index as usize) + .cloned() + .unwrap_or_else(|| { + rule_info + .definitions + .iter() + .map(|b| vec![false; b.len()]) + .collect() + }); // Compute early_exit_on_first_success: if every definition has // the same static value, the VM can stop after the first success. diff --git a/src/languages/rego/compiler/rules.rs b/src/languages/rego/compiler/rules.rs index 2619f8cdf..e18f4b720 100644 --- a/src/languages/rego/compiler/rules.rs +++ b/src/languages/rego/compiler/rules.rs @@ -386,6 +386,10 @@ impl<'a> Compiler<'a> { self.rule_definition_destructuring_patterns.push(Vec::new()); } + while self.rule_definition_else_bodies.len() <= rule_index as usize { + self.rule_definition_else_bodies.push(Vec::new()); + } + while self.rule_definition_static_values.len() <= rule_index as usize { self.rule_definition_static_values.push(Vec::new()); } @@ -528,6 +532,7 @@ impl<'a> Compiler<'a> { }; self.push_context(context); let mut body_entry_points = Vec::new(); + let mut body_is_else = Vec::new(); if bodies.is_empty() { let value_expr_opt = self.context_stack.last().unwrap().value_expr.clone(); @@ -558,6 +563,7 @@ impl<'a> Compiler<'a> { let body_entry_point = self.program.instructions.len() as u32; body_entry_points.push(body_entry_point); + body_is_else.push(body.is_else); ::core::convert::identity(body_idx); @@ -567,7 +573,13 @@ impl<'a> Compiler<'a> { .and_then(|ctx| ctx.value_expr.clone()); let mut body_value_expr = body.assign.as_ref().map(|assign| assign.value.clone()); - if body_value_expr.is_none() && body_idx == 0 { + if body_value_expr.is_none() + && (body_idx == 0 + || matches!( + rule_type, + RuleType::PartialSet | RuleType::PartialObject + )) + { body_value_expr = previous_value_expr.clone(); } @@ -627,7 +639,13 @@ impl<'a> Compiler<'a> { for (bi, b) in bodies.iter().enumerate() { let mut bve: Option = b.assign.as_ref().map(|a| a.value.clone()); - if bve.is_none() && bi == 0 { + if bve.is_none() + && (bi == 0 + || matches!( + rule_type, + RuleType::PartialSet | RuleType::PartialObject + )) + { bve = head_value.clone(); } match Self::static_value_of_expr(&bve) { @@ -655,6 +673,7 @@ impl<'a> Compiler<'a> { self.rule_definition_static_values[rule_index as usize].push(def_static_value); self.rule_definitions[rule_index as usize].push(body_entry_points); + self.rule_definition_else_bodies[rule_index as usize].push(body_is_else); if self.register_counter > num_registers_used { num_registers_used = self.register_counter; diff --git a/src/parser.rs b/src/parser.rs index 1f96464ac..8f7f73ee7 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -1725,6 +1725,7 @@ impl<'source> Parser<'source> { bodies.push(RuleBody { span, assign, + is_else: false, query, }); // Guard rule body accumulation against allocator limits. @@ -1745,6 +1746,7 @@ impl<'source> Parser<'source> { bodies.push(RuleBody { span, assign, + is_else: false, query, }); // Guard rule body accumulation against allocator limits. @@ -1772,6 +1774,7 @@ impl<'source> Parser<'source> { bodies.push(RuleBody { span, assign: None, + is_else: false, query, }); // Guard rule body accumulation against allocator limits. @@ -1806,6 +1809,7 @@ impl<'source> Parser<'source> { bodies.push(RuleBody { span, assign, + is_else: true, query, }); // Guard rule body accumulation against allocator limits. @@ -1821,6 +1825,7 @@ impl<'source> Parser<'source> { bodies.push(RuleBody { span, assign, + is_else: true, query, }); // Guard rule body accumulation against allocator limits. @@ -1848,6 +1853,7 @@ impl<'source> Parser<'source> { bodies.push(RuleBody { span, assign, + is_else: true, query, }); // Guard rule body accumulation against allocator limits. diff --git a/src/rvm/program/types.rs b/src/rvm/program/types.rs index 1b74681cf..37e3817c8 100644 --- a/src/rvm/program/types.rs +++ b/src/rvm/program/types.rs @@ -97,6 +97,10 @@ pub struct RuleInfo { /// Optional destructuring block entry point per definition /// Index: definition_index → Some(entry_point) | None pub destructuring_blocks: Vec>, + /// Per-definition markers for bodies introduced by `else`. Partial rules + /// accumulate independent bodies but must short-circuit else branches. + #[serde(default)] + pub else_bodies: Vec>, /// If true, all definitions are statically known to produce the same result /// value, so the VM can stop after the first successful definition without /// checking consistency with remaining definitions. @@ -113,6 +117,10 @@ impl RuleInfo { num_registers: u8, ) -> Self { let num_definitions = definitions.len(); + let else_bodies = definitions + .iter() + .map(|b| alloc::vec![false; b.len()]) + .collect(); Self { name, rule_type, @@ -122,6 +130,7 @@ impl RuleInfo { result_reg, num_registers, destructuring_blocks: alloc::vec![None; num_definitions], + else_bodies, early_exit_on_first_success: false, } } @@ -137,6 +146,10 @@ impl RuleInfo { ) -> Self { let num_params = u32::try_from(param_names.len()).unwrap_or(u32::MAX); let num_definitions = definitions.len(); + let else_bodies = definitions + .iter() + .map(|b| alloc::vec![false; b.len()]) + .collect(); Self { name, rule_type, @@ -149,6 +162,7 @@ impl RuleInfo { result_reg, num_registers, destructuring_blocks: alloc::vec![None; num_definitions], + else_bodies, early_exit_on_first_success: false, } } diff --git a/src/rvm/tests/vm.rs b/src/rvm/tests/vm.rs index 3bd108919..564e25dce 100644 --- a/src/rvm/tests/vm.rs +++ b/src/rvm/tests/vm.rs @@ -429,6 +429,10 @@ mod tests { result_reg, num_registers: 50, // Increased to accommodate test cases with higher register indices destructuring_blocks, + else_bodies: definitions + .iter() + .map(|b| alloc::vec![false; b.len()]) + .collect(), early_exit_on_first_success: false, }; diff --git a/src/rvm/vm/rules.rs b/src/rvm/vm/rules.rs index 135863bdd..863a176e3 100644 --- a/src/rvm/vm/rules.rs +++ b/src/rvm/vm/rules.rs @@ -144,9 +144,26 @@ impl RegoVM { } } - // Once a body in this definition succeeds, remaining bodies - // are treated as else-branches and must not be evaluated. - break; + // Independent bodies of partial rules accumulate their + // contributions. `else` bodies (and all complete/function + // rules) preserve first-success semantics. + let next_is_else = rule_info + .else_bodies + .get(def_idx) + .and_then(|bodies| { + body_entry_point_idx + .checked_add(1) + .and_then(|next_idx| bodies.get(next_idx)) + }) + .copied() + .unwrap_or(false); + if !matches!( + rule_info.rule_type, + RuleType::PartialSet | RuleType::PartialObject + ) || next_is_else + { + break; + } } Err(e) if Self::is_fatal_vm_error(&e) => { self.restore_rule_state( diff --git a/tests/interpreter/cases/builtins/objects/json.patch.yaml b/tests/interpreter/cases/builtins/objects/json.patch.yaml index bd8782fc3..10fa41ca9 100644 --- a/tests/interpreter/cases/builtins/objects/json.patch.yaml +++ b/tests/interpreter/cases/builtins/objects/json.patch.yaml @@ -246,3 +246,176 @@ cases: result = json.patch(obj, patches) query: data.test.result == {"a", "b", "c"} want_result: true + + - note: array-form path rejects an integral float index + data: {} + modules: + - | + package test + + result := json.patch(["a", "b"], [{"op": "remove", "path": [1.0]}]) + query: data.test.result + no_result: true + + - note: removing the root produces undefined rather than null + data: {} + modules: + - | + package test + + result := json.patch({"a": 1}, [{"op": "remove", "path": ""}]) + query: data.test.result + no_result: true + + - note: root add after root remove restores the document + data: {} + modules: + - | + package test + + result := json.patch({"a": 1}, [{"op": "remove", "path": ""}, {"op": "add", "path": "", "value": {"b": 2}}]) + query: data.test.result + want_result: + b: 2 + + - note: replace nested value inside a set member + data: {} + modules: + - | + package test + + target := {{"a": [1, 2]}} + member := {"a": [1, 2]} + result := json.patch(target, [{"op": "replace", "path": [member, "a", 1], "value": 9}]) + query: 'data.test.result == {{"a": [1, 9]}}' + want_result: true + + - note: copy nested value inside a set member + data: {} + modules: + - | + package test + + target := {{"a": [1]}} + member := {"a": [1]} + result := json.patch(target, [{"op": "copy", "from": [member, "a", 0], "path": [member, "a", "-"]}]) + query: 'data.test.result == {{"a": [1, 1]}}' + want_result: true + + - note: test nested value inside a set member + data: {} + modules: + - | + package test + + member := {"a": [1, 2]} + result := json.patch({member}, [{"op": "test", "path": [member, "a", 1], "value": 2}]) + query: 'data.test.result == {{"a": [1, 2]}}' + want_result: true + + - note: reject negative array index + data: {} + modules: + - | + package test + result := json.patch([1, 2], [{"op": "remove", "path": [-1]}]) + query: data.test.result + no_result: true + + - note: reject overflowing array index + data: {} + modules: + - | + package test + result := json.patch([1, 2], [{"op": "remove", "path": [999999999999999999999999999999999999]}]) + query: data.test.result + no_result: true + + - note: reject leading zero array index + data: {} + modules: + - | + package test + result := json.patch([1, 2], [{"op": "remove", "path": "/01"}]) + query: data.test.result + no_result: true + + - note: reject dash index for remove + data: {} + modules: + - | + package test + result := json.patch([1, 2], [{"op": "remove", "path": "/-"}]) + query: data.test.result + no_result: true + + - note: reject missing operation attribute + data: {} + modules: + - | + package test + result := json.patch({}, [{"path": "/a", "value": 1}]) + query: data.test.result + no_result: true + + - note: reject missing path attribute + data: {} + modules: + - | + package test + result := json.patch({}, [{"op": "add", "value": 1}]) + query: data.test.result + no_result: true + + - note: reject missing value attribute + data: {} + modules: + - | + package test + result := json.patch({}, [{"op": "add", "path": "/a"}]) + query: data.test.result + no_result: true + + - note: reject missing from attribute + data: {} + modules: + - | + package test + result := json.patch({}, [{"op": "move", "path": "/a"}]) + query: data.test.result + no_result: true + + - note: reject malformed operation and path types + data: {} + modules: + - | + package test + result := json.patch({}, [{"op": 1, "path": {}, "value": 1}]) + query: data.test.result + no_result: true + + - note: reject moving a node into its own child + data: {} + modules: + - | + package test + result := json.patch({"a": {"b": 1}}, [{"op": "move", "from": "/a", "path": "/a/c"}]) + query: data.test.result + no_result: true + + - note: failed patch does not mutate its input value + data: {} + modules: + - | + package test + + original := {"a": [1, 2, 3]} + failed := json.patch(original, [ + {"op": "remove", "path": "/a/0"}, + {"op": "remove", "path": "/missing"} + ]) + original_unchanged { + original == {"a": [1, 2, 3]} + } + query: data.test.original_unchanged + want_result: true diff --git a/tests/memory_limits.rs b/tests/memory_limits.rs index 6fa85c124..e80d05c21 100644 --- a/tests/memory_limits.rs +++ b/tests/memory_limits.rs @@ -72,6 +72,13 @@ package limit large_array := json.unmarshal(data.limit.large_json) "#; +#[cfg(feature = "jsonpatch")] +const JSON_PATCH_MODULE: &str = r#" +package limit + +patched := json.patch(input, [{"op": "add", "path": "/-", "value": 0}]) +"#; + fn assert_memory_limit_error(err: &Error) { match err.downcast_ref::() { Some(LimitError::MemoryLimitExceeded { .. }) => {} @@ -158,6 +165,25 @@ fn interpreter_memory_limit_during_large_allocation() { assert_memory_limit_error(&err); } +#[cfg(feature = "jsonpatch")] +#[test] +fn json_patch_propagates_memory_limit_errors() { + let mut guard = LimitGuard::lock(); + let mut engine = new_engine_with_module(JSON_PATCH_MODULE); + let input = Value::from((0..50_000).map(Value::from).collect::>()); + engine.set_input(input); + + // Entry checks require no meaningful allocation. This budget lets + // evaluation enter the builtin, then forces the edit-tree construction + // to trip the allocator limit. The builtin must propagate LimitError, + // never translate it to Undefined as it does malformed patches. + guard.set_with_additional_budget(64 * 1024); + let err = engine + .eval_rule("data.limit.patched".to_string()) + .expect_err("expected json.patch edit-tree allocation to hit the memory limit"); + assert_memory_limit_error(&err); +} + #[cfg(feature = "rvm")] #[test] fn vm_memory_limit_during_large_allocation() { diff --git a/tests/opa.rs b/tests/opa.rs index 1dfaa1bd2..79be338b0 100644 --- a/tests/opa.rs +++ b/tests/opa.rs @@ -433,14 +433,6 @@ fn run_opa_tests(opa_tests_dir: String, folders: &[String]) -> Result<()> { case.note ); skip_rvm_validation = true; - } else if case.note == "jsonpatch/json_patch_tests" { - // Interpreter now correctly runs every body of a multi-body - // partial-object rule (`passed_cases[k] = t { .. } { .. }`), - // matching OPA. RVM's partial-object-rule codegen doesn't - // yet handle multiple bodies contributing distinct keys -- - // tracked separately in - // https://github.com/microsoft/regorus/issues/665. - skip_rvm_validation = true; } else if case.note == "reachable_paths/cycle_1022_3" { // The OPA behavior is not well-defined. // See: https://github.com/open-policy-agent/opa/issues/5871 diff --git a/tests/rvm/rego/cases/partial_object_rules.yaml b/tests/rvm/rego/cases/partial_object_rules.yaml index e7f268bb7..0f3d7d7b4 100644 --- a/tests/rvm/rego/cases/partial_object_rules.yaml +++ b/tests/rvm/rego/cases/partial_object_rules.yaml @@ -218,6 +218,65 @@ cases: BAZ: true FOO: true + - note: partial_object_legacy_body_retains_rule_head_value + rego_v0: true + data: {} + input: {} + modules: + - | + package test + + values[k] = "expected" { + k := "unused" + false + } { + k := "x" + } + query: data.test.values + want_result: + x: expected + + - note: partial_object_else_short_circuits_after_success + data: {} + input: {} + modules: + - | + package test + + p[k] := 1 if { + k := "x" + true + } else := 2 if { + k := "x" + true + } + query: data.test.p + want_result: + x: 1 + + - note: partial_object_parity_does_not_reverse_negated_decision + rego_v0: true + data: {} + input: {} + modules: + - | + package test + + default allow := false + + parts[k] = true { + k := "a" + } { + k := "b" + } + + allow { + parts.a + not parts.b + } + query: data.test.allow + want_result: false + - note: partial_set_contains_collects_all_bindings data: {} input: diff --git a/tests/rvm/rego/mod.rs b/tests/rvm/rego/mod.rs index 7eb6f3fbe..05c2f8815 100644 --- a/tests/rvm/rego/mod.rs +++ b/tests/rvm/rego/mod.rs @@ -34,6 +34,7 @@ struct TestCase { pub want_error_code: Option, #[serde(default = "default_strict")] pub strict: bool, + pub rego_v0: Option, pub allow_interpreter_success: Option, pub allow_interpreter_incorrect_behavior: Option, pub skip_interpreter: Option, @@ -406,6 +407,7 @@ fn yaml_test_impl(file: &str) -> Result<()> { executed_count += 1; let mut engine = Engine::new(); + engine.set_rego_v0(case.rego_v0 == Some(true)); for (idx, module) in case.modules.iter().enumerate() { engine.add_policy(format!("rego_{idx}"), module.clone())?; } From d65d65b7e14c771295cdc22ef4675719be6ac6ac Mon Sep 17 00:00:00 2001 From: Vitalii Tverdokhlib Date: Fri, 7 Aug 2026 12:37:40 +0500 Subject: [PATCH 6/6] fix: match OPA set replace semantics --- src/builtins/json_patch.rs | 9 ++++++++- .../cases/builtins/objects/json.patch.yaml | 16 +++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/builtins/json_patch.rs b/src/builtins/json_patch.rs index 17d005a19..3bbb90242 100644 --- a/src/builtins/json_patch.rs +++ b/src/builtins/json_patch.rs @@ -222,6 +222,7 @@ impl EditNode { Ok(removed) } + #[cfg(test)] fn replace(&mut self, path: &[Value], value: EditNode) -> Result<()> { enforce_limit()?; let Some((head, rest)) = path.split_first() else { @@ -326,6 +327,7 @@ impl EditTree { } } + #[cfg(test)] fn replace_value(&mut self, path: &[Value], value: &Value) -> Result<()> { let node = EditNode::from_value(value)?; if path.is_empty() { @@ -375,7 +377,12 @@ pub(super) fn apply(target: &Value, operations: &[Value]) -> Result { } "replace" => { let path = parse_path(field("path")?)?; - tree.replace_value(&path, field("value")?)?; + // OPA composes replace from delete + insert. This distinction + // is observable for sets, whose keys must equal their values: + // replacing member "a" at path ["a"] with "b" must fail + // rather than silently changing the set's membership key. + tree.remove(&path)?; + tree.insert_value(&path, field("value")?)?; } "move" => { let from = parse_path(field("from")?)?; diff --git a/tests/interpreter/cases/builtins/objects/json.patch.yaml b/tests/interpreter/cases/builtins/objects/json.patch.yaml index 10fa41ca9..ffce57645 100644 --- a/tests/interpreter/cases/builtins/objects/json.patch.yaml +++ b/tests/interpreter/cases/builtins/objects/json.patch.yaml @@ -247,6 +247,16 @@ cases: query: data.test.result == {"a", "b", "c"} want_result: true + - note: replacing a set member with a different value is undefined + data: {} + modules: + - | + package test + + result := json.patch({"a", "b"}, [{"op": "replace", "path": ["a"], "value": "c"}]) + query: data.test.result + no_result: true + - note: array-form path rejects an integral float index data: {} modules: @@ -278,7 +288,7 @@ cases: want_result: b: 2 - - note: replace nested value inside a set member + - note: replacing a nested value inside a set member is undefined data: {} modules: - | @@ -287,8 +297,8 @@ cases: target := {{"a": [1, 2]}} member := {"a": [1, 2]} result := json.patch(target, [{"op": "replace", "path": [member, "a", 1], "value": 9}]) - query: 'data.test.result == {{"a": [1, 9]}}' - want_result: true + query: data.test.result + no_result: true - note: copy nested value inside a set member data: {}