Do not trigger on_replace if has_one is being updated

lib/ecto/changeset/relation.ex

@@ -184,8 +184,8 @@ defmodule Ecto.Changeset.Relation do
           |> put_new_action(:insert)}
   end
 
-  defp do_cast(relation, nil, struct) do
-    on_replace(relation, struct)
+  defp do_cast(relation, nil, current) do
+    on_replace(relation, current)
   end
 
   defp do_cast(%{related: model, on_cast: fun}, params, struct) do
@@ -299,6 +299,8 @@ defmodule Ecto.Changeset.Relation do
 
   defp cast_or_change(_, _, _, _, _, _), do: :error
 
+  # map changes
+
   defp map_changes(list, pks, param_pks, fun, current) do
     map_changes(list, param_pks, fun, process_current(current, pks), [], true, true)
   end
@@ -319,7 +321,7 @@ defmodule Ecto.Changeset.Relation do
           {nil, current, [:insert]}
       end
 
-    case create_changeset!(map, model, fun, allowed_actions) do
+    case build_changeset!(map, model, fun, allowed_actions) do
       {:ok, changeset} ->
         map_changes(rest, pks, fun, current, [changeset | acc],
                     valid? && changeset.valid?, skip? && skip?(changeset))
@@ -337,7 +339,7 @@ defmodule Ecto.Changeset.Relation do
   end
 
   defp reduce_delete_changesets([model | rest], fun, acc, valid?, skip?) do
