Repository: elixir-ecto/ecto
Pull request #4469 (2024-08-01 23:06:24)

`insert_all` with source (+ update syntax)
Closes https://github.com/elixir-ecto/ecto/issues/4464

I also think we can make it work with selecting just a source. This could be useful if for instance you are trying to move data to a new table.

greg-rychlewski: I messed up. `fields` is already translated to `:source`. I'll try to see if there's anything else I can do.
greg-rychlewski: @josevalim thanks for the great feedback. i believe i addressed all of it :) 
josevalim: @greg-rychlewski I have been thinking about this PR and I am not really sure if we should support the update the syntax. The biggest issue is that, if you write this:

    select: %{p | foo: 321}

It tells me you are selecting **all* of p and updating the value of `:foo`. The fact we are selecting only a subset is a bit awkward to me, even though it is the most logical. What would we do if the user really wants to select all of them in the future? Sorry for not noticing this earlier, but we can probably keep the other warts of this PR (except the update)?

Patch:
diff --git a/lib/ecto/query/planner.ex b/lib/ecto/query/planner.ex
index 18005e50..a83be614 100644
--- a/lib/ecto/query/planner.ex
+++ b/lib/ecto/query/planner.ex
@@ -2195,7 +2195,7 @@ defmodule Ecto.Query.Planner do
         {{:source, {source, schema}, prefix || query.prefix, types}, fields}
 
       {{:ok, {_, fields}}, _} ->
-        {{:map, Enum.map(fields, &{&1, {:value, :any}})}, Enum.map(fields, &select_field(&1, ix))}
+        {{:map, Enum.map(fields, &{&1, {:value, :any}})}, Enum.map(fields, &select_field(&1, ix, false))}
 
       {:error, {:fragment, _, _}} ->
         {{:value, :map}, [{:&, [], [ix]}]}
@@ -2222,7 +2222,7 @@ defmodule Ecto.Query.Planner do
 
       {:error, %Ecto.SubQuery{select: select}} ->
         fields = subquery_source_fields(select)
-        {select, Enum.map(fields, &select_field(&1, ix))}
+        {select, Enum.map(fields, &select_field(&1, ix, false))}
     end
   end
 
@@ -2231,16 +2231,16 @@ defmodule Ecto.Query.Planner do
     |> Enum.reverse()
     |> Enum.reduce({[], []}, fn
       field, {types, exprs} when is_atom(field) and not is_map_key(drop, field) ->
-        {source, type, _read_only?} = Map.get(dumper, field, {field, :any, false})
-        {[{field, type} | types], [select_field(source, ix) | exprs]}
+        {source, type, read_only?} = Map.get(dumper, field, {field, :any, false})
+        {[{field, type} | types], [select_field(source, ix, read_only?) | exprs]}
 
       _field, acc ->
         acc
     end)
   end
 
-  defp select_field(field, ix) do
-    {{:., [], [{:&, [], [ix]}, field]}, [], []}
+  defp select_field(field, ix, read_only?) do
+    {{:., [read_only: read_only?], [{:&, [], [ix]}, field]}, [], []}
   end
 
   defp get_ix!({:&, _, [ix]} = expr, _kind, query) do
diff --git a/lib/ecto/repo/schema.ex b/lib/ecto/repo/schema.ex
index 0fd803a4..a7225f93 100644
--- a/lib/ecto/repo/schema.ex
+++ b/lib/ecto/repo/schema.ex
@@ -108,62 +108,64 @@ defmodule Ecto.Repo.Schema do
     {query, opts} = repo.prepare_query(:insert_all, query, opts)
     query = attach_prefix(query, opts)
 
-    {query, cast_params, dump_params} = Ecto.Adapter.Queryable.plan_query(:insert_all, adapter, query)
+    {query, cast_params, dump_params} =
+      Ecto.Adapter.Queryable.plan_query(:insert_all, adapter, query)
 
