Repository: elixir-ecto/ecto (elixir)

Issue:
Query fails to encode on literal UUID
### Elixir version

All

### Database and Version

PostgreSQL (all)

### Ecto Versions

All

### Database Adapter and Versions (postgrex, myxql, etc)

postgrex

### Current behavior

A query on a Ecto.UUID field, where the UUID is literal, emits SQL with invalid bytes:

```
where([s], s.uuid == "017f65d1-80bd-152d-f997-afa6dd33a00f")
```

The reason why this happens is because the Planner calls Ecto.UUID.dump, which returns a binary, which we then promptly inject into the query.

Ecto actually has a mechanism to deal with these, called Ecto.Query.Tagged, and the original parameter is wrapped in such a struct, but the struct is removed during the Planner.prewalk time. The only reason why this doesn't happen to binaries is because we explicit keep the tagged struct:

```
  defp prewalk(%Ecto.Query.Tagged{value: v, type: type} = tagged, kind, query, expr, acc, adapter) do
    if Ecto.Type.base?(type) do
      {tagged, acc}
    else
      {dump_param(kind, query, expr, v, type, adapter), acc}
    end
  end
```


### Expected behavior


We should probably dump the param and check its underlying type, if the value is a string and the underlying type is not a string (or any), then we should probably keep the tagged struct. Note we would still need to support Tagged UUIDs in EctoSQL.

Pull request:
Raise on literal non-base binary/uuids in query
Closes #4289.
Closes https://github.com/elixir-ecto/ecto_sql/issues/520.

Patch:
diff --git a/lib/ecto/changeset.ex b/lib/ecto/changeset.ex
index fbb731ca..cd2febc0 100644
--- a/lib/ecto/changeset.ex
+++ b/lib/ecto/changeset.ex
@@ -729,7 +729,7 @@ defmodule Ecto.Changeset do
           opts
 
         empty_values != empty_values() ->
-          # TODO: Remove changeset.empty_values field
+          # TODO: Remove changeset.empty_values field in Ecto v3.14
           IO.warn(
             "Changing the empty_values field of Ecto.Changeset is deprecated, " <>
               "please pass the :empty_values option on cast instead"
@@ -3152,7 +3152,7 @@ defmodule Ecto.Changeset do
          _spec_function,
          %Decimal{} = target_value
        ) do
-    result = Decimal.compare(value, target_value) |> normalize_compare()
+    result = Decimal.compare(value, target_value)
 
     case decimal_compare(result, spec_key) do
       true -> nil
@@ -3175,17 +3175,6 @@ defmodule Ecto.Changeset do
     end
   end
 
-  # TODO: Remove me once we support Decimal 2.0 only
-  # Support mismatch between API for Decimal.compare/2 for versions 1.6 and 2.0
-  defp normalize_compare(result) do
-    case result do
-      %Decimal{coef: 1, sign: -1} -> :lt
-      %Decimal{coef: 0} -> :eq
-      %Decimal{coef: 1, sign: 1} -> :gt
-      _ -> result
-    end
-  end
-
   defp decimal_new(term) when is_float(term), do: Decimal.from_float(term)
   defp decimal_new(term), do: Decimal.new(term)
 
diff --git a/lib/ecto/query/builder.ex b/lib/ecto/query/builder.ex
index 6e1770b2..7f27578a 100644
--- a/lib/ecto/query/builder.ex
+++ b/lib/ecto/query/builder.ex
@@ -1128,13 +1128,7 @@ defmodule Ecto.Query.Builder do
   @doc """
   Negates the given number.
   """
