From a6390d7e3ad8087ca344070e58b0299ceb4abd6c Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 15 Jan 2025 14:37:43 +0200 Subject: [PATCH 01/13] Refactor segments model --- lib/plausible/{ => segments}/segment.ex | 2 +- lib/plausible/segments/segments.ex | 234 +++++++++++++ .../api/internal/segments_controller.ex | 313 +++++------------- .../plugs/feature_flag_check_plug.ex | 5 +- lib/plausible_web/router.ex | 10 +- .../segment_test.exs} | 24 +- .../segments_controller_test.exs | 9 +- test/support/factory.ex | 2 +- 8 files changed, 339 insertions(+), 260 deletions(-) rename lib/plausible/{ => segments}/segment.ex (99%) create mode 100644 lib/plausible/segments/segments.ex rename test/plausible/{segment_schema_test.exs => segments/segment_test.exs} (85%) diff --git a/lib/plausible/segment.ex b/lib/plausible/segments/segment.ex similarity index 99% rename from lib/plausible/segment.ex rename to lib/plausible/segments/segment.ex index d85ba0a6f5fa..26846bfb46b5 100644 --- a/lib/plausible/segment.ex +++ b/lib/plausible/segments/segment.ex @@ -1,4 +1,4 @@ -defmodule Plausible.Segment do +defmodule Plausible.Segments.Segment do @moduledoc """ Schema for segments. Segments are saved filter combinations. """ diff --git a/lib/plausible/segments/segments.ex b/lib/plausible/segments/segments.ex new file mode 100644 index 000000000000..9753219de5da --- /dev/null +++ b/lib/plausible/segments/segments.ex @@ -0,0 +1,234 @@ +defmodule Plausible.Segments do + @moduledoc """ + Module for accessing Segments. + """ + alias __MODULE__ + use Plausible.Repo + + @type error_not_enough_permissions() :: {:error, :not_enough_permissions} + @type error_segment_not_found() :: {:error, :segment_not_found} + + @spec index(pos_integer() | nil, Plausible.Site.t(), atom()) :: + {:ok, [Segments.Segment.t()]} | error_not_enough_permissions() + def index(user_id, %Plausible.Site{} = site, site_role) do + fields_in_index_query = [ + :id, + :name, + :type, + :inserted_at, + :updated_at, + :owner_id + ] + + site_segments_available? = + site_segments_available?(site) + + cond do + site_role in [:public] and + site_segments_available? -> + {:ok, + Repo.all(get_site_segments_only_query(site.id, fields_in_index_query -- [:owner_id]))} + + site_role in Segments.roles_with_maybe_site_segments() and + site_segments_available? -> + {:ok, + Repo.all(get_personal_and_site_segments_query(user_id, site.id, fields_in_index_query))} + + site_role in Segments.roles_with_personal_segments() -> + {:ok, Repo.all(get_personal_segments_only_query(user_id, site.id, fields_in_index_query))} + + true -> + {:error, :not_enough_permissions} + end + end + + @spec get_one(pos_integer(), Plausible.Site.t(), atom(), pos_integer() | nil) :: + {:ok, Segments.Segment.t()} + | error_not_enough_permissions() + | error_segment_not_found() + def get_one(user_id, site, site_role, segment_id) do + if site_role in roles_with_personal_segments() do + case do_get_one(user_id, site.id, segment_id) do + %Segments.Segment{} = segment -> {:ok, segment} + nil -> {:error, :segment_not_found} + end + else + {:error, :not_enough_permissions} + end + end + + def insert_one( + user_id, + %Plausible.Site{} = site, + site_role, + %{} = params + ) do + with :ok <- can_insert_one?(site, site_role, params), + %{valid?: true} = changeset <- + Plausible.Segments.Segment.changeset( + %Plausible.Segments.Segment{}, + Map.merge(params, %{"site_id" => site.id, "owner_id" => user_id}) + ), + :ok <- + Plausible.Segments.Segment.validate_segment_data(site, params["segment_data"], true) do + {:ok, Repo.insert!(changeset)} + else + %{valid?: false, errors: errors} -> + {:error, {:invalid_segment, errors}} + + {:error, {:invalid_filters, message}} -> + {:error, {:invalid_segment, segment_data: {message, []}}} + + {:error, _type} = error -> + error + end + end + + def update_one( + user_id, + %Plausible.Site{} = site, + site_role, + segment_id, + %{} = params + ) do + with {:ok, segment} <- get_one(user_id, site, site_role, segment_id), + :ok <- can_update_one?(site, site_role, params, segment.type), + %{valid?: true} = changeset <- + Segments.Segment.changeset( + segment, + Map.merge(params, %{"owner_id" => user_id}) + ), + :ok <- + Segments.Segment.validate_segment_data_if_exists( + site, + params["segment_data"], + true + ) do + {:ok, + Repo.update!( + changeset, + returning: true + )} + else + %{valid?: false, errors: errors} -> + {:error, {:invalid_segment, errors}} + + {:error, {:invalid_filters, message}} -> + {:error, {:invalid_segment, segment_data: {message, []}}} + + {:error, _type} = error -> + error + end + end + + def delete_one(user_id, %Plausible.Site{} = site, site_role, segment_id) do + with {:ok, segment} <- get_one(user_id, site, site_role, segment_id) do + cond do + segment.type == :site and site_role in roles_with_maybe_site_segments() -> + {:ok, do_delete_one(segment)} + + segment.type == :personal and site_role in roles_with_personal_segments() -> + {:ok, do_delete_one(segment)} + + true -> + {:error, :not_enough_permissions} + end + end + end + + @spec do_get_one(pos_integer(), pos_integer(), pos_integer() | nil) :: + Segments.Segment.t() | nil + defp do_get_one(user_id, site_id, segment_id) + + defp do_get_one(_user_id, _site_id, nil) do + nil + end + + defp do_get_one(user_id, site_id, segment_id) do + query = + from(segment in Plausible.Segments.Segment, + where: segment.site_id == ^site_id, + where: segment.id == ^segment_id, + where: segment.type == :site or segment.owner_id == ^user_id + ) + + Repo.one(query) + end + + defp do_delete_one(segment) do + Repo.delete!(segment) + segment + end + + defp can_update_one?(%Plausible.Site{} = site, site_role, params, existing_segment_type) do + updating_to_site_segment? = params["type"] == "site" + + cond do + (existing_segment_type == :site or + updating_to_site_segment?) and site_role in roles_with_maybe_site_segments() and + site_segments_available?(site) -> + :ok + + existing_segment_type == :personal and not updating_to_site_segment? and + site_role in roles_with_personal_segments() -> + :ok + + true -> + {:error, :not_enough_permissions} + end + end + + defp can_insert_one?(%Plausible.Site{} = site, site_role, params) do + cond do + params["type"] == "site" and site_role in roles_with_maybe_site_segments() and + site_segments_available?(site) -> + :ok + + params["type"] == "personal" and + site_role in roles_with_personal_segments() -> + :ok + + true -> + {:error, :not_enough_permissions} + end + end + + def roles_with_personal_segments(), do: [:viewer, :editor, :admin, :owner, :super_admin] + def roles_with_maybe_site_segments(), do: [:editor, :admin, :owner, :super_admin] + + def site_segments_available?(%Plausible.Site{} = site), + do: Plausible.Billing.Feature.Props.check_availability(site.team) == :ok + + @spec get_site_segments_only_query(pos_integer(), list(atom())) :: Ecto.Query.t() + defp get_site_segments_only_query(site_id, fields) do + from(segment in Plausible.Segments.Segment, + select: ^fields, + where: segment.site_id == ^site_id, + where: segment.type == :site, + order_by: [desc: segment.updated_at, desc: segment.id] + ) + end + + @spec get_personal_segments_only_query(pos_integer(), pos_integer(), list(atom())) :: + Ecto.Query.t() + defp get_personal_segments_only_query(user_id, site_id, fields) do + from(segment in Plausible.Segments.Segment, + select: ^fields, + where: segment.site_id == ^site_id, + where: segment.type == :personal and segment.owner_id == ^user_id, + order_by: [desc: segment.updated_at, desc: segment.id] + ) + end + + @spec get_personal_and_site_segments_query(pos_integer(), pos_integer(), list(atom())) :: + Ecto.Query.t() + defp get_personal_and_site_segments_query(user_id, site_id, fields) do + from(segment in Plausible.Segments.Segment, + select: ^fields, + where: segment.site_id == ^site_id, + where: + segment.type == :site or (segment.type == :personal and segment.owner_id == ^user_id), + order_by: [desc: segment.updated_at, desc: segment.id] + ) + end +end diff --git a/lib/plausible_web/controllers/api/internal/segments_controller.ex b/lib/plausible_web/controllers/api/internal/segments_controller.ex index bcc65b972c5f..fd75d2b2b9bf 100644 --- a/lib/plausible_web/controllers/api/internal/segments_controller.ex +++ b/lib/plausible_web/controllers/api/internal/segments_controller.ex @@ -1,71 +1,37 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do + @moduledoc """ + Internal API controller for segments. + """ use Plausible use PlausibleWeb, :controller - use Plausible.Repo use PlausibleWeb.Plugs.ErrorHandler alias PlausibleWeb.Api.Helpers, as: H + alias Plausible.Segments - @fields_in_index_query [ - :id, - :name, - :type, - :inserted_at, - :updated_at, - :owner_id - ] - - def get_all_segments( + def index( %Plug.Conn{ assigns: %{ site: site, - site_role: site_role, - current_user: current_user + site_role: site_role } } = conn, %{} = _params ) do - site_segments_available? = - site_segments_available?(site) - - cond do - site_role in roles_with_personal_segments() and - site_segments_available? -> - result = - Repo.all( - get_personal_and_site_segments_query(current_user.id, site.id, @fields_in_index_query) - ) - - json(conn, result) - - site_role in roles_with_personal_segments() -> - result = - Repo.all( - get_personal_segments_only_query(current_user.id, site.id, @fields_in_index_query) - ) + user_id = normalize_current_user_id(conn) - json(conn, result) - - site_role in [:public] and site_segments_available? -> - result = - Repo.all( - get_site_segments_only_query( - site.id, - @fields_in_index_query -- [:owner_id] - ) - ) - - json(conn, result) - - true -> + case Segments.index(user_id, site, site_role) do + {:error, :not_enough_permissions} -> H.not_enough_permissions(conn, "Not enough permissions to get segments") + + {:ok, segments} -> + json(conn, segments) end end - def get_segment( + def get( %Plug.Conn{ assigns: %{ site: site, - current_user: current_user, site_role: site_role } } = conn, @@ -73,46 +39,58 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do ) do segment_id = normalize_segment_id_param(params["segment_id"]) - if site_role in roles_with_personal_segments() do - result = get_one_segment(current_user.id, site.id, segment_id) + user_id = normalize_current_user_id(conn) - case result do - nil -> H.not_found(conn, "Segment not found with ID #{inspect(params["segment_id"])}") - result when is_map(result) -> json(conn, result) - end - else - H.not_enough_permissions(conn, "Not enough permissions to get segment data") + case Segments.get_one( + user_id, + site, + site_role, + segment_id + ) do + {:error, :not_enough_permissions} -> + H.not_enough_permissions(conn, "Not enough permissions to get segment data") + + {:error, :segment_not_found} -> + segment_not_found(conn, params["segment_id"]) + + {:ok, segment} -> + json(conn, segment) end end - def create_segment( + def create( %Plug.Conn{ assigns: %{ site: site, + current_user: %{id: user_id}, site_role: site_role } } = conn, %{} = params ) do - site_segments_available? = site_segments_available?(site) + case Segments.insert_one(user_id, site, site_role, params) do + {:error, :not_enough_permissions} -> + H.not_enough_permissions(conn, "Not enough permissions to create segment") - cond do - params["type"] == Atom.to_string(:personal) and - site_role in roles_with_personal_segments() -> - do_insert_segment(conn, params) + {:error, :segment_not_found} -> + segment_not_found(conn, params["segment_id"]) - params["type"] == Atom.to_string(:site) and site_segments_available? and - site_role in roles_with_maybe_site_segments() -> - do_insert_segment(conn, params) + {:error, {:invalid_segment, errors}} when is_list(errors) -> + conn + |> put_status(400) + |> json(%{ + errors: + Enum.map(errors, fn {field_key, {message, opts}} -> [field_key, message, opts] end) + }) - true -> - H.not_enough_permissions(conn, "Not enough permissions to create segment") + {:ok, segment} -> + json(conn, segment) end end - def create_segment(%Plug.Conn{} = conn, _params), do: H.bad_request(conn, "Invalid request") + def create(%Plug.Conn{} = conn, _params), do: invalid_request(conn) - def update_segment( + def update( %Plug.Conn{ assigns: %{ site: site, @@ -123,36 +101,31 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do conn, %{} = params ) do - site_segments_available? = site_segments_available?(site) - segment_id = normalize_segment_id_param(params["segment_id"]) - existing_segment = get_one_segment(user_id, site.id, segment_id) - - cond do - is_nil(existing_segment) -> - H.not_found(conn, "Segment not found with ID #{inspect(params["segment_id"])}") - - existing_segment.type == :personal and params["type"] != "site" and - site_role in roles_with_personal_segments() -> - do_update_segment(conn, params, existing_segment, user_id) + case Segments.update_one(user_id, site, site_role, segment_id, params) do + {:error, :not_enough_permissions} -> + H.not_enough_permissions(conn, "Not enough permissions to edit segment") - existing_segment.type == :personal and params["type"] == "site" and site_segments_available? and - site_role in roles_with_maybe_site_segments() -> - do_update_segment(conn, params, existing_segment, user_id) + {:error, :segment_not_found} -> + segment_not_found(conn, params["segment_id"]) - existing_segment.type == :site and site_segments_available? and - site_role in roles_with_maybe_site_segments() -> - do_update_segment(conn, params, existing_segment, user_id) + {:error, {:invalid_segment, errors}} when is_list(errors) -> + conn + |> put_status(400) + |> json(%{ + errors: + Enum.map(errors, fn {field_key, {message, opts}} -> [field_key, message, opts] end) + }) - true -> - H.not_enough_permissions(conn, "Not enough permissions to edit segment") + {:ok, segment} -> + json(conn, segment) end end - def update_segment(%Plug.Conn{} = conn, _params), do: H.bad_request(conn, "Invalid request") + def update(%Plug.Conn{} = conn, _params), do: invalid_request(conn) - def delete_segment( + def delete( %Plug.Conn{ assigns: %{ site: site, @@ -163,64 +136,25 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do conn, %{} = params ) do - site_segments_available? = site_segments_available?(site) - segment_id = normalize_segment_id_param(params["segment_id"]) - existing_segment = get_one_segment(user_id, site.id, segment_id) - - cond do - is_nil(existing_segment) -> - H.not_found(conn, "Segment not found with ID #{inspect(params["segment_id"])}") - - existing_segment.type == :personal and - site_role in roles_with_personal_segments() -> - do_delete_segment(conn, existing_segment) - - existing_segment.type == :site and - site_segments_available? and - site_role in roles_with_maybe_site_segments() -> - do_delete_segment(conn, existing_segment) - - true -> + case Segments.delete_one(user_id, site, site_role, segment_id) do + {:error, :not_enough_permissions} -> H.not_enough_permissions(conn, "Not enough permissions to delete segment") - end - end - def delete_segment(%Plug.Conn{} = conn, _params), do: H.bad_request(conn, "Invalid request") + {:error, :segment_not_found} -> + segment_not_found(conn, params["segment_id"]) - @spec get_site_segments_only_query(pos_integer(), list(atom())) :: Ecto.Query.t() - defp get_site_segments_only_query(site_id, fields) do - from(segment in Plausible.Segment, - select: ^fields, - where: segment.site_id == ^site_id, - where: segment.type == :site, - order_by: [desc: segment.updated_at, desc: segment.id] - ) + {:ok, segment} -> + json(conn, segment) + end end - @spec get_personal_segments_only_query(pos_integer(), pos_integer(), list(atom())) :: - Ecto.Query.t() - defp get_personal_segments_only_query(user_id, site_id, fields) do - from(segment in Plausible.Segment, - select: ^fields, - where: segment.site_id == ^site_id, - where: segment.type == :personal and segment.owner_id == ^user_id, - order_by: [desc: segment.updated_at, desc: segment.id] - ) - end + def delete(%Plug.Conn{} = conn, _params), do: invalid_request(conn) - @spec get_personal_and_site_segments_query(pos_integer(), pos_integer(), list(atom())) :: - Ecto.Query.t() - defp get_personal_and_site_segments_query(user_id, site_id, fields) do - from(segment in Plausible.Segment, - select: ^fields, - where: segment.site_id == ^site_id, - where: - segment.type == :site or (segment.type == :personal and segment.owner_id == ^user_id), - order_by: [desc: segment.updated_at, desc: segment.id] - ) - end + @spec normalize_current_user_id(%Plug.Conn{}) :: nil | pos_integer() + defp normalize_current_user_id(conn), + do: if(is_nil(conn.assigns[:current_user]), do: nil, else: conn.assigns[:current_user].id) @spec normalize_segment_id_param(any()) :: nil | pos_integer() defp normalize_segment_id_param(input) do @@ -230,104 +164,11 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do end end - defp get_one_segment(_user_id, _site_id, nil) do - nil + defp segment_not_found(%Plug.Conn{} = conn, segment_id_param) do + H.not_found(conn, "Segment not found with ID #{inspect(segment_id_param)}") end - defp get_one_segment(user_id, site_id, segment_id) do - query = - from(segment in Plausible.Segment, - where: segment.site_id == ^site_id, - where: segment.id == ^segment_id, - where: segment.type == :site or segment.owner_id == ^user_id - ) - - Repo.one(query) + defp invalid_request(%Plug.Conn{} = conn) do + H.bad_request(conn, "Invalid request") end - - defp do_insert_segment( - %Plug.Conn{ - assigns: %{ - site: site, - current_user: %{id: user_id} - } - } = conn, - %{} = params - ) do - segment_definition = Map.merge(params, %{"site_id" => site.id, "owner_id" => user_id}) - - with %{valid?: true} = changeset <- - Plausible.Segment.changeset( - %Plausible.Segment{}, - segment_definition - ), - :ok <- Plausible.Segment.validate_segment_data(site, params["segment_data"], true) do - segment = Repo.insert!(changeset) - json(conn, segment) - else - %{valid?: false, errors: errors} -> - conn - |> put_status(400) - |> json(%{ - errors: Enum.map(errors, fn {field_key, {message, _}} -> [field_key, message] end) - }) - - {:error, {:invalid_filters, message}} -> - conn |> put_status(400) |> json(%{errors: [["segment_data", message]]}) - end - end - - defp do_update_segment( - %Plug.Conn{} = conn, - %{} = params, - %Plausible.Segment{} = existing_segment, - owner_override - ) do - partial_segment_definition = Map.merge(params, %{"owner_id" => owner_override}) - - with %{valid?: true} = changeset <- - Plausible.Segment.changeset( - existing_segment, - partial_segment_definition - ), - :ok <- - Plausible.Segment.validate_segment_data_if_exists( - conn.assigns.site, - params["segment_data"], - true - ) do - json( - conn, - Repo.update!( - changeset, - returning: true - ) - ) - else - %{valid?: false, errors: errors} -> - conn - |> put_status(400) - |> json(%{ - errors: - Enum.map(errors, fn {field_key, {message, opts}} -> [field_key, message, opts] end) - }) - - {:error, {:invalid_filters, message}} -> - conn |> put_status(400) |> json(%{errors: [["segment_data", message, []]]}) - end - end - - defp do_delete_segment( - %Plug.Conn{} = conn, - %Plausible.Segment{} = existing_segment - ) do - Repo.delete!(existing_segment) - json(conn, existing_segment) - end - - defp roles_with_personal_segments(), do: [:viewer, :editor, :admin, :owner, :super_admin] - defp roles_with_maybe_site_segments(), do: [:editor, :admin, :owner, :super_admin] - - defp site_segments_available?(site), - do: Plausible.Billing.Feature.Props.check_availability(site.team) == :ok end diff --git a/lib/plausible_web/plugs/feature_flag_check_plug.ex b/lib/plausible_web/plugs/feature_flag_check_plug.ex index 75698da0848c..9483a89c772a 100644 --- a/lib/plausible_web/plugs/feature_flag_check_plug.ex +++ b/lib/plausible_web/plugs/feature_flag_check_plug.ex @@ -12,13 +12,16 @@ defmodule PlausibleWeb.Plugs.FeatureFlagCheckPlug do do: raise(ArgumentError, "The first argument must be a non-empty list of feature flags") def call(%Plug.Conn{} = conn, flags) do - if validate(conn.assigns.current_user, conn.assigns.site, flags) do + if validate(conn.assigns[:current_user], conn.assigns.site, flags) do conn else PlausibleWeb.Api.Helpers.not_found(conn, "Not found") end end + defp validate(_current_user = nil, site, flags), + do: Enum.all?(flags, fn flag -> FunWithFlags.enabled?(flag, for: site) end) + defp validate(current_user, site, flags), do: Enum.all?(flags, fn flag -> diff --git a/lib/plausible_web/router.ex b/lib/plausible_web/router.ex index c457d422545e..9ac044d348a9 100644 --- a/lib/plausible_web/router.ex +++ b/lib/plausible_web/router.ex @@ -224,11 +224,11 @@ defmodule PlausibleWeb.Router do do: plug(PlausibleWeb.Plugs.FeatureFlagCheckPlug, [:saved_segments]) pipe_through :segments_endpoints - get "/", SegmentsController, :get_all_segments - post "/", SegmentsController, :create_segment - get "/:segment_id", SegmentsController, :get_segment - patch "/:segment_id", SegmentsController, :update_segment - delete "/:segment_id", SegmentsController, :delete_segment + get "/", SegmentsController, :index + post "/", SegmentsController, :create + get "/:segment_id", SegmentsController, :get + patch "/:segment_id", SegmentsController, :update + delete "/:segment_id", SegmentsController, :delete end end diff --git a/test/plausible/segment_schema_test.exs b/test/plausible/segments/segment_test.exs similarity index 85% rename from test/plausible/segment_schema_test.exs rename to test/plausible/segments/segment_test.exs index 104fbf160cfb..355aa635d616 100644 --- a/test/plausible/segment_schema_test.exs +++ b/test/plausible/segments/segment_test.exs @@ -1,9 +1,9 @@ -defmodule Plausible.SegmentSchemaTest do +defmodule Plausible.Segments.SegmentTest do use ExUnit.Case - doctest Plausible.Segment, import: true + doctest Plausible.Segments.Segment, import: true setup do - segment = %Plausible.Segment{ + segment = %Plausible.Segments.Segment{ name: "any name", type: :personal, segment_data: %{"filters" => ["is", "visit:page", ["/blog"]]}, @@ -15,7 +15,7 @@ defmodule Plausible.SegmentSchemaTest do end test "changeset has required fields" do - assert Plausible.Segment.changeset(%Plausible.Segment{}, %{}).errors == [ + assert Plausible.Segments.Segment.changeset(%Plausible.Segments.Segment{}, %{}).errors == [ segment_data: {"property \"filters\" must be an array with at least one member", []}, name: {"can't be blank", [validation: :required]}, segment_data: {"can't be blank", [validation: :required]}, @@ -27,7 +27,7 @@ defmodule Plausible.SegmentSchemaTest do test "changeset does not allow setting owner_id to nil (setting to nil happens with database triggers)", %{segment: valid_segment} do - assert Plausible.Segment.changeset( + assert Plausible.Segments.Segment.changeset( valid_segment, %{ owner_id: nil @@ -38,7 +38,7 @@ defmodule Plausible.SegmentSchemaTest do end test "changeset forbids too long name", %{segment: valid_segment} do - assert Plausible.Segment.changeset( + assert Plausible.Segments.Segment.changeset( valid_segment, %{ name: String.duplicate("a", 256) @@ -51,7 +51,7 @@ defmodule Plausible.SegmentSchemaTest do end test "changeset forbids too large segment_data", %{segment: valid_segment} do - assert Plausible.Segment.changeset( + assert Plausible.Segments.Segment.changeset( valid_segment, %{ segment_data: @@ -66,8 +66,8 @@ defmodule Plausible.SegmentSchemaTest do test "changeset allows setting nil owner_id to a user id (to be able to recover dangling site segments)", %{segment: valid_segment} do - assert Plausible.Segment.changeset( - %Plausible.Segment{ + assert Plausible.Segments.Segment.changeset( + %Plausible.Segments.Segment{ valid_segment | owner_id: nil }, @@ -78,7 +78,7 @@ defmodule Plausible.SegmentSchemaTest do end test "changeset requires segment_data to be structured as expected", %{segment: valid_segment} do - assert Plausible.Segment.changeset( + assert Plausible.Segments.Segment.changeset( valid_segment, %{ segment_data: %{"filters" => 1, "labels" => true, "other" => []} @@ -93,7 +93,7 @@ defmodule Plausible.SegmentSchemaTest do end test "changeset forbids empty filters list", %{segment: valid_segment} do - assert Plausible.Segment.changeset( + assert Plausible.Segments.Segment.changeset( valid_segment, %{ segment_data: %{ @@ -107,7 +107,7 @@ defmodule Plausible.SegmentSchemaTest do end test "changeset permits well-structured segment data", %{segment: valid_segment} do - assert Plausible.Segment.changeset( + assert Plausible.Segments.Segment.changeset( valid_segment, %{ segment_data: %{ diff --git a/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs b/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs index 5d355c13cd75..71cc6550d5b9 100644 --- a/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs +++ b/test/plausible_web/controllers/api/internal_controller/segments_controller_test.exs @@ -330,7 +330,8 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do "errors" => [ [ "segment_data", - "#/filters/0: Invalid filter [\"is\", \"entry_page\", [\"/blog\"]]" + "#/filters/0: Invalid filter [\"is\", \"entry_page\", [\"/blog\"]]", + [] ] ] } @@ -366,7 +367,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do assert is_binary(response["updated_at"]) assert response["inserted_at"] == response["updated_at"] - verify_segment_in_db(%Plausible.Segment{ + verify_segment_in_db(%Plausible.Segments.Segment{ id: response["id"], name: response["name"], type: String.to_existing_atom(response["type"]), @@ -569,11 +570,11 @@ defmodule PlausibleWeb.Api.Internal.SegmentsControllerTest do defp verify_segment_in_db(segment) do uncomparable_keys = [:__meta__, :site] - assert Map.drop(Repo.get(Plausible.Segment, segment.id), uncomparable_keys) == + assert Map.drop(Repo.get(Plausible.Segments.Segment, segment.id), uncomparable_keys) == Map.drop(segment, uncomparable_keys) end defp verify_no_segment_in_db(segment) do - assert Repo.get(Plausible.Segment, segment.id) == nil + assert Repo.get(Plausible.Segments.Segment, segment.id) == nil end end diff --git a/test/support/factory.ex b/test/support/factory.ex index b74ef7929565..0814ac0eb591 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -379,7 +379,7 @@ defmodule Plausible.Factory do end def segment_factory do - %Plausible.Segment{ + %Plausible.Segments.Segment{ segment_data: %{"filters" => [["is", "visit:entry_page", ["/blog"]]]} } end From ec5ae0a56408c8da615fcef5edb69f6dfc502096 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 16 Jan 2025 11:10:43 +0200 Subject: [PATCH 02/13] Fix inconsistent code --- .../controllers/api/internal/segments_controller.ex | 2 +- lib/plausible_web/plugs/feature_flag_check_plug.ex | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/plausible_web/controllers/api/internal/segments_controller.ex b/lib/plausible_web/controllers/api/internal/segments_controller.ex index fd75d2b2b9bf..886166d32f3a 100644 --- a/lib/plausible_web/controllers/api/internal/segments_controller.ex +++ b/lib/plausible_web/controllers/api/internal/segments_controller.ex @@ -152,7 +152,7 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do def delete(%Plug.Conn{} = conn, _params), do: invalid_request(conn) - @spec normalize_current_user_id(%Plug.Conn{}) :: nil | pos_integer() + @spec normalize_current_user_id(Plug.Conn.t()) :: nil | pos_integer() defp normalize_current_user_id(conn), do: if(is_nil(conn.assigns[:current_user]), do: nil, else: conn.assigns[:current_user].id) diff --git a/lib/plausible_web/plugs/feature_flag_check_plug.ex b/lib/plausible_web/plugs/feature_flag_check_plug.ex index 9483a89c772a..c04e23e38d33 100644 --- a/lib/plausible_web/plugs/feature_flag_check_plug.ex +++ b/lib/plausible_web/plugs/feature_flag_check_plug.ex @@ -19,7 +19,7 @@ defmodule PlausibleWeb.Plugs.FeatureFlagCheckPlug do end end - defp validate(_current_user = nil, site, flags), + defp validate(nil = _current_user, site, flags), do: Enum.all?(flags, fn flag -> FunWithFlags.enabled?(flag, for: site) end) defp validate(current_user, site, flags), From 8876a2ad9afbaef11565d1cb006e36ba7134b515 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 16 Jan 2025 11:15:10 +0200 Subject: [PATCH 03/13] Remove superfluous error case --- .../controllers/api/internal/segments_controller.ex | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/plausible_web/controllers/api/internal/segments_controller.ex b/lib/plausible_web/controllers/api/internal/segments_controller.ex index 886166d32f3a..a1df13bff790 100644 --- a/lib/plausible_web/controllers/api/internal/segments_controller.ex +++ b/lib/plausible_web/controllers/api/internal/segments_controller.ex @@ -72,9 +72,6 @@ defmodule PlausibleWeb.Api.Internal.SegmentsController do {:error, :not_enough_permissions} -> H.not_enough_permissions(conn, "Not enough permissions to create segment") - {:error, :segment_not_found} -> - segment_not_found(conn, params["segment_id"]) - {:error, {:invalid_segment, errors}} when is_list(errors) -> conn |> put_status(400) From 8676123f79fd1e9fe4f2b687a191216399922001 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Mon, 20 Jan 2025 10:45:55 +0200 Subject: [PATCH 04/13] Beautify Plausible.Segments module --- lib/plausible/segments/segments.ex | 105 ++++++++++++++--------------- 1 file changed, 51 insertions(+), 54 deletions(-) diff --git a/lib/plausible/segments/segments.ex b/lib/plausible/segments/segments.ex index 9753219de5da..6284bd6345ad 100644 --- a/lib/plausible/segments/segments.ex +++ b/lib/plausible/segments/segments.ex @@ -2,23 +2,20 @@ defmodule Plausible.Segments do @moduledoc """ Module for accessing Segments. """ - alias __MODULE__ - use Plausible.Repo + alias Plausible.Segments.Segment + alias Plausible.Repo + import Ecto.Query + + @roles_with_personal_segments [:viewer, :editor, :admin, :owner, :super_admin] + @roles_with_maybe_site_segments [:editor, :admin, :owner, :super_admin] @type error_not_enough_permissions() :: {:error, :not_enough_permissions} @type error_segment_not_found() :: {:error, :segment_not_found} @spec index(pos_integer() | nil, Plausible.Site.t(), atom()) :: - {:ok, [Segments.Segment.t()]} | error_not_enough_permissions() + {:ok, [Segment.t()]} | error_not_enough_permissions() def index(user_id, %Plausible.Site{} = site, site_role) do - fields_in_index_query = [ - :id, - :name, - :type, - :inserted_at, - :updated_at, - :owner_id - ] + fields = [:id, :name, :type, :inserted_at, :updated_at, :owner_id] site_segments_available? = site_segments_available?(site) @@ -26,16 +23,14 @@ defmodule Plausible.Segments do cond do site_role in [:public] and site_segments_available? -> - {:ok, - Repo.all(get_site_segments_only_query(site.id, fields_in_index_query -- [:owner_id]))} + {:ok, get_public_site_segments(site.id, fields -- [:owner_id])} - site_role in Segments.roles_with_maybe_site_segments() and + site_role in @roles_with_maybe_site_segments and site_segments_available? -> - {:ok, - Repo.all(get_personal_and_site_segments_query(user_id, site.id, fields_in_index_query))} + {:ok, get_segments(user_id, site.id, fields)} - site_role in Segments.roles_with_personal_segments() -> - {:ok, Repo.all(get_personal_segments_only_query(user_id, site.id, fields_in_index_query))} + site_role in @roles_with_personal_segments -> + {:ok, get_segments(user_id, site.id, fields, only: :personal)} true -> {:error, :not_enough_permissions} @@ -43,13 +38,13 @@ defmodule Plausible.Segments do end @spec get_one(pos_integer(), Plausible.Site.t(), atom(), pos_integer() | nil) :: - {:ok, Segments.Segment.t()} + {:ok, Segment.t()} | error_not_enough_permissions() | error_segment_not_found() def get_one(user_id, site, site_role, segment_id) do if site_role in roles_with_personal_segments() do case do_get_one(user_id, site.id, segment_id) do - %Segments.Segment{} = segment -> {:ok, segment} + %Segment{} = segment -> {:ok, segment} nil -> {:error, :segment_not_found} end else @@ -65,12 +60,12 @@ defmodule Plausible.Segments do ) do with :ok <- can_insert_one?(site, site_role, params), %{valid?: true} = changeset <- - Plausible.Segments.Segment.changeset( - %Plausible.Segments.Segment{}, + Segment.changeset( + %Segment{}, Map.merge(params, %{"site_id" => site.id, "owner_id" => user_id}) ), :ok <- - Plausible.Segments.Segment.validate_segment_data(site, params["segment_data"], true) do + Segment.validate_segment_data(site, params["segment_data"], true) do {:ok, Repo.insert!(changeset)} else %{valid?: false, errors: errors} -> @@ -94,12 +89,12 @@ defmodule Plausible.Segments do with {:ok, segment} <- get_one(user_id, site, site_role, segment_id), :ok <- can_update_one?(site, site_role, params, segment.type), %{valid?: true} = changeset <- - Segments.Segment.changeset( + Segment.changeset( segment, Map.merge(params, %{"owner_id" => user_id}) ), :ok <- - Segments.Segment.validate_segment_data_if_exists( + Segment.validate_segment_data_if_exists( site, params["segment_data"], true @@ -137,7 +132,7 @@ defmodule Plausible.Segments do end @spec do_get_one(pos_integer(), pos_integer(), pos_integer() | nil) :: - Segments.Segment.t() | nil + Segment.t() | nil defp do_get_one(user_id, site_id, segment_id) defp do_get_one(_user_id, _site_id, nil) do @@ -146,7 +141,7 @@ defmodule Plausible.Segments do defp do_get_one(user_id, site_id, segment_id) do query = - from(segment in Plausible.Segments.Segment, + from(segment in Segment, where: segment.site_id == ^site_id, where: segment.id == ^segment_id, where: segment.type == :site or segment.owner_id == ^user_id @@ -199,36 +194,38 @@ defmodule Plausible.Segments do def site_segments_available?(%Plausible.Site{} = site), do: Plausible.Billing.Feature.Props.check_availability(site.team) == :ok - @spec get_site_segments_only_query(pos_integer(), list(atom())) :: Ecto.Query.t() - defp get_site_segments_only_query(site_id, fields) do - from(segment in Plausible.Segments.Segment, - select: ^fields, - where: segment.site_id == ^site_id, - where: segment.type == :site, - order_by: [desc: segment.updated_at, desc: segment.id] + @spec get_public_site_segments(pos_integer(), list(atom())) :: [Segment.t()] + defp get_public_site_segments(site_id, fields) do + Repo.all( + from(segment in Segment, + select: ^fields, + where: segment.site_id == ^site_id, + where: segment.type == :site, + order_by: [desc: segment.updated_at, desc: segment.id] + ) ) end - @spec get_personal_segments_only_query(pos_integer(), pos_integer(), list(atom())) :: - Ecto.Query.t() - defp get_personal_segments_only_query(user_id, site_id, fields) do - from(segment in Plausible.Segments.Segment, - select: ^fields, - where: segment.site_id == ^site_id, - where: segment.type == :personal and segment.owner_id == ^user_id, - order_by: [desc: segment.updated_at, desc: segment.id] - ) - end + @spec get_segments(pos_integer(), pos_integer(), list(atom()), Keyword.t()) :: [Segment.t()] + defp get_segments(user_id, site_id, fields, opts \\ []) do + query = + from(segment in Segment, + select: ^fields, + where: segment.site_id == ^site_id, + order_by: [desc: segment.updated_at, desc: segment.id] + ) - @spec get_personal_and_site_segments_query(pos_integer(), pos_integer(), list(atom())) :: - Ecto.Query.t() - defp get_personal_and_site_segments_query(user_id, site_id, fields) do - from(segment in Plausible.Segments.Segment, - select: ^fields, - where: segment.site_id == ^site_id, - where: - segment.type == :site or (segment.type == :personal and segment.owner_id == ^user_id), - order_by: [desc: segment.updated_at, desc: segment.id] - ) + query = + if Keyword.get(opts, :only) == :personal do + where(query, [segment], segment.type == :personal and segment.owner_id == ^user_id) + else + where( + query, + [segment], + segment.type == :site or (segment.type == :personal and segment.owner_id == ^user_id) + ) + end + + Repo.all(query) end end From 39998c95d079df08aa470806dd956d93df0910a1 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Wed, 15 Jan 2025 16:34:12 +0200 Subject: [PATCH 05/13] Expand segments in filters --- lib/plausible/segments/segments.ex | 15 ++++ lib/plausible/stats/filters/filters.ex | 2 +- lib/plausible/stats/filters/query_parser.ex | 77 ++++++++++++++++++++- priv/json-schemas/query-api-schema.json | 31 +++++++-- 4 files changed, 117 insertions(+), 8 deletions(-) diff --git a/lib/plausible/segments/segments.ex b/lib/plausible/segments/segments.ex index 6284bd6345ad..37f420e6d604 100644 --- a/lib/plausible/segments/segments.ex +++ b/lib/plausible/segments/segments.ex @@ -37,6 +37,21 @@ defmodule Plausible.Segments do end end + @spec get_many(Plausible.Site.t(), list(pos_integer()), Keyword.t()) :: + {:ok, [Segment.t()]} + def get_many(%Plausible.Site{} = site, segment_ids, opts) when is_list(segment_ids) do + fields = Keyword.get(opts, :fields, [:id]) + + query = + from(segment in Segment, + select: ^fields, + where: segment.site_id == ^site.id, + where: segment.id in ^segment_ids + ) + + {:ok, Repo.all(query)} + end + @spec get_one(pos_integer(), Plausible.Site.t(), atom(), pos_integer() | nil) :: {:ok, Segment.t()} | error_not_enough_permissions() diff --git a/lib/plausible/stats/filters/filters.ex b/lib/plausible/stats/filters/filters.ex index 9f4116f2c8a0..679def1a78de 100644 --- a/lib/plausible/stats/filters/filters.ex +++ b/lib/plausible/stats/filters/filters.ex @@ -154,7 +154,7 @@ defmodule Plausible.Stats.Filters do end end - defp traverse(filters, depth \\ -1) do + def traverse(filters, depth \\ -1) do filters |> Enum.flat_map(&traverse_tree(&1, depth + 1)) end diff --git a/lib/plausible/stats/filters/query_parser.ex b/lib/plausible/stats/filters/query_parser.ex index 048e61f98845..54667520b54e 100644 --- a/lib/plausible/stats/filters/query_parser.ex +++ b/lib/plausible/stats/filters/query_parser.ex @@ -34,6 +34,9 @@ defmodule Plausible.Stats.Filters.QueryParser do utc_time_range = raw_time_range |> DateTimeRange.to_timezone("Etc/UTC"), {:ok, metrics} <- parse_metrics(Map.get(params, "metrics", [])), {:ok, filters} <- parse_filters(Map.get(params, "filters", [])), + {:ok, preloaded_segments} <- preload_needed_segments(site, filters), + {:ok, filters} <- + resolve_segments(filters, preloaded_segments), {:ok, dimensions} <- parse_dimensions(Map.get(params, "dimensions", [])), {:ok, order_by} <- parse_order_by(Map.get(params, "order_by")), {:ok, include} <- parse_include(site, Map.get(params, "include", %{})), @@ -161,7 +164,10 @@ defmodule Plausible.Stats.Filters.QueryParser do "Invalid visit:country filter, visit:country needs to be a valid 2-letter country code."} end - {_, true} -> + {"segment", _} when all_integers? -> + {:ok, list} + + {_, true} when filter_key !== "segment" -> {:ok, list} _ -> @@ -396,6 +402,9 @@ defmodule Plausible.Stats.Filters.QueryParser do {:error, error_message} end + "segment" -> + {:ok, filter_key} + _ -> {:error, error_message} end @@ -443,6 +452,72 @@ defmodule Plausible.Stats.Filters.QueryParser do } end + def get_segment_ids(filters) do + max_segment_filters_count = 10 + + ids = + filters + |> Filters.traverse() + |> Enum.filter(fn {[_, filter_key | _rest], _depth} -> filter_key === "segment" end) + |> Enum.map(fn {[_, _filter_key, clauses], _depth} -> clauses end) + |> Enum.concat() + + if length(ids) > max_segment_filters_count do + {:error, + "Invalid filters. You can only use up to #{max_segment_filters_count} segment filters in a query."} + else + {:ok, Enum.uniq(ids)} + end + end + + def preload_needed_segments(%Plausible.Site{} = site, filters) do + with {:ok, segment_ids} <- get_segment_ids(filters), + {:ok, segments} <- + Plausible.Segments.get_many( + site, + segment_ids, + fields: [:id, :segment_data] + ), + {:ok, segments_by_id} <- + {:ok, + Enum.into( + segments, + %{}, + fn %Plausible.Segments.Segment{id: id, segment_data: segment_data} -> + case parse_filters(segment_data["filters"]) do + {:ok, filters} -> {id, filters} + _ -> {id, nil} + end + end + )}, + :ok <- + if(Enum.any?(segment_ids, fn id -> is_nil(Map.get(segments_by_id, id)) end), + do: {:error, "Invalid filters. Some segments don't exist or aren't accessible."}, + else: :ok + ) do + {:ok, segments_by_id} + end + end + + defp replace_segment_with_filter_tree([_, "segment", clauses], preloaded_segments) do + [[:or, Enum.map(clauses, fn id -> [:and, Map.get(preloaded_segments, id)] end)]] + end + + defp replace_segment_with_filter_tree(_filter_tree, _preloaded_segments) do + nil + end + + def resolve_segments(original_filters, preloaded_segments) do + if Map.keys(preloaded_segments) !== [] do + {:ok, + Filters.transform_filters(original_filters, fn f -> + replace_segment_with_filter_tree(f, preloaded_segments) + end)} + else + {:ok, original_filters} + end + end + @only_toplevel ["event:goal", "event:hostname"] defp validate_toplevel_only_filter_dimension(query) do not_toplevel = Filters.dimensions_used_in_filters(query.filters, min_depth: 1) diff --git a/priv/json-schemas/query-api-schema.json b/priv/json-schemas/query-api-schema.json index df18503283fa..9558b5dffa1a 100644 --- a/priv/json-schemas/query-api-schema.json +++ b/priv/json-schemas/query-api-schema.json @@ -98,13 +98,11 @@ "minItems": 2, "maxItems": 2, "items": { - "type": "string", - "format": "date" + "type": "string", + "format": "date" }, "description": "If custom period. A list of two ISO8601 dates or timestamps to compare against.", - "examples": [ - ["2024-01-01", "2024-01-31"] - ] + "examples": [["2024-01-01", "2024-01-31"]] } }, "required": ["mode", "date_range"], @@ -439,11 +437,32 @@ } ] }, + "filter_for_segment": { + "type": "array", + "additionalItems": false, + "minItems": 3, + "maxItems": 3, + "items": [ + { + "const": "is" + }, + { + "const": "segment" + }, + { + "type": "array", + "items": { + "type": ["integer"] + } + } + ] + }, "filter_entry": { "oneOf": [ { "$ref": "#/definitions/filter_without_goals" }, { "$ref": "#/definitions/filter_with_goals" }, - { "$ref": "#/definitions/filter_with_pattern" } + { "$ref": "#/definitions/filter_with_pattern" }, + { "$ref": "#/definitions/filter_for_segment" } ] }, "filter_tree": { From 56df03f23f1cf8a871a84e24079ab7e44c8beca6 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 16 Jan 2025 10:50:33 +0200 Subject: [PATCH 06/13] Add tests --- lib/plausible/stats/filters/filters.ex | 12 +++++- lib/plausible/stats/filters/query_parser.ex | 42 +++++++++++++++++++-- test/plausible/stats/query_parser_test.exs | 3 +- 3 files changed, 52 insertions(+), 5 deletions(-) diff --git a/lib/plausible/stats/filters/filters.ex b/lib/plausible/stats/filters/filters.ex index 679def1a78de..422163500b12 100644 --- a/lib/plausible/stats/filters/filters.ex +++ b/lib/plausible/stats/filters/filters.ex @@ -57,7 +57,7 @@ defmodule Plausible.Stats.Filters do Returns an empty list when argument type is unexpected (e.g. `nil`). - ### Examples: + ## Examples: iex> Filters.parse("visit:browser!=Chrome") [[:is_not, "visit:browser", ["Chrome"]]] @@ -128,6 +128,16 @@ defmodule Plausible.Stats.Filters do Transformer will receive each node (filter, and/or/not subtree) of query and must return a list of nodes to replace it with or nil to ignore and look deeper. + + ## Examples + + iex> Filters.transform_filters([[:is, "visit:entry_page", ["/blog"]]], fn f -> [f] end) + [[:is, "visit:entry_page", ["/blog"]]] + + iex> Filters.transform_filters([[:is, "visit:entry_page", ["/blog"]]], fn _f -> nil end) + [[[:is, "visit:entry_page", ["/blog"]]]] + + """ def transform_filters(filters, transformer) do filters diff --git a/lib/plausible/stats/filters/query_parser.ex b/lib/plausible/stats/filters/query_parser.ex index 54667520b54e..670bb93bd25b 100644 --- a/lib/plausible/stats/filters/query_parser.ex +++ b/lib/plausible/stats/filters/query_parser.ex @@ -452,6 +452,18 @@ defmodule Plausible.Stats.Filters.QueryParser do } end + @doc """ + Finds unique segment IDs used in query filters. + + ## Examples + iex> get_segment_ids([[:not, [:is, "segment", [10, 20]]], [:contains, "visit:entry_page", ["blog"]]]) + + {:ok, [10, 20]} + + iex> get_segment_ids([[:and, [[:is, "segment", Enum.to_list(1..6)], [:is, "segment", Enum.to_list(1..6)]]]]) + + {:error, "Invalid filters. You can only use up to 10 segment filters in a query."} + """ def get_segment_ids(filters) do max_segment_filters_count = 10 @@ -500,13 +512,37 @@ defmodule Plausible.Stats.Filters.QueryParser do end defp replace_segment_with_filter_tree([_, "segment", clauses], preloaded_segments) do - [[:or, Enum.map(clauses, fn id -> [:and, Map.get(preloaded_segments, id)] end)]] + if length(clauses) === 1 do + [[:and, Map.get(preloaded_segments, Enum.at(clauses, 0))]] + else + [[:or, Enum.map(clauses, fn id -> [:and, Map.get(preloaded_segments, id)] end)]] + end end - defp replace_segment_with_filter_tree(_filter_tree, _preloaded_segments) do - nil + defp replace_segment_with_filter_tree(filter, _preloaded_segments) do + [filter] end + @doc """ + ## Examples + + iex> resolve_segments([[:is, "visit:entry_page", ["/home"]]], %{}) + {:ok, [[:is, "visit:entry_page", ["/home"]]]} + + iex> resolve_segments([[:is, "visit:entry_page", ["/home"]], [:is, "segment", [1]]], %{1 => [[:contains, "visit:entry_page", ["blog"]], [:is, "visit:country", ["PL"]]]}) + {:ok, [ + [:is, "visit:entry_page", ["/home"]], + [:and, [[:contains, "visit:entry_page", ["blog"]], [:is, "visit:country", ["PL"]]]] + ]} + + iex> resolve_segments([[:is, "segment", [1, 2]]], %{1 => [[:contains, "event:goal", ["Singup"]], [:is, "visit:country", ["PL"]]], 2 => [[:contains, "event:goal", ["Sauna"]], [:is, "visit:country", ["EE"]]]}) + {:ok, [ + [:or, [ + [:and, [[:contains, "event:goal", ["Singup"]], [:is, "visit:country", ["PL"]]]], + [:and, [[:contains, "event:goal", ["Sauna"]], [:is, "visit:country", ["EE"]]]]] + ] + ]} + """ def resolve_segments(original_filters, preloaded_segments) do if Map.keys(preloaded_segments) !== [] do {:ok, diff --git a/test/plausible/stats/query_parser_test.exs b/test/plausible/stats/query_parser_test.exs index ba7c8e8b818c..13681b692f4b 100644 --- a/test/plausible/stats/query_parser_test.exs +++ b/test/plausible/stats/query_parser_test.exs @@ -2,10 +2,11 @@ defmodule Plausible.Stats.Filters.QueryParserTest do use Plausible use Plausible.DataCase use Plausible.Teams.Test + import Plausible.Stats.Filters.QueryParser + doctest Plausible.Stats.Filters.QueryParser alias Plausible.Stats.DateTimeRange alias Plausible.Stats.Filters - import Plausible.Stats.Filters.QueryParser setup [:create_user, :create_site] From fb2fbafece377ff4d3c82126e569802d53e21236 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 16 Jan 2025 11:12:03 +0200 Subject: [PATCH 07/13] Generate types --- assets/js/types/query-api.d.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/assets/js/types/query-api.d.ts b/assets/js/types/query-api.d.ts index 2b77acf19216..51b52f505099 100644 --- a/assets/js/types/query-api.d.ts +++ b/assets/js/types/query-api.d.ts @@ -64,7 +64,7 @@ export type CustomPropertyFilterDimensions = string; export type GoalDimension = "event:goal"; export type TimeDimensions = "time" | "time:month" | "time:week" | "time:day" | "time:hour"; export type FilterTree = FilterEntry | FilterAndOr | FilterNot; -export type FilterEntry = FilterWithoutGoals | FilterWithGoals | FilterWithPattern; +export type FilterEntry = FilterWithoutGoals | FilterWithGoals | FilterWithPattern | FilterForSegment; /** * @minItems 3 * @maxItems 4 @@ -115,6 +115,11 @@ export type FilterWithPattern = [ * filter operation */ export type FilterOperationRegex = "matches" | "matches_not"; +/** + * @minItems 3 + * @maxItems 3 + */ +export type FilterForSegment = ["is", "segment", number[]]; /** * @minItems 2 * @maxItems 2 From 0027bccd6de4d074332b395c6517fc3775091821 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Thu, 16 Jan 2025 11:24:37 +0200 Subject: [PATCH 08/13] Remove extraneous newlines --- lib/plausible/stats/filters/query_parser.ex | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/plausible/stats/filters/query_parser.ex b/lib/plausible/stats/filters/query_parser.ex index 670bb93bd25b..df733613dea3 100644 --- a/lib/plausible/stats/filters/query_parser.ex +++ b/lib/plausible/stats/filters/query_parser.ex @@ -457,11 +457,9 @@ defmodule Plausible.Stats.Filters.QueryParser do ## Examples iex> get_segment_ids([[:not, [:is, "segment", [10, 20]]], [:contains, "visit:entry_page", ["blog"]]]) - {:ok, [10, 20]} iex> get_segment_ids([[:and, [[:is, "segment", Enum.to_list(1..6)], [:is, "segment", Enum.to_list(1..6)]]]]) - {:error, "Invalid filters. You can only use up to 10 segment filters in a query."} """ def get_segment_ids(filters) do From d25d08817ed68f5ab43133f940ead5c1acb0f3ad Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Mon, 20 Jan 2025 12:56:07 +0200 Subject: [PATCH 09/13] Move Segment filters logic away from QueryParser --- lib/plausible/segments/filters.ex | 105 ++++++++++++++ lib/plausible/stats/filters/filters.ex | 15 +- lib/plausible/stats/filters/query_parser.ex | 105 +------------- priv/json-schemas/query-api-schema.json | 5 +- test/plausible/segments/filters_test.exs | 5 + test/plausible/stats/query_parser_test.exs | 145 ++++++++++++++++++++ 6 files changed, 268 insertions(+), 112 deletions(-) create mode 100644 lib/plausible/segments/filters.ex create mode 100644 test/plausible/segments/filters_test.exs diff --git a/lib/plausible/segments/filters.ex b/lib/plausible/segments/filters.ex new file mode 100644 index 000000000000..2e057a9d5381 --- /dev/null +++ b/lib/plausible/segments/filters.ex @@ -0,0 +1,105 @@ +defmodule Plausible.Segments.Filters do + alias Plausible.Segments + alias Plausible.Stats.Filters + + @doc """ + Finds unique segment IDs used in query filters. + + ## Examples + iex> get_segment_ids([[:not, [:is, "segment", [10, 20]]], [:contains, "visit:entry_page", ["blog"]]]) + {:ok, [10, 20]} + + iex> get_segment_ids([[:and, [[:is, "segment", Enum.to_list(1..6)], [:is, "segment", Enum.to_list(1..6)]]]]) + {:error, "Invalid filters. You can only use up to 10 segment filters in a query."} + """ + def get_segment_ids(filters) do + max_segment_filters_count = 10 + + ids = + filters + |> Filters.traverse() + |> Enum.flat_map(fn + {[_operation, "segment", clauses], _depth} -> clauses + _ -> [] + end) + + if length(ids) > max_segment_filters_count do + {:error, + "Invalid filters. You can only use up to #{max_segment_filters_count} segment filters in a query."} + else + {:ok, Enum.uniq(ids)} + end + end + + def preload_needed_segments(%Plausible.Site{} = site, filters) do + with {:ok, segment_ids} <- get_segment_ids(filters), + {:ok, segments} <- + Segments.get_many( + site, + segment_ids, + fields: [:id, :segment_data] + ), + {:ok, segments_by_id} <- + {:ok, + Enum.into( + segments, + %{}, + fn %Segments.Segment{id: id, segment_data: segment_data} -> + case Filters.QueryParser.parse_filters(segment_data["filters"]) do + {:ok, filters} -> {id, filters} + _ -> {id, nil} + end + end + )}, + :ok <- + if(Enum.any?(segment_ids, fn id -> is_nil(Map.get(segments_by_id, id)) end), + do: {:error, "Invalid filters. Some segments don't exist or aren't accessible."}, + else: :ok + ) do + {:ok, segments_by_id} + end + end + + defp replace_segment_with_filter_tree([_, "segment", clauses], preloaded_segments) do + if length(clauses) === 1 do + [[:and, Map.get(preloaded_segments, Enum.at(clauses, 0))]] + else + [[:or, Enum.map(clauses, fn id -> [:and, Map.get(preloaded_segments, id)] end)]] + end + end + + defp replace_segment_with_filter_tree(_filter, _preloaded_segments) do + nil + end + + @doc """ + ## Examples + + iex> resolve_segments([[:is, "visit:entry_page", ["/home"]]], %{}) + {:ok, [[:is, "visit:entry_page", ["/home"]]]} + + iex> resolve_segments([[:is, "visit:entry_page", ["/home"]], [:is, "segment", [1]]], %{1 => [[:contains, "visit:entry_page", ["blog"]], [:is, "visit:country", ["PL"]]]}) + {:ok, [ + [:is, "visit:entry_page", ["/home"]], + [:and, [[:contains, "visit:entry_page", ["blog"]], [:is, "visit:country", ["PL"]]]] + ]} + + iex> resolve_segments([[:is, "segment", [1, 2]]], %{1 => [[:contains, "event:goal", ["Singup"]], [:is, "visit:country", ["PL"]]], 2 => [[:contains, "event:goal", ["Sauna"]], [:is, "visit:country", ["EE"]]]}) + {:ok, [ + [:or, [ + [:and, [[:contains, "event:goal", ["Singup"]], [:is, "visit:country", ["PL"]]]], + [:and, [[:contains, "event:goal", ["Sauna"]], [:is, "visit:country", ["EE"]]]]] + ] + ]} + """ + def resolve_segments(original_filters, preloaded_segments) do + if map_size(preloaded_segments) > 0 do + {:ok, + Filters.transform_filters(original_filters, fn f -> + replace_segment_with_filter_tree(f, preloaded_segments) + end)} + else + {:ok, original_filters} + end + end +end diff --git a/lib/plausible/stats/filters/filters.ex b/lib/plausible/stats/filters/filters.ex index 422163500b12..4c828fef1e50 100644 --- a/lib/plausible/stats/filters/filters.ex +++ b/lib/plausible/stats/filters/filters.ex @@ -130,14 +130,11 @@ defmodule Plausible.Stats.Filters do to ignore and look deeper. ## Examples - - iex> Filters.transform_filters([[:is, "visit:entry_page", ["/blog"]]], fn f -> [f] end) - [[:is, "visit:entry_page", ["/blog"]]] - - iex> Filters.transform_filters([[:is, "visit:entry_page", ["/blog"]]], fn _f -> nil end) - [[[:is, "visit:entry_page", ["/blog"]]]] - - + iex> Filters.transform_filters([[:is, "visit:os", ["Linux"]], [:and, [[:is, "segment", [1]], [:is, "segment", [2]]]]], fn + ...> [_, "segment", _] -> [[:is, "segment", ["changed"]]] + ...> _ -> nil + ...> end) + [[:is, "visit:os", ["Linux"]], [:and, [[:is, "segment", ["changed"]], [:is, "segment", ["changed"]]]]] """ def transform_filters(filters, transformer) do filters @@ -156,7 +153,7 @@ defmodule Plausible.Stats.Filters do # Reached a leaf node, return existing value {nil, filter} -> - [[filter]] + [filter] # Transformer returned a value - don't transform that subtree {transformed_filters, _filter} -> diff --git a/lib/plausible/stats/filters/query_parser.ex b/lib/plausible/stats/filters/query_parser.ex index df733613dea3..0dd6a976c8ac 100644 --- a/lib/plausible/stats/filters/query_parser.ex +++ b/lib/plausible/stats/filters/query_parser.ex @@ -34,9 +34,10 @@ defmodule Plausible.Stats.Filters.QueryParser do utc_time_range = raw_time_range |> DateTimeRange.to_timezone("Etc/UTC"), {:ok, metrics} <- parse_metrics(Map.get(params, "metrics", [])), {:ok, filters} <- parse_filters(Map.get(params, "filters", [])), - {:ok, preloaded_segments} <- preload_needed_segments(site, filters), + {:ok, preloaded_segments} <- + Plausible.Segments.Filters.preload_needed_segments(site, filters), {:ok, filters} <- - resolve_segments(filters, preloaded_segments), + Plausible.Segments.Filters.resolve_segments(filters, preloaded_segments), {:ok, dimensions} <- parse_dimensions(Map.get(params, "dimensions", [])), {:ok, order_by} <- parse_order_by(Map.get(params, "order_by")), {:ok, include} <- parse_include(site, Map.get(params, "include", %{})), @@ -452,106 +453,6 @@ defmodule Plausible.Stats.Filters.QueryParser do } end - @doc """ - Finds unique segment IDs used in query filters. - - ## Examples - iex> get_segment_ids([[:not, [:is, "segment", [10, 20]]], [:contains, "visit:entry_page", ["blog"]]]) - {:ok, [10, 20]} - - iex> get_segment_ids([[:and, [[:is, "segment", Enum.to_list(1..6)], [:is, "segment", Enum.to_list(1..6)]]]]) - {:error, "Invalid filters. You can only use up to 10 segment filters in a query."} - """ - def get_segment_ids(filters) do - max_segment_filters_count = 10 - - ids = - filters - |> Filters.traverse() - |> Enum.filter(fn {[_, filter_key | _rest], _depth} -> filter_key === "segment" end) - |> Enum.map(fn {[_, _filter_key, clauses], _depth} -> clauses end) - |> Enum.concat() - - if length(ids) > max_segment_filters_count do - {:error, - "Invalid filters. You can only use up to #{max_segment_filters_count} segment filters in a query."} - else - {:ok, Enum.uniq(ids)} - end - end - - def preload_needed_segments(%Plausible.Site{} = site, filters) do - with {:ok, segment_ids} <- get_segment_ids(filters), - {:ok, segments} <- - Plausible.Segments.get_many( - site, - segment_ids, - fields: [:id, :segment_data] - ), - {:ok, segments_by_id} <- - {:ok, - Enum.into( - segments, - %{}, - fn %Plausible.Segments.Segment{id: id, segment_data: segment_data} -> - case parse_filters(segment_data["filters"]) do - {:ok, filters} -> {id, filters} - _ -> {id, nil} - end - end - )}, - :ok <- - if(Enum.any?(segment_ids, fn id -> is_nil(Map.get(segments_by_id, id)) end), - do: {:error, "Invalid filters. Some segments don't exist or aren't accessible."}, - else: :ok - ) do - {:ok, segments_by_id} - end - end - - defp replace_segment_with_filter_tree([_, "segment", clauses], preloaded_segments) do - if length(clauses) === 1 do - [[:and, Map.get(preloaded_segments, Enum.at(clauses, 0))]] - else - [[:or, Enum.map(clauses, fn id -> [:and, Map.get(preloaded_segments, id)] end)]] - end - end - - defp replace_segment_with_filter_tree(filter, _preloaded_segments) do - [filter] - end - - @doc """ - ## Examples - - iex> resolve_segments([[:is, "visit:entry_page", ["/home"]]], %{}) - {:ok, [[:is, "visit:entry_page", ["/home"]]]} - - iex> resolve_segments([[:is, "visit:entry_page", ["/home"]], [:is, "segment", [1]]], %{1 => [[:contains, "visit:entry_page", ["blog"]], [:is, "visit:country", ["PL"]]]}) - {:ok, [ - [:is, "visit:entry_page", ["/home"]], - [:and, [[:contains, "visit:entry_page", ["blog"]], [:is, "visit:country", ["PL"]]]] - ]} - - iex> resolve_segments([[:is, "segment", [1, 2]]], %{1 => [[:contains, "event:goal", ["Singup"]], [:is, "visit:country", ["PL"]]], 2 => [[:contains, "event:goal", ["Sauna"]], [:is, "visit:country", ["EE"]]]}) - {:ok, [ - [:or, [ - [:and, [[:contains, "event:goal", ["Singup"]], [:is, "visit:country", ["PL"]]]], - [:and, [[:contains, "event:goal", ["Sauna"]], [:is, "visit:country", ["EE"]]]]] - ] - ]} - """ - def resolve_segments(original_filters, preloaded_segments) do - if Map.keys(preloaded_segments) !== [] do - {:ok, - Filters.transform_filters(original_filters, fn f -> - replace_segment_with_filter_tree(f, preloaded_segments) - end)} - else - {:ok, original_filters} - end - end - @only_toplevel ["event:goal", "event:hostname"] defp validate_toplevel_only_filter_dimension(query) do not_toplevel = Filters.dimensions_used_in_filters(query.filters, min_depth: 1) diff --git a/priv/json-schemas/query-api-schema.json b/priv/json-schemas/query-api-schema.json index 9558b5dffa1a..6a325c8d8e0e 100644 --- a/priv/json-schemas/query-api-schema.json +++ b/priv/json-schemas/query-api-schema.json @@ -462,7 +462,10 @@ { "$ref": "#/definitions/filter_without_goals" }, { "$ref": "#/definitions/filter_with_goals" }, { "$ref": "#/definitions/filter_with_pattern" }, - { "$ref": "#/definitions/filter_for_segment" } + { + "$ref": "#/definitions/filter_for_segment", + "$comment": "only :internal" + } ] }, "filter_tree": { diff --git a/test/plausible/segments/filters_test.exs b/test/plausible/segments/filters_test.exs new file mode 100644 index 000000000000..e19b58df73b0 --- /dev/null +++ b/test/plausible/segments/filters_test.exs @@ -0,0 +1,5 @@ +defmodule Plausible.Segments.FiltersTest do + use ExUnit.Case, async: true + + doctest Plausible.Segments.Filters, import: true +end diff --git a/test/plausible/stats/query_parser_test.exs b/test/plausible/stats/query_parser_test.exs index 13681b692f4b..86b70265e0f7 100644 --- a/test/plausible/stats/query_parser_test.exs +++ b/test/plausible/stats/query_parser_test.exs @@ -2029,4 +2029,149 @@ defmodule Plausible.Stats.Filters.QueryParserTest do }) end end + + describe "filtering with segments" do + test "parsing fails when too many segments in query", %{ + user: user, + site: site + } do + segments = + insert_list(11, :segment, + type: :site, + owner: user, + site: site, + name: "any" + ) + + %{ + "site_id" => site.domain, + "metrics" => ["visitors"], + "date_range" => "all", + "filters" => [ + ["and", segments |> Enum.map(fn segment -> ["is", "segment", [segment.id]] end)] + ] + } + |> check_error( + site, + "Invalid filters. You can only use up to 10 segment filters in a query.", + :internal + ) + end + + test "parsing fails when segment filter is used, but segment is from another site", %{ + site: site + } do + other_user = new_user() + other_site = new_site(owner: other_user) + + segment = + insert(:segment, + type: :site, + owner: other_user, + site: other_site, + name: "any" + ) + + %{ + "site_id" => site.domain, + "metrics" => ["visitors"], + "date_range" => "all", + "filters" => [["is", "segment", [segment.id]]] + } + |> check_error( + site, + "Invalid filters. Some segments don't exist or aren't accessible.", + :internal + ) + end + + test "resolves segments correctly", %{site: site, user: user} do + emea_segment = + insert(:segment, + type: :site, + owner: user, + site: site, + name: "EMEA", + segment_data: %{ + "filters" => [["is", "visit:country", ["FR", "DE"]]], + "labels" => %{"FR" => "France", "DE" => "Germany"} + } + ) + + apac_segment = + insert(:segment, + type: :site, + owner: user, + site: site, + name: "APAC", + segment_data: %{ + "filters" => [["is", "visit:country", ["AU", "NZ"]]], + "labels" => %{"AU" => "Australia", "NZ" => "New Zealand"} + } + ) + + firefox_segment = + insert(:segment, + type: :site, + owner: user, + site: site, + name: "APAC", + segment_data: %{ + "filters" => [ + ["is", "visit:browser", ["Firefox"]], + ["is", "visit:os", ["Linux"]] + ] + } + ) + + %{ + "site_id" => site.domain, + "metrics" => ["visitors", "events"], + "date_range" => "all", + "filters" => [ + [ + "and", + [ + ["is", "segment", [apac_segment.id, emea_segment.id]], + ["is", "segment", [firefox_segment.id]] + ] + ] + ] + } + |> check_success( + site, + %{ + metrics: [:visitors, :events], + utc_time_range: @date_range_day, + filters: [ + [ + :and, + [ + [ + :or, + [ + [:and, [[:is, "visit:country", ["AU", "NZ"]]]], + [:and, [[:is, "visit:country", ["FR", "DE"]]]] + ] + ], + [ + :and, + [ + [:is, "visit:browser", ["Firefox"]], + [:is, "visit:os", ["Linux"]] + ] + ] + ] + ] + ], + dimensions: [], + order_by: nil, + timezone: site.timezone, + include: %{imports: false, time_labels: false, total_rows: false, comparisons: nil}, + pagination: %{limit: 10_000, offset: 0} + }, + :internal + ) + end + end end From 85a98d692e308a2013940e26d8f471f71507d114 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Mon, 20 Jan 2025 13:12:40 +0200 Subject: [PATCH 10/13] Add moduledoc --- lib/plausible/segments/filters.ex | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/plausible/segments/filters.ex b/lib/plausible/segments/filters.ex index 2e057a9d5381..1e1e5a278e68 100644 --- a/lib/plausible/segments/filters.ex +++ b/lib/plausible/segments/filters.ex @@ -1,4 +1,7 @@ defmodule Plausible.Segments.Filters do + @moduledoc """ + This module contains functions that enable resolving segments in filters. + """ alias Plausible.Segments alias Plausible.Stats.Filters From 9b8abb21200b24caf18b98a599685de323d79354 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Mon, 20 Jan 2025 17:27:20 +0200 Subject: [PATCH 11/13] Add tests for /v2/query-internal-test --- .../external_stats_controller/query_test.exs | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) 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 009cec618903..f085ece80478 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 @@ -4471,4 +4471,117 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do assert "metric_warnings" not in json_response(conn, 200)["meta"] end end + + describe "segment filters" do + setup [:create_user, :create_site, :create_api_key, :use_api_key] + + test "segment filters are (not yet) available in public API", %{conn: conn, site: site} do + conn = + post(conn, "/api/v2/query", %{ + "site_id" => site.domain, + "filters" => [["is", "segment", [1]]], + "date_range" => "all", + "metrics" => ["visitors"] + }) + + assert json_response(conn, 400) == %{ + "error" => "#/filters/0: Invalid filter [\"is\", \"segment\", [1]]" + } + end + + test "site segments of other sites don't resolve", %{ + conn: conn, + site: site + } do + other_site = new_site() + other_user = add_guest(other_site, role: :editor) + + segment = + insert(:segment, + name: "Segment of another site", + type: :site, + owner: other_user, + site: other_site, + segment_data: %{ + "filters" => [["is", "event:page", ["/blog"]]] + } + ) + + conn = + post(conn, "/api/v2/query-internal-test", %{ + "site_id" => site.domain, + "filters" => [["is", "segment", [segment.id]]], + "date_range" => "all", + "metrics" => ["events"] + }) + + assert json_response(conn, 400) == %{ + "error" => "Invalid filters. Some segments don't exist or aren't accessible." + } + end + + test "personal segments of other users of the same site resolve to filters", %{ + conn: conn, + site: site + } do + other_user = add_guest(site, role: :editor) + + segment = + insert(:segment, + type: :personal, + owner: other_user, + site: site, + name: "Signups", + segment_data: %{ + "filters" => [["is", "event:name", ["Signup"]]] + } + ) + + insert(:goal, %{site: site, event_name: "Signup"}) + + populate_stats(site, [ + build(:event, + name: "Signup", + timestamp: ~N[2021-01-01 00:00:00] + ), + build(:event, + name: "Signup", + user_id: @user_id, + timestamp: ~N[2021-01-01 00:00:00] + ), + build(:event, + name: "Signup", + user_id: @user_id, + timestamp: ~N[2021-01-01 00:00:00] + ), + build(:event, + name: "AnyOtherEvent", + timestamp: ~N[2021-01-01 00:00:00] + ) + ]) + + conn = + post(conn, "/api/v2/query-internal-test", %{ + "site_id" => site.domain, + "filters" => [["is", "segment", [segment.id]]], + "date_range" => "all", + "metrics" => ["events"] + }) + + assert json_response(conn, 200) == %{ + "results" => [%{"dimensions" => [], "metrics" => [3]}], + "meta" => %{}, + "query" => %{ + "date_range" => ["2021-01-01T00:00:00+00:00", "2025-01-20T23:59:59+00:00"], + "dimensions" => [], + "filters" => [["and", [["is", "event:name", ["Signup"]]]]], + "include" => %{}, + "metrics" => ["events"], + "order_by" => [["events", "desc"]], + "pagination" => %{"limit" => 10000, "offset" => 0}, + "site_id" => site.domain + } + } + end + end end From 363b4b7d183559d0ddf2ba4844310b8fdbe89d22 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Mon, 20 Jan 2025 17:33:44 +0200 Subject: [PATCH 12/13] Refactor max segment filters count to module attribute --- lib/plausible/segments/filters.ex | 8 ++++---- .../api/external_stats_controller/query_test.exs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/plausible/segments/filters.ex b/lib/plausible/segments/filters.ex index 1e1e5a278e68..a18cf33e787b 100644 --- a/lib/plausible/segments/filters.ex +++ b/lib/plausible/segments/filters.ex @@ -5,6 +5,8 @@ defmodule Plausible.Segments.Filters do alias Plausible.Segments alias Plausible.Stats.Filters + @max_segment_filters_count 10 + @doc """ Finds unique segment IDs used in query filters. @@ -16,8 +18,6 @@ defmodule Plausible.Segments.Filters do {:error, "Invalid filters. You can only use up to 10 segment filters in a query."} """ def get_segment_ids(filters) do - max_segment_filters_count = 10 - ids = filters |> Filters.traverse() @@ -26,9 +26,9 @@ defmodule Plausible.Segments.Filters do _ -> [] end) - if length(ids) > max_segment_filters_count do + if length(ids) > @max_segment_filters_count do {:error, - "Invalid filters. You can only use up to #{max_segment_filters_count} segment filters in a query."} + "Invalid filters. You can only use up to #{@max_segment_filters_count} segment filters in a query."} else {:ok, Enum.uniq(ids)} 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 f085ece80478..a55d89845d46 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 @@ -4578,7 +4578,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do "include" => %{}, "metrics" => ["events"], "order_by" => [["events", "desc"]], - "pagination" => %{"limit" => 10000, "offset" => 0}, + "pagination" => %{"limit" => 10_000, "offset" => 0}, "site_id" => site.domain } } From a7d61d6eb13709c5d9a3b83ad73695ec6e827fc1 Mon Sep 17 00:00:00 2001 From: Artur Pata Date: Tue, 21 Jan 2025 12:31:43 +0200 Subject: [PATCH 13/13] Add more parser tests and unify asserts in query --- test/plausible/stats/query_parser_test.exs | 66 +++++++++++++++++++ .../external_stats_controller/query_test.exs | 29 ++++---- 2 files changed, 77 insertions(+), 18 deletions(-) diff --git a/test/plausible/stats/query_parser_test.exs b/test/plausible/stats/query_parser_test.exs index 86b70265e0f7..968d04fa0fcf 100644 --- a/test/plausible/stats/query_parser_test.exs +++ b/test/plausible/stats/query_parser_test.exs @@ -2085,6 +2085,72 @@ defmodule Plausible.Stats.Filters.QueryParserTest do ) end + test "hiding custom properties filters in segments doesn't allow bypasssing feature check", + %{ + site: site, + user: user + } do + subscribe_to_enterprise_plan(user, features: [Plausible.Billing.Feature.StatsAPI]) + + segment = + insert(:segment, + type: :site, + owner: user, + site: site, + name: "segment with custom props filter", + segment_data: %{"filters" => [["is", "event:props:foobar", ["foo"]]]} + ) + + %{ + "site_id" => site.domain, + "metrics" => ["visitors"], + "date_range" => "all", + "filters" => [["is", "segment", [segment.id]]] + } + |> check_error( + site, + "The owner of this site does not have access to the custom properties feature.", + :internal + ) + end + + test "querying conversion rate is illegal if the complex event:goal filter is within a segment", + %{ + site: site, + user: user + } do + segment = + insert(:segment, + type: :site, + owner: user, + site: site, + name: "any", + segment_data: %{ + "filters" => [ + [ + "or", + [ + ["is", "event:goal", ["Signup"]], + ["contains", "event:page", ["/"]] + ] + ] + ] + } + ) + + %{ + "site_id" => site.domain, + "metrics" => ["visitors", "conversion_rate"], + "date_range" => "all", + "filters" => [["is", "segment", [segment.id]]] + } + |> check_error( + site, + "Invalid filters. Dimension `event:goal` can only be filtered at the top level.", + :internal + ) + end + test "resolves segments correctly", %{site: site, user: user} do emea_segment = insert(:segment, 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 a55d89845d46..605ab5eac13f 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 @@ -4520,10 +4520,11 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do } end - test "personal segments of other users of the same site resolve to filters", %{ - conn: conn, - site: site - } do + test "even personal segments of other users of the same site resolve to filters, with segments expanded in response", + %{ + conn: conn, + site: site + } do other_user = add_guest(site, role: :editor) segment = @@ -4568,20 +4569,12 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do "metrics" => ["events"] }) - assert json_response(conn, 200) == %{ - "results" => [%{"dimensions" => [], "metrics" => [3]}], - "meta" => %{}, - "query" => %{ - "date_range" => ["2021-01-01T00:00:00+00:00", "2025-01-20T23:59:59+00:00"], - "dimensions" => [], - "filters" => [["and", [["is", "event:name", ["Signup"]]]]], - "include" => %{}, - "metrics" => ["events"], - "order_by" => [["events", "desc"]], - "pagination" => %{"limit" => 10_000, "offset" => 0}, - "site_id" => site.domain - } - } + assert json_response(conn, 200)["results"] == [%{"dimensions" => [], "metrics" => [3]}] + + # response shows what filters the segment was resolved to + assert json_response(conn, 200)["query"]["filters"] == [ + ["and", [["is", "event:name", ["Signup"]]]] + ] end end end