Skip to content

Commit

Permalink
Allow strings in field/2 (#4384)
Browse files Browse the repository at this point in the history
  • Loading branch information
greg-rychlewski authored Dec 25, 2024
1 parent 06a2969 commit 4990abd
Show file tree
Hide file tree
Showing 7 changed files with 114 additions and 11 deletions.
33 changes: 31 additions & 2 deletions lib/ecto/query/api.ex
Original file line number Diff line number Diff line change
Expand Up @@ -579,13 +579,42 @@ defmodule Ecto.Query.API do
@doc """
Allows a field to be dynamically accessed.
The field name can be given as either an atom or a string. In a schemaless
query, the two types of names behave the same. However, when referencing
a field from a schema the behaviours are different.
Using an atom to reference a schema field will inherit all the properties from
the schema. For example, the field name will be changed to the value of `:source`
before generating the final query and its type behaviour will be dictated by the
one specified in the schema.
Using a string to reference a schema field is equivalent to bypassing all of the
above and accessing the field directly from the source (i.e. the underlying table).
This means the name will not be changed to the value of `:source` and the type
behaviour will be dictated by the underlying driver (e.g. Postgrex or MyXQL).
Take the following schema and query:
defmodule Car do
use Ecto.Schema
schema "cars" do
field :doors, source: :num_doors
field :tires, source: :num_tires
end
end
def at_least_four(doors_or_tires) do
from c in Car,
where: field(c, ^doors_or_tires) >= 4
end
In the example above, both `at_least_four(:doors)` and `at_least_four(:tires)`
would be valid calls as the field is dynamically generated.
In the example above, `at_least_four(:doors)` and `at_least_four("num_doors")`
would be valid ways to return the set of cars having at least 4 doors.
String names can be particularly useful when your application is dynamically
generating many schemaless queries at runtime and you want to avoid creating
a large number of atoms.
"""
def field(source, field), do: doc!([source, field])

Expand Down
48 changes: 40 additions & 8 deletions lib/ecto/query/builder.ex
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ defmodule Ecto.Query.Builder do
defp escape_field!({var, _, context}, field, vars)
when is_atom(var) and is_atom(context) do
var = escape_var!(var, vars)
field = quoted_atom!(field, "field/2")
field = quoted_atom_or_string!(field, "field/2")
dot = {:{}, [], [:., [], [var, field]]}
{:{}, [], [dot, [], []]}
end
Expand All @@ -706,7 +706,7 @@ defmodule Ecto.Query.Builder do
when kind in [:as, :parent_as] do
value = late_binding!(kind, value)
as = {:{}, [], [kind, [], [value]]}
field = quoted_atom!(field, "field/2")
field = quoted_atom_or_string!(field, "field/2")
dot = {:{}, [], [:., [], [as, field]]}
{:{}, [], [dot, [], []]}
end
Expand Down Expand Up @@ -850,9 +850,8 @@ defmodule Ecto.Query.Builder do
do: {find_var!(var, vars), field}

def validate_type!({:field, _, [{var, _, context}, field]}, vars, _env)
when is_atom(var) and is_atom(context) and is_atom(field),
do: {find_var!(var, vars), field}

when is_atom(var) and is_atom(context) and (is_atom(field) or is_binary(field)),
do: {find_var!(var, vars), field}
def validate_type!({:field, _, [{var, _, context}, {:^, _, [field]}]}, vars, _env)
when is_atom(var) and is_atom(context),
do: {find_var!(var, vars), field}
Expand Down Expand Up @@ -1110,6 +1109,27 @@ defmodule Ecto.Query.Builder do
"`#{Macro.to_string(other)}`"
)

@doc """
Checks if the field is an atom or string at compilation time or
delegate the check to runtime for interpolation.
"""
def quoted_atom_or_string!({:^, _, [expr]}, used_ref),
do: quote(do: Ecto.Query.Builder.atom_or_string!(unquote(expr), unquote(used_ref)))

def quoted_atom_or_string!(atom, _used_ref) when is_atom(atom),
do: atom

