Refactor building the Query struct (#5893)

* rename Query.build -> Query.parse_and_build

* rename two test files and move 4 %Query{} building functions into subfolder

* rename StatsAPIFilterParser to LegacyStatsAPIFilterParser

* rename Filters.QueryParser to QueryParser

* turn QueryParserTest into QueryParseAndBuildTest

* move query_parser.ex out of filters directory

* separate build from parse

* disable sample_threshold in the new intermediate build function, for now

* remove now redundant test util functions

* remove unused import

* address todo from earlier

* credo

* Make module names in sync with paths in tests

---------

Co-authored-by: Adrian Gruntkowski <[email protected]>

Author: RobertJoonas

extra/lib/plausible/stats/consolidated_view.ex

@@ -56,7 +56,7 @@ defmodule Plausible.Stats.ConsolidatedView do
       |> DateTime.to_iso8601()
 
     stats_query =
-      Stats.Query.build!(view, :internal, %{
+      Stats.Query.parse_and_build!(view, :internal, %{
         "site_id" => view.domain,
         "metrics" => ["visitors", "visits", "pageviews", "views_per_visit"],
         "include" => %{"comparisons" => %{"mode" => "custom", "date_range" => [c_from, c_to]}},
@@ -91,7 +91,7 @@ defmodule Plausible.Stats.ConsolidatedView do
 
   defp query_24h_intervals(view, now) do
     graph_query =
-      Stats.Query.build!(
+      Stats.Query.parse_and_build!(
         view,
         :internal,
         %{

extra/lib/plausible_web/live/funnel_settings/form.ex

@@ -349,7 +349,7 @@ defmodule PlausibleWeb.Live.FunnelSettings.Form do
       )
 
     query =
-      Plausible.Stats.Query.build!(
+      Plausible.Stats.Query.parse_and_build!(
         site,
         :internal,
         %{

lib/plausible/segments/filters.ex

@@ -3,7 +3,7 @@ defmodule Plausible.Segments.Filters do
   This module contains functions that enable resolving segments in filters.
   """
   alias Plausible.Segments
-  alias Plausible.Stats.Filters
+  alias Plausible.Stats.{Filters, QueryParser}
 
   @max_segment_filters_count 10
 
@@ -48,7 +48,7 @@ defmodule Plausible.Segments.Filters do
               segments,
               %{},
               fn %Segments.Segment{id: id, segment_data: segment_data} ->
-                case Filters.QueryParser.parse_filters(segment_data["filters"]) do
+                case QueryParser.parse_filters(segment_data["filters"]) do
                   {:ok, filters} -> {id, filters}
                   _ -> {id, nil}
                 end

lib/plausible/segments/segment.ex

@@ -131,7 +131,7 @@ defmodule Plausible.Segments.Segment do
   """
   def build_naive_query_from_segment_data(%Plausible.Site{} = site, filters),
     do:
-      Plausible.Stats.Query.build(
+      Plausible.Stats.Query.parse_and_build(
         site,
         :internal,
         %{

lib/plausible/stats/filters/filters.ex

@@ -4,8 +4,8 @@ defmodule Plausible.Stats.Filters do
   """
 
   alias Plausible.Stats.Query
-  alias Plausible.Stats.Filters.QueryParser
-  alias Plausible.Stats.Filters.StatsAPIFilterParser
+  alias Plausible.Stats.QueryParser
+  alias Plausible.Stats.Filters.LegacyStatsAPIFilterParser
 
   @visit_props [
     :source,
@@ -70,7 +70,7 @@ defmodule Plausible.Stats.Filters do
     case Jason.decode(filters) do
       {:ok, filters} when is_list(filters) -> parse(filters)
       {:ok, _} -> []
-      {:error, err} -> StatsAPIFilterParser.parse_filter_expression(err.data)
+      {:error, err} -> LegacyStatsAPIFilterParser.parse_filter_expression(err.data)
     end
   end
 

…stats/filters/stats_api_filter_parser.ex …ilters/legacy_stats_api_filter_parser.exlib/plausible/stats/filters/stats_api_filter_parser.ex renamed to lib/plausible/stats/filters/legacy_stats_api_filter_parser.ex lib/plausible/stats/filters/stats_api_filter_parser.ex renamed to lib/plausible/stats/filters/legacy_stats_api_filter_parser.ex

@@ -1,5 +1,7 @@
-defmodule Plausible.Stats.Filters.StatsAPIFilterParser do
-  @moduledoc false
+defmodule Plausible.Stats.Filters.LegacyStatsAPIFilterParser do
+  @moduledoc """
+  Parser for legacy filter format used in Stats API v1.
+  """
 
   @non_escaped_pipe_regex ~r/(?<!\\)\|/
 

lib/plausible/stats/goal_suggestions.ex

@@ -41,7 +41,7 @@ defmodule Plausible.Stats.GoalSuggestions do
     from_date = Date.shift(to_date, month: -6)
 
     query =
-      Plausible.Stats.Query.build!(
+      Plausible.Stats.Query.parse_and_build!(
         site,
         :internal,
         %{

lib/plausible/stats/legacy/legacy_query_builder.ex

@@ -8,7 +8,7 @@ defmodule Plausible.Stats.Legacy.QueryBuilder do
 
   use Plausible
 
-  alias Plausible.Stats.{Filters, Interval, Query, DateTimeRange}
+  alias Plausible.Stats.{Filters, Interval, Query, QueryParser, QueryBuilder, DateTimeRange}
 
   def from(site, params, debug_metadata, now \\ nil) do
     now = now || Plausible.Stats.Query.Test.get_fixed_now()
@@ -31,9 +31,9 @@ defmodule Plausible.Stats.Legacy.QueryBuilder do
       |> put_consolidated_site_ids(site)
       |> put_order_by(params)
       |> put_include(site, params)
-      |> Query.put_comparison_utc_time_range()
+      |> QueryBuilder.put_comparison_utc_time_range()
       |> Query.put_imported_opts(site)
-      |> Query.set_time_on_page_data(site)
+      |> QueryBuilder.set_time_on_page_data(site)
 
     on_ee do
       query = Plausible.Stats.Sampling.put_threshold(query, site, params)
@@ -68,7 +68,7 @@ defmodule Plausible.Stats.Legacy.QueryBuilder do
 
   defp preload_goals_and_revenue(query, site) do
     {preloaded_goals, revenue_warning, revenue_currencies} =
-      Plausible.Stats.Filters.QueryParser.preload_goals_and_revenue(
+      Plausible.Stats.QueryBuilder.preload_goals_and_revenue(
         site,
         query.metrics,
         query.filters,
@@ -269,36 +269,37 @@ defmodule Plausible.Stats.Legacy.QueryBuilder do
     [{:visitors, :asc}, {"visit:source", :desc}]
   """
   def parse_order_by(order_by) do
-    json_decode(order_by)
-    |> unwrap([])
-    |> Filters.QueryParser.parse_order_by()
-    |> unwrap([])
+    with true <- is_binary(order_by),
+         {:ok, order_by} <- JSON.decode(order_by),
+         {:ok, order_by} <- QueryParser.parse_order_by(order_by) do
+      order_by
+    else
+      _ -> []
+    end
   end
 
   @doc """
   ### Examples:
     iex> QueryBuilder.parse_include(%{}, nil)
-    QueryParser.default_include()
+    Plausible.Stats.ParsedQueryParams.default_include()
 
     iex> QueryBuilder.parse_include(%{}, ~s({"total_rows": true}))
-    Map.merge(QueryParser.default_include(), %{total_rows: true})
+    Map.merge(Plausible.Stats.ParsedQueryParams.default_include(), %{total_rows: true})
   """
   def parse_include(site, include) do
-    json_decode(include)
-    |> unwrap(%{})
-    |> Filters.QueryParser.parse_include(site)
-    |> unwrap(Filters.QueryParser.default_include())
-  end
+    include =
+      with true <- is_binary(include),
+           {:ok, include} <- JSON.decode(include),
+           {:ok, include} <- QueryParser.parse_include(include, site) do
+        include
+      else
+        _ -> %{}
+      end
 
-  defp json_decode(string) when is_binary(string) do
-    Jason.decode(string)
+    Plausible.Stats.ParsedQueryParams.default_include()
+    |> Map.merge(include)
   end
 
-  defp json_decode(_other), do: :error
-
-  defp unwrap({:ok, result}, _default), do: result
-  defp unwrap(_, default), do: default
-
   defp put_order_by(query, %{} = params) do
     struct!(query, order_by: parse_order_by(params["order_by"]))
   end
@@ -342,7 +343,7 @@ defmodule Plausible.Stats.Legacy.QueryBuilder do
 
   def parse_comparison_params(site, %{"comparison" => "custom"} = params) do
     {:ok, date_range} =
-      Filters.QueryParser.parse_date_range_pair(site, [
+      QueryParser.parse_date_range_pair(site, [
         params["compare_from"],
         params["compare_to"]
       ])

lib/plausible/stats/parsed_query_params.ex

@@ -0,0 +1,55 @@
+defmodule Plausible.Stats.ParsedQueryParams do
+  @moduledoc false
+
+  defstruct [
+    :now,
+    :utc_time_range,
+    :metrics,
+    :filters,
+    :dimensions,
+    :order_by,
+    :pagination,
+    :include
+  ]
+
+  alias Plausible.Stats.DateTimeRange
+
+  @default_include %{
+    imports: false,
+    # `include.imports_meta` can be true even when `include.imports`
+    # is false. Even if we don't want to include imported data, we
+    # might still want to know whether imported data can be toggled
+    # on/off on the dashboard.
+    imports_meta: false,
+    time_labels: false,
+    total_rows: false,
+    trim_relative_date_range: false,
+    comparisons: nil,
+    legacy_time_on_page_cutoff: nil
+  }
+
+  def default_include(), do: @default_include
+
+  @default_pagination %{
+    limit: 10_000,
+    offset: 0
+  }
+
+  def default_pagination(), do: @default_pagination
+
+  def new!(params) when is_map(params) do
+    %DateTimeRange{} = utc_time_range = Map.fetch!(params, :utc_time_range)
+    [_ | _] = metrics = Map.fetch!(params, :metrics)
+
+    %__MODULE__{
+      now: params[:now],
+      utc_time_range: utc_time_range,
+      metrics: metrics,
+      filters: params[:filters] || [],
+      dimensions: params[:dimensions] || [],
+      order_by: params[:order_by],
+      pagination: Map.merge(@default_pagination, params[:pagination] || %{}),
+      include: Map.merge(@default_include, params[:include] || %{})
+    }
+  end
+end

lib/plausible/stats/query.ex

@@ -18,7 +18,7 @@ defmodule Plausible.Stats.Query do
             timezone: nil,
             legacy_breakdown: false,
             preloaded_goals: [],
-            include: Plausible.Stats.Filters.QueryParser.default_include(),
+            include: Plausible.Stats.ParsedQueryParams.default_include(),
             debug_metadata: %{},
             pagination: nil,
             # Revenue metric specific metadata
@@ -34,45 +34,40 @@ defmodule Plausible.Stats.Query do
             smear_session_metrics: false
 
   require OpenTelemetry.Tracer, as: Tracer
-  alias Plausible.Stats.{DateTimeRange, Filters, Imported, Legacy, Comparisons}
+
+  alias Plausible.Stats.{
+    DateTimeRange,
+    Imported,
+    Legacy,
+    Comparisons,
+    QueryParser,
+    ParsedQueryParams,
+    QueryBuilder
+  }
 
   @type t :: %__MODULE__{}
 
-  def build(
+  def parse_and_build(
         %Plausible.Site{domain: domain} = site,
         schema_type,
         %{"site_id" => domain} = params,
         debug_metadata \\ %{}
       ) do
-    with {:ok, query_data} <- Filters.QueryParser.parse(site, schema_type, params) do
-      query =
-        %__MODULE__{
-          debug_metadata: debug_metadata,
-          site_id: site.id,
-          site_native_stats_start_at: site.native_stats_start_at
-        }
-        |> struct!(Map.to_list(query_data))
-        |> set_time_on_page_data(site)
-        |> put_comparison_utc_time_range()
-        |> put_imported_opts(site)
-
-      on_ee do
-        query = Plausible.Stats.Sampling.put_threshold(query, site, params)
-      end
-
-      {:ok, query}
+    with {:ok, %ParsedQueryParams{} = parsed_query_params} <-
+           QueryParser.parse(site, schema_type, params) do
+      QueryBuilder.build(site, parsed_query_params, params, debug_metadata)
     end
   end
 
-  def build!(site, schema_type, params, debug_metadata \\ %{}) do
-    case build(site, schema_type, params, debug_metadata) do
+  def parse_and_build!(site, schema_type, params, debug_metadata \\ %{}) do
+    case parse_and_build(site, schema_type, params, debug_metadata) do
       {:ok, query} -> query
       {:error, reason} -> raise "Failed to build query: #{inspect(reason)}"
     end
   end
 
   @doc """
-  Builds query from old-style stats APIv1 params. New code should use `Query.build`.
+  Builds query from old-style stats APIv1 params. New code should use `Query.parse_and_build`.
   """
   def from(site, params, debug_metadata \\ %{}, now \\ nil) do
     Legacy.QueryBuilder.from(site, params, debug_metadata, now)
@@ -143,13 +138,6 @@ defmodule Plausible.Stats.Query do
     put_imported_opts(query, nil)
   end
 
-  def put_comparison_utc_time_range(%__MODULE__{include: %{comparisons: nil}} = query), do: query
-
-  def put_comparison_utc_time_range(%__MODULE__{include: %{comparisons: comparison_opts}} = query) do
-    datetime_range = Comparisons.get_comparison_utc_time_range(query, comparison_opts)
-    struct!(query, comparison_utc_time_range: datetime_range)
-  end
-
   def put_imported_opts(query, site) do
     requested? = query.include.imports
 
@@ -190,15 +178,6 @@ defmodule Plausible.Stats.Query do
     in_comparison_range ++ in_range
   end
 
-  def set_time_on_page_data(query, site) do
-    struct!(query,
-      time_on_page_data: %{
-        new_metric_visible: Plausible.Stats.TimeOnPage.new_time_on_page_visible?(site),
-        cutoff_date: site.legacy_time_on_page_cutoff
-      }
-    )
-  end
-
   @spec get_skip_imported_reason(t()) ::
           nil | :no_imported_data | :out_of_range | :unsupported_query
   def get_skip_imported_reason(query) do