Fix breakdown API pagination when using event metrics (#2562)

* Fix breakdown API pagination when using event metrics

This commit fixes a bug where the subsequent breakdown API pages had
the same items as the first page. The fix sorts the underlying
ClickHouse query by timestamp, keeping the same order between requests,
as we use OFFSET/LIMIT pagination.

* Fix repeated results assertion

* Add different ORDER BY to each breakdown property

CHANGELOG.md

@@ -4,6 +4,7 @@ All notable changes to this project will be documented in this file.
 ## Unreleased
 
 ### Fixed
+- Fix breakdown API pagination when using event metrics plausible/analytics#2562
 - Automatically update all visible dashboard reports in the realtime view
 
 ### Changed

lib/plausible/stats/breakdown.ex

@@ -327,7 +327,8 @@ defmodule Plausible.Stats.Breakdown do
       group_by: e.name,
       where: meta.key == ^prop,
       group_by: meta.value,
-      select_merge: %{^prop => meta.value}
+      select_merge: %{^prop => meta.value},
+      order_by: {:asc, meta.value}
     )
   end
 
@@ -340,7 +341,8 @@ defmodule Plausible.Stats.Breakdown do
       inner_lateral_join: meta in fragment("meta"),
       where: meta.key == ^prop,
       group_by: meta.value,
-      select_merge: %{^prop => meta.value}
+      select_merge: %{^prop => meta.value},
+      order_by: {:asc, meta.value}
     )
   end
 
@@ -351,7 +353,8 @@ defmodule Plausible.Stats.Breakdown do
     from(
       e in q,
       group_by: e.name,
-      select_merge: %{name: e.name}
+      select_merge: %{name: e.name},
+      order_by: {:asc, e.name}
     )
   end
 
@@ -362,7 +365,8 @@ defmodule Plausible.Stats.Breakdown do
     from(
       e in q,
       group_by: e.pathname,
-      select_merge: %{page: e.pathname}
+      select_merge: %{page: e.pathname},
+      order_by: {:asc, e.pathname}
     )
   end
 
@@ -378,7 +382,8 @@ defmodule Plausible.Stats.Breakdown do
           select_merge: %{
             index: fragment("arrayJoin(indices) as index"),
             page_match: fragment("array(?)[index]", ^match_exprs)
-          }
+          },
+          order_by: {:asc, fragment("index")}
         )
     end
   end
@@ -389,7 +394,8 @@ defmodule Plausible.Stats.Breakdown do
       group_by: s.referrer_source,
       select_merge: %{
         source: fragment("if(empty(?), ?, ?)", s.referrer_source, @no_ref, s.referrer_source)
-      }
+      },
+      order_by: {:asc, s.referrer_source}
     )
   end
 
@@ -398,7 +404,8 @@ defmodule Plausible.Stats.Breakdown do
       s in q,
       where: s.country_code != "\0\0" and s.country_code != "ZZ",
       group_by: s.country_code,
-      select_merge: %{country: s.country_code}
+      select_merge: %{country: s.country_code},
+      order_by: {:asc, s.country_code}
     )
   end
 
@@ -407,7 +414,8 @@ defmodule Plausible.Stats.Breakdown do
       s in q,
       where: s.subdivision1_code != "",
       group_by: s.subdivision1_code,
-      select_merge: %{region: s.subdivision1_code}
+      select_merge: %{region: s.subdivision1_code},
+      order_by: {:asc, s.subdivision1_code}
     )
   end
 
@@ -416,23 +424,26 @@ defmodule Plausible.Stats.Breakdown do
       s in q,
       where: s.city_geoname_id != 0,
       group_by: s.city_geoname_id,
-      select_merge: %{city: s.city_geoname_id}
+      select_merge: %{city: s.city_geoname_id},
+      order_by: {:asc, s.city_geoname_id}
     )
   end
 
   defp do_group_by(q, "visit:entry_page") do
     from(
       s in q,
       group_by: s.entry_page,
-      select_merge: %{entry_page: s.entry_page}
+      select_merge: %{entry_page: s.entry_page},
+      order_by: {:asc, s.entry_page}
     )
   end
 
   defp do_group_by(q, "visit:exit_page") do
     from(
       s in q,
       group_by: s.exit_page,
-      select_merge: %{exit_page: s.exit_page}
+      select_merge: %{exit_page: s.exit_page},
+      order_by: {:asc, s.exit_page}
     )
   end
 
