diff --git a/ee/clickhouse/views/test/__snapshots__/test_clickhouse_experiments.ambr b/ee/clickhouse/views/test/__snapshots__/test_clickhouse_experiments.ambr index b1eb79a39945c..525c25fb7bf3a 100644 --- a/ee/clickhouse/views/test/__snapshots__/test_clickhouse_experiments.ambr +++ b/ee/clickhouse/views/test/__snapshots__/test_clickhouse_experiments.ambr @@ -227,97 +227,26 @@ # --- # name: ClickhouseTestFunnelExperimentResults.test_experiment_flow_with_event_results_and_events_out_of_time_range_timezones.1 ''' - /* user_id:0 request:_snapshot_ */ - SELECT array(replaceRegexpAll(JSONExtractRaw(properties, '$feature/a-b-test'), '^"|"$', '')) AS value, - count(*) as count - FROM events e - WHERE team_id = 2 - AND event IN ['$pageleave', '$pageview'] - AND toTimeZone(timestamp, 'Europe/Amsterdam') >= toDateTime('2020-01-01 14:20:21', 'Europe/Amsterdam') - AND toTimeZone(timestamp, 'Europe/Amsterdam') <= toDateTime('2020-01-06 10:00:00', 'Europe/Amsterdam') - GROUP BY value - ORDER BY count DESC, value DESC - LIMIT 26 - OFFSET 0 + /* celery:posthog.tasks.tasks.sync_insight_caching_state */ + SELECT team_id, + date_diff('second', max(timestamp), now()) AS age + FROM events + WHERE timestamp > date_sub(DAY, 3, now()) + AND timestamp < now() + GROUP BY team_id + ORDER BY age; ''' # --- # name: ClickhouseTestFunnelExperimentResults.test_experiment_flow_with_event_results_and_events_out_of_time_range_timezones.2 ''' - /* user_id:0 request:_snapshot_ */ - SELECT countIf(steps = 1) step_1, - countIf(steps = 2) step_2, - avg(step_1_average_conversion_time_inner) step_1_average_conversion_time, - median(step_1_median_conversion_time_inner) step_1_median_conversion_time, - prop - FROM - (SELECT aggregation_target, - steps, - avg(step_1_conversion_time) step_1_average_conversion_time_inner, - median(step_1_conversion_time) step_1_median_conversion_time_inner , - prop - FROM - (SELECT aggregation_target, - steps, - max(steps) over (PARTITION BY aggregation_target, - prop) as max_steps, - step_1_conversion_time , - prop - FROM - (SELECT *, - if(latest_0 <= latest_1 - AND latest_1 <= latest_0 + INTERVAL 14 DAY, 2, 1) AS steps , - if(isNotNull(latest_1) - AND latest_1 <= latest_0 + INTERVAL 14 DAY, dateDiff('second', toDateTime(latest_0), toDateTime(latest_1)), NULL) step_1_conversion_time, - prop - FROM - (SELECT aggregation_target, timestamp, step_0, - latest_0, - step_1, - min(latest_1) over (PARTITION by aggregation_target, - prop - ORDER BY timestamp DESC ROWS BETWEEN UNBOUNDED PRECEDING AND 0 PRECEDING) latest_1 , - if(has([['test'], ['control']], prop), prop, ['Other']) as prop - FROM - (SELECT *, - if(notEmpty(arrayFilter(x -> notEmpty(x), prop_vals)), prop_vals, ['']) as prop - FROM - (SELECT e.timestamp as timestamp, - pdi.person_id as aggregation_target, - pdi.person_id as person_id, - if(event = '$pageview', 1, 0) as step_0, - if(step_0 = 1, timestamp, null) as latest_0, - if(event = '$pageleave', 1, 0) as step_1, - if(step_1 = 1, timestamp, null) as latest_1, - array(replaceRegexpAll(JSONExtractRaw(properties, '$feature/a-b-test'), '^"|"$', '')) AS prop_basic, - prop_basic as prop, - argMinIf(prop, timestamp, notEmpty(arrayFilter(x -> notEmpty(x), prop))) over (PARTITION by aggregation_target) as prop_vals - FROM events e - INNER JOIN - (SELECT distinct_id, - argMax(person_id, version) as person_id - FROM person_distinct_id2 - WHERE team_id = 2 - AND distinct_id IN - (SELECT distinct_id - FROM events - WHERE team_id = 2 - AND event IN ['$pageleave', '$pageview'] - AND toTimeZone(timestamp, 'Europe/Amsterdam') >= toDateTime('2020-01-01 14:20:21', 'Europe/Amsterdam') - AND toTimeZone(timestamp, 'Europe/Amsterdam') <= toDateTime('2020-01-06 10:00:00', 'Europe/Amsterdam') ) - GROUP BY distinct_id - HAVING argMax(is_deleted, version) = 0) AS pdi ON e.distinct_id = pdi.distinct_id - WHERE team_id = 2 - AND event IN ['$pageleave', '$pageview'] - AND toTimeZone(timestamp, 'Europe/Amsterdam') >= toDateTime('2020-01-01 14:20:21', 'Europe/Amsterdam') - AND toTimeZone(timestamp, 'Europe/Amsterdam') <= toDateTime('2020-01-06 10:00:00', 'Europe/Amsterdam') - AND (step_0 = 1 - OR step_1 = 1) ))) - WHERE step_0 = 1 )) - GROUP BY aggregation_target, - steps, - prop - HAVING steps = max_steps) - GROUP BY prop + /* celery:posthog.tasks.tasks.sync_insight_caching_state */ + SELECT team_id, + date_diff('second', max(timestamp), now()) AS age + FROM events + WHERE timestamp > date_sub(DAY, 3, now()) + AND timestamp < now() + GROUP BY team_id + ORDER BY age; ''' # --- # name: ClickhouseTestFunnelExperimentResults.test_experiment_flow_with_event_results_and_events_out_of_time_range_timezones.3 diff --git a/posthog/api/feature_flag.py b/posthog/api/feature_flag.py index 029a3186d4365..62647ff923925 100644 --- a/posthog/api/feature_flag.py +++ b/posthog/api/feature_flag.py @@ -246,10 +246,17 @@ def properties_all_match(predicate): detail=f"Invalid date value: {prop.value}", code="invalid_date" ) - # make sure regex and icontains properties have string values - if prop.operator in ["regex", "icontains", "not_regex", "not_icontains"] and not isinstance( - prop.value, str - ): + # make sure regex, icontains, gte, lte, lt, and gt properties have string values + if prop.operator in [ + "regex", + "icontains", + "not_regex", + "not_icontains", + "gte", + "lte", + "gt", + "lt", + ] and not isinstance(prop.value, str): raise serializers.ValidationError( detail=f"Invalid value for operator {prop.operator}: {prop.value}", code="invalid_value" ) diff --git a/posthog/api/test/__snapshots__/test_feature_flag.ambr b/posthog/api/test/__snapshots__/test_feature_flag.ambr index 475b0ab956bb2..0eb5a83cdcc17 100644 --- a/posthog/api/test/__snapshots__/test_feature_flag.ambr +++ b/posthog/api/test/__snapshots__/test_feature_flag.ambr @@ -1801,26 +1801,6 @@ LIMIT 100 SETTINGS optimize_aggregation_in_order = 1 ''' # --- -# name: TestFeatureFlag.test_creating_static_cohort.16 - ''' - /* user_id:0 request:_snapshot_ */ - SELECT id - FROM person - INNER JOIN - (SELECT person_id - FROM person_static_cohort - WHERE team_id = 2 - AND cohort_id = 2 - GROUP BY person_id, - cohort_id, - team_id) cohort_persons ON cohort_persons.person_id = person.id - WHERE team_id = 2 - GROUP BY id - HAVING max(is_deleted) = 0 - ORDER BY argMax(person.created_at, version) DESC, id DESC - LIMIT 100 SETTINGS optimize_aggregation_in_order = 1 - ''' -# --- # name: TestFeatureFlag.test_creating_static_cohort.2 ''' SELECT "posthog_organizationmembership"."id", diff --git a/posthog/api/test/test_decide.py b/posthog/api/test/test_decide.py index fd265ea60b98d..34009fdf4ea12 100644 --- a/posthog/api/test/test_decide.py +++ b/posthog/api/test/test_decide.py @@ -2200,6 +2200,149 @@ def test_flag_with_regular_cohorts(self, *args): self.assertEqual(response.json()["featureFlags"], {"cohort-flag": False}) self.assertEqual(response.json()["errorsWhileComputingFlags"], False) + def test_flag_with_invalid_cohort_filter_condition(self, *args): + self.team.app_urls = ["https://example.com"] + self.team.save() + self.client.logout() + + person1_distinct_id = "example_id" + Person.objects.create( + team=self.team, + distinct_ids=[person1_distinct_id], + properties={"registration_ts": 1716447600}, + ) + + # Create a cohort with an invalid filter condition (tis broken filter came from this issue: https://github.com/PostHog/posthog/issues/23213) + # The invalid condition is that the registration_ts property is compared against a list of values + # Since this filter must match everything, the flag should evaluate to False + cohort = Cohort.objects.create( + team=self.team, + filters={ + "properties": { + "type": "OR", + "values": [ + { + "type": "AND", + "values": [ + # This is the valid condition + { + "key": "registration_ts", + "type": "person", + "value": "1716274800", + "operator": "gte", + }, + # This is the invalid condition (lte operator comparing against a list of values) + { + "key": "registration_ts", + "type": "person", + "value": ["1716447600"], + "operator": "lte", + }, + ], + } + ], + } + }, + name="Test cohort", + ) + + # Create a feature flag that uses the cohort + FeatureFlag.objects.create( + team=self.team, + filters={ + "groups": [ + { + "properties": [ + { + "key": "id", + "type": "cohort", + "value": cohort.pk, + } + ], + } + ] + }, + name="This is a cohort-based flag", + key="cohort-flag", + created_by=self.user, + ) + + with self.assertNumQueries(5): + response = self._post_decide(api_version=3, distinct_id=person1_distinct_id) + self.assertEqual(response.json()["featureFlags"], {"cohort-flag": False}) + self.assertEqual(response.json()["errorsWhileComputingFlags"], False) + + def test_flag_with_invalid_but_safe_cohort_filter_condition(self, *args): + self.team.app_urls = ["https://example.com"] + self.team.save() + self.client.logout() + + person1_distinct_id = "example_id" + Person.objects.create( + team=self.team, + distinct_ids=[person1_distinct_id], + properties={"registration_ts": 1716447600}, + ) + + # Create a cohort with a safe OR filter that contains an invalid condition + # it should still evaluate the FeatureFlag to True + cohort = Cohort.objects.create( + team=self.team, + filters={ + "properties": { + "type": "OR", + "values": [ + { + "type": "OR", + "values": [ + # This is the valid condition + { + "key": "registration_ts", + "type": "person", + "value": "1716274800", + "operator": "gte", + }, + # This is the invalid condition (lte operator comparing against a list of values) + { + "key": "registration_ts", + "type": "person", + "value": ["1716447600"], + "operator": "lte", + }, + ], + } + ], + } + }, + name="Test cohort", + ) + + # Create a feature flag that uses the cohort + FeatureFlag.objects.create( + team=self.team, + filters={ + "groups": [ + { + "properties": [ + { + "key": "id", + "type": "cohort", + "value": cohort.pk, + } + ], + } + ] + }, + name="This is a cohort-based flag", + key="cohort-flag", + created_by=self.user, + ) + + with self.assertNumQueries(5): + response = self._post_decide(api_version=3, distinct_id=person1_distinct_id) + self.assertEqual(response.json()["featureFlags"], {"cohort-flag": True}) + self.assertEqual(response.json()["errorsWhileComputingFlags"], False) + def test_flag_with_unknown_cohort(self, *args): self.team.app_urls = ["https://example.com"] self.team.save() diff --git a/posthog/api/test/test_feature_flag.py b/posthog/api/test/test_feature_flag.py index 3536f0b8e7352..03fccd5f3b730 100644 --- a/posthog/api/test/test_feature_flag.py +++ b/posthog/api/test/test_feature_flag.py @@ -86,139 +86,45 @@ def test_cant_create_flag_with_duplicate_key(self): def test_cant_create_flag_with_invalid_filters(self): count = FeatureFlag.objects.count() - response = self.client.post( - f"/api/projects/{self.team.id}/feature_flags", - { - "name": "Beta feature", - "key": "beta-x", - "filters": { - "groups": [ - { - "rollout_percentage": 65, - "properties": [ - { - "key": "email", - "type": "person", - "value": ["@posthog.com"], - "operator": "icontains", - } - ], - } - ] - }, - }, - ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual( - response.json(), - { - "type": "validation_error", - "code": "invalid_value", - "detail": "Invalid value for operator icontains: ['@posthog.com']", - "attr": "filters", - }, - ) + invalid_operators = ["icontains", "regex", "not_icontains", "not_regex", "lt", "gt", "lte", "gte"] - response = self.client.post( - f"/api/projects/{self.team.id}/feature_flags", - { - "name": "Beta feature", - "key": "beta-x", - "filters": { - "groups": [ - { - "rollout_percentage": 65, - "properties": [ - { - "key": "email", - "type": "person", - "value": ["@posthog.com"], - "operator": "regex", - } - ], - } - ] + for operator in invalid_operators: + response = self.client.post( + f"/api/projects/{self.team.id}/feature_flags", + { + "name": "Beta feature", + "key": "beta-x", + "filters": { + "groups": [ + { + "rollout_percentage": 65, + "properties": [ + { + "key": "email", + "type": "person", + "value": ["@posthog.com"], + "operator": operator, + } + ], + } + ] + }, }, - }, - ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual( - response.json(), - { - "type": "validation_error", - "code": "invalid_value", - "detail": "Invalid value for operator regex: ['@posthog.com']", - "attr": "filters", - }, - ) - - response = self.client.post( - f"/api/projects/{self.team.id}/feature_flags", - { - "name": "Beta feature", - "key": "beta-x", - "filters": { - "groups": [ - { - "rollout_percentage": 65, - "properties": [ - { - "key": "email", - "type": "person", - "value": ["@posthog.com"], - "operator": "not_icontains", - } - ], - } - ] + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual( + response.json(), + { + "type": "validation_error", + "code": "invalid_value", + "detail": f"Invalid value for operator {operator}: ['@posthog.com']", + "attr": "filters", }, - }, - ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual( - response.json(), - { - "type": "validation_error", - "code": "invalid_value", - "detail": "Invalid value for operator not_icontains: ['@posthog.com']", - "attr": "filters", - }, - ) + ) - response = self.client.post( - f"/api/projects/{self.team.id}/feature_flags", - { - "name": "Beta feature", - "key": "beta-x", - "filters": { - "groups": [ - { - "rollout_percentage": 65, - "properties": [ - { - "key": "email", - "type": "person", - "value": ["@posthog.com"], - "operator": "not_regex", - } - ], - } - ] - }, - }, - ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual( - response.json(), - { - "type": "validation_error", - "code": "invalid_value", - "detail": "Invalid value for operator not_regex: ['@posthog.com']", - "attr": "filters", - }, - ) self.assertEqual(FeatureFlag.objects.count(), count) + # Test that a string value is still acceptable response = self.client.post( f"/api/projects/{self.team.id}/feature_flags", { @@ -241,6 +147,7 @@ def test_cant_create_flag_with_invalid_filters(self): }, }, ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) def test_cant_update_flag_with_duplicate_key(self): diff --git a/posthog/models/filters/test/__snapshots__/test_filter.ambr b/posthog/models/filters/test/__snapshots__/test_filter.ambr index 04594a21b0f0e..99b4b95de8e77 100644 --- a/posthog/models/filters/test/__snapshots__/test_filter.ambr +++ b/posthog/models/filters/test/__snapshots__/test_filter.ambr @@ -357,16 +357,6 @@ AND "posthog_person"."team_id" = 2) ''' # --- -# name: TestDjangoPropertiesToQ.test_icontains_with_array_value.2 - ''' - SELECT "posthog_person"."uuid" - FROM "posthog_person" - WHERE (("posthog_person"."properties" -> '$key') > '["2"]'::jsonb - AND "posthog_person"."properties" ? '$key' - AND NOT (("posthog_person"."properties" -> '$key') = 'null'::jsonb) - AND "posthog_person"."team_id" = 2) - ''' -# --- # name: TestDjangoPropertiesToQ.test_person_relative_date_parsing_with_invalid_date ''' SELECT 1 AS "a" diff --git a/posthog/models/filters/test/test_filter.py b/posthog/models/filters/test/test_filter.py index eb99a3ac42941..d7901c721e96a 100644 --- a/posthog/models/filters/test/test_filter.py +++ b/posthog/models/filters/test/test_filter.py @@ -796,6 +796,218 @@ def test_person_relative_date_parsing(self): ) self.assertTrue(matched_person) + def test_person_matching_greater_than_filter(self): + person1_distinct_id = "example_id" + Person.objects.create( + team=self.team, + distinct_ids=[person1_distinct_id], + properties={"registration_ts": 5}, + ) + filter = Filter( + data={"properties": [{"key": "registration_ts", "value": "4", "type": "person", "operator": "gt"}]} + ) + + with self.assertNumQueries(1): + matched_person = ( + Person.objects.annotate( + **{ + "properties_registrationts_68f210b8c014e1b_type": Func( + F("properties__registration_ts"), + function="JSONB_TYPEOF", + output_field=CharField(), + ) + } + ) + .filter( + team_id=self.team.pk, + persondistinctid__distinct_id=person1_distinct_id, + ) + .filter(properties_to_Q(self.team.pk, filter.property_groups.flat)) + .exists() + ) + self.assertTrue(matched_person) + + def test_broken_person_filter_never_matching(self): + person1_distinct_id = "example_id" + Person.objects.create( + team=self.team, + distinct_ids=[person1_distinct_id], + properties={"registration_ts": 1716447600}, + ) + # This broken filter came from this issue: https://github.com/PostHog/posthog/issues/23213 + filter = Filter( + data={ + "properties": { + "type": "OR", + "values": [ + { + "type": "AND", + "values": [ + # This is the valid condition + { + "key": "registration_ts", + "type": "person", + "value": "1716274800", + "negation": False, + "operator": "gte", + }, + # This is the invalid condition (lte operator comparing against a list of values) + { + "key": "registration_ts", + "type": "person", + "value": ["1716447600"], + "negation": False, + "operator": "lte", + }, + ], + } + ], + } + } + ) + + with self.assertNumQueries(1): + matched_person = ( + Person.objects.annotate( + **{ + "properties_registrationts_68f210b8c014e1b_type": Func( + F("properties__registration_ts"), + function="JSONB_TYPEOF", + output_field=CharField(), + ) + } + ) + .filter( + team_id=self.team.pk, + persondistinctid__distinct_id=person1_distinct_id, + ) + .filter(properties_to_Q(self.team.pk, filter.property_groups.flat)) + .exists() + ) + # This shouldn't pass because we have an AND condition with a broken lte operator + # (we should never have a lte operator comparing against a list of values) + # So this should never match + self.assertFalse(matched_person) + + def test_broken_condition_does_not_break_entire_filter(self): + person1_distinct_id = "example_id" + Person.objects.create( + team=self.team, + distinct_ids=[person1_distinct_id], + properties={"registration_ts": 1716447600}, + ) + # Create a cohort with an OR filter that has an invalid condition + # (a lte operator comparing against a list of values) + # This should still evaluate to True, though, because the other condition is valid + cohort = Cohort.objects.create( + team=self.team, + name="Test OR Cohort", + filters={ + "properties": { + "type": "OR", + "values": [ + { + "type": "OR", + # This is the valid condition + "values": [ + { + "key": "registration_ts", + "type": "person", + "value": "1716274800", + "negation": False, + "operator": "gte", + }, + # This is the invalid condition + { + "key": "registration_ts", + "type": "person", + "value": ["1716447600"], + "negation": False, + "operator": "lte", + }, + ], + } + ], + } + }, + ) + filter = Filter(data={"properties": [{"key": "id", "value": cohort.pk, "type": "cohort"}]}) + with self.assertNumQueries(2): + matched_person = ( + Person.objects.annotate( + **{ + "properties_registrationts_68f210b8c014e1b_type": Func( + F("properties__registration_ts"), + function="JSONB_TYPEOF", + output_field=CharField(), + ) + } + ) + .filter( + team_id=self.team.pk, + persondistinctid__distinct_id=person1_distinct_id, + ) + .filter(properties_to_Q(self.team.pk, filter.property_groups.flat)) + .exists() + ) + # This should now pass because the cohort filter still has one valid condition + self.assertTrue(matched_person) + + def test_person_matching_real_filter(self): + person1_distinct_id = "example_id" + Person.objects.create( + team=self.team, + distinct_ids=[person1_distinct_id], + properties={"registration_ts": 1716447600}, + ) + filter = Filter( + data={ + "properties": { + "type": "OR", + "values": [ + { + "type": "AND", + "values": [ + { + "key": "registration_ts", + "type": "person", + "value": "1716274800", + "negation": False, + "operator": "gt", + }, + { + "key": "registration_ts", + "type": "person", + "value": ["1716447600"], + "negation": False, + "operator": "exact", + }, + ], + } + ], + } + } + ) + with self.assertNumQueries(1): + matched_person = ( + Person.objects.annotate( + **{ + "properties_registrationts_68f210b8c014e1b_type": Func( + F("properties__registration_ts"), + function="JSONB_TYPEOF", + output_field=CharField(), + ) + } + ) + .filter( + team_id=self.team.pk, + persondistinctid__distinct_id=person1_distinct_id, + ) + .filter(properties_to_Q(self.team.pk, filter.property_groups.flat)) + .exists() + ) + self.assertTrue(matched_person) + def test_person_relative_date_parsing_with_override_property(self): person1_distinct_id = "example_id" Person.objects.create( @@ -978,20 +1190,6 @@ def filter_persons_with_annotation(filter: Filter, team: Team): ) self.assertEqual(len(filter_persons_with_annotation(filter, self.team)), 1) - filter = Filter( - data={ - "properties": [ - { - "type": "person", - "key": "$key", - "value": ["2"], - "operator": "gt", - } - ] - } - ) - self.assertEqual(len(filter_persons_with_annotation(filter, self.team)), 0) - def filter_persons_with_property_group( filter: Filter, team: Team, property_overrides: Optional[dict[str, Any]] = None diff --git a/posthog/queries/base.py b/posthog/queries/base.py index e5cf6e717444b..3d7f52e7b98d8 100644 --- a/posthog/queries/base.py +++ b/posthog/queries/base.py @@ -240,9 +240,16 @@ def empty_or_null_with_value_q( else: parsed_value = None if operator in ("gt", "gte", "lt", "lte"): + if isinstance(value, list): + # If the value is a list for these operators, + # we should not return any results, as we can't compare a list to a single value + # TODO: should we try and parse each value in the list and return results based on that? + return Q(pk__isnull=True) + + # At this point, we know that the value is not a list, so we can safely parse it + # There might still be exceptions, but we're catching them below try: - # try to parse even if arrays can't be parsed, the catch will handle it - parsed_value = float(value) # type: ignore + parsed_value = float(value) except Exception: pass diff --git a/posthog/test/__snapshots__/test_feature_flag.ambr b/posthog/test/__snapshots__/test_feature_flag.ambr index a1fc1f4654fab..8699844b2b647 100644 --- a/posthog/test/__snapshots__/test_feature_flag.ambr +++ b/posthog/test/__snapshots__/test_feature_flag.ambr @@ -551,39 +551,6 @@ AND "posthog_person"."team_id" = 2) ''' # --- -# name: TestFeatureFlagMatcher.test_numeric_operator_with_cohorts_and_nested_cohorts.2 - ''' - SELECT (((("posthog_person"."properties" -> 'number') > '"100"' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'number')) = ('string')) - OR (("posthog_person"."properties" -> 'number') > '100.0' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'number')) = ('number'))) - AND "posthog_person"."properties" ? 'number' - AND NOT (("posthog_person"."properties" -> 'number') = 'null')) AS "flag_X_condition_0", - (((("posthog_person"."properties" -> 'version') > '"1.05"' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'version')) = ('string')) - OR (("posthog_person"."properties" -> 'version') > '1.05' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'version')) = ('number'))) - AND "posthog_person"."properties" ? 'version' - AND NOT (("posthog_person"."properties" -> 'version') = 'null')) AS "flag_X_condition_0", - (((("posthog_person"."properties" -> 'number') < '"31"' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'number')) = ('string')) - OR (("posthog_person"."properties" -> 'number') < '31.0' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'number')) = ('number'))) - AND "posthog_person"."properties" ? 'number' - AND NOT (("posthog_person"."properties" -> 'number') = 'null') - AND ((("posthog_person"."properties" -> 'nested_prop') > '"20"' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'nested_prop')) = ('string')) - OR (("posthog_person"."properties" -> 'nested_prop') > '20.0' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'nested_prop')) = ('number'))) - AND "posthog_person"."properties" ? 'nested_prop' - AND NOT (("posthog_person"."properties" -> 'nested_prop') = 'null')) AS "flag_X_condition_0" - 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) - ''' -# --- # name: TestFeatureFlagMatcher.test_numeric_operator_with_groups_and_person_flags ''' SELECT "posthog_grouptypemapping"."id", @@ -656,26 +623,6 @@ AND "posthog_person"."team_id" = 2) ''' # --- -# name: TestFeatureFlagMatcher.test_super_condition_matches_string.1 - ''' - SELECT ((("posthog_person"."properties" -> 'is_enabled') = 'true' - OR ("posthog_person"."properties" -> 'is_enabled') = '"true"') - AND "posthog_person"."properties" ? 'is_enabled' - AND NOT (("posthog_person"."properties" -> 'is_enabled') = 'null')) AS "flag_X_super_condition", ("posthog_person"."properties" -> 'is_enabled') IS NOT NULL AS "flag_X_super_condition_is_set", - (("posthog_person"."properties" -> 'email') = '"fake@posthog.com"' - AND "posthog_person"."properties" ? 'email' - AND NOT (("posthog_person"."properties" -> 'email') = 'null')) AS "flag_X_condition_0", - (("posthog_person"."properties" -> 'email') = '"test@posthog.com"' - AND "posthog_person"."properties" ? 'email' - AND NOT (("posthog_person"."properties" -> 'email') = 'null')) AS "flag_X_condition_1", - (true) AS "flag_X_condition_2" - FROM "posthog_person" - INNER JOIN "posthog_persondistinctid" ON ("posthog_person"."id" = "posthog_persondistinctid"."person_id") - WHERE ("posthog_persondistinctid"."distinct_id" = 'test_id' - AND "posthog_persondistinctid"."team_id" = 2 - AND "posthog_person"."team_id" = 2) - ''' -# --- # name: TestFeatureFlagMatcher.test_with_sql_injection_properties_and_other_aliases ''' SELECT "posthog_team"."id", @@ -822,43 +769,6 @@ AND "posthog_person"."team_id" = 2) ''' # --- -# name: TestFeatureFlagMatcher.test_with_sql_injection_properties_and_other_aliases.4 - ''' - SELECT (((("posthog_person"."properties" -> 'number space') > '"100"' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'number space')) = ('string')) - OR (("posthog_person"."properties" -> 'number space') > '100.0' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'number space')) = ('number'))) - AND "posthog_person"."properties" ? 'number space' - AND NOT (("posthog_person"."properties" -> 'number space') = 'null') - AND ((JSONB_TYPEOF(("posthog_person"."properties" -> ';''" SELECT 1; DROP TABLE posthog_featureflag;')) = ('string') - AND ("posthog_person"."properties" -> ';''" SELECT 1; DROP TABLE posthog_featureflag;') > '"100"') - OR (JSONB_TYPEOF(("posthog_person"."properties" -> ';''" SELECT 1; DROP TABLE posthog_featureflag;')) = ('number') - AND ("posthog_person"."properties" -> ';''" SELECT 1; DROP TABLE posthog_featureflag;') > '100.0')) - AND "posthog_person"."properties" ? ';''" SELECT 1; DROP TABLE posthog_featureflag;' - AND NOT (("posthog_person"."properties" -> ';''" SELECT 1; DROP TABLE posthog_featureflag;') = 'null')) AS "flag_X_condition_0", - (((JSONB_TYPEOF(("posthog_person"."properties" -> ';''" SELECT 1; DROP TABLE posthog_featureflag;')) = ('string') - AND ("posthog_person"."properties" -> ';''" SELECT 1; DROP TABLE posthog_featureflag;') > '"100"') - OR (JSONB_TYPEOF(("posthog_person"."properties" -> ';''" SELECT 1; DROP TABLE posthog_featureflag;')) = ('number') - AND ("posthog_person"."properties" -> ';''" SELECT 1; DROP TABLE posthog_featureflag;') > '100.0')) - AND "posthog_person"."properties" ? ';''" SELECT 1; DROP TABLE posthog_featureflag;' - AND NOT (("posthog_person"."properties" -> ';''" SELECT 1; DROP TABLE posthog_featureflag;') = 'null')) AS "flag_X_condition_1", - (((("posthog_person"."properties" -> 'version!!!') > '"1.05"' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'version!!!')) = ('string')) - OR (("posthog_person"."properties" -> 'version!!!') > '1.05' - AND JSONB_TYPEOF(("posthog_person"."properties" -> 'version!!!')) = ('number'))) - AND "posthog_person"."properties" ? 'version!!!' - AND NOT (("posthog_person"."properties" -> 'version!!!') = 'null')) AS "flag_X_condition_2", - ((("posthog_person"."properties" -> 'nested_prop --random #comment //test') = '"21"' - OR ("posthog_person"."properties" -> 'nested_prop --random #comment //test') = '21') - AND "posthog_person"."properties" ? 'nested_prop --random #comment //test' - AND NOT (("posthog_person"."properties" -> 'nested_prop --random #comment //test') = 'null')) AS "flag_X_condition_3" - 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) - ''' -# --- # name: TestHashKeyOverridesRaceConditions.test_hash_key_overrides_with_simulated_error_race_conditions_on_person_merging 'BEGIN' # --- diff --git a/posthog/test/test_feature_flag.py b/posthog/test/test_feature_flag.py index 098726bbd270d..5c6388f3dec8e 100644 --- a/posthog/test/test_feature_flag.py +++ b/posthog/test/test_feature_flag.py @@ -798,6 +798,63 @@ def test_invalid_regex_match_flag(self): FeatureFlagMatch(False, None, FeatureFlagMatchReason.NO_CONDITION_MATCH, 0), ) + def test_feature_flag_with_greater_than_filter(self): + Person.objects.create( + team=self.team, + distinct_ids=["example_id"], + properties={"$some_prop": 5}, + ) + feature_flag = self.create_feature_flag( + key="flag-with-gt-filter", + filters={ + "groups": [{"properties": [{"key": "$some_prop", "value": 4, "type": "person", "operator": "gt"}]}] + }, + ) + + with self.assertNumQueries(4): + self.assertEqual( + self.match_flag(feature_flag, "example_id"), + FeatureFlagMatch(True, None, FeatureFlagMatchReason.CONDITION_MATCH, 0), + ) + + def test_feature_flag_with_greater_than_filter_no_match(self): + Person.objects.create( + team=self.team, + distinct_ids=["example_id"], + properties={"$some_prop": 3}, + ) + feature_flag = self.create_feature_flag( + key="flag-with-gt-filter", + filters={ + "groups": [{"properties": [{"key": "$some_prop", "value": 4, "type": "person", "operator": "gt"}]}] + }, + ) + + with self.assertNumQueries(4): + self.assertEqual( + self.match_flag(feature_flag, "example_id"), + FeatureFlagMatch(False, None, FeatureFlagMatchReason.NO_CONDITION_MATCH, 0), + ) + + def test_feature_flag_with_greater_than_filter_invalid_value(self): + Person.objects.create( + team=self.team, + distinct_ids=["example_id"], + properties={"$some_prop": 3}, + ) + feature_flag = self.create_feature_flag( + key="flag-with-gt-filter", + filters={ + "groups": [{"properties": [{"key": "$some_prop", "value": ["4"], "type": "person", "operator": "gt"}]}] + }, + ) + + with self.assertNumQueries(3): + self.assertEqual( + self.match_flag(feature_flag, "example_id"), + FeatureFlagMatch(False, None, FeatureFlagMatchReason.NO_CONDITION_MATCH, 0), + ) + def test_coercion_of_strings_and_numbers(self): Person.objects.create( team=self.team,