From 25c106f866a6961d765743a550c8da52b29a0cc9 Mon Sep 17 00:00:00 2001 From: Cal Stepanian <61707847+cstepanian@users.noreply.github.com> Date: Wed, 23 Oct 2024 18:45:27 -0400 Subject: [PATCH] feat: Evaluate JSON Pointers using late binding This commit replaces the previous preprocessor-based implementation of JSON Pointers with one that natively runs in the Policy Expression `Executor`. It uses the ExprMutator visitor trait to evaluate JSON Pointers in two steps: first, pointers are looked up but the original pointer string is preserved. Then, the looked-up value is inserted. This structure is meant to allow for better error messages and user-facing explanations of why a Policy Expression was evaluated a given way. --- hipcheck/src/policy_exprs/error.rs | 4 + hipcheck/src/policy_exprs/expr.rs | 20 +- hipcheck/src/policy_exprs/json_pointer.rs | 219 ++++++---------------- hipcheck/src/policy_exprs/mod.rs | 43 ++++- hipcheck/src/policy_exprs/token.rs | 1 + 5 files changed, 116 insertions(+), 171 deletions(-) diff --git a/hipcheck/src/policy_exprs/error.rs b/hipcheck/src/policy_exprs/error.rs index 387aca53..6469cfd3 100644 --- a/hipcheck/src/policy_exprs/error.rs +++ b/hipcheck/src/policy_exprs/error.rs @@ -15,6 +15,10 @@ pub enum Error { #[error("Multiple errors: {0:?}")] MultipleErrors(Vec), + #[error("internal error: '{0}'")] + #[allow(clippy::enum_variant_names)] + InternalError(String), + #[error("missing close paren")] MissingOpenParen, diff --git a/hipcheck/src/policy_exprs/expr.rs b/hipcheck/src/policy_exprs/expr.rs index f7a29e8f..019c0510 100644 --- a/hipcheck/src/policy_exprs/expr.rs +++ b/hipcheck/src/policy_exprs/expr.rs @@ -139,8 +139,9 @@ pub struct Ident(pub String); /// A late-binding for a JSON pointer #[derive(Debug, Clone, PartialEq, Eq)] pub struct JsonPointer { - pointer: String, - value: Option, + /// The JSON Pointer source string, without the initial '$' character. + pub pointer: String, + pub value: Option>, } impl From for Expr { fn from(value: JsonPointer) -> Self { @@ -309,6 +310,14 @@ pub fn parse(input: &str) -> Result { } } +#[cfg(test)] +pub fn json_ptr(name: &str) -> Expr { + Expr::JsonPointer(JsonPointer { + pointer: String::from(name), + value: None, + }) +} + #[cfg(test)] mod tests { use super::*; @@ -365,13 +374,6 @@ mod tests { Array::new(vals).into() } - fn json_ptr(name: &str) -> Expr { - Expr::JsonPointer(JsonPointer { - pointer: String::from(name), - value: None, - }) - } - #[test] fn parse_bool() { let input = "#t"; diff --git a/hipcheck/src/policy_exprs/json_pointer.rs b/hipcheck/src/policy_exprs/json_pointer.rs index fb1c79b2..1b14c7be 100644 --- a/hipcheck/src/policy_exprs/json_pointer.rs +++ b/hipcheck/src/policy_exprs/json_pointer.rs @@ -1,81 +1,16 @@ // SPDX-License-Identifier: Apache-2.0 +#![deny(unused)] + use crate::policy_exprs::{ error, error::{Error, Result}, expr::{Array, Expr, Primitive}, + ExprMutator, JsonPointer, }; use ordered_float::NotNan; -use regex::{Captures, Regex, RegexBuilder}; use serde_json::Value; -/// Preprocess a Policy Expr source string by replacing JSON Pointer syntax with -/// values looked up from the `context` data. -pub(crate) fn process_json_pointers(raw_program: &str, context: &Value) -> Result { - let re = json_pointer_regex(); - let mut any_error: bool = false; - let mut errors: Vec = Vec::new(); - let result = re.replace_all(raw_program, |caps: &Captures| { - let pointer = &caps[1]; - let res = process_pointer(pointer, context); - match res { - Ok(expr) => expr, - Err(e) => { - any_error = true; - errors.push(e); - // Return a bogus string from the closure for Regex.replace_all to use. - // The final string should never be used in that case. - "ERROR".into() - } - } - }); - - if any_error { - if errors.len() > 1 { - Err(Error::MultipleErrors(errors)) - } else { - Err(errors.remove(0)) - } - } else { - Ok(result.into_owned()) - } -} - -/// Return the Regex used for parsing JSON pointers embedded in a Policy Expression. -/// Note that the initial $ is not captured. -/// A valid JSON Pointer must be either empty or start with '/', but this regex -/// still captures invalid syntax to provide better error handling. -fn json_pointer_regex() -> Regex { - let pat = r" - # JSON pointers embedded in policy expressions are signified by $. - # It is not part of JSON pointer syntax, so it is not captured. - \$ - ( - [ - / - ~ - _ - [:alnum:] - ] - * - ) - "; - // Panic safety: the regex is static and programmer-defined. - // It is considered a programmer error if the regex syntax is invalid. - RegexBuilder::new(pat) - .ignore_whitespace(true) - .build() - .unwrap() -} - -/// Lookup a single JSON Pointer reference and convert it to Policy Expr syntax, -/// if possible. -fn process_pointer(pointer: &str, context: &Value) -> Result { - let val = lookup_json_pointer(pointer, context)?; - let expr = json_to_policy_expr(val, pointer, context)?; - Ok(expr.to_string()) -} - /// Wrap serde_json's `Value::pointer` method to provide better error handling. fn lookup_json_pointer<'val>(pointer: &str, context: &'val Value) -> Result<&'val Value> { // serde_json's JSON Pointer implementation does not distinguish between @@ -157,71 +92,78 @@ fn json_array_item_to_policy_expr_primitive( } } -#[cfg(test)] -mod tests { - use super::*; - use test_log::test; - - fn parse_json_pointer(src: &str) -> Option<(&str, &str)> { - json_pointer_regex().captures(src).map(|caps| { - let (whole, [cap]) = caps.extract(); - (whole, cap) - }) - } +/// Policy Expression stage that looks up JSON Pointers from the JSON `context` +/// value. +pub struct LookupJsonPointers<'ctx> { + context: &'ctx Value, +} - #[test] - fn parse_basic_slashes() { - let src = "(eq 1 $/data/one)"; - let matches = parse_json_pointer(src); - assert_eq!(matches, Some(("$/data/one", "/data/one"))); +impl<'ctx> LookupJsonPointers<'ctx> { + pub fn with_context(context: &'ctx Value) -> Self { + LookupJsonPointers { context } } +} - #[test] - fn basic_float() { - let program = "$"; - let context = serde_json::json!(2.3); - let processed = process_json_pointers(program, &context).unwrap(); - assert_eq!(processed, "2.3"); +impl<'ctx> ExprMutator for LookupJsonPointers<'ctx> { + fn visit_json_pointer(&self, mut jp: JsonPointer) -> Result { + let pointer = &jp.pointer; + let context = self.context; + let val = lookup_json_pointer(pointer, context)?; + let expr = json_to_policy_expr(val, pointer, context)?; + jp.value = Some(Box::new(expr)); + Ok(jp.into()) } +} - #[test] - fn basic_bool() { - let program = "$"; - let context = serde_json::json!(true); - let processed = process_json_pointers(program, &context).unwrap(); - assert_eq!(processed, "#t"); - } +#[cfg(test)] +mod tests { + use super::*; + use crate::policy_exprs::expr::json_ptr; + use crate::policy_exprs::F64; + use test_log::test; #[test] - fn underscore() { - let program = "(lte 0.05 $/pct_reviewed)"; - let context = serde_json::json!({ - "pct_reviewed": 0.15, + /// LookupJsonPointers writes to `value` in the JsonPointer Expr. + fn toplevel_bool_set() { + let expr = Expr::JsonPointer(JsonPointer { + pointer: "".to_owned(), + value: None, }); - let processed = process_json_pointers(program, &context).unwrap(); - assert_eq!(processed, "(lte 0.05 0.15)"); + let val = serde_json::json!(true); + let expected = Expr::JsonPointer(JsonPointer { + pointer: "".to_owned(), + value: Some(Box::new(Primitive::Bool(true).into())), + }); + + let result = LookupJsonPointers::with_context(&val).visit_expr(expr); + assert_eq!(result, Ok(expected)) } #[test] - fn multiple() { - let program = "$/alpha $/bravo $/charlie"; - let context = serde_json::json!({ - "alpha": 4.5, - "bravo": false, - "charlie": [0, 1, 2, 3], + /// LookupJsonPointers writes to `value` in the JsonPointer Expr. + fn toplevel_f64_set() { + let expr = Expr::JsonPointer(JsonPointer { + pointer: "".to_owned(), + value: None, + }); + let val = serde_json::json!(1.23); + let expected = Expr::JsonPointer(JsonPointer { + pointer: "".to_owned(), + value: Some(Box::new(Primitive::Float(F64::new(1.23).unwrap()).into())), }); - let processed = process_json_pointers(program, &context).unwrap(); - assert_eq!(processed, "4.5 #f [0 1 2 3]"); + + let result = LookupJsonPointers::with_context(&val).visit_expr(expr); + assert_eq!(result, Ok(expected)) } #[test] fn error_lookup_failed() { // Note spelling - let program = "$/alpa"; + let expr = json_ptr("/alpa"); let context = serde_json::json!({ "alpha": 4.5, }); - let result = process_json_pointers(program, &context); + let result = LookupJsonPointers::with_context(&context).visit_expr(expr); assert_eq!( result, Err(Error::JSONPointerLookupFailed { @@ -231,52 +173,13 @@ mod tests { ); } - #[test] - fn error_invalid_syntax() { - // Note missing '/' at beginning of pointer - let program = "$alpha"; - let context = serde_json::json!({ - "alpha": 4.5, - }); - let result = process_json_pointers(program, &context); - assert_eq!( - result, - Err(Error::JSONPointerInvalidSyntax { - pointer: "alpha".into() - }) - ); - } - - #[test] - fn multiple_errors() { - let program = "$/alpa $/brave"; - let context = serde_json::json!({ - "alpha": 4.5, - "bravo": false, - }); - let result = process_json_pointers(program, &context); - assert_eq!( - result, - Err(Error::MultipleErrors(vec![ - Error::JSONPointerLookupFailed { - pointer: "/alpa".into(), - context: context.clone(), - }, - Error::JSONPointerLookupFailed { - pointer: "/brave".into(), - context: context.clone(), - }, - ])) - ); - } - #[test] fn error_unrepresentable_string() { - let program = "$/str"; + let expr = json_ptr("/str"); let context = serde_json::json!({ "str": "Hello World!", }); - let result = process_json_pointers(program, &context); + let result = LookupJsonPointers::with_context(&context).visit_expr(expr); assert_eq!( result, Err(Error::JSONPointerUnrepresentableType { @@ -290,14 +193,14 @@ mod tests { #[test] fn error_unrepresentable_object() { - let program = "$/obj"; + let expr = json_ptr("/obj"); let context = serde_json::json!({ "obj": { "a": 4.5, "b": true, } }); - let result = process_json_pointers(program, &context); + let result = LookupJsonPointers::with_context(&context).visit_expr(expr); assert_eq!( result, Err(Error::JSONPointerUnrepresentableType { @@ -311,11 +214,11 @@ mod tests { #[test] fn error_unrepresentable_array_nonprimitive() { - let program = "$/array"; + let expr = json_ptr("/array"); let context = serde_json::json!({ "array": [0, [5, 10], 100], }); - let result = process_json_pointers(program, &context); + let result = LookupJsonPointers::with_context(&context).visit_expr(expr); assert_eq!( result, Err(Error::JSONPointerUnrepresentableType { diff --git a/hipcheck/src/policy_exprs/mod.rs b/hipcheck/src/policy_exprs/mod.rs index 017be102..1a9c21b4 100644 --- a/hipcheck/src/policy_exprs/mod.rs +++ b/hipcheck/src/policy_exprs/mod.rs @@ -20,7 +20,7 @@ pub use crate::policy_exprs::{ }; use env::Binding; pub use expr::{parse, Primitive}; -use json_pointer::process_json_pointers; +use json_pointer::LookupJsonPointers; use serde_json::Value; use std::ops::Deref; @@ -45,9 +45,10 @@ impl Executor { /// Run a `deke` program, but don't try to convert the result to a `bool`. pub fn parse_and_eval(&self, raw_program: &str, context: &Value) -> Result { - let processed_program = process_json_pointers(raw_program, context)?; - let program = parse(&processed_program)?; - let expr = self.env.visit_expr(program)?; + let program = parse(raw_program)?; + // JSON Pointer lookup failures happen on this line. + let processed_program = LookupJsonPointers::with_context(context).run(program)?; + let expr = self.env.visit_expr(processed_program)?; Ok(expr) } } @@ -68,6 +69,18 @@ impl ExprMutator for Env<'_> { fn visit_lambda(&self, l: Lambda) -> Result { Ok((*l.body).clone()) } + fn visit_json_pointer(&self, jp: JsonPointer) -> Result { + let expr = &jp.value; + match expr { + None => Err(Error::InternalError(format!( + "JsonPointer's `value` field was not set. \ + All `value` fields must be set by `LookupJsonPointers` before evaluation. \ + JsonPointer: {:?}", + &jp + ))), + Some(expr) => Ok(*expr.to_owned()), + } + } } #[cfg(test)] @@ -75,6 +88,20 @@ mod tests { use super::*; use test_log::test; + #[test] + fn visitor_replaces_json_pointer() { + // Assume that json_pointer::LookupJsonPointers has already run, + // so `value` will contain an Expr. + let expr = Expr::JsonPointer(JsonPointer { + pointer: "".to_owned(), + value: Some(Box::new(Primitive::Bool(true).into())), + }); + let expected = Primitive::Bool(true).into(); + + let result = Env::std().visit_expr(expr); + assert_eq!(result, Ok(expected)) + } + #[test] fn run_bool() { let program = "#t"; @@ -83,6 +110,14 @@ mod tests { assert!(is_true); } + #[test] + fn run_jsonptr_bool() { + let program = "$"; + let context = Value::Bool(true); + let is_true = Executor::std().run(program, &context).unwrap(); + assert!(is_true); + } + #[test] fn run_basic() { let program = "(eq (add 1 2) 3)"; diff --git a/hipcheck/src/policy_exprs/token.rs b/hipcheck/src/policy_exprs/token.rs index c3bf2657..a3ae119e 100644 --- a/hipcheck/src/policy_exprs/token.rs +++ b/hipcheck/src/policy_exprs/token.rs @@ -146,6 +146,7 @@ fn lex_ident(input: &mut Lexer<'_, Token>) -> Result { } /// Lex a JSON Pointer. +/// The initial '$' character is removed. fn lex_json_pointer(input: &mut Lexer<'_, Token>) -> Result { let token = input.slice(); // Remove the initial '$' character.