Properly validate combining event-only metrics with session-only dime…

…nsions

Currently the following query results in a 500:

```json
{
  "site_id": "plausible.io",
  "metrics": ["visitors", "events", "pageviews"],
  "dimensions": ["visit:exit_page"],
  "date_range": "7d"
}
```

This adds proper validation for that case that was previously missing.

lib/plausible/stats/filters/query_parser.ex

@@ -592,7 +592,7 @@ defmodule Plausible.Stats.Filters.QueryParser do
 
   defp validate_metrics(query) do
     with :ok <- validate_list(query.metrics, &validate_metric(&1, query)) do
-      validate_no_metrics_filters_conflict(query)
+      TableDecider.validate_no_metrics_dimensions_conflict(query)
     end
   end
 
@@ -631,27 +631,6 @@ defmodule Plausible.Stats.Filters.QueryParser do
 
   defp validate_metric(_, _), do: :ok
 
-  defp validate_no_metrics_filters_conflict(query) do
-    {_event_metrics, sessions_metrics, _other_metrics} =
-      TableDecider.partition_metrics(query.metrics, query)
-
-    if Enum.empty?(sessions_metrics) or
-         not event_dimensions_not_allowing_session_metrics?(query.dimensions) do
-      :ok
-    else
-      {:error,
-       "Session metric(s) `#{sessions_metrics |> Enum.join(", ")}` cannot be queried along with event dimensions."}
-    end
-  end
-
-  defp event_dimensions_not_allowing_session_metrics?(dimensions) do
-    Enum.any?(dimensions, fn
-      "event:page" -> false
-      "event:" <> _ -> true
-      _ -> false
-    end)
-  end
-
   defp validate_include(query) do
     time_dimension? = Enum.any?(query.dimensions, &Time.time_dimension?/1)
 

lib/plausible/stats/table_decider.ex

@@ -21,6 +21,39 @@ defmodule Plausible.Stats.TableDecider do
     |> Enum.any?(&(dimension_partitioner(query, &1) == :event))
   end
 
+  @doc """
+  Validates whether metrics and dimensions are compatible with each other.
+
+  During query building we split query into two: event and session queries. However dimensions need to be
+  present in both queries and hence must be compatible.
+
+  Used during query parsing
+  """
+  def validate_no_metrics_dimensions_conflict(query) do
+    %{event: event_only_metrics, session: session_only_metrics} =
+      partition(query.metrics, query, &metric_partitioner/2)
+
+    %{event: event_only_dimensions, session: session_only_dimensions} =
+      partition(query.dimensions, query, &dimension_partitioner/2)
+
+    cond do
+      # event:page is a special case handled in QueryOptimizer.split_sessions_query
+      event_only_dimensions == ["event:page"] ->
+        :ok
+
+      not empty?(session_only_metrics) and not empty?(event_only_dimensions) ->
+        {:error,
+         "Session metric(s) #{i(session_only_metrics)} cannot be queried along with event dimension(s) #{i(event_only_dimensions)}"}
+
+      not empty?(event_only_metrics) and not empty?(session_only_dimensions) ->
+        {:error,
+         "Event metric(s) #{i(event_only_metrics)} cannot be queried along with session dimension(s) #{i(session_only_dimensions)}"}
+
+      true ->
+        :ok
+    end
+  end
+
   def partition_metrics(metrics, query) do
     %{
       event: event_only_metrics,
@@ -104,4 +137,10 @@ defmodule Plausible.Stats.TableDecider do
       Map.put(acc, key, Map.fetch!(acc, key) ++ [value])
     end)
   end
+
+  defp i(list) when is_list(list) do
+    list
+    |> Enum.map(&"`#{&1}`")
+    |> Enum.join(", ")
+  end
 end

test/plausible/stats/query_parser_test.exs

@@ -2187,7 +2187,20 @@ defmodule Plausible.Stats.Filters.QueryParserTest do
       }
       |> check_error(
         site,
-        "Session metric(s) `bounce_rate` cannot be queried along with event dimensions."
+        "Session metric(s) `bounce_rate` cannot be queried along with event dimension(s) `event:props:foo`"
+      )
+    end
+
+    test "fails if using event metric with session-only dimension", %{site: site} do
+      %{
+        "site_id" => site.domain,
+        "metrics" => ["events"],
+        "date_range" => "all",
+        "dimensions" => ["visit:exit_page"]
+      }
+      |> check_error(
+        site,
+        "Event metric(s) `events` cannot be queried along with session dimension(s) `visit:exit_page`"
       )
     end
 

test/plausible/stats/table_decider_test.exs

@@ -152,7 +152,35 @@ defmodule Plausible.Stats.TableDeciderTest do
     end
   end
 
-  defp make_query(filter_dimensions, dimensions \\ []) do
+  describe "validate_no_metrics_dimensions_conflict" do
+    for {metrics, dimensions, expected} <- [
+          {[], [], :ok},
+          {[:bounce_rate], [], :ok},
+          {[:scroll_depth], [], :ok},
+          {[:bounce_rate], ["visit:exit_page"], :ok},
+          {[:scroll_depth], ["event:name"], :ok},
+          {[:scroll_depth], ["visit:device"], :ok},
+          {[:bounce_rate, :scroll_depth], ["event:name"],
+           {:error,
+            "Session metric(s) `bounce_rate` cannot be queried along with event dimension(s) `event:name`"}},
+          {[:visit_duration], ["event:props:foo"],
+           {:error,
+            "Session metric(s) `visit_duration` cannot be queried along with event dimension(s) `event:props:foo`"}},
+          {[:bounce_rate, :scroll_depth], ["visit:exit_page"],
+           {:error,
+            "Event metric(s) `scroll_depth` cannot be queried along with session dimension(s) `visit:exit_page`"}},
+          {[:bounce_rate, :scroll_depth], ["event:page"], :ok}
+        ] do
+      test "metrics #{inspect(metrics)} and dimensions #{inspect(dimensions)}" do
+        query =
+          make_query() |> Query.set(metrics: unquote(metrics), dimensions: unquote(dimensions))
+
+        assert validate_no_metrics_dimensions_conflict(query) == unquote(expected)
+      end
+    end
+  end
+
+  defp make_query(filter_dimensions \\ [], dimensions \\ []) do
     Query.from(build(:site), %{
       "filters" =>
         Enum.map(filter_dimensions, fn filter_dimension -> ["is", filter_dimension, []] end),

test/plausible_web/controllers/api/external_stats_controller/query_validations_test.exs

@@ -163,7 +163,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryValidationsTest do
         })
 
       assert json_response(conn, 400)["error"] =~
-               "Session metric(s) `bounce_rate` cannot be queried along with event dimensions."
+               "Session metric(s) `bounce_rate` cannot be queried along with event dimension(s) `event:name`"
     end
 
     test "session metrics cannot be used with event:props:* dimension", %{conn: conn, site: site} do
@@ -176,7 +176,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryValidationsTest do
         })
 
       assert json_response(conn, 400)["error"] =~
-               "Session metric(s) `bounce_rate` cannot be queried along with event dimensions."
+               "Session metric(s) `bounce_rate` cannot be queried along with event dimension(s) `event:props:url`"
     end
 
     test "validates that metric views_per_visit cannot be used with event:page filter", %{