diff --git a/limitador-server/src/envoy_rls/server.rs b/limitador-server/src/envoy_rls/server.rs index e51352e4..04ba74e1 100644 --- a/limitador-server/src/envoy_rls/server.rs +++ b/limitador-server/src/envoy_rls/server.rs @@ -268,7 +268,9 @@ mod tests { Limit::new(namespace, 0, 60, vec!["x == 1", "y == 2"], vec!["z"]), ] .into_iter() - .for_each(|limit| limiter.add_limit(limit)); + .for_each(|limit| { + limiter.add_limit(limit); + }); let rate_limiter = MyRateLimiter::new(Arc::new(Limiter::Blocking(limiter))); diff --git a/limitador-server/src/http_api/server.rs b/limitador-server/src/http_api/server.rs index 88345f10..15ac9efc 100644 --- a/limitador-server/src/http_api/server.rs +++ b/limitador-server/src/http_api/server.rs @@ -390,7 +390,7 @@ mod tests { match &limiter { Limiter::Blocking(limiter) => limiter.add_limit(limit.clone()), Limiter::Async(limiter) => limiter.add_limit(limit.clone()), - } + }; limit } } diff --git a/limitador/src/lib.rs b/limitador/src/lib.rs index 46d6a19d..2ceab0ad 100644 --- a/limitador/src/lib.rs +++ b/limitador/src/lib.rs @@ -312,7 +312,7 @@ impl RateLimiter { self.storage.get_namespaces() } - pub fn add_limit(&self, limit: Limit) { + pub fn add_limit(&self, limit: Limit) -> bool { self.storage.add_limit(limit) } @@ -483,7 +483,7 @@ impl AsyncRateLimiter { self.storage.get_namespaces() } - pub fn add_limit(&self, limit: Limit) { + pub fn add_limit(&self, limit: Limit) -> bool { self.storage.add_limit(limit) } diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index e7dc1abe..5e142fc5 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -129,9 +129,7 @@ impl Limit { impl Hash for Limit { fn hash(&self, state: &mut H) { self.namespace.hash(state); - self.max_value.hash(state); self.seconds.hash(state); - self.name.hash(state); let mut encoded_conditions = self .conditions @@ -156,9 +154,7 @@ impl Hash for Limit { impl PartialEq for Limit { fn eq(&self, other: &Self) -> bool { self.namespace == other.namespace - && self.max_value == other.max_value && self.seconds == other.seconds - && self.name == other.name && self.conditions == other.conditions && self.variables == other.variables } diff --git a/limitador/src/storage/mod.rs b/limitador/src/storage/mod.rs index 345ab946..59cb852c 100644 --- a/limitador/src/storage/mod.rs +++ b/limitador/src/storage/mod.rs @@ -52,14 +52,14 @@ impl Storage { self.limits.read().unwrap().keys().cloned().collect() } - pub fn add_limit(&self, limit: Limit) { + pub fn add_limit(&self, limit: Limit) -> bool { let namespace = limit.namespace().clone(); self.limits .write() .unwrap() .entry(namespace) .or_default() - .insert(limit); + .insert(limit) } pub fn get_limits(&self, namespace: &Namespace) -> HashSet { @@ -140,19 +140,20 @@ impl AsyncStorage { self.limits.read().unwrap().keys().cloned().collect() } - pub fn add_limit(&self, limit: Limit) { + pub fn add_limit(&self, limit: Limit) -> bool { let namespace = limit.namespace().clone(); let mut limits_for_namespace = self.limits.write().unwrap(); match limits_for_namespace.get_mut(&namespace) { Some(limits) => { - limits.insert(limit); + limits.insert(limit) } None => { let mut limits = HashSet::new(); limits.insert(limit); limits_for_namespace.insert(namespace, limits); + true } } } diff --git a/limitador/tests/helpers/tests_limiter.rs b/limitador/tests/helpers/tests_limiter.rs index 9b34b722..f8cc298b 100644 --- a/limitador/tests/helpers/tests_limiter.rs +++ b/limitador/tests/helpers/tests_limiter.rs @@ -39,7 +39,7 @@ impl TestsLimiter { } } - pub async fn add_limit(&self, limit: &Limit) { + pub async fn add_limit(&self, limit: &Limit) -> bool { match &self.limiter_impl { LimiterImpl::Blocking(limiter) => limiter.add_limit(limit.clone()), LimiterImpl::Async(limiter) => limiter.add_limit(limit.clone()), diff --git a/limitador/tests/integration_tests.rs b/limitador/tests/integration_tests.rs index ffe8ea7f..8fcade34 100644 --- a/limitador/tests/integration_tests.rs +++ b/limitador/tests/integration_tests.rs @@ -138,6 +138,7 @@ mod test { test_with_all_storage_impls!(configure_with_creates_the_given_limits); test_with_all_storage_impls!(configure_with_keeps_the_given_limits_and_counters_if_they_exist); test_with_all_storage_impls!(configure_with_deletes_all_except_the_limits_given); + test_with_all_storage_impls!(add_limit_only_adds_if_not_present); // All these functions need to use async/await. That's needed to support // both the sync and the async implementations of the rate limiter. @@ -328,7 +329,7 @@ mod test { ]; for limit in limits.iter() { - rate_limiter.add_limit(limit).await + rate_limiter.add_limit(limit).await; } rate_limiter.delete_limits(namespace).await.unwrap(); @@ -830,7 +831,7 @@ mod test { let namespace = "test_namespace"; let limit_to_be_kept = - Limit::new(namespace, 10, 60, vec!["req.method == GET"], vec!["app_id"]); + Limit::new(namespace, 10, 1, vec!["req.method == GET"], vec!["app_id"]); let limit_to_be_deleted = Limit::new(namespace, 20, 60, vec!["req.method == GET"], vec!["app_id"]); @@ -849,4 +850,29 @@ mod test { assert!(limits.contains(&limit_to_be_kept)); assert!(!limits.contains(&limit_to_be_deleted)); } + + async fn add_limit_only_adds_if_not_present(rate_limiter: &mut TestsLimiter) { + let namespace = "test_namespace"; + + let limit_1 = + Limit::new(namespace, 10, 60, vec!["req.method == GET"], vec!["app_id"]); + + let limit_2 = + Limit::new(namespace, 20, 60, vec!["req.method == GET"], vec!["app_id"]); + + let mut limit_3 = + Limit::new(namespace, 20, 60, vec!["req.method == GET"], vec!["app_id"]); + limit_3.set_name("Name is irrelevant too".to_owned()); + + assert!(rate_limiter.add_limit(&limit_1).await); + assert!(!rate_limiter.add_limit(&limit_2).await); + assert!(!rate_limiter.add_limit(&limit_3).await); + + let limits = rate_limiter.get_limits(namespace).await; + + assert_eq!(limits.len(), 1); + let known_limit = limits.iter().next().unwrap(); + assert_eq!(known_limit.max_value(), 10); + assert_eq!(known_limit.name(), None); + } }