Skip to content

Commit

Permalink
fix(hogql): validate funnel exclusions (#20794)
Browse files Browse the repository at this point in the history
  • Loading branch information
@thmsobrmlr
thmsobrmlr authored Mar 8, 2024
1 parent ea340fc commit 01b1f48
Show file tree
Hide file tree
Showing 3 changed files with 113 additions and 69 deletions.
22 changes: 22 additions & 0 deletions posthog/hogql_queries/insights/funnels/base.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
128 changes: 65 additions & 63 deletions posthog/hogql_queries/insights/funnels/test/test_funnel.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 = {
Expand Down
32 changes: 26 additions & 6 deletions posthog/hogql_queries/insights/utils/entities.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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):
Expand Down

0 comments on commit 01b1f48

Please sign in to comment.