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

Author: greg-rychlewski

lib/ecto/query/api.ex

@@ -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.
@@ -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])
 

lib/ecto/query/builder.ex

@@ -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
@@ -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 """

test/ecto/query_test.exs

@@ -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