-    ix = case query.select do
-      %Ecto.Query.SelectExpr{expr: {:&, _, [ix]}} -> ix
-      _ -> nil
-    end
+    ix =
+      case query.select do
+        %Ecto.Query.SelectExpr{expr: {:&, _, [ix]}} -> ix
+        _ -> nil
+      end
 
-    header = case query.select do
-      %Ecto.Query.SelectExpr{expr: {:%{}, _ctx, args}} ->
-        Enum.map(args, fn {field, _} ->
-          case dumper do
-            %{^field => {source, _, false}} -> source
-            %{} -> raise ArgumentError, "cannot select unwritable field `#{field}` for insert_all"
-            nil -> field
-          end
-        end)
+    header =
+      case query.select do
+        %Ecto.Query.SelectExpr{expr: {:%{}, [], [{:|, _, [{:&, _, [ix]}, args]}]}, fields: fields} ->
+          select_fields =
+            for {{:., _, [{:&, _, [^ix]}, _]}, [], []} = expr <- fields,
+                do: insert_all_select_dump!(expr)
 
-      %Ecto.Query.SelectExpr{take: %{^ix => {_fun, fields}}} ->
-        Enum.map(fields, fn field ->
-          case dumper do
-            %{^field => {source, _, false}} -> source
-            %{} -> raise ArgumentError, "cannot select unwritable field `#{field}` for insert_all"
-            nil -> field
-          end
-        end)
+          update_fields = args |> Keyword.keys() |> Enum.map(&insert_all_select_dump!(&1, dumper))
+          select_fields ++ update_fields
 
-      %Ecto.Query.SelectExpr{expr: {:merge, _, _}} ->
-        raise ArgumentError, """
-        the source query given to `insert_all` has selected both `map/2` and
-        a literal map:
+        %Ecto.Query.SelectExpr{expr: {:%{}, _ctx, args}} ->
+          Enum.map(args, fn {field, _} -> insert_all_select_dump!(field, dumper) end)
 
-          #{inspect query}
+        %Ecto.Query.SelectExpr{take: %{^ix => {_fun, fields}}} ->
+          Enum.map(fields, &insert_all_select_dump!(&1, dumper))
 
-        when using `select_merge` with `insert_all`, you must always use literal
-        maps or always use `map/2`. The two cannot be combined.
-        """
+        %Ecto.Query.SelectExpr{expr: {:&, _, [_ix]}, fields: fields} ->
+          Enum.map(fields, &insert_all_select_dump!(&1))
 
-      _ ->
-        raise ArgumentError, """
-        cannot generate a fields list for insert_all from the given source query
-        because it does not have a select clause that uses a map:
+        %Ecto.Query.SelectExpr{expr: {:merge, _, _}} ->
+          raise ArgumentError, """
+          the source query given to `insert_all` has selected both `map/2` and
+          a literal map:
 
-          #{inspect query}
+            #{inspect(query)}
 
-        Please add a select clause that selects into a map, like this:
+          when using `select_merge` with `insert_all`, you must always use literal
+          maps or always use `map/2`. The two cannot be combined.
+          """
 
-          from x in Source,
-            ...,
-            select: %{
-              field_a: x.bar,
-              field_b: x.foo
-            }
+        _ ->
+          raise ArgumentError, """
+          cannot generate a fields list for insert_all from the given source query
+          because it does not have a select clause that uses a map:
 
-        All keys must exist in the schema that is being inserted into
-        """
-    end
+            #{inspect(query)}
+
+          Please add a select clause that selects into a map, like this:
+
+            from x in Source,
+              ...,
+              select: %{
+                field_a: x.bar,
+                field_b: x.foo
+              }
+
+          All keys must exist in the schema that is being inserted into
+          """
+      end
 
     counter = fn -> length(dump_params) end
 
@@ -271,6 +273,22 @@ defmodule Ecto.Repo.Schema do
     {rows, Enum.reverse(cast_params), counter}
   end
 
