From 80a645cc88fd5b1549010fd60667d7622e9370c9 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 11 Sep 2024 15:11:39 -0400 Subject: [PATCH 1/2] Make Limit.id immutable Signed-off-by: Alex Snaps --- .../src/http_api/request_types.rs | 29 ++++++++++------ limitador/src/limit.rs | 34 +++++++++++++++---- limitador/src/storage/keys.rs | 32 +++++++++++------ limitador/tests/integration_tests.rs | 4 +-- 4 files changed, 69 insertions(+), 30 deletions(-) diff --git a/limitador-server/src/http_api/request_types.rs b/limitador-server/src/http_api/request_types.rs index 9751ee2e..c563e411 100644 --- a/limitador-server/src/http_api/request_types.rs +++ b/limitador-server/src/http_api/request_types.rs @@ -43,17 +43,24 @@ impl From<&LimitadorLimit> for Limit { impl From for LimitadorLimit { fn from(limit: Limit) -> Self { - let mut limitador_limit = Self::new( - limit.namespace, - limit.max_value, - limit.seconds, - limit.conditions, - limit.variables, - ); - - if let Some(id) = limit.id { - limitador_limit.set_id(id); - } + let mut limitador_limit = if let Some(id) = limit.id { + Self::with_id( + id, + limit.namespace, + limit.max_value, + 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) diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index fa0e124c..883cf2d8 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -334,12 +334,34 @@ impl Limit { } } - pub fn namespace(&self) -> &Namespace { - &self.namespace + pub fn with_id, N: Into, T: TryInto>( + id: S, + namespace: N, + max_value: u64, + seconds: u64, + conditions: impl IntoIterator, + variables: impl IntoIterator>, + ) -> Self + where + >::Error: core::fmt::Debug, + >::Error: core::fmt::Debug, + { + 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(), + } } - pub fn set_id(&mut self, value: String) { - self.id = Some(value); + pub fn namespace(&self) -> &Namespace { + &self.namespace } pub fn id(&self) -> &Option { @@ -1012,14 +1034,14 @@ mod tests { #[test] fn limit_id() { - let mut limit = Limit::new( + let limit = Limit::with_id( + "test_id", "test_namespace", 10, 60, vec!["req.method == 'GET'"], vec!["app_id"], ); - limit.set_id("test_id".to_string()); assert_eq!(limit.id().clone(), Some("test_id".to_string())) } diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 3fe5ded1..06a3d037 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -130,14 +130,14 @@ mod tests { #[test] fn key_for_limit_with_id_format() { - let mut limit = Limit::new( + let limit = Limit::with_id( + "test_id", "example.com", 10, 60, vec!["req.method == 'GET'"], vec!["app_id"], ); - limit.set_id("test_id".to_string()); assert_eq!( "\u{2}\u{7}test_id".as_bytes(), key_for_counters_of_limit(&limit) @@ -280,8 +280,14 @@ pub mod bin { .collect(); // we are not able to rebuild the full limit since we only have the id and variables. - let mut limit = Limit::new::<&str, &str>("", u64::default(), 0, vec![], map.keys()); - limit.set_id(id.to_string()); + let limit = Limit::with_id::<&str, &str, &str>( + id, + "", + u64::default(), + 0, + vec![], + map.keys(), + ); Counter::new(limit, map) } _ => panic!("Unknown version: {}", version), @@ -316,14 +322,13 @@ pub mod bin { #[cfg(test)] mod tests { - use crate::counter::Counter; - use crate::Limit; - use std::collections::HashMap; - use super::{ key_for_counter, key_for_counter_v2, partial_counter_from_counter_key, prefix_for_namespace, CounterKey, }; + use crate::counter::Counter; + use crate::Limit; + use std::collections::HashMap; #[test] fn counter_key_serializes_and_back() { @@ -375,9 +380,14 @@ pub mod bin { let namespace = "ns_counter:"; let limit_without_id = Limit::new(namespace, 1, 1, vec!["req.method == 'GET'"], vec!["app_id"]); - let mut limit_with_id = - Limit::new(namespace, 1, 1, vec!["req.method == 'GET'"], vec!["app_id"]); - limit_with_id.set_id("id200".to_string()); + let limit_with_id = Limit::with_id( + "id200", + namespace, + 1, + 1, + vec!["req.method == 'GET'"], + vec!["app_id"], + ); let counter_with_id = Counter::new(limit_with_id, HashMap::default()); let serialized_with_id_counter = key_for_counter(&counter_with_id); diff --git a/limitador/tests/integration_tests.rs b/limitador/tests/integration_tests.rs index 5ecf661f..49fafc45 100644 --- a/limitador/tests/integration_tests.rs +++ b/limitador/tests/integration_tests.rs @@ -523,14 +523,14 @@ mod test { async fn rate_limited_id_counter(rate_limiter: &mut TestsLimiter) { let namespace = "test_namespace"; let max_hits = 3; - let mut limit = Limit::new( + let limit = Limit::with_id( + "test-rate_limited_id_counter", namespace, max_hits, 60, vec!["req.method == 'GET'"], vec!["app_id"], ); - limit.set_id("test-rate_limited_id_counter".to_string()); rate_limiter.add_limit(&limit).await; From e9579ffcffde10008dabe8a2db9009761b25783c Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 11 Sep 2024 15:29:57 -0400 Subject: [PATCH 2/2] Slightly lighter API Signed-off-by: Alex Snaps --- .../src/http_api/request_types.rs | 2 +- limitador/src/counter.rs | 2 +- limitador/src/limit.rs | 6 +++--- limitador/src/storage/keys.rs | 21 +++++++++---------- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/limitador-server/src/http_api/request_types.rs b/limitador-server/src/http_api/request_types.rs index c563e411..d9419c05 100644 --- a/limitador-server/src/http_api/request_types.rs +++ b/limitador-server/src/http_api/request_types.rs @@ -30,7 +30,7 @@ pub struct Limit { impl From<&LimitadorLimit> for Limit { fn from(ll: &LimitadorLimit) -> Self { Self { - id: ll.id().clone(), + id: ll.id().map(|id| id.to_string()), namespace: ll.namespace().as_ref().to_string(), max_value: ll.max_value(), seconds: ll.seconds(), diff --git a/limitador/src/counter.rs b/limitador/src/counter.rs index 0b55bd6c..1f7ce923 100644 --- a/limitador/src/counter.rs +++ b/limitador/src/counter.rs @@ -72,7 +72,7 @@ impl Counter { Duration::from_secs(self.limit.seconds()) } - pub fn id(&self) -> &Option { + pub fn id(&self) -> Option<&str> { self.limit.id() } diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 883cf2d8..7fe15fd2 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -364,8 +364,8 @@ impl Limit { &self.namespace } - pub fn id(&self) -> &Option { - &self.id + pub fn id(&self) -> Option<&str> { + self.id.as_deref() } pub fn max_value(&self) -> u64 { @@ -1043,6 +1043,6 @@ mod tests { vec!["app_id"], ); - assert_eq!(limit.id().clone(), Some("test_id".to_string())) + assert_eq!(limit.id(), Some("test_id")) } } diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 06a3d037..037d5f9b 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -40,26 +40,25 @@ pub fn key_for_counter(counter: &Counter) -> Vec { } pub fn key_for_counters_of_limit(limit: &Limit) -> Vec { - if limit.id().is_none() { - let namespace = limit.namespace().as_ref(); - format!( - "namespace:{{{namespace}}},counters_of_limit:{}", - serde_json::to_string(limit).unwrap() - ) - .into_bytes() - } else { + if let Some(id) = limit.id() { #[derive(PartialEq, Debug, Serialize, Deserialize)] struct IdLimitKey<'a> { id: &'a str, } - let id = limit.id().as_ref().unwrap(); - let key = IdLimitKey { id: id.as_ref() }; + let key = IdLimitKey { id }; let mut encoded_key = Vec::new(); encoded_key = postcard::to_extend(&2u8, encoded_key).unwrap(); encoded_key = postcard::to_extend(&key, encoded_key).unwrap(); encoded_key + } else { + let namespace = limit.namespace().as_ref(); + format!( + "namespace:{{{namespace}}},counters_of_limit:{}", + serde_json::to_string(limit).unwrap() + ) + .into_bytes() } } @@ -182,7 +181,7 @@ pub mod bin { impl<'a> From<&'a Counter> for IdCounterKey<'a> { fn from(counter: &'a Counter) -> Self { IdCounterKey { - id: counter.id().as_ref().unwrap().as_ref(), + id: counter.id().unwrap(), variables: counter.variables_for_key(), } }