diff --git a/Cargo.toml b/Cargo.toml index d43ff2c7c..c8b432eb8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,6 +40,7 @@ http = [] glob = ["dep:globset"] graph = [] jsonschema = ["dep:jsonschema"] +jsonpatch = [] 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", @@ -86,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..3bbb90242 --- /dev/null +++ b/src/builtins/json_patch.rs @@ -0,0 +1,549 @@ +// 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) + } + + #[cfg(test)] + 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) + } + } + + #[cfg(test)] + 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")?)?; + // 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")?)?; + 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 b5fac7bad..a52884c23 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,31 @@ fn json_match_schema( .to_vec(), )) } + +// 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()?; + + 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 + // 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 36cc703ac..fc0294891 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -3412,30 +3412,72 @@ impl Interpreter { ) -> Result { self.check_execution_time()?; let n_scopes = self.scopes.len(); + // 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()); 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 output_expr = body.assign.as_ref().map(|e| e.value.clone()); - self.contexts.push(Context { + let popped = self + .contexts + .pop() + .ok_or_else(|| anyhow!("internal error: rule's context already popped"))?; + // 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(), // ..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; + 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; + } + } + Err(_) => break, + Ok(false) => {} } } - result + if is_partial { + result.map(|_| any_success) + } else { + result + } }; let popped_ctx = match self.contexts.pop() { 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/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 new file mode 100644 index 000000000..ffce57645 --- /dev/null +++ b/tests/interpreter/cases/builtins/objects/json.patch.yaml @@ -0,0 +1,431 @@ +# 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" + + - 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 + + - 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: + - | + 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: replacing a nested value inside a set member is undefined + 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 + no_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.passing b/tests/opa.passing index 439bef129..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 @@ -147,6 +148,7 @@ v1/intersection v1/jsonbuiltins v1/jsonfilter v1/jsonfilteridempotent +v1/jsonpatch v1/jsonremove v1/jsonremoveidempotent v1/jsonschema 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())?; }