diff --git a/lib/ecto/repo/schema.ex b/lib/ecto/repo/schema.ex index 0ac755b928..c80e62f920 100644 --- a/lib/ecto/repo/schema.ex +++ b/lib/ecto/repo/schema.ex @@ -6,6 +6,7 @@ defmodule Ecto.Repo.Schema do alias Ecto.Changeset alias Ecto.Changeset.Relation require Ecto.Query + require Logger import Ecto.Query.Planner, only: [attach_prefix: 2] @@ -451,8 +452,8 @@ 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 ++ assocs) - changeset = update_in(changeset.changes, &Map.drop(&1, drop_fields)) + 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)) wrap_in_transaction(adapter, adapter_meta, opts, changeset, assocs, embeds, prepare, fn -> assoc_opts = assoc_opts(assocs, opts) @@ -560,7 +561,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, &Map.drop(&1, drop_fields)) + changeset = update_in(changeset.changes, &drop_non_writable_changes!(&1, drop_fields, schema, :update)) if changeset.changes != %{} or force? do wrap_in_transaction(adapter, adapter_meta, opts, changeset, assocs, embeds, prepare, fn -> @@ -624,6 +625,33 @@ 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 -> + case changes do + %{^field => _change} -> + handle_writable_violation(field, schema, action) + changes + %{} -> + changes + end + end) + end + + defp handle_writable_violation(field, schema, action) do + on_writable_violation = schema.__schema__(:on_writable_violation)[field] + + message = "attempted to write to non-writable field #{inspect(field)} during #{action}" + + case on_writable_violation do + :raise -> + raise ArgumentError, message + :warn -> + Logger.warning(message) + _ -> + :ok + end + end + @doc """ Implementation for `Ecto.Repo.insert_or_update/2`. """ diff --git a/lib/ecto/schema.ex b/lib/ecto/schema.ex index de9d03d82c..b8bdd26938 100644 --- a/lib/ecto/schema.ex +++ b/lib/ecto/schema.ex @@ -525,6 +525,7 @@ defmodule Ecto.Schema do Module.register_attribute(__MODULE__, :ecto_autogenerate, accumulate: true) Module.register_attribute(__MODULE__, :ecto_autoupdate, accumulate: true) Module.register_attribute(__MODULE__, :ecto_redact_fields, accumulate: true) + Module.register_attribute(__MODULE__, :ecto_on_writable_violation, accumulate: true) end end @@ -544,7 +545,8 @@ defmodule Ecto.Schema do :where, :references, :skip_default_validation, - :writable + :writable, + :on_writable_violation ] @doc """ @@ -700,6 +702,13 @@ defmodule Ecto.Schema do be further modified, even in an upsert. If set to `:never`, the field becomes read only. Defaults to `:always`. + * `:on_writable_violation` - Defines what action to take when performing an insert or update + 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`. + """ defmacro field(name, type \\ :string, opts \\ []) do quote do @@ -2013,6 +2022,7 @@ defmodule Ecto.Schema do virtual? = opts[:virtual] || false pk? = opts[:primary_key] || false writable = opts[:writable] || :always + on_writable_violation = opts[:on_writable_violation] || :nothing put_struct_field(mod, name, Keyword.get(opts, :default)) redact_field? = @@ -2069,6 +2079,8 @@ defmodule Ecto.Schema do raise ArgumentError, "autogenerated fields must always be writable" end + Module.put_attribute(mod, :ecto_on_writable_violation, {name, on_writable_violation}) + if pk? do Module.put_attribute(mod, :ecto_primary_keys, name) end @@ -2367,6 +2379,7 @@ defmodule Ecto.Schema do autoupdate = Module.get_attribute(module, :ecto_autoupdate) |> Enum.reverse() read_after_writes = Module.get_attribute(module, :ecto_raw) |> Enum.reverse() autogenerate_id = Module.get_attribute(module, :ecto_autogenerate_id) + on_writable_violation = Module.get_attribute(module, :ecto_on_writable_violation) struct_fields = Module.get_attribute(module, :ecto_struct_fields) |> Enum.reverse() derive = Module.get_attribute(module, :derive) @@ -2450,6 +2463,7 @@ defmodule Ecto.Schema do {[:embeds], embed_names}, {[:updatable_fields], updatable}, {[:insertable_fields], insertable}, + {[:on_writable_violation], on_writable_violation |> Map.new() |> Macro.escape()}, {[:redact_fields], redacted_fields}, {[:autogenerate_fields], Enum.flat_map(autogenerate, &elem(&1, 0))}, {[:virtual_fields], Enum.map(virtual_fields, &elem(&1, 0))}, diff --git a/test/ecto/repo_test.exs b/test/ecto/repo_test.exs index 31991ef54a..2a71ad107b 100644 --- a/test/ecto/repo_test.exs +++ b/test/ecto/repo_test.exs @@ -3,6 +3,7 @@ defmodule Ecto.RepoTest do import Ecto.Query import Ecto, only: [put_meta: 2] + import ExUnit.CaptureLog alias Ecto.TestRepo defmodule MyParent do @@ -181,6 +182,26 @@ defmodule Ecto.RepoTest do end end + defmodule MySchemaWritableWarn do + use Ecto.Schema + + schema "my_schema" do + field :never, :integer, writable: :never, on_writable_violation: :warn + field :always, :integer, writable: :always + field :insert, :integer, writable: :insert, on_writable_violation: :warn + end + end + + defmodule MySchemaWritableRaise do + use Ecto.Schema + + schema "my_schema" do + field :never, :integer, writable: :never, on_writable_violation: :raise + field :always, :integer, writable: :always + field :insert, :integer, writable: :insert, on_writable_violation: :raise + end + end + defmodule MySchemaOneField do use Ecto.Schema @@ -2418,7 +2439,7 @@ defmodule Ecto.RepoTest do ] end - test "update only saves changes for writable: :always" do + 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() @@ -2426,6 +2447,39 @@ defmodule Ecto.RepoTest do 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() + + assert_received {:update, %{changes: [always: 10]}} + end) + + assert log =~ "attempted to write to non-writable field :insert during update" + assert log =~ "attempted to write to non-writable field :never during update" + end + + test "update with on_writable_violation: :raise saves changes for writable: :always and raises for changes for writable: :insert/:never" 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() + 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() + end + + %MySchemaWritableRaise{id: 3} + |> Ecto.Changeset.change(%{always: 12}) + |> TestRepo.update() + + assert_received {:update, %{changes: [always: 12]}} + end + test "update is a no-op when updatable fields are not changed" do %MySchemaWritable{id: 1} |> Ecto.Changeset.change(%{never: "can't update", insert: "can't update either"}) @@ -2459,7 +2513,16 @@ defmodule Ecto.RepoTest do end end - test "insert only saves changes for writable: :always/:insert" do + 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() + + 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() @@ -2468,6 +2531,62 @@ defmodule Ecto.RepoTest do 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 + log = capture_log(fn -> + %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] + end) + + assert log =~ "attempted to write to non-writable field :never during insert" + end + + 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() + + assert_received {:insert, %{fields: inserted_fields}} + assert Enum.sort(inserted_fields) == [always: 10, id: 1, insert: 12] + end) + + assert log =~ "attempted to write to non-writable field :never during insert" + end + + test "insert with surfaced changes and on_writable_violation: :raise saves changes for writable: :always/:insert and raises for changes for writable: :never" 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() + end + + %MySchemaWritableRaise{id: 2, insert: 11, always: 12} + |> Ecto.Changeset.change(%{}) + |> TestRepo.insert() + + assert_received {:insert, %{fields: inserted_fields}} + assert Enum.sort(inserted_fields) == [always: 12, id: 2, insert: 11] + end + + test "insert with on_writable_violation: :raise saves changes for writable: :always/:insert and raises for changes for writable: :never" 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() + end + + %MySchemaWritableRaise{id: 2} + |> Ecto.Changeset.change(%{insert: 11, always: 12}) + |> TestRepo.insert() + + assert_received {:insert, %{fields: inserted_fields}} + assert Enum.sort(inserted_fields) == [always: 12, id: 2, insert: 11] + end + test "insert with returning" do %MySchemaWritable{id: 1} |> Ecto.Changeset.change(%{always: 10, never: 11, insert: 12})