diff --git a/lib/ecto/association.ex b/lib/ecto/association.ex index 01d9ecc893..18c6da3f62 100644 --- a/lib/ecto/association.ex +++ b/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} diff --git a/lib/ecto/schema.ex b/lib/ecto/schema.ex index 53ab8fcf3c..4b31eba8d0 100644 --- a/lib/ecto/schema.ex +++ b/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 diff --git a/test/ecto/repo/has_assoc_test.exs b/test/ecto/repo/has_assoc_test.exs index bc6cb21362..decd7686fe 100644 --- a/test/ecto/repo/has_assoc_test.exs +++ b/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) diff --git a/test/support/test_repo.exs b/test/support/test_repo.exs index 7d7b8a322f..b3ac68342b 100644 --- a/test/support/test_repo.exs +++ b/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