fix(decide): is not operator matches non-existing persons (#21576)

posthog/models/feature_flag/flag_matching.py

@@ -7,7 +7,7 @@
 
 from prometheus_client import Counter
 from django.conf import settings
-from django.db import DatabaseError, IntegrityError, OperationalError
+from django.db import DatabaseError, IntegrityError
 from django.db.models.expressions import ExpressionWrapper, RawSQL
 from django.db.models.fields import BooleanField
 from django.db.models import Q, Func, F, CharField
@@ -329,7 +329,7 @@ def is_condition_match(
                     )
                 condition_match = all(match_property(property, target_properties) for property in properties)
             else:
-                match_if_entity_doesnt_exist = check_pure_is_not_set_operator_condition(condition)
+                match_if_entity_doesnt_exist = check_pure_is_not_operator_condition(condition)
                 condition_match = self._condition_matches(
                     feature_flag,
                     condition_index,
@@ -425,7 +425,7 @@ def query_conditions(self) -> Dict[str, bool]:
 
                 person_fields: List[str] = []
 
-                for existence_condition_key in self.has_pure_is_not_set_conditions:
+                for existence_condition_key in self.has_pure_is_not_conditions:
                     if existence_condition_key == PERSON_KEY:
                         person_exists = person_query.exists()
                         all_conditions[f"{ENTITY_EXISTS_PREFIX}{PERSON_KEY}"] = person_exists
@@ -659,11 +659,11 @@ def get_highest_priority_match_evaluation(
         return current_match, current_index
 
     @cached_property
-    def has_pure_is_not_set_conditions(self) -> set[Literal["person"] | GroupTypeIndex]:
+    def has_pure_is_not_conditions(self) -> set[Literal["person"] | GroupTypeIndex]:
         entity_to_condition_check: set[Literal["person"] | GroupTypeIndex] = set()
         for feature_flag in self.feature_flags:
             for condition in feature_flag.conditions:
-                if check_pure_is_not_set_operator_condition(condition):
+                if check_pure_is_not_operator_condition(condition):
                     if feature_flag.aggregation_group_type_index is not None:
                         entity_to_condition_check.add(feature_flag.aggregation_group_type_index)
                     else:
@@ -967,13 +967,12 @@ def handle_feature_flag_exception(err: Exception, log_message: str = "", set_hea
 
 def parse_exception_for_error_message(err: Exception):
     reason = "unknown"
-    if isinstance(err, OperationalError):
+    if isinstance(err, DatabaseError):
         if "statement timeout" in str(err):
             reason = "timeout"
         elif "no more connections" in str(err):
             reason = "no_more_connections"
-    elif isinstance(err, DatabaseError):
-        if "Failed to fetch conditions" in str(err):
+        elif "Failed to fetch conditions" in str(err):
             reason = "flag_condition_retry"
         elif "Failed to fetch group" in str(err):
             reason = "group_mapping_retry"
@@ -1037,8 +1036,8 @@ def add_local_person_and_group_properties(distinct_id, groups, person_properties
     return all_person_properties, all_group_properties
 
 
-def check_pure_is_not_set_operator_condition(condition: dict) -> bool:
+def check_pure_is_not_operator_condition(condition: dict) -> bool:
     properties = condition.get("properties", [])
-    if properties and all(prop.get("operator") == "is_not_set" for prop in properties):
+    if properties and all(prop.get("operator") in ("is_not_set", "is_not") for prop in properties):
         return True
     return False

posthog/test/__snapshots__/test_feature_flag.ambr

@@ -1,5 +1,16 @@
 # serializer version: 1
 # name: TestFeatureFlagMatcher.test_coercion_of_booleans_with_is_not_operator
+  '''
+  SELECT 1 AS "a"
+  FROM "posthog_person"
+  INNER JOIN "posthog_persondistinctid" ON ("posthog_person"."id" = "posthog_persondistinctid"."person_id")
+  WHERE ("posthog_persondistinctid"."distinct_id" = '307'
+         AND "posthog_persondistinctid"."team_id" = 2
+         AND "posthog_person"."team_id" = 2)
+  LIMIT 1
+  '''
+# ---
+# name: TestFeatureFlagMatcher.test_coercion_of_booleans_with_is_not_operator.1
   '''
   SELECT NOT ((("posthog_person"."properties" -> 'disabled') = 'false'::jsonb
                OR ("posthog_person"."properties" -> 'disabled') = '"false"'::jsonb)
@@ -45,6 +56,17 @@
   '''
 # ---
 # name: TestFeatureFlagMatcher.test_coercion_of_strings_and_numbers_with_is_not_operator
+  '''
+  SELECT 1 AS "a"
+  FROM "posthog_person"
+  INNER JOIN "posthog_persondistinctid" ON ("posthog_person"."id" = "posthog_persondistinctid"."person_id")
+  WHERE ("posthog_persondistinctid"."distinct_id" = '307'
+         AND "posthog_persondistinctid"."team_id" = 2
+         AND "posthog_person"."team_id" = 2)
+  LIMIT 1
+  '''
+# ---
+# name: TestFeatureFlagMatcher.test_coercion_of_strings_and_numbers_with_is_not_operator.1
   '''
   SELECT (NOT ((("posthog_person"."properties" -> 'Organizer Id') IN ('"307"'::jsonb)
                 OR ("posthog_person"."properties" -> 'Organizer Id') IN ('307'::jsonb))
@@ -67,7 +89,18 @@
          AND "posthog_person"."team_id" = 2)
   '''
 # ---
-# name: TestFeatureFlagMatcher.test_coercion_of_strings_and_numbers_with_is_not_operator.1
+# name: TestFeatureFlagMatcher.test_coercion_of_strings_and_numbers_with_is_not_operator.2
+  '''
+  SELECT 1 AS "a"
+  FROM "posthog_person"
+  INNER JOIN "posthog_persondistinctid" ON ("posthog_person"."id" = "posthog_persondistinctid"."person_id")
+  WHERE ("posthog_persondistinctid"."distinct_id" = '307'
+         AND "posthog_persondistinctid"."team_id" = 2
+         AND "posthog_person"."team_id" = 2)
+  LIMIT 1
+  '''
+# ---
+# name: TestFeatureFlagMatcher.test_coercion_of_strings_and_numbers_with_is_not_operator.3
   '''
   SELECT NOT ((("posthog_person"."properties" -> 'Distinct Id') IN ('"307"'::jsonb)
                OR ("posthog_person"."properties" -> 'Distinct Id') IN ('307'::jsonb))

posthog/test/test_feature_flag.py

@@ -985,7 +985,7 @@ def test_coercion_of_strings_and_numbers_with_is_not_operator(self):
             },
         )
 
-        with snapshot_postgres_queries_context(self), self.assertNumQueries(4):
+        with snapshot_postgres_queries_context(self), self.assertNumQueries(5):
             self.assertEqual(
                 self.match_flag(feature_flag, "307"),
                 FeatureFlagMatch(False, None, FeatureFlagMatchReason.NO_CONDITION_MATCH, 0),
@@ -1014,7 +1014,7 @@ def test_coercion_of_strings_and_numbers_with_is_not_operator(self):
         )
 
         # test with a flag where the property is a number
-        with snapshot_postgres_queries_context(self), self.assertNumQueries(4):
+        with snapshot_postgres_queries_context(self), self.assertNumQueries(5):
             self.assertEqual(
                 self.match_flag(feature_flag2, "307"),
                 FeatureFlagMatch(False, None, FeatureFlagMatchReason.NO_CONDITION_MATCH, 3),
@@ -1493,7 +1493,7 @@ def test_coercion_of_booleans_with_is_not_operator(self):
             FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 1),
         )
 
-        with snapshot_postgres_queries_context(self), self.assertNumQueries(4):
+        with snapshot_postgres_queries_context(self), self.assertNumQueries(5):
             self.assertEqual(
                 self.match_flag(feature_flag2, "307"),
                 FeatureFlagMatch(False, None, FeatureFlagMatchReason.NO_CONDITION_MATCH, 8),
@@ -1625,6 +1625,11 @@ def test_non_existing_key_passes_is_not_check(self):
             distinct_ids=["307"],
             properties={},
         )
+        Person.objects.create(
+            team=self.team,
+            distinct_ids=["309"],
+            properties={"Distinct Id": "307"},
+        )
         feature_flag = self.create_feature_flag(
             key="random",
             filters={
@@ -1648,8 +1653,14 @@ def test_non_existing_key_passes_is_not_check(self):
             FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0),
         )
 
+        # person doesn't exist, meaning the property doesn't exist, so it should pass the is_not check
         self.assertEqual(
             self.match_flag(feature_flag, "308"),
+            FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0),
+        )
+
+        self.assertEqual(
+            self.match_flag(feature_flag, "309"),
             FeatureFlagMatch(False, None, FeatureFlagMatchReason.NO_CONDITION_MATCH, 0),
         )
 
@@ -3322,6 +3333,43 @@ def test_non_existing_person_with_is_not_set(self):
                 FeatureFlagMatch(False, None, FeatureFlagMatchReason.NO_CONDITION_MATCH, 0),
             )
 
+    def test_is_not_equal_with_non_existing_person(self):
+        feature_flag = self.create_feature_flag(
+            filters={
+                "groups": [
+                    {
+                        "properties": [
+                            {"key": "$initial_utm_source", "type": "person", "value": ["fb"], "operator": "is_not"}
+                        ]
+                    }
+                ]
+            }
+        )
+
+        # one extra query to check existence
+        with self.assertNumQueries(5):
+            self.assertEqual(
+                FeatureFlagMatcher([feature_flag], "not-seen-person").get_match(feature_flag),
+                FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0),
+            )
+
+            self.assertEqual(
+                FeatureFlagMatcher(
+                    [feature_flag],
+                    "not-seen-person",
+                    property_value_overrides={"$initial_utm_source": "fb"},
+                ).get_match(feature_flag),
+                FeatureFlagMatch(False, None, FeatureFlagMatchReason.NO_CONDITION_MATCH, 0),
+            )
+            self.assertEqual(
+                FeatureFlagMatcher(
+                    [feature_flag],
+                    "not-seen-person",
+                    property_value_overrides={"$initial_utm_source": "fbx"},
+                ).get_match(feature_flag),
+                FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0),
+            )
+
     def test_is_not_set_operator_with_overrides(self):
         Person.objects.create(
             team=self.team,