Only namespace, seconds, conditions and variable make for a Limit's i…

…dentity

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)));
 

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
     }
 }

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)
     }
 

limitador/src/limit.rs

@@ -129,9 +129,7 @@ impl Limit {
 impl Hash for Limit {
     fn hash<H: Hasher>(&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
     }

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<Limit> {
@@ -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
             }
         }
     }

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()),

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);
+    }
 }