+  defp insert_all_select_dump!({{:., dot_meta, [{:&, _, [_]}, field]}, [], []}) do
+    if dot_meta[:read_only] == true do
+      raise ArgumentError, "cannot select unwritable field `#{field}` for insert_all"
+    else
+      field
+    end
+  end
+
+  defp insert_all_select_dump!(field, dumper) when is_atom(field) do
+    case dumper do
+      %{^field => {source, _, false}} -> source
+      %{} -> raise ArgumentError, "cannot select unwritable field `#{field}` for insert_all"
+      nil -> field
+    end
+  end
+
   defp autogenerate_id(nil, fields, header, _adapter) do
     {fields, header}
   end


Test patch:
diff --git a/test/ecto/query/planner_test.exs b/test/ecto/query/planner_test.exs
index 00292acc..4c3affda 100644
--- a/test/ecto/query/planner_test.exs
+++ b/test/ecto/query/planner_test.exs
@@ -166,7 +166,7 @@ defmodule Ecto.Query.PlannerTest do
 
   defp select_fields(fields, ix) do
     for field <- fields do
-      {{:., [], [{:&, [], [ix]}, field]}, [], []}
+      {{:., [read_only: false], [{:&, [], [ix]}, field]}, [], []}
     end
   end
 
@@ -1863,7 +1863,7 @@ defmodule Ecto.Query.PlannerTest do
     start_param_ix = 0
     native_types = %{bid: :uuid, num: :integer}
     types_kw = Enum.map(types, fn {field, _} -> {field, native_types[field]} end)
-    field_ast = Enum.map(types, fn {field, _} -> {{:., [], [{:&, [], [0]}, field]}, [], []} end)
+    field_ast = Enum.map(types, fn {field, _} -> {{:., [read_only: false], [{:&, [], [0]}, field]}, [], []} end)
 
     assert q.from.source == {:values, [], [types_kw, start_param_ix, length(values)]}
     assert q.select.fields == field_ast
@@ -1881,7 +1881,7 @@ defmodule Ecto.Query.PlannerTest do
     start_param_ix = 1
     native_types = %{bid: :uuid, num: :integer}
     types_kw = Enum.map(types, fn {field, _} -> {field, native_types[field]} end)
-    field_ast = Enum.map(types, fn {field, _} -> {{:., [], [{:&, [], [1]}, field]}, [], []} end)
+    field_ast = Enum.map(types, fn {field, _} -> {{:., [read_only: false], [{:&, [], [1]}, field]}, [], []} end)
     [join] = q.joins
 
     assert join.source == {:values, [], [types_kw, start_param_ix, length(values)]}
@@ -2672,8 +2672,8 @@ defmodule Ecto.Query.PlannerTest do
       query = "schema" |> select([s], %{x1: selected_as(s.x, :integer), x2: s.x})
       %{select: select} = from(q in subquery(query)) |> normalize()
 
-      field1 = {{:., [], [{:&, [], [0]}, :integer]}, [], []}
-      field2 = {{:., [], [{:&, [], [0]}, :x2]}, [], []}
+      field1 = {{:., [read_only: false], [{:&, [], [0]}, :integer]}, [], []}
+      field2 = {{:., [read_only: false], [{:&, [], [0]}, :x2]}, [], []}
       assert [^field1, ^field2] = select.fields
     end
 
@@ -2682,8 +2682,8 @@ defmodule Ecto.Query.PlannerTest do
       s2 = from s in subquery(s1), select: %{y1: selected_as(s.integer, :integer2), y2: s.x2}
       %{select: select} = from(q in subquery(s2)) |> normalize()
 
-      field1 = {{:., [], [{:&, [], [0]}, :integer2]}, [], []}
-      field2 = {{:., [], [{:&, [], [0]}, :y2]}, [], []}
+      field1 = {{:., [read_only: false], [{:&, [], [0]}, :integer2]}, [], []}
+      field2 = {{:., [read_only: false], [{:&, [], [0]}, :y2]}, [], []}
       assert [^field1, ^field2] = select.fields
     end
 
