diff --git a/src/interpreter.rs b/src/interpreter.rs index fe969f9..f88c14a 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -4,9 +4,9 @@ use crate::ast::*; use crate::builtins::{self, BuiltinFcn}; -use crate::compiled_policy::CompiledPolicyData; #[cfg(feature = "azure_policy")] use crate::compiled_policy::TargetInfo; +use crate::compiled_policy::{CompiledPolicyData, DefaultRuleInfo}; use crate::compiler::destructuring_planner::{ AssignmentPlan, BindingPlan, DestructuringPlan, WildcardSide, }; @@ -1724,6 +1724,9 @@ impl Interpreter { // For now, we restrict constant refs to those that contain only simple literals. fn is_constant_ref(mut expr: &Ref) -> Result { loop { + if Self::is_simple_literal(expr)? { + return Ok(true); + } match expr.as_ref() { Expr::Var { .. } => break, Expr::RefDot { refr, .. } => expr = refr, @@ -1747,6 +1750,27 @@ impl Interpreter { )) } + fn is_constant_key_expr(&self, expr: &Ref) -> Result { + if Self::is_simple_literal(expr)? { + return Ok(true); + } + + match expr.as_ref() { + Expr::Var { span, .. } => { + let scope = self + .scopes + .last() + .ok_or_else(|| anyhow!("internal error: no current scope"))?; + Ok(!scope.contains_key(&span.source_str())) + } + Expr::RefDot { refr, .. } => self.is_constant_key_expr(refr), + Expr::RefBrack { refr, index, .. } => { + Ok(self.is_constant_key_expr(refr)? && self.is_constant_key_expr(index)?) + } + _ => Ok(false), + } + } + // A rule's output expression is constant if it does not contain local variables. // For now, we restrict output expressions to those that contain only simple literals. fn is_constant_output(key_expr: &Option>, output_expr: &Ref) -> Result { @@ -1798,8 +1822,10 @@ impl Interpreter { output } else { // Implicit-true partial object rules can vary with each successful key binding. - if key_expr.is_some() && !is_old_style_set { - is_const_rule = false; + if let Some(ke) = &key_expr { + if !is_old_style_set && !self.is_constant_key_expr(ke)? { + is_const_rule = false; + } } Value::Bool(true) }; @@ -2945,6 +2971,41 @@ impl Interpreter { Ok(()) } + fn default_rules_for_path(&self, path: &str) -> Option> { + if let Some(rules) = self.compiled_policy.default_rules.get(path) { + return Some(rules.clone()); + } + + let (parent_path, index) = path.rsplit_once('.')?; + let rules = self.compiled_policy.default_rules.get(parent_path)?; + let matches = rules + .iter() + .filter(|(_, rule_index)| Self::default_rule_index_matches(rule_index, index)) + .cloned() + .collect::>(); + + if matches.is_empty() { + None + } else { + Some(matches) + } + } + + fn has_default_rules_for_path(&self, path: &str) -> bool { + self.default_rules_for_path(path).is_some() + } + + fn default_rule_index_matches(index: &Option, path_component: &str) -> bool { + match index.as_deref() { + Some(index) if index == path_component => true, + Some(index) => index + .strip_prefix('"') + .and_then(|index| index.strip_suffix('"')) + .is_some_and(|index| index == path_component), + None => false, + } + } + fn ensure_rule_evaluated(&mut self, path: String) -> Result<()> { self.check_execution_time()?; let mut matched = false; @@ -2959,9 +3020,9 @@ impl Interpreter { } // Evaluate the associated default rules after non-default rules - if let Some(rules) = self.compiled_policy.default_rules.get(&path) { + if let Some(rules) = self.default_rules_for_path(&path) { matched = true; - for (r, _) in rules.clone() { + for (r, _) in rules { if !self.processed.contains(&r) { let module = self.get_rule_module(&r)?; let prev_module = self.set_current_module(Some(module))?; @@ -3052,10 +3113,7 @@ impl Interpreter { let prefix = fields.iter().take(i).copied().collect::>(); let prefix_path = format!("data.{}", prefix.join(".")); if self.compiled_policy.rules.contains_key(&prefix_path) - || self - .compiled_policy - .default_rules - .contains_key(&prefix_path) + || self.has_default_rules_for_path(&prefix_path) { self.ensure_rule_evaluated(prefix_path)?; break; @@ -3079,7 +3137,7 @@ impl Interpreter { if !no_error && !self.compiled_policy.rules.contains_key(&rule_path) - && !self.compiled_policy.default_rules.contains_key(&rule_path) + && !self.has_default_rules_for_path(&rule_path) && !self.compiled_policy.imports.contains_key(&rule_path) { bail!(span.error(&format!( @@ -3102,7 +3160,7 @@ impl Interpreter { }; if self.compiled_policy.rules.contains_key(&path) - || self.compiled_policy.default_rules.contains_key(&path) + || self.has_default_rules_for_path(&path) { self.ensure_rule_evaluated(path)?; found = true; @@ -3649,7 +3707,7 @@ impl Interpreter { self.data = Value::Undefined; self.ensure_loop_var_values_capacity(); - let default_rules = self.compiled_policy.default_rules.get(rule_path).cloned(); + let default_rules = self.default_rules_for_path(rule_path); if let Some(rules) = default_rules { for (rule, _) in rules { diff --git a/src/languages/rego/compiler/program.rs b/src/languages/rego/compiler/program.rs index ff39dce..2f4b51f 100644 --- a/src/languages/rego/compiler/program.rs +++ b/src/languages/rego/compiler/program.rs @@ -181,10 +181,6 @@ impl<'a> Compiler<'a> { } fn evaluate_default_rule(&mut self, rule_path: &str) -> Option { - if !self.policy.inner.default_rules.contains_key(rule_path) { - return None; - } - let mut interpreter = Interpreter::new_from_compiled_policy(self.policy.inner.clone()); match interpreter.eval_default_rule_for_compiler(rule_path) { diff --git a/src/languages/rego/compiler/rules.rs b/src/languages/rego/compiler/rules.rs index ad0efcc..df2c0ae 100644 --- a/src/languages/rego/compiler/rules.rs +++ b/src/languages/rego/compiler/rules.rs @@ -56,7 +56,12 @@ impl<'a> Compiler<'a> { match head { RuleHead::Set { .. } => RuleType::PartialSet, RuleHead::Compr { refr, .. } => match refr.as_ref() { - crate::ast::Expr::RefBrack { .. } => RuleType::PartialObject, + crate::ast::Expr::RefBrack { index, .. } + if super::expressions::try_eval_const(index.as_ref()).is_none() => + { + RuleType::PartialObject + } + crate::ast::Expr::RefBrack { .. } => RuleType::Complete, _ => RuleType::Complete, }, _ => RuleType::Complete, diff --git a/src/rvm/vm/rules.rs b/src/rvm/vm/rules.rs index a30af85..690a26c 100644 --- a/src/rvm/vm/rules.rs +++ b/src/rvm/vm/rules.rs @@ -17,8 +17,9 @@ use super::execution_model::{ use super::machine::RegoVM; impl RegoVM { - /// Returns true if the error represents a resource-limit violation that - /// must never be silently absorbed by rule evaluation. + /// Returns true if the error must never be silently absorbed by rule + /// evaluation backtracking, including resource-limit failures and semantic + /// rule consistency errors. pub(super) const fn is_fatal_vm_error(err: &VmError) -> bool { matches!( err, diff --git a/tests/interpreter/cases/default/basic.yaml b/tests/interpreter/cases/default/basic.yaml index 1ea298f..325cc33 100644 --- a/tests/interpreter/cases/default/basic.yaml +++ b/tests/interpreter/cases/default/basic.yaml @@ -341,6 +341,35 @@ cases: result: false reasons: [] + - note: default_rule_with_object_key + data: {} + input: {} + modules: + - | + package test + import rego.v1 + default config["timeout"] := 30 + config["timeout"] := input.val if { + val := input.val + } + query: data.test.config.timeout + want_result: 30 + + - note: default_rule_with_object_key_override + data: {} + input: + val: 60 + modules: + - | + package test + import rego.v1 + default config["timeout"] := 30 + config["timeout"] := input.val if { + val := input.val + } + query: data.test.config.timeout + want_result: 60 + - note: default_only_rule_with_package_query data: {} modules: diff --git a/tests/interpreter/cases/rule/partial_object_iteration.yaml b/tests/interpreter/cases/rule/partial_object_iteration.yaml index 438baca..ce2e153 100644 --- a/tests/interpreter/cases/rule/partial_object_iteration.yaml +++ b/tests/interpreter/cases/rule/partial_object_iteration.yaml @@ -114,6 +114,26 @@ cases: BAZ: true FOO: true + - note: constant_key_implicit_true_rule_is_complete_v1 + data: {} + input: + enabled: true + other: false + modules: + - | + package test + import rego.v1 + + p["x"] if { + input.enabled + } + + p["x"] if { + input.other + } + query: data.test.p.x + want_result: true + - note: partial_object_duplicate_keys_same_value_are_ok_v1 data: {} input: diff --git a/tests/rvm/rego/cases/default_rules.yaml b/tests/rvm/rego/cases/default_rules.yaml index 2aee74b..4857f0d 100644 --- a/tests/rvm/rego/cases/default_rules.yaml +++ b/tests/rvm/rego/cases/default_rules.yaml @@ -48,18 +48,34 @@ cases: want_result: true - note: default_rule_with_object_key - skip: true # TODO: Fix rule type classification for config["timeout"] - should be Complete, not PartialObject data: {} + input: {} modules: - | package test + import rego.v1 default config["timeout"] := 30 - config["timeout"] := 60 if { - false # This will fail + config["timeout"] := input.val if { + val := input.val } query: data.test.config.timeout want_result: 30 + - note: default_rule_with_object_key_override + data: {} + input: + val: 60 + modules: + - | + package test + import rego.v1 + default config["timeout"] := 30 + config["timeout"] := input.val if { + val := input.val + } + query: data.test.config.timeout + want_result: 60 + - note: default_rule_complex_value data: {} modules: diff --git a/tests/rvm/rego/cases/partial_object_rules.yaml b/tests/rvm/rego/cases/partial_object_rules.yaml index eeacb99..0a1f7d2 100644 --- a/tests/rvm/rego/cases/partial_object_rules.yaml +++ b/tests/rvm/rego/cases/partial_object_rules.yaml @@ -109,6 +109,26 @@ cases: BAZ: true FOO: true + - note: constant_key_implicit_true_rule_is_complete_v1 + data: {} + input: + enabled: true + other: false + modules: + - | + package test + import rego.v1 + + p["x"] if { + input.enabled + } + + p["x"] if { + input.other + } + query: data.test.p.x + want_result: true + - note: partial_object_duplicate_keys_same_value_are_ok_v1 data: {} input: diff --git a/tests/rvm/vm/suites/object_operations.yaml b/tests/rvm/vm/suites/object_operations.yaml index b7085e6..fe3dc64 100644 --- a/tests/rvm/vm/suites/object_operations.yaml +++ b/tests/rvm/vm/suites/object_operations.yaml @@ -8,7 +8,7 @@ cases: - note: object_key_collision_conflict description: Setting same key twice with different values should raise a rule output conflict - example_rego: "p[key] := value if { value := [1, 2][_] }" + example_rego: "p[\"key\"] := value if { value := [1, 2][_] }" literals: - {} - "key" @@ -30,6 +30,28 @@ cases: - "Return { value: 0 }" want_error: "multiple outputs" + - note: object_key_duplicate_same_value + description: Setting same key twice with the same value should succeed + example_rego: "p[\"key\"] := 1 if { some _ in [0, 1] }" + literals: + - {} + - "key" + - 1 + instruction_params: + object_create_params: + - dest: 0 + template_literal_idx: 0 + literal_key_fields: [] + fields: [] + instructions: + - "ObjectCreate { params_index: 0 }" + - "Load { dest: 1, literal_idx: 1 }" # key + - "Load { dest: 2, literal_idx: 2 }" # value + - "ObjectSet { obj: 0, key: 1, value: 2 }" + - "ObjectSet { obj: 0, key: 1, value: 2 }" + - "Return { value: 0 }" + want_result: {"key": 1} + - note: object_dynamic_key_generation description: Generate object keys dynamically from loop iteration example_rego: "{sprintf(\"key_%d\", [i]): i | i := [1, 2][_]}"