From 4990abda4c504133f89deed469582785166208f5 Mon Sep 17 00:00:00 2001 From: Greg Rychlewski Date: Wed, 25 Dec 2024 09:24:27 -0500 Subject: [PATCH] Allow strings in `field/2` (#4384) --- lib/ecto/query/api.ex | 33 ++++++++++++++++++++-- lib/ecto/query/builder.ex | 48 ++++++++++++++++++++++++++------ lib/ecto/query/inspect.ex | 6 ++++ lib/ecto/query/planner.ex | 2 ++ test/ecto/query/builder_test.exs | 10 ++++++- test/ecto/query/inspect_test.exs | 5 ++++ test/ecto/query/planner_test.exs | 21 ++++++++++++++ 7 files changed, 114 insertions(+), 11 deletions(-) diff --git a/lib/ecto/query/api.ex b/lib/ecto/query/api.ex index 4dfe57d9bd..30422a1e18 100644 --- a/lib/ecto/query/api.ex +++ b/lib/ecto/query/api.ex @@ -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]) diff --git a/lib/ecto/query/builder.ex b/lib/ecto/query/builder.ex index d2d78dfe52..94dbb5649e 100644 --- a/lib/ecto/query/builder.ex +++ b/lib/ecto/query/builder.ex @@ -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 @@ -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 @@ -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} @@ -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. """ @@ -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. @@ -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 diff --git a/lib/ecto/query/inspect.ex b/lib/ecto/query/inspect.ex index b09d2602ee..0a509723bf 100644 --- a/lib/ecto/query/inspect.ex +++ b/lib/ecto/query/inspect.ex @@ -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, []} diff --git a/lib/ecto/query/planner.ex b/lib/ecto/query/planner.ex index ce57afe200..0f1a7a050b 100644 --- a/lib/ecto/query/planner.ex +++ b/lib/ecto/query/planner.ex @@ -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, _, _} -> diff --git a/test/ecto/query/builder_test.exs b/test/ecto/query/builder_test.exs index 2dbd141375..59163d0d3e 100644 --- a/test/ecto/query/builder_test.exs +++ b/test/ecto/query/builder_test.exs @@ -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 @@ -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 @@ -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) diff --git a/test/ecto/query/inspect_test.exs b/test/ecto/query/inspect_test.exs index f2c0f379f7..3f3ce545d1 100644 --- a/test/ecto/query/inspect_test.exs +++ b/test/ecto/query/inspect_test.exs @@ -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 + end + def plan(query) do {query, _, _} = Ecto.Adapter.Queryable.plan_query(:all, Ecto.TestAdapter, query) query diff --git a/test/ecto/query/planner_test.exs b/test/ecto/query/planner_test.exs index 2eca569430..06c84e25d7 100644 --- a/test/ecto/query/planner_test.exs +++ b/test/ecto/query/planner_test.exs @@ -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 @@ -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 @@ -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, _, _} = @@ -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