Repository: elixir-ecto/ecto
Pull request #4458 (2024-07-24 13:54:20)

Expand order by before recursion
I was trying to fix the regression introduced by [this PR](https://github.com/elixir-ecto/ecto/pull/4449) where something like this doesn't work anymore for order by

```elixir
defmacro my_custom_order(p) do
  quote do
    [unquote(p).id, some_sql_function(unquote(p))]
  end
end
```

It's caused because we are calling `escape` again when the macro is expanded so each element inside of it gets escaped twice.

I am probably missing something obvious, but the approach I took in this PR is to escape the order by expression before we go down into the recursion, similar to what we do with `from`. 


Patch:
diff --git a/lib/ecto/query/builder.ex b/lib/ecto/query/builder.ex
index d0d65c05..318a4a88 100644
--- a/lib/ecto/query/builder.ex
+++ b/lib/ecto/query/builder.ex
@@ -292,11 +292,6 @@ defmodule Ecto.Query.Builder do
   end
 
   # lists
-  def escape([{dir, _} | _] = expr, _type, params_acc, _vars, _env)
-      when dir in ~w(asc asc_nulls_last asc_nulls_first desc desc_nulls_last desc_nulls_first)a do
-    {expr, params_acc}
-  end
-
   def escape(list, type, params_acc, vars, env) when is_list(list) do
     if Enum.all?(list, &(is_binary(&1) or is_number(&1) or is_boolean(&1))) do
       {literal(list, type, vars), params_acc}
diff --git a/lib/ecto/query/builder/order_by.ex b/lib/ecto/query/builder/order_by.ex
index 8c70185e..86df7f0a 100644
--- a/lib/ecto/query/builder/order_by.ex
+++ b/lib/ecto/query/builder/order_by.ex
@@ -51,39 +51,37 @@ defmodule Ecto.Query.Builder.OrderBy do
           {Macro.t(), {list, term}}
   def escape(kind, expr, params_acc, vars, env) do
     expr
+    |> Macro.expand_once(env)
     |> List.wrap()
-    |> Enum.flat_map_reduce(params_acc, &do_escape(&1, &2, kind, vars, env))
+    |> Enum.map_reduce(params_acc, &do_escape(&1, &2, kind, vars, env))
   end
 
   defp do_escape({dir, {:^, _, [expr]}}, params_acc, kind, _vars, _env) do
-    {[{quoted_dir!(kind, dir),
-      quote(do: Ecto.Query.Builder.OrderBy.field!(unquote(kind), unquote(expr)))}], params_acc}
+    {{quoted_dir!(kind, dir),
+      quote(do: Ecto.Query.Builder.OrderBy.field!(unquote(kind), unquote(expr)))}, params_acc}
   end
 
   defp do_escape({:^, _, [expr]}, params_acc, kind, _vars, _env) do
-    {[{:asc, quote(do: Ecto.Query.Builder.OrderBy.field!(unquote(kind), unquote(expr)))}],
+    {{:asc, quote(do: Ecto.Query.Builder.OrderBy.field!(unquote(kind), unquote(expr)))},
      params_acc}
   end
 
   defp do_escape({dir, field}, params_acc, kind, _vars, _env) when is_atom(field) do
-    {[{quoted_dir!(kind, dir), Macro.escape(to_field(field))}], params_acc}
+    {{quoted_dir!(kind, dir), Macro.escape(to_field(field))}, params_acc}
   end
 
   defp do_escape(field, params_acc, _kind, _vars, _env) when is_atom(field) do
-    {[{:asc, Macro.escape(to_field(field))}], params_acc}
+    {{:asc, Macro.escape(to_field(field))}, params_acc}
   end
 
   defp do_escape({dir, expr}, params_acc, kind, vars, env) do
     {ast, params_acc} = Builder.escape(expr, :any, params_acc, vars, env)
-    {[{quoted_dir!(kind, dir), ast}], params_acc}
+    {{quoted_dir!(kind, dir), ast}, params_acc}
   end
 
-  defp do_escape(expr, params_acc, kind, vars, env) do
+  defp do_escape(expr, params_acc, _kind, vars, env) do
     {ast, params_acc} = Builder.escape(expr, :any, params_acc, vars, env)
-
-    if is_list(ast),
-      do: escape(kind, ast, params_acc, vars, env),
-      else: {[{:asc, ast}], params_acc}
+    {{:asc, ast}, params_acc}
   end
 
   @doc """


Test patch:
diff --git a/test/ecto/query/builder/order_by_test.exs b/test/ecto/query/builder/order_by_test.exs
index 0b5ca90b..2e13924e 100644
--- a/test/ecto/query/builder/order_by_test.exs
+++ b/test/ecto/query/builder/order_by_test.exs
@@ -11,6 +11,12 @@ defmodule Ecto.Query.Builder.OrderByTest do
       quote(do: fragment("lower(?)", unquote(p).title))
     end
 
+    defmacro my_custom_order(p) do
+      quote do
+        [unquote(p).id, my_custom_field(unquote(p)), nth_value(unquote(p).links, 1)]
+      end
+    end
+
     defmacro my_complex_order(p) do
       quote(do: [desc: unquote(p).id, asc: my_custom_field(unquote(p)), asc: nth_value(unquote(p).links, 1)])
     end
@@ -46,6 +52,9 @@ defmodule Ecto.Query.Builder.OrderByTest do
 
       assert {Macro.escape(quote do [desc: &0.id(), asc: fragment({:raw, "lower("}, {:expr, &0.title()}, {:raw, ")"}), asc: nth_value(&0.links(), 1)] end), {[], %{}}} ==
              escape(:order_by, quote do my_complex_order(x) end, {[], %{}}, [x: 0], __ENV__)
+
+      assert {Macro.escape(quote do [asc: &0.id(), asc: fragment({:raw, "lower("}, {:expr, &0.title()}, {:raw, ")"}), asc: nth_value(&0.links(), 1)] end), {[], %{}}} ==
+             escape(:order_by, quote do my_custom_order(x) end, {[], %{}}, [x: 0], __ENV__)
     end
 
     test "raises on unbound variables" do