-    case create_changeset!(nil, model, fun, [:update, :delete]) do
+    case build_changeset!(nil, model, fun, [:update, :delete]) do
       {:ok, changeset} ->
         reduce_delete_changesets(rest, fun, [changeset | acc],
                                  valid? && changeset.valid?,
@@ -347,38 +349,39 @@ defmodule Ecto.Changeset.Relation do
     end
   end
 
+  # single changes
+
+  defp single_change(_relation, nil, _current_pks, _new_pks, fun, current) do
+    single_changeset!(nil, current, fun, [:update, :delete])
+  end
+
+  defp single_change(_relation, new, _current_pks, _new_pks, fun, nil) do
+    single_changeset!(new, nil, fun, [:insert])
+  end
+
   defp single_change(relation, new, current_pks, new_pks, fun, current) do
-    case single_change_action(relation, new, new_pks, current, current_pks) do
-      {current, allowed_actions} ->
-        case create_changeset!(new, current, fun, allowed_actions) do
-          {:ok, changeset} ->
-            {:ok, changeset, changeset.valid?, skip?(changeset)}
-          :error ->
-            :error
-        end
-      :error ->
-        :error
+    if get_pks(new, new_pks) == get_pks(current, current_pks) do
+      single_changeset!(new, current, fun, [:update, :delete])
+    else
+      case local_on_replace(relation, current) do
+        :ok -> single_changeset!(new, nil, fun, [:insert])
+        :error -> :error
+      end
     end
   end
 
-  defp single_change_action(_relation, nil, _new_pks, current, _current_pks),
-    do: {current, [:update, :delete]}
-  defp single_change_action(_relation, _new, _new_pks, nil, _current_pks),
-    do: {nil, [:insert]}
-  defp single_change_action(relation, new, new_pks, current, current_pks) do
-    case local_on_replace(relation, current) do
+  # helpers
+
+  defp single_changeset!(new, current, fun, allowed_actions) do
+    case build_changeset!(new, current, fun, allowed_actions) do
+      {:ok, changeset} ->
+        {:ok, changeset, changeset.valid?, skip?(changeset)}
       :error ->
         :error
-      _ ->
-        if get_pks(new, new_pks) == get_pks(current, current_pks) do
-          {current, [:update, :delete]}
-        else
-          {nil, [:insert]}
-        end
     end
   end
 
-  defp create_changeset!(new, current, fun, allowed_actions) do
+  defp build_changeset!(new, current, fun, allowed_actions) do
     case fun.(new, current) do
       {:ok, changeset} ->
         action = changeset.action

test/ecto/association_test.exs

@@ -585,6 +585,13 @@ defmodule Ecto.AssociationTest do
     assert changeset.errors == [profile: "can't be blank"]
   end
 
+  test "cast has_one with optional" do
+    changeset = Changeset.cast(%Author{profile: %Profile{id: "id"}},
+                               %{"profile" => nil},
+                               [], ~w(profile))
+    assert changeset.changes.profile == nil
+  end
+
   test "cast has_one with custom changeset" do
     changeset = Changeset.cast(%Author{}, %{"profile" => %{"name" => "michal"}},
                      [profile: :optional_changeset])
@@ -636,6 +643,11 @@ defmodule Ecto.AssociationTest do
 
   test "cast has_one with on_replace: :raise" do
     model = %Summary{raise_profile: %Profile{id: 1}}
+
+    params = %{"raise_profile" => %{"name" => "jose", "id" => "1"}}
+    changeset = Changeset.cast(model, params, [:raise_profile])
+    assert changeset.changes.raise_profile.action == :update
+
     assert_raise RuntimeError, ~r"you are attempting to change relation", fn ->
       Changeset.cast(model, %{"raise_profile" => nil}, [], [:raise_profile])
     end
@@ -722,37 +734,39 @@ defmodule Ecto.AssociationTest do
 
   # Please note the order is important in this test.
   test "cast has_many changing models" do
-    posts = [%Post{title: "hello", id: 1},
-             %Post{title: "unknown", id: 2},
-             %Post{title: "other", id: 3}]
+    posts = [%Post{title: "first", id: 1},
+             %Post{title: "second", id: 2},
+             %Post{title: "third", id: 3}]
     params = [%{"title" => "new"},
               %{"id" => 2, "title" => nil},
               %{"id" => 3, "title" => "new name"}]
 
     changeset = Changeset.cast(%Author{posts: posts}, %{"posts" => params}, ~w(posts))
-    [hello, new, unknown, other] = changeset.changes.posts
-    assert hello.model.id == 1
-    assert hello.required == [] # Check for not running chgangeset function
-    assert hello.action == :delete
-    assert hello.valid?
+    [first, new, second, third] = changeset.changes.posts
+
+    assert first.model.id == 1
+    assert first.required == [] # Check for not running changeset function
+    assert first.action == :delete
+    assert first.valid?
+
     assert new.changes == %{title: "new"}
     assert new.action == :insert
     assert new.valid?
-    assert unknown.model.id == 2
-    assert unknown.errors == [title: "can't be blank"]
-    assert unknown.action == :update
-    refute unknown.valid?
-    assert other.model.id == 3
-    assert other.action == :update
-    assert other.valid?
-    refute changeset.valid?
 
-    params = %{"posts" => [%{"id" => "10", "title" => "post"}]}
-    changeset = Changeset.cast(%Author{posts: []}, params, ~w(posts))
-    [post] = changeset.changes.posts
-    assert post.changes == %{title: "post"}
-    assert post.action  == :insert
+    assert second.model.id == 2
+    assert second.errors == [title: "can't be blank"]
+    assert second.action == :update
+    refute second.valid?
+
+    assert third.model.id == 3
+    assert third.action == :update
+    assert third.valid?
+
+    refute changeset.valid?
+  end
 
+  test "cast has_many with invalid operation" do
+    params = %{"posts" => [%{"id" => 1, "title" => "new"}]}
     assert_raise RuntimeError, ~r"cannot update .* it does not exist in the parent model", fn ->
       Changeset.cast(%Author{posts: []}, params, [posts: :set_action])
     end

test/ecto/embedded_test.exs

@@ -182,6 +182,13 @@ defmodule Ecto.EmbeddedTest do
     assert changeset.errors == [profile: "can't be blank"]
   end
 
+  test "cast embeds_one with optional" do
+    changeset = Changeset.cast(%Author{profile: %Profile{id: "id"}},
+                               %{"profile" => nil},
+                               [], ~w(profile))
+    assert changeset.changes.profile == nil
+  end
+
   test "cast embeds_one with custom changeset" do
     changeset = Changeset.cast(%Author{}, %{"profile" => %{"name" => "michal"}},
                      [profile: :optional_changeset])
@@ -225,12 +232,16 @@ defmodule Ecto.EmbeddedTest do
   end
 
   test "cast embeds_one with on_replace: :raise" do
-    model = %Author{raise_profile: %Profile{}}
+    model = %Author{raise_profile: %Profile{id: 1}}
+
+    params = %{"raise_profile" => %{"name" => "jose", "id" => 1}}
+    changeset = Changeset.cast(model, params, [:raise_profile])
+    assert changeset.changes.raise_profile.action == :update
+
     assert_raise RuntimeError, ~r"you are attempting to change relation", fn ->
       Changeset.cast(model, %{"raise_profile" => nil}, [], [:raise_profile])
     end
 
-    model = %Author{raise_profile: %Profile{id: 1}}
     params = %{"raise_profile" => %{"name" => "new", "id" => 2}}
     assert_raise RuntimeError, ~r"you are attempting to change relation", fn ->
       Changeset.cast(model, params, [:raise_profile])
@@ -288,37 +299,38 @@ defmodule Ecto.EmbeddedTest do
 
   # Please note the order is important in this test.
   test "cast embeds_many changing models" do
-    posts = [%Post{title: "hello",   id: 1},
-             %Post{title: "unknown", id: 2},
-             %Post{title: "other",   id: 3}]
+    posts = [%Post{title: "first",   id: 1},
+             %Post{title: "second", id: 2},
+             %Post{title: "third",   id: 3}]
     params = [%{"title" => "new"},
               %{"id" => "2", "title" => nil},
               %{"id" => "3", "title" => "new name"}]
 
     changeset = Changeset.cast(%Author{posts: posts}, %{"posts" => params}, ~w(posts))
-    [hello, new, unknown, other] = changeset.changes.posts
-    assert hello.model.id == 1
-    assert hello.required == [] # Check for not running chgangeset function
-    assert hello.action == :delete
-    assert hello.valid?
+    [first, new, second, third] = changeset.changes.posts
+
+    assert first.model.id == 1
+    assert first.required == [] # Check for not running chgangeset function
+    assert first.action == :delete
+    assert first.valid?
+
     assert new.changes == %{title: "new"}
     assert new.action == :insert
     assert new.valid?
-    assert unknown.model.id == 2
-    assert unknown.errors == [title: "can't be blank"]
-    assert unknown.action == :update
-    refute unknown.valid?
-    assert other.model.id == 3
-    assert other.action == :update
-    assert other.valid?
-    refute changeset.valid?
 
-    params = %{"posts" => [%{"id" => "10", "title" => "post"}]}
-    changeset = Changeset.cast(%Author{posts: []}, params, ~w(posts))
-    [post] = changeset.changes.posts
-    assert post.changes == %{title: "post"}
-    assert post.action  == :insert
+    assert second.model.id == 2
+    assert second.errors == [title: "can't be blank"]
+    assert second.action == :update
+    refute second.valid?
+
+    assert third.model.id == 3
+    assert third.action == :update
+    assert third.valid?
+    refute changeset.valid?
+  end
 
+  test "cast embeds_many with invalid operation" do
+    params = %{"posts" => [%{"id" => 1, "title" => "new"}]}
     assert_raise RuntimeError, ~r"cannot update .* it does not exist in the parent model", fn ->
       Changeset.cast(%Author{posts: []}, params, [posts: :set_action])
     end