diff --git a/posthog/hogql_queries/insights/funnels/base.py b/posthog/hogql_queries/insights/funnels/base.py index a9d33da4d0a0c..ddecc4c0eb2cd 100644 --- a/posthog/hogql_queries/insights/funnels/base.py +++ b/posthog/hogql_queries/insights/funnels/base.py @@ -54,6 +54,28 @@ def __init__(self, context: FunnelQueryContext): self._extra_event_fields = ["uuid"] self._extra_event_properties = ["$session_id", "$window_id"] + # validate exclusions + if self.context.funnelsFilter.exclusions is not None: + for exclusion in self.context.funnelsFilter.exclusions: + if exclusion.funnelFromStep >= exclusion.funnelToStep: + raise ValidationError( + "Exclusion event range is invalid. End of range should be greater than start." + ) + + if exclusion.funnelFromStep >= len(self.context.query.series) - 1: + raise ValidationError( + "Exclusion event range is invalid. Start of range is greater than number of steps." + ) + + if exclusion.funnelToStep > len(self.context.query.series) - 1: + raise ValidationError( + "Exclusion event range is invalid. End of range is greater than number of steps." + ) + + for entity in self.context.query.series[exclusion.funnelFromStep : exclusion.funnelToStep + 1]: + if is_equal(entity, exclusion) or is_superset(entity, exclusion): + raise ValidationError("Exclusion steps cannot contain an event that's part of funnel steps.") + def get_query(self) -> ast.SelectQuery: raise NotImplementedError() diff --git a/posthog/hogql_queries/insights/funnels/test/test_funnel.py b/posthog/hogql_queries/insights/funnels/test/test_funnel.py index 649d7b4154b85..d132927863542 100644 --- a/posthog/hogql_queries/insights/funnels/test/test_funnel.py +++ b/posthog/hogql_queries/insights/funnels/test/test_funnel.py @@ -3,6 +3,7 @@ import uuid from django.test import override_settings from freezegun import freeze_time +from rest_framework.exceptions import ValidationError from posthog.api.instance_settings import get_instance_setting from posthog.clickhouse.client.execute import sync_execute from posthog.constants import INSIGHT_FUNNELS, FunnelOrderType @@ -1765,69 +1766,70 @@ def test_funnel_conversion_window_seconds(self): ids_to_compare, ) - # def test_funnel_exclusions_invalid_params(self): - # filters = { - # "events": [ - # {"id": "user signed up", "type": "events", "order": 0}, - # {"id": "paid", "type": "events", "order": 1}, - # ], - # "insight": INSIGHT_FUNNELS, - # "funnel_window_days": 14, - # "date_from": "2021-05-01 00:00:00", - # "date_to": "2021-05-14 00:00:00", - # "exclusions": [ - # { - # "id": "x", - # "type": "events", - # "funnel_from_step": 1, - # "funnel_to_step": 1, - # } - # ], - # } - # filter = Filter(data=filters) - # self.assertRaises(ValidationError, lambda: Funnel(filter, self.team)) - - # filter = filter.shallow_clone( - # { - # "exclusions": [ - # { - # "id": "x", - # "type": "events", - # "funnel_from_step": 1, - # "funnel_to_step": 2, - # } - # ] - # } - # ) - # self.assertRaises(ValidationError, lambda: Funnel(filter, self.team)) - - # filter = filter.shallow_clone( - # { - # "exclusions": [ - # { - # "id": "x", - # "type": "events", - # "funnel_from_step": 2, - # "funnel_to_step": 1, - # } - # ] - # } - # ) - # self.assertRaises(ValidationError, lambda: Funnel(filter, self.team)) - - # filter = filter.shallow_clone( - # { - # "exclusions": [ - # { - # "id": "x", - # "type": "events", - # "funnel_from_step": 0, - # "funnel_to_step": 2, - # } - # ] - # } - # ) - # self.assertRaises(ValidationError, lambda: Funnel(filter, self.team)) + def test_funnel_exclusions_invalid_params(self): + filters = { + "events": [ + {"id": "user signed up", "type": "events", "order": 0}, + {"id": "paid", "type": "events", "order": 1}, + ], + "insight": INSIGHT_FUNNELS, + "funnel_window_days": 14, + "date_from": "2021-05-01 00:00:00", + "date_to": "2021-05-14 00:00:00", + "exclusions": [ + { + "id": "x", + "type": "events", + "funnel_from_step": 1, + "funnel_to_step": 1, + } + ], + } + + query = cast(FunnelsQuery, filter_to_query(filters)) + self.assertRaises(ValidationError, lambda: FunnelsQueryRunner(query=query, team=self.team).calculate()) + + filters = { + **filters, + "exclusions": [ + { + "id": "x", + "type": "events", + "funnel_from_step": 1, + "funnel_to_step": 2, + } + ], + } + query = cast(FunnelsQuery, filter_to_query(filters)) + self.assertRaises(ValidationError, lambda: FunnelsQueryRunner(query=query, team=self.team).calculate()) + + filters = { + **filters, + "exclusions": [ + { + "id": "x", + "type": "events", + "funnel_from_step": 2, + "funnel_to_step": 1, + } + ], + } + query = cast(FunnelsQuery, filter_to_query(filters)) + self.assertRaises(ValidationError, lambda: FunnelsQueryRunner(query=query, team=self.team).calculate()) + + filters = { + **filters, + "exclusions": [ + { + "id": "x", + "type": "events", + "funnel_from_step": 0, + "funnel_to_step": 2, + } + ], + } + query = cast(FunnelsQuery, filter_to_query(filters)) + self.assertRaises(ValidationError, lambda: FunnelsQueryRunner(query=query, team=self.team).calculate()) def test_funnel_exclusion_no_end_event(self): filters = { diff --git a/posthog/hogql_queries/insights/utils/entities.py b/posthog/hogql_queries/insights/utils/entities.py index 4fd6d7260672a..794ce6170da11 100644 --- a/posthog/hogql_queries/insights/utils/entities.py +++ b/posthog/hogql_queries/insights/utils/entities.py @@ -3,25 +3,45 @@ CohortPropertyFilter, EmptyPropertyFilter, EventsNode, + FunnelExclusionActionsNode, + FunnelExclusionEventsNode, HogQLPropertyFilter, ) -from posthog.types import AnyPropertyFilter, EntityNode +from posthog.types import AnyPropertyFilter, EntityNode, ExclusionEntityNode from collections import Counter +from rest_framework.exceptions import ValidationError -def is_equal(a: EntityNode, b: EntityNode, compare_properties=True) -> bool: +def is_equal_type(a: EntityNode, b: EntityNode | ExclusionEntityNode) -> bool: + if isinstance(a, EventsNode): + return isinstance(b, EventsNode) or isinstance(b, FunnelExclusionEventsNode) + elif isinstance(a, ActionsNode): + return isinstance(b, ActionsNode) or isinstance(b, FunnelExclusionActionsNode) + else: + raise ValidationError(detail=f"Type comparision for {type(a)} and {type(b)} not implemented.") + + +def is_equal(a: EntityNode, b: EntityNode | ExclusionEntityNode, compare_properties=True) -> bool: """Checks if two entities are semantically equal.""" # different type - if type(a) != type(b): + if not is_equal_type(a, b): return False # different action - if isinstance(a, ActionsNode) and isinstance(b, ActionsNode) and a.id != b.id: + if ( + isinstance(a, ActionsNode | FunnelExclusionActionsNode) + and isinstance(b, ActionsNode | FunnelExclusionActionsNode) + and a.id != b.id + ): return False # different event - if isinstance(a, EventsNode) and isinstance(b, EventsNode) and a.event != b.event: + if ( + isinstance(a, EventsNode | FunnelExclusionEventsNode) + and isinstance(b, EventsNode | FunnelExclusionEventsNode) + and a.event != b.event + ): return False # different properties @@ -37,7 +57,7 @@ def is_equal(a: EntityNode, b: EntityNode, compare_properties=True) -> bool: return True -def is_superset(a: EntityNode, b: EntityNode) -> bool: +def is_superset(a: EntityNode, b: EntityNode | ExclusionEntityNode) -> bool: """Checks if this entity is a superset version of other. The nodes match and the properties of (a) is a subset of the properties of (b).""" if not is_equal(a, b, compare_properties=False):