Deal with syntax error of conditions

Signed-off-by: Alex Snaps <[email protected]>

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/src/errors.rs

@@ -1,32 +1,51 @@
+use crate::limit::ConditionParsingError;
 use crate::storage::StorageErr;
+use std::convert::Infallible;
 use std::error::Error;
 use std::fmt::{Display, Formatter};
 
 #[derive(Debug)]
 pub enum LimitadorError {
-    Storage(StorageErr),
+    StorageError(StorageErr),
+    InterpreterError(ConditionParsingError),
 }
 
 impl Display for LimitadorError {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         match self {
-            LimitadorError::Storage(err) => {
+            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::Storage(err) => Some(err),
+            LimitadorError::StorageError(err) => Some(err),
+            LimitadorError::InterpreterError(err) => Some(err),
         }
     }
 }
 
 impl From<StorageErr> for LimitadorError {
     fn from(e: StorageErr) -> Self {
-        Self::Storage(e)
+        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)
     }
 }

limitador/src/lib.rs

@@ -73,7 +73,7 @@
 //!      60,
 //!      vec!["req.method == 'GET'"],
 //!      vec!["user_id"],
-//! );
+//! ).unwrap();
 //! let mut rate_limiter = RateLimiter::new(1000);
 //!
 //! // Add a limit
@@ -105,7 +105,7 @@
 //!      60,
 //!      vec!["req.method == 'GET'"],
 //!      vec!["user_id"],
-//! );
+//! ).unwrap();
 //! rate_limiter.add_limit(limit);
 //!
 //! // We've defined a limit of 2. So we can report 2 times before being
@@ -169,7 +169,7 @@
 //!      60,
 //!      vec!["req.method == 'GET'"],
 //!      vec!["user_id"],
-//! );
+//! ).unwrap();
 //!
 //! async {
 //!     let rate_limiter = AsyncRateLimiter::new_with_storage(
@@ -708,7 +708,8 @@ mod test {
             100,
             Vec::<String>::default(),
             Vec::<String>::default(),
-        );
+        )
+        .expect("This must be a valid limit!");
         rl.add_limit(l.clone());
         let limits = rl.get_limits(&namespace.into());
         assert_eq!(limits.len(), 1);

limitador/src/limit.rs

@@ -25,6 +25,8 @@ mod deprecated {
     }
 }
 
+use crate::errors::LimitadorError;
+use crate::LimitadorResult;
 #[cfg(feature = "lenient_conditions")]
 pub use deprecated::check_deprecated_syntax_usages_and_reset;
 
@@ -296,23 +298,26 @@ impl Limit {
         seconds: u64,
         conditions: impl IntoIterator<Item = T>,
         variables: impl IntoIterator<Item = impl Into<String>>,
-    ) -> Self
+    ) -> LimitadorResult<Self>
     where
         <N as TryInto<Namespace>>::Error: core::fmt::Debug,
         <T as TryInto<Condition>>::Error: core::fmt::Debug,
+        LimitadorError: From<<T as TryInto<Condition>>::Error>,
     {
         // the above where-clause is needed in order to call unwrap().
-        Self {
-            id: None,
-            namespace: namespace.into(),
-            max_value,
-            seconds,
-            name: None,
-            conditions: conditions
-                .into_iter()
-                .map(|cond| cond.try_into().expect("Invalid condition"))
-                .collect(),
-            variables: variables.into_iter().map(|var| var.into()).collect(),
+        let conditions: Result<BTreeSet<_>, _> =
+            conditions.into_iter().map(|cond| cond.try_into()).collect();
+        match conditions {
+            Ok(conditions) => Ok(Self {
+                id: None,
+                namespace: namespace.into(),
+                max_value,
+                seconds,
+                name: None,
+                conditions,
+                variables: variables.into_iter().map(|var| var.into()).collect(),
+            }),
+            Err(err) => Err(err.into()),
         }
     }
 
@@ -323,22 +328,21 @@ impl Limit {
         seconds: u64,
         conditions: impl IntoIterator<Item = T>,
         variables: impl IntoIterator<Item = impl Into<String>>,
-    ) -> Self
+    ) -> LimitadorResult<Self>
     where
