fix: don't allow modifying changeset with atomic conditions

this fixes a bug where by some conditions on changes were not being
honored. This may cause a problem for certiain specific formulations
of changes plus conditions right now, but the current behavior cannot
be kept. We will likely need a way to return after_action hooks that
will be called conditionally if the expression returns true or false
when placed as a calcualtion into the update. Not an easy change
unfortunately.

improvement: add `touching?` option to changing validation

Author: zachdaniel

lib/ash/changeset/changeset.ex

@@ -1022,21 +1022,25 @@ defmodule Ash.Changeset do
       )
 
     with {:ok, change_opts} <- module.init(change_opts),
-         {:atomic, changeset, atomic_changes, validations} <-
-           atomic_with_changeset(
-             module.atomic(changeset, change_opts, struct(Ash.Resource.Change.Context, context)),
-             changeset
-           ),
-         {:atomic, condition} <- atomic_condition(where, changeset, context) do
-      changeset =
-        case condition do
-          true ->
-            apply_atomic_update(changeset, atomic_changes)
+         {:atomic, condition} <-
+           atomic_condition(where, changeset, context),
+         {{:atomic, modified_changeset?, new_changeset, atomic_changes, validations}, condition} <-
+           {atomic_with_changeset(
+              module.atomic(changeset, change_opts, struct(Ash.Resource.Change.Context, context)),
+              changeset
+            ), condition} do
+      case condition do
+        true ->
+          {:atomic, apply_atomic_update(new_changeset, atomic_changes)}
 
-          false ->
-            changeset
+        false ->
+          {:atomic, changeset}
 
-          condition ->
+        condition ->
+          if modified_changeset? do
+            {:not_atomic,
+             "change `#{inspect(module)}` modified the changeset, but had a condition that could not be checked without running the action"}
+          else
             atomic_changes =
               Map.new(atomic_changes, fn {key, value} ->
                 new_value =
@@ -1051,24 +1055,33 @@ defmodule Ash.Changeset do
                 {key, new_value}
               end)
 
-            apply_atomic_update(changeset, atomic_changes)
-        end
+            {:atomic, apply_atomic_update(new_changeset, atomic_changes)}
+          end
+      end
+      |> case do
+        {:not_atomic, reason} ->
+          {:not_atomic, reason}
 
-      Enum.reduce(
-        List.wrap(validations),
-        changeset,
-        fn {:atomic, _, condition_expr, error_expr}, changeset ->
-          validate_atomically(changeset, condition_expr, error_expr)
-        end
-      )
+        {:atomic, changeset} ->
+          Enum.reduce(
+            List.wrap(validations),
+            changeset,
+            fn {:atomic, _, condition_expr, error_expr}, changeset ->
+              validate_atomically(changeset, condition_expr, error_expr)
+            end
+          )
+      end
     else
-      {:ok, changeset} ->
-        changeset
+      {{:ok, new_changeset}, _condition} ->
+        new_changeset
+
+      {{:not_atomic, reason}, _} ->
+        {:not_atomic, reason}
 
       {:not_atomic, reason} ->
         {:not_atomic, reason}
 
-      :ok ->
+      {:ok, _} ->
         changeset
     end
   end
@@ -1133,9 +1146,15 @@ defmodule Ash.Changeset do
   end
 
   defp atomic_with_changeset({:atomic, changeset, atomics}, _changeset),
-    do: {:atomic, changeset, atomics, []}
+    do: {:atomic, true, changeset, atomics, []}
+
+  defp atomic_with_changeset({:atomic, atomics}, changeset),
+    do: {:atomic, false, changeset, atomics, []}
+
+  defp atomic_with_changeset({:ok, changeset}, _) do
+    {:atomic, true, changeset, %{}, []}
+  end
 
-  defp atomic_with_changeset({:atomic, atomics}, changeset), do: {:atomic, changeset, atomics, []}
   defp atomic_with_changeset(other, _), do: other
 
   defp validate_atomically(changeset, condition_expr, error_expr) do
@@ -1207,6 +1226,22 @@ defmodule Ash.Changeset do
           |> then(&{:cont, {:atomic, &1}})
       end
     end)
+    |> case do
+      {:atomic, expr} ->
+        case Ash.Expr.eval(expr,
+               resource: changeset.resource,
+               unknown_on_unknown_refs?: true
+             ) do
+          {:ok, value} ->
+            {:atomic, value}
+
+          :unknown ->
+            {:atomic, expr}
+
+          {:error, _} ->
+            {:atomic, false}
+        end
+    end
   end
 
   # This is not expressly necessary as `expr(true and not ^new_expr)` would also

lib/ash/resource/change/after_action.ex

@@ -13,11 +13,4 @@ defmodule Ash.Resource.Change.AfterAction do
       prepend?: opts[:prepend?]
     )
   end
-
-  # we had to remove this. We can't be sure that the change function won't access
-  # `changeset.data`, so cannot do this automatically
-  # @impl true
-  # def atomic(changeset, opts, context) do
-  #   {:ok, change(changeset, opts, context)}
-  # end
 end

lib/ash/resource/change/set_attribute.ex

@@ -80,49 +80,45 @@ defmodule Ash.Resource.Change.SetAttribute do
   end
 
   @impl true
-  def atomic(changeset, opts, context) do
+  def atomic(changeset, opts, _context) do
     value =
       case opts[:value] do
         value when is_function(value) -> value.()
         value -> value
       end
 
