From 266c3ff5fd005fe76b2b20c7e9b4641096550f55 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Wed, 22 Nov 2023 12:31:52 +0000 Subject: [PATCH] fix(flags): Dont locally match cohorts when id property is provided (#18814) --- posthog/models/feature_flag/flag_matching.py | 4 ++ posthog/test/test_feature_flag.py | 74 ++++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/posthog/models/feature_flag/flag_matching.py b/posthog/models/feature_flag/flag_matching.py index fbdf09b56dd7c..d81f44bd61807 100644 --- a/posthog/models/feature_flag/flag_matching.py +++ b/posthog/models/feature_flag/flag_matching.py @@ -588,6 +588,10 @@ def can_compute_locally( self.cache.group_type_index_to_name[group_type_index], {} ) for property in properties: + # can't locally compute if property is a cohort + # need to atleast fetch the cohort + if property.type == "cohort": + return False if property.key not in target_properties: return False if property.operator == "is_not_set": diff --git a/posthog/test/test_feature_flag.py b/posthog/test/test_feature_flag.py index da323dd5cb32f..d1056f26e5722 100644 --- a/posthog/test/test_feature_flag.py +++ b/posthog/test/test_feature_flag.py @@ -3458,6 +3458,80 @@ def test_cohort_filters_with_override_properties(self): FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0), ) + def test_cohort_filters_with_override_id_property(self): + cohort1 = Cohort.objects.create( + team=self.team, + groups=[ + { + "properties": [ + { + "key": "email", + "type": "person", + "value": "@posthog.com", + "negation": False, + "operator": "icontains", + } + ] + } + ], + name="cohort1", + ) + + feature_flag1: FeatureFlag = self.create_feature_flag( + key="x1", + filters={"groups": [{"properties": [{"key": "id", "value": cohort1.pk, "type": "cohort"}]}]}, + ) + Person.objects.create( + team=self.team, + distinct_ids=["example_id"], + properties={"email": "tim@posthog.com"}, + ) + + with self.assertNumQueries(5): + self.assertEqual( + FeatureFlagMatcher( + [feature_flag1], + "example_id", + property_value_overrides={}, + ).get_match(feature_flag1), + FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0), + ) + + with self.assertNumQueries(4): + # no local computation because cohort lookup is required + # no postgres person query required here to get the person, because email is sufficient + # property id override shouldn't confuse the matcher + self.assertEqual( + FeatureFlagMatcher( + [feature_flag1], + "example_id", + property_value_overrides={"id": "example_id", "email": "bzz"}, + ).get_match(feature_flag1), + FeatureFlagMatch(False, None, FeatureFlagMatchReason.NO_CONDITION_MATCH, 0), + ) + + with self.assertNumQueries(4): + # no postgres query required here to get the person + self.assertEqual( + FeatureFlagMatcher( + [feature_flag1], + "example_id", + property_value_overrides={"id": "second_id", "email": "neil@posthog.com"}, + ).get_match(feature_flag1), + FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0), + ) + + with self.assertNumQueries(5): + # postgres query required here to get the person + self.assertEqual( + FeatureFlagMatcher( + [feature_flag1], + "example_id", + property_value_overrides={"id": "second_id"}, + ).get_match(feature_flag1), + FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0), + ) + @pytest.mark.skip("This case is not supported yet") def test_complex_cohort_filter_with_override_properties(self): # TODO: Currently we don't support this case for persons who haven't been ingested yet