fix: handle subquery-requiring calculations in calculate/2

documentation/dsls/DSL:-Ash.Resource.md

@@ -1528,7 +1528,7 @@ update :flag_for_review, primary?: true
 |------|------|---------|------|
 | [`manual`](#actions-update-manual){: #actions-update-manual } | `(any, any -> any) \| module` |  | Override the update behavior. Accepts a module or module and opts, or a function that takes the changeset and context. See the [manual actions guide](/documentation/topics/manual-actions.md) for more. |
 | [`require_atomic?`](#actions-update-require_atomic?){: #actions-update-require_atomic? } | `boolean` | `true` | Require that the update be atomic. This means that all changes and validations implement the `atomic` callback. See the guide on atomic updates for more. |
-| [`atomic_upgrade?`](#actions-update-atomic_upgrade?){: #actions-update-atomic_upgrade? } | `boolean` | `true` | If set to `true`, atomic upgrades will be performed. See the update actions guide for more. |
+| [`atomic_upgrade?`](#actions-update-atomic_upgrade?){: #actions-update-atomic_upgrade? } | `boolean` | `false` | If set to `true`, atomic upgrades will be performed. Ignored if `required_atomic?` is `true`. See the update actions guide for more. |
 | [`atomic_upgrade_with`](#actions-update-atomic_upgrade_with){: #actions-update-atomic_upgrade_with } | `:atom \| nil` |  | Configure the read action used when performing atomic upgrades. Defaults to the primary read action. |
 | [`primary?`](#actions-update-primary?){: #actions-update-primary? } | `boolean` | `false` | Whether or not this action should be used when no action is specified by the caller. |
 | [`description`](#actions-update-description){: #actions-update-description } | `String.t` |  | An optional description for the action |
@@ -1770,7 +1770,7 @@ end
 | [`soft?`](#actions-destroy-soft?){: #actions-destroy-soft? } | `boolean` | `false` | If specified, the destroy action behaves as an update internally |
 | [`manual`](#actions-destroy-manual){: #actions-destroy-manual } | `(any, any -> any) \| module` |  | Override the update behavior. Accepts a module or module and opts, or a function that takes the changeset and context. See the [manual actions guide](/documentation/topics/manual-actions.md) for more. |
 | [`require_atomic?`](#actions-destroy-require_atomic?){: #actions-destroy-require_atomic? } | `boolean` | `true` | Require that the update be atomic. Only relevant if `soft?` is set to `true`. This means that all changes and validations implement the `atomic` callback. See the guide on atomic updates for more. |
-| [`atomic_upgrade?`](#actions-destroy-atomic_upgrade?){: #actions-destroy-atomic_upgrade? } | `boolean` | `true` | If set to `true`, atomic upgrades will be performed. See the update actions guide for more. |
+| [`atomic_upgrade?`](#actions-destroy-atomic_upgrade?){: #actions-destroy-atomic_upgrade? } | `boolean` | `false` | If set to `true`, atomic upgrades will be performed. See the update actions guide for more. |
 | [`atomic_upgrade_with`](#actions-destroy-atomic_upgrade_with){: #actions-destroy-atomic_upgrade_with } | `:atom \| nil` |  | Configure the read action used when performing atomic upgrades. Defaults to the primary read action. |
 | [`primary?`](#actions-destroy-primary?){: #actions-destroy-primary? } | `boolean` | `false` | Whether or not this action should be used when no action is specified by the caller. |
 | [`description`](#actions-destroy-description){: #actions-destroy-description } | `String.t` |  | An optional description for the action |

lib/ash/actions/read/calculations.ex

@@ -31,6 +31,21 @@ defmodule Ash.Actions.Read.Calculations do
           end
         end)
 
+      primary_key =
+        case Ash.Resource.Info.primary_key(resource) do
+          [] ->
+            nil
+
+          primary_key ->
+            Enum.reduce_while(primary_key, %{}, fn key, acc ->
+              case Map.get(record, key) do
+                nil -> {:halt, nil}
+                %Ash.NotLoaded{} -> {:halt, nil}
+                other -> {:cont, Map.put(acc, key, other)}
+              end
+            end)
+        end
+
       calc_context =
         %Ash.Resource.Calculation.Context{
           actor: opts[:actor],
@@ -56,105 +71,109 @@ defmodule Ash.Actions.Read.Calculations do
         with {:ok, expr} <- expr,
              {:ok, expr} <-
                Ash.Filter.hydrate_refs(expr, %{resource: resource}) do
-          case Ash.Expr.eval(expr,
-                 record: record,
-                 resource: resource,
-                 unknown_on_unknown_refs?: true
-               ) do
-            {:ok, result} ->
-              {:ok, result}
+          if is_nil(primary_key) && requires_primary_key?(expr) do
+            {:error,
+             Ash.Error.Query.CalculationRequiresPrimaryKey.exception(
+               resource: resource,
+               calculation: calculation
+             )}
+          else
+            if !is_nil(primary_key) do
+              case Ash.load(record, [{calculation, arguments}],
+                     actor: opts[:actor],
+                     domain: opts[:domain],
+                     tenant: opts[:tenant],
+                     authorize?: opts[:authorize?],
+                     tracer: opts[:tracer],
+                     resource: opts[:resource],
+                     context: opts[:context] || %{}
+                   ) do
+                {:ok, record} -> {:ok, Map.get(record, calculation)}
+                {:error, error} -> {:error, error}
+              end
+            else
+              expr = replace_refs(expr, Keyword.put(opts, :record, record))
 
-            :unknown ->
-              expr =
-                Ash.Filter.map(expr, fn
-                  %Ash.Query.Ref{relationship_path: path, attribute: attribute} ->
-                    name =
-                      case attribute do
-                        %{name: name} -> name
-                        name -> name
-                      end
-
-                    get_in(opts[:refs] || %{}, path ++ [name])
-
-                  other ->
-                    other
-                end)
-
-              data_layer_result =
-                if Ash.DataLayer.data_layer_can?(resource, :calculate) do
-                  Ash.DataLayer.calculate(resource, [expr], opts[:context] || %{})
-                else
-                  :cant_calculate
+              evaled =
+                try do
+                  Ash.Expr.eval(expr,
+                    record: record,
+                    resource: resource,
+                    unknown_on_unknown_refs?: true
+                  )
+                rescue
+                  _ ->
+                    :unknown
                 end
 
-              case data_layer_result do
+              case evaled do
                 {:ok, result} ->
-                  {:ok, Enum.at(result, 0)}
+                  {:ok, result}
 
-                :cant_calculate ->
-                  {:error,
-                   "Failed to run calculation in memory, or in the data layer, and no `calculate/3` is defined on #{inspect(module)}. Data layer does not support one-off calculations."}
+                :unknown ->
+                  data_layer_result =
+                    if Ash.DataLayer.data_layer_can?(resource, :calculate) do
+                      Ash.DataLayer.calculate(resource, [expr], %{
+                        calculation_context: calc_context,
+                        primary_key: primary_key
+                      })
+                    else
+                      :cant_calculate
+                    end
+
+                  case data_layer_result do
+                    {:ok, result} ->
+                      {:ok, Enum.at(result, 0)}
+
+                    :cant_calculate ->
+                      {:error,
+                       "Failed to run calculation in memory, or in the data layer, and no `calculate/3` is defined on #{inspect(module)}. Data layer does not support one-off calculations."}
+
+                    {:error, error} ->
+                      if module.has_calculate?() do
+                        case module.calculate([record], calc_opts, calc_context) do
+                          [result] ->
+                            result
+
+                          {:ok, [result]} ->
+                            {:ok, result}
+
+                          {:ok, _} ->
+                            {:error, "Invalid calculation return"}
+
+                          {:error, error} ->
+                            {:error, error}
+
+                          :unknown ->
+                            {:error,
+                             "Failed to run calculation in memory, or in the data layer. Data layer returned #{inspect(error)}"}
+                        end
+                      else
+                        {:error,
+                         "Failed to run calculation in memory, or in the data layer, and no `calculate/3` is defined on #{inspect(module)}. Data layer returned #{inspect(error)}"}
+                      end
+                  end
 
                 {:error, error} ->
-                  if module.has_calculate?() do
-                    case module.calculate([record], calc_opts, calc_context) do
-                      [result] ->
-                        result
-
-                      {:ok, [result]} ->
-                        {:ok, result}
-
-                      {:ok, _} ->
-                        {:error, "Invalid calculation return"}
-
-                      {:error, error} ->
-                        {:error, error}
-
-                      :unknown ->
-                        {:error,
-                         "Failed to run calculation in memory, or in the data layer. Data layer returned #{inspect(error)}"}
-                    end
-                  else
-                    {:error,
-                     "Failed to run calculation in memory, or in the data layer, and no `calculate/3` is defined on #{inspect(module)}. Data layer returned #{inspect(error)}"}
-                  end
+                  {:error, error}
               end
-
-            {:error, error} ->
-              {:error, error}
+            end
           end
         end
       else
-        primary_key = Ash.Resource.Info.primary_key(resource)
-
         if module.has_calculate?() do
-          if Enum.all?(primary_key, &(not is_nil(Map.get(record, &1)))) do
-            case Ash.load(record, [{calculation, arguments}],
-                   actor: opts[:actor],
-                   domain: opts[:domain],
-                   tenant: opts[:tenant],
-                   authorize?: opts[:authorize?],
-                   tracer: opts[:tracer],
-                   resource: opts[:resource],
-                   context: opts[:context] || %{}
-                 ) do
-              {:ok, record} -> {:ok, Map.get(record, calculation)}
-              {:error, error} -> {:error, error}
-            end
-          else
-            case module.calculate([record], calc_opts, calc_context) do
-              [result] ->
-                {:ok, result}
+          case module.calculate([record], calc_opts, calc_context) do
+            [result] ->
+              {:ok, result}
 
-              {:ok, [result]} ->
-                {:ok, result}
+            {:ok, [result]} ->
+              {:ok, result}
 
-              {:ok, _} ->
-                {:error, "Invalid calculation return"}
+            {:ok, _} ->
+              {:error, "Invalid calculation return"}
 
-              {:error, error} ->
-                {:error, error}
-            end
+            {:error, error} ->
+              {:error, error}
           end
         else
           {:error, "Module #{inspect(module)} does not have an expression or calculate function"}
@@ -169,6 +188,53 @@ defmodule Ash.Actions.Read.Calculations do
     end
   end
 
+  defp requires_primary_key?(expr) do
+    Ash.Filter.find_value(expr, fn
+      %Ash.Query.Ref{attribute: %agg_struct{}}
+      when agg_struct in [Ash.Query.Aggregate, Ash.Resource.Aggregate] ->
+        true
+
+      %Ash.Query.Exists{} ->
+        true
+
+      %Ash.Query.Parent{} ->
+        true
+
+      _ ->
+        false
+    end) ||
+      false
+  end
+
+  defp replace_refs(expr, opts) do
+    Ash.Filter.map(expr, fn
+      %Ash.Query.Ref{relationship_path: path, attribute: %Ash.Resource.Attribute{} = attribute} ->
+        name =
+          case attribute do
+            %{name: name} -> name
+            name -> name
+          end
+
+        Ash.Expr.get_path(opts[:record] || %{}, path ++ [name])
+
+      %Ash.Query.Exists{expr: expr} = exists ->
+        %{
+          exists
+          | expr:
+              Ash.Filter.map(expr, fn
+                %Ash.Query.Parent{expr: expr} = parent ->
+                  %{parent | expr: replace_refs(expr, opts)}
+
+                other ->
+                  other
+              end)
+        }
+
+      other ->
+        other
+    end)
+  end
+
   def run([], _, _, _calculations_in_query), do: {:ok, []}
 
   def run(records, ash_query, calculations_at_runtime, calculations_in_query) do

lib/ash/error/query/calculation_requires_primary_key.ex

@@ -0,0 +1,21 @@
+defmodule Ash.Error.Query.CalculationRequiresPrimaryKey do
+  @moduledoc "Used when a calculation requires a primary key but was not supplied with one"
+  use Ash.Error.Exception
+
+  use Splode.Error, fields: [:resource, :calculation], class: :invalid
+
+  def message(error) do
+    identifier =
+      if String.Chars.impl_for(error.calculation) do
+        "#{inspect(error.resource)}.#{error.calculation}"
+      else
+        "#{inspect(error.resource)}.#{inspect(error.calculation)}"
+      end
+
+    """
+    Primary key is required for #{identifier}, as it uses aggregates or `exists` expressions.
+
+    In practice, this means accepting a record, or adding arguments for each key in the primary key.
+    """
+  end
+end

lib/ash/error/query/invalid_calculation_argument.ex

@@ -6,7 +6,7 @@ defmodule Ash.Error.Query.InvalidCalculationArgument do
 
   def message(error) do
     """
-    Invalid value provided for calculation argument #{error.field} in #{error.calculation}: #{do_message(error)}
+    Invalid value provided for calculation argument #{error.field} in #{error.calculation}#{do_message(error)}
 
     #{inspect(error.value)}
     """

lib/ash/expr/expr.ex

@@ -204,19 +204,20 @@ defmodule Ash.Expr do
     end)
   end
 
-  defp get_path(map, [key]) when is_struct(map) do
+  @doc false
+  def get_path(map, [key]) when is_struct(map) do
     Map.get(map, key)
   end
 
-  defp get_path(map, [key]) when is_map(map) do
+  def get_path(map, [key]) when is_map(map) do
     Map.get(map, key)
   end
 
-  defp get_path(map, [key | rest]) when is_map(map) do
+  def get_path(map, [key | rest]) when is_map(map) do
     get_path(get_path(map, [key]), rest)
   end
 
-  defp get_path(_, _), do: nil
+  def get_path(_, _), do: nil
 
   @doc false
   def template_references_actor?(template) do

lib/ash/filter/runtime.ex

@@ -258,6 +258,8 @@ defmodule Ash.Filter.Runtime do
   defp load_unflattened(record, []), do: record
   defp load_unflattened(nil, _), do: nil
 
+  defp load_unflattened(%Ash.NotLoaded{}, _), do: nil
+
   defp load_unflattened(records, path) when is_list(records) do
     Enum.map(records, &load_unflattened(&1, path))
   end

lib/ash/query/query.ex

@@ -1727,8 +1727,34 @@ defmodule Ash.Query do
                   expr(type(^casted, ^argument.type, ^argument.constraints))
                 )}}
             else
+            cond do
+            is_nil(casted) && argument.allow_nil? ->
+              {:cont, {:ok, Map.put(arg_values, argument.name, nil)}}
+
+            is_nil(casted) && is_nil(argument.default) ->
+              {:halt,
+               {:error,
+                InvalidCalculationArgument.exception(
+                  field: argument.name,
+                  calculation: calculation.name,
+                  message: "is required",
+                  value: value
+                )}}
+
+            is_nil(Map.get(args, argument.name, Map.get(args, to_string(argument.name)))) &&
+                not is_nil(value) ->
+                {:cont,
+                 {:ok,
+                  Map.put(
+                    arg_values,
+                    argument.name,
+                    value
+                  )}}
+              true ->
+
               {:cont, {:ok, Map.put(arg_values, argument.name, casted)}}
             end
+            end
           else
             {:error, error} when is_binary(error) ->
               {:halt,

test/code_interface_test.exs

@@ -81,7 +81,7 @@ defmodule Ash.Test.CodeInterfaceTest do
     calculations do
       calculate :full_name, :string, expr(first_name <> ^arg(:separator) <> last_name) do
         public?(true)
-        argument :separator, :string, default: " ", allow_nil?: false
+        argument :separator, :string, default: " ", allow_nil?: false, constraints: [allow_empty?: true, trim?: false]
       end
 
       calculate :full_name_functional,