Enforce max limit for goals per site (#5917)

* Limit preloading goals

* Enforce max limit for goals per site

* typo

* credo

* Remove logger call

* Integrate #5916

* Add a test

* Add test

* Unignore opts

config/test.exs

@@ -63,3 +63,5 @@ config :plausible, Plausible.InstallationSupport.Checks.VerifyInstallation,
   ]
 
 config :plausible, Plausible.Session.Salts, interval: :timer.hours(1)
+
+config :plausible, max_goals_per_site: 10

extra/lib/plausible_web/controllers/api/external_sites_controller.ex

@@ -408,6 +408,11 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
       {:missing, param} ->
         H.bad_request(conn, "Parameter `#{param}` is required to create a goal")
 
+      {:error, %Ecto.Changeset{} = changeset} ->
+        message = Enum.map_join(changeset.errors, ", ", &translate_error/1)
+
+        H.bad_request(conn, message)
+
       e ->
         H.bad_request(conn, "Something went wrong: #{inspect(e)}")
     end
@@ -605,4 +610,10 @@ defmodule PlausibleWeb.Api.ExternalSitesController do
     # remap to `custom_properties`
     |> Map.put(:custom_properties, site.allowed_event_props || [])
   end
+
+  defp translate_error({field, {msg, opts}}) do
+    Enum.reduce(opts, "#{field}: #{msg}", fn {key, value}, acc ->
+      String.replace(acc, "%{#{key}}", fn _ -> to_string(value) end)
+    end)
+  end
 end

lib/plausible/goals/goals.ex

@@ -7,6 +7,19 @@ defmodule Plausible.Goals do
 
   alias Plausible.Goal
   alias Ecto.Multi
+  alias Ecto.Changeset
+
+  @max_goals_per_site 1_000
+  @spec max_goals_per_site(Keyword.t()) :: pos_integer()
+  def max_goals_per_site(opts \\ []) do
+    override = Keyword.get(opts, :max_goals_per_site)
+
+    if override do
+      override
+    else
+      Application.get_env(:plausible, :max_goals_per_site, @max_goals_per_site)
+    end
+  end
 
   @spec get(Plausible.Site.t(), pos_integer()) :: nil | Plausible.Goal.t()
   def get(site, id) when is_integer(id) do
@@ -20,7 +33,9 @@ defmodule Plausible.Goals do
   end
 
   @spec create(Plausible.Site.t(), map(), Keyword.t()) ::
-          {:ok, Goal.t()} | {:error, Ecto.Changeset.t()} | {:error, :upgrade_required}
+          {:ok, Goal.t()}
+          | {:error, Changeset.t()}
+          | {:error, :upgrade_required}
   @doc """
   Creates a Goal for a site.
 
@@ -28,10 +43,8 @@ defmodule Plausible.Goals do
   refreshed by the sites cache, as revenue goals are used during ingestion.
   """
   def create(site, params, opts \\ []) do
-    upsert? = Keyword.get(opts, :upsert?, false)
-
     Repo.transaction(fn ->
-      case insert_goal(site, params, upsert?) do
+      case insert_goal(site, params, opts) do
         {:ok, :insert, goal} ->
           on_ee do
             now = Keyword.get(opts, :now, DateTime.utc_now())
@@ -53,7 +66,7 @@ defmodule Plausible.Goals do
   end
 
   @spec update(Plausible.Goal.t(), map()) ::
-          {:ok, Goal.t()} | {:error, Ecto.Changeset.t()} | {:error, :upgrade_required}
+          {:ok, Goal.t()} | {:error, Changeset.t()} | {:error, :upgrade_required}
   def update(goal, params) do
     changeset = Goal.changeset(goal, params)
 
@@ -69,7 +82,7 @@ defmodule Plausible.Goals do
           updated_goal
         end
       else
-        {:error, %Ecto.Changeset{} = changeset} ->
+        {:error, %Changeset{} = changeset} ->
           Repo.rollback(changeset)
 
         {:error, :upgrade_required} ->
@@ -78,16 +91,19 @@ defmodule Plausible.Goals do
     end)
   end
 
+  def find_or_create(site, params, opts \\ [])
+
   def find_or_create(
         site,
         %{
           "goal_type" => "event",
           "event_name" => event_name,
           "currency" => currency
-        } = params
+        } = params,
+        opts
       )
       when is_binary(event_name) and is_binary(currency) do
-    with {:ok, goal} <- create(site, params, upsert?: true) do
+    with {:ok, goal} <- create(site, params, do_upsert(opts)) do
       if to_string(goal.currency) == currency do
         {:ok, goal}
       else
