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 <[email protected]>
  • Loading branch information
mkulke committed Apr 24, 2024
1 parent 7fde338 commit 52fefb1
Show file tree
Hide file tree
Showing 46 changed files with 930 additions and 473 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
3 changes: 2 additions & 1 deletion src/builtins/bitwise.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@
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;
type Result<T> = std::result::Result<T, BuiltinError>;

pub fn register(m: &mut HashMap<&'static str, builtins::BuiltinFcn>) {
m.insert("bits.and", (and, 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
21 changes: 14 additions & 7 deletions src/builtins/encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,21 @@
// Licensed under the MIT License.

use crate::ast::{Expr, Ref};
use crate::bail;
use crate::builtins;
#[allow(unused)]
use crate::builtins::utils::{
ensure_args_count, ensure_object, ensure_string, ensure_string_collection,
};
use crate::builtins::BuiltinError;
use crate::lexer::Span;
use crate::value::Value;

use std::collections::HashMap;

#[allow(unused)]
use anyhow::{anyhow, bail, Context, Result};
pub(crate) use data_encoding::DecodeError;

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

pub fn register(m: &mut HashMap<&'static str, builtins::BuiltinFcn>) {
#[cfg(feature = "base64")]
Expand Down Expand Up @@ -119,7 +122,7 @@ fn base64url_decode(
{
data_encoding::BASE64URL_NOPAD
.decode(encoded_str.as_bytes())
.map_err(|_| anyhow!(params[0].span().error("not a valid url")))?
.map_err(|_| params[0].span().error("not a valid url"))?
}
#[cfg(not(feature = "base64url"))]
{
Expand Down Expand Up @@ -327,7 +330,7 @@ fn yaml_marshal(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool
ensure_args_count(span, name, params, args, 1)?;
Ok(Value::String(
serde_yaml::to_string(&args[0])
.with_context(|| span.error("could not serialize to yaml"))?
.map_err(|_| BuiltinError::SerializeFailed(span.error("could not serialize to yaml")))?
.into(),
))
}
Expand All @@ -342,7 +345,9 @@ fn yaml_unmarshal(
let name = "yaml.unmarshal";
ensure_args_count(span, name, params, args, 1)?;
let yaml_str = ensure_string(name, &params[0], &args[0])?;
Value::from_yaml_str(&yaml_str).with_context(|| span.error("could not deserialize yaml."))
let value = Value::from_yaml_str(&yaml_str)
.map_err(|_| BuiltinError::DeserializeFailed(span.error("could not deserialize yaml")))?;
Ok(value)
}

fn json_is_valid(
Expand All @@ -363,7 +368,7 @@ fn json_marshal(span: &Span, params: &[Ref<Expr>], args: &[Value], _strict: bool
ensure_args_count(span, name, params, args, 1)?;
Ok(Value::String(
serde_json::to_string(&args[0])
.with_context(|| span.error("could not serialize to json"))?
.map_err(|_| BuiltinError::SerializeFailed(span.error("could not serialize to json")))?
.into(),
))
}
Expand All @@ -377,5 +382,7 @@ fn json_unmarshal(
let name = "json.unmarshal";
ensure_args_count(span, name, params, args, 1)?;
let json_str = ensure_string(name, &params[0], &args[0])?;
Value::from_json_str(&json_str).with_context(|| span.error("could not deserialize json."))
let value = Value::from_json_str(&json_str)
.map_err(|_| BuiltinError::DeserializeFailed(span.error("could not deserialize json")))?;
Ok(value)
}
6 changes: 4 additions & 2 deletions src/builtins/glob.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, ensure_string, ensure_string_collection};
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>;
//use glob::{Pattern, MatchOptions};
use wax::{Glob, Pattern};

Expand All @@ -28,7 +30,7 @@ fn suppress_unix_style_delimiter(s: &str) -> Result<String> {

fn make_delimiters_unix_style(s: &str, delimiters: &[char]) -> Result<String> {
if s.contains(PLACE_HOLDER) {
bail!("string contains internal glob placeholder");
return Err(BuiltinError::StringContainsGlobPattern);
}

let has_unix_style = delimiters.contains(&'/');
Expand Down
8 changes: 6 additions & 2 deletions src/builtins/graph.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, ensure_object};
use crate::builtins::BuiltinError;
use crate::lexer::Span;
use crate::value::Value;

use std::collections::{BTreeMap, BTreeSet, 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("graph.reachable", (reachable, 2));
Expand Down Expand Up @@ -94,7 +96,9 @@ fn visit(
set.len()
}
Some(&Value::Null) => 0,
_ => bail!(format!("neighbors for node `{node}` must be array/set.")),
_ => {
return Err(BuiltinError::WrongNeighbours(node.clone()));
}
};

if n == 0 {
Expand Down
Loading

0 comments on commit 52fefb1

Please sign in to comment.