Add change_order option to has_many

lib/ecto/association.ex

@@ -625,8 +625,11 @@ defmodule Ecto.Association do
          opts,
          {parent, changes, halt, all_valid?}
        ) do
+
     {changesets, structs, halt, valid?} =
-      Enum.reduce(changesets, {[], [], halt, true}, fn
+      changesets
+      |> maybe_sort_assocs(meta)
+      |> Enum.reduce({[], [], halt, true}, fn
         %{action: action} = changeset, {changesets, structs, halt, valid?} ->
           check_action!(meta, action, repo_action)
 
@@ -659,6 +662,21 @@ defmodule Ecto.Association do
     end
   end
 
+  defp maybe_sort_assocs(changesets, %{change_order: :unique_safe} = refl) do
+    # Define sort order delete < update < insert to avoid duplicate unique values
+    Enum.sort_by(changesets, fn cs ->
+      case cs.action do
+        :delete -> 0
+        :replace when refl.on_replace == :delete -> 0
+        :update -> 1
+        :replace when refl.on_replace == :nilify -> 1
+        :insert -> 2
+      end
+    end)
+  end
+
+  defp maybe_sort_assocs(changesets, _), do: changesets
+
   defp check_action!(%{related: schema}, :delete, :insert) do
     raise ArgumentError,
           "got action :delete in changeset for associated #{inspect(schema)} while inserting"
@@ -730,6 +748,7 @@ defmodule Ecto.Association.Has do
     * `defaults` - Default fields used when building the association
     * `relationship` - The relationship to the specified schema, default is `:child`
     * `preload_order` - Default `order_by` of the association, used only by preload
+    * `change_order` - The change order for associations when parent is manipulated (`has_many` only).
   """
 
   @behaviour Ecto.Association
@@ -747,6 +766,7 @@ defmodule Ecto.Association.Has do
     :queryable,
     :on_delete,
     :on_replace,
+    :change_order,
     where: [],
     unique: true,
     defaults: [],
@@ -837,7 +857,8 @@ defmodule Ecto.Association.Has do
       on_replace: on_replace,
       defaults: defaults,
       where: where,
-      preload_order: preload_order
+      preload_order: preload_order,
+      change_order: opts[:change_order]
     }
   end
 
@@ -904,6 +925,7 @@ defmodule Ecto.Association.Has do
         adapter,
         opts
       ) do
+
     changeset =
       case on_replace do
         :nilify -> %{changeset | action: :update}

lib/ecto/schema.ex

@@ -791,6 +791,13 @@ defmodule Ecto.Schema do
       the custom ordering will take precedence and the `:preload_order` will not be applied.
       See `Ecto.Query.order_by/3` to learn more about ordering expressions.
 
+    * `:change_order` - Determines the order of operations when assocations are changed
+      as part of an insert or update on the parent record. The allowed values are:
+        * `:unique_safe` - Delete, then update, then insert to ensure unique constraints are not violated
+        * `nil` - No deterministic order
+
+      Defaults to `nil`.
+
   ## Examples
 
       defmodule Post do
@@ -2110,7 +2117,8 @@ defmodule Ecto.Schema do
     :defaults,
     :on_replace,
     :where,
-    :preload_order
+    :preload_order,
+    :change_order
   ]
 
   @doc false

test/ecto/repo/has_assoc_test.exs

@@ -25,9 +25,9 @@ defmodule Ecto.Repo.HasAssocTest do
 
     def changeset(struct, params) do
       if params[:delete] do
-        %{Ecto.Changeset.cast(struct, params, []) | action: :delete}
+        %{Ecto.Changeset.cast(struct, params, [:x]) | action: :delete}
       else
-        Ecto.Changeset.cast(struct, params, [])
+        Ecto.Changeset.cast(struct, params, [:x])
       end
     end
   end
@@ -44,6 +44,8 @@ defmodule Ecto.Repo.HasAssocTest do
       has_one :on_delete_delete_assoc, MyAssoc, on_delete: :delete_all
       has_one :on_delete_nilify_assoc, MyAssoc, on_delete: :nilify_all
       has_many :assocs, MyAssoc, on_replace: :delete
+      has_many :ordered_assocs, MyAssoc, on_replace: :delete, change_order: :unique_safe
+      has_many :ordered_nilify_assocs, MyAssoc, on_replace: :nilify, change_order: :unique_safe
       has_many :delete_assocs, MyAssoc
       has_many :on_delete_delete_assocs, MyAssoc, on_delete: :delete_all
       has_many :on_delete_nilify_assocs, MyAssoc, on_delete: :nilify_all
@@ -410,6 +412,54 @@ defmodule Ecto.Repo.HasAssocTest do
     assert_received {:delete, _}
   end
 
+  test "updating assocs with change_order: :unique_safe" do
+    sample1 = %MyAssoc{id: 10, x: "xyz"} |> Ecto.put_meta(state: :loaded)
+    sample2 = %MyAssoc{id: 11, x: "abc"} |> Ecto.put_meta(state: :loaded)
+
+    changeset =
+      %MySchema{id: 1, ordered_assocs: [sample1, sample2]}
+      |> Ecto.Changeset.cast(
+        %{x: "abc", ordered_assocs: [%{id: 12, x: "xyz"}, %{id: 10, x: "tuv"}]},
+        [:x]
+      )
+      |> Ecto.Changeset.cast_assoc(:ordered_assocs)
+
+    TestRepo.update!(changeset)
+    # Parent
+    assert_received {:update, _}
+    # Assoc changes
+    assert_received {:delete, %{order: td}}
+    assert_received {:update, %{order: tu}}
+    assert_received {:insert, %{order: ti}}
+
+    assert td < tu
+    assert tu < ti
+  end
+
+  test "updating assocs with change_order: :unique_safe and on_replace: :nilify" do
+    sample1 = %MyAssoc{id: 10, x: "xyz", my_schema_id: 1} |> Ecto.put_meta(state: :loaded)
+    sample2 = %MyAssoc{id: 11, x: "abc", my_schema_id: 1} |> Ecto.put_meta(state: :loaded)
+
+    changeset =
+      %MySchema{id: 1, ordered_nilify_assocs: [sample1, sample2]}
+      |> Ecto.Changeset.cast(
+        %{x: "abc", ordered_nilify_assocs: [%{id: 12, x: "xyz"}, %{id: 10, x: "tuv"}]},
+        [:x]
+      )
+      |> Ecto.Changeset.cast_assoc(:ordered_nilify_assocs)
+
+    TestRepo.update!(changeset)
+    # Parent
+    assert_received {:update, _}
+    # Assoc changes
+    assert_received {:update, %{filters: [id: 10], order: tu1}}
+    assert_received {:update, %{filters: [id: 11], order: tu2}}
+    assert_received {:insert, %{order: ti}}
+
+    assert tu1 < ti
+    assert tu2 < ti
+  end
+
   test "replacing assocs on update on_replace: :delete" do
     sample = %MyAssoc{id: 10, x: "xyz"} |> Ecto.put_meta(state: :loaded)
 

test/support/test_repo.exs

@@ -102,7 +102,8 @@ defmodule Ecto.TestAdapter do
         fields: fields,
         on_conflict: on_conflict,
         returning: returning,
-        prefix: prefix
+        prefix: prefix,
+        order: System.unique_integer([:monotonic])
       })
 
     send(self(), {:insert, meta})
@@ -115,7 +116,14 @@ defmodule Ecto.TestAdapter do
 
   # Notice the list of changes is never empty.
   def update(_, %{context: nil} = meta, [_ | _] = changes, filters, returning, _opts) do
-    meta = Map.merge(meta, %{changes: changes, filters: filters, returning: returning})
+    meta =
+      Map.merge(meta, %{
+        changes: changes,
+        filters: filters,
+        returning: returning,
+        order: System.unique_integer([:monotonic])
+      })
+
     send(self(), {:update, meta})
     {:ok, Enum.zip(returning, 1..length(returning)//1)}
   end
@@ -125,7 +133,13 @@ defmodule Ecto.TestAdapter do
   end
 
   def delete(_, %{context: nil} = meta, filters, returning, _opts) do
-    meta = Map.merge(meta, %{filters: filters, returning: returning})
+    meta =
+      Map.merge(meta, %{
+        filters: filters,
+        returning: returning,
+        order: System.unique_integer([:monotonic])
+      })
+
     send(self(), {:delete, meta})
     {:ok, Enum.zip(returning, 1..length(returning)//1)}
   end
@@ -192,7 +206,13 @@ defmodule Ecto.CachingTestAdapter do
     []
   end
 
-  def execute(_adapter_meta, _query_meta, {_cache_status, {_operation, _query}}, _dump_params, _opts) do
+  def execute(
+        _adapter_meta,
+        _query_meta,
+        {_cache_status, {_operation, _query}},
+        _dump_params,
+        _opts
+      ) do
     {1, nil}
   end