Remove read-only changes from returned record during insert/update (#4525)

Author: greg-rychlewski

lib/ecto/repo/schema.ex

@@ -362,7 +362,7 @@ defmodule Ecto.Repo.Schema do
     struct = struct_from_changeset!(:insert, changeset)
     schema = struct.__struct__
     dumper = schema.__schema__(:dump)
-    insertable_fields = schema.__schema__(:insertable_fields)
+    {keep_fields, drop_fields} = schema.__schema__(:insertable_fields)
     assocs = schema.__schema__(:associations)
     embeds = schema.__schema__(:embeds)
 
@@ -379,7 +379,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, insertable_fields ++ assocs)
+    changeset = Relation.surface_changes(changeset, struct, keep_fields ++ assocs)
+    changeset = update_in(changeset.changes, &Map.drop(&1, drop_fields))
 
     wrap_in_transaction(adapter, adapter_meta, opts, changeset, assocs, embeds, prepare, fn ->
       assoc_opts = assoc_opts(assocs, opts)
@@ -398,7 +399,7 @@ defmodule Ecto.Repo.Schema do
         {changes, cast_extra, dump_extra, return_types, return_sources} =
           autogenerate_id(autogen_id, changes, return_types, return_sources, adapter)
 
-        changes = Map.take(changes, insertable_fields)
+        changes = Map.take(changes, keep_fields)
         autogen = autogenerate_changes(schema, :insert, changes)
 
         dump_changes =
@@ -454,7 +455,7 @@ defmodule Ecto.Repo.Schema do
     struct = struct_from_changeset!(:update, changeset)
     schema = struct.__struct__
     dumper = schema.__schema__(:dump)
-    updatable_fields = schema.__schema__(:updatable_fields)
+    {keep_fields, drop_fields} = schema.__schema__(:updatable_fields)
     assocs = schema.__schema__(:associations)
     embeds = schema.__schema__(:embeds)
 
@@ -471,6 +472,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))
 
     if changeset.changes != %{} or force? do
       wrap_in_transaction(adapter, adapter_meta, opts, changeset, assocs, embeds, prepare, fn ->
@@ -483,7 +485,7 @@ defmodule Ecto.Repo.Schema do
         if changeset.valid? do
           embeds = Ecto.Embedded.prepare(changeset, embeds, adapter, :update)
 
-          changes = changeset.changes |> Map.merge(embeds) |> Map.take(updatable_fields)
+          changes = changeset.changes |> Map.merge(embeds) |> Map.take(keep_fields)
           autogen = autogenerate_changes(schema, :update, changes)
           dump_changes = dump_changes!(:update, changes, autogen, schema, [], dumper, adapter)
 
@@ -797,7 +799,8 @@ defmodule Ecto.Repo.Schema do
   end
 
   defp replace_all_fields!(_kind, schema, to_remove) do
-    Enum.map(schema.__schema__(:updatable_fields) -- to_remove, &field_source!(schema, &1))
+    {updatable_fields, _} = schema.__schema__(:updatable_fields)
+    Enum.map(updatable_fields -- to_remove, &field_source!(schema, &1))
   end
 
   defp field_source!(nil, field) do

lib/ecto/schema.ex

@@ -671,11 +671,29 @@ defmodule Ecto.Schema do
         def __schema__(:redact_fields), do: unquote(redacted_fields)
         def __schema__(:virtual_fields), do: unquote(Enum.map(virtual_fields, &elem(&1, 0)))
 
-        def __schema__(:updatable_fields),
-          do: unquote(for {name, {_, :always}} <- fields, do: name)
+        def __schema__(:updatable_fields) do
+          unquote(
+            for {name, {_, writable}} <- fields, reduce: {[], []} do
+              {keep, drop} ->
+                case writable do
+                  :always -> {[name | keep], drop}
+                  _ -> {keep, [name | drop]}
+                end
+            end
+          )
+        end
 
-        def __schema__(:insertable_fields),
-          do: unquote(for {name, {_, writable}} when writable != :never <- fields, do: name)
+        def __schema__(:insertable_fields) do
+          unquote(
+            for {name, {_, writable}} <- fields, reduce: {[], []} do
+              {keep, drop} ->
+                case writable do
+                  :never -> {keep, [name | drop]}
+                  _ -> {[name | keep], drop}
+                end
+            end
+          )
+        end
 
         def __schema__(:autogenerate_fields),
           do: unquote(Enum.flat_map(autogenerate, &elem(&1, 0)))

test/ecto/repo_test.exs

@@ -1839,13 +1839,13 @@ defmodule Ecto.RepoTest do
 
   describe "on conflict" do
     test "passes all fields on replace_all" do
-      fields = [:id, :x, :yyy, :z, :array, :map]
+      fields = [:map, :array, :z, :yyy, :x, :id]
       TestRepo.insert(%MySchema{id: 1}, on_conflict: :replace_all)
       assert_received {:insert, %{source: "my_schema", on_conflict: {^fields, [], []}}}
     end
 
     test "passes all fields+embeds on replace_all" do
-      fields = [:id, :x, :embeds]
+      fields = [:embeds, :x, :id]
       TestRepo.insert(%MySchemaEmbedsMany{id: 1}, on_conflict: :replace_all)
       assert_received {:insert, %{source: "my_schema", on_conflict: {^fields, [], []}}}
     end
@@ -1875,7 +1875,7 @@ defmodule Ecto.RepoTest do
     end
 
     test "passes all fields except given fields" do
-      fields = [:x, :yyy, :z, :map]
+      fields = [:map, :z, :yyy, :x]
       TestRepo.insert(%MySchema{id: 1}, on_conflict: {:replace_all_except, [:id, :array]})
       assert_received {:insert, %{source: "my_schema", on_conflict: {^fields, [], []}}}
     end

test/ecto/schema_test.exs

@@ -30,10 +30,10 @@ defmodule Ecto.SchemaTest do
              [:id, :name, :email, :password, :count, :array, :uuid, :no_query_load, :unwritable, :non_updatable, :comment_id]
 
     assert Schema.__schema__(:insertable_fields) ==
-             [:id, :name, :email, :password, :count, :array, :uuid, :no_query_load, :non_updatable, :comment_id]
+             {[:comment_id, :non_updatable, :no_query_load, :uuid, :array, :count, :password, :email, :name, :id], [:unwritable]}
 
     assert Schema.__schema__(:updatable_fields) ==
-             [:id, :name, :email, :password, :count, :array, :uuid, :no_query_load, :comment_id]
+             {[:comment_id, :no_query_load, :uuid, :array, :count, :password, :email, :name, :id], [:non_updatable, :unwritable]}
 
     assert Schema.__schema__(:virtual_fields) == [:temp]