From c7000717cfb641f80591164160f0dda4135c139f Mon Sep 17 00:00:00 2001 From: Greg Date: Wed, 8 Jan 2025 22:27:09 -0500 Subject: [PATCH 1/4] Revert "Allow numbers in literal/1 (#4562)" This reverts commit 3ab8279b574b0162372c838441ce9ec1772a71e1. --- lib/ecto/query/api.ex | 15 ++++++--------- lib/ecto/query/builder.ex | 11 ++++++----- test/ecto/query_test.exs | 17 +++-------------- 3 files changed, 15 insertions(+), 28 deletions(-) diff --git a/lib/ecto/query/api.ex b/lib/ecto/query/api.ex index d831c28f06..30422a1e18 100644 --- a/lib/ecto/query/api.ex +++ b/lib/ecto/query/api.ex @@ -480,20 +480,17 @@ defmodule Ecto.Query.API do def fragment(fragments), do: doc!([fragments]) @doc """ - Allows a literal identifier or number to be injected into a fragment: + Allows a literal identifier to be injected into a fragment: collation = "es_ES" fragment("? COLLATE ?", ^name, literal(^collation)) - limit = 10 - limit(query, fragment("?", literal(^limit))) - - The example above will inject `collation` and `limit` into the queries as - literals instead of query parameters. Note that each different value passed - to `literal/1` will emit a different query, which will be independently prepared - and cached. + The example above will inject `collation` into the query as + a literal identifier instead of a query parameter. Note that + each different value of `collation` will emit a different query, + which will be independently prepared and cached. """ - def literal(literal), do: doc!([literal]) + def literal(binary), do: doc!([binary]) @doc """ Allows a list argument to be spliced into a fragment. diff --git a/lib/ecto/query/builder.ex b/lib/ecto/query/builder.ex index 6d40a4a38a..94dbb5649e 100644 --- a/lib/ecto/query/builder.ex +++ b/lib/ecto/query/builder.ex @@ -1256,12 +1256,13 @@ defmodule Ecto.Query.Builder do @doc """ Called by escaper at runtime to verify literal in fragments. """ - def literal!(literal) when is_binary(literal), do: literal - def literal!(literal) when is_number(literal), do: literal - def literal!(literal) do - raise ArgumentError, - "literal(^value) expects `value` to be a string or a number, got `#{inspect(literal)}`" + if is_binary(literal) do + literal + else + raise ArgumentError, + "literal(^value) expects `value` to be a string, got `#{inspect(literal)}`" + end end @doc """ diff --git a/test/ecto/query_test.exs b/test/ecto/query_test.exs index dd84544718..e3b8307acb 100644 --- a/test/ecto/query_test.exs +++ b/test/ecto/query_test.exs @@ -988,20 +988,9 @@ defmodule Ecto.QueryTest do raw: "" ] = parts - query = from p in "posts", limit: fragment("?", literal(^1)) - assert {:fragment, _, parts} = query.limit.expr - - assert [ - raw: "", - expr: {:literal, _, [1]}, - raw: "" - ] = parts - - assert_raise ArgumentError, - "literal(^value) expects `value` to be a string or a number, got `%{}`", - fn -> - from p in "posts", select: fragment("? COLLATE ?", p.name, literal(^%{})) - end + assert_raise ArgumentError, "literal(^value) expects `value` to be a string, got `123`", fn -> + from p in "posts", select: fragment("? COLLATE ?", p.name, literal(^123)) + end end test "supports list splicing" do From c161ae3a4c6d0ed55ea7d076bc672b1a301546db Mon Sep 17 00:00:00 2001 From: Greg Date: Wed, 8 Jan 2025 23:03:17 -0500 Subject: [PATCH 2/4] introduce_identifier_constant --- lib/ecto/query/api.ex | 34 ++++++++++++++++++++++++- lib/ecto/query/builder.ex | 52 +++++++++++++++++++++++++++++++++++++++ test/ecto/query_test.exs | 37 ++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 1 deletion(-) diff --git a/lib/ecto/query/api.ex b/lib/ecto/query/api.ex index 30422a1e18..ea23d8225b 100644 --- a/lib/ecto/query/api.ex +++ b/lib/ecto/query/api.ex @@ -492,6 +492,38 @@ defmodule Ecto.Query.API do """ def literal(binary), do: doc!([binary]) + @doc """ + Allows a dynamic identifier to be injected into a fragment: + + collation = "es_ES" + select("posts", [p], fragment("? COLLATE ?", p.title, identifier(^"es_ES"))) + + The example above will inject the value of `collation` directly + into the query instead of treating it as a query parameter. It will + generate a query such as `SELECT p0.title COLLATE "es_ES" FROM "posts" AS p0` + as opposed to `SELECT p0.title COLLATE $1 FROM "posts" AS p0`. + + Note that each different value of `collation` will emit a different query, + which will be independently prepared and cached. + """ + def identifier(binary), do: doc!([binary]) + + @doc """ + Allows a dynamic string or number to be injected into a fragment: + + limit = 10 + "posts" |> select([p], p.title) |> limit(fragment("?", constant(^limit))) + + The example above will inject the value of `limit` directly + into the query instead of treating it as a query parameter. It will + generate a query such as `SELECT p0.title FROM "posts" AS p0 LIMIT 1` + as opposed to `SELECT p0.title FROM "posts" AS p0` LIMIT $1`. + + Note that each different value of `limit` will emit a different query, + which will be independently prepared and cached. + """ + def constant(value), do: doc!([value]) + @doc """ Allows a list argument to be spliced into a fragment. @@ -504,7 +536,7 @@ defmodule Ecto.Query.API do You may only splice runtime values. For example, this would not work because query bindings are compile-time constructs: - from p in Post, where: fragment("concat(?)", splice(^[p.count, " ", "count"]) + from p in Post, where: fragment("concat(?)", splice(^[p.count, " ", "count"])) """ def splice(list), do: doc!([list]) diff --git a/lib/ecto/query/builder.ex b/lib/ecto/query/builder.ex index 94dbb5649e..091e842727 100644 --- a/lib/ecto/query/builder.ex +++ b/lib/ecto/query/builder.ex @@ -765,6 +765,34 @@ defmodule Ecto.Query.Builder do end end + defp escape_fragment({:identifier, _meta, [expr]}, params_acc, _vars, _env) do + case expr do + {:^, _, [expr]} -> + checked = quote do: Ecto.Query.Builder.identifier!(unquote(expr)) + escaped = {:{}, [], [:identifier, [], [checked]]} + {escaped, params_acc} + + _ -> + error!( + "identifier/1 in fragment expects an interpolated value, such as identifier(^value), got `#{Macro.to_string(expr)}`" + ) + end + end + + defp escape_fragment({:constant, _meta, [expr]}, params_acc, _vars, _env) do + case expr do + {:^, _, [expr]} -> + checked = quote do: Ecto.Query.Builder.constant!(unquote(expr)) + escaped = {:{}, [], [:constant, [], [checked]]} + {escaped, params_acc} + + _ -> + error!( + "constant/1 in fragment expects an interpolated value, such as constant(^value), got `#{Macro.to_string(expr)}`" + ) + end + end + defp escape_fragment({:splice, _meta, [splice]}, params_acc, vars, env) do case splice do {:^, _, [value]} = expr -> @@ -1265,6 +1293,30 @@ defmodule Ecto.Query.Builder do end end + @doc """ + Called by escaper at runtime to verify identifier in fragments. + """ + def identifier!(identifier) do + if is_binary(identifier) do + identifier + else + raise ArgumentError, + "identifier(^value) expects `value` to be a string, got `#{inspect(identifier)}`" + end + end + + @doc """ + Called by escaper at runtime to verify constant in fragments. + """ + def constant!(constant) do + if is_binary(constant) or is_number(constant) do + constant + else + raise ArgumentError, + "constant(^value) expects `value` to be a string or a number, got `#{inspect(constant)}`" + end + end + @doc """ Called by escaper at runtime to verify splice in fragments. """ diff --git a/test/ecto/query_test.exs b/test/ecto/query_test.exs index e3b8307acb..ccf24cbf92 100644 --- a/test/ecto/query_test.exs +++ b/test/ecto/query_test.exs @@ -993,6 +993,43 @@ defmodule Ecto.QueryTest do end end + test "supports identifiers" do + query = from p in "posts", select: fragment("? COLLATE ?", p.name, identifier(^"es_ES")) + assert {:fragment, _, parts} = query.select.expr + + assert [ + raw: "", + expr: {{:., _, [{:&, _, [0]}, :name]}, _, _}, + raw: " COLLATE ", + expr: {:identifier, _, ["es_ES"]}, + raw: "" + ] = parts + + msg = "identifier(^value) expects `value` to be a string, got `123`" + + assert_raise ArgumentError, msg, fn -> + from p in "posts", select: fragment("? COLLATE ?", p.name, identifier(^123)) + end + end + + test "supports constants" do + query = + from p in "posts", + select: fragment("?", constant(^"hi")), + limit: fragment("?", constant(^1)) + + assert {:fragment, _, select_parts} = query.select.expr + assert {:fragment, _, limit_parts} = query.limit.expr + assert [raw: "", expr: {:constant, _, ["hi"]}, raw: ""] = select_parts + assert [raw: "", expr: {:constant, _, [1]}, raw: ""] = limit_parts + + msg = "constant(^value) expects `value` to be a string or a number, got `%{}`" + + assert_raise ArgumentError, msg, fn -> + from p in "posts", limit: fragment("?", constant(^%{})) + end + end + test "supports list splicing" do two = 2 three = 3 From 6be76a6f560d689c132c9c6bf4a7c7fee8df1401 Mon Sep 17 00:00:00 2001 From: Greg Date: Thu, 9 Jan 2025 04:58:16 -0500 Subject: [PATCH 3/4] review comments --- lib/ecto/query/api.ex | 13 ------------- lib/ecto/query/builder.ex | 31 ++++++++----------------------- test/ecto/query_test.exs | 17 ----------------- 3 files changed, 8 insertions(+), 53 deletions(-) diff --git a/lib/ecto/query/api.ex b/lib/ecto/query/api.ex index ea23d8225b..20ed4fe212 100644 --- a/lib/ecto/query/api.ex +++ b/lib/ecto/query/api.ex @@ -479,19 +479,6 @@ defmodule Ecto.Query.API do """ def fragment(fragments), do: doc!([fragments]) - @doc """ - Allows a literal identifier to be injected into a fragment: - - collation = "es_ES" - fragment("? COLLATE ?", ^name, literal(^collation)) - - The example above will inject `collation` into the query as - a literal identifier instead of a query parameter. Note that - each different value of `collation` will emit a different query, - which will be independently prepared and cached. - """ - def literal(binary), do: doc!([binary]) - @doc """ Allows a dynamic identifier to be injected into a fragment: diff --git a/lib/ecto/query/builder.ex b/lib/ecto/query/builder.ex index 091e842727..cf1094ed44 100644 --- a/lib/ecto/query/builder.ex +++ b/lib/ecto/query/builder.ex @@ -751,18 +751,15 @@ defmodule Ecto.Query.Builder do ) end - defp escape_fragment({:literal, _meta, [expr]}, params_acc, _vars, _env) do - case expr do - {:^, _, [expr]} -> - checked = quote do: Ecto.Query.Builder.literal!(unquote(expr)) - escaped = {:{}, [], [:literal, [], [checked]]} - {escaped, params_acc} + defp escape_fragment({:literal, meta, [expr]}, params_acc, vars, env) do + env = if {env, _fun} = env, do: env, else: env - _ -> - error!( - "literal/1 in fragment expects an interpolated value, such as literal(^value), got `#{Macro.to_string(expr)}`" - ) - end + IO.warn( + "`literal/1` is deprecated. Please use `identifier/1` instead.", + Macro.Env.stacktrace(env) + ) + + escape_fragment({:identifier, meta, [expr]}, params_acc, vars, env) end defp escape_fragment({:identifier, _meta, [expr]}, params_acc, _vars, _env) do @@ -1281,18 +1278,6 @@ defmodule Ecto.Query.Builder do end end - @doc """ - Called by escaper at runtime to verify literal in fragments. - """ - def literal!(literal) do - if is_binary(literal) do - literal - else - raise ArgumentError, - "literal(^value) expects `value` to be a string, got `#{inspect(literal)}`" - end - end - @doc """ Called by escaper at runtime to verify identifier in fragments. """ diff --git a/test/ecto/query_test.exs b/test/ecto/query_test.exs index ccf24cbf92..1647e89291 100644 --- a/test/ecto/query_test.exs +++ b/test/ecto/query_test.exs @@ -976,23 +976,6 @@ defmodule Ecto.QueryTest do end end - test "supports literals" do - query = from p in "posts", select: fragment("? COLLATE ?", p.name, literal(^"es_ES")) - assert {:fragment, _, parts} = query.select.expr - - assert [ - raw: "", - expr: {{:., _, [{:&, _, [0]}, :name]}, _, _}, - raw: " COLLATE ", - expr: {:literal, _, ["es_ES"]}, - raw: "" - ] = parts - - assert_raise ArgumentError, "literal(^value) expects `value` to be a string, got `123`", fn -> - from p in "posts", select: fragment("? COLLATE ?", p.name, literal(^123)) - end - end - test "supports identifiers" do query = from p in "posts", select: fragment("? COLLATE ?", p.name, identifier(^"es_ES")) assert {:fragment, _, parts} = query.select.expr From e15a8ce3bda4294a4ecfc7317968a67b50fbb5b3 Mon Sep 17 00:00:00 2001 From: Greg Date: Thu, 9 Jan 2025 05:01:40 -0500 Subject: [PATCH 4/4] oops --- lib/ecto/query/builder.ex | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/ecto/query/builder.ex b/lib/ecto/query/builder.ex index cf1094ed44..f1e3256bcc 100644 --- a/lib/ecto/query/builder.ex +++ b/lib/ecto/query/builder.ex @@ -752,7 +752,11 @@ defmodule Ecto.Query.Builder do end defp escape_fragment({:literal, meta, [expr]}, params_acc, vars, env) do - env = if {env, _fun} = env, do: env, else: env + env = + case env do + {env, _fun} -> env + env -> env + end IO.warn( "`literal/1` is deprecated. Please use `identifier/1` instead.",