Don't send dynamic map keys to the database (#4327)

lib/ecto/query.ex

@@ -1959,7 +1959,8 @@ defmodule Ecto.Query do
   >
   > When selecting a literal atom, its value must be the same across
   > all queries. Otherwise, the value from the parent query will be
-  > applied to all other queries.
+  > applied to all other queries. This also holds true for selecting
+  > maps with atom keys.
 
   Union expression returns only unique rows as if each query returned
   distinct results. This may cause a performance penalty. If you need
@@ -2011,7 +2012,8 @@ defmodule Ecto.Query do
   >
   > When selecting a literal atom, its value must be the same across
   > all queries. Otherwise, the value from the parent query will be
-  > applied to all other queries.
+  > applied to all other queries. This also holds true for selecting
+  > maps with atom keys.
 
   Note that the operations `order_by`, `limit` and `offset` of the
   current `query` apply to the result of the union. `order_by` must
@@ -2058,7 +2060,8 @@ defmodule Ecto.Query do
   >
   > When selecting a literal atom, its value must be the same across
   > all queries. Otherwise, the value from the parent query will be
-  > applied to all other queries.
+  > applied to all other queries. This also holds true for selecting
+  > maps with atom keys.
 
   Except expression returns only unique rows as if each query returned
   distinct results. This may cause a performance penalty. If you need
@@ -2110,7 +2113,8 @@ defmodule Ecto.Query do
   >
   > When selecting a literal atom, its value must be the same across
   > all queries. Otherwise, the value from the parent query will be
-  > applied to all other queries.
+  > applied to all other queries. This also holds true for selecting
+  > maps with atom keys.
 
   Note that the operations `order_by`, `limit` and `offset` of the
   current `query` apply to the result of the set difference. `order_by`
@@ -2157,7 +2161,8 @@ defmodule Ecto.Query do
   >
   > When selecting a literal atom, its value must be the same across
   > all queries. Otherwise, the value from the parent query will be
-  > applied to all other queries.
+  > applied to all other queries. This also holds true for selecting
+  > maps with atom keys.
 
   Intersect expression returns only unique rows as if each query returned
   distinct results. This may cause a performance penalty. If you need
@@ -2209,7 +2214,8 @@ defmodule Ecto.Query do
   >
   > When selecting a literal atom, its value must be the same across
   > all queries. Otherwise, the value from the parent query will be
-  > applied to all other queries.
+  > applied to all other queries. This also holds true for selecting
+  > maps with atom keys.
 
   Note that the operations `order_by`, `limit` and `offset` of the
   current `query` apply to the result of the set difference. `order_by`

lib/ecto/query/builder/select.ex

@@ -140,6 +140,11 @@ defmodule Ecto.Query.Builder.Select do
     {k, params_acc}
   end
 
+  defp escape_key({:^, _, [k]}, params_acc, _vars, _env) do
+    checked = quote do: Ecto.Query.Builder.Select.map_key!(unquote(k))
+    {checked, params_acc}
+  end
+
   defp escape_key(k, params_acc, vars, env) do
     escape(k, params_acc, vars, env)
   end
@@ -174,6 +179,20 @@ defmodule Ecto.Query.Builder.Select do
     end
   end
 
+  @doc """
+  Called at runtime to verify a map key
+  """
+  def map_key!(key) when is_binary(key), do: key
+  def map_key!(key) when is_integer(key), do: key
+  def map_key!(key) when is_float(key), do: key
+  def map_key!(key) when is_atom(key), do: key
+
+  def map_key!(other) do
+    Builder.error!(
+      "interpolated map keys in `:select` can only be atoms, strings or numbers, got: #{inspect(other)}"
+    )
+  end
+
   # atom list sigils
   defp take?({name, _, [_, modifiers]}) when name in ~w(sigil_w sigil_W)a do
     ?a in modifiers

test/ecto/query/builder/select_test.exs

@@ -486,6 +486,16 @@ defmodule Ecto.Query.Builder.SelectTest do
         %Ecto.Query{} |> select([], 1) |> select([], 2)
       end
     end
+
+    test "supports interpolated map keys" do
+      key = :test_key
+
+      q = from p in "posts", select: %{^key => 1}
+      assert {:%{}, [], [test_key: 1]} = q.select.expr
+
+      q = from p in "posts", select: %{^:test_key => 1}
+      assert {:%{}, [], [test_key: 1]} = q.select.expr
+    end
   end
 
   describe "select_merge" do
@@ -574,7 +584,7 @@ defmodule Ecto.Query.Builder.SelectTest do
         end)
 
       assert Macro.to_string(query.select.expr) ==
-               "merge(merge(%{comments: count(&2.id())}, %{^0 => &0.name()}), %{^1 => &1.author()})"
+               "%{comments: count(&2.id()), name: &0.name(), author: &1.author()}"
     end
 
     test "supports '...' in binding list with no prior select" do
@@ -672,5 +682,24 @@ defmodule Ecto.Query.Builder.SelectTest do
           select_merge: %{t: p.title, b: p.body}
       assert Macro.to_string(query.select.expr) =~ "merge"
     end
+
+    test "supports interpolated map keys" do
+      shared_key = :shared
+      merge_key = :merge
+
+      q =
+        from p in "posts",
+          select: %{^shared_key => :old},
+          select_merge: %{^shared_key => :new, ^merge_key => :merge}
+
+      assert {:%{}, [], [shared: :new, merge: :merge]} = q.select.expr
+
+      q =
+        from p in "posts",
+          select: %{^:shared => :old},
+          select_merge: %{^:shared => :new, ^:merge => :merge}
+
+      assert {:%{}, [], [shared: :new, merge: :merge]} = q.select.expr
+    end
   end
 end

test/ecto/query/planner_test.exs

@@ -1639,6 +1639,22 @@ defmodule Ecto.Query.PlannerTest do
     assert true in union_query.select.fields
   end
 
+  test "normalize: select with combinations and dynamic map keys" do
+    union_map_key = :id
+    outer_map_key = :text
+    union_query = from(c in Comment, select: %{^union_map_key => c.id, :text => c.text})
+
+    {normalized_query, _, _, select} =
+      from(c in Comment, select: %{:id => c.id, ^outer_map_key => c.text}, union: ^union_query)
+      |> normalize_with_params()
+
+    normalized_union_query = normalized_query.combinations |> List.first() |> elem(1)
+
+    assert select.postprocess == {:map, [id: {:value, :id}, text: {:value, :string}]}
+    assert {:%{}, _, [id: _, text: _]} = normalized_query.select.expr
+    assert {:%{}, _, [id: _, text: _]} = normalized_union_query.select.expr
+  end
+
   test "normalize: select on schemaless" do
     assert_raise Ecto.QueryError, ~r"need to explicitly pass a :select clause in query", fn ->
       from("posts", []) |> normalize()