diff --git a/lib/plausible/stats/sql/expression.ex b/lib/plausible/stats/sql/expression.ex index 34a0c2dec..e5e8fb447 100644 --- a/lib/plausible/stats/sql/expression.ex +++ b/lib/plausible/stats/sql/expression.ex @@ -252,7 +252,10 @@ defmodule Plausible.Stats.SQL.Expression do wrap_alias([], %{ bounce_rate: fragment( - "toUInt32(ifNotFinite(round(sumIf(is_bounce * sign, ?) / sumIf(sign, ?) * 100), 0))", + # :TRICKY: Before PR #4493, we could have sessions where `sum(is_bounce * sign)` + # is negative, leading to an underflow and >100% bounce rate. This works around + # that issue. + "toUInt32(greatest(ifNotFinite(round(sumIf(is_bounce * sign, ?) / sumIf(sign, ?) * 100), 0), 0))", ^condition, ^condition ), diff --git a/lib/plausible/stats/sql/fragments.ex b/lib/plausible/stats/sql/fragments.ex index 5e0b0f6d1..7092db19b 100644 --- a/lib/plausible/stats/sql/fragments.ex +++ b/lib/plausible/stats/sql/fragments.ex @@ -30,7 +30,12 @@ defmodule Plausible.Stats.SQL.Fragments do defmacro bounce_rate() do quote do - fragment("toUInt32(ifNotFinite(round(sum(is_bounce * sign) / sum(sign) * 100), 0))") + fragment( + # :TRICKY: Before PR #4493, we could have sessions where `sum(is_bounce * sign)` + # is negative, leading to an underflow and >100% bounce rate. This works around + # that issue. + "toUInt32(greatest(ifNotFinite(round(sum(is_bounce * sign) / sum(sign) * 100), 0), 0))" + ) end end diff --git a/test/plausible_web/controllers/api/external_stats_controller/query_test.exs b/test/plausible_web/controllers/api/external_stats_controller/query_test.exs index db78c9bcf..7d16b4f76 100644 --- a/test/plausible_web/controllers/api/external_stats_controller/query_test.exs +++ b/test/plausible_web/controllers/api/external_stats_controller/query_test.exs @@ -3159,6 +3159,66 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do ] end + test "bounce rate calculation handles invalid session data gracefully", %{ + conn: conn, + site: site + } do + # NOTE: At the time of this test is added, it appears it does _not_ + # catch the regression on MacOS (ARM), regardless if Clickhouse is run + # natively or from a docker container. The test still does catch + # that regression when ran on Linux for instance (including CI). + user_id = System.unique_integer([:positive]) + + populate_stats(site, [ + build(:pageview, + user_id: user_id, + pathname: "/", + timestamp: ~N[2021-01-01 00:00:00] + ) + ]) + + first_session = Plausible.Cache.Adapter.get(:sessions, {site.id, user_id}) + + populate_stats(site, [ + build(:pageview, + user_id: user_id, + pathname: "/", + timestamp: ~N[2021-01-01 00:01:00] + ) + ]) + + Plausible.Cache.Adapter.put(:sessions, {site.id, user_id}, first_session) + + populate_stats(site, [ + build(:pageview, + user_id: user_id, + pathname: "/", + timestamp: ~N[2021-01-01 00:01:00] + ) + ]) + + conn = + get(conn, "/api/v1/stats/breakdown", %{ + "site_id" => site.domain, + "period" => "day", + "date" => "2021-01-01", + "property" => "event:page", + "metrics" => "bounce_rate" + }) + + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "date_range" => "all", + "metrics" => ["bounce_rate"], + "dimensions" => ["event:page"] + }) + + assert json_response(conn, 200)["results"] == [ + %{"dimensions" => ["/"], "metrics" => [0]} + ] + end + describe "using the returned query object in a new POST request" do test "yields the same results for a simple aggregate query", %{conn: conn, site: site} do Plausible.Site.changeset(site, %{timezone: "Europe/Tallinn"})