From ddb747e1c8bbab1bd7e092c30e2a86b0f060d2ec Mon Sep 17 00:00:00 2001 From: Greg Date: Fri, 11 Oct 2024 08:51:20 -0400 Subject: [PATCH 1/5] fix writable changes --- lib/ecto/repo/schema.ex | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/ecto/repo/schema.ex b/lib/ecto/repo/schema.ex index b0a83f5896..a6fb96f31f 100644 --- a/lib/ecto/repo/schema.ex +++ b/lib/ecto/repo/schema.ex @@ -365,6 +365,7 @@ defmodule Ecto.Repo.Schema do insertable_fields = schema.__schema__(:insertable_fields) assocs = schema.__schema__(:associations) embeds = schema.__schema__(:embeds) + virtuals = schema.__schema__(:virtual_fields) {return_types, return_sources} = schema @@ -380,6 +381,8 @@ defmodule Ecto.Repo.Schema do # 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 = update_in(changeset.changes, &Map.take(&1, insertable_fields ++ virtuals ++ assocs)) + wrap_in_transaction(adapter, adapter_meta, opts, changeset, assocs, embeds, prepare, fn -> assoc_opts = assoc_opts(assocs, opts) @@ -457,6 +460,7 @@ defmodule Ecto.Repo.Schema do updatable_fields = schema.__schema__(:updatable_fields) assocs = schema.__schema__(:associations) embeds = schema.__schema__(:embeds) + virtuals = schema.__schema__(:virtual_fields) force? = !!opts[:force] filters = add_pk_filter!(changeset.filters, struct) @@ -471,6 +475,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.take(&1, updatable_fields ++ virtuals ++ assocs)) if changeset.changes != %{} or force? do wrap_in_transaction(adapter, adapter_meta, opts, changeset, assocs, embeds, prepare, fn -> From 02f23ecb7e1d6c61a14ddea817833070c01f909b Mon Sep 17 00:00:00 2001 From: Greg Date: Fri, 11 Oct 2024 08:54:34 -0400 Subject: [PATCH 2/5] change list append order --- lib/ecto/repo/schema.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ecto/repo/schema.ex b/lib/ecto/repo/schema.ex index a6fb96f31f..a948eb3a33 100644 --- a/lib/ecto/repo/schema.ex +++ b/lib/ecto/repo/schema.ex @@ -381,7 +381,7 @@ defmodule Ecto.Repo.Schema do # 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 = update_in(changeset.changes, &Map.take(&1, insertable_fields ++ virtuals ++ assocs)) + changeset = update_in(changeset.changes, &Map.take(&1, virtuals ++ assocs ++ insertable_fields)) wrap_in_transaction(adapter, adapter_meta, opts, changeset, assocs, embeds, prepare, fn -> @@ -475,7 +475,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.take(&1, updatable_fields ++ virtuals ++ assocs)) + changeset = update_in(changeset.changes, &Map.take(&1, virtuals ++ assocs ++ updatable_fields)) if changeset.changes != %{} or force? do wrap_in_transaction(adapter, adapter_meta, opts, changeset, assocs, embeds, prepare, fn -> From 520258ae27abb5bcfa14ae30f07a865164507237 Mon Sep 17 00:00:00 2001 From: Greg Date: Fri, 11 Oct 2024 09:00:02 -0400 Subject: [PATCH 3/5] don't append list twice --- lib/ecto/repo/schema.ex | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/ecto/repo/schema.ex b/lib/ecto/repo/schema.ex index a948eb3a33..39e8fdcf41 100644 --- a/lib/ecto/repo/schema.ex +++ b/lib/ecto/repo/schema.ex @@ -366,6 +366,7 @@ defmodule Ecto.Repo.Schema do assocs = schema.__schema__(:associations) embeds = schema.__schema__(:embeds) virtuals = schema.__schema__(:virtual_fields) + surfaceables = assocs ++ insertable_fields {return_types, return_sources} = schema @@ -380,8 +381,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 = update_in(changeset.changes, &Map.take(&1, virtuals ++ assocs ++ insertable_fields)) + changeset = Relation.surface_changes(changeset, struct, surfaceables) + changeset = update_in(changeset.changes, &Map.take(&1, virtuals ++ surfaceables)) wrap_in_transaction(adapter, adapter_meta, opts, changeset, assocs, embeds, prepare, fn -> From 3eff49bc149619526f5d7ac23a455769b162a65c Mon Sep 17 00:00:00 2001 From: Greg Date: Fri, 11 Oct 2024 15:33:10 -0400 Subject: [PATCH 4/5] review comment --- lib/ecto/repo/schema.ex | 20 +++++++++----------- lib/ecto/schema.ex | 26 ++++++++++++++++++++++---- test/ecto/repo_test.exs | 6 +++--- test/ecto/schema_test.exs | 4 ++-- 4 files changed, 36 insertions(+), 20 deletions(-) diff --git a/lib/ecto/repo/schema.ex b/lib/ecto/repo/schema.ex index 39e8fdcf41..a9e666de4c 100644 --- a/lib/ecto/repo/schema.ex +++ b/lib/ecto/repo/schema.ex @@ -362,11 +362,9 @@ 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) - virtuals = schema.__schema__(:virtual_fields) - surfaceables = assocs ++ insertable_fields {return_types, return_sources} = schema @@ -381,8 +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, surfaceables) - changeset = update_in(changeset.changes, &Map.take(&1, virtuals ++ surfaceables)) + 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 -> @@ -402,7 +400,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 = @@ -458,10 +456,9 @@ 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) - virtuals = schema.__schema__(:virtual_fields) force? = !!opts[:force] filters = add_pk_filter!(changeset.filters, struct) @@ -476,7 +473,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.take(&1, virtuals ++ assocs ++ updatable_fields)) + 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 -> @@ -489,7 +486,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) @@ -803,7 +800,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 diff --git a/lib/ecto/schema.ex b/lib/ecto/schema.ex index e7450aae03..842698573d 100644 --- a/lib/ecto/schema.ex +++ b/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))) diff --git a/test/ecto/repo_test.exs b/test/ecto/repo_test.exs index d9fb87e76f..bb6f0d6132 100644 --- a/test/ecto/repo_test.exs +++ b/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 diff --git a/test/ecto/schema_test.exs b/test/ecto/schema_test.exs index c71c9171dc..ea6c1cd0db 100644 --- a/test/ecto/schema_test.exs +++ b/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] From 1f848b0ce05f30176d14379477b9d63f07e87b07 Mon Sep 17 00:00:00 2001 From: Greg Date: Fri, 11 Oct 2024 15:41:20 -0400 Subject: [PATCH 5/5] remove line --- lib/ecto/repo/schema.ex | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/ecto/repo/schema.ex b/lib/ecto/repo/schema.ex index a9e666de4c..aedc99cf34 100644 --- a/lib/ecto/repo/schema.ex +++ b/lib/ecto/repo/schema.ex @@ -382,7 +382,6 @@ defmodule Ecto.Repo.Schema do 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) user_changeset = run_prepare(changeset, prepare)