APIv2: Bugfixes to location filtering (#4321)

* Fix filter ["contains", "visit:country", ["E"]]

This blew up since FixedString columns dont natively support matching functions for some reason

* Dont blow up for 3-letter country codes, instead fail validation

lib/plausible/stats/filters/query_parser.ex

@@ -90,15 +90,30 @@ defmodule Plausible.Stats.Filters.QueryParser do
        when operator in [:is, :is_not, :matches, :does_not_match, :contains, :does_not_contain],
        do: parse_clauses_list(filter)
 
-  defp parse_clauses_list([_operation, filter_key, list] = filter) when is_list(list) do
+  defp parse_clauses_list([operation, filter_key, list] = filter) when is_list(list) do
     all_strings? = Enum.all?(list, &is_binary/1)
     all_integers? = Enum.all?(list, &is_integer/1)
 
-    case {filter_key, all_strings?, all_integers?} do
-      {"event:goal", true, _} -> {:ok, [Filters.Utils.wrap_goal_value(list)]}
-      {"visit:city", _, true} -> {:ok, [list]}
-      {_, true, _} -> {:ok, [list]}
-      _ -> {:error, "Invalid filter '#{inspect(filter)}'"}
+    case {filter_key, all_strings?} do
+      {"event:goal", true} ->
+        {:ok, [Filters.Utils.wrap_goal_value(list)]}
+
+      {"visit:city", false} when all_integers? ->
+        {:ok, [list]}
+
+      {"visit:country", true} when operation in ["is", "is_not"] ->
+        if Enum.all?(list, &(String.length(&1) == 2)) do
+          {:ok, [list]}
+        else
+          {:error,
+           "Invalid visit:country filter, visit:country needs to be a valid 2-letter country code"}
+        end
+
+      {_, true} ->
+        {:ok, [list]}
+
+      _ ->
+        {:error, "Invalid filter '#{inspect(filter)}'"}
     end
   end
 

lib/plausible/stats/sql/where_builder.ex

@@ -231,20 +231,31 @@ defmodule Plausible.Stats.SQL.WhereBuilder do
 
   defp filter_field(db_field, [:matches, _key, glob_exprs]) do
     page_regexes = Enum.map(glob_exprs, &page_regex/1)
-    dynamic([x], fragment("multiMatchAny(?, ?)", field(x, ^db_field), ^page_regexes))
+
+    dynamic(
+      [x],
+      fragment("multiMatchAny(?, ?)", type(field(x, ^db_field), :string), ^page_regexes)
+    )
   end
 
   defp filter_field(db_field, [:does_not_match, _key, glob_exprs]) do
     page_regexes = Enum.map(glob_exprs, &page_regex/1)
-    dynamic([x], fragment("not(multiMatchAny(?, ?))", field(x, ^db_field), ^page_regexes))
+
+    dynamic(
+      [x],
+      fragment("not(multiMatchAny(?, ?))", type(field(x, ^db_field), :string), ^page_regexes)
+    )
   end
 
   defp filter_field(db_field, [:contains, _key, values]) do
-    dynamic([x], fragment("multiSearchAny(?, ?)", field(x, ^db_field), ^values))
+    dynamic([x], fragment("multiSearchAny(?, ?)", type(field(x, ^db_field), :string), ^values))
   end
 
   defp filter_field(db_field, [:does_not_contain, _key, values]) do
-    dynamic([x], fragment("not(multiSearchAny(?, ?))", field(x, ^db_field), ^values))
+    dynamic(
+      [x],
+      fragment("not(multiSearchAny(?, ?))", type(field(x, ^db_field), :string), ^values)
+    )
   end
 
   defp filter_field(db_field, [:is, _key, list]) do

test/plausible/stats/query_parser_test.exs

@@ -197,14 +197,14 @@ defmodule Plausible.Stats.Filters.QueryParserTest do
           "metrics" => ["visitors"],
           "date_range" => "all",
           "filters" => [
-            ["is", "visit:#{unquote(dimension)}", ["foo"]]
+            ["is", "visit:#{unquote(dimension)}", ["ab"]]
           ]
         }
         |> check_success(site, %{
           metrics: [:visitors],
           date_range: @date_range,
           filters: [
-            [:is, "visit:#{unquote(dimension)}", ["foo"]]
+            [:is, "visit:#{unquote(dimension)}", ["ab"]]
           ],
           dimensions: [],
           order_by: nil,
@@ -292,6 +292,18 @@ defmodule Plausible.Stats.Filters.QueryParserTest do
         preloaded_goals: []
       })
     end
+
+    test "invalid visit:country filter", %{site: site} do
+      %{
+        "metrics" => ["visitors"],
+        "date_range" => "all",
+        "filters" => [["is", "visit:country", ["USA"]]]
+      }
+      |> check_error(
+        site,
+        ~r/Invalid visit:country filter, visit:country needs to be a valid 2-letter country code/
+      )
+    end
   end
 
   describe "include validation" do

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

@@ -814,7 +814,7 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do
       assert json_response(conn, 200)["results"] == [%{"metrics" => [3], "dimensions" => []}]
     end
 
-    test "handles filtering by visit country", %{
+    test "handles filtering by visit:country", %{
       conn: conn,
       site: site
     } do
@@ -835,6 +835,28 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryTest do
       assert json_response(conn, 200)["results"] == [%{"metrics" => [3], "dimensions" => []}]
     end
 
+    test "handles filtering by visit:country with contains", %{
+      conn: conn,
+      site: site
+    } do
+      populate_stats(site, [
+        build(:pageview, country_code: "EE"),
+        build(:pageview, country_code: "EE"),
+        build(:pageview, country_code: "IT"),
+        build(:pageview, country_code: "DE")
+      ])
+
+      conn =
+        post(conn, "/api/v2/query", %{
+          "site_id" => site.domain,
+          "date_range" => "all",
+          "metrics" => ["pageviews"],
+          "filters" => [["contains", "visit:country", ["E"]]]
+        })
+
+      assert json_response(conn, 200)["results"] == [%{"metrics" => [3], "dimensions" => []}]
+    end
+
     test "handles filtering by visit:city", %{
       conn: conn,
       site: site

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

@@ -230,5 +230,30 @@ defmodule PlausibleWeb.Api.ExternalStatsController.QueryValidationsTest do
                "error" => "Metrics cannot be queried multiple times"
              }
     end
+
+    test "handles filtering by visit:country with invalid country code", %{
+      conn: conn,
+      site: site
+    } do
+      populate_stats(site, [
+        build(:pageview, country_code: "EE"),
+        build(:pageview, country_code: "EE"),
+        build(:pageview, country_code: "IT"),
+        build(:pageview, country_code: "DE")
+      ])
+
+      conn =
+        post(conn, "/api/v2/query", %{
+          "site_id" => site.domain,
+          "date_range" => "all",
+          "metrics" => ["pageviews"],
+          "filters" => [["is", "visit:country", ["USA"]]]
+        })
+
+      assert json_response(conn, 400) == %{
+               "error" =>
+                 "Invalid visit:country filter, visit:country needs to be a valid 2-letter country code"
+             }
+    end
   end
 end