def quoted_atom_or_string!(string, _used_ref) when is_binary(string),
do: string

def quoted_atom_or_string!(other, used_ref),
do:
error!(
"expected literal atom or string or interpolated value in #{used_ref}, got: " <>
"`#{Macro.to_string(other)}`"
)


@doc """
Called by escaper at runtime to verify that value is an atom.
"""
Expand All @@ -1119,6 +1139,18 @@ defmodule Ecto.Query.Builder do
def atom!(other, used_ref),
do: error!("expected atom in #{used_ref}, got: `#{inspect(other)}`")

@doc """
Called by escaper at runtime to verify that value is an atom or string.
"""
def atom_or_string!(atom, _used_ref) when is_atom(atom),
do: atom

def atom_or_string!(string, _used_ref) when is_binary(string),
do: string

def atom_or_string!(other, used_ref),
do: error!("expected atom or string in #{used_ref}, got: `#{inspect other}`")

@doc """
Checks if the value of a late binding is an interpolation or
a quoted atom.
Expand Down Expand Up @@ -1284,11 +1316,11 @@ defmodule Ecto.Query.Builder do
end

def quoted_type({:field, _, [{var, _, context}, field]}, vars)
when is_atom(var) and is_atom(context) and is_atom(field),
do: {find_var!(var, vars), field}
when is_atom(var) and is_atom(context) and (is_atom(field) or is_binary(field)),
do: {find_var!(var, vars), field}

def quoted_type({:field, _, [{kind, _, [value]}, field]}, _vars)
when kind in [:as, :parent_as] and is_atom(field) do
when kind in [:as, :parent_as] and (is_atom(field) or is_binary(field)) do
value = late_binding!(kind, value)
{{:{}, [], [kind, [], [value]]}, field}
end
Expand Down
6 changes: 6 additions & 0 deletions lib/ecto/query/inspect.ex
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,12 @@ defimpl Inspect, for: Ecto.Query do
binding_to_expr(ix, names, part)
end

# Format field/2 with string name
defp postwalk({{:., _, [{_, _, _} = binding, field]}, meta, []}, _names, _part)
when is_binary(field) do
{:field, meta, [binding, field]}
end

# Remove parens from field calls
defp postwalk({{:., _, [_, _]} = dot, meta, []}, _names, _part) do
{dot, [no_parens: true] ++ meta, []}
Expand Down
2 changes: 2 additions & 0 deletions lib/ecto/query/planner.ex
Original file line number Diff line number Diff line change
Expand Up @@ -2420,6 +2420,8 @@ defmodule Ecto.Query.Planner do

defp type!(_kind, _query, _expr, nil, _field, _allow_virtuals?), do: :any

defp type!(_kind, _query, _expr, _ix, field, _allow_virtuals?) when is_binary(field), do: :any

defp type!(kind, query, expr, ix, field, allow_virtuals?) when is_integer(ix) do
case get_source!(kind, query, ix) do
{:fragment, _, _} ->
Expand Down
10 changes: 9 additions & 1 deletion test/ecto/query/builder_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ defmodule Ecto.Query.BuilderTest do
|> Code.eval_quoted([], __ENV__)
|> elem(0)

assert {{:., [], [{:&, [], [0]}, "z"]}, [], []} ==
escape(quote do field(x, "z") end, [x: 0], __ENV__)
|> elem(0)
|> Code.eval_quoted([], __ENV__)
|> elem(0)

assert {Macro.escape(quote do -&0.y() end), []} ==
escape(quote do -x.y() end, [x: 0], __ENV__)
end
Expand Down Expand Up @@ -266,7 +272,7 @@ defmodule Ecto.Query.BuilderTest do
escape(quote(do: x.y == 1), [], __ENV__)
end

assert_raise Ecto.Query.CompileError, ~r"expected literal atom or interpolated value.*got: `var`", fn ->
assert_raise Ecto.Query.CompileError, ~r"expected literal atom or string or interpolated value.*got: `var`", fn ->
escape(quote(do: field(x, var)), [x: 0], __ENV__) |> elem(0) |> Code.eval_quoted([], __ENV__)
end

Expand Down Expand Up @@ -360,6 +366,8 @@ defmodule Ecto.Query.BuilderTest do
assert validate_type!(quote do x.title end, [x: 0], env) == {0, :title}
assert validate_type!(quote do field(x, :title) end, [x: 0], env) == {0, :title}
assert validate_type!(quote do field(x, ^:title) end, [x: 0], env) == {0, :title}
assert validate_type!(quote do field(x, "title") end, [x: 0], env) == {0, "title"}
assert validate_type!(quote do field(x, ^"title") end, [x: 0], env) == {0, "title"}

assert_raise Ecto.Query.CompileError, ~r"^type/2 expects an alias, atom", fn ->
validate_type!(quote do "string" end, [x: 0], env)
Expand Down
5 changes: 5 additions & 0 deletions test/ecto/query/inspect_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,11 @@ defmodule Ecto.Query.InspectTest do
assert i(plan(query)) == "from v0 in values (#{fields})"
end

test "field/2 with string name" do
query = from p in Post, select: field(p, "visit")
assert i(query) == ~s<from p0 in Inspect.Post, select: field(p0, "visit")>
end

def plan(query) do
{query, _, _} = Ecto.Adapter.Queryable.plan_query(:all, Ecto.TestAdapter, query)
query
Expand Down
21 changes: 21 additions & 0 deletions test/ecto/query/planner_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -1339,6 +1339,12 @@ defmodule Ecto.Query.PlannerTest do
assert Macro.to_string(hd(query.wheres).expr) == "&0.visits() == ^0"
assert cast_params == [123]

{query, cast_params, _, _} =
from(Post, as: :posts, where: field(as(^as), "visits") == ^"123") |> normalize_with_params()

assert Macro.to_string(hd(query.wheres).expr) == "&0 . \"visits\"() == ^0"
assert cast_params == ["123"]

assert_raise Ecto.QueryError, ~r/could not find named binding `as\(:posts\)`/, fn ->
from(Post, where: as(^as).visits == ^"123") |> normalize()
end
Expand All @@ -1353,6 +1359,9 @@ defmodule Ecto.Query.PlannerTest do
query = from(Post, as: ^as, where: field(as(^as), :visits) == ^"123") |> normalize()
assert Macro.to_string(hd(query.wheres).expr) == "&0.visits() == ^0"

query = from(Post, as: ^as, where: field(as(^as), "visits") == ^"123") |> normalize()
assert Macro.to_string(hd(query.wheres).expr) == "&0 . \"visits\"() == ^0"

assert_raise Ecto.QueryError, ~r/could not find named binding `as\(\{:posts\}\)`/, fn ->
from(Post, where: as(^as).visits == ^"123") |> normalize()
end
Expand Down Expand Up @@ -1417,6 +1426,10 @@ defmodule Ecto.Query.PlannerTest do
assert Macro.to_string(hd(query.joins).source.query.select.expr) ==
"%{map: parent_as(:posts).posted()}"

child = from(c in Comment, select: %{map: field(parent_as(^as), "posted")})
query = from(Post, as: :posts, join: c in subquery(child), on: true) |> normalize()
assert Macro.to_string(hd(query.joins).source.query.select.expr) == "%{map: parent_as(:posts) . \"posted\"()}"

child = from(c in Comment, where: parent_as(^as).visits == ^"123")

{query, cast_params, _, _} =
Expand All @@ -1436,6 +1449,14 @@ defmodule Ecto.Query.PlannerTest do
"parent_as(:posts).visits() == ^0"

assert cast_params == [123]

child = from(c in Comment, where: field(parent_as(^as), "visits") == ^"123")

{query, cast_params, _, _} =
from(Post, as: :posts, join: c in subquery(child), on: true) |> normalize_with_params()

assert Macro.to_string(hd(hd(query.joins).source.query.wheres).expr) == "parent_as(:posts) . \"visits\"() == ^0"
assert cast_params == ["123"]
end

test "normalize: nested parent_as" do
Expand Down

0 comments on commit 4990abd

Please sign in to comment.