diff --git a/test/ecto/query/subquery_test.exs b/test/ecto/query/subquery_test.exs
index 15a63f77..80ac26f5 100644
--- a/test/ecto/query/subquery_test.exs
+++ b/test/ecto/query/subquery_test.exs
@@ -56,7 +56,7 @@ defmodule Ecto.Query.SubqueryTest do
 
   defp select_fields(fields, ix) do
     for field <- fields do
-      {{:., [], [{:&, [], [ix]}, field]}, [], []}
+      {{:., [read_only: false], [{:&, [], [ix]}, field]}, [], []}
     end
   end
 
@@ -386,7 +386,7 @@ defmodule Ecto.Query.SubqueryTest do
     test "keeps field with nil values" do
       query = from p in subquery(from p in Post, select: %{title: nil})
       assert normalize(query).from.source.query.select.fields == [title: nil]
-      assert normalize(query).select.fields == [{{:., [], [{:&, [], [0]}, :title]}, [], []}]
+      assert [{{:., _, [{:&, [], [0]}, :title]}, [], []}] = normalize(query).select.fields
     end
 
     test "with params in from" do
@@ -442,11 +442,11 @@ defmodule Ecto.Query.SubqueryTest do
 
       subquery = from p in Post, select: %{id: p.id, title: p.title}
       query = normalize(from(p in subquery(subquery), select: [:title]))
-      assert query.select.fields == [{{:., [], [{:&, [], [0]}, :title]}, [], []}]
+      assert [{{:., _, [{:&, [], [0]}, :title]}, [], []}] = query.select.fields
 
       subquery = from p in Post, select: %{id: p.id, title: p.title}
       query = normalize(from(p in subquery(subquery), select: map(p, [:title])))
-      assert query.select.fields == [{{:., [], [{:&, [], [0]}, :title]}, [], []}]
+      assert [{{:., _, [{:&, [], [0]}, :title]}, [], []}] = query.select.fields
 
       assert_raise Ecto.QueryError, ~r/it is not possible to return a struct subset of a subquery/, fn ->
         subquery = from p in Post, select: %{id: p.id, title: p.title}
diff --git a/test/ecto/repo_test.exs b/test/ecto/repo_test.exs
index 4a8b4486..99b7ae82 100644
--- a/test/ecto/repo_test.exs
+++ b/test/ecto/repo_test.exs
@@ -160,6 +160,14 @@ defmodule Ecto.RepoTest do
     end
   end
 
+  defmodule MySchemaReadOnly do
+    use Ecto.Schema
+
+    schema "my_schema" do
+      field :x, :string, read_only: true
+    end
+  end
+
   test "defines child_spec/1" do
     assert TestRepo.child_spec([]) == %{
              id: TestRepo,
@@ -621,7 +629,7 @@ defmodule Ecto.RepoTest do
       end)
     end
 
-    test "takes query as datasource" do
+    test "takes query selecting on map" do
       import Ecto.Query
 
       threshold = "ten"
@@ -641,7 +649,7 @@ defmodule Ecto.RepoTest do
       assert ["one", "two", "ten"] = params
     end
 
-    test "takes query as datasource with literals" do
+    test "takes query selecting on map with literals" do
       threshold = "ten"
 
       query =
@@ -660,7 +668,7 @@ defmodule Ecto.RepoTest do
       assert ["ten"] = params
     end
 
-    test "takes query selecting on struct" do
+    test "takes query selecting on struct/2" do
       query =
         from s in MySchema,
           select: struct(s, [:x, :y])
@@ -670,7 +678,7 @@ defmodule Ecto.RepoTest do
       assert_received {:insert_all, %{source: "my_schema"}, {%Ecto.Query{}, _params}}
     end
 
-    test "takes query selecting on map" do
+    test "takes query selecting on map/2" do
       query =
         from s in MySchema,
           select: map(s, [:x, :y])
@@ -680,6 +688,86 @@ defmodule Ecto.RepoTest do
       assert_received {:insert_all, %{source: "my_schema"}, {%Ecto.Query{}, _params}}
     end
 
