Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Allow numbers in literal/1 (#4562)" and add identifier/1 and constant/1 instead #4563

Merged
merged 4 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions lib/ecto/query/api.ex
Original file line number Diff line number Diff line change
Expand Up @@ -480,20 +480,36 @@ 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 dynamic identifier to be injected into a fragment:

collation = "es_ES"
fragment("? COLLATE ?", ^name, literal(^collation))
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
limit(query, fragment("?", literal(^limit)))
"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`.

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.
Note that each different value of `limit` will emit a different query,
which will be independently prepared and cached.
"""
def literal(literal), do: doc!([literal])
def constant(value), do: doc!([value])

@doc """
Allows a list argument to be spliced into a fragment.
Expand All @@ -507,7 +523,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])

Expand Down
62 changes: 52 additions & 10 deletions lib/ecto/query/builder.ex
Original file line number Diff line number Diff line change
Expand Up @@ -751,16 +751,45 @@ defmodule Ecto.Query.Builder do
)
end

defp escape_fragment({:literal, _meta, [expr]}, params_acc, _vars, _env) do
defp escape_fragment({:literal, meta, [expr]}, params_acc, vars, env) do
env =
case env do
{env, _fun} -> env
env -> env
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
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.literal!(unquote(expr))
escaped = {:{}, [], [:literal, [], [checked]]}
checked = quote do: Ecto.Query.Builder.constant!(unquote(expr))
escaped = {:{}, [], [:constant, [], [checked]]}
{escaped, params_acc}

_ ->
error!(
"literal/1 in fragment expects an interpolated value, such as literal(^value), got `#{Macro.to_string(expr)}`"
"constant/1 in fragment expects an interpolated value, such as constant(^value), got `#{Macro.to_string(expr)}`"
)
end
end
Expand Down Expand Up @@ -1254,14 +1283,27 @@ defmodule Ecto.Query.Builder do
end

@doc """
Called by escaper at runtime to verify literal in fragments.
Called by escaper at runtime to verify identifier in fragments.
"""
def literal!(literal) when is_binary(literal), do: literal
def literal!(literal) when is_number(literal), do: literal
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

def literal!(literal) do
raise ArgumentError,
"literal(^value) expects `value` to be a string or a number, got `#{inspect(literal)}`"
@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 """
Expand Down
39 changes: 24 additions & 15 deletions test/ecto/query_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -976,32 +976,41 @@ defmodule Ecto.QueryTest do
end
end

test "supports literals" do
query = from p in "posts", select: fragment("? COLLATE ?", p.name, literal(^"es_ES"))
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: {:literal, _, ["es_ES"]},
expr: {:identifier, _, ["es_ES"]},
raw: ""
] = parts

query = from p in "posts", limit: fragment("?", literal(^1))
assert {:fragment, _, parts} = query.limit.expr
msg = "identifier(^value) expects `value` to be a string, got `123`"

assert [
raw: "",
expr: {:literal, _, [1]},
raw: ""
] = parts
assert_raise ArgumentError, msg, fn ->
from p in "posts", select: fragment("? COLLATE ?", p.name, identifier(^123))
end
end

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
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
Expand Down
Loading