Skip to content

Commit

Permalink
Improve error message on custom preload, closes #4554
Browse files Browse the repository at this point in the history
  • Loading branch information
@josevalim
josevalim committed Dec 2, 2024
1 parent c0203db commit 4d3c5ee
Showing 1 changed file with 55 additions and 41 deletions.
96 changes: 55 additions & 41 deletions lib/ecto/query/builder/preload.ex
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,61 +44,65 @@ defmodule Ecto.Query.Builder.Preload do
** (Ecto.Query.CompileError) cannot preload join association `:bar` with binding `c` because parent preload is not a join association
"""
@spec escape(Macro.t, Keyword.t) :: {[Macro.t], [Macro.t]}
@spec escape(Macro.t(), Keyword.t()) :: {[Macro.t()], [Macro.t()]}
def escape(preloads, vars) do
{preloads, assocs} = escape(preloads, :both, [], [], vars)
{Enum.reverse(preloads), Enum.reverse(assocs)}
end

defp escape(atom, _mode, preloads, assocs, _vars) when is_atom(atom) do
{[atom|preloads], assocs}
{[atom | preloads], assocs}
end

defp escape(list, mode, preloads, assocs, vars) when is_list(list) do
Enum.reduce list, {preloads, assocs}, fn item, acc ->
Enum.reduce(list, {preloads, assocs}, fn item, acc ->
escape_each(item, mode, acc, vars)
end
end)
end

defp escape({:^, _, [inner]}, _mode, preloads, assocs, _vars) do
{[inner|preloads], assocs}
{[inner | preloads], assocs}
end

defp escape(other, _mode, _preloads, _assocs, _vars) do
Builder.error! "`#{Macro.to_string other}` is not a valid preload expression. " <>
"preload expects an atom, a list of atoms or a keyword list with " <>
"more preloads as values. Use ^ on the outermost preload to interpolate a value"
Builder.error!(
"`#{Macro.to_string(other)}` is not a valid preload expression. " <>
"preload expects an atom, a list of atoms or a keyword list with " <>
"more preloads as values. Use ^ on the outermost preload to interpolate a value"
)
end

defp escape_each({key, {:^, _, [inner]}}, _mode, {preloads, assocs}, _vars) do
key = escape_key(key)
{[{key, inner}|preloads], assocs}
{[{key, inner} | preloads], assocs}
end

defp escape_each({key, {var, _, context}}, mode, {preloads, assocs}, vars) when is_atom(context) do
defp escape_each({key, {var, _, context}}, mode, {preloads, assocs}, vars)
when is_atom(context) do
assert_assoc!(mode, key, var)
key = escape_key(key)
idx = Builder.find_var!(var, vars)
{preloads, [{key, {idx, []}}|assocs]}
{preloads, [{key, {idx, []}} | assocs]}
end

defp escape_each({key, {{var, _, context}, list}}, mode, {preloads, assocs}, vars) when is_atom(context) do
defp escape_each({key, {{var, _, context}, list}}, mode, {preloads, assocs}, vars)
when is_atom(context) do
assert_assoc!(mode, key, var)
key = escape_key(key)
idx = Builder.find_var!(var, vars)
{inner_preloads, inner_assocs} = escape(list, :assoc, [], [], vars)
assocs = [{key, {idx, Enum.reverse(inner_assocs)}}|assocs]
assocs = [{key, {idx, Enum.reverse(inner_assocs)}} | assocs]

case inner_preloads do
[] -> {preloads, assocs}
_ -> {[{key, Enum.reverse(inner_preloads)}|preloads], assocs}
_ -> {[{key, Enum.reverse(inner_preloads)} | preloads], assocs}
end
end

defp escape_each({key, list}, _mode, {preloads, assocs}, vars) do
key = escape_key(key)
{inner_preloads, []} = escape(list, :preload, [], [], vars)
{[{key, Enum.reverse(inner_preloads)}|preloads], assocs}
{[{key, Enum.reverse(inner_preloads)} | preloads], assocs}
end

defp escape_each(other, mode, {preloads, assocs}, vars) do
Expand All @@ -114,17 +118,18 @@ defmodule Ecto.Query.Builder.Preload do
end

defp escape_key(other) do
Builder.error! "malformed key in preload `#{Macro.to_string(other)}` in query expression"
Builder.error!("malformed key in preload `#{Macro.to_string(other)}` in query expression")
end

@doc """
Called at runtime to check dynamic preload keys.
"""
def key!(key) when is_atom(key),
do: key

def key!(key) do
raise ArgumentError,
"expected key in preload to be an atom, got: `#{inspect key}`"
"expected key in preload to be an atom, got: `#{inspect(key)}`"
end

@doc """
Expand All @@ -134,7 +139,7 @@ defmodule Ecto.Query.Builder.Preload do
If possible, it does all calculations at compile time to avoid
runtime work.
"""
@spec build(Macro.t, [Macro.t], Macro.t, Macro.Env.t) :: Macro.t
@spec build(Macro.t(), [Macro.t()], Macro.t(), Macro.Env.t()) :: Macro.t()
def build(query, _binding, {:^, _, [expr]}, _env) do
quote do
Ecto.Query.Builder.Preload.preload!(unquote(query), unquote(expr))
Expand All @@ -150,10 +155,11 @@ defmodule Ecto.Query.Builder.Preload do
@doc """
The callback applied by `build/4` to build the query.
"""
@spec apply(Ecto.Queryable.t, term, term) :: Ecto.Query.t
@spec apply(Ecto.Queryable.t(), term, term) :: Ecto.Query.t()
def apply(%Ecto.Query{preloads: p, assocs: a} = query, preloads, assocs) do
%{query | preloads: p ++ preloads, assocs: a ++ assocs}
end

def apply(query, preloads, assocs) do
apply(Ecto.Queryable.to_query(query), preloads, assocs)
end
Expand Down Expand Up @@ -205,7 +211,7 @@ defmodule Ecto.Query.Builder.Preload do
end

defp expand(atom, _query, _mode, preloads, assocs) when is_atom(atom) do
{[atom|preloads], assocs}
{[atom | preloads], assocs}
end

defp expand(list, query, mode, preloads, assocs) when is_list(list) do
Expand All @@ -219,39 +225,44 @@ defmodule Ecto.Query.Builder.Preload do

defp expand(other, _query, _mode, _preloads, _assocs) do
raise ArgumentError,
"`#{inspect(other)}` is not a valid preload expression, " <>
"expected an atom or a list."
"`#{inspect(other)}` is not a valid preload expression, " <>
"expected an atom or a list."
end

defp expand_each(atom, _query, _mode, {preloads, assocs}) when is_atom(atom) do
{[atom|preloads], assocs}
{[atom | preloads], assocs}
end

defp expand_each({key, atom}, _query, _mode, {preloads, assocs}) when is_atom(atom) do
assert_key!(key)

{[{key, atom}|preloads], assocs}
{[{key, atom} | preloads], assocs}
end

defp expand_each({key, %Ecto.Query.DynamicExpr{} = dynamic}, query, mode, {preloads, assocs}) do
assert_key!(key)
assert_assoc!(mode, key)

idx = expand_dynamic(dynamic, query)
{preloads, [{key, {idx, []}}|assocs]}
{preloads, [{key, {idx, []}} | assocs]}
end

defp expand_each({key, {%Ecto.Query.DynamicExpr{} = dynamic, inner}}, query, mode, {preloads, assocs}) do
defp expand_each(
{key, {%Ecto.Query.DynamicExpr{} = dynamic, inner}},
query,
mode,
{preloads, assocs}
) do
assert_key!(key)
assert_assoc!(mode, key)

idx = expand_dynamic(dynamic, query)
{inner_preloads, inner_assocs} = expand(inner, query, :assoc, [], [])
assocs = [{key, {idx, inner_assocs}}|assocs]
assocs = [{key, {idx, inner_assocs}} | assocs]

case inner_preloads do
[] -> {preloads, assocs}
_ -> {[{key, inner_preloads}|preloads], assocs}
_ -> {[{key, inner_preloads} | preloads], assocs}
end
end

Expand All @@ -260,21 +271,21 @@ defmodule Ecto.Query.Builder.Preload do
assert_query_or_fun!(query_or_fun, key)

{inner_preloads, []} = expand(inner, query, :preload, [], [])
{[{key, {query_or_fun, inner_preloads}}|preloads], assocs}
{[{key, {query_or_fun, inner_preloads}} | preloads], assocs}
end

defp expand_each({key, list}, query, _mode, {preloads, assocs}) when is_list(list) do
assert_key!(key)

{inner_preloads, []} = expand(list, query, :preload, [], [])
{[{key, inner_preloads}|preloads], assocs}
{[{key, inner_preloads} | preloads], assocs}
end

defp expand_each({key, query_or_fun}, _query, _mode, {preloads, assocs}) do
assert_key!(key)
assert_query_or_fun!(query_or_fun, key)

{[{key, query_or_fun}|preloads], assocs}
{[{key, query_or_fun} | preloads], assocs}
end

defp expand_each(other, query, mode, {preloads, assocs}) do
Expand All @@ -288,35 +299,38 @@ defmodule Ecto.Query.Builder.Preload do

_ ->
raise ArgumentError,
"invalid dynamic in preload: `#{inspect(dynamic)}`. " <>
"Dynamic expressions in preload must evaluate to a single binding, as in: " <>
"`dynamic([comments: c], c)`"
"invalid dynamic in preload: `#{inspect(dynamic)}`. " <>
"Dynamic expressions in preload must evaluate to a single binding, as in: " <>
"`dynamic([comments: c], c)`"
end
end

defp assert_key!(key), do: key!(key) && :ok

defp assert_query_or_fun!(%Ecto.Query{}, _key), do: :ok
defp assert_query_or_fun!(fun, _key) when is_function(fun, 1), do: :ok

defp assert_query_or_fun!(other, key) do
raise ArgumentError,
"invalid preload for key `#{inspect(key)}`: #{inspect(other)}. " <>
"Preloads can be a query, a function (arity 1), or a dynamic that " <>
"evaluates to a single binding."
"invalid preload for key `#{inspect(key)}`: #{inspect(other)}. " <>
"Preloads can be a query, a function expecting one or two arguments, " <>
"or a dynamic that evaluates to a single binding"
end

defp assert_assoc!(mode, _atom) when mode in [:both, :assoc], do: :ok

defp assert_assoc!(_mode, atom) do
raise ArgumentError,
"cannot preload join association `#{inspect(atom)}` " <>
"because parent preload is not a join association"
"cannot preload join association `#{inspect(atom)}` " <>
"because parent preload is not a join association"
end

defp assert_assoc!(mode, _atom, _var) when mode in [:both, :assoc], do: :ok

defp assert_assoc!(_mode, atom, var) do
Builder.error!(
"cannot preload join association `#{Macro.to_string atom}` with binding `#{var}` " <>
"because parent preload is not a join association"
"cannot preload join association `#{Macro.to_string(atom)}` with binding `#{var}` " <>
"because parent preload is not a join association"
)
end
end

0 comments on commit 4d3c5ee

Please sign in to comment.