-  # TODO: Remove check when we depend on decimal v2.0
-  if Code.ensure_loaded?(Decimal) and function_exported?(Decimal, :negate, 1) do
-    def negate!(%Decimal{} = decimal), do: Decimal.negate(decimal)
-  else
-    def negate!(%Decimal{} = decimal), do: Decimal.minus(decimal)
-  end
-
+  def negate!(%Decimal{} = decimal), do: Decimal.negate(decimal)
   def negate!(number) when is_number(number), do: -number
 
   @doc """
diff --git a/lib/ecto/query/planner.ex b/lib/ecto/query/planner.ex
index 451bb306..550d941e 100644
--- a/lib/ecto/query/planner.ex
+++ b/lib/ecto/query/planner.ex
@@ -1309,7 +1309,28 @@ defmodule Ecto.Query.Planner do
     if Ecto.Type.base?(type) do
       {tagged, acc}
     else
-      {dump_param(kind, query, expr, v, type, adapter), acc}
+      type = field_type!(kind, query, expr, type)
+
+      with {:ok, type} <- normalize_param(kind, type, v),
+           {:ok, value} <- dump_param(adapter, type, v) do
+        # We cannot encode binary/uuid in queries because they would emit
+        # invalid queries with binary parts in them. In theory, we could
+        # wrap them in Ecto.Query.Tagged, but a tagged UUID would most
+        # likely wrap its string representation, not its binary one.
+        # So it is best to be consistent and not support query-dumping of
+        # non-base types.
+        if is_binary(value) and Ecto.Type.type(type) in [:binary_id, :binary, :uuid] do
+          error = "cannot encode value `#{inspect v}` of type `#{inspect(type)}` within a query, please interpolate (using ^) instead"
+          error! query, expr, error
+        else
+          {value, acc}
+        end
+      else
+        {:error, error} ->
+          error = error <> ". Or the value is incompatible or it must be " <>
+                           "interpolated (using ^) so it may be cast accordingly"
+          error! query, expr, error
+      end
     end
   end
 
@@ -1344,24 +1365,6 @@ defmodule Ecto.Query.Planner do
     end
   end
 
-  defp dump_param(kind, query, expr, v, type, adapter) do
-    type = field_type!(kind, query, expr, type)
-
-    case dump_param(kind, type, v, adapter) do
-      {:ok, v} ->
-        v
-      {:error, error} ->
-        error = error <> ". Or the value is incompatible or it must be " <>
-                         "interpolated (using ^) so it may be cast accordingly"
-        error! query, expr, error
-    end
-  end
-
-  defp dump_param(kind, type, v, adapter) do
-    with {:ok, type} <- normalize_param(kind, type, v),
-         do: dump_param(adapter, type, v)
-  end
-
   defp validate_in(meta, expr, param, acc, adapter) do
     {v, t} = Enum.fetch!(expr.params, param)
     length = length(v)
diff --git a/lib/ecto/schema.ex b/lib/ecto/schema.ex
index d846c3e4..b5d5308d 100644
--- a/lib/ecto/schema.ex
+++ b/lib/ecto/schema.ex
@@ -2244,9 +2244,7 @@ defmodule Ecto.Schema do
   def __after_compile__(%{module: module} = env, _) do
     # If we are compiling code, we can validate associations now,
     # as the Elixir compiler will solve dependencies.
-    #
-    # TODO: Use Code.can_await_module_compilation?/0 from Elixir v1.11+.
-    if Process.info(self(), :error_handler) == {:error_handler, Kernel.ErrorHandler} do
+    if Code.can_await_module_compilation?() do
       for name <- module.__schema__(:associations) do
         assoc = module.__schema__(:association, name)
 
diff --git a/lib/ecto/type.ex b/lib/ecto/type.ex
index c44d2522..5f9bc091 100644
--- a/lib/ecto/type.ex
+++ b/lib/ecto/type.ex
@@ -858,8 +858,6 @@ defmodule Ecto.Type do
 
   defp cast_decimal(term) when is_binary(term) do
     case Decimal.parse(term) do
-      {:ok, decimal} -> check_decimal(decimal, false)
-      # The following two clauses exist to support earlier versions of Decimal.
       {decimal, ""} -> check_decimal(decimal, false)
       {_, remainder} when is_binary(remainder) and byte_size(remainder) > 0 -> :error
       :error -> :error
diff --git a/lib/mix/ecto.ex b/lib/mix/ecto.ex
index 4226a1e6..7f5a37a3 100644
--- a/lib/mix/ecto.ex
+++ b/lib/mix/ecto.ex
@@ -24,9 +24,7 @@ defmodule Mix.Ecto do
   defp parse_repo([], []) do
     apps =
       if apps_paths = Mix.Project.apps_paths() do
-        # TODO: Use the proper ordering from Mix.Project.deps_apps
-        # when we depend on Elixir v1.11+.
-        apps_paths |> Map.keys() |> Enum.sort()
+        Enum.filter(Mix.Project.deps_apps(), &is_map_key(apps_paths, &1))
       else
         [Mix.Project.config()[:app]]
       end
@@ -64,14 +62,7 @@ defmodule Mix.Ecto do
   def ensure_repo(repo, args) do
     # Do not pass the --force switch used by some tasks downstream
     args = List.delete(args, "--force")
-
-    # TODO: Use only app.config when we depend on Elixir v1.11+.
-    if Code.ensure_loaded?(Mix.Tasks.App.Config) do
-      Mix.Task.run("app.config", args)
-    else
-      Mix.Task.run("loadpaths", args)
-      "--no-compile" not in args && Mix.Task.run("compile", args)
-    end
+    Mix.Task.run("app.config", args)
 
     case Code.ensure_compiled(repo) do
       {:module, _} ->
diff --git a/mix.exs b/mix.exs
index b246b3f4..7849dca4 100644
--- a/mix.exs
+++ b/mix.exs
@@ -33,7 +33,7 @@ defmodule Ecto.MixProject do
   defp deps do
     [
       {:telemetry, "~> 0.4 or ~> 1.0"},
-      {:decimal, "~> 1.6 or ~> 2.0"},
+      {:decimal, "~> 2.0"},
       {:jason, "~> 1.0", optional: true},
       {:ex_doc, "~> 0.20", only: :docs}
     ]


Test patch:
diff --git a/test/ecto/query/planner_test.exs b/test/ecto/query/planner_test.exs
index b1c16df1..8af5fdf5 100644
--- a/test/ecto/query/planner_test.exs
+++ b/test/ecto/query/planner_test.exs
@@ -902,6 +902,14 @@ defmodule Ecto.Query.PlannerTest do
     end
   end
 
+  test "normalize: raises on literal non-base binary/uuids in query" do
+    query = from c in Comment, where: c.uuid == "017f65d1-80bd-152d-f997-afa6dd33a00f"
+
+    assert_raise Ecto.QueryError,
+                 ~r"cannot encode value `\"[\w-]{36}\"` of type `:binary_id`",
+                 fn -> normalize(query) end
+  end
+
   test "normalize: casts atom values" do
     {_query, cast_params, dump_params, _key} = normalize_with_params(Post |> where([p], p.status == :draft))
     assert cast_params == []