fix: don't optimize equality into in expressions

Author: zachdaniel

lib/ash.ex

@@ -4047,4 +4047,15 @@ defmodule Ash do
 
   @doc false
   def stream_opt_keys, do: Keyword.keys(@stream_opts)
+
+  @doc false
+  def pkey_filter(records, pkey) do
+    case pkey do
+      [single_key] ->
+        [{single_key, [in: Enum.map(records, &Map.get(&1, single_key))]}]
+
+      composite_keys ->
+        [or: Enum.map(records, &Map.take(&1, composite_keys))]
+    end
+  end
 end

lib/ash/actions/destroy/bulk.ex

@@ -1002,7 +1002,7 @@ defmodule Ash.Actions.Destroy.Bulk do
       opts,
       ref,
       fn batch ->
-        pkeys = [or: Enum.map(batch, &Map.take(&1, pkey))]
+        pkeys = Ash.pkey_filter(batch, pkey)
 
         read_action = Ash.Actions.Update.Bulk.get_read_action(resource, action, opts).name
 

lib/ash/actions/read/read.ex

@@ -1700,18 +1700,12 @@ defmodule Ash.Actions.Read do
        """}
     else
       filter =
-        initial_data
-        |> List.wrap()
-        |> Enum.map(&Map.take(&1, primary_key))
-        |> case do
+        case List.wrap(initial_data) do
           [] ->
             false
 
-          [single] ->
-            [single]
-
-          multiple ->
-            [or: multiple]
+          records ->
+            Ash.pkey_filter(records, primary_key)
         end
 
       with %{valid?: true} = query <-

lib/ash/actions/update/bulk.ex

@@ -1298,7 +1298,7 @@ defmodule Ash.Actions.Update.Bulk do
       opts,
       ref,
       fn batch ->
-        pkeys = [or: Enum.map(batch, &Map.take(&1, pkey))]
+        pkeys = Ash.pkey_filter(batch, pkey)
 
         read_action = get_read_action(resource, action, opts).name
 

lib/ash/can.ex

@@ -878,15 +878,14 @@ defmodule Ash.Can do
           data = List.wrap(opts[:data])
 
           pkey = Ash.Resource.Info.primary_key(query.resource)
-          pkey_values = Enum.map(data, &Map.take(&1, pkey))
 
-          if Enum.any?(pkey_values, fn pkey_value ->
-               pkey_value |> Map.values() |> Enum.any?(&is_nil/1)
+          if Enum.any?(data, fn record ->
+               pkey |> Enum.map(&Map.get(record, &1)) |> Enum.any?(&is_nil/1)
              end) do
             {:ok, :maybe}
           else
             query
-            |> Ash.Query.do_filter(or: pkey_values)
+            |> Ash.Query.do_filter(Ash.pkey_filter(data, pkey))
             |> Ash.Query.select([])
             |> Ash.Query.set_tenant(tenant)
             |> Ash.Actions.Read.add_calc_context_to_query(

lib/ash/policy/filter_check.ex

@@ -354,11 +354,7 @@ defmodule Ash.Policy.FilterCheck do
       def check(actor, data, authorizer, opts) do
         pkey = Ash.Resource.Info.primary_key(authorizer.resource)
 
-        filter =
-          case data do
-            [record] -> Map.take(record, pkey)
-            records -> [or: Enum.map(data, &Map.take(&1, pkey))]
-          end
+        filter = Ash.pkey_filter(data, pkey)
 
         authorizer.resource
         |> Ash.Query.filter(^filter)

lib/ash/query/boolean_expression.ex

@@ -33,9 +33,8 @@ defmodule Ash.Query.BooleanExpression do
   # statements are contradictions. However, that would likely confuse users. For example:
   # `Ash.Query.filter(Resource, x == 1 and x in [2, 3])` becomes `#Ash.Query<filter: false>`
   # We may want to go down this route some day, but for now we simply use this to combine
-  # statements where possible, which helps with authorization logic that leverages the query
-  # For example, `x in [1, 2] or x == 3` becomes `x in [1, 2, 3]`, and `x in [1, 2, 3] and x != 1`
-  # becomes `x in [2, 3]`
+  # statements where possible, which helps with authorization logic that leverages the query.
+  # For example, `x in [1, 2, 3] and x != 1` becomes `x in [2, 3]`
   def optimized_new(_, nil, nil), do: false
   def optimized_new(:and, false, _), do: false
   def optimized_new(:and, _, false), do: false
@@ -94,18 +93,6 @@ defmodule Ash.Query.BooleanExpression do
     do_new(op, left, right)
   end
 
-  def optimized_new(
-        :or,
-        %Eq{left: left, right: value} = left_op,
-        %In{left: left, right: %MapSet{} = mapset} = right
-      ) do
-    if can_optimize?(value) do
-      %{right | right: MapSet.put(mapset, value)}
-    else
-      do_new(:or, left_op, right)
-    end
-  end
-
   def optimized_new(
         :and,
         %Eq{left: left, right: value} = left_expr,
@@ -142,18 +129,6 @@ defmodule Ash.Query.BooleanExpression do
     optimized_new(op, right, left)
   end
 
-  def optimized_new(
-        :or,
-        %Eq{left: left, right: left_value} = left_expr,
-        %Eq{left: left, right: right_value} = right_expr
-      ) do
-    if can_optimize?(left_value) && can_optimize?(right_value) do
-      %In{left: left, right: MapSet.new([left_value, right_value])}
-    else
-      do_new(:or, left_expr, right_expr)
-    end
-  end
-
   def optimized_new(
         :and,
         %Eq{left: left, right: left_value} = left_expr,
@@ -178,14 +153,6 @@ defmodule Ash.Query.BooleanExpression do
     end
   end
 
-  def optimized_new(
-        :or,
-        %In{left: left, right: %MapSet{} = left_values},
-        %In{left: left, right: %MapSet{} = right_values} = right_expr
-      ) do
-    %{right_expr | right: MapSet.union(left_values, right_values)}
-  end
-
   def optimized_new(
         :and,
         %In{left: left, right: left_values} = left_expr,
@@ -204,41 +171,6 @@ defmodule Ash.Query.BooleanExpression do
     end
   end
 
-  def optimized_new(
-        :or,
-        %__MODULE__{op: :or, left: left, right: right} = left_expr,
-        right_expr
-      ) do
-    case right_expr do
-      %In{} = in_op ->
-        with {:left, nil} <- {:left, Ash.Filter.find(left, &simplify?(&1, in_op), true, false)},
-             {:right, nil} <- {:right, Ash.Filter.find(right, &simplify?(&1, in_op), true, false)} do
-          do_new(:or, left_expr, in_op)
-        else
-          {:left, _} ->
-            %{left_expr | left: optimized_new(:or, left, in_op)}
-
-          {:right, _} ->
-            %{left_expr | right: optimized_new(:or, right, in_op)}
-        end
-
-      %Eq{} = eq_op ->
-        with {:left, nil} <- {:left, Ash.Filter.find(left, &simplify?(&1, eq_op), true, false)},
-             {:right, nil} <- {:right, Ash.Filter.find(right, &simplify?(&1, eq_op), true, false)} do
-          do_new(:or, left_expr, eq_op)
-        else
-          {:left, _} ->
-            %{left_expr | left: optimized_new(:or, left, eq_op)}
-
-          {:right, _} ->
-            %{left_expr | right: optimized_new(:or, right, eq_op)}
-        end
-
-      _ ->
-        do_new(:or, left_expr, right_expr)
-    end
-  end
-
   def optimized_new(
         :and,
         %__MODULE__{op: :and, left: left, right: right} = left_expr,

test/filter/filter_test.exs

@@ -298,25 +298,25 @@ defmodule Ash.Test.Filter.FilterTest do
   describe "predicate optimization" do
     # Testing against the stringified query may be a bad idea, but its a quick win and we
     # can switch to actually checking the structure if this bites us
-    test "equality simplifies to `in`" do
+    test "equality does not simplify to `in`" do
       stringified_query =
         Post
         |> Ash.Query.filter(title == "foo" or title == "bar")
         |> inspect()
 
-      assert stringified_query =~ ~S(title in ["bar", "foo"])
+      assert stringified_query =~ ~S(title == "foo" or title == "bar")
     end
 
-    test "in with equality simplifies to `in`" do
+    test "in with equality does not simplify to `in`" do
       stringified_query =
         Post
         |> Ash.Query.filter(title in ["foo", "bar", "baz"] or title == "bar")
         |> inspect()
 
-      assert stringified_query =~ ~S(title in ["bar", "baz", "foo"])
+      assert stringified_query =~ ~S(title == "bar" or title in ["bar", "baz", "foo"])
     end
 
-    test "multiple nested equality simplifies to `in`" do
+    test "multiple nested equality does not simplify to `in`" do
       import Ash.Expr
       keys = [:id]
 
@@ -340,7 +340,7 @@ defmodule Ash.Test.Filter.FilterTest do
         |> Ash.Query.filter(^expr)
         |> inspect()
 
-      assert stringified_query =~ ~S(id in ["bar", "baz", "buz", "foo"])
+      assert stringified_query =~ ~S(id == "foo" or id == "bar" or id == "baz" or id == "buz")
     end
 
     test "in across ands in ors isn't optimized" do
@@ -367,13 +367,13 @@ defmodule Ash.Test.Filter.FilterTest do
       assert stringified_query =~ ~S(title in ["baz", "foo"])
     end
 
-    test "in with or-in simplifies to `in`" do
+    test "in with or-in does not simplify to `in`" do
       stringified_query =
         Post
         |> Ash.Query.filter(title in ["foo", "bar"] or title in ["bar", "baz"])
         |> inspect()
 
-      assert stringified_query =~ ~S(title in ["bar", "baz", "foo"])
+      assert stringified_query =~ ~S(title in ["bar", "foo"] or title in ["bar", "baz"])
     end
 
     test "in with and-in simplifies to `in` when multiple values overlap" do