Behaviour return validation: validate callback return types across Ash behaviours

- Add Ash.BehaviourHelpers.call_and_validate_return for Notifier, Change, etc.
- Notifier.notify/1 must return :ok
- Change.atomic/3 validates return (e.g. {:not_atomic, String.t()})
- Fix test Notifiers to return :ok; fix test atomic changes to return {:not_atomic, reason}
- Add behaviour_return_validation_test.exs

Made-with: Cursor

Author: celeste-wahlquist

lib/ash/actions/create/bulk.ex

@@ -1959,7 +1959,7 @@ defmodule Ash.Actions.Create.Bulk do
                 end) ||
                 (module.has_batch_change?() &&
                    module.has_after_batch?() &&
-                   module.batch_callbacks?(batch, change_opts, context))
+                   Ash.Resource.Change.batch_callbacks?(module,batch, change_opts, context))
 
             %{
               state
@@ -2020,7 +2020,7 @@ defmodule Ash.Actions.Create.Bulk do
                   end) ||
                   (module.has_batch_change?() &&
                      module.has_after_batch?() &&
-                     module.batch_callbacks?(batch, change_opts, context))
+                     Ash.Resource.Change.batch_callbacks?(module,batch, change_opts, context))
 
               %{
                 state

lib/ash/actions/destroy/bulk.ex

@@ -324,7 +324,7 @@ defmodule Ash.Actions.Destroy.Bulk do
                   fn
                     %{change: {module, change_opts}} ->
                       module.has_after_batch?() &&
-                        module.batch_callbacks?(query, change_opts, context)
+                        Ash.Resource.Change.batch_callbacks?(module, query, change_opts, context)
 
                     _ ->
                       false

lib/ash/actions/helpers.ex

@@ -81,7 +81,7 @@ defmodule Ash.Actions.Helpers do
                     changeset
                   )
 
-                if mod.batch_callbacks?([changeset], change_opts, context) do
+                if Ash.Resource.Change.batch_callbacks?(mod, [changeset], change_opts, context) do
                   [%{change | change: {mod, change_opts}}]
                 else
                   []

lib/ash/actions/read/calculations.ex

@@ -80,7 +80,7 @@ defmodule Ash.Actions.Read.Calculations do
           source_context: opts[:context] || %{}
         }
 
-      if reuse_values? && module.has_expression?() do
+      if reuse_values? && Ash.Resource.Calculation.has_expression?(module) do
         expr =
           case Ash.Resource.Calculation.expression(module, calc_opts, calc_context) do
             {:ok, result} -> {:ok, result}
@@ -167,7 +167,7 @@ defmodule Ash.Actions.Read.Calculations do
                        "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
