From 3e1ddac2a906a59cfdb6c9b6bed745b0a273e594 Mon Sep 17 00:00:00 2001 From: Anand Krishnamoorthi <35780660+anakrish@users.noreply.github.com> Date: Fri, 5 Jan 2024 11:13:25 -0800 Subject: [PATCH] OPA conformance: Pass refheads test suite (#90) - parser: allow non-string index at any position; not just last - impl Default for Context - Fix width of OPA test results table - Allow non string compoenent anywhere in rule ref; not just as last item. - Normalize want_result before comparison. - Ensure that object rules are created even if no definition succeed, - Sort want_result values for "refheads/general, multiple result-set entries" The entries are in reverse order of how OPA and regorus produce. - Emit PASS status for each OPA testpoint - Detect rule conflicts Signed-off-by: Anand Krishnamoorthi --- src/ast.rs | 8 ++ src/interpreter.rs | 345 +++++++++++++++++++++++++++++++++++++-------- src/parser.rs | 30 ---- tests/opa.passing | 1 + tests/opa.rs | 32 +++-- 5 files changed, 315 insertions(+), 101 deletions(-) diff --git a/src/ast.rs b/src/ast.rs index 3eb3975..23e4ea3 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -327,6 +327,14 @@ pub enum Rule { }, } +impl Rule { + pub fn span(&self) -> &Span { + match self { + Self::Spec { span, .. } | Self::Default { span, .. } => span, + } + } +} + #[derive(Debug)] pub struct Package { pub span: Span, diff --git a/src/interpreter.rs b/src/interpreter.rs index e165b41..f6fd71f 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -13,7 +13,9 @@ use crate::value::*; use anyhow::{anyhow, bail, Result}; use log::info; use serde::Serialize; +use std::collections::btree_map::Entry as BTreeMapEntry; use std::collections::{hash_map::Entry, BTreeMap, BTreeSet, HashMap}; +use std::ops::Bound::*; use std::rc::Rc; use std::str::FromStr; @@ -27,6 +29,7 @@ type State = ( Value, BTreeSet>, BTreeMap, + BTreeMap, (Value, Ref)>, ); #[derive(Debug, Clone)] @@ -54,6 +57,7 @@ pub struct Interpreter { rules: HashMap>>, default_rules: HashMap>, processed: BTreeSet>, + rule_values: BTreeMap, (Value, Ref)>, active_rules: Vec>, builtins_cache: BTreeMap<(&'static str, Vec), Value>, no_rules_lookup: bool, @@ -112,6 +116,27 @@ struct Context { result: Option, results: QueryResults, is_compr: bool, + rule_ref: Option, + rule_value: Value, + is_set: bool, + is_old_style_set: bool, +} + +impl Default for Context { + fn default() -> Self { + Self { + key_expr: None, + output_expr: None, + value: Value::Undefined, + result: None, + results: QueryResults::default(), + is_compr: false, + rule_ref: None, + rule_value: Value::new_object(), + is_set: false, + is_old_style_set: false, + } + } } #[derive(Debug)] @@ -177,6 +202,7 @@ impl Interpreter { rules: HashMap::new(), default_rules: HashMap::new(), processed: BTreeSet::new(), + rule_values: BTreeMap::new(), active_rules: vec![], builtins_cache: BTreeMap::new(), no_rules_lookup: false, @@ -734,12 +760,8 @@ impl Interpreter { self.scopes.push(Scope::new()); self.contexts.push(Context { - key_expr: None, - output_expr: None, value: Value::new_set(), - result: None, - results: QueryResults::default(), - is_compr: false, + ..Context::default() }); let mut r = true; match domain { @@ -1183,8 +1205,10 @@ impl Interpreter { let data = self.data.clone(); let processed = self.processed.clone(); let with_functions = self.with_functions.clone(); + let rule_values = self.rule_values.clone(); self.processed.clear(); + self.rule_values.clear(); let mut skip_exec = false; // Apply with modifiers. @@ -1275,7 +1299,14 @@ impl Interpreter { self.data = self.with_document["data"].clone(); self.input = self.with_document["input"].clone(); Ok(( - Some((with_document, input, data, processed, with_functions)), + Some(( + with_document, + input, + data, + processed, + with_functions, + rule_values, + )), skip_exec, )) } else { @@ -1291,6 +1322,7 @@ impl Interpreter { self.data, self.processed, self.with_functions, + self.rule_values, ) = s; } Ok(()) @@ -1472,10 +1504,160 @@ impl Interpreter { } } + fn eval_rule_ref(&mut self, refr: &ExprRef) -> Result> { + let mut comps = vec![]; + let mut expr = refr; + loop { + match expr.as_ref() { + Expr::Var(v) => { + comps.push(Value::String(v.text().into())); + break; + } + Expr::RefBrack { refr, index, .. } => { + comps.push(self.eval_expr(index)?); + expr = refr; + } + Expr::RefDot { refr, field, .. } => { + comps.push(Value::String(field.text().into())); + expr = refr; + } + _ => { + bail!(expr.span().error("not a valid rule ref")); + } + } + } + comps.reverse(); + Ok(comps) + } + + fn update_rule_value( + &mut self, + span: &Span, + path: Vec, + mut value: Value, + is_set: bool, + ) -> Result<()> { + // If rule's value already exists in initial document, prefer it. + { + let mut init_obj = &self.init_data; + for p in path.iter() { + init_obj = &init_obj[p]; + } + + if init_obj != &Value::Undefined { + return Ok(()); + } + } + + let mut obj = &mut self.data; + let len = path.len(); + for (idx, p) in path.into_iter().enumerate() { + if idx == len - 1 { + // last key. + if is_set { + let set = obj + .as_object_mut() + .map_err(|_| anyhow!(span.error("previous value is not an object")))? + .entry(p) + .or_insert(Value::new_set()) + .as_set_mut() + .map_err(|_| anyhow!(span.error("previous value is not a set")))?; + set.append(value.as_set_mut()?); + } else { + let obj = obj + .as_object_mut() + .map_err(|_| anyhow!(span.error("previous value is not an object")))?; + match obj.entry(p) { + BTreeMapEntry::Vacant(v) => { + if value != Value::Undefined { + v.insert(value); + } else { + // TODO: clean this assumption between Undefined vs Object. + v.insert(Value::new_object()); + } + } + BTreeMapEntry::Occupied(o) => { + if o.get() != &value && value != Value::Undefined { + bail!(span + .error("complete rules should not produce multiple outputs")) + } + } + } + } + break; + } else { + obj = obj + .as_object_mut() + .map_err(|_| anyhow!(span.error("previous value is not an object")))? + .entry(p) + .or_insert(Value::new_object()); + } + } + Ok(()) + } + fn eval_output_expr_in_loop(&mut self, loops: &[LoopExpr]) -> Result { if loops.is_empty() { let (key_expr, output_expr) = self.get_exprs_from_context()?; + let ctx = self.get_current_context()?; + let (is_set, is_old_style_set) = (ctx.is_set, ctx.is_old_style_set); + if let Some(rule_ref) = ctx.rule_ref.clone() { + let mut comps = self.eval_rule_ref(&rule_ref)?; + if let Some(ke) = &key_expr { + comps.push(self.eval_expr(ke)?); + } + let output = if let Some(oe) = &output_expr { + self.eval_expr(oe)? + } else if is_old_style_set && !comps.is_empty() { + let output = comps[comps.len() - 1].clone(); + comps.pop(); + output + } else { + Value::Bool(true) + }; + + let comps_defined = comps.iter().all(|v| v != &Value::Undefined); + let ctx = self.contexts.last_mut().expect("no current context"); + + if output == Value::Undefined || !comps_defined { + return Ok(false); + } + + if is_set { + // Ensure that set rule is created even if the element is undefined. + let set = ctx + .rule_value + .as_object_mut()? + .entry(Value::from_array(comps)) + .or_insert(Value::new_set()); + if output != Value::Undefined { + set.as_set_mut()?.insert(output); + return Ok(true); + } + return Ok(false); + } + + // Non-set rule. + match ctx + .rule_value + .as_object_mut()? + .entry(Value::from_array(comps)) + { + BTreeMapEntry::Vacant(v) => { + v.insert(output); + } + BTreeMapEntry::Occupied(o) if o.get() != &output => bail!(rule_ref + .span() + .error("rules must not produce multiple outputs")), + _ => { + // Rule produced same value. + } + } + + return Ok(true); + } + match (key_expr, output_expr) { (Some(ke), Some(oe)) => { let key = self.eval_expr(&ke)?; @@ -1788,12 +1970,10 @@ impl Interpreter { fn eval_array_compr(&mut self, term: &ExprRef, query: &Ref) -> Result { // Push new context self.contexts.push(Context { - key_expr: None, output_expr: Some(term.clone()), value: Value::new_array(), - result: None, - results: QueryResults::default(), is_compr: true, + ..Context::default() }); // Evaluate body first. @@ -1808,12 +1988,10 @@ impl Interpreter { fn eval_set_compr(&mut self, term: &ExprRef, query: &Ref) -> Result { // Push new context self.contexts.push(Context { - key_expr: None, output_expr: Some(term.clone()), value: Value::new_set(), - result: None, - results: QueryResults::default(), is_compr: true, + ..Context::default() }); self.eval_query(query)?; @@ -1835,9 +2013,8 @@ impl Interpreter { key_expr: Some(key.clone()), output_expr: Some(value.clone()), value: Value::new_object(), - result: None, - results: QueryResults::default(), is_compr: true, + ..Context::default() }); self.eval_query(query)?; @@ -2056,12 +2233,9 @@ impl Interpreter { } let ctx = Context { - key_expr: None, output_expr: output_expr.clone(), value: Value::new_set(), - result: None, - results: QueryResults::default(), - is_compr: false, + ..Context::default() }; let value = match self.eval_rule_bodies(ctx, span, bodies) { @@ -2453,29 +2627,28 @@ impl Interpreter { }; Parser::get_path_ref_components_into(refr, &mut path)?; - Ok(( Context { key_expr, output_expr, value, - result: None, - results: QueryResults::default(), - is_compr: false, + rule_ref: Some(refr.clone()), + ..Context::default() }, path, )) } RuleHead::Set { refr, key, .. } => { Parser::get_path_ref_components_into(refr, &mut path)?; + let is_old_style_set = key.is_none(); Ok(( Context { - key_expr: None, output_expr: key.clone(), value: Value::new_set(), - result: None, - results: QueryResults::default(), - is_compr: false, + rule_ref: Some(refr.clone()), + is_set: true, + is_old_style_set, + ..Context::default() }, path, )) @@ -2512,12 +2685,10 @@ impl Interpreter { self.contexts.pop(); let output_expr = body.assign.as_ref().map(|e| e.value.clone()); self.contexts.push(Context { - key_expr: None, output_expr, - value: Value::new_array(), - result: None, - results: QueryResults::default(), - is_compr: false, + // value: Value::new_array(), + // ..Context::default() + ..ctx.clone() }); } result = self.eval_query(&body.query); @@ -2541,6 +2712,14 @@ impl Interpreter { assert_eq!(self.scopes.len(), n_scopes); + if ctx.rule_ref.is_some() { + if result { + return Ok(ctx.rule_value); + } else { + return Ok(Value::Undefined); + } + } + Ok(match result { true => match &ctx.value { Value::Object(_) => ctx.value, @@ -2741,6 +2920,7 @@ impl Interpreter { let (refr, index) = match refr.as_ref() { Expr::RefBrack { refr, index, .. } => (refr, Some(index.clone())), + Expr::RefDot { .. } => (refr, None), Expr::Var(_) => (refr, None), _ => bail!(refr.span().error(&format!( "invalid token {:?} with the default keyword", @@ -2806,6 +2986,42 @@ impl Interpreter { } } + fn check_rule_path( + &mut self, + refr: &ExprRef, + path: &[Value], + value: &Value, + is_set: bool, + ) -> Result<()> { + // TODO: can copying of path be avoided below? + let range = self.rule_values.range((Unbounded, Included(path.to_vec()))); + + // Check whether any rules evaluated so far is a parent of given rule. + let mut conflict = None; + for (k, (v, r)) in range.rev() { + if path.starts_with(k) { + if k == path && (is_set || v == value) { + // rule evaluated to same value again. + } else { + conflict = Some((v, r)); + } + } else { + break; + } + } + + if let Some((_, r)) = conflict { + bail!(refr.span().error(&format!( + "rule conflicts with the following rule:\n{}", + r.span().message("", "defined here") + ))); + } + self.rule_values + .insert(path.to_vec(), (value.clone(), refr.clone())); + + Ok(()) + } + pub fn eval_rule(&mut self, module: &Ref, rule: &Ref) -> Result<()> { // Skip reprocessing rule if self.processed.contains(rule) { @@ -2851,32 +3067,41 @@ impl Interpreter { } => { match rule_head { RuleHead::Compr { refr, .. } | RuleHead::Set { refr, .. } => { - let (ctx, mut path) = self.make_rule_context(rule_head)?; - let special_set = - matches!((&ctx.output_expr, &ctx.value), (None, Value::Set(_))); - let value = match self.eval_rule_bodies(ctx, span, rule_body)? { - Value::Set(_) if special_set => { - let entry = path[path.len() - 1].text(); - let mut s = BTreeSet::new(); - s.insert(Value::String(entry.to_string().into())); - path = path[0..path.len() - 1].to_vec(); - Value::from_set(s) + let (ctx, _) = self.make_rule_context(rule_head)?; + let is_set = ctx.is_set; + let is_object = ctx.key_expr.is_some() && !is_set; + + let value = self.eval_rule_bodies(ctx, span, rule_body)?; + let package_components = self.eval_rule_ref(&module.package.refr)?; + + if value != Value::Undefined { + for (path, value) in value.as_object()? { + let mut full_path = package_components.clone(); + full_path.append(&mut path.as_array()?.clone()); + self.check_rule_path(refr, &full_path, value, is_set)?; + self.update_rule_value(span, full_path, value.clone(), is_set)?; } - v => v, - }; - - let paths: Vec<&str> = path.iter().map(|s| s.text()).collect(); - - if let RuleHead::Set { .. } = &rule_head { - // Ensure that sets are created as empty. - let vref = Self::make_or_get_value_mut(&mut self.data, &paths)?; - if *vref == Value::Undefined { - *vref = Value::new_set(); + } else if is_set { + if let Ok(mut comps) = self.eval_rule_ref(refr) { + let mut full_path = package_components; + full_path.append(&mut comps); + self.update_rule_value(span, full_path, Value::new_set(), true)?; + } + } else if is_object { + // Fetch the rule, ignoring the key. + if let Expr::RefBrack { refr, .. } = refr.as_ref() { + if let Ok(mut comps) = self.eval_rule_ref(refr) { + let mut full_path = package_components; + full_path.append(&mut comps); + self.update_rule_value( + span, + full_path, + Value::Undefined, + false, + )?; + } } } - - self.update_data(span, refr, &paths[..], value)?; - self.processed.insert(rule.clone()); } RuleHead::Func { @@ -2901,12 +3126,9 @@ impl Interpreter { if args.is_empty() { let ctx = Context { - key_expr: None, output_expr: assign.as_ref().map(|a| a.value.clone()), value: Value::new_array(), - result: None, - results: QueryResults::default(), - is_compr: false, + ..Context::default() }; let value = self.eval_rule_bodies(ctx, span, rule_body)?; @@ -2946,13 +3168,10 @@ impl Interpreter { // Push new context. self.contexts.push(Context { - key_expr: None, - output_expr: None, value: Value::new_set(), // Request that results be gathered. result: Some(QueryResult::default()), - results: QueryResults::default(), - is_compr: false, + ..Context::default() }); let prev_module = self.set_current_module(Some(module.clone()))?; @@ -3021,6 +3240,8 @@ impl Interpreter { Expr::RefBrack { refr, index, .. } => { if let Expr::String(s) = index.as_ref() { components.push(s.text().into()); + } else { + components.clear(); } refr } diff --git a/src/parser.rs b/src/parser.rs index 0cdce70..09d2957 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -1112,27 +1112,6 @@ impl<'source> Parser<'source> { Ok(refr) } - fn check_rule_ref(&self, mut refr: &Expr) -> Result<()> { - // Only the last term can be non-string - loop { - refr = match refr { - Expr::RefDot { refr, .. } => refr, - Expr::RefBrack { span, refr, index } => { - if !matches!(index.as_ref(), Expr::String(_)) { - return Err(self.source.error( - span.line, - span.col, - "only the final ref term can be non-string", - )); - } - refr - } - Expr::Var(_) => return Ok(()), - _ => bail!("internal error: not a valid ref"), - }; - } - } - fn parse_rule_ref(&mut self) -> Result { let start = self.tok.1.start; let span = self.tok.1.clone(); @@ -1209,7 +1188,6 @@ impl<'source> Parser<'source> { let rule_ref = Ref::new(self.parse_rule_ref()?); match self.token_text() { "(" => { - self.check_rule_ref(&rule_ref)?; self.next_token()?; let mut args = vec![]; if self.token_text() != ")" { @@ -1235,7 +1213,6 @@ impl<'source> Parser<'source> { }) } "contains" => { - self.check_rule_ref(&rule_ref)?; self.next_token()?; let key = Ref::new(self.parse_membership_expr()?); span.end = self.end; @@ -1249,13 +1226,6 @@ impl<'source> Parser<'source> { let assign = self.parse_rule_assign()?; span.end = self.end; - // Ensure that only the last term can be non-string. - match rule_ref.as_ref() { - Expr::RefBrack { refr, .. } => self.check_rule_ref(refr)?, - Expr::RefDot { refr, .. } => self.check_rule_ref(refr)?, - _ => (), - } - // Determine whether to create a set or a compr let is_set_follower = !self.is_keyword(self.token_text()) && !self.is_imported_future_keyword(self.token_text()); diff --git a/tests/opa.passing b/tests/opa.passing index fdd3682..833babd 100644 --- a/tests/opa.passing +++ b/tests/opa.passing @@ -77,6 +77,7 @@ partialsetdoc planner-ir rand reachable +refheads regexfind regexfindallstringsubmatch regexisvalid diff --git a/tests/opa.rs b/tests/opa.rs index 9fba8e7..90757c7 100644 --- a/tests/opa.rs +++ b/tests/opa.rs @@ -104,6 +104,7 @@ fn eval_test_case(case: &TestCase) -> Result { Value::Undefined }); } + let result = Value::from_array(values); // Make result json compatible. (E.g: avoid sets). Value::from_json_str(&result.to_string()) @@ -128,7 +129,7 @@ fn json_schema_tests_check(actual: &Value, expected: &Value) -> bool { fn run_opa_tests(opa_tests_dir: String, folders: &[String]) -> Result<()> { println!("OPA TESTSUITE: {opa_tests_dir}"); let tests_path = Path::new(&opa_tests_dir); - let mut status = BTreeMap::::new(); + let mut status = BTreeMap::::new(); let mut n = 0; let mut missing_functions = BTreeMap::new(); for entry in WalkDir::new(&opa_tests_dir) @@ -156,7 +157,7 @@ fn run_opa_tests(opa_tests_dir: String, folders: &[String]) -> Result<()> { continue; } - let entry = status.entry(path_dir_str).or_insert((0, 0)); + let entry = status.entry(path_dir_str).or_insert((0, 0, 0)); let yaml_str = std::fs::read_to_string(&path_str)?; let test: YamlTest = serde_yaml::from_str(&yaml_str)?; @@ -186,8 +187,20 @@ fn run_opa_tests(opa_tests_dir: String, folders: &[String]) -> Result<()> { // Skip the test. println!("skipping impossible test: {}", case.note); continue; + } else if case.note == "refheads/general, multiple result-set entries" { + // The entries are specified in reverse order of how object keys would be sorted. + if let Some(ref mut want_result) = &mut case.want_result { + want_result.as_array_mut()?.sort(); + } } + // Normalize for comparison. + if let Some(want_result) = case.want_result { + case.want_result = Some(serde_json::from_str(&want_result.to_string())?); + } + + print!("{:4}: {:90}", entry.2, case.note); + entry.2 += 1; match (eval_test_case(&case), &case.want_result) { (Ok(actual), Some(expected)) if is_json_schema_test && json_schema_tests_check(&actual, &expected) => @@ -271,7 +284,8 @@ fn run_opa_tests(opa_tests_dir: String, folders: &[String]) -> Result<()> { n += 1; continue; } - }; + } + println!(" \x1b[32mPASSED\x1b[0m"); } } @@ -279,7 +293,7 @@ fn run_opa_tests(opa_tests_dir: String, folders: &[String]) -> Result<()> { println!(" {:40} {:4} {:4}", "FOLDER", "PASS", "FAIL"); let (mut npass, mut nfail) = (0, 0); let mut passing = vec![]; - for (dir, (pass, fail)) in status { + for (dir, (pass, fail, _)) in status { if fail == 0 { println!("\x1b[32m {dir:40}: {pass:4} {fail:4}\x1b[0m"); passing.push(dir); @@ -296,20 +310,20 @@ fn run_opa_tests(opa_tests_dir: String, folders: &[String]) -> Result<()> { if npass == 0 && nfail == 0 { bail!("no matching tests found."); } else if nfail == 0 { - println!("\x1b[32m {:40}: {npass:4} {nfail:4}\x1b[0m", "TOTAL"); + println!("\x1b[32m {:42}: {npass:4} {nfail:4}\x1b[0m", "TOTAL"); } else { - println!("\x1b[31m {:40}: {npass:4} {nfail:4}\x1b[0m", "TOTAL"); + println!("\x1b[31m {:42}: {npass:4} {nfail:4}\x1b[0m", "TOTAL"); } if !missing_functions.is_empty() { println!("\nMISSING FUNCTIONS"); - println!(" {:4} {:40} {}", "", "FUNCTION", "FAILURES"); + println!(" {:4} {:42} {}", "", "FUNCTION", "FAILURES"); let mut ncalls = 0; for (idx, (fcn, calls)) in missing_functions.iter().enumerate() { - println!("\x1b[31m {:4}: {fcn:40} {calls}\x1b[0m", idx + 1); + println!("\x1b[31m {:4}: {fcn:42} {calls}\x1b[0m", idx + 1); ncalls += calls; } - println!("\x1b[31m {:4} {:40} {ncalls}\x1b[0m", "", "TOTAL"); + println!("\x1b[31m {:4} {:42} {ncalls}\x1b[0m", "", "TOTAL"); } if nfail != 0 {