-    if Ash.Expr.expr?(value) do
-      if opts[:new?] do
-        {:atomic,
-         %{
-           opts[:attribute] =>
-             expr(
-               if is_nil(^atomic_ref(opts[:attribute])) do
-                 ^value
-               else
-                 ^atomic_ref(opts[:attribute])
-               end
-             )
-         }}
+    if opts[:new?] do
+      {:atomic,
+       %{
+         opts[:attribute] =>
+           expr(
+             if is_nil(^atomic_ref(opts[:attribute])) do
+               ^value
+             else
+               ^atomic_ref(opts[:attribute])
+             end
+           )
+       }}
+    else
+      if opts[:set_when_nil?] do
+        {:atomic, %{opts[:attribute] => value}}
       else
-        if opts[:set_when_nil?] do
-          {:atomic, %{opts[:attribute] => value}}
+        if is_nil(value) do
+          {:ok, changeset}
         else
-          if is_nil(value) do
-            {:ok, changeset}
-          else
-            {:atomic,
-             %{
-               opts[:attribute] =>
-                 expr(
-                   if is_nil(^value) do
-                     ^atomic_ref(opts[:attribute])
-                   else
-                     ^value
-                   end
-                 )
-             }}
-          end
+          {:atomic,
+           %{
+             opts[:attribute] =>
+               expr(
+                 if is_nil(^value) do
+                   ^atomic_ref(opts[:attribute])
+                 else
+                   ^value
+                 end
+               )
+           }}
         end
       end
-    else
-      {:ok, change(changeset, opts, context)}
     end
   end
 end

lib/ash/resource/validation/builtins.ex

@@ -26,10 +26,15 @@ defmodule Ash.Resource.Validation.Builtins do
 
       validate changing(:first_name)
       validate changing(:comments)
+      validate changing(:comments, touching?: true)
+
+  ## Options
+
+  #{Ash.Resource.Validation.Changing.Opts.docs()}
   """
-  @spec changing(attribute_or_relationship :: atom) :: Validation.ref()
-  def changing(field) do
-    {Validation.Changing, field: field}
+  @spec changing(attribute_or_relationship :: atom, opts :: Keyword.t()) :: Validation.ref()
+  def changing(field, opts \\ []) do
+    {Validation.Changing, Keyword.merge(opts, field: field)}
   end
 
   @doc """

lib/ash/resource/validation/changing.ex

@@ -15,7 +15,15 @@ defmodule Ash.Resource.Validation.Changing do
         field: [
           type: :atom,
           required: true,
-          doc: "The attribute or relationship to check for changes."
+          doc:
+            "The attribute or relationship to check for changes. Using a relationship does not compare old and new value, returning `true` if the value is being touched)"
+        ],
+        touching?: [
+          type: :atom,
+          required: false,
+          default: false,
+          doc:
+            "Whether to consider a field as changing if it is just being touched (i.e consider it changed when it is being changed to its current value)"
         ]
       ]
   end
@@ -35,12 +43,28 @@ defmodule Ash.Resource.Validation.Changing do
 
   @impl Ash.Resource.Validation
   def validate(changeset, opts, context) do
-    if field_is_changing?(changeset, Keyword.fetch!(opts, :field)),
+    if field_is_changing?(changeset, Keyword.fetch!(opts, :field), opts[:touching?]),
       do: :ok,
       else: {:error, exception(opts, context)}
   end
 
-  defp field_is_changing?(changeset, field) do
+  defp field_is_changing?(changeset, field, true) do
+    case Ash.Resource.Info.relationship(changeset.resource, field) do
+      nil ->
+        Map.has_key?(changeset.casted_attributes, field) ||
+          Map.has_key?(changeset.attributes, field) || Keyword.has_key?(changeset.atomcis, field)
+
+      %{type: :belongs_to} = relationship ->
+        Map.has_key?(changeset.casted_attributes, field) ||
+          Map.has_key?(changeset.attributes, field) || Keyword.has_key?(changeset.atomcis, field) ||
+          changing_relationship?(changeset, relationship.name)
+
+      relationship ->
+        changing_relationship?(changeset, relationship.name)
+    end
+  end
+
+  defp field_is_changing?(changeset, field, _) do
     case Ash.Resource.Info.relationship(changeset.resource, field) do
       nil ->
         changing_attribute?(changeset, field)
@@ -56,23 +80,27 @@ defmodule Ash.Resource.Validation.Changing do
 
   @impl Ash.Resource.Validation
   def atomic(changeset, opts, context) do
-    case atomic_field(changeset, Keyword.fetch!(opts, :field)) do
-      {:changing, field} ->
-        {:atomic, [field], expr(^atomic_ref(field) == ^ref(field)),
-         expr(
-           error(^InvalidAttribute, %{
-             field: ^field,
-             value: ^atomic_ref(field),
-             message: ^(context.message || @default_error_message),
-             vars: %{field: ^field}
-           })
-         )}
-
-      {:not_changing, _field} ->
-        {:error, exception(opts, context)}
-
-      {:not_atomic, field} ->
-        {:not_atomic, "can't atomically check if #{field} relationship is changing"}
+    if opts[:touching?] do
+      validate(changeset, opts, context)
+    else
+      case atomic_field(changeset, Keyword.fetch!(opts, :field)) do
+        {:changing, field} ->
+          {:atomic, [field], expr(^atomic_ref(field) == ^ref(field)),
+           expr(
+             error(^InvalidAttribute, %{
+               field: ^field,
+               value: ^atomic_ref(field),
+               message: ^(context.message || @default_error_message),
+               vars: %{field: ^field}
+             })
+           )}
+
+        {:not_changing, _field} ->
+          {:error, exception(opts, context)}
+
+        {:not_atomic, field} ->
+          {:not_atomic, "can't atomically check if #{field} relationship is changing"}
+      end
     end
   end