Merge pull request #386 from Kuadrant/error_types

Proper `Error` types for the crate

limitador-server/Cargo.toml

@@ -20,7 +20,7 @@ distributed_storage = ["limitador/distributed_storage"]
 [dependencies]
 limitador = { path = "../limitador", features = ['lenient_conditions'] }
 tokio = { version = "1", features = ["full"] }
-thiserror = "1"
+thiserror = "2"
 tonic = "0.12.3"
 tonic-reflection = "0.12.3"
 prost = "0.13.3"
@@ -38,17 +38,17 @@ opentelemetry-otlp = "0.15"
 url = "2"
 actix-web = "4.1"
 actix-rt = "2"
-paperclip = { version = "0.8.0", features = ["actix4"] }
+paperclip = { version = "0.9", features = ["actix4"] }
 serde = { version = "1", features = ["derive"] }
-notify = "6.0.1"
+notify = "7"
 const_format = "0.2.31"
 lazy_static = "1.4.0"
 clap = "4.3"
-sysinfo = "0.30.10"
+sysinfo = "0.32"
 openssl = { version = "0.10.66", features = ["vendored"] }
 metrics = "0.22.3"
 metrics-exporter-prometheus = "0.14.0"
 
 
 [build-dependencies]
-tonic-build = "0.11"
+tonic-build = "0.12"

limitador-server/build.rs