@@ -442,7 +453,8 @@ defmodule Plausible.Stats.Breakdown do
       group_by: s.referrer,
       select_merge: %{
         referrer: fragment("if(empty(?), ?, ?)", s.referrer, @no_ref, s.referrer)
-      }
+      },
+      order_by: {:asc, s.referrer}
     )
   end
 
@@ -500,39 +512,44 @@ defmodule Plausible.Stats.Breakdown do
     from(
       s in q,
       group_by: s.screen_size,
-      select_merge: %{device: s.screen_size}
+      select_merge: %{device: s.screen_size},
+      order_by: {:asc, s.screen_size}
     )
   end
 
   defp do_group_by(q, "visit:os") do
     from(
       s in q,
       group_by: s.operating_system,
-      select_merge: %{operating_system: s.operating_system}
+      select_merge: %{operating_system: s.operating_system},
+      order_by: {:asc, s.operating_system}
     )
   end
 
   defp do_group_by(q, "visit:os_version") do
     from(
       s in q,
       group_by: s.operating_system_version,
-      select_merge: %{os_version: s.operating_system_version}
+      select_merge: %{os_version: s.operating_system_version},
+      order_by: {:asc, s.operating_system_version}
     )
   end
 
   defp do_group_by(q, "visit:browser") do
     from(
       s in q,
       group_by: s.browser,
-      select_merge: %{browser: s.browser}
+      select_merge: %{browser: s.browser},
+      order_by: {:asc, s.browser}
     )
   end
 
   defp do_group_by(q, "visit:browser_version") do
     from(
       s in q,
       group_by: s.browser_version,
-      select_merge: %{browser_version: s.browser_version}
+      select_merge: %{browser_version: s.browser_version},
+      order_by: {:asc, s.browser_version}
     )
   end
 

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

@@ -1588,6 +1588,41 @@ defmodule PlausibleWeb.Api.ExternalStatsController.BreakdownTest do
       assert Enum.count(res["results"]) == 2
     end
 
+    test "does not repeat results", %{conn: conn, site: site} do
+      populate_stats([
+        build(:pageview, %{domain: site.domain, "meta.key": ["item"], "meta.value": ["apple"]}),
+        build(:pageview, %{domain: site.domain, "meta.key": ["item"], "meta.value": ["kiwi"]}),
+        build(:pageview, %{domain: site.domain, "meta.key": ["item"], "meta.value": ["pineapple"]}),
+        build(:pageview, %{domain: site.domain, "meta.key": ["item"], "meta.value": ["grapes"]})
+      ])
+
+      params = %{
+        "site_id" => site.domain,
+        "metrics" => "visitors",
+        "property" => "event:props:item",
+        "limit" => 3,
+        "page" => nil
+      }
+
+      first_page =
+        conn
+        |> get("/api/v1/stats/breakdown", %{params | "page" => 1})
+        |> json_response(200)
+        |> Map.get("results")
+        |> Enum.map(& &1["item"])
+        |> MapSet.new()
+
+      second_page =
+        conn
+        |> get("/api/v1/stats/breakdown", %{params | "page" => 2})
+        |> json_response(200)
+        |> Map.get("results")
+        |> Enum.map(& &1["item"])
+        |> MapSet.new()
+
+      assert first_page |> MapSet.intersection(second_page) |> Enum.empty?()
+    end
+
     @invalid_limit_message "Please provide limit as a number between 1 and 1000."
 
     test "returns error when limit too large", %{conn: conn, site: site} do

test/plausible_web/controllers/api/stats_controller/countries_test.exs

