From 7cc4ff459d7f09e5d210ae707ba3e182233afa10 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 3 Dec 2024 06:48:25 -0500 Subject: [PATCH] Fixed test and take ownership of vars in ctx Signed-off-by: Alex Snaps --- limitador-server/src/envoy_rls/server.rs | 2 +- limitador-server/src/http_api/server.rs | 6 +-- limitador/benches/bench.rs | 20 ++++----- limitador/src/counter.rs | 2 +- limitador/src/limit.rs | 16 +++---- limitador/src/limit/cel.rs | 19 ++++---- limitador/src/storage/disk/rocksdb_storage.rs | 2 +- limitador/src/storage/distributed/mod.rs | 2 +- limitador/src/storage/in_memory.rs | 2 +- limitador/src/storage/keys.rs | 12 ++--- limitador/src/storage/redis/counters_cache.rs | 3 +- limitador/src/storage/redis/redis_cached.rs | 6 +-- limitador/tests/integration_tests.rs | 45 ++++++++++--------- 13 files changed, 70 insertions(+), 67 deletions(-) diff --git a/limitador-server/src/envoy_rls/server.rs b/limitador-server/src/envoy_rls/server.rs index d9925a68..b6d24e1b 100644 --- a/limitador-server/src/envoy_rls/server.rs +++ b/limitador-server/src/envoy_rls/server.rs @@ -109,7 +109,7 @@ impl RateLimitService for MyRateLimiter { req.hits_addend }; - let ctx = (&values).into(); + let ctx = values.into(); let rate_limited_resp = match &*self.limiter { Limiter::Blocking(limiter) => limiter.check_rate_limited_and_update( diff --git a/limitador-server/src/http_api/server.rs b/limitador-server/src/http_api/server.rs index ddbd0d64..ed413e53 100644 --- a/limitador-server/src/http_api/server.rs +++ b/limitador-server/src/http_api/server.rs @@ -122,7 +122,7 @@ async fn check( response_headers: _, } = request.into_inner(); let namespace = namespace.into(); - let ctx = (&values).into(); + let ctx = values.into(); let is_rate_limited_result = match state.get_ref().limiter() { Limiter::Blocking(limiter) => limiter.is_rate_limited(&namespace, &ctx, delta), Limiter::Async(limiter) => limiter.is_rate_limited(&namespace, &ctx, delta).await, @@ -153,7 +153,7 @@ async fn report( response_headers: _, } = request.into_inner(); let namespace = namespace.into(); - let ctx = (&values).into(); + let ctx = values.into(); let update_counters_result = match data.get_ref().limiter() { Limiter::Blocking(limiter) => limiter.update_counters(&namespace, &ctx, delta), Limiter::Async(limiter) => limiter.update_counters(&namespace, &ctx, delta).await, @@ -178,7 +178,7 @@ async fn check_and_report( response_headers, } = request.into_inner(); let namespace = namespace.into(); - let ctx = (&values).into(); + let ctx = values.into(); let rate_limit_data = data.get_ref(); let rate_limited_and_update_result = match rate_limit_data.limiter() { Limiter::Blocking(limiter) => limiter.check_rate_limited_and_update( diff --git a/limitador/benches/bench.rs b/limitador/benches/bench.rs index 68a360b9..fed3fec8 100644 --- a/limitador/benches/bench.rs +++ b/limitador/benches/bench.rs @@ -7,7 +7,7 @@ use criterion::{black_box, criterion_group, criterion_main, Bencher, BenchmarkId use rand::seq::SliceRandom; use rand::SeedableRng; -use limitador::limit::Limit; +use limitador::limit::{Context, Limit}; #[cfg(feature = "disk_storage")] use limitador::storage::disk::{DiskStorage, OptimizeFor}; #[cfg(feature = "distributed_storage")] @@ -89,9 +89,9 @@ const TEST_SCENARIOS: &[&TestScenario] = &[ }, ]; -struct TestCallParams { +struct TestCallParams<'a> { namespace: String, - values: HashMap, + ctx: Context<'a>, delta: u64, } @@ -329,7 +329,7 @@ fn bench_is_rate_limited( rate_limiter .is_rate_limited( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, ) .unwrap(), @@ -357,7 +357,7 @@ fn async_bench_is_rate_limited( rate_limiter .is_rate_limited( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, ) .await @@ -383,7 +383,7 @@ fn bench_update_counters( rate_limiter .update_counters( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, ) .unwrap(); @@ -410,7 +410,7 @@ fn async_bench_update_counters( rate_limiter .update_counters( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, ) .await @@ -437,7 +437,7 @@ fn bench_check_rate_limited_and_update( rate_limiter .check_rate_limited_and_update( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, false, ) @@ -467,7 +467,7 @@ fn async_bench_check_rate_limited_and_update( rate_limiter .check_rate_limited_and_update( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, false, ) @@ -562,7 +562,7 @@ fn generate_test_limits(scenario: &TestScenario) -> (Vec, Vec = HashMap::new(); values.insert("x".into(), "5".into()); - assert!(!limit.applies(&(&values).into())) + assert!(!limit.applies(&values.into())) } #[test] @@ -333,7 +333,7 @@ mod tests { values.insert("y".into(), "2".into()); values.insert("z".into(), "1".into()); - assert!(limit.applies(&(&values).into())) + assert!(limit.applies(&values.into())) } #[test] @@ -354,7 +354,7 @@ mod tests { values.insert("y".into(), "2".into()); values.insert("z".into(), "1".into()); - assert!(!limit.applies(&(&values).into())) + assert!(!limit.applies(&values.into())) } #[test] @@ -434,7 +434,7 @@ mod tests { ], Vec::default(), ); - assert!(limit.applies(&(&HashMap::default()).into())); + assert!(limit.applies(&Context::default())); let limit = Limit::with_id( "my_id", @@ -462,7 +462,7 @@ mod tests { ("foo".to_string(), "nice bar!".to_string()), ("bar".to_string(), "foo,baz".to_string()), ]); - let ctx = Context::new(String::default(), &map); + let ctx = map.into(); assert!(limit.applies(&ctx)); assert_eq!( Counter::new(limit, &ctx) diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index 41421b20..d44609d1 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -78,22 +78,21 @@ pub struct Context<'a> { } impl<'a> Context<'a> { - pub(crate) fn new(root: String, values: &HashMap) -> Self { + pub(crate) fn new(root: String, values: HashMap) -> Self { let mut ctx = cel_interpreter::Context::default(); + let mut variables = HashSet::new(); if root.is_empty() { for (binding, value) in values { - ctx.add_variable_from_value(binding, value.clone()) + ctx.add_variable_from_value(binding.clone(), value.clone()); + variables.insert(binding); } } else { let map = cel_interpreter::objects::Map::from(values.clone()); ctx.add_variable_from_value(root, Value::Map(map)); } - Self { - variables: values.keys().cloned().collect(), - ctx, - } + Self { variables, ctx } } pub fn list_binding(&mut self, name: String, value: Vec>) { @@ -146,12 +145,12 @@ impl<'a> Context<'a> { impl Default for Context<'_> { fn default() -> Self { - Self::new(String::default(), &HashMap::default()) + Self::new(String::default(), HashMap::default()) } } -impl From<&HashMap> for Context<'_> { - fn from(value: &HashMap) -> Self { +impl From> for Context<'_> { + fn from(value: HashMap) -> Self { Self::new(String::default(), value) } } @@ -411,7 +410,7 @@ mod tests { fn predicate_no_key() { let pred = Predicate::parse("there.not == 42").expect("failed to parse"); assert_eq!( - pred.test(&(&HashMap::from([("there".to_string(), String::default())])).into()), + pred.test(&HashMap::from([("there".to_string(), String::default())]).into()), Ok(false) ); } diff --git a/limitador/src/storage/disk/rocksdb_storage.rs b/limitador/src/storage/disk/rocksdb_storage.rs index 0d758c27..96496693 100644 --- a/limitador/src/storage/disk/rocksdb_storage.rs +++ b/limitador/src/storage/disk/rocksdb_storage.rs @@ -250,7 +250,7 @@ mod tests { vec!["app_id".try_into().expect("failed parsing!")], ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new(limit, &ctx) .unwrap() .expect("must have a counter"); diff --git a/limitador/src/storage/distributed/mod.rs b/limitador/src/storage/distributed/mod.rs index 1c8f23c7..77f0a3ff 100644 --- a/limitador/src/storage/distributed/mod.rs +++ b/limitador/src/storage/distributed/mod.rs @@ -341,7 +341,7 @@ fn encode_limit_to_key(limit: &Limit) -> Vec { .into_iter() .map(|k| (k, "".to_string())) .collect(); - let ctx = (&vars).into(); + let ctx = vars.into(); let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation can't fail! faked vars!") .expect("must have a counter"); diff --git a/limitador/src/storage/in_memory.rs b/limitador/src/storage/in_memory.rs index 6d741f65..28dc2119 100644 --- a/limitador/src/storage/in_memory.rs +++ b/limitador/src/storage/in_memory.rs @@ -270,7 +270,7 @@ mod tests { vec!["app_id".try_into().expect("failed parsing!")], ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter_1 = Counter::new(limit_1, &ctx) .expect("counter creation failed!") .expect("Should have a counter"); diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 247eb2f6..10fe5a44 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -154,7 +154,7 @@ mod tests { vec!["app_id".try_into().expect("failed parsing!")], ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation failed!") .expect("must have a counter"); @@ -173,7 +173,7 @@ mod tests { vec!["app_id".try_into().expect("failed parsing!")], ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation failed!") .expect("must have a counter"); @@ -387,7 +387,7 @@ pub mod bin { vars.insert("role".to_string(), "admin".to_string()); vars.insert("app_id".to_string(), "123".to_string()); vars.insert("wat".to_string(), "dunno".to_string()); - let ctx = (&vars).into(); + let ctx = vars.into(); let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation failed!") .expect("must have a counter"); @@ -411,7 +411,7 @@ pub mod bin { ); let mut variables = HashMap::default(); variables.insert("app_id".to_string(), "123".to_string()); - let ctx = (&variables).into(); + let ctx = variables.into(); let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation failed!") .expect("must have a counter"); @@ -430,7 +430,7 @@ pub mod bin { vec!["app_id".try_into().expect("failed parsing!")], ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new(limit, &ctx) .expect("counter creation failed!") .expect("must have a counter"); @@ -460,7 +460,7 @@ pub mod bin { ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter_with_id = Counter::new(limit_with_id, &ctx) .expect("counter creation failed!") .expect("must have a counter"); diff --git a/limitador/src/storage/redis/counters_cache.rs b/limitador/src/storage/redis/counters_cache.rs index fb05300e..49c83e45 100644 --- a/limitador/src/storage/redis/counters_cache.rs +++ b/limitador/src/storage/redis/counters_cache.rs @@ -676,7 +676,6 @@ mod tests { if let Some(overrides) = other_values { values.extend(overrides); } - let ctx = (&values).into(); Counter::new( Limit::new( "test_namespace", @@ -685,7 +684,7 @@ mod tests { vec!["req_method == 'POST'".try_into().expect("failed parsing!")], vec!["app_id".try_into().expect("failed parsing!")], ), - &ctx, + &values.into(), ) .expect("failed creating counter") .expect("Should have a counter") diff --git a/limitador/src/storage/redis/redis_cached.rs b/limitador/src/storage/redis/redis_cached.rs index 0cc0e99e..eda05161 100644 --- a/limitador/src/storage/redis/redis_cached.rs +++ b/limitador/src/storage/redis/redis_cached.rs @@ -440,7 +440,7 @@ mod tests { let mut counters_and_deltas = HashMap::new(); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new( Limit::new( "test_namespace", @@ -506,7 +506,7 @@ mod tests { #[tokio::test] async fn flush_batcher_and_update_counters_test() { let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new( Limit::new( "test_namespace", @@ -569,7 +569,7 @@ mod tests { #[tokio::test] async fn flush_batcher_reverts_on_err() { let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new( Limit::new( "test_namespace", diff --git a/limitador/tests/integration_tests.rs b/limitador/tests/integration_tests.rs index 4f8f2d4a..a19eb756 100644 --- a/limitador/tests/integration_tests.rs +++ b/limitador/tests/integration_tests.rs @@ -380,7 +380,7 @@ mod test { values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); rate_limiter - .update_counters(namespace, &(&values).into(), 1) + .update_counters(namespace, &values.into(), 1) .await .unwrap(); @@ -473,7 +473,7 @@ mod test { values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); rate_limiter - .update_counters(namespace, &(&values).into(), 1) + .update_counters(namespace, &values.into(), 1) .await .unwrap(); @@ -506,7 +506,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); for i in 0..max_hits { assert!( @@ -544,7 +544,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); for i in 0..max_hits { assert!( @@ -592,12 +592,12 @@ mod test { let mut get_values: HashMap = HashMap::new(); get_values.insert("req_method".to_string(), "GET".to_string()); get_values.insert("app_id".to_string(), "test_app_id".to_string()); - let get_ctx = (&get_values).into(); + let get_ctx = get_values.into(); let mut post_values: HashMap = HashMap::new(); post_values.insert("req_method".to_string(), "POST".to_string()); post_values.insert("app_id".to_string(), "test_app_id".to_string()); - let post_ctx = (&post_values).into(); + let post_ctx = post_values.into(); for i in 0..max_hits { assert!( @@ -652,7 +652,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); // Report 5 hits twice. The limit is 10, so the first limited call should be // the third one. @@ -688,7 +688,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!(rate_limiter .is_rate_limited(namespace, &ctx, max + 1) @@ -712,12 +712,13 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); for i in 0..max_hits { // Add an extra value that does not apply to the limit on each // iteration. It should not affect. + let mut values = values.clone(); values.insert("does_not_apply".to_string(), i.to_string()); + let ctx = values.into(); assert!( !rate_limiter @@ -731,6 +732,7 @@ mod test { .await .unwrap(); } + let ctx = values.into(); assert!(rate_limiter .is_rate_limited(namespace, &ctx, 1) .await @@ -742,7 +744,7 @@ mod test { ) { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!(!rate_limiter .is_rate_limited("test_namespace", &ctx, 1) @@ -769,7 +771,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "POST".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!(!rate_limiter .is_rate_limited(namespace, &ctx, 1) @@ -792,7 +794,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!(rate_limiter .is_rate_limited(namespace, &ctx, 1) @@ -817,7 +819,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); for _ in 0..max_hits { assert!( @@ -855,7 +857,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); for hit in 0..max_hits { let result = rate_limiter @@ -907,7 +909,7 @@ mod test { values.insert("app_id".to_string(), "test_app_id".to_string()); // Does not match the limit defined values.insert("req_method".to_string(), "POST".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!( !rate_limiter @@ -935,7 +937,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!( rate_limiter @@ -965,13 +967,16 @@ mod test { let mut values = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); rate_limiter .update_counters(namespace, &ctx, hits_app_1) .await .unwrap(); + let mut values = HashMap::new(); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "2".to_string()); + let ctx = values.into(); rate_limiter .update_counters(namespace, &ctx, hits_app_2) .await @@ -1044,7 +1049,7 @@ mod test { let mut values = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); rate_limiter .update_counters(namespace, &ctx, 1) .await @@ -1109,7 +1114,7 @@ mod test { let mut values = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); rate_limiter .update_counters(namespace, &ctx, hits_to_report) .await @@ -1262,7 +1267,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); for i in 0..max_hits { // Alternate between the two rate limiters