address review comments

elixir-ecto · greg-rychlewski · Aug 3, 2024 · Jul 30, 2024 · Aug 1, 2024 · Aug 1, 2024
commit 738e55e9b07481976a75c1b68e5f14a52d33c0f2
diff --git a/integration_test/cases/repo.exs b/integration_test/cases/repo.exs
@@ -970,81 970,6 @@ defmodule Ecto.Integration.RepoTest do
  assert {1, _} = TestRepo.insert_all(Permalink, source)
  end
 
- @tag :upsert_all
- @tag :with_conflict_target
- @tag :returning
- test "insert_all with query and source" do
- {:ok, %User{id: u_id}} = TestRepo.insert(%User{})
- {:ok, %Comment{id: c_id}} = TestRepo.insert(%Comment{author_id: u_id})
- source = from c in Comment, select: c, where: c.id == ^c_id
-
- assert {1, [%Comment{id: ^c_id}]} =
- TestRepo.insert_all(Comment, source,
- conflict_target: [:id],
- on_conflict: :replace_all,
- returning: true
- )
-
- source =
- from c in Comment, join: u in User, on: c.author_id == u.id, select: u, where: c.id == ^c_id
-
- assert {1, [%User{id: ^u_id}]} =
- TestRepo.insert_all(User, source,
- conflict_target: [:id],
- on_conflict: :replace_all,
- returning: true
- )
- end
-
- @tag :upsert_all
- @tag :with_conflict_target
- @tag :returning
- test "insert_all with query and source with update syntax" do
- {:ok, %Post{id: p_id, visits: visits}} = TestRepo.insert(%Post{visits: 10})
-
- {:ok, %Comment{id: c_id, text: text, lock_version: version}} =
- TestRepo.insert(%Comment{text: "comment1_text", lock_version: 1, post_id: p_id})
-
- # select param
- new_lock_version = version 1
-
- source =
- from c in Comment, select: %{c | lock_version: ^new_lock_version}, where: c.id == ^c_id
-
- assert {1, [%Comment{lock_version: ^new_lock_version, text: ^text}]} =
- TestRepo.insert_all(Comment, source,
- conflict_target: [:id],
- on_conflict: :replace_all,
- returning: true
- )
-
- # select literal
- source = from c in Comment, select: %{c | lock_version: 2}, where: c.id == ^c_id
-
- assert {1, [%Comment{lock_version: 2, text: ^text}]} =
- TestRepo.insert_all(Comment, source,
- conflict_target: [:id],
- on_conflict: :replace_all,
- returning: true
- )
-
- # select another field
- source =
- from c in Comment,
- join: p in Post,
- on: c.post_id == p.id,
- select: %{c | lock_version: p.visits},
- where: c.id == ^c_id
-
- assert {1, [%Comment{lock_version: ^visits, text: ^text}]} =
- TestRepo.insert_all(Comment, source,
- conflict_target: [:id],
- on_conflict: :replace_all,
- returning: true
- )
- end
- end
-
  @tag :invalid_prefix
  @tag :insert_cell_wise_defaults
  test "insert all with invalid prefix" do

diff --git 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, �to.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
@@ -108,84 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
- �to.Query.SelectExpr{expr: {:&, _, [ix]}} -> ix
- _ -> nil
- end
  ix =
  case query.select do
  �to.Query.SelectExpr{expr: {:&, _, [ix]}} -> ix
  _ -> nil
  end
 
- header = case query.select do
- �to.Query.SelectExpr{expr: {:%{}, [], [{:|, _, [{:&, _, [ix]}, args]}]}, fields: fields} ->
- select_fields = for {{:., _, [{:&, _, [^ix]}, field]}, [], []} <- fields, do: field
- update_fields = Keyword.keys(args)
-
- select_fields update_fields
- |> Enum.map(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
  �to.Query.SelectExpr{expr: {:%{}, [], [{:|, _, [{:&, _, [ix]}, args]}]}, fields: fields} ->
  select_fields =
  for {{:., _, [{:&, _, [^ix]}, _]}, [], []} = expr <- fields,
  do: insert_all_select_dump!(expr)
 
- �to.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)
  update_fields = args |> Keyword.keys() |> Enum.map(&insert_all_select_dump!(&1, dumper))
  select_fields update_fields
 
- �to.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)
  �to.Query.SelectExpr{expr: {:%{}, _ctx, args}} ->
  Enum.map(args, fn {field, _} -> insert_all_select_dump!(field, dumper) end)
 
- �to.Query.SelectExpr{expr: {:&, _, [_ix]}, fields: fields} ->
- Enum.map(fields, fn {{:., _, [{:&, _, [_ix]}, field]}, [], []} ->
- case dumper do
- %{^field => {source, _, false}} -> source
- %{} -> raise ArgumentError, "cannot select unwritable field `#{field}` for insert_all"
- nil -> field
- end
- end)
  �to.Query.SelectExpr{take: %{^ix => {_fun, fields}}} ->
  Enum.map(fields, &insert_all_select_dump!(&1, dumper))
 
- �to.Query.SelectExpr{expr: {:merge, _, _}} ->
- raise ArgumentError, """
- the source query given to `insert_all` has selected both `map/2` and
- a literal map:
  �to.Query.SelectExpr{expr: {:&, _, [_ix]}, fields: fields} ->
  Enum.map(fields, &insert_all_select_dump!(&1))
 
- #{inspect query}
  �to.Query.SelectExpr{expr: {:merge, _, _}} ->
  raise ArgumentError, """
  the source query given to `insert_all` has selected both `map/2` and
  a literal map:
 
- when using `select_merge` with `insert_all`, you must always use literal
- maps or always use `map/2`. The two cannot be combined.
- """
  #{inspect(query)}
 
- _ ->
- 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:
  when using `select_merge` with `insert_all`, you must always use literal
  maps or always use `map/2`. The two cannot be combined.
  """
 
- #{inspect query}
  _ ->
  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:
 
- Please add a select clause that selects into a map, like this:
   #{inspect(query)}
 
- from x in Source,
- ...,
- select: %{
- field_a: x.bar,
- field_b: x.foo
- }
  Please add a select clause that selects into a map, like this:
 
- All keys must exist in the schema that is being inserted into
- """
- end
  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
 
@@ -293,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

diff --git 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
@@ -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}