-        <N as TryInto<Namespace>>::Error: core::fmt::Debug,
-        <T as TryInto<Condition>>::Error: core::fmt::Debug,
+        LimitadorError: From<<T as TryInto<Condition>>::Error>,
     {
-        Self {
-            id: Some(id.into()),
-            namespace: namespace.into(),
-            max_value,
-            seconds,
-            name: None,
-            conditions: conditions
-                .into_iter()
-                .map(|cond| cond.try_into().expect("Invalid condition"))
-                .collect(),
-            variables: variables.into_iter().map(|var| var.into()).collect(),
+        match conditions.into_iter().map(|cond| cond.try_into()).collect() {
+            Ok(conditions) => Ok(Self {
+                id: Some(id.into()),
+                namespace: namespace.into(),
+                max_value,
+                seconds,
+                name: None,
+                conditions,
+                variables: variables.into_iter().map(|var| var.into()).collect(),
+            }),
+            Err(err) => Err(err.into()),
         }
     }
 
@@ -858,7 +862,8 @@ mod tests {
 
     #[test]
     fn limit_can_have_an_optional_name() {
-        let mut limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]);
+        let mut limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"])
+            .expect("This must be a valid limit!");
         assert!(limit.name.is_none());
 
         let name = "Test Limit";
@@ -868,7 +873,8 @@ mod tests {
 
     #[test]
     fn limit_applies() {
-        let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]);
+        let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"])
+            .expect("This must be a valid limit!");
 
         let mut values: HashMap<String, String> = HashMap::new();
         values.insert("x".into(), "5".into());
@@ -879,7 +885,8 @@ mod tests {
 
     #[test]
     fn limit_does_not_apply_when_cond_is_false() {
-        let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]);
+        let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"])
+            .expect("This must be a valid limit!");
 
         let mut values: HashMap<String, String> = HashMap::new();
         values.insert("x".into(), "1".into());
@@ -891,7 +898,8 @@ mod tests {
     #[test]
     #[cfg(feature = "lenient_conditions")]
     fn limit_does_not_apply_when_cond_is_false_deprecated_style() {
-        let limit = Limit::new("test_namespace", 10, 60, vec!["x == 5"], vec!["y"]);
+        let limit = Limit::new("test_namespace", 10, 60, vec!["x == 5"], vec!["y"])
+            .expect("This must be a valid limit!");
 
         let mut values: HashMap<String, String> = HashMap::new();
         values.insert("x".into(), "1".into());
@@ -901,7 +909,8 @@ mod tests {
         assert!(check_deprecated_syntax_usages_and_reset());
         assert!(!check_deprecated_syntax_usages_and_reset());
 
-        let limit = Limit::new("test_namespace", 10, 60, vec!["x == foobar"], vec!["y"]);
+        let limit = Limit::new("test_namespace", 10, 60, vec!["x == foobar"], vec!["y"])
+            .expect("This must be a valid limit!");
 
         let mut values: HashMap<String, String> = HashMap::new();
         values.insert("x".into(), "foobar".into());
@@ -914,7 +923,8 @@ mod tests {
 
     #[test]
     fn limit_does_not_apply_when_cond_var_is_not_set() {
-        let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]);
+        let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"])
+            .expect("This must be a valid limit!");
 
         // Notice that "x" is not set
         let mut values: HashMap<String, String> = HashMap::new();
@@ -926,7 +936,8 @@ mod tests {
 
     #[test]
     fn limit_does_not_apply_when_var_not_set() {
-        let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]);
+        let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"])
+            .expect("This must be a valid limit!");
 
         // Notice that "y" is not set
         let mut values: HashMap<String, String> = HashMap::new();
@@ -943,7 +954,8 @@ mod tests {
             60,
             vec!["x == \"5\"", "y == \"2\""],
             vec!["z"],
-        );
+        )
+        .expect("This must be a valid limit!");
 
         let mut values: HashMap<String, String> = HashMap::new();
         values.insert("x".into(), "5".into());
@@ -961,7 +973,8 @@ mod tests {
             60,
             vec!["x == \"5\"", "y == \"2\""],
             vec!["z"],
-        );
+        )
+        .expect("This must be a valid limit!");
 
         let mut values: HashMap<String, String> = HashMap::new();
         values.insert("x".into(), "3".into());
@@ -1045,7 +1058,8 @@ mod tests {
             60,
             vec!["req.method == 'GET'"],
             vec!["app_id"],
-        );
+        )
+        .expect("This must be a valid limit!");
 
         assert_eq!(limit.id(), Some("test_id"))
     }
@@ -1059,15 +1073,17 @@ mod tests {
             60,
             vec!["req.method == 'GET'"],
             vec!["app_id"],
-        );
+        )
+        .expect("This must be a valid limit!");
 
         let mut limit2 = Limit::new(
             limit1.namespace.clone(),
             limit1.max_value + 10,
             limit1.seconds,
             limit1.conditions.clone(),
             limit1.variables.clone(),
-        );
+        )
+        .expect("This must be a valid limit!");
         limit2.set_name("Who cares?".to_string());
 
         assert_eq!(limit1.partial_cmp(&limit2), Some(Equal));