@@ -15,7 +15,7 @@ fn generate_protobuf() -> Result<(), Box<dyn Error>> {
     tonic_build::configure()
         .build_server(true)
         .file_descriptor_set_path(original_out_dir.join("rls.bin"))
-        .compile(
+        .compile_protos(
             &["envoy/service/ratelimit/v3/rls.proto"],
             &[
                 "vendor/protobufs/data-plane-api",

limitador-server/src/envoy_rls/server.rs

@@ -255,7 +255,8 @@ mod tests {
             60,
             vec!["req.method == 'GET'"],
             vec!["app_id"],
-        );
+        )
+        .expect("This must be a valid limit!");
 
         let limiter = RateLimiter::new(10_000);
         limiter.add_limit(limit);
@@ -394,8 +395,10 @@ mod tests {
         let namespace = "test_namespace";
 
         vec![
-            Limit::new(namespace, 10, 60, vec!["x == '1'"], vec!["z"]),
-            Limit::new(namespace, 0, 60, vec!["x == '1'", "y == '2'"], vec!["z"]),
+            Limit::new(namespace, 10, 60, vec!["x == '1'"], vec!["z"])
+                .expect("This must be a valid limit!"),
+            Limit::new(namespace, 0, 60, vec!["x == '1'", "y == '2'"], vec!["z"])
+                .expect("This must be a valid limit!"),
         ]
         .into_iter()
         .for_each(|limit| {
@@ -459,7 +462,8 @@ mod tests {
     #[tokio::test]
     async fn test_takes_into_account_the_hits_addend_param() {
         let namespace = "test_namespace";
-        let limit = Limit::new(namespace, 10, 60, vec!["x == '1'"], vec!["y"]);
+        let limit = Limit::new(namespace, 10, 60, vec!["x == '1'"], vec!["y"])
+            .expect("This must be a valid limit!");
 
         let limiter = RateLimiter::new(10_000);
         limiter.add_limit(limit);
@@ -528,7 +532,8 @@ mod tests {
         // "hits_addend" is optional according to the spec, and should default
         // to 1, However, with the autogenerated structs it defaults to 0.
         let namespace = "test_namespace";
-        let limit = Limit::new(namespace, 1, 60, vec!["x == '1'"], vec!["y"]);
+        let limit = Limit::new(namespace, 1, 60, vec!["x == '1'"], vec!["y"])
+            .expect("This must be a valid limit!");
 
         let limiter = RateLimiter::new(10_000);
         limiter.add_limit(limit);

limitador-server/src/http_api/request_types.rs

@@ -1,9 +1,9 @@
 use limitador::counter::Counter as LimitadorCounter;
+use limitador::errors::LimitadorError;
 use limitador::limit::Limit as LimitadorLimit;
 use paperclip::actix::Apiv2Schema;
 use serde::{Deserialize, Serialize};
 use std::collections::{BTreeMap, HashMap};
-
 // We need to define the Limit and Counter types. They're basically the same as
 // defined in the lib but with some modifications to be able to derive
 // Apiv2Schema (needed to generate the OpenAPI specs).
@@ -41,8 +41,10 @@ impl From<&LimitadorLimit> for Limit {
     }
 }
 
-impl From<Limit> for LimitadorLimit {
-    fn from(limit: Limit) -> Self {
+impl TryFrom<Limit> for LimitadorLimit {
+    type Error = LimitadorError;
+
+    fn try_from(limit: Limit) -> Result<Self, Self::Error> {
         let mut limitador_limit = if let Some(id) = limit.id {
             Self::with_id(
                 id,
@@ -51,22 +53,22 @@ impl From<Limit> for LimitadorLimit {
                 limit.seconds,
                 limit.conditions,
                 limit.variables,
-            )
+            )?
         } else {
             Self::new(
                 limit.namespace,
                 limit.max_value,
                 limit.seconds,
                 limit.conditions,
                 limit.variables,
-            )
+            )?
         };
 
         if let Some(name) = limit.name {
             limitador_limit.set_name(name)
         }
 
-        limitador_limit
+        Ok(limitador_limit)
     }
 }
 

limitador-server/src/http_api/server.rs

@@ -555,7 +555,8 @@ mod tests {
             60,
             vec!["req.method == 'GET'"],
             vec!["app_id"],
-        );
+        )
+        .expect("This must be a valid limit!");
 
         match &limiter {
             Limiter::Blocking(limiter) => limiter.add_limit(limit.clone()),

limitador/Cargo.toml

@@ -21,18 +21,14 @@ lenient_conditions = []
 
 [dependencies]
 moka = { version = "0.12", features = ["sync"] }
-dashmap = "5.5.3"
-getrandom = { version = "0.2", features = ["js"] }
+dashmap = "6.1"
 serde = { version = "1", features = ["derive", "rc"] }
 postcard = { version = "1.0.4", features = ["use-std"] }
 serde_json = "1"
-rmp-serde = "1.1.0"
-thiserror = "1"
-futures = "0.3"
 async-trait = "0.1"
 cfg-if = "1"
 tracing = "0.1.40"
-metrics = "0.22.3"
+metrics = "0.24"
 
 # Optional dependencies
 rocksdb = { version = "0.22", optional = true, features = ["multi-threaded-cf"] }
@@ -52,13 +48,12 @@ tokio = { version = "1", optional = true, features = [
 
 base64 = { version = "0.22", optional = true }
 tokio-stream = { version = "0.1", optional = true }
-h2 = { version = "0.3", optional = true }
+h2 = { version = "0.4", optional = true }
 uuid = { version = "1.8.0", features = ["v4", "fast-rng"], optional = true }
 tonic = { version = "0.12.3", optional = true }
 tonic-reflection = { version = "0.12.3", optional = true }
 prost = { version = "0.13.3", optional = true }
 prost-types = { version = "0.13.3", optional = true }
-time = "0.3.36"
 
 [dev-dependencies]
 serial_test = "3.0"
@@ -80,7 +75,7 @@ tokio = { version = "1", features = [
 ] }
 
 [build-dependencies]
-tonic-build = "0.11"
+tonic-build = "0.12"
 
 [[bench]]
 name = "bench"

limitador/benches/bench.rs

@@ -547,13 +547,16 @@ fn generate_test_limits(scenario: &TestScenario) -> (Vec<Limit>, Vec<TestCallPar
         let namespace = idx_namespace.to_string();
 
         for limit_idx in 0..scenario.n_limits_per_ns {
-            test_limits.push(Limit::new(
-                namespace.clone(),
-                u64::MAX,
-                ((limit_idx * 60) + 10) as u64,
-                conditions.clone(),
-                variables.clone(),
-            ))
+            test_limits.push(
+                Limit::new(
+                    namespace.clone(),
+                    u64::MAX,
+                    ((limit_idx * 60) + 10) as u64,
+                    conditions.clone(),
+                    variables.clone(),
+                )
+                .expect("This must be a valid limit!"),
+            )
         }
 
         call_params.push(TestCallParams {

limitador/build.rs

@@ -15,7 +15,7 @@ fn generate_protobuf() -> Result<(), Box<dyn Error>> {
 
         tonic_build::configure()
             .protoc_arg("--experimental_allow_proto3_optional")
-            .compile(&[proto_path], &[proto_dir])?;
+            .compile_protos(&[proto_path], &[proto_dir])?;
     }
 
     Ok(())

limitador/src/errors.rs

@@ -1,14 +1,51 @@
+use crate::limit::ConditionParsingError;
 use crate::storage::StorageErr;
-use thiserror::Error;
+use std::convert::Infallible;
+use std::error::Error;
+use std::fmt::{Display, Formatter};
 
-#[derive(Error, Debug, Eq, PartialEq)]
+#[derive(Debug)]
 pub enum LimitadorError {
-    #[error("error while accessing the limits storage: {0:?}")]
-    Storage(String),
+    StorageError(StorageErr),
+    InterpreterError(ConditionParsingError),
+}
+
+impl Display for LimitadorError {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        match self {
+            LimitadorError::StorageError(err) => {
+                write!(f, "error while accessing the limits storage: {err:?}")
+            }
+            LimitadorError::InterpreterError(err) => {
+                write!(f, "error parsing condition: {err:?}")
+            }
+        }
+    }
+}
+
+impl Error for LimitadorError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            LimitadorError::StorageError(err) => Some(err),
+            LimitadorError::InterpreterError(err) => Some(err),
+        }
+    }
 }
 
 impl From<StorageErr> for LimitadorError {
     fn from(e: StorageErr) -> Self {
-        Self::Storage(e.msg().to_owned())
+        Self::StorageError(e)
+    }
+}
+
+impl From<ConditionParsingError> for LimitadorError {
+    fn from(err: ConditionParsingError) -> Self {
+        LimitadorError::InterpreterError(err)
+    }
+}
+
+impl From<Infallible> for LimitadorError {
+    fn from(value: Infallible) -> Self {
+        unreachable!("unexpected infallible value: {:?}", value)
     }
 }