From 759f364841adb43c159f52f0115aa7d3a8157871 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Wed, 22 Nov 2023 11:47:15 +0000 Subject: [PATCH 1/3] fix(flags): Dont locally match cohorts when id property is provided --- 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 From e3d965b98da62a63e54286c94c484a65a0ad8e30 Mon Sep 17 00:00:00 2001 From: Neil Kakkar Date: Wed, 22 Nov 2023 13:32:00 +0000 Subject: [PATCH 2/3] fix(cohorts): Add default property tests --- posthog/api/cohort.py | 42 +- .../test/__snapshots__/test_feature_flag.ambr | 637 ++++++++++++++++++ posthog/api/test/test_feature_flag.py | 204 +++++- 3 files changed, 878 insertions(+), 5 deletions(-) diff --git a/posthog/api/cohort.py b/posthog/api/cohort.py index d417246b76d95..90324a48bfda0 100644 --- a/posthog/api/cohort.py +++ b/posthog/api/cohort.py @@ -10,6 +10,7 @@ get_feature_flag_hash_key_overrides, ) from posthog.models.person.person import PersonDistinctId +from posthog.models.property.property import Property from posthog.queries.insight import insight_sync_execute import posthoganalytics from posthog.metrics import LABEL_TEAM_ID @@ -556,7 +557,7 @@ def insert_actors_into_cohort_by_query(cohort: Cohort, query: str, params: Dict[ capture_exception(err) -def get_cohort_actors_for_feature_flag(cohort_id: int, flag: str, team_id: int, batchsize: int = 5_000): +def get_cohort_actors_for_feature_flag(cohort_id: int, flag: str, team_id: int, batchsize: int = 1_000): # :TODO: Find a way to incorporate this into the same code path as feature flag evaluation try: feature_flag = FeatureFlag.objects.get(team_id=team_id, key=flag) @@ -572,14 +573,16 @@ def get_cohort_actors_for_feature_flag(cohort_id: int, flag: str, team_id: int, cohorts_cache = {} if feature_flag.uses_cohorts: + # TODO: Consider disabling flags with cohorts for creating static cohorts + # because this is currently a lot more inefficient for flag matching, + # as we're required to go to the database for each person. cohorts_cache = {cohort.pk: cohort for cohort in Cohort.objects.filter(team_id=team_id, deleted=False)} default_person_properties = {} for condition in feature_flag.conditions: property_list = Filter(data=condition).property_groups.flat for property in property_list: - if property.operator not in ("is_set", "is_not_set") and property.type == "person": - default_person_properties[property.key] = "" + default_person_properties.update(get_default_person_property(property, cohorts_cache)) try: # QuerySet.Iterator() doesn't work with pgbouncer, it will load everything into memory and then stream @@ -596,7 +599,7 @@ def get_cohort_actors_for_feature_flag(cohort_id: int, flag: str, team_id: int, # "distinct_id", flat=True # )[0] distinct_id_subquery = Subquery( - PersonDistinctId.objects.filter(person_id=OuterRef("person_id")).values_list("id", flat=True)[:1] + PersonDistinctId.objects.filter(person_id=OuterRef("person_id")).values_list("id", flat=True)[:3] ) prefetch_related_objects( batch_of_persons, @@ -612,6 +615,10 @@ def get_cohort_actors_for_feature_flag(cohort_id: int, flag: str, team_id: int, break for person in all_persons: + # ignore almost-deleted persons / persons with no distinct ids + if len(person.distinct_ids) == 0: + continue + distinct_id = person.distinct_ids[0] person_overrides = {} if feature_flag.ensure_experience_continuity: @@ -660,3 +667,30 @@ def get_cohort_actors_for_feature_flag(cohort_id: int, flag: str, team_id: int, if settings.DEBUG or settings.TEST: raise err capture_exception(err) + + +def get_default_person_property(prop: Property, cohorts_cache: Dict[int, Cohort]): + default_person_properties = {} + + if prop.operator not in ("is_set", "is_not_set") and prop.type == "person": + default_person_properties[prop.key] = "" + elif prop.type == "cohort" and not isinstance(prop.value, list): + try: + parsed_cohort_id = int(prop.value) + except (ValueError, TypeError): + return None + cohort = cohorts_cache.get(parsed_cohort_id) + if cohort: + return get_default_person_properties_for_cohort(cohort, cohorts_cache) + return default_person_properties + + +def get_default_person_properties_for_cohort(cohort: Cohort, cohorts_cache: Dict[int, Cohort]) -> Dict[str, str]: + """ + Returns a dictionary of default person properties to use when evaluating a feature flag + """ + default_person_properties = {} + for property in cohort.properties.flat: + default_person_properties.update(get_default_person_property(property, cohorts_cache)) + + return default_person_properties diff --git a/posthog/api/test/__snapshots__/test_feature_flag.ambr b/posthog/api/test/__snapshots__/test_feature_flag.ambr index 2e6fddaa836e2..dd4735aebb1da 100644 --- a/posthog/api/test/__snapshots__/test_feature_flag.ambr +++ b/posthog/api/test/__snapshots__/test_feature_flag.ambr @@ -754,6 +754,643 @@ LIMIT 1))) ' --- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too + ' + SELECT "posthog_featureflag"."id", + "posthog_featureflag"."key", + "posthog_featureflag"."name", + "posthog_featureflag"."filters", + "posthog_featureflag"."rollout_percentage", + "posthog_featureflag"."team_id", + "posthog_featureflag"."created_by_id", + "posthog_featureflag"."created_at", + "posthog_featureflag"."deleted", + "posthog_featureflag"."active", + "posthog_featureflag"."rollback_conditions", + "posthog_featureflag"."performed_rollback", + "posthog_featureflag"."ensure_experience_continuity", + "posthog_featureflag"."usage_dashboard_id", + "posthog_featureflag"."has_enriched_analytics" + FROM "posthog_featureflag" + WHERE ("posthog_featureflag"."key" = 'some-feature-new' + AND "posthog_featureflag"."team_id" = 2) + LIMIT 21 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too.1 + ' + SELECT "posthog_cohort"."id", + "posthog_cohort"."name", + "posthog_cohort"."description", + "posthog_cohort"."team_id", + "posthog_cohort"."deleted", + "posthog_cohort"."filters", + "posthog_cohort"."version", + "posthog_cohort"."pending_version", + "posthog_cohort"."count", + "posthog_cohort"."created_by_id", + "posthog_cohort"."created_at", + "posthog_cohort"."is_calculating", + "posthog_cohort"."last_calculation", + "posthog_cohort"."errors_calculating", + "posthog_cohort"."is_static", + "posthog_cohort"."groups" + FROM "posthog_cohort" + WHERE "posthog_cohort"."id" = 2 + LIMIT 21 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too.10 + ' + SELECT "posthog_person"."uuid" + FROM "posthog_person" + WHERE ("posthog_person"."team_id" = 2 + AND "posthog_person"."uuid" IN ('00000000-0000-0000-0000-000000000000'::uuid, + '00000000-0000-0000-0000-000000000001'::uuid /* ... */) + AND NOT (EXISTS + (SELECT (1) AS "a" + FROM "posthog_cohortpeople" U1 + WHERE (U1."cohort_id" = 2 + AND U1."person_id" = "posthog_person"."id") + LIMIT 1))) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too.11 + ' + SELECT "posthog_team"."id", + "posthog_team"."uuid", + "posthog_team"."organization_id", + "posthog_team"."api_token", + "posthog_team"."app_urls", + "posthog_team"."name", + "posthog_team"."slack_incoming_webhook", + "posthog_team"."created_at", + "posthog_team"."updated_at", + "posthog_team"."anonymize_ips", + "posthog_team"."completed_snippet_onboarding", + "posthog_team"."has_completed_onboarding_for", + "posthog_team"."ingested_event", + "posthog_team"."autocapture_opt_out", + "posthog_team"."autocapture_exceptions_opt_in", + "posthog_team"."autocapture_exceptions_errors_to_ignore", + "posthog_team"."session_recording_opt_in", + "posthog_team"."session_recording_sample_rate", + "posthog_team"."session_recording_minimum_duration_milliseconds", + "posthog_team"."session_recording_linked_flag", + "posthog_team"."session_recording_network_payload_capture_config", + "posthog_team"."capture_console_log_opt_in", + "posthog_team"."capture_performance_opt_in", + "posthog_team"."surveys_opt_in", + "posthog_team"."session_recording_version", + "posthog_team"."signup_token", + "posthog_team"."is_demo", + "posthog_team"."access_control", + "posthog_team"."week_start_day", + "posthog_team"."inject_web_apps", + "posthog_team"."test_account_filters", + "posthog_team"."test_account_filters_default_checked", + "posthog_team"."path_cleaning_filters", + "posthog_team"."timezone", + "posthog_team"."data_attributes", + "posthog_team"."person_display_name_properties", + "posthog_team"."live_events_columns", + "posthog_team"."recording_domains", + "posthog_team"."primary_dashboard_id", + "posthog_team"."extra_settings", + "posthog_team"."correlation_config", + "posthog_team"."session_recording_retention_period_days", + "posthog_team"."plugins_opt_in", + "posthog_team"."opt_out_capture", + "posthog_team"."event_names", + "posthog_team"."event_names_with_usage", + "posthog_team"."event_properties", + "posthog_team"."event_properties_with_usage", + "posthog_team"."event_properties_numerical", + "posthog_team"."external_data_workspace_id", + "posthog_team"."external_data_workspace_last_synced_at" + FROM "posthog_team" + WHERE "posthog_team"."id" = 2 + LIMIT 21 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too.2 + ' + SELECT "posthog_cohort"."id", + "posthog_cohort"."name", + "posthog_cohort"."description", + "posthog_cohort"."team_id", + "posthog_cohort"."deleted", + "posthog_cohort"."filters", + "posthog_cohort"."version", + "posthog_cohort"."pending_version", + "posthog_cohort"."count", + "posthog_cohort"."created_by_id", + "posthog_cohort"."created_at", + "posthog_cohort"."is_calculating", + "posthog_cohort"."last_calculation", + "posthog_cohort"."errors_calculating", + "posthog_cohort"."is_static", + "posthog_cohort"."groups" + FROM "posthog_cohort" + WHERE (NOT "posthog_cohort"."deleted" + AND "posthog_cohort"."team_id" = 2) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too.3 + ' + SELECT "posthog_person"."id", + "posthog_person"."created_at", + "posthog_person"."properties_last_updated_at", + "posthog_person"."properties_last_operation", + "posthog_person"."team_id", + "posthog_person"."properties", + "posthog_person"."is_user_id", + "posthog_person"."is_identified", + "posthog_person"."uuid", + "posthog_person"."version" + FROM "posthog_person" + WHERE "posthog_person"."team_id" = 2 + ORDER BY "posthog_person"."id" ASC + LIMIT 1000 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too.4 + ' + SELECT "posthog_persondistinctid"."id", + "posthog_persondistinctid"."team_id", + "posthog_persondistinctid"."person_id", + "posthog_persondistinctid"."distinct_id", + "posthog_persondistinctid"."version" + FROM "posthog_persondistinctid" + WHERE ("posthog_persondistinctid"."id" IN + (SELECT U0."id" + FROM "posthog_persondistinctid" U0 + WHERE U0."person_id" = "posthog_persondistinctid"."person_id" + LIMIT 3) + AND "posthog_persondistinctid"."person_id" IN (1, + 2, + 3, + 4, + 5 /* ... */)) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too.5 + ' + SELECT ("posthog_person"."id" IS NULL + OR "posthog_person"."id" IS NULL + OR EXISTS + (SELECT (1) AS "a" + FROM "posthog_cohortpeople" U0 + WHERE (U0."cohort_id" = 2 + AND U0."cohort_id" = 2 + AND U0."person_id" = "posthog_person"."id") + LIMIT 1) + OR "posthog_person"."id" IS 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" = 'person1' + AND "posthog_persondistinctid"."team_id" = 2 + AND "posthog_person"."team_id" = 2) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too.6 + ' + SELECT ("posthog_person"."id" IS NOT NULL + OR "posthog_person"."id" IS NULL + OR EXISTS + (SELECT (1) AS "a" + FROM "posthog_cohortpeople" U0 + WHERE (U0."cohort_id" = 2 + AND U0."cohort_id" = 2 + AND U0."person_id" = "posthog_person"."id") + LIMIT 1) + OR "posthog_person"."id" IS 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" = 'person2' + AND "posthog_persondistinctid"."team_id" = 2 + AND "posthog_person"."team_id" = 2) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too.7 + ' + SELECT ("posthog_person"."id" IS NULL + OR "posthog_person"."id" IS NOT NULL + OR EXISTS + (SELECT (1) AS "a" + FROM "posthog_cohortpeople" U0 + WHERE (U0."cohort_id" = 2 + AND U0."cohort_id" = 2 + AND U0."person_id" = "posthog_person"."id") + LIMIT 1) + OR "posthog_person"."id" IS 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" = 'person3' + AND "posthog_persondistinctid"."team_id" = 2 + AND "posthog_person"."team_id" = 2) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too.8 + ' + SELECT ("posthog_person"."id" IS NULL + OR "posthog_person"."id" IS NULL + OR EXISTS + (SELECT (1) AS "a" + FROM "posthog_cohortpeople" U0 + WHERE (U0."cohort_id" = 2 + AND U0."cohort_id" = 2 + AND U0."person_id" = "posthog_person"."id") + LIMIT 1) + OR "posthog_person"."id" IS 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" = 'person4' + AND "posthog_persondistinctid"."team_id" = 2 + AND "posthog_person"."team_id" = 2) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too.9 + ' + SELECT "posthog_person"."id", + "posthog_person"."created_at", + "posthog_person"."properties_last_updated_at", + "posthog_person"."properties_last_operation", + "posthog_person"."team_id", + "posthog_person"."properties", + "posthog_person"."is_user_id", + "posthog_person"."is_identified", + "posthog_person"."uuid", + "posthog_person"."version" + FROM "posthog_person" + WHERE "posthog_person"."team_id" = 2 + ORDER BY "posthog_person"."id" ASC + LIMIT 1000 + OFFSET 1000 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment + ' + SELECT "posthog_featureflag"."id", + "posthog_featureflag"."key", + "posthog_featureflag"."name", + "posthog_featureflag"."filters", + "posthog_featureflag"."rollout_percentage", + "posthog_featureflag"."team_id", + "posthog_featureflag"."created_by_id", + "posthog_featureflag"."created_at", + "posthog_featureflag"."deleted", + "posthog_featureflag"."active", + "posthog_featureflag"."rollback_conditions", + "posthog_featureflag"."performed_rollback", + "posthog_featureflag"."ensure_experience_continuity", + "posthog_featureflag"."usage_dashboard_id", + "posthog_featureflag"."has_enriched_analytics" + FROM "posthog_featureflag" + WHERE ("posthog_featureflag"."key" = 'some-feature2' + AND "posthog_featureflag"."team_id" = 2) + LIMIT 21 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.1 + ' + SELECT "posthog_cohort"."id", + "posthog_cohort"."name", + "posthog_cohort"."description", + "posthog_cohort"."team_id", + "posthog_cohort"."deleted", + "posthog_cohort"."filters", + "posthog_cohort"."version", + "posthog_cohort"."pending_version", + "posthog_cohort"."count", + "posthog_cohort"."created_by_id", + "posthog_cohort"."created_at", + "posthog_cohort"."is_calculating", + "posthog_cohort"."last_calculation", + "posthog_cohort"."errors_calculating", + "posthog_cohort"."is_static", + "posthog_cohort"."groups" + FROM "posthog_cohort" + WHERE "posthog_cohort"."id" = 2 + LIMIT 21 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.10 + ' + SELECT "posthog_persondistinctid"."id", + "posthog_persondistinctid"."team_id", + "posthog_persondistinctid"."person_id", + "posthog_persondistinctid"."distinct_id", + "posthog_persondistinctid"."version" + FROM "posthog_persondistinctid" + WHERE ("posthog_persondistinctid"."id" IN + (SELECT U0."id" + FROM "posthog_persondistinctid" U0 + WHERE U0."person_id" = "posthog_persondistinctid"."person_id" + LIMIT 3) + AND "posthog_persondistinctid"."person_id" IN (1, + 2, + 3, + 4, + 5 /* ... */)) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.11 + ' + SELECT ("posthog_person"."properties" -> 'key') IS NOT 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" = 'person3' + AND "posthog_persondistinctid"."team_id" = 2 + AND "posthog_person"."team_id" = 2) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.12 + ' + SELECT "posthog_person"."id", + "posthog_person"."created_at", + "posthog_person"."properties_last_updated_at", + "posthog_person"."properties_last_operation", + "posthog_person"."team_id", + "posthog_person"."properties", + "posthog_person"."is_user_id", + "posthog_person"."is_identified", + "posthog_person"."uuid", + "posthog_person"."version" + FROM "posthog_person" + WHERE "posthog_person"."team_id" = 2 + ORDER BY "posthog_person"."id" ASC + LIMIT 5000 + OFFSET 5000 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.13 + ' + SELECT "posthog_person"."uuid" + FROM "posthog_person" + WHERE ("posthog_person"."team_id" = 2 + AND "posthog_person"."uuid" IN ('00000000-0000-0000-0000-000000000000'::uuid, + '00000000-0000-0000-0000-000000000001'::uuid /* ... */) + AND NOT (EXISTS + (SELECT (1) AS "a" + FROM "posthog_cohortpeople" U1 + WHERE (U1."cohort_id" = 2 + AND U1."person_id" = "posthog_person"."id") + LIMIT 1))) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.14 + ' + SELECT "posthog_team"."id", + "posthog_team"."uuid", + "posthog_team"."organization_id", + "posthog_team"."api_token", + "posthog_team"."app_urls", + "posthog_team"."name", + "posthog_team"."slack_incoming_webhook", + "posthog_team"."created_at", + "posthog_team"."updated_at", + "posthog_team"."anonymize_ips", + "posthog_team"."completed_snippet_onboarding", + "posthog_team"."has_completed_onboarding_for", + "posthog_team"."ingested_event", + "posthog_team"."autocapture_opt_out", + "posthog_team"."autocapture_exceptions_opt_in", + "posthog_team"."autocapture_exceptions_errors_to_ignore", + "posthog_team"."session_recording_opt_in", + "posthog_team"."session_recording_sample_rate", + "posthog_team"."session_recording_minimum_duration_milliseconds", + "posthog_team"."session_recording_linked_flag", + "posthog_team"."session_recording_network_payload_capture_config", + "posthog_team"."capture_console_log_opt_in", + "posthog_team"."capture_performance_opt_in", + "posthog_team"."surveys_opt_in", + "posthog_team"."session_recording_version", + "posthog_team"."signup_token", + "posthog_team"."is_demo", + "posthog_team"."access_control", + "posthog_team"."week_start_day", + "posthog_team"."inject_web_apps", + "posthog_team"."test_account_filters", + "posthog_team"."test_account_filters_default_checked", + "posthog_team"."path_cleaning_filters", + "posthog_team"."timezone", + "posthog_team"."data_attributes", + "posthog_team"."person_display_name_properties", + "posthog_team"."live_events_columns", + "posthog_team"."recording_domains", + "posthog_team"."primary_dashboard_id", + "posthog_team"."extra_settings", + "posthog_team"."correlation_config", + "posthog_team"."session_recording_retention_period_days", + "posthog_team"."plugins_opt_in", + "posthog_team"."opt_out_capture", + "posthog_team"."event_names", + "posthog_team"."event_names_with_usage", + "posthog_team"."event_properties", + "posthog_team"."event_properties_with_usage", + "posthog_team"."event_properties_numerical", + "posthog_team"."external_data_workspace_id", + "posthog_team"."external_data_workspace_last_synced_at" + FROM "posthog_team" + WHERE "posthog_team"."id" = 2 + LIMIT 21 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.2 + ' + SELECT "posthog_person"."id", + "posthog_person"."created_at", + "posthog_person"."properties_last_updated_at", + "posthog_person"."properties_last_operation", + "posthog_person"."team_id", + "posthog_person"."properties", + "posthog_person"."is_user_id", + "posthog_person"."is_identified", + "posthog_person"."uuid", + "posthog_person"."version" + FROM "posthog_person" + WHERE "posthog_person"."team_id" = 2 + ORDER BY "posthog_person"."id" ASC + LIMIT 5000 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.3 + ' + SELECT "posthog_persondistinctid"."id", + "posthog_persondistinctid"."team_id", + "posthog_persondistinctid"."person_id", + "posthog_persondistinctid"."distinct_id", + "posthog_persondistinctid"."version" + FROM "posthog_persondistinctid" + WHERE ("posthog_persondistinctid"."id" IN + (SELECT U0."id" + FROM "posthog_persondistinctid" U0 + WHERE U0."person_id" = "posthog_persondistinctid"."person_id" + LIMIT 3) + AND "posthog_persondistinctid"."person_id" IN (1, + 2, + 3, + 4, + 5 /* ... */)) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.4 + ' + SELECT "posthog_person"."id", + "posthog_person"."created_at", + "posthog_person"."properties_last_updated_at", + "posthog_person"."properties_last_operation", + "posthog_person"."team_id", + "posthog_person"."properties", + "posthog_person"."is_user_id", + "posthog_person"."is_identified", + "posthog_person"."uuid", + "posthog_person"."version" + FROM "posthog_person" + WHERE "posthog_person"."team_id" = 2 + ORDER BY "posthog_person"."id" ASC + LIMIT 5000 + OFFSET 5000 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.5 + ' + SELECT "posthog_person"."uuid" + FROM "posthog_person" + WHERE ("posthog_person"."team_id" = 2 + AND "posthog_person"."uuid" IN ('00000000-0000-0000-0000-000000000000'::uuid, + '00000000-0000-0000-0000-000000000001'::uuid /* ... */) + AND NOT (EXISTS + (SELECT (1) AS "a" + FROM "posthog_cohortpeople" U1 + WHERE (U1."cohort_id" = 2 + AND U1."person_id" = "posthog_person"."id") + LIMIT 1))) + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.6 + ' + SELECT "posthog_team"."id", + "posthog_team"."uuid", + "posthog_team"."organization_id", + "posthog_team"."api_token", + "posthog_team"."app_urls", + "posthog_team"."name", + "posthog_team"."slack_incoming_webhook", + "posthog_team"."created_at", + "posthog_team"."updated_at", + "posthog_team"."anonymize_ips", + "posthog_team"."completed_snippet_onboarding", + "posthog_team"."has_completed_onboarding_for", + "posthog_team"."ingested_event", + "posthog_team"."autocapture_opt_out", + "posthog_team"."autocapture_exceptions_opt_in", + "posthog_team"."autocapture_exceptions_errors_to_ignore", + "posthog_team"."session_recording_opt_in", + "posthog_team"."session_recording_sample_rate", + "posthog_team"."session_recording_minimum_duration_milliseconds", + "posthog_team"."session_recording_linked_flag", + "posthog_team"."session_recording_network_payload_capture_config", + "posthog_team"."capture_console_log_opt_in", + "posthog_team"."capture_performance_opt_in", + "posthog_team"."surveys_opt_in", + "posthog_team"."session_recording_version", + "posthog_team"."signup_token", + "posthog_team"."is_demo", + "posthog_team"."access_control", + "posthog_team"."week_start_day", + "posthog_team"."inject_web_apps", + "posthog_team"."test_account_filters", + "posthog_team"."test_account_filters_default_checked", + "posthog_team"."path_cleaning_filters", + "posthog_team"."timezone", + "posthog_team"."data_attributes", + "posthog_team"."person_display_name_properties", + "posthog_team"."live_events_columns", + "posthog_team"."recording_domains", + "posthog_team"."primary_dashboard_id", + "posthog_team"."extra_settings", + "posthog_team"."correlation_config", + "posthog_team"."session_recording_retention_period_days", + "posthog_team"."plugins_opt_in", + "posthog_team"."opt_out_capture", + "posthog_team"."event_names", + "posthog_team"."event_names_with_usage", + "posthog_team"."event_properties", + "posthog_team"."event_properties_with_usage", + "posthog_team"."event_properties_numerical", + "posthog_team"."external_data_workspace_id", + "posthog_team"."external_data_workspace_last_synced_at" + FROM "posthog_team" + WHERE "posthog_team"."id" = 2 + LIMIT 21 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.7 + ' + SELECT "posthog_featureflag"."id", + "posthog_featureflag"."key", + "posthog_featureflag"."name", + "posthog_featureflag"."filters", + "posthog_featureflag"."rollout_percentage", + "posthog_featureflag"."team_id", + "posthog_featureflag"."created_by_id", + "posthog_featureflag"."created_at", + "posthog_featureflag"."deleted", + "posthog_featureflag"."active", + "posthog_featureflag"."rollback_conditions", + "posthog_featureflag"."performed_rollback", + "posthog_featureflag"."ensure_experience_continuity", + "posthog_featureflag"."usage_dashboard_id", + "posthog_featureflag"."has_enriched_analytics" + FROM "posthog_featureflag" + WHERE ("posthog_featureflag"."key" = 'some-feature-new' + AND "posthog_featureflag"."team_id" = 2) + LIMIT 21 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.8 + ' + SELECT "posthog_cohort"."id", + "posthog_cohort"."name", + "posthog_cohort"."description", + "posthog_cohort"."team_id", + "posthog_cohort"."deleted", + "posthog_cohort"."filters", + "posthog_cohort"."version", + "posthog_cohort"."pending_version", + "posthog_cohort"."count", + "posthog_cohort"."created_by_id", + "posthog_cohort"."created_at", + "posthog_cohort"."is_calculating", + "posthog_cohort"."last_calculation", + "posthog_cohort"."errors_calculating", + "posthog_cohort"."is_static", + "posthog_cohort"."groups" + FROM "posthog_cohort" + WHERE "posthog_cohort"."id" = 2 + LIMIT 21 + ' +--- +# name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.9 + ' + SELECT "posthog_person"."id", + "posthog_person"."created_at", + "posthog_person"."properties_last_updated_at", + "posthog_person"."properties_last_operation", + "posthog_person"."team_id", + "posthog_person"."properties", + "posthog_person"."is_user_id", + "posthog_person"."is_identified", + "posthog_person"."uuid", + "posthog_person"."version" + FROM "posthog_person" + WHERE "posthog_person"."team_id" = 2 + ORDER BY "posthog_person"."id" ASC + LIMIT 5000 + ' +--- # name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_deleted_flag ' SELECT "posthog_featureflag"."id", diff --git a/posthog/api/test/test_feature_flag.py b/posthog/api/test/test_feature_flag.py index 06997913184f8..31f46aabff9a0 100644 --- a/posthog/api/test/test_feature_flag.py +++ b/posthog/api/test/test_feature_flag.py @@ -3738,6 +3738,38 @@ def test_creating_static_cohort_with_group_flag(self): response = self.client.get(f"/api/cohort/{cohort.pk}/persons") self.assertEqual(len(response.json()["results"]), 0, response) + def test_creating_static_cohort_with_no_person_distinct_ids(self): + FeatureFlag.objects.create( + team=self.team, + rollout_percentage=100, + filters={ + "groups": [{"properties": [], "rollout_percentage": 100}], + "multivariate": None, + }, + name="some feature", + key="some-feature2", + created_by=self.user, + ) + + Person.objects.create(team=self.team) + + cohort = Cohort.objects.create( + team=self.team, + is_static=True, + name="some cohort", + ) + + with self.assertNumQueries(5): + get_cohort_actors_for_feature_flag(cohort.pk, "some-feature2", self.team.pk) + + cohort.refresh_from_db() + self.assertEqual(cohort.name, "some cohort") + # don't even try inserting anything, because invalid flag, so None instead of 0 + self.assertEqual(cohort.count, None) + + response = self.client.get(f"/api/cohort/{cohort.pk}/persons") + self.assertEqual(len(response.json()["results"]), 0, response) + def test_creating_static_cohort_with_non_existing_flag(self): cohort = Cohort.objects.create( team=self.team, @@ -3783,7 +3815,6 @@ def test_creating_static_cohort_with_experience_continuity_flag(self): properties={"key": "value"}, ) flush_persons_and_events() - # TODO: Check right person is added here FeatureFlagHashKeyOverride.objects.create( feature_flag_key="some-feature2", @@ -3873,6 +3904,177 @@ def test_creating_static_cohort_iterator(self): response = self.client.get(f"/api/cohort/{cohort.pk}/persons") self.assertEqual(len(response.json()["results"]), 3, response) + def test_creating_static_cohort_with_default_person_properties_adjustment(self): + FeatureFlag.objects.create( + team=self.team, + filters={ + "groups": [ + { + "properties": [{"key": "key", "value": "value", "type": "person", "operator": "icontains"}], + "rollout_percentage": 100, + } + ], + "multivariate": None, + }, + name="some feature", + key="some-feature2", + created_by=self.user, + ensure_experience_continuity=False, + ) + FeatureFlag.objects.create( + team=self.team, + filters={ + "groups": [ + { + "properties": [{"key": "key", "value": "value", "type": "person", "operator": "is_set"}], + "rollout_percentage": 100, + } + ], + "multivariate": None, + }, + name="some feature", + key="some-feature-new", + created_by=self.user, + ensure_experience_continuity=False, + ) + + _create_person(team=self.team, distinct_ids=[f"person1"], properties={"key": "value"}) + _create_person( + team=self.team, + distinct_ids=[f"person2"], + properties={"key": "vaalue"}, + ) + _create_person( + team=self.team, + distinct_ids=[f"person3"], + properties={"key22": "value"}, + ) + flush_persons_and_events() + + cohort = Cohort.objects.create( + team=self.team, + is_static=True, + name="some cohort", + ) + + with snapshot_postgres_queries_context(self), self.assertNumQueries(9): + # no queries to evaluate flags, because all evaluated using override properties + get_cohort_actors_for_feature_flag(cohort.pk, "some-feature2", self.team.pk) + + cohort.refresh_from_db() + self.assertEqual(cohort.name, "some cohort") + self.assertEqual(cohort.count, 1) + + response = self.client.get(f"/api/cohort/{cohort.pk}/persons") + self.assertEqual(len(response.json()["results"]), 1, response) + + cohort2 = Cohort.objects.create( + team=self.team, + is_static=True, + name="some cohort2", + ) + + with snapshot_postgres_queries_context(self), self.assertNumQueries(13): + # need to evaluate flags for person3 using db, because is_set operator can't have defaults added. + get_cohort_actors_for_feature_flag(cohort2.pk, "some-feature-new", self.team.pk) + + cohort2.refresh_from_db() + self.assertEqual(cohort2.name, "some cohort2") + self.assertEqual(cohort2.count, 2) + + def test_creating_static_cohort_with_cohort_flag_adds_cohort_props_as_default_too(self): + cohort_nested = Cohort.objects.create( + team=self.team, + filters={ + "properties": { + "type": "OR", + "values": [ + { + "type": "OR", + "values": [ + {"key": "does-not-exist", "value": "none", "type": "person"}, + ], + } + ], + } + }, + ) + cohort_static = Cohort.objects.create( + team=self.team, + is_static=True, + ) + cohort_existing = Cohort.objects.create( + team=self.team, + filters={ + "properties": { + "type": "OR", + "values": [ + { + "type": "OR", + "values": [ + {"key": "group", "value": "none", "type": "person"}, + {"key": "group2", "value": [1, 2, 3], "type": "person"}, + {"key": "id", "value": cohort_static.pk, "type": "cohort"}, + {"key": "id", "value": cohort_nested.pk, "type": "cohort"}, + ], + } + ], + } + }, + name="cohort1", + ) + FeatureFlag.objects.create( + team=self.team, + filters={ + "groups": [ + { + "properties": [{"key": "id", "value": cohort_existing.pk, "type": "cohort"}], + "rollout_percentage": 100, + }, + {"properties": [{"key": "key", "value": "value", "type": "person"}], "rollout_percentage": 100}, + ], + "multivariate": None, + }, + name="some feature", + key="some-feature-new", + created_by=self.user, + ensure_experience_continuity=False, + ) + + _create_person(team=self.team, distinct_ids=[f"person1"], properties={"key": "value"}) + _create_person( + team=self.team, + distinct_ids=[f"person2"], + properties={"group": "none"}, + ) + _create_person( + team=self.team, + distinct_ids=[f"person3"], + properties={"key22": "value", "group2": 2}, + ) + _create_person( + team=self.team, + distinct_ids=[f"person4"], + properties={}, + ) + flush_persons_and_events() + + cohort_static.insert_users_by_list([f"person4"]) + + cohort = Cohort.objects.create( + team=self.team, + is_static=True, + name="some cohort", + ) + + with snapshot_postgres_queries_context(self), self.assertNumQueries(26): + # forced to evaluate flags by going to db, because cohorts need db query to evaluate + get_cohort_actors_for_feature_flag(cohort.pk, "some-feature-new", self.team.pk) + + cohort.refresh_from_db() + self.assertEqual(cohort.name, "some cohort") + self.assertEqual(cohort.count, 4) + class TestBlastRadius(ClickhouseTestMixin, APIBaseTest): @snapshot_clickhouse_queries From 0d6f56e6ff177f332d066ebbedce1d28475450ca Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 22 Nov 2023 13:40:01 +0000 Subject: [PATCH 3/3] Update query snapshots --- .../test/__snapshots__/test_feature_flag.ambr | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/posthog/api/test/__snapshots__/test_feature_flag.ambr b/posthog/api/test/__snapshots__/test_feature_flag.ambr index de0a1d5aa8c34..ffe583b425eac 100644 --- a/posthog/api/test/__snapshots__/test_feature_flag.ambr +++ b/posthog/api/test/__snapshots__/test_feature_flag.ambr @@ -604,7 +604,7 @@ (SELECT U0."id" FROM "posthog_persondistinctid" U0 WHERE U0."person_id" = "posthog_persondistinctid"."person_id" - LIMIT 1) + LIMIT 3) AND "posthog_persondistinctid"."person_id" IN (1, 2, 3, @@ -731,7 +731,7 @@ (SELECT U0."id" FROM "posthog_persondistinctid" U0 WHERE U0."person_id" = "posthog_persondistinctid"."person_id" - LIMIT 1) + LIMIT 3) AND "posthog_persondistinctid"."person_id" IN (1, 2, 3, @@ -1120,8 +1120,8 @@ FROM "posthog_person" WHERE "posthog_person"."team_id" = 2 ORDER BY "posthog_person"."id" ASC - LIMIT 5000 - OFFSET 5000 + LIMIT 1000 + OFFSET 1000 ' --- # name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.13 @@ -1212,7 +1212,7 @@ FROM "posthog_person" WHERE "posthog_person"."team_id" = 2 ORDER BY "posthog_person"."id" ASC - LIMIT 5000 + LIMIT 1000 ' --- # name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.3 @@ -1250,8 +1250,8 @@ FROM "posthog_person" WHERE "posthog_person"."team_id" = 2 ORDER BY "posthog_person"."id" ASC - LIMIT 5000 - OFFSET 5000 + LIMIT 1000 + OFFSET 1000 ' --- # name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_default_person_properties_adjustment.5 @@ -1388,7 +1388,7 @@ FROM "posthog_person" WHERE "posthog_person"."team_id" = 2 ORDER BY "posthog_person"."id" ASC - LIMIT 5000 + LIMIT 1000 ' --- # name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_deleted_flag @@ -1642,7 +1642,7 @@ FROM "posthog_person" WHERE "posthog_person"."team_id" = 2 ORDER BY "posthog_person"."id" ASC - LIMIT 5000 + LIMIT 1000 ' --- # name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_experience_continuity_flag.3 @@ -1657,7 +1657,7 @@ (SELECT U0."id" FROM "posthog_persondistinctid" U0 WHERE U0."person_id" = "posthog_persondistinctid"."person_id" - LIMIT 1) + LIMIT 3) AND "posthog_persondistinctid"."person_id" IN (1, 2, 3, @@ -1722,8 +1722,8 @@ FROM "posthog_person" WHERE "posthog_person"."team_id" = 2 ORDER BY "posthog_person"."id" ASC - LIMIT 5000 - OFFSET 5000 + LIMIT 1000 + OFFSET 1000 ' --- # name: TestCohortGenerationForFeatureFlag.test_creating_static_cohort_with_experience_continuity_flag.8 @@ -2046,7 +2046,7 @@ (SELECT U0."id" FROM "posthog_persondistinctid" U0 WHERE U0."person_id" = "posthog_persondistinctid"."person_id" - LIMIT 1) + LIMIT 3) AND "posthog_persondistinctid"."person_id" IN (1, 2, 3, @@ -2069,8 +2069,8 @@ FROM "posthog_person" WHERE "posthog_person"."team_id" = 2 ORDER BY "posthog_person"."id" ASC - LIMIT 5000 - OFFSET 5000 /*controller='project_feature_flags-create-static-cohort-for-flag',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/feature_flags/%28%3FP%3Cpk%3E%5B%5E/.%5D%2B%29/create_static_cohort_for_flag/%3F%24'*/ + LIMIT 1000 + OFFSET 1000 /*controller='project_feature_flags-create-static-cohort-for-flag',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/feature_flags/%28%3FP%3Cpk%3E%5B%5E/.%5D%2B%29/create_static_cohort_for_flag/%3F%24'*/ ' --- # name: TestFeatureFlag.test_creating_static_cohort.12 @@ -2521,7 +2521,7 @@ FROM "posthog_person" WHERE "posthog_person"."team_id" = 2 ORDER BY "posthog_person"."id" ASC - LIMIT 5000 /*controller='project_feature_flags-create-static-cohort-for-flag',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/feature_flags/%28%3FP%3Cpk%3E%5B%5E/.%5D%2B%29/create_static_cohort_for_flag/%3F%24'*/ + LIMIT 1000 /*controller='project_feature_flags-create-static-cohort-for-flag',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/feature_flags/%28%3FP%3Cpk%3E%5B%5E/.%5D%2B%29/create_static_cohort_for_flag/%3F%24'*/ ' --- # name: TestResiliency.test_feature_flags_v3_with_experience_continuity_working_slow_db