+                      if Ash.Resource.Calculation.has_calculate?(module) do
                         case with_trace(
                                fn ->
                                  Ash.Resource.Calculation.calculate(
@@ -236,7 +236,7 @@ defmodule Ash.Actions.Read.Calculations do
             {:error, error} -> {:error, error}
           end
         else
-          if module.has_calculate?() do
+          if Ash.Resource.Calculation.has_calculate?(module) do
             record =
               if primary_key do
                 Ash.load(
@@ -880,7 +880,7 @@ defmodule Ash.Actions.Read.Calculations do
       {name, []} ->
         relationship = Ash.Resource.Info.relationship(ash_query.resource, name)
 
-        if calculation.module.strict_loads?() do
+        if Ash.Resource.Calculation.strict_loads?(calculation.module) do
           []
         else
           query = Ash.Query.new(relationship.destination)
@@ -991,7 +991,7 @@ defmodule Ash.Actions.Read.Calculations do
       |> Map.values()
       |> Enum.reduce_while({:ok, [], [], ash_query}, fn calculation,
                                                         {:ok, in_query, at_runtime, ash_query} ->
-        if calculation.module.has_expression?() do
+        if Ash.Resource.Calculation.has_expression?(calculation.module) do
           expression =
             calculation.opts
             |> Ash.Expr.fill_template(
@@ -1043,7 +1043,7 @@ defmodule Ash.Actions.Read.Calculations do
                     {:halt, {:error, error}}
                 end
               else
-                if calculation.module.has_calculate?() do
+                if Ash.Resource.Calculation.has_calculate?(calculation.module) do
                   {:cont, {:ok, in_query, [calculation | at_runtime], ash_query}}
                 else
                   {:cont,
@@ -1094,7 +1094,7 @@ defmodule Ash.Actions.Read.Calculations do
     end
     |> if do
       if reuse_values? && calculation.module != Ash.Resource.Calculation.Expression &&
-           calculation.module.has_calculate?() do
+           Ash.Resource.Calculation.has_calculate?(calculation.module) do
         {:ok, calculation}
       else
         Enum.reduce_while(initial_data, {:ok, []}, fn record, {:ok, results} ->
@@ -1163,7 +1163,7 @@ defmodule Ash.Actions.Read.Calculations do
   end
 
   defp should_be_in_expression?(calculation, expression \\ nil, ash_query) do
-    if calculation.module.has_expression?() do
+    if Ash.Resource.Calculation.has_expression?(calculation.module) do
       case Map.fetch(calculation.context, :should_be_in_expression?) do
         {:ok, value} ->
           {:ok, value}
@@ -1199,7 +1199,7 @@ defmodule Ash.Actions.Read.Calculations do
               expression
               |> Ash.Filter.used_calculations(ash_query.resource, :*)
               |> Enum.all?(fn %{module: module} ->
-                module.has_expression?()
+                Ash.Resource.Calculation.has_expression?(module)
               end)
               |> then(&{:ok, &1})
 
@@ -1230,7 +1230,7 @@ defmodule Ash.Actions.Read.Calculations do
     if {calculation.module, calculation.opts} in checked_calculations do
       query
     else
-      has_expression? = calculation.module.has_expression?()
+      has_expression? = Ash.Resource.Calculation.has_expression?(calculation.module)
 
       should_be_in_expression_result =
         if has_expression? do
@@ -1277,7 +1277,7 @@ defmodule Ash.Actions.Read.Calculations do
             calculation.name,
             calculation.load,
             calc_path,
-            calculation.module.strict_loads?(),
+            Ash.Resource.Calculation.strict_loads?(calculation.module),
             relationship_path,
             can_expression_calculation?,
             checked_calculations,
@@ -2020,7 +2020,7 @@ defmodule Ash.Actions.Read.Calculations do
 
   defp find_equivalent_calculation(query, calculation, authorize?) do
     reusable? =
-      if authorize? && calculation.module.has_expression?() do
+      if authorize? && Ash.Resource.Calculation.has_expression?(calculation.module) do
         Ash.Resource.Calculation.expression(
           calculation.module,
           calculation.opts,

lib/ash/actions/read/read.ex

@@ -2027,7 +2027,7 @@ defmodule Ash.Actions.Read do
 
           case aggregate.field do
             %Ash.Query.Calculation{} = calculation ->
-              if calculation.module.has_expression?() and
+              if Ash.Resource.Calculation.has_expression?(calculation.module) and
                    Ash.DataLayer.data_layer_can?(query.resource, :expression_calculation) do
                 expression =
                   Ash.Resource.Calculation.expression(
@@ -2476,7 +2476,7 @@ defmodule Ash.Actions.Read do
       } = ref ->
         calc = add_calc_context(calc, actor, authorize?, tenant, tracer, domain, resource, opts)
 
-        if Keyword.get(opts, :expand?, false) && calc.module.has_expression?() do
+        if Keyword.get(opts, :expand?, false) && Ash.Resource.Calculation.has_expression?(calc.module) do
           expr =
             case Ash.Resource.Calculation.expression(calc.module, calc.opts, calc.context) do
               %Ash.Query.Function.Type{} = expr ->
@@ -2627,7 +2627,7 @@ defmodule Ash.Actions.Read do
   defp should_expand_expression?(struct, calc, opts) do
     struct == Ash.Query.Calculation &&
       Keyword.get(opts, :expand?, true) &&
-      calc.module.has_expression?()
+      Ash.Resource.Calculation.has_expression?(calc.module)
   end
 
   defp expand_expression(calc, resource, parent_stack, first_combination) do
@@ -4594,7 +4594,7 @@ defmodule Ash.Actions.Read do
 
     related_resource = Ash.Resource.Info.related(agg.resource, agg.relationship_path)
 
-    if calc.module.has_expression?() do
+    if Ash.Resource.Calculation.has_expression?(calc.module) do
       expr =
         case Ash.Resource.Calculation.expression(calc.module, calc.opts, calc.context) do
           %Ash.Query.Function.Type{} = expr ->

lib/ash/actions/read/relationships.ex

@@ -1815,7 +1815,7 @@ defmodule Ash.Actions.Read.Relationships do
               field: %Ash.Query.Calculation{module: module, opts: opts, context: context}
             }
           } ->
-            if module.has_expression?() do
+            if Ash.Resource.Calculation.has_expression?(module) do
               Ash.Resource.Calculation.expression(module, opts, context)
               |> do_has_parent_expr?(depth + 1)
             else

lib/ash/actions/update/bulk.ex

@@ -323,7 +323,7 @@ defmodule Ash.Actions.Update.Bulk do
                   fn
                     %{change: {module, change_opts}} ->
                       module.has_after_batch?() &&
-                        module.batch_callbacks?(query, change_opts, context)
+                        Ash.Resource.Change.batch_callbacks?(module,query, change_opts, context)
 
                     _ ->
                       false
@@ -1157,7 +1157,7 @@ defmodule Ash.Actions.Update.Bulk do
       |> Enum.filter(fn
         {%{change: {module, change_opts}}, _index} ->
           module.has_after_batch?() &&
-            module.batch_callbacks?(query, change_opts, context)
+            Ash.Resource.Change.batch_callbacks?(module,query, change_opts, context)
 
         _ ->
           false
@@ -2406,7 +2406,7 @@ defmodule Ash.Actions.Update.Bulk do
           case change_opts do
             {:templated, change_opts} ->
               if module.has_before_batch?() && module.has_batch_change?() &&
-                   module.batch_callbacks?(batch, change_opts, context) do
+                   Ash.Resource.Change.batch_callbacks?(module,batch, change_opts, context) do
                 Ash.Resource.Change.before_batch(
                   module,
                   batch,
@@ -2420,7 +2420,7 @@ defmodule Ash.Actions.Update.Bulk do
             change_opts ->
               Enum.flat_map(batch, fn changeset ->
                 if module.has_before_batch?() && module.has_batch_change?() &&
-                     module.batch_callbacks?(batch, change_opts, context) do
+                     Ash.Resource.Change.batch_callbacks?(module,batch, change_opts, context) do
                   change_opts =
                     templated_opts(
                       change_opts,
@@ -2458,7 +2458,7 @@ defmodule Ash.Actions.Update.Bulk do
               {:templated, change_opts} ->
                 if module.has_before_batch?() &&
                      module.has_batch_change?() &&
-                     module.batch_callbacks?(matches, change_opts, context) do
+                     Ash.Resource.Change.batch_callbacks?(module,matches, change_opts, context) do
                   Ash.Resource.Change.before_batch(
                     module,
                     matches,
@@ -2483,7 +2483,7 @@ defmodule Ash.Actions.Update.Bulk do
 
                   if module.has_before_batch?() &&
                        module.has_batch_change?() &&
-                       module.batch_callbacks?([changeset], change_opts, context) do
+                       Ash.Resource.Change.batch_callbacks?(module,[changeset], change_opts, context) do
                     Ash.Resource.Change.before_batch(
                       module,
                       [changeset],
@@ -3249,7 +3249,7 @@ defmodule Ash.Actions.Update.Bulk do
             {:templated, change_opts} ->
               if module.has_after_batch?() &&
                    module.has_batch_change?() &&
-                   module.batch_callbacks?(changesets, change_opts, context) do
+                   Ash.Resource.Change.batch_callbacks?(module,changesets, change_opts, context) do
                 Ash.Resource.Change.after_batch(
                   module,
                   results_for_callback,
@@ -3292,7 +3292,7 @@ defmodule Ash.Actions.Update.Bulk do
 
                 if module.has_after_batch?() &&
                      module.has_batch_change?() &&
-                     module.batch_callbacks?(changesets, change_opts, context) do
+                     Ash.Resource.Change.batch_callbacks?(module,changesets, change_opts, context) do
                   Ash.Resource.Change.after_batch(
                     module,
                     [{changeset, record}],
@@ -3340,7 +3340,7 @@ defmodule Ash.Actions.Update.Bulk do
               {:templated, change_opts} ->
                 if module.has_after_batch?() &&
                      module.has_batch_change?() &&
-                     module.batch_callbacks?(changesets, change_opts, context) do
+                     Ash.Resource.Change.batch_callbacks?(module,changesets, change_opts, context) do
                   Ash.Resource.Change.after_batch(
                     module,
                     matches_for_callback,
@@ -3383,7 +3383,7 @@ defmodule Ash.Actions.Update.Bulk do
                 Enum.flat_map(matches_for_callback, fn {changeset, record} = result ->
                   if module.has_after_batch?() &&
                        module.has_batch_change?() &&
-                       module.batch_callbacks?(changesets, change_opts, context) do
+                       Ash.Resource.Change.batch_callbacks?(module,changesets, change_opts, context) do
                     change_opts =
                       templated_opts(
                         change_opts,
@@ -3597,7 +3597,7 @@ defmodule Ash.Actions.Update.Bulk do
                 end) ||
                 (module.has_after_batch?() &&
                    module.has_batch_change?() &&
-                   module.batch_callbacks?(batch, change_opts, context))
+                   Ash.Resource.Change.batch_callbacks?(module,batch, change_opts, context))
 
             match_indices =
               if Enum.empty?(non_matches) do
@@ -3729,7 +3729,7 @@ defmodule Ash.Actions.Update.Bulk do
       {:templated, change_opts} ->
         cond do
           !must_be_atomic? && module.has_batch_change?() &&
-              module.batch_callbacks?(batch, change_opts, context) ->
+              Ash.Resource.Change.batch_callbacks?(module,batch, change_opts, context) ->
             {:ok, change_opts} = Ash.Resource.Change.init(module, change_opts)
             Ash.Resource.Change.batch_change(module, batch, change_opts, context)
 
@@ -3800,7 +3800,7 @@ defmodule Ash.Actions.Update.Bulk do
         cond do
           !must_be_atomic? && module.has_batch_change?() ->
             Enum.flat_map(batch, fn changeset ->
-              if module.batch_callbacks?(batch, change_opts, context) do
+              if Ash.Resource.Change.batch_callbacks?(module,batch, change_opts, context) do
                 change_opts =
                   templated_opts(
                     change_opts,

lib/ash/authorizer.ex

@@ -60,7 +60,7 @@ defmodule Ash.Authorizer do
           Ash.Domain.t()
         ) :: state
   def initial_state(module, actor, resource, action, domain) do
-    result = module.initial_state(actor, resource, action, domain)
+    result = apply(module, :initial_state, [actor, resource, action, domain])
 
     if is_map(result) do
       result
@@ -78,7 +78,7 @@ defmodule Ash.Authorizer do
   @spec exception(module(), atom(), state()) :: no_return
   def exception(module, reason, state) do
     if function_exported?(module, :exception, 2) do
-      module.exception(reason, state)
+      apply(module, :exception, [reason, state])
     else
       if reason == :must_pass_strict_check do
         raise Ash.Error.Forbidden.MustPassStrictCheck.exception([])
@@ -91,7 +91,7 @@ defmodule Ash.Authorizer do
   @doc false
   @spec strict_check_context(module(), state()) :: [atom()]
   def strict_check_context(module, state) do
-    result = module.strict_check_context(state)
+    result = apply(module, :strict_check_context, [state])
 
     if is_list(result) and Enum.all?(result, &is_atom/1) do
       Enum.uniq(result ++ [:query, :changeset])
@@ -190,18 +190,14 @@ defmodule Ash.Authorizer do
           {:ok, term()} | {:error, Ash.Error.t()}
   def alter_sort(module, state, sort, context) do
     if function_exported?(module, :alter_sort, 3) do
-      result = module.alter_sort(sort, state, context)
-
-      if match?({:ok, _}, result) or match?({:error, _}, result) do
-        result
-      else
-        raise Ash.Error.Framework.InvalidReturnType,
-          message: """
-          Invalid value returned from #{inspect(module)}.alter_sort/3.
-
-          The callback expects {:ok, sort} or {:error, Ash.Error.t()}.
-          """
-      end
+      Ash.BehaviourHelpers.call_and_validate_return(
+        module,
+        :alter_sort,
+        [sort, state, context],
+        [{:ok, :_}, {:error, :_}],
+        behaviour: __MODULE__,
+        callback_name: "alter_sort/3"
+      )
     else
       {:ok, sort}
     end
@@ -210,7 +206,7 @@ defmodule Ash.Authorizer do
   @doc false
   @spec check_context(module(), state()) :: [atom()]
   def check_context(module, state) do
-    result = module.check_context(state)
+    result = apply(module, :check_context, [state])
 
     if is_list(result) and Enum.all?(result, &is_atom/1) do
       result