fix(flags): handle deleted flags and some property matching edge cases

rust/feature-flags/src/client/redis.rs

@@ -61,7 +61,7 @@ impl Client for RedisClient {
         let mut conn = self.client.get_async_connection().await?;
 
         let results = conn.zrangebyscore(k, min, max);
-        let fut = timeout(Duration::from_secs(REDIS_TIMEOUT_MILLISECS), results).await?;
+        let fut = timeout(Duration::from_millis(REDIS_TIMEOUT_MILLISECS), results).await?;
 
         Ok(fut?)
     }
@@ -76,7 +76,7 @@ impl Client for RedisClient {
 
         let count = count.unwrap_or(1);
         let results = conn.hincr(k, v, count);
-        let fut = timeout(Duration::from_secs(REDIS_TIMEOUT_MILLISECS), results).await?;
+        let fut = timeout(Duration::from_millis(REDIS_TIMEOUT_MILLISECS), results).await?;
 
         fut.map_err(CustomRedisError::from)
     }
@@ -86,7 +86,7 @@ impl Client for RedisClient {
 
         let results = conn.get(k);
         let fut: Result<Vec<u8>, RedisError> =
-            timeout(Duration::from_secs(REDIS_TIMEOUT_MILLISECS), results).await?;
+            timeout(Duration::from_millis(REDIS_TIMEOUT_MILLISECS), results).await?;
 
         // return NotFound error when empty or not found
         if match &fut {
@@ -96,9 +96,11 @@ impl Client for RedisClient {
             return Err(CustomRedisError::NotFound);
         }
 
+        let raw_bytes = fut?;
+
         // TRICKY: We serialise data to json, then django pickles it.
         // Here we deserialize the bytes using serde_pickle, to get the json string.
-        let string_response: String = serde_pickle::from_slice(&fut?, Default::default())?;
+        let string_response: String = serde_pickle::from_slice(&raw_bytes, Default::default())?;
 
         Ok(string_response)
     }
@@ -111,7 +113,7 @@ impl Client for RedisClient {
         let mut conn = self.client.get_async_connection().await?;
 
         let results = conn.set(k, bytes);
-        let fut = timeout(Duration::from_secs(REDIS_TIMEOUT_MILLISECS), results).await?;
+        let fut = timeout(Duration::from_millis(REDIS_TIMEOUT_MILLISECS), results).await?;
 
         Ok(fut?)
     }
@@ -120,7 +122,7 @@ impl Client for RedisClient {
         let mut conn = self.client.get_async_connection().await?;
 
         let results = conn.del(k);
-        let fut = timeout(Duration::from_secs(REDIS_TIMEOUT_MILLISECS), results).await?;
+        let fut = timeout(Duration::from_millis(REDIS_TIMEOUT_MILLISECS), results).await?;
 
         fut.map_err(CustomRedisError::from)
     }
@@ -130,7 +132,7 @@ impl Client for RedisClient {
 
         let results = conn.hget(k, field);
         let fut: Result<Option<String>, RedisError> =
-            timeout(Duration::from_secs(REDIS_TIMEOUT_MILLISECS), results).await?;
+            timeout(Duration::from_millis(REDIS_TIMEOUT_MILLISECS), results).await?;
 
         match fut? {
             Some(value) => Ok(value),

rust/feature-flags/src/cohort/cohort_models.rs

@@ -9,7 +9,7 @@ pub struct Cohort {
     pub description: Option<String>,
     pub team_id: i32,
     pub deleted: bool,
-    pub filters: serde_json::Value,
+    pub filters: Option<serde_json::Value>,
     pub query: Option<serde_json::Value>,
     pub version: Option<i32>,
     pub pending_version: Option<i32>,

rust/feature-flags/src/cohort/cohort_operations.rs

@@ -38,8 +38,13 @@ impl Cohort {
     // https://github.com/PostHog/posthog/blob/feat/dynamic-cohorts-rust/posthog/models/cohort/cohort.py#L114-L169
     // I'll handle that in a separate PR.
     pub fn parse_filters(&self) -> Result<Vec<PropertyFilter>, FlagError> {
-        let cohort_property: CohortProperty = serde_json::from_value(self.filters.clone())
-            .map_err(|e| {
+        let filters = match &self.filters {
+            Some(filters) => filters,
+            None => return Ok(Vec::new()), // Return empty vec if no filters
+        };
+
+        let cohort_property: CohortProperty =
+            serde_json::from_value(filters.to_owned()).map_err(|e| {
                 tracing::error!("Failed to parse filters for cohort {}: {}", self.id, e);
                 FlagError::CohortFiltersParsingError
             })?;
@@ -51,8 +56,13 @@ impl Cohort {
 
     /// Extracts dependent CohortIds from the cohort's filters
     pub fn extract_dependencies(&self) -> Result<HashSet<CohortId>, FlagError> {
-        let cohort_property: CohortProperty = serde_json::from_value(self.filters.clone())
-            .map_err(|e| {
+        let filters = match &self.filters {
+            Some(filters) => filters,
+            None => return Ok(HashSet::new()), // Return empty set if no filters
+        };
+
+        let cohort_property: CohortProperty =
+            serde_json::from_value(filters.clone()).map_err(|e| {
                 tracing::error!("Failed to parse filters for cohort {}: {}", self.id, e);
                 FlagError::CohortFiltersParsingError
             })?;
@@ -208,7 +218,9 @@ mod tests {
             description: None,
             team_id: 1,
             deleted: false,
-            filters: json!({"properties": {"type": "OR", "values": [{"type": "OR", "values": [{"key": "$initial_browser_version", "type": "person", "value": ["125"], "negation": false, "operator": "exact"}]}]}}),
+            filters: Some(
+                json!({"properties": {"type": "OR", "values": [{"type": "OR", "values": [{"key": "$initial_browser_version", "type": "person", "value": ["125"], "negation": false, "operator": "exact"}]}]}}),
+            ),
             query: None,
             version: None,
             pending_version: None,

rust/feature-flags/src/flags/flag_operations.rs

@@ -81,7 +81,7 @@ impl FeatureFlagList {
             FlagError::DatabaseUnavailable
         })?;
 
-        let query = "SELECT id, team_id, name, key, filters, deleted, active, ensure_experience_continuity FROM posthog_featureflag WHERE team_id = $1";
+        let query = "SELECT id, team_id, name, key, filters, deleted, active, ensure_experience_continuity FROM posthog_featureflag WHERE team_id = $1 AND deleted = false";
         let flags_row = sqlx::query_as::<_, FeatureFlagRow>(query)
             .bind(team_id)
             .fetch_all(&mut *conn)
@@ -969,10 +969,7 @@ mod tests {
             .expect("Failed to fetch flags from Redis");
 
         assert_eq!(redis_flags.flags.len(), 2);
-        assert!(redis_flags
-            .flags
-            .iter()
-            .any(|f| f.key == "deleted_flag" && f.deleted));
+        assert!(redis_flags.flags.iter().any(|f| f.deleted));
         assert!(redis_flags
             .flags
             .iter()
@@ -983,15 +980,12 @@ mod tests {
             .await
             .expect("Failed to fetch flags from Postgres");
 
-        assert_eq!(pg_flags.flags.len(), 2);
-        assert!(pg_flags
-            .flags
-            .iter()
-            .any(|f| f.key == "deleted_flag" && f.deleted));
+        assert_eq!(pg_flags.flags.len(), 1);
+        assert!(!pg_flags.flags.iter().any(|f| f.deleted)); // no deleted flags
         assert!(pg_flags
             .flags
             .iter()
-            .any(|f| f.key == "inactive_flag" && !f.active));
+            .any(|f| f.key == "inactive_flag" && !f.active)); // only inactive flag is left
     }
 
     #[tokio::test]

rust/feature-flags/src/properties/property_matching.rs

@@ -82,7 +82,10 @@ pub fn match_property(
                     Ok(!compute_exact_match(value, match_value))
                 }
             } else {
-                Ok(false)
+                // When value doesn't exist:
+                // - for Exact: it's not a match (false)
+                // - for IsNot: it is a match (true)
+                Ok(operator == OperatorType::IsNot)
             }
         }
         OperatorType::IsSet => Ok(matching_property_values.contains_key(key)),
@@ -111,15 +114,19 @@ pub fn match_property(
                     Ok(!is_contained)
                 }
             } else {
-                // When value doesn't exist, it's not a match
-                Ok(false)
+                // When value doesn't exist:
+                // - for Icontains: it's not a match (false)
+                // - for NotIcontains: it is a match (true)
+                Ok(operator == OperatorType::NotIcontains)
             }
         }
         OperatorType::Regex | OperatorType::NotRegex => {
             if match_value.is_none() {
-                return Ok(false);
+                // When value doesn't exist:
+                // - for Regex: it's not a match (false)
+                // - for NotRegex: it is a match (true)
+                return Ok(operator == OperatorType::NotRegex);
             }
-
             let pattern = match Regex::new(&to_string_representation(value)) {
                 Ok(pattern) => pattern,
                 Err(_) => return Ok(false),
@@ -138,6 +145,8 @@ pub fn match_property(
         }
         OperatorType::Gt | OperatorType::Gte | OperatorType::Lt | OperatorType::Lte => {
             if match_value.is_none() {
+                // When value doesn't exist:
+                // - for Gt/Gte/Lt/Lte: it's not a match (false)
                 return Ok(false);
             }
             // TODO: Move towards only numeric matching of these operators???
@@ -173,6 +182,8 @@ pub fn match_property(
             let parsed_date = determine_parsed_date_for_property_matching(match_value);
 
             if parsed_date.is_none() {
+                // When value doesn't exist:
+                // - for IsDateExact/IsDateAfter/IsDateBefore: it's not a match (false)
                 return Ok(false);
             }
 
@@ -194,8 +205,9 @@ pub fn match_property(
                 Ok(false)
             }
         }
-        // NB: In/NotIn operators should be handled by cohort matching
-        // because by the time we match properties, we've already decomposed the cohort
+        // NB: In/NotIn operators are only for Cohorts,
+        // and should be handled by cohort matching code because
+        // by the time we match properties, we've already decomposed the cohort
         // filter into multiple property filters
         OperatorType::In | OperatorType::NotIn => Err(FlagMatchingError::ValidationError(
             "In/NotIn operators should be handled by cohort matching".to_string(),
@@ -1302,7 +1314,7 @@ mod test_match_properties {
             negation: None,
         };
 
-        assert!(!match_property(
+        assert!(match_property(
             &property_not_icontains,
             &HashMap::from([("key2".to_string(), json!("value"))]),
             false
@@ -1334,7 +1346,7 @@ mod test_match_properties {
             negation: None,
         };
 
-        assert!(!match_property(
+        assert!(match_property(
             &property_not_regex,
             &HashMap::from([("key2".to_string(), json!("value.com"))]),
             false

rust/feature-flags/src/utils/test_utils.rs

@@ -391,7 +391,7 @@ pub async fn insert_cohort_for_team_in_pg(
         description: Some("Description for cohort".to_string()),
         team_id,
         deleted: false,
-        filters,
+        filters: Some(filters),
         query: None,
         version: Some(1),
         pending_version: None,