@@ -95,7 +111,7 @@ defmodule Plausible.Goals do
         changeset =
           goal
           |> Goal.changeset()
-          |> Ecto.Changeset.add_error(
+          |> Changeset.add_error(
             :event_name,
             "'#{goal.event_name}' (with currency: #{goal.currency}) has already been taken"
           )
@@ -105,23 +121,25 @@ defmodule Plausible.Goals do
     end
   end
 
-  def find_or_create(site, %{"goal_type" => "event", "event_name" => event_name} = params)
+  def find_or_create(site, %{"goal_type" => "event", "event_name" => event_name} = params, opts)
       when is_binary(event_name) do
-    create(site, params, upsert?: true)
+    create(site, params, do_upsert(opts))
   end
 
-  def find_or_create(_, %{"goal_type" => "event"}), do: {:missing, "event_name"}
+  def find_or_create(_, %{"goal_type" => "event"}, _), do: {:missing, "event_name"}
 
-  def find_or_create(site, %{"goal_type" => "page", "page_path" => _} = params) do
-    create(site, params, upsert?: true)
+  def find_or_create(site, %{"goal_type" => "page", "page_path" => _} = params, opts) do
+    create(site, params, do_upsert(opts))
   end
 
-  def find_or_create(_, %{"goal_type" => "page"}), do: {:missing, "page_path"}
+  def find_or_create(_, %{"goal_type" => "page"}, _), do: {:missing, "page_path"}
 
   def list_revenue_goals(site) do
     from(g in Plausible.Goal,
       where: g.site_id == ^site.id and not is_nil(g.currency),
-      select: %{display_name: g.display_name, currency: g.currency}
+      select: %{display_name: g.display_name, currency: g.currency},
+      order_by: [desc: g.id],
+      limit: ^max_goals_per_site()
     )
     |> Plausible.Repo.all()
   end
@@ -133,13 +151,21 @@ defmodule Plausible.Goals do
     |> Enum.map(&maybe_trim/1)
   end
 
-  def for_site_query(site, opts \\ []) do
+  def for_site_query(site \\ nil, opts \\ []) do
     query =
       from g in Goal,
-        inner_join: assoc(g, :site),
-        where: g.site_id == ^site.id,
         order_by: [desc: g.id],
-        preload: [:site]
+        limit: ^max_goals_per_site(opts)
+
+    query =
+      if site do
+        from g in query,
+          inner_join: assoc(g, :site),
+          where: g.site_id == ^site.id,
+          preload: [:site]
+      else
+        query
+      end
 
     if ee?() and opts[:preload_funnels?] == true do
       from(g in query,
@@ -152,9 +178,9 @@ defmodule Plausible.Goals do
     end
   end
 
-  def batch_create_event_goals(names, site) do
+  def batch_create_event_goals(names, site, opts \\ []) do
     Enum.reduce(names, [], fn name, acc ->
-      case insert_goal(site, %{event_name: name}, true) do
+      case insert_goal(site, %{event_name: name}, do_upsert(opts)) do
         {:ok, _, goal} -> acc ++ [goal]
         _ -> acc
       end
@@ -248,25 +274,25 @@ defmodule Plausible.Goals do
 
   @spec create_outbound_links(Plausible.Site.t()) :: :ok
   def create_outbound_links(%Plausible.Site{} = site) do
-    create(site, %{"event_name" => "Outbound Link: Click"}, upsert?: true)
+    create(site, %{"event_name" => "Outbound Link: Click"}, do_upsert())
     :ok
   end
 
   @spec create_file_downloads(Plausible.Site.t()) :: :ok
   def create_file_downloads(%Plausible.Site{} = site) do
-    create(site, %{"event_name" => "File Download"}, upsert?: true)
+    create(site, %{"event_name" => "File Download"}, do_upsert())
     :ok
   end
 
   @spec create_form_submissions(Plausible.Site.t()) :: :ok
   def create_form_submissions(%Plausible.Site{} = site) do
-    create(site, %{"event_name" => "Form: Submission"}, upsert?: true)
+    create(site, %{"event_name" => "Form: Submission"}, do_upsert())
     :ok
   end
 
   @spec create_404(Plausible.Site.t()) :: :ok
   def create_404(%Plausible.Site{} = site) do
-    create(site, %{"event_name" => "404"}, upsert?: true)
+    create(site, %{"event_name" => "404"}, do_upsert())
     :ok
   end
 
@@ -314,11 +340,11 @@ defmodule Plausible.Goals do
     :ok
   end
 
-  defp insert_goal(site, params, upsert?) do
+  defp insert_goal(site, params, opts) do
     params = Map.delete(params, "site_id")
 
     insert_opts =
-      if upsert? do
+      if upsert?(opts) do
         [on_conflict: :nothing]
       else
         []
@@ -328,6 +354,7 @@ defmodule Plausible.Goals do
 
     with :ok <- maybe_check_feature_access(site, changeset),
          :ok <- check_no_currency_if_consolidated(site, changeset),
+         :ok <- check_goals_limit(site, changeset, opts),
          {:ok, goal} <- Repo.insert(changeset, insert_opts) do
       # Upsert with `on_conflict: :nothing` strategy
       # will result in goal struct missing primary key field
@@ -346,7 +373,7 @@ defmodule Plausible.Goals do
   end
 
   defp maybe_check_feature_access(site, changeset) do
-    if Ecto.Changeset.get_field(changeset, :currency) do
+    if Changeset.get_field(changeset, :currency) do
       site = Plausible.Repo.preload(site, :team)
       Plausible.Billing.Feature.RevenueGoals.check_availability(site.team)
     else
@@ -354,8 +381,23 @@ defmodule Plausible.Goals do
     end
   end
 
+  defp check_goals_limit(site, changeset, opts) do
+    if upsert?(opts) and goal_exists_for_upsert?(site, changeset) do
+      :ok
+    else
+      if count(site) >= max_goals_per_site(opts) and changeset.valid? do
+        changeset
+        |> Changeset.add_error(:page_path, "Maximum number of goals reached")
+        |> Changeset.add_error(:event_name, "Maximum number of goals reached")
+        |> Changeset.apply_action(:insert)
+      else
+        :ok
+      end
+    end
+  end
+
   defp check_no_currency_if_consolidated(site, changeset) do
-    if Plausible.Sites.consolidated?(site) && Ecto.Changeset.get_field(changeset, :currency) do
+    if Plausible.Sites.consolidated?(site) && Changeset.get_field(changeset, :currency) do
       {:error, :revenue_goals_unavailable}
     else
       :ok
@@ -377,4 +419,29 @@ defmodule Plausible.Goals do
   defp maybe_trim(other) do
     other
   end
+
+  defp upsert?(opts) do
+    Keyword.get(opts, :upsert?, false)
+  end
+
+  defp do_upsert(opts \\ []) do
+    Keyword.put(opts, :upsert?, true)
+  end
+
+  defp goal_exists_for_upsert?(site, changeset) do
+    event_name = Changeset.get_field(changeset, :event_name)
+    page_path = Changeset.get_field(changeset, :page_path)
+    scroll_threshold = Changeset.get_field(changeset, :scroll_threshold)
+
+    query_params =
+      [site_id: site.id]
+      |> maybe_add_filter(:event_name, event_name)
+      |> maybe_add_filter(:page_path, page_path)
+      |> maybe_add_filter(:scroll_threshold, scroll_threshold)
+
+    Repo.exists?(from(g in Goal, where: ^query_params))
+  end
+
+  defp maybe_add_filter(params, _key, nil), do: params
+  defp maybe_add_filter(params, key, value), do: Keyword.put(params, key, value)
 end

lib/plausible/stats/goal_suggestions.ex

@@ -26,9 +26,7 @@ defmodule Plausible.Stats.GoalSuggestions do
   def suggest_event_names(site, search_input, opts \\ []) do
     matches = "%#{search_input}%"
 
-    site =
-      site
-      |> Repo.preload(:goals)
+    site = Repo.preload(site, goals: Plausible.Goals.for_site_query())
 
     excluded =
       opts

lib/plausible_web/controllers/api/stats_controller.ex

@@ -1391,7 +1391,11 @@ defmodule PlausibleWeb.Api.StatsController do
 
   def conversions(conn, params) do
     pagination = parse_pagination(params)
-    site = Plausible.Repo.preload(conn.assigns.site, :goals)
+
+    site =
+      Plausible.Repo.preload(conn.assigns.site,
+        goals: Plausible.Goals.for_site_query()
+      )
 
     params =
       params

lib/plausible_web/live/components/combo_box/static_search.ex

@@ -2,7 +2,7 @@ defmodule PlausibleWeb.Live.Components.ComboBox.StaticSearch do
   @moduledoc """
   Default suggestion engine for the `ComboBox` component.
 
-  Assumes, the user have already queried the database and the data set is
+  Assumes, the user has already queried the database and the data set is
   small enough to be kept in state and filtered based on external input.
 
   Favours exact matches. Skips entries shorter than input.

test/plausible/goals_test.exs

@@ -276,6 +276,14 @@ defmodule Plausible.GoalsTest do
     assert [%{page_path: "/Signup"}, %{event_name: "Signup"}] = goals
   end
 
+  test "for_site/1 returns goals up to a limit" do
+    site = new_site()
+    for i <- 1..11, do: insert(:goal, %{site: site, event_name: "G#{i}"})
+
+    assert Goals.count(site) == 11
+    assert length(Goals.for_site(site)) == 10
+  end
+
   test "goals are present after domain change" do
     site = new_site()
     insert(:goal, %{site: site, event_name: " Signup "})
@@ -388,4 +396,75 @@ defmodule Plausible.GoalsTest do
 
     assert {"cannot co-exist with page_path", _} = changeset.errors[:event_name]
   end
+
+  test "enforces goal limit per site" do
+    site = new_site()
+
+    for i <- 1..3 do
+      assert {:ok, _goal} = Goals.create(site, %{"event_name" => "Event #{i}"})
+    end
+
+    assert {:ok, _} =
+             Goals.create(site, %{"event_name" => "Event 4"})
+
+    assert {:error, changeset} =
+             Goals.create(site, %{"event_name" => "Event 5"}, max_goals_per_site: 4)
+
+    assert {"Maximum number of goals reached", _} = changeset.errors[:event_name]
+    assert {"Maximum number of goals reached", _} = changeset.errors[:page_path]
+  end
+
+  test "find_or_create with upsert bypasses limit check" do
+    site = new_site()
+
+    for i <- 1..3 do
+      assert {:ok, _goal} = Goals.create(site, %{"event_name" => "Event #{i}"})
+    end
+
+    assert {:ok, goal} =
+             Goals.find_or_create(site, %{"goal_type" => "event", "event_name" => "Event 1"},
+               max_goals_per_site: 3
+             )
+
+    assert goal.event_name == "Event 1"
+  end
+
+  test "allows creating goals after deleting some" do
+    site = new_site()
+
+    for i <- 1..3 do
+      assert {:ok, _goal} = Goals.create(site, %{"event_name" => "Event #{i}"})
+    end
+
+    assert {:error, changeset} =
+             Goals.create(site, %{"event_name" => "Event 4"}, max_goals_per_site: 3)
+
+    assert {"Maximum number of goals reached", _} = changeset.errors[:event_name]
+    assert {"Maximum number of goals reached", _} = changeset.errors[:page_path]
+
+    [goal | _] = Goals.for_site(site)
+    :ok = Goals.delete(goal.id, site)
+
+    assert {:ok, _goal} = Goals.create(site, %{"event_name" => "Event 4"}, max_goals_per_site: 3)
+  end
+
+  test "batch_create_event_goals respects limit" do
+    site = new_site()
+
+    for i <- 1..6 do
+      assert {:ok, _goal} = Goals.create(site, %{"event_name" => "Event #{i}"})
+    end
+
+    event_names = for i <- 6..20, do: "Event #{i}"
+
+    created_goals = Goals.batch_create_event_goals(event_names, site, max_goals_per_site: 10)
+
+    assert length(created_goals) == 5
+    assert Enum.count(Goals.for_site(site)) == 10
+  end
+
+  test "on Mix.env == :test, max goals per site is 10 and can be overridden" do
+    assert Plausible.Goals.max_goals_per_site() == 10
+    assert Plausible.Goals.max_goals_per_site(max_goals_per_site: 5) == 5
+  end
 end

test/plausible_web/controllers/api/external_sites_controller_test.exs

@@ -650,6 +650,21 @@ defmodule PlausibleWeb.Api.ExternalSitesControllerTest do
         res = json_response(conn, 400)
         assert res["error"] == "Parameter `page_path` is required to create a goal"
       end
+
+      test "fails when max goals per site limit reached", %{conn: conn, site: site} do
+        for i <- 1..10, do: {:ok, _} = Plausible.Goals.create(site, %{"event_name" => "G#{i}"})
+
+        conn =
+          put(conn, "/api/v1/sites/goals", %{
+            site_id: site.domain,
+            goal_type: "event",
+            event_name: "Signup"
+          })
+
+        res = json_response(conn, 400)
+
+        assert res["error"] =~ "Maximum number of goals reached"
+      end
     end
 
     describe "DELETE /api/v1/sites/custom-props/:property" do

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

@@ -129,6 +129,38 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryGoalDimensionTest do
                %{"dimensions" => ["Signup"], "metrics" => [50.0]}
              ]
     end
+
+    test "returns goals up to a limit", %{conn: conn, site: site} do
+      for i <- 1..11, do: insert(:goal, %{site: site, event_name: "G#{i}"})
+
+      populate_stats(
+        site,
+        for(
+          i <- 1..11,
+          do:
+            build(:event,
+              name: "G#{i}",
+              timestamp: NaiveDateTime.add(~N[2021-01-01 00:00:03], i, :second)
+            )
+        )
+      )
+
+      conn =
+        post(conn, "/api/v2/query", %{
+          "site_id" => site.domain,
+          "date_range" => "all",
+          "metrics" => ["visitors"],
+          "dimensions" => ["event:goal"]
+        })
+
+      results = json_response(conn, 200)["results"]
+
+      assert Enum.find(results, &(&1["dimensions"] == ["G11"]))
+      assert Enum.find(results, &(&1["dimensions"] == ["G2"]))
+      refute Enum.find(results, &(&1["dimensions"] == ["G1"]))
+
+      assert length(results) == Plausible.Goals.max_goals_per_site()
+    end
   end
 
   describe "page scroll goals" do

test/plausible_web/live/funnel_settings/form_test.exs

@@ -60,19 +60,6 @@ defmodule PlausibleWeb.Live.FunnelSettings.FormTest do
       refute text_of_element(doc, "ul#dropdown-step-2 li") =~ "Another World"
     end
 
-    test "suggestions are limited on change", %{conn: conn, site: site} do
-      setup_goals(site, for(i <- 1..20, do: "Goal #{i}"))
-      lv = get_liveview(conn, site)
-
-      doc =
-        lv
-        |> element("li#dropdown-step-1-option-1 a")
-        |> render_click()
-
-      assert element_exists?(doc, ~s/li#dropdown-step-1-option-15/)
-      refute element_exists?(doc, ~s/li#dropdown-step-1-option-16/)
-    end
-
     test "removing one option alters suggestions for other", %{conn: conn, site: site} do
       setup_goals(site, ["Hello World", "Plausible", "Another World"])
 
@@ -110,7 +97,9 @@ defmodule PlausibleWeb.Live.FunnelSettings.FormTest do
   defp setup_goals(site, goal_names) when is_list(goal_names) do
     goals =
       Enum.map(goal_names, fn goal_name ->
-        {:ok, g} = Plausible.Goals.create(site, %{"event_name" => goal_name})
+        {:ok, g} =
+          Plausible.Goals.create(site, %{"event_name" => goal_name}, max_goals_per_site: 100)
+
         g
       end)
 

test/plausible_web/live/goal_settings/form_test.exs

@@ -198,6 +198,24 @@ defmodule PlausibleWeb.Live.GoalSettings.FormTest do
       html = render(lv)
       assert html =~ "Visit /page/**"
       assert html =~ "Pageview"
+
+      assert Plausible.Goals.count(site) == 1
+    end
+
+    test "fails to create a goal above limit", %{conn: conn, site: site} do
+      for i <- 1..10, do: {:ok, _} = Plausible.Goals.create(site, %{"event_name" => "G#{i}"})
+
+      lv = get_liveview(conn, site)
+      refute render(lv) =~ "Visit /page/**"
+
+      lv
+      |> element("#goals-form-modalseq0 form")
+      |> render_submit(%{goal: %{page_path: "/page/**"}})
+
+      html = render(lv)
+      assert html =~ "Maximum number of goals reached"
+
+      assert Plausible.Goals.count(site) == 10
     end
   end
 

test/plausible_web/plugins/api/controllers/goals_test.exs

@@ -280,6 +280,32 @@ defmodule PlausibleWeb.Plugins.API.Controllers.GoalsTest do
       assert [%{page_path: "/checkout"}] = Plausible.Goals.for_site(site)
     end
 
+    test "fails to create goal when max goals per site limit reached", %{
+      conn: conn,
+      token: token,
+      site: site
+    } do
+      for i <- 1..10, do: {:ok, _} = Plausible.Goals.create(site, %{"event_name" => "G#{i}"})
+
+      url = Routes.plugins_api_goals_url(PlausibleWeb.Endpoint, :create)
+
+      payload = %{goal_type: "Goal.Pageview", goal: %{path: "/checkout"}}
+
+      resp =
+        conn
+        |> authenticate(site.domain, token)
+        |> put_req_header("content-type", "application/json")
+        |> put(url, payload)
+        |> json_response(422)
+        |> assert_schema("UnprocessableEntityError", spec())
+
+      assert %Schemas.Error{
+               detail: "event_name: Maximum number of goals reached"
+             } in resp.errors
+
+      assert Plausible.Goals.count(site) == 10
+    end
+
     test "is idempotent", %{conn: conn, token: token, site: site} do
       url = Routes.plugins_api_goals_url(PlausibleWeb.Endpoint, :create)