From c9cbd71738e165b78c46d16e2a544b5f37546763 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 16 Apr 2024 14:51:38 +0100 Subject: [PATCH 1/2] fix(decide): is not operator matches non-existing persons --- posthog/models/feature_flag/flag_matching.py | 17 +++--- .../test/__snapshots__/test_feature_flag.ambr | 35 +++++++++++- posthog/test/test_feature_flag.py | 54 +++++++++++++++++-- 3 files changed, 93 insertions(+), 13 deletions(-) diff --git a/posthog/models/feature_flag/flag_matching.py b/posthog/models/feature_flag/flag_matching.py index 50f0a5d848149..9cb569255c980 100644 --- a/posthog/models/feature_flag/flag_matching.py +++ b/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,12 +967,11 @@ 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): reason = "flag_condition_retry" elif "Failed to fetch group" in str(err): @@ -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 diff --git a/posthog/test/__snapshots__/test_feature_flag.ambr b/posthog/test/__snapshots__/test_feature_flag.ambr index 84cd8f1298878..df9d02c4049c2 100644 --- a/posthog/test/__snapshots__/test_feature_flag.ambr +++ b/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)) diff --git a/posthog/test/test_feature_flag.py b/posthog/test/test_feature_flag.py index c9f32b526ee3d..38afbe7dbbcd7 100644 --- a/posthog/test/test_feature_flag.py +++ b/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, From a89cb730a53c145395f399eeff8448270abf82a7 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Tue, 16 Apr 2024 14:54:53 +0100 Subject: [PATCH 2/2] fix --- posthog/models/feature_flag/flag_matching.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/posthog/models/feature_flag/flag_matching.py b/posthog/models/feature_flag/flag_matching.py index 9cb569255c980..e9faf83effa4f 100644 --- a/posthog/models/feature_flag/flag_matching.py +++ b/posthog/models/feature_flag/flag_matching.py @@ -972,7 +972,7 @@ def parse_exception_for_error_message(err: Exception): reason = "timeout" elif "no more connections" in str(err): reason = "no_more_connections" - 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"