APIv2: Fix percentage metric 500s (#5099)

* Refactor: remove metrics argument from merge_imported()

* Support querying percentage without visitors metric

* Fix ordering by special metrics with imports causing a 500

We don't calculate all metrics directly on imports, hence cannot order
the import by them either.

* Changelog

CHANGELOG.md

@@ -32,6 +32,7 @@ All notable changes to this project will be documented in this file.
 - Fix year over year comparisons being offset by a day for leap years
 - Breakdown modals now display correct comparison values instead of 0 after pagination
 - Fix database mismatch between event and session user_ids after rotating salts
+- `/api/v2/query` no longer returns a 500 when querying percentage metric without `visitors`
 
 ## v2.1.5-rc.1 - 2025-01-17
 

lib/plausible/stats/imported/imported.ex

@@ -218,26 +218,26 @@ defmodule Plausible.Stats.Imported do
     {table, db_field}
   end
 
-  def merge_imported(q, _, %Query{include_imported: false}, _), do: q
+  def merge_imported(q, _, %Query{include_imported: false}), do: q
 
-  def merge_imported(q, site, %Query{dimensions: []} = query, metrics) do
+  def merge_imported(q, site, %Query{dimensions: []} = query) do
     q = paginate_optimization(q, query)
 
     imported_q =
       site
       |> Imported.Base.query_imported(query)
-      |> select_imported_metrics(metrics)
+      |> select_imported_metrics(query.metrics)
       |> paginate_optimization(query)
 
     from(
       s in subquery(q),
       cross_join: i in subquery(imported_q),
       select: %{}
     )
-    |> select_joined_metrics(metrics)
+    |> select_joined_metrics(query.metrics)
   end
 
-  def merge_imported(q, site, %Query{dimensions: ["event:goal"]} = query, metrics) do
+  def merge_imported(q, site, %Query{dimensions: ["event:goal"]} = query) do
     goal_join_data = Plausible.Stats.Goals.goal_join_data(query)
 
     Imported.Base.decide_tables(query)
@@ -253,7 +253,7 @@ defmodule Plausible.Stats.Imported do
               i.name
             )
         })
-        |> select_imported_metrics(metrics)
+        |> select_imported_metrics(query.metrics)
         |> group_by([], selected_as(:dim0))
         |> where([], selected_as(:dim0) != 0)
 
@@ -282,14 +282,14 @@ defmodule Plausible.Stats.Imported do
         |> select_merge_as([_i, index], %{
           dim0: fragment("CAST(?, 'UInt64')", index)
         })
-        |> select_imported_metrics(metrics)
+        |> select_imported_metrics(query.metrics)
     end)
     |> Enum.reduce(q, fn imports_q, q ->
-      naive_dimension_join(q, imports_q, metrics)
+      naive_dimension_join(q, imports_q, query.metrics)
     end)
   end
 
-  def merge_imported(q, site, query, metrics) do
+  def merge_imported(q, site, query) do
     if schema_supports_query?(query) do
       q = paginate_optimization(q, query)
 
@@ -298,7 +298,7 @@ defmodule Plausible.Stats.Imported do
         |> Imported.Base.query_imported(query)
         |> where([i], i.visitors > 0)
         |> group_imported_by(query)
-        |> select_imported_metrics(metrics)
+        |> select_imported_metrics(query.metrics)
         |> paginate_optimization(query)
 
       from(s in subquery(q),
@@ -307,7 +307,7 @@ defmodule Plausible.Stats.Imported do
         select: %{}
       )
       |> select_joined_dimensions(query)
-      |> select_joined_metrics(metrics)
+      |> select_joined_metrics(query.metrics)
     else
       q
     end
@@ -356,9 +356,17 @@ defmodule Plausible.Stats.Imported do
     end
   end
 
+  @cannot_optimize_metrics [
+    :scroll_depth,
+    :percentage,
+    :conversion_rate,
+    :group_conversion_rate,
+    :time_on_page
+  ]
+
   defp can_order_by?(query) do
     Enum.all?(query.order_by, fn
-      {:scroll_depth, _} -> false
+      {metric, _direction} when metric in @cannot_optimize_metrics -> false
       {metric, _direction} when is_atom(metric) -> metric in query.metrics
       _ -> true
     end)

lib/plausible/stats/sql/query_builder.ex

@@ -48,7 +48,7 @@ defmodule Plausible.Stats.SQL.QueryBuilder do
     q
     |> join_sessions_if_needed(events_query)
     |> build_group_by(:events, events_query)
-    |> merge_imported(site, events_query, events_query.metrics)
+    |> merge_imported(site, events_query)
     |> SQL.SpecialMetrics.add(site, events_query)
   end
 
@@ -94,7 +94,7 @@ defmodule Plausible.Stats.SQL.QueryBuilder do
     q
     |> join_events_if_needed(sessions_query)
     |> build_group_by(:sessions, sessions_query)
-    |> merge_imported(site, sessions_query, sessions_query.metrics)
+    |> merge_imported(site, sessions_query)
     |> SQL.SpecialMetrics.add(site, sessions_query)
   end
 

lib/plausible/stats/util.ex

@@ -10,7 +10,10 @@ defmodule Plausible.Stats.Util do
   """
   def maybe_add_visitors_metric(metrics) do
     needed? =
-      Enum.any?([:conversion_rate, :group_conversion_rate, :time_on_page], &(&1 in metrics))
+      Enum.any?(
+        [:percentage, :conversion_rate, :group_conversion_rate, :time_on_page],
+        &(&1 in metrics)
+      )
 
     if needed? and :visitors not in metrics do
       metrics ++ [:visitors]

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

@@ -190,4 +190,30 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QuerySpecialMetricsTest do
              %{"dimensions" => ["Mobile"], "metrics" => [50]}
            ]
   end
+
+  test "can break down by visit:device with only percentage metric", %{conn: conn, site: site} do
+    site_import = insert(:site_import, site: site)
+
+    populate_stats(site, site_import.id, [
+      build(:pageview, screen_size: "Mobile"),
+      build(:pageview, screen_size: "Mobile"),
+      build(:pageview, screen_size: "Desktop"),
+      build(:imported_visitors, visitors: 5, date: ~D[2021-01-01]),
+      build(:imported_devices, device: "Desktop", visitors: 5, date: ~D[2021-01-01])
+    ])
+
+    conn =
+      post(conn, "/api/v2/query", %{
+        "site_id" => site.domain,
+        "metrics" => ["percentage"],
+        "date_range" => "all",
+        "dimensions" => ["visit:device"],
+        "include" => %{"imports" => true}
+      })
+
+    assert json_response(conn, 200)["results"] == [
+             %{"dimensions" => ["Desktop"], "metrics" => [75.0]},
+             %{"dimensions" => ["Mobile"], "metrics" => [25.0]}
+           ]
+  end
 end