fix(funnels): Validate funnelStep (#23253)

* fix(funnels): Validate `funnelStep`

* Apply suggestions from code review

Co-authored-by: Thomas Obermüller <[email protected]>

* Fix issues caused by rewording

---------

Co-authored-by: Thomas Obermüller <[email protected]>

posthog/hogql_queries/insights/funnels/base.py

@@ -315,6 +315,25 @@ def _serialize_step(
     def extra_event_fields_and_properties(self):
         return self._extra_event_fields + self._extra_event_properties
 
+    @property
+    def _absolute_actors_step(self) -> Optional[int]:
+        """The actor query's 1-indexed target step converted to our 0-indexed SQL form. Never a negative integer."""
+        if self.context.actorsQuery is None or self.context.actorsQuery.funnelStep is None:
+            return None
+
+        target_step = self.context.actorsQuery.funnelStep
+        if target_step < 0:
+            if target_step == -1:
+                raise ValueError(
+                    "The first valid drop-off argument for funnelStep is -2. -2 refers to persons who performed "
+                    "the first step but never made it to the second."
+                )
+            return abs(target_step) - 2
+        elif target_step == 0:
+            raise ValueError("Funnel steps are 1-indexed, so step 0 doesn't exist")
+        else:
+            return target_step - 1
+
     def _get_inner_event_query(
         self,
         entities: list[EntityNode] | None = None,
@@ -626,19 +645,14 @@ def _get_funnel_person_step_events(self) -> list[ast.Expr]:
             and self.context.actorsQuery is not None
             and self.context.actorsQuery.includeRecordings
         ):
-            step_num = self.context.actorsQuery.funnelStep
             if self.context.includeFinalMatchingEvents:
                 # Always returns the user's final step of the funnel
                 return [parse_expr("final_matching_events as matching_events")]
-            elif step_num is None:
+
+            absolute_actors_step = self._absolute_actors_step
+            if absolute_actors_step is None:
                 raise ValueError("Missing funnelStep actors query property")
-            if step_num >= 0:
-                # None drop off case
-                matching_events_step_num = step_num - 1
-            else:
-                # Drop off case if negative number
-                matching_events_step_num = abs(step_num) - 2
-            return [parse_expr(f"step_{matching_events_step_num}_matching_events as matching_events")]
+            return [parse_expr(f"step_{absolute_actors_step}_matching_events as matching_events")]
         return []
 
     def _get_count_columns(self, max_steps: int) -> list[ast.Expr]:
@@ -765,29 +779,13 @@ def _get_timestamp_selects(self) -> tuple[list[ast.Expr], list[ast.Expr]]:
         Returns timestamp selectors for the target step and optionally the preceding step.
         In the former case, always returns the timestamp for the first and last step as well.
         """
-        actorsQuery, max_steps = (
-            self.context.actorsQuery,
-            self.context.max_steps,
-        )
-        if not actorsQuery:
-            return [], []
-
-        target_step = actorsQuery.funnelStep
-        final_step = max_steps - 1
-        first_step = 0
+        target_step = self._absolute_actors_step
 
-        if not target_step:
+        if target_step is None:
             return [], []
 
-        if target_step < 0:
-            # the first valid dropoff argument for funnel_step is -2
-            # -2 refers to persons who performed the first step but never made it to the second
-            if target_step == -1:
-                raise ValueError("To request dropoff of initial step use -2")
-
-            target_step = abs(target_step) - 2
-        else:
-            target_step -= 1
+        final_step = self.context.max_steps - 1
+        first_step = 0
 
         if self.context.includePrecedingTimestamp:
             if target_step == 0:

posthog/hogql_queries/insights/funnels/test/test_funnel_unordered_persons.py

@@ -39,26 +39,29 @@ def _create_sample_data_multiple_dropoffs(self):
 
         journeys_for(events_by_person, self.team)
 
-    # def test_invalid_steps(self):
-    #     filters = {
-    #         "insight": INSIGHT_FUNNELS,
-    #         "funnel_order_type": "unordered",
-    #         "interval": "day",
-    #         "date_from": "2021-05-01 00:00:00",
-    #         "date_to": "2021-05-07 00:00:00",
-    #         "funnel_window_days": 7,
-    #         "events": [
-    #             {"id": "step one", "order": 0},
-    #             {"id": "step two", "order": 1},
-    #             {"id": "step three", "order": 2},
-    #         ],
-    #     }
-
-    #     with self.assertRaises(ValueError):
-    #         get_actors(filters, self.team, funnelStep="blah")  # type: ignore
-
-    #     with pytest.raises(ValueError):
-    #         get_actors(filters, self.team, funnelStep=-1)
+    def test_invalid_steps(self):
+        filters = {
+            "insight": INSIGHT_FUNNELS,
+            "funnel_order_type": "unordered",
+            "interval": "day",
+            "date_from": "2021-05-01 00:00:00",
+            "date_to": "2021-05-07 00:00:00",
+            "funnel_window_days": 7,
+            "events": [
+                {"id": "step one", "order": 0},
+                {"id": "step two", "order": 1},
+                {"id": "step three", "order": 2},
+            ],
+        }
+
+        with self.assertRaisesMessage(ValueError, "Input should be a valid integer"):
+            get_actors(filters, self.team, funnel_step="blah")  # type: ignore
+
+        with self.assertRaisesMessage(ValueError, "Funnel steps are 1-indexed, so step 0 doesn't exist"):
+            get_actors(filters, self.team, funnel_step=0)
+
+        with self.assertRaisesMessage(ValueError, "The first valid drop-off argument for funnelStep is -2"):
+            get_actors(filters, self.team, funnel_step=-1)
 
     def test_first_step(self):
         self._create_sample_data_multiple_dropoffs()