fix: ignore imported data when querying goals with custom props

lib/plausible/stats/goals.ex

@@ -98,11 +98,20 @@ defmodule Plausible.Stats.Goals do
         goal_type = Plausible.Goal.type(goal)
         {prop_keys, prop_values} = Enum.unzip(goal.custom_props)
 
+        # `types` and `event_names_imports` are only used for matching goals
+        # against imported data. Imported tables are aggregated without custom
+        # properties, so goals with custom props must never match - "" ensures
+        # that.
+        queryable_from_imports? = not Plausible.Goal.has_custom_props?(goal)
+
         %{
           indices: [idx | acc.indices],
-          types: [to_string(goal_type) | acc.types],
+          types: [if(queryable_from_imports?, do: to_string(goal_type), else: "") | acc.types],
           # This will contain "" for non-event goals
-          event_names_imports: [to_string(goal.event_name) | acc.event_names_imports],
+          event_names_imports: [
+            if(queryable_from_imports?, do: to_string(goal.event_name), else: "")
+            | acc.event_names_imports
+          ],
           event_names_by_type: [event_name_by_type(goal_type, goal) | acc.event_names_by_type],
           # Event goals are considered to match everything for the sake of efficient queries in query_builder.ex
           # See also Plausible.Stats.SQL.Expression.event_goal_join/1

lib/plausible/stats/imported/base.ex

@@ -142,13 +142,21 @@ defmodule Plausible.Stats.Imported.Base do
       |> Enum.any?(&(&1 not in [property, "event:name", "event:goal"]))
 
     if has_required_event_or_goal_name_filter? and
-         not has_unsupported_filters? do
+         not has_unsupported_filters? and
+         not any_custom_prop_goal_filters?(query) do
       ["imported_custom_events"]
     else
       []
     end
   end
 
+  # Goals with custom props cannot be queried from imported data, as imported
+  # tables are aggregated without custom properties.
+  defp any_custom_prop_goal_filters?(query) do
+    query.preloaded_goals.matching_toplevel_filters
+    |> Enum.any?(&Plausible.Goal.has_custom_props?/1)
+  end
+
   defp do_decide_tables(%Query{filters: [], dimensions: []}), do: ["imported_visitors"]
 
   defp do_decide_tables(%Query{filters: [], dimensions: ["event:goal"]}) do
@@ -172,6 +180,7 @@ defmodule Plausible.Stats.Imported.Base do
       Enum.any?(filter_dimensions, &(&1 not in ["event:page", "event:name", "event:goal"]))
 
     cond do
+      any_custom_prop_goal_filters?(query) -> []
       any_other_filters? -> []
       any_event_name_filters? and not any_page_filters? -> ["imported_custom_events"]
       any_page_filters? and not any_event_name_filters? -> ["imported_pages"]
@@ -192,11 +201,13 @@ defmodule Plausible.Stats.Imported.Base do
 
     filter_goal_table_candidates =
       query.preloaded_goals.matching_toplevel_filters
-      |> Enum.map(&Plausible.Goal.type/1)
-      |> Enum.map(fn
-        :event -> "imported_custom_events"
-        :page -> "imported_pages"
-        :scroll -> nil
+      |> Enum.map(fn goal ->
+        case {Plausible.Goal.has_custom_props?(goal), Plausible.Goal.type(goal)} do
+          {true, _} -> nil
+          {false, :event} -> "imported_custom_events"
+          {false, :page} -> "imported_pages"
+          {false, :scroll} -> nil
+        end
       end)
 
     case Enum.uniq(table_candidates ++ filter_goal_table_candidates) do

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

@@ -367,4 +367,226 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryGoalCustomPropsTest do
       refute Enum.find(results, &(&1["dimensions"] == ["Button Click"]))
     end
   end
+
+  describe "goals with custom props and imported data" do
+    setup :create_site_import
+
+    test "filtering by a goal with custom props excludes imported data instead of crashing", %{
+      conn: conn,
+      site: site,
+      site_import: site_import
+    } do
+      {:ok, _goal} =
+        Goals.create(site, %{
+          "event_name" => "Purchase",
+          "custom_props" => %{"variant" => "A"}
+        })
+
+      populate_stats(site, site_import.id, [
+        build(:event,
+          name: "Purchase",
+          "meta.key": ["variant"],
+          "meta.value": ["A"],
+          timestamp: ~N[2023-01-01 00:00:00]
+        ),
+        build(:imported_custom_events,
+          name: "Purchase",
+          visitors: 3,
+          events: 5,
+          date: ~D[2023-01-01]
+        )
+      ])
+
+      conn =
+        post(conn, "/api/v2/query", %{
+          "site_id" => site.domain,
+          "date_range" => "all",
+          "metrics" => ["visitors", "events"],
+          "filters" => [["is", "event:goal", ["Purchase"]]],
+          "include" => %{"imports" => true}
+        })
+
+      resp = json_response(conn, 200)
+
+      # Imported data cannot be filtered by the goal's custom props,
+      # so it must be excluded from the query.
+      assert resp["results"] == [%{"dimensions" => [], "metrics" => [1, 1]}]
+      refute resp["meta"]["imports_included"]
+    end
+
+    test "breakdown by event:goal does not attribute imported events to a goal with custom props",
+         %{
+           conn: conn,
+           site: site,
+           site_import: site_import
+         } do
+      {:ok, _goal} =
+        Goals.create(site, %{
+          "event_name" => "Purchase",
+          "custom_props" => %{"variant" => "A"}
+        })
+
+      populate_stats(site, site_import.id, [
+        build(:event,
+          name: "Purchase",
+          "meta.key": ["variant"],
+          "meta.value": ["A"],
+          timestamp: ~N[2023-01-01 00:00:00]
+        ),
+        build(:imported_custom_events,
+          name: "Purchase",
+          visitors: 3,
+          events: 5,
+          date: ~D[2023-01-01]
+        )
+      ])
+
+      conn =
+        post(conn, "/api/v2/query", %{
+          "site_id" => site.domain,
+          "date_range" => "all",
+          "metrics" => ["visitors", "events"],
+          "dimensions" => ["event:goal"],
+          "include" => %{"imports" => true}
+        })
+
+      resp = json_response(conn, 200)
+
+      # Imported events are aggregated by name only and cannot be checked
+      # against the goal's custom props, so they must not be counted.
+      assert resp["results"] == [%{"dimensions" => ["Purchase"], "metrics" => [1, 1]}]
+    end
+
+    test "breakdown by event:goal filtered by a goal with custom props excludes imported data instead of crashing",
+         %{
+           conn: conn,
+           site: site,
+           site_import: site_import
+         } do
+      {:ok, _goal} =
+        Goals.create(site, %{
+          "event_name" => "Purchase",
+          "custom_props" => %{"variant" => "A"}
+        })
+
+      populate_stats(site, site_import.id, [
+        build(:event,
+          name: "Purchase",
+          "meta.key": ["variant"],
+          "meta.value": ["A"],
+          timestamp: ~N[2023-01-01 00:00:00]
+        ),
+        build(:imported_custom_events,
+          name: "Purchase",
+          visitors: 3,
+          events: 5,
+          date: ~D[2023-01-01]
+        )
+      ])
+
+      conn =
+        post(conn, "/api/v2/query", %{
+          "site_id" => site.domain,
+          "date_range" => "all",
+          "metrics" => ["visitors", "events"],
+          "dimensions" => ["event:goal"],
+          "filters" => [["is", "event:goal", ["Purchase"]]],
+          "include" => %{"imports" => true}
+        })
+
+      resp = json_response(conn, 200)
+
+      assert resp["results"] == [%{"dimensions" => ["Purchase"], "metrics" => [1, 1]}]
+      refute resp["meta"]["imports_included"]
+    end
+
+    test "filtering by a custom property and a special goal with custom props excludes imported data instead of crashing",
+         %{
+           conn: conn,
+           site: site,
+           site_import: site_import
+         } do
+      {:ok, _goal} =
+        Goals.create(site, %{
+          "event_name" => "Outbound Link: Click",
+          "custom_props" => %{"page_theme" => "dark"}
+        })
+
+      populate_stats(site, site_import.id, [
+        build(:event,
+          name: "Outbound Link: Click",
+          "meta.key": ["url", "page_theme"],
+          "meta.value": ["https://example.com", "dark"],
+          timestamp: ~N[2023-01-01 00:00:00]
+        ),
+        build(:imported_custom_events,
+          name: "Outbound Link: Click",
+          link_url: "https://example.com",
+          visitors: 3,
+          events: 5,
+          date: ~D[2023-01-01]
+        )
+      ])
+
+      conn =
+        post(conn, "/api/v2/query", %{
+          "site_id" => site.domain,
+          "date_range" => "all",
+          "metrics" => ["visitors", "events"],
+          "filters" => [
+            ["is", "event:goal", ["Outbound Link: Click"]],
+            ["is", "event:props:url", ["https://example.com"]]
+          ],
+          "include" => %{"imports" => true}
+        })
+
+      resp = json_response(conn, 200)
+
+      assert resp["results"] == [%{"dimensions" => [], "metrics" => [1, 1]}]
+      refute resp["meta"]["imports_included"]
+    end
+
+    test "breakdown by event:goal does not attribute imported pageviews to a page goal with custom props",
+         %{
+           conn: conn,
+           site: site,
+           site_import: site_import
+         } do
+      {:ok, _goal} =
+        Goals.create(site, %{
+          "page_path" => "/checkout",
+          "custom_props" => %{"variant" => "A"}
+        })
+
+      populate_stats(site, site_import.id, [
+        build(:pageview,
+          pathname: "/checkout",
+          "meta.key": ["variant"],
+          "meta.value": ["A"],
+          timestamp: ~N[2023-01-01 00:00:00]
+        ),
+        build(:imported_pages,
+          page: "/checkout",
+          visitors: 3,
+          pageviews: 5,
+          date: ~D[2023-01-01]
+        )
+      ])
+
+      conn =
+        post(conn, "/api/v2/query", %{
+          "site_id" => site.domain,
+          "date_range" => "all",
+          "metrics" => ["visitors"],
+          "dimensions" => ["event:goal"],
+          "include" => %{"imports" => true}
+        })
+
+      resp = json_response(conn, 200)
+
+      # Imported pageviews are aggregated by page only and cannot be checked
+      # against the goal's custom props, so they must not be counted.
+      assert resp["results"] == [%{"dimensions" => ["Visit /checkout"], "metrics" => [1]}]
+    end
+  end
 end