@@ -100,15 +100,6 @@ defmodule PlausibleWeb.Api.StatsController.CountriesTest do
       conn = get(conn, "/api/stats/#{site.domain}/countries?period=day&filters=#{filters}")
 
       assert json_response(conn, 200) == [
-               %{
-                 "code" => "GB",
-                 "alpha_3" => "GBR",
-                 "name" => "United Kingdom",
-                 "flag" => "🇬🇧",
-                 "total_visitors" => 1,
-                 "visitors" => 1,
-                 "conversion_rate" => 100.0
-               },
                %{
                  "code" => "EE",
                  "alpha_3" => "EST",
@@ -117,6 +108,15 @@ defmodule PlausibleWeb.Api.StatsController.CountriesTest do
                  "total_visitors" => 2,
                  "visitors" => 1,
                  "conversion_rate" => 50.0
+               },
+               %{
+                 "code" => "GB",
+                 "alpha_3" => "GBR",
+                 "name" => "United Kingdom",
+                 "flag" => "🇬🇧",
+                 "total_visitors" => 1,
+                 "visitors" => 1,
+                 "conversion_rate" => 100.0
                }
              ]
     end

test/plausible_web/controllers/api/stats_controller/pages_test.exs

@@ -745,21 +745,21 @@ defmodule PlausibleWeb.Api.StatsController.PagesTest do
         )
 
       assert json_response(conn, 200) == [
-               %{
-                 "total_visitors" => 1,
-                 "unique_entrances" => 1,
-                 "total_entrances" => 1,
-                 "name" => "/page2",
-                 "visit_duration" => 900,
-                 "conversion_rate" => 100.0
-               },
                %{
                  "total_visitors" => 2,
                  "unique_entrances" => 1,
                  "total_entrances" => 1,
                  "name" => "/page1",
                  "visit_duration" => 0,
                  "conversion_rate" => 50.0
+               },
+               %{
+                 "total_visitors" => 1,
+                 "unique_entrances" => 1,
+                 "total_entrances" => 1,
+                 "name" => "/page2",
+                 "visit_duration" => 900,
+                 "conversion_rate" => 100.0
                }
              ]
     end

test/plausible_web/controllers/api/stats_controller/sources_test.exs

@@ -279,17 +279,17 @@ defmodule PlausibleWeb.Api.StatsController.SourcesTest do
         )
 
       assert json_response(conn, 200) == [
-               %{
-                 "name" => "Google",
-                 "visitors" => 1,
-                 "bounce_rate" => 0,
-                 "visit_duration" => 900
-               },
                %{
                  "name" => "DuckDuckGo",
                  "visitors" => 1,
                  "bounce_rate" => 100,
                  "visit_duration" => 0
+               },
+               %{
+                 "name" => "Google",
+                 "visitors" => 1,
+                 "bounce_rate" => 0,
+                 "visit_duration" => 900
                }
              ]
     end
@@ -344,17 +344,17 @@ defmodule PlausibleWeb.Api.StatsController.SourcesTest do
         )
 
       assert json_response(conn, 200) == [
-               %{
-                 "name" => "Google",
-                 "visitors" => 1,
-                 "bounce_rate" => 0,
-                 "visit_duration" => 900
-               },
                %{
                  "name" => "DuckDuckGo",
                  "visitors" => 1,
                  "bounce_rate" => 100,
                  "visit_duration" => 0
+               },
+               %{
+                 "name" => "Google",
+                 "visitors" => 1,
+                 "bounce_rate" => 0,
+                 "visit_duration" => 900
                }
              ]
 
@@ -812,17 +812,17 @@ defmodule PlausibleWeb.Api.StatsController.SourcesTest do
         )
 
       assert json_response(conn, 200) == [
-               %{
-                 "name" => "Google",
-                 "visitors" => 1,
-                 "conversion_rate" => 50.0,
-                 "total_visitors" => 2
-               },
                %{
                  "name" => "DuckDuckGo",
                  "visitors" => 1,
                  "conversion_rate" => 100.0,
                  "total_visitors" => 1
+               },
+               %{
+                 "name" => "Google",
+                 "visitors" => 1,
+                 "conversion_rate" => 50.0,
+                 "total_visitors" => 2
                }
              ]
     end