+    test "takes query selecting on source" do
+      query = from s in MySchema, select: s
+      TestRepo.insert_all(MySchema, query)
+
+      assert_received {:insert_all, %{source: "my_schema", header: header}, {%Ecto.Query{}, _params}}
+      assert header == [:id, :x, :yyy, :z, :array, :map]
+    end
+
+    test "takes query selecting on source with join" do
+      query = from p in MyParent, join: a in MySchemaWithAssoc, on: true, select: a
+      TestRepo.insert_all(MySchemaWithAssoc, query)
+
+      assert_received {:insert_all, %{source: "my_schema", header: header}, {%Ecto.Query{}, _params}}
+      assert header == [:id, :n, :parent_id]
+    end
+
+    test "takes query selecting on source with literal update" do
+      query = from s in MySchema, select: %{s | x: "x"}
+      TestRepo.insert_all(MySchema, query)
+
+      assert_received {:insert_all, %{source: "my_schema", header: header}, {%Ecto.Query{} = query, _params}}
+
+      source_fields = [:id, :yyy, :z, :array, :map]
+      update_fields = [:x]
+      update_values = ["x"]
+
+      assert header == source_fields ++ update_fields
+      assert query.select.fields == select_fields(source_fields, 0) ++ update_values
+    end
+
+    test "takes query selecting on source with dynamic update" do
+      query = from s in MySchema, select: %{s | x: ^"x"}
+      TestRepo.insert_all(MySchema, query)
+
+      assert_received {:insert_all, %{source: "my_schema", header: header}, {%Ecto.Query{} = query, _params}}
+
+      source_fields = [:id, :yyy, :z, :array, :map]
+      update_fields = [:x]
+      update_values = [%Ecto.Query.Tagged{tag: :string, type: :string, value: {:^, [], [0]}}]
+
+      assert header == source_fields ++ update_fields
+      assert query.select.fields == select_fields(source_fields, 0) ++ update_values
+    end
+
+    test "takes query selecting on source with join column update" do
+      query = from p in MyParent, join: a in MySchemaWithAssoc, on: true, select: %{p | id: a.parent_id}
+      TestRepo.insert_all(MySchemaWithAssoc, query)
+
+      assert_received {:insert_all, %{source: "my_schema", header: header}, {%Ecto.Query{} = query, _params}}
+
+      source_fields = [:n]
+      update_fields = [:id]
+      update_values = [{{:., [type: :id], [{:&, [], [1]}, :parent_id]}, [], []}]
+
+      assert header == source_fields ++ update_fields
+      assert query.select.fields == select_fields(source_fields, 0) ++ update_values
+    end
+
+    test "takes query selecting on map/2 with update" do
+      query = from s in MySchema, select: %{map(s, [:id, :x, :z]) | x: "x"}
+      TestRepo.insert_all(MySchema, query)
+
+      assert_received {:insert_all, %{source: "my_schema", header: header}, {%Ecto.Query{} = query, _params}}
+
+      source_fields = [:id, :z]
+      update_fields = [:x]
+      update_values = ["x"]
+
+      assert header == source_fields ++ update_fields
+      assert query.select.fields == select_fields(source_fields, 0) ++ update_values
+    end
+
+    test "raises with query selecting read only fields" do
+      msg = ~r"cannot select unwritable field"
+
+      assert_raise ArgumentError, msg, fn ->
+        TestRepo.insert_all(MySchemaReadOnly, from(r in MySchemaReadOnly, select: r))
+      end
+    end
+
     test "raises when a bad query is given as source" do
       assert_raise ArgumentError, fn ->
         TestRepo.insert_all(MySchema, from(s in MySchema))
@@ -2001,4 +2089,10 @@ defmodule Ecto.RepoTest do
       assert pid in Ecto.Repo.all_running()
     end
   end
+
+  defp select_fields(fields, ix) do
+    for field <- fields do
+      {{:., [read_only: false], [{:&, [], [ix]}, field]}, [], []}
+    end
+  end
 end