Skip to content

Commit

Permalink
errors: replace anyhow errors with thiserror
Browse files Browse the repository at this point in the history
fixes microsoft#161

This change replaces stringly-typed anyhow errors with
thiserror-constructed explicit error enums.

The following strategy has been used in the conversion:

- Every module maintains its own error enum
- Exception is builtins/*.rs in which all errors are consolidated in one
  enum to avoid duplication
- Source errors are being wrapped and propagated in the error string
- Crate-specific businesslogic errors have been convernted into their
  own enum
- A simple `bail!` macro has been added to reduce changes
- Extension fn's require a `Box<dyn std::error::Error>` error type. it
  would be nice if the Extension fn's could have a result with trait
  bound `std::error::Error` but that requires some unergonomic type
  gymnastics on the trait + impls
- A local type alias for `Result` has been added to modules to reduce
  changes in the Signatures where reasonable.

Signed-off-by: Magnus Kulke <magnuskulke@microsoft.com>
  • Loading branch information
mkulke committed Apr 24, 2024
1 parent 7fde338 commit 69fca12
Show file tree
Hide file tree
Showing 46 changed files with 965 additions and 480 deletions.
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,7 @@ Cargo.lock
.vscode/

# worktrees
worktrees/
worktrees/

# vim files
.vim
3 changes: 2 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ full-opa = [
opa-testutil = []

[dependencies]
anyhow = { version = "1.0.45", default-features=false }
serde = {version = "1.0.150", features = ["derive", "rc"] }
serde_json = "1.0.89"
serde_yaml = {version = "0.9.16", optional = true }
Expand Down Expand Up @@ -98,8 +97,10 @@ chrono = { version = "0.4.31", optional = true }
chrono-tz = { version = "0.8.5", optional = true }
jsonwebtoken = { version = "9.2.0", optional = true }
itertools = "0.12.1"
thiserror = "1.0.59"

[dev-dependencies]
anyhow = "1.0"
cfg-if = "1.0.0"
clap = { version = "4.4.7", features = ["derive"] }
prettydiff = { version = "0.6.4", default-features = false }
Expand Down
1 change: 0 additions & 1 deletion bindings/ffi/RegorusFFI.g.cs
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,3 @@ internal enum RegorusStatus : uint


}

4 changes: 3 additions & 1 deletion src/builtins/aggregates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,17 @@
// Licensed under the MIT License.

use crate::ast::{Expr, Ref};
use crate::bail;
use crate::builtins;
use crate::builtins::utils::{ensure_args_count, ensure_numeric};
use crate::builtins::BuiltinError;
use crate::lexer::Span;
use crate::number::Number;
use crate::value::Value;

use std::collections::HashMap;

use anyhow::{bail, Result};
type Result<T> = std::result::Result<T, BuiltinError>;

pub fn register(m: &mut HashMap<&'static str, builtins::BuiltinFcn>) {
m.insert("count", (count, 1));
Expand Down
24 changes: 19 additions & 5 deletions src/builtins/arrays.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,25 @@
use crate::ast::{Expr, Ref};
use crate::builtins;
use crate::builtins::utils::{ensure_args_count, ensure_array, ensure_numeric};
use crate::builtins::BuiltinError;
use crate::lexer::Span;
use crate::Rc;
use crate::Value;

use std::collections::HashMap;

use anyhow::Result;

pub fn register(m: &mut HashMap<&'static str, builtins::BuiltinFcn>) {
m.insert("array.concat", (concat, 2));
m.insert("array.reverse", (reverse, 1));
m.insert("array.slice", (slice, 3));
}

fn concat(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> {
fn concat(
span: &Span,
params: &[Ref<Expr>],
args: &[Value],
_strict: bool,
) -> Result<Value, BuiltinError> {
let name = "array.concat";
ensure_args_count(span, name, params, args, 2)?;
let mut v1 = ensure_array(name, &params[0], args[0].clone())?;
Expand All @@ -28,7 +32,12 @@ fn concat(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> R
Ok(Value::Array(v1))
}

fn reverse(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> {
fn reverse(
span: &Span,
params: &[Ref<Expr>],
args: &[Value],
_strict: bool,
) -> Result<Value, BuiltinError> {
let name = "array.reverse";
ensure_args_count(span, name, params, args, 1)?;

Expand All @@ -37,7 +46,12 @@ fn reverse(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) ->
Ok(Value::Array(v1))
}

fn slice(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> {
fn slice(
span: &Span,
params: &[Ref<Expr>],
args: &[Value],
_strict: bool,
) -> Result<Value, BuiltinError> {
let name = "array.slice";
ensure_args_count(span, name, params, args, 3)?;

Expand Down
45 changes: 37 additions & 8 deletions src/builtins/bitwise.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@
use crate::ast::{Expr, Ref};
use crate::builtins;
use crate::builtins::utils::{ensure_args_count, ensure_numeric};
use crate::builtins::BuiltinError;

use crate::lexer::Span;
use crate::value::Value;

use std::collections::HashMap;

use anyhow::Result;

pub fn register(m: &mut HashMap<&'static str, builtins::BuiltinFcn>) {
m.insert("bits.and", (and, 2));
m.insert("bits.lsh", (lsh, 2));
Expand All @@ -21,7 +20,12 @@ pub fn register(m: &mut HashMap<&'static str, builtins::BuiltinFcn>) {
m.insert("bits.xor", (xor, 2));
}

fn and(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> {
fn and(
span: &Span,
params: &[Ref<Expr>],
args: &[Value],
_strict: bool,
) -> Result<Value, BuiltinError> {
let name = "bits.and";
ensure_args_count(span, name, params, args, 2)?;

Expand All @@ -34,7 +38,12 @@ fn and(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Resu
})
}

fn lsh(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> {
fn lsh(
span: &Span,
params: &[Ref<Expr>],
args: &[Value],
_strict: bool,
) -> Result<Value, BuiltinError> {
let name = "bits.lsh";
ensure_args_count(span, name, params, args, 2)?;

Expand All @@ -47,7 +56,12 @@ fn lsh(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Resu
})
}

fn negate(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> {
fn negate(
span: &Span,
params: &[Ref<Expr>],
args: &[Value],
_strict: bool,
) -> Result<Value, BuiltinError> {
let name = "bits.negate";
ensure_args_count(span, name, params, args, 1)?;

Expand All @@ -59,7 +73,12 @@ fn negate(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> R
})
}

fn or(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> {
fn or(
span: &Span,
params: &[Ref<Expr>],
args: &[Value],
_strict: bool,
) -> Result<Value, BuiltinError> {
let name = "bits.or";
ensure_args_count(span, name, params, args, 2)?;

Expand All @@ -72,7 +91,12 @@ fn or(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Resul
})
}

fn rsh(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> {
fn rsh(
span: &Span,
params: &[Ref<Expr>],
args: &[Value],
_strict: bool,
) -> Result<Value, BuiltinError> {
let name = "bits.rsh";
ensure_args_count(span, name, params, args, 2)?;

Expand All @@ -85,7 +109,12 @@ fn rsh(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Resu
})
}

fn xor(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -> Result<Value> {
fn xor(
span: &Span,
params: &[Ref<Expr>],
args: &[Value],
_strict: bool,
) -> Result<Value, BuiltinError> {
let name = "bits.xor";
ensure_args_count(span, name, params, args, 2)?;

Expand Down
3 changes: 2 additions & 1 deletion src/builtins/comparison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
// Licensed under the MIT License.

use crate::ast::BoolOp;
use crate::builtins::BuiltinError;
use crate::value::Value;

use anyhow::Result;
type Result<T> = std::result::Result<T, BuiltinError>;

/// compare two values
///
Expand Down
4 changes: 3 additions & 1 deletion src/builtins/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@
// Licensed under the MIT License.

use crate::ast::{Expr, Ref};
use crate::bail;
use crate::builtins;
use crate::builtins::utils::ensure_args_count;
use crate::builtins::BuiltinError;
use crate::lexer::Span;
use crate::value::Value;

use std::collections::HashMap;

use anyhow::{bail, Result};
type Result<T> = std::result::Result<T, BuiltinError>;

pub fn register(m: &mut HashMap<&'static str, builtins::BuiltinFcn>) {
m.insert("to_number", (to_number, 1));
Expand Down
25 changes: 16 additions & 9 deletions src/builtins/crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,23 @@
// Licensed under the MIT License.

use crate::ast::{Expr, Ref};
use crate::bail;
use crate::builtins;
use crate::builtins::utils::{ensure_args_count, ensure_string};
use crate::builtins::BuiltinError;
use crate::lexer::Span;
use crate::value::Value;

use std::collections::HashMap;

use anyhow::{bail, Result};
use constant_time_eq::constant_time_eq;
use hmac::{Hmac, Mac};
use md5::{Digest, Md5};
use sha1::Sha1;
use sha2::{Sha256, Sha512};

type Result<T> = std::result::Result<T, BuiltinError>;

pub fn register(m: &mut HashMap<&'static str, builtins::BuiltinFcn>) {
m.insert("crypto.hmac.equal", (hmac_equal_fixed_time, 2));
m.insert("crypto.hmac.md5", (hmac_md5, 2));
Expand Down Expand Up @@ -53,8 +56,9 @@ fn hmac_md5(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) ->
let x = ensure_string(name, &params[0], &args[0])?;
let key = ensure_string(name, &params[1], &args[1])?;

let mut hmac = Hmac::<Md5>::new_from_slice(key.as_bytes())
.or_else(|_| bail!(span.error("failed to create hmac instance")))?;
let Ok(mut hmac) = Hmac::<Md5>::new_from_slice(key.as_bytes()) else {
bail!(span.error("failed to create md5 hmac instance"));
};

hmac.update(x.as_bytes());
let result = hmac.finalize();
Expand All @@ -69,8 +73,9 @@ fn hmac_sha1(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool) -
let x = ensure_string(name, &params[0], &args[0])?;
let key = ensure_string(name, &params[1], &args[1])?;

let mut hmac = Hmac::<Sha1>::new_from_slice(key.as_bytes())
.or_else(|_| bail!(span.error("failed to create hmac instance")))?;
let Ok(mut hmac) = Hmac::<Sha1>::new_from_slice(key.as_bytes()) else {
bail!(span.error("failed to sha1 create hmac instance"));
};

hmac.update(x.as_bytes());
let result = hmac.finalize();
Expand All @@ -85,8 +90,9 @@ fn hmac_sha256(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool)
let x = ensure_string(name, &params[0], &args[0])?;
let key = ensure_string(name, &params[1], &args[1])?;

let mut hmac = Hmac::<Sha256>::new_from_slice(key.as_bytes())
.or_else(|_| bail!(span.error("failed to create hmac instance")))?;
let Ok(mut hmac) = Hmac::<Sha256>::new_from_slice(key.as_bytes()) else {
bail!(span.error("failed to create sha256 hmac instance"));
};

hmac.update(x.as_bytes());
let result = hmac.finalize();
Expand All @@ -101,8 +107,9 @@ fn hmac_sha512(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool)
let x = ensure_string(name, &params[0], &args[0])?;
let key = ensure_string(name, &params[1], &args[1])?;

let mut hmac = Hmac::<Sha512>::new_from_slice(key.as_bytes())
.or_else(|_| bail!(span.error("failed to create hmac instance")))?;
let Ok(mut hmac) = Hmac::<Sha512>::new_from_slice(key.as_bytes()) else {
bail!(span.error("failed to create sha512 hmac instance"));
};

hmac.update(x.as_bytes());
let result = hmac.finalize();
Expand Down
4 changes: 3 additions & 1 deletion src/builtins/debugging.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
// Licensed under the MIT License.

use crate::ast::{Expr, Ref};
use crate::bail;
use crate::builtins;
use crate::builtins::BuiltinError;
use crate::lexer::Span;
use crate::value::Value;

use std::collections::HashMap;

use anyhow::{bail, Result};
type Result<T> = std::result::Result<T, BuiltinError>;

// TODO: Should we avoid this limit?
const MAX_ARGS: u8 = std::u8::MAX;
Expand Down
5 changes: 4 additions & 1 deletion src/builtins/deprecated.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,22 @@
// Licensed under the MIT License.

use crate::ast::{Expr, Ref};
use crate::bail;
use crate::builtins::utils::{ensure_args_count, ensure_set};
use crate::builtins::BuiltinError;
use crate::builtins::BuiltinFcn;
use crate::lexer::Span;
use crate::value::Value;

use std::collections::HashMap;

use anyhow::{bail, Result};
use lazy_static::lazy_static;

#[cfg(feature = "regex")]
use crate::builtins::regex::regex_match;

type Result<T> = std::result::Result<T, BuiltinError>;

#[rustfmt::skip]
lazy_static! {
pub static ref DEPRECATED: HashMap<&'static str, BuiltinFcn> = {
Expand Down
Loading

0 comments on commit 69fca12

Please sign in to comment.