Do not reflect changes ignored due to :writable in returned struct (#4736)

Author: green-david

lib/ecto/repo/schema.ex

@@ -452,8 +452,9 @@ defmodule Ecto.Repo.Schema do
     # On insert, we always merge the whole struct into the
     # changeset as changes, except the primary key if it is nil.
     changeset = put_repo_and_action(changeset, :insert, repo, tuplet)
-    changeset = Relation.surface_changes(changeset, struct, keep_fields ++ drop_fields ++ assocs)
-    changeset = update_in(changeset.changes, &drop_non_writable_changes!(&1, drop_fields, schema, :insert))
+    changeset = Relation.surface_changes(changeset, struct, keep_fields ++ assocs)
+    changeset = detect_unsurfaced_non_writable_data!(changeset, drop_fields, schema)
+    changeset = drop_non_writable_changes!(changeset, drop_fields, schema, :insert)
 
     wrap_in_transaction(adapter, adapter_meta, opts, changeset, assocs, embeds, prepare, fn ->
       assoc_opts = assoc_opts(assocs, opts)
@@ -522,6 +523,23 @@ defmodule Ecto.Repo.Schema do
     {:error, put_repo_and_action(changeset, :insert, repo, tuplet)}
   end
 
+  defp detect_unsurfaced_non_writable_data!(changeset, [], _schema) do
+    changeset
+  end
+
+  defp detect_unsurfaced_non_writable_data!(changeset, non_writable_fields, schema) do
+    Enum.each(non_writable_fields, fn field ->
+      case changeset.data do
+        %{^field => value} when value != nil ->
+          handle_writable_violation(field, schema, :insert)
+        %{} ->
+          :ok
+      end
+    end)
+
+    changeset
+  end
+
   @doc """
   Implementation for `Ecto.Repo.update/2`.
   """
@@ -561,7 +579,7 @@ defmodule Ecto.Repo.Schema do
     # fields into the changeset. All changes must be in the
     # changeset before hand.
     changeset = put_repo_and_action(changeset, :update, repo, tuplet)
-    changeset = update_in(changeset.changes, &drop_non_writable_changes!(&1, drop_fields, schema, :update))
+    changeset = drop_non_writable_changes!(changeset, drop_fields, schema, :update)
 
     if changeset.changes != %{} or force? do
       wrap_in_transaction(adapter, adapter_meta, opts, changeset, assocs, embeds, prepare, fn ->
@@ -625,16 +643,23 @@ defmodule Ecto.Repo.Schema do
     {:error, put_repo_and_action(changeset, :update, repo, tuplet)}
   end
 
-  defp drop_non_writable_changes!(changes, non_writable_fields, schema, action) do
-    Enum.reduce(non_writable_fields, changes, fn field, changes ->
+  defp drop_non_writable_changes!(changeset, [], _schema, _action) do
+    changeset
+  end
+
+  defp drop_non_writable_changes!(changeset, non_writable_fields, schema, action) do
+   changes = Enum.reduce(non_writable_fields, changeset.changes, fn field, changes ->
       case changes do
         %{^field => _change} ->
           handle_writable_violation(field, schema, action)
-          changes
+
+          Map.delete(changes, field)
         %{} ->
           changes
       end
     end)
+
+    %{changeset | changes: changes}
   end
 
   defp handle_writable_violation(field, schema, action) do

lib/ecto/schema.ex

@@ -710,8 +710,7 @@ defmodule Ecto.Schema do
       attempts to modify a field that should not be modified according to it's `:writable` value.
       Must be one of `:nothing`, `:warn`, or `:raise`. If set to `:nothing`, the modification is
       silently ignored. If set to `:warn`, the modification is ignored and a warning is logged. If set
-      to `:raise`, an exception is raised and the operation is aborted. If `:writable` is set to `:always`,
-      `:on_writable_violation` must be set to `:nothing`. Defaults to `:nothing`.
+      to `:raise`, an exception is raised and the operation is aborted. Defaults to `:nothing`.
 
   """
   defmacro field(name, type \\ :string, opts \\ []) do

test/ecto/repo_test.exs

@@ -2440,18 +2440,20 @@ defmodule Ecto.RepoTest do
     end
 
     test "update with on_writable_violation: :nothing saves changes for writable: :always and ignores changes for writable: :insert/:never" do
-      %MySchemaWritable{id: 1}
-      |> Ecto.Changeset.change(%{always: 10, never: 11, insert: 12})
-      |> TestRepo.update()
+      %{always: 10, never: nil, insert: nil} =
+        %MySchemaWritable{id: 1}
+        |> Ecto.Changeset.change(%{always: 10, never: 11, insert: 12})
+        |> TestRepo.update!()
 
       assert_received {:update, %{changes: [always: 10]}}
     end
 
     test "update with on_writable_violation: :warn saves changes for writable: :always, ignores changes for writable: :insert/:never, and logs a warning" do
       log = capture_log(fn ->
-        %MySchemaWritableWarn{id: 1}
-        |> Ecto.Changeset.change(%{always: 10, never: 11, insert: 12})
-        |> TestRepo.update()
+        %{always: 10, never: nil, insert: nil} =
+          %MySchemaWritableWarn{id: 1}
+          |> Ecto.Changeset.change(%{always: 10, never: 11, insert: 12})
+          |> TestRepo.update!()
 
         assert_received {:update, %{changes: [always: 10]}}
       end)
@@ -2464,18 +2466,18 @@ defmodule Ecto.RepoTest do
       assert_raise ArgumentError, "attempted to write to non-writable field :never during update", fn ->
         %MySchemaWritableRaise{id: 1}
         |> Ecto.Changeset.change(%{never: 10})
-        |> TestRepo.update()
+        |> TestRepo.update!()
       end
 
       assert_raise ArgumentError, "attempted to write to non-writable field :insert during update", fn ->
         %MySchemaWritableRaise{id: 2}
         |> Ecto.Changeset.change(%{insert: 11})
-        |> TestRepo.update()
+        |> TestRepo.update!()
       end
 
       %MySchemaWritableRaise{id: 3}
       |> Ecto.Changeset.change(%{always: 12})
-      |> TestRepo.update()
+      |> TestRepo.update!()
 
       assert_received {:update, %{changes: [always: 12]}}
     end
@@ -2514,28 +2516,37 @@ defmodule Ecto.RepoTest do
     end
 
     test "insert with surfaced changes on_writable_violation: :nothing saves changes for writable: :always/:insert and ignores changes for writable: :never" do
-      %MySchemaWritable{id: 1, always: 10, never: 11, insert: 12}
-      |> Ecto.Changeset.change(%{})
-      |> TestRepo.insert()
+      # For surfaced changes from the underlying struct, the value in the returned struct is
+      # maintained even though the underlying write was not performed, as opposed to "normal" changes
+      # provided via a changeset.
+      %{always: 10, never: 11, insert: 12} =
+        %MySchemaWritable{id: 1, always: 10, never: 11, insert: 12}
+        |> Ecto.Changeset.change(%{})
+        |> TestRepo.insert!()
 
       assert_received {:insert, %{fields: inserted_fields}}
       assert Enum.sort(inserted_fields) == [always: 10, id: 1, insert: 12]
     end
 
     test "insert with on_writable_violation: :nothing saves changes for writable: :always/:insert and ignores changes for writable: :never" do
-      %MySchemaWritable{id: 1}
-      |> Ecto.Changeset.change(%{always: 10, never: 11, insert: 12})
-      |> TestRepo.insert()
+      %{always: 10, never: nil, insert: 12} =
+        %MySchemaWritable{id: 1}
+        |> Ecto.Changeset.change(%{always: 10, never: 11, insert: 12})
+        |> TestRepo.insert!()
 
       assert_received {:insert, %{fields: inserted_fields}}
       assert Enum.sort(inserted_fields) == [always: 10, id: 1, insert: 12]
     end
 
-    test "insert with with surfaced changes and on_writable_violation: :warn saves changes for writable: :always/:insert, ignores changes for writable: :never, and logs a warning" do
+    test "insert with surfaced changes and on_writable_violation: :warn saves changes for writable: :always/:insert, ignores changes for writable: :never, and logs a warning" do
       log = capture_log(fn ->
-        %MySchemaWritableWarn{id: 1, always: 10, never: 11, insert: 12}
-        |> Ecto.Changeset.change(%{})
-        |> TestRepo.insert()
+        # For surfaced changes from the underlying struct, the value in the returned struct is
+        # maintained even though the underlying write was not performed, as opposed to "normal" changes
+        # provided via a changeset.
+        %{always: 10, never: 11, insert: 12} =
+          %MySchemaWritableWarn{id: 1, always: 10, never: 11, insert: 12}
+          |> Ecto.Changeset.change(%{})
+          |> TestRepo.insert!()
 
         assert_received {:insert, %{fields: inserted_fields}}
         assert Enum.sort(inserted_fields) == [always: 10, id: 1, insert: 12]
@@ -2546,9 +2557,10 @@ defmodule Ecto.RepoTest do
 
     test "insert with on_writable_violation: :warn saves changes for writable: :always/:insert, ignores changes for writable: :never, and logs a warning" do
       log = capture_log(fn ->
-        %MySchemaWritableWarn{id: 1}
-        |> Ecto.Changeset.change(%{always: 10, never: 11, insert: 12})
-        |> TestRepo.insert()
+        %{always: 10, never: nil, insert: 12} =
+          %MySchemaWritableWarn{id: 1}
+          |> Ecto.Changeset.change(%{always: 10, never: 11, insert: 12})
+          |> TestRepo.insert!()
 
         assert_received {:insert, %{fields: inserted_fields}}
         assert Enum.sort(inserted_fields) == [always: 10, id: 1, insert: 12]
@@ -2561,12 +2573,13 @@ defmodule Ecto.RepoTest do
       assert_raise ArgumentError, "attempted to write to non-writable field :never during insert", fn ->
         %MySchemaWritableRaise{id: 1, never: 10}
         |> Ecto.Changeset.change(%{})
-        |> TestRepo.insert()
+        |> TestRepo.insert!()
       end
 
+
       %MySchemaWritableRaise{id: 2, insert: 11, always: 12}
       |> Ecto.Changeset.change(%{})
-      |> TestRepo.insert()
+      |> TestRepo.insert!()
 
       assert_received {:insert, %{fields: inserted_fields}}
       assert Enum.sort(inserted_fields) == [always: 12, id: 2, insert: 11]
@@ -2576,12 +2589,12 @@ defmodule Ecto.RepoTest do
       assert_raise ArgumentError, "attempted to write to non-writable field :never during insert", fn ->
         %MySchemaWritableRaise{id: 1}
         |> Ecto.Changeset.change(%{never: 10})
-        |> TestRepo.insert()
+        |> TestRepo.insert!()
       end
 
       %MySchemaWritableRaise{id: 2}
       |> Ecto.Changeset.change(%{insert: 11, always: 12})
-      |> TestRepo.insert()
+      |> TestRepo.insert!()
 
       assert_received {:insert, %{fields: inserted_fields}}
       assert Enum.sort(inserted_fields) == [always: 12, id: 2, insert: 11]