diff --git a/lib/ecto/repo/schema.ex b/lib/ecto/repo/schema.ex index c80e62f920..6a9e2921ef 100644 --- a/lib/ecto/repo/schema.ex +++ b/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 diff --git a/lib/ecto/schema.ex b/lib/ecto/schema.ex index b8bdd26938..831d300103 100644 --- a/lib/ecto/schema.ex +++ b/lib/ecto/schema.ex @@ -706,8 +706,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 diff --git a/test/ecto/repo_test.exs b/test/ecto/repo_test.exs index 2a71ad107b..cd90759701 100644 --- a/test/ecto/repo_test.exs +++ b/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]