From 3f977ff98d48bd331e2e2f22d61b5590ed9abce3 Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Sat, 27 Apr 2024 10:13:43 -0400 Subject: [PATCH] improvement: simplifications and clarifications around bulk callback behavior --- documentation/topics/resources/changes.md | 6 +++++- lib/ash/actions/create/bulk.ex | 12 ++++++++---- lib/ash/actions/destroy/bulk.ex | 6 +++++- lib/ash/actions/update/bulk.ex | 3 +++ lib/ash/policy/filter_check.ex | 23 +++++++++++++++-------- test/actions/bulk/bulk_create_test.exs | 8 ++++++++ test/actions/bulk/bulk_destroy_test.exs | 6 +++++- test/actions/bulk/bulk_update_test.exs | 7 ++++++- 8 files changed, 55 insertions(+), 16 deletions(-) diff --git a/documentation/topics/resources/changes.md b/documentation/topics/resources/changes.md index ae96a1cb..543b40c7 100644 --- a/documentation/topics/resources/changes.md +++ b/documentation/topics/resources/changes.md @@ -118,7 +118,7 @@ end ## Atomic Changes -To make a change atomic, you have to implement the `c:Ash.Resource.Change.atomic/3` callback. This callback returns a map of changes to attributes that should be changed atomically. We will also honor any `c:Ash.Resource.Change.after_batch/3` functionality to run code after atomic changes have been applied. +To make a change atomic, you have to implement the `c:Ash.Resource.Change.atomic/3` callback. This callback returns a map of changes to attributes that should be changed atomically. We will also honor any `c:Ash.Resource.Change.after_batch/3` functionality to run code after atomic changes have been applied (only if `atomic/3` callback has also been defined). Note that `c:Ash.Resource.Change.before_batch/3` is not supported in this scenario and will be ignored. ```elixir defmodule MyApp.Changes.Slugify do @@ -160,6 +160,10 @@ end Changes can support being run on batches of changesets, using the `c:Ash.Resource.Change.batch_change/3`, `c:Ash.Resource.Change.before_batch/3`, and `c:Ash.Resource.Change.after_batch/3` callbacks. +> ### batch_change/3 must be defined {: .warning} +> +> `c:Ash.Resource.Change.before_batch/3` must be defined for `c:Ash.Resource.Change.before_batch/3` and `c:Ash.Resource.Change.after_batch/3` to be called! + For some changes, this may not be necessary at all, i.e the `Slugify` example has no benefit from batching. If no batch callbacks are added, your change will be run on a loop over the changesets. For the sake of example, however, we will show what it might look like to implement batching for our `Slugify` example. ```elixir diff --git a/lib/ash/actions/create/bulk.ex b/lib/ash/actions/create/bulk.ex index abf91cc9..441a3087 100644 --- a/lib/ash/actions/create/bulk.ex +++ b/lib/ash/actions/create/bulk.ex @@ -687,7 +687,8 @@ defmodule Ash.Actions.Create.Bulk do all_changes |> Enum.filter(fn {%{change: {module, _opts}}, _} -> - function_exported?(module, :before_batch, 3) + function_exported?(module, :batch_change, 3) && + function_exported?(module, :before_batch, 3) _ -> false @@ -1264,7 +1265,8 @@ defmodule Ash.Actions.Create.Bulk do all_changes |> Enum.filter(fn {%{change: {module, change_opts}}, _} -> - function_exported?(module, :after_batch, 3) && + function_exported?(module, :batch_change, 3) && + function_exported?(module, :after_batch, 3) && module.batch_callbacks?(changesets, change_opts, context) _ -> @@ -1435,7 +1437,8 @@ defmodule Ash.Actions.Create.Bulk do Enum.any?(batch, fn item -> item.relationships not in [nil, %{}] || !Enum.empty?(item.after_action) end) || - (function_exported?(module, :after_batch, 3) && + (function_exported?(module, :batch_change, 3) && + function_exported?(module, :after_batch, 3) && module.batch_callbacks?(batch, change_opts, context)) %{ @@ -1486,7 +1489,8 @@ defmodule Ash.Actions.Create.Bulk do Enum.any?(batch, fn item -> item.relationships not in [nil, %{}] || !Enum.empty?(item.after_action) end) || - (function_exported?(module, :after_batch, 3) && + (function_exported?(module, :batch_change, 3) && + function_exported?(module, :after_batch, 3) && module.batch_callbacks?(batch, change_opts, context)) %{ diff --git a/lib/ash/actions/destroy/bulk.ex b/lib/ash/actions/destroy/bulk.ex index 05576411..dc7c16ee 100644 --- a/lib/ash/actions/destroy/bulk.ex +++ b/lib/ash/actions/destroy/bulk.ex @@ -412,7 +412,8 @@ defmodule Ash.Actions.Destroy.Bulk do |> Map.new() |> Map.put(:calculations, calculations) - with {:ok, query} <- authorize_bulk_query(query, atomic_changeset, opts), + with {:ok, query} <- + authorize_bulk_query(query, atomic_changeset, opts), {:ok, atomic_changeset, query} <- authorize_atomic_changeset(query, atomic_changeset, opts), {query, atomic_changeset} <- add_changeset_filters(query, atomic_changeset), @@ -1802,6 +1803,7 @@ defmodule Ash.Actions.Destroy.Bulk do |> Enum.filter(fn {%{change: {module, change_opts}}, _} -> function_exported?(module, :after_batch, 3) && + function_exported?(module, :batch_change, 3) && module.batch_callbacks?(changesets, change_opts, context) _ -> @@ -1960,6 +1962,7 @@ defmodule Ash.Actions.Destroy.Bulk do item.relationships not in [nil, %{}] || !Enum.empty?(item.after_action) end) || (function_exported?(module, :after_batch, 3) && + function_exported?(module, :batch_change, 3) && module.batch_callbacks?(batch, change_opts, context)) %{ @@ -2010,6 +2013,7 @@ defmodule Ash.Actions.Destroy.Bulk do item.relationships not in [nil, %{}] || !Enum.empty?(item.after_action) end) || (function_exported?(module, :after_batch, 3) && + function_exported?(module, :batch_change, 3) && module.batch_callbacks?(batch, change_opts, context)) %{ diff --git a/lib/ash/actions/update/bulk.ex b/lib/ash/actions/update/bulk.ex index b471ff28..2d7b1e49 100644 --- a/lib/ash/actions/update/bulk.ex +++ b/lib/ash/actions/update/bulk.ex @@ -2144,6 +2144,7 @@ defmodule Ash.Actions.Update.Bulk do |> Enum.filter(fn {%{change: {module, change_opts}}, _} -> function_exported?(module, :after_batch, 3) && + function_exported?(module, :batch_change, 3) && module.batch_callbacks?(changesets, change_opts, context) _ -> @@ -2314,6 +2315,7 @@ defmodule Ash.Actions.Update.Bulk do item.relationships not in [nil, %{}] || !Enum.empty?(item.after_action) end) || (function_exported?(module, :after_batch, 3) && + function_exported?(module, :batch_change, 3) && module.batch_callbacks?(batch, change_opts, context)) %{ @@ -2364,6 +2366,7 @@ defmodule Ash.Actions.Update.Bulk do item.relationships not in [nil, %{}] || !Enum.empty?(item.after_action) end) || (function_exported?(module, :after_batch, 3) && + function_exported?(module, :batch_change, 3) && module.batch_callbacks?(batch, change_opts, context)) %{ diff --git a/lib/ash/policy/filter_check.ex b/lib/ash/policy/filter_check.ex index cee4c22e..e335ee31 100644 --- a/lib/ash/policy/filter_check.ex +++ b/lib/ash/policy/filter_check.ex @@ -146,21 +146,28 @@ defmodule Ash.Policy.FilterCheck do public?: false }) do {:ok, hydrated} -> + opts = [ + resource: resource, + unknown_on_unknown_refs?: true + ] + # We don't want to authorize on stale data in real life # but when using utilities to check if something *will* be authorized # that is our intent - data = + opts = if changeset.context[:private][:pre_flight_authorization?] do - data + case data do + %Ash.Changeset.OriginalDataNotAvailable{reason: :atomic_query_destroy} -> + opts + + data -> + Keyword.put(opts, :record, data) + end else - nil + opts end - Ash.Expr.eval_hydrated(hydrated, - record: data, - resource: resource, - unknown_on_unknown_refs?: true - ) + Ash.Expr.eval_hydrated(hydrated, opts) {:error, error} -> {:error, error} diff --git a/test/actions/bulk/bulk_create_test.exs b/test/actions/bulk/bulk_create_test.exs index d24bdf44..6d935282 100644 --- a/test/actions/bulk/bulk_create_test.exs +++ b/test/actions/bulk/bulk_create_test.exs @@ -11,6 +11,10 @@ defmodule Ash.Test.Actions.BulkCreateTest do changeset end + def batch_change(changesets, _, _) do + changesets + end + def after_batch(results, _, _) do Stream.map(results, fn {_changeset, result} -> {:ok, %{result | title: result.title <> "_after"}} @@ -25,6 +29,10 @@ defmodule Ash.Test.Actions.BulkCreateTest do changeset end + def batch_change(changesets, _, _) do + changesets + end + def before_batch(changesets, _, _) do changesets |> Stream.map(fn changeset -> diff --git a/test/actions/bulk/bulk_destroy_test.exs b/test/actions/bulk/bulk_destroy_test.exs index 455d9787..a5f51c31 100644 --- a/test/actions/bulk/bulk_destroy_test.exs +++ b/test/actions/bulk/bulk_destroy_test.exs @@ -8,12 +8,16 @@ defmodule Ash.Test.Actions.BulkDestroyTest do defmodule AddAfterToTitle do use Ash.Resource.Change - def change(changeset, _, %{bulk?: true}) do + def change(changeset, _, _) do changeset end def atomic(_, _, _), do: :ok + def batch_change(changesets, _, _) do + changesets + end + def after_batch(results, _, _) do Stream.map(results, fn {_changeset, result} -> {:ok, %{result | title: result.title <> "_after"}} diff --git a/test/actions/bulk/bulk_update_test.exs b/test/actions/bulk/bulk_update_test.exs index 49934021..3abbf1fa 100644 --- a/test/actions/bulk/bulk_update_test.exs +++ b/test/actions/bulk/bulk_update_test.exs @@ -8,10 +8,15 @@ defmodule Ash.Test.Actions.BulkUpdateTest do use Ash.Resource.Change @impl true - def change(changeset, _, %{bulk?: true}) do + def change(changeset, _, _) do changeset end + @impl true + def batch_change(changesets, _opts, _context) do + changesets + end + @impl true def after_batch(results, _, _) do Stream.map(results, fn {_changeset, result} ->