Repository: elixir-ecto/ecto
Pull request #4476 (2024-08-05 23:57:32)

Improve `insert_all` select expression message
Closes https://github.com/elixir-ecto/ecto/issues/4474

You're right there are too many issues trying to make this work At minimum we'd have to traverse through the select expressions again to find any parameters, even if there are none.

It's not as obvious how to catch the subquery case so will look into that and follow up.

greg-rychlewski: What do you think about my last commit for subqueries? 

I believe this should work because if there is a mismatch in these two numbers then it should be an error in Ecto. `select_merge` should be the only thing that can erase a select expression from existence, if I'm not missing anything.

I don't think we can port this over to `insert_all` though because we will have the issue that we will have to try extracting the field names from an unbounded amount of nested `:merge` nodes.

Patch:
diff --git a/lib/ecto/query/planner.ex b/lib/ecto/query/planner.ex
index 0051e048..7f17eed4 100644
--- a/lib/ecto/query/planner.ex
+++ b/lib/ecto/query/planner.ex
@@ -370,7 +370,11 @@ defmodule Ecto.Query.Planner do
     {schema_or_source, expr, %{select: select} = query} =
       rewrite_subquery_select_expr(query, source?)
 
-    {expr, _} = prewalk(expr, :select, query, select, 0, adapter)
+    {expr, counter} = prewalk(expr, :select, query, select, 0, adapter)
+
+    if counter != length(select.params) do
+      error!(query, "subqueries cannot select_merge onto an existing field that has an interpolation")
+    end
 
     {{:map, types}, fields, _from} =
       collect_fields(expr, [], :none, query, select.take, true, %{})
diff --git a/lib/ecto/repo/schema.ex b/lib/ecto/repo/schema.ex
index 41bea349..7b030867 100644
--- a/lib/ecto/repo/schema.ex
+++ b/lib/ecto/repo/schema.ex
@@ -142,32 +142,22 @@ defmodule Ecto.Repo.Schema do
         %Ecto.Query.SelectExpr{expr: {:&, _, [_ix]}, fields: fields} ->
           Enum.map(fields, &insert_all_select_dump!(&1))
 
-        %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)}
-
-          when using `select_merge` with `insert_all`, you must always use literal
-          maps or always use `map/2`. The two cannot be combined.
-          """
-
         _ ->
           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:
+          cannot generate a fields list for insert_all from the given source query:
 
             #{inspect(query)}
 
-          Please add a select clause that selects into a map, like this:
+          The select clause must be one of the following:
 
-            from x in Source,
-              ...,
-              select: %{
-                field_a: x.bar,
-                field_b: x.foo
-              }
+            * A single `map/2` or several `map/2` expressions combined with `select_merge`
+            * A single `struct/2` or several `struct/2` expressions combined with `select_merge`
+            * A source such as `p` in the query `from p in Post`
+            * A single literal map or several literal maps combined with `select_merge`. If
+              combining several literal maps, there cannot be any query interpolations
+              except in the last `select_merge`. Consider using `Ecto.Query.exclude/2`
+              to rebuild the select expression from scratch if you need multiple `select_merge`
+              statements with interpolations
 
           All keys must exist in the schema that is being inserted into
           """


Test patch:
diff --git a/test/ecto/query/subquery_test.exs b/test/ecto/query/subquery_test.exs
index 8842f78f..ed31681a 100644
--- a/test/ecto/query/subquery_test.exs
+++ b/test/ecto/query/subquery_test.exs
@@ -555,4 +555,22 @@ defmodule Ecto.Query.SubqueryTest do
       from(c in Comment, as: :c, where: c.id in subquery(union_query)) |> normalize()
     end
   end
+
+  describe "plan: select_merge with interpolations in subquery" do
+    test "non-overlapping interpolations produce valid query" do
+      q = from c in Comment, select: %{id: ^1}, select_merge: %{text: ^"hi"}
+      {planned_q, _, _, _}  = from(s in subquery(q)) |> plan()
+
+      assert Macro.to_string(planned_q.from.source.query.select.expr) == "%{id: ^0, text: ^1}"
+    end
+
+    test "overlapping interpolations are allowed" do
+      msg = ~r"subqueries cannot select_merge onto an existing field that has an interpolation"
+
+      assert_raise Ecto.SubQueryError, msg, fn ->
+        q = from c in Comment, select: %{id: ^1}, select_merge: %{id: ^2}
+        from(s in subquery(q)) |> plan()
+      end
+    end
+  end
 end