From 847343f7a950ca8119dd8d2f307e8c7f64d31ca2 Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Fri, 24 Jan 2025 10:48:34 +0200 Subject: [PATCH] Properly validate combining event-only metrics with session-only dimensions 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 | 23 +---------- lib/plausible/stats/table_decider.ex | 39 +++++++++++++++++++ test/plausible/stats/query_parser_test.exs | 15 ++++++- test/plausible/stats/table_decider_test.exs | 30 +++++++++++++- .../query_validations_test.exs | 4 +- 5 files changed, 85 insertions(+), 26 deletions(-) diff --git a/lib/plausible/stats/filters/query_parser.ex b/lib/plausible/stats/filters/query_parser.ex index c41795bf202e..574cd383621c 100644 --- a/lib/plausible/stats/filters/query_parser.ex +++ b/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) diff --git a/lib/plausible/stats/table_decider.ex b/lib/plausible/stats/table_decider.ex index 6820f7c607fd..b30f9a8d1f7d 100644 --- a/lib/plausible/stats/table_decider.ex +++ b/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 diff --git a/test/plausible/stats/query_parser_test.exs b/test/plausible/stats/query_parser_test.exs index 338e071e7922..bde1dfe8e504 100644 --- a/test/plausible/stats/query_parser_test.exs +++ b/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 diff --git a/test/plausible/stats/table_decider_test.exs b/test/plausible/stats/table_decider_test.exs index df19b16d41bd..2dd2c2c78085 100644 --- a/test/plausible/stats/table_decider_test.exs +++ b/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), diff --git a/test/plausible_web/controllers/api/external_stats_controller/query_validations_test.exs b/test/plausible_web/controllers/api/external_stats_controller/query_validations_test.exs index 1c805c08fb0f..deb4dc97ce9c 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/query_validations_test.exs +++ b/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", %{