From 45b77593532209274c7bd05c5e483ce72b2ac9e4 Mon Sep 17 00:00:00 2001 From: Lukasz Samson Date: Sun, 5 Jul 2026 00:08:51 +0200 Subject: [PATCH] Fix subquery order in query builders Fixes #4744 --- lib/ecto/query/builder/distinct.ex | 2 +- lib/ecto/query/builder/group_by.ex | 10 +++- lib/ecto/query/builder/order_by.ex | 10 +++- lib/ecto/query/builder/windows.ex | 12 ++--- test/ecto/query/builder/distinct_test.exs | 33 +++++++++++++ test/ecto/query/builder/group_by_test.exs | 26 ++++++++++ test/ecto/query/builder/order_by_test.exs | 33 +++++++++++++ test/ecto/query/builder/windows_test.exs | 60 +++++++++++++++++++++++ 8 files changed, 175 insertions(+), 11 deletions(-) diff --git a/lib/ecto/query/builder/distinct.ex b/lib/ecto/query/builder/distinct.ex index a919428190..99e3235af9 100644 --- a/lib/ecto/query/builder/distinct.ex +++ b/lib/ecto/query/builder/distinct.ex @@ -69,7 +69,7 @@ defmodule Ecto.Query.Builder.Distinct do distinct = quote do: %Ecto.Query.ByExpr{ expr: unquote(expr), params: unquote(params), - subqueries: unquote(acc.subqueries), + subqueries: unquote(Enum.reverse(acc.subqueries)), file: unquote(env.file), line: unquote(env.line)} Builder.apply_query(query, __MODULE__, [distinct], env) diff --git a/lib/ecto/query/builder/group_by.ex b/lib/ecto/query/builder/group_by.ex index 0965d111c6..59345ac00a 100644 --- a/lib/ecto/query/builder/group_by.ex +++ b/lib/ecto/query/builder/group_by.ex @@ -49,8 +49,14 @@ defmodule Ecto.Query.Builder.GroupBy do Shared between group_by and partition_by. """ def group_or_partition_by!(kind, query, exprs, params) do + {expr, params, subqueries} = group_or_partition_by!(kind, query, exprs, params, []) + {expr, params, Enum.reverse(subqueries)} + end + + @doc false + def group_or_partition_by!(kind, query, exprs, params, subqueries) do {expr, {params, _, subqueries}} = - Enum.map_reduce(List.wrap(exprs), {params, length(params), []}, fn + Enum.map_reduce(List.wrap(exprs), {params, length(params), subqueries}, fn field, params_count when is_atom(field) -> {to_field(field), params_count} @@ -99,7 +105,7 @@ defmodule Ecto.Query.Builder.GroupBy do group_by = quote do: %Ecto.Query.ByExpr{ expr: unquote(expr), params: unquote(params), - subqueries: unquote(acc.subqueries), + subqueries: unquote(Enum.reverse(acc.subqueries)), file: unquote(env.file), line: unquote(env.line)} Builder.apply_query(query, __MODULE__, [group_by], env) diff --git a/lib/ecto/query/builder/order_by.ex b/lib/ecto/query/builder/order_by.ex index 25f04e0a65..ff02f4436a 100644 --- a/lib/ecto/query/builder/order_by.ex +++ b/lib/ecto/query/builder/order_by.ex @@ -159,8 +159,14 @@ defmodule Ecto.Query.Builder.OrderBy do Shared between order_by and distinct. """ def order_by_or_distinct!(kind, query, exprs, params) do + {expr, params, subqueries} = order_by_or_distinct!(kind, query, exprs, params, []) + {expr, params, Enum.reverse(subqueries)} + end + + @doc false + def order_by_or_distinct!(kind, query, exprs, params, subqueries) do {expr, {params, _, subqueries}} = - Enum.map_reduce(List.wrap(exprs), {params, length(params), []}, fn + Enum.map_reduce(List.wrap(exprs), {params, length(params), subqueries}, fn {dir, expr}, params_count when dir in @directions -> {expr, params} = dynamic_or_field!(kind, expr, query, params_count) {{dir, expr}, params} @@ -248,7 +254,7 @@ defmodule Ecto.Query.Builder.OrderBy do quote do: %Ecto.Query.ByExpr{ expr: unquote(expr), params: unquote(params), - subqueries: unquote(acc.subqueries), + subqueries: unquote(Enum.reverse(acc.subqueries)), file: unquote(env.file), line: unquote(env.line) } diff --git a/lib/ecto/query/builder/windows.ex b/lib/ecto/query/builder/windows.ex index 9bd341aa2c..93abd64945 100644 --- a/lib/ecto/query/builder/windows.ex +++ b/lib/ecto/query/builder/windows.ex @@ -148,7 +148,7 @@ defmodule Ecto.Query.Builder.Windows do %Ecto.Query.ByExpr{ expr: unquote(compile_acc), params: unquote(params), - subqueries: unquote(acc.subqueries), + subqueries: unquote(Enum.reverse(acc.subqueries)), file: unquote(env.file), line: unquote(env.line) } @@ -166,7 +166,7 @@ defmodule Ecto.Query.Builder.Windows do windows = Enum.map(runtime, fn {name, compile_acc, runtime_acc, params, escape_acc} -> {{acc, subqueries}, params} = do_runtime_window!(runtime_acc, query, {compile_acc, escape_acc.subqueries}, params) - expr = %Ecto.Query.ByExpr{expr: Enum.reverse(acc), params: Enum.reverse(params), file: file, line: line, subqueries: subqueries} + expr = %Ecto.Query.ByExpr{expr: Enum.reverse(acc), params: Enum.reverse(params), file: file, line: line, subqueries: Enum.reverse(subqueries)} {name, expr} end) @@ -174,15 +174,15 @@ defmodule Ecto.Query.Builder.Windows do end defp do_runtime_window!([{:order_by, order_by} | kw], query, {acc, subqueries_acc}, params) do - {order_by, params, subqueries} = OrderBy.order_by_or_distinct!(:order_by, query, order_by, params) + {order_by, params, subqueries} = OrderBy.order_by_or_distinct!(:order_by, query, order_by, params, subqueries_acc) - do_runtime_window!(kw, query, {[{:order_by, order_by} | acc], subqueries_acc ++ subqueries}, params) + do_runtime_window!(kw, query, {[{:order_by, order_by} | acc], subqueries}, params) end defp do_runtime_window!([{:partition_by, partition_by} | kw], query, {acc, subqueries_acc}, params) do - {partition_by, params, subqueries} = GroupBy.group_or_partition_by!(:partition_by, query, partition_by, params) + {partition_by, params, subqueries} = GroupBy.group_or_partition_by!(:partition_by, query, partition_by, params, subqueries_acc) - do_runtime_window!(kw, query, {[{:partition_by, partition_by} | acc], subqueries_acc ++ subqueries}, params) + do_runtime_window!(kw, query, {[{:partition_by, partition_by} | acc], subqueries}, params) end defp do_runtime_window!([{:frame, frame} | kw], query, {acc, subqueries_acc}, params) do diff --git a/test/ecto/query/builder/distinct_test.exs b/test/ecto/query/builder/distinct_test.exs index 42765a2617..17d7b0822d 100644 --- a/test/ecto/query/builder/distinct_test.exs +++ b/test/ecto/query/builder/distinct_test.exs @@ -6,6 +6,10 @@ defmodule Ecto.Query.Builder.DistinctTest do import Ecto.Query + defp subquery_sources(%{subqueries: subqueries}) do + Enum.map(subqueries, fn subquery -> elem(subquery.query.from.source, 0) end) + end + describe "escape" do test "handles expressions and params" do assert {true, {[], %{}}} == @@ -79,6 +83,20 @@ defmodule Ecto.Query.Builder.DistinctTest do assert distinct.expr == [asc: {:exists, [], [subquery: 0]}] assert [_] = distinct.subqueries + distinct = [ + asc: dynamic([p], exists(from f in "foos", where: f.id == parent_as(:p).id)), + desc: dynamic([p], exists(from b in "bars", where: b.id == parent_as(:p).id)) + ] + + %{distinct: distinct} = from p in "posts", as: :p, distinct: ^distinct + + assert distinct.expr == [ + asc: {:exists, [], [subquery: 0]}, + desc: {:exists, [], [subquery: 1]} + ] + + assert subquery_sources(distinct) == ["foos", "bars"] + %{distinct: distinct} = from p in "posts", as: :p, @@ -88,6 +106,21 @@ defmodule Ecto.Query.Builder.DistinctTest do assert distinct.expr == [asc: {:exists, [], [subquery: 0]}] assert [_] = distinct.subqueries + + %{distinct: distinct} = + from p in "posts", + as: :p, + distinct: [ + asc: exists(from f in "foos", where: f.id == parent_as(:p).id), + desc: exists(from b in "bars", where: b.id == parent_as(:p).id) + ] + + assert distinct.expr == [ + asc: {:exists, [], [subquery: 0]}, + desc: {:exists, [], [subquery: 1]} + ] + + assert subquery_sources(distinct) == ["foos", "bars"] end test "raises on non-atoms" do diff --git a/test/ecto/query/builder/group_by_test.exs b/test/ecto/query/builder/group_by_test.exs index 7f52840fb9..a69fb55ee9 100644 --- a/test/ecto/query/builder/group_by_test.exs +++ b/test/ecto/query/builder/group_by_test.exs @@ -6,6 +6,10 @@ defmodule Ecto.Query.Builder.GroupByTest do import Ecto.Query + defp subquery_sources(%{subqueries: subqueries}) do + Enum.map(subqueries, fn subquery -> elem(subquery.query.from.source, 0) end) + end + describe "escape" do test "handles expressions and params" do assert {Macro.escape(quote(do: [&0.y()])), {[], %{}}} == @@ -62,6 +66,15 @@ defmodule Ecto.Query.Builder.GroupByTest do assert group_by.expr == [{:exists, [], [{:subquery, 0}]}] assert [_] = group_by.subqueries + key = [ + dynamic([q], exists(from f in "foos", where: f.title == parent_as(:q).title)), + dynamic([q], exists(from b in "bars", where: b.title == parent_as(:q).title)) + ] + + assert [group_by] = from(q in "q", as: :q, group_by: ^key).group_bys + assert group_by.expr == [{:exists, [], [{:subquery, 0}]}, {:exists, [], [{:subquery, 1}]}] + assert subquery_sources(group_by) == ["foos", "bars"] + assert [group_by] = group_by( "q", @@ -71,6 +84,19 @@ defmodule Ecto.Query.Builder.GroupByTest do assert group_by.expr == [{:exists, [], [{:subquery, 0}]}] assert [_] = group_by.subqueries + + assert [group_by] = + from( + q in "q", + as: :q, + group_by: [ + exists(from f in "foos", where: f.title == parent_as(:q).title), + exists(from b in "bars", where: b.title == parent_as(:q).title) + ] + ).group_bys + + assert group_by.expr == [{:exists, [], [{:subquery, 0}]}, {:exists, [], [{:subquery, 1}]}] + assert subquery_sources(group_by) == ["foos", "bars"] end test "raises when no a field or a list of fields" do diff --git a/test/ecto/query/builder/order_by_test.exs b/test/ecto/query/builder/order_by_test.exs index 65ca6e80bc..a43f51006d 100644 --- a/test/ecto/query/builder/order_by_test.exs +++ b/test/ecto/query/builder/order_by_test.exs @@ -6,6 +6,10 @@ defmodule Ecto.Query.Builder.OrderByTest do import Ecto.Query + defp subquery_sources(%{subqueries: subqueries}) do + Enum.map(subqueries, fn subquery -> elem(subquery.query.from.source, 0) end) + end + describe "escape" do defmacro my_custom_field(p) do quote(do: fragment("lower(?)", unquote(p).title)) @@ -233,6 +237,20 @@ defmodule Ecto.Query.Builder.OrderByTest do assert order_by.expr == [asc: {:exists, [], [subquery: 0]}] assert [_] = order_by.subqueries + order_by = [ + asc: dynamic([p], exists(from f in "foos", where: f.id == parent_as(:p).id)), + desc: dynamic([p], exists(from b in "bars", where: b.id == parent_as(:p).id)) + ] + + %{order_bys: [order_by]} = from p in "posts", as: :p, order_by: ^order_by + + assert order_by.expr == [ + asc: {:exists, [], [subquery: 0]}, + desc: {:exists, [], [subquery: 1]} + ] + + assert subquery_sources(order_by) == ["foos", "bars"] + %{order_bys: [order_by]} = from p in "posts", as: :p, @@ -242,6 +260,21 @@ defmodule Ecto.Query.Builder.OrderByTest do assert order_by.expr == [asc: {:exists, [], [subquery: 0]}] assert [_] = order_by.subqueries + + %{order_bys: [order_by]} = + from p in "posts", + as: :p, + order_by: [ + asc: exists(from f in "foos", where: f.id == parent_as(:p).id), + desc: exists(from b in "bars", where: b.id == parent_as(:p).id) + ] + + assert order_by.expr == [ + asc: {:exists, [], [subquery: 0]}, + desc: {:exists, [], [subquery: 1]} + ] + + assert subquery_sources(order_by) == ["foos", "bars"] end test "supports interpolated atomnames in selected_as/1" do diff --git a/test/ecto/query/builder/windows_test.exs b/test/ecto/query/builder/windows_test.exs index 162c335a55..c195823adb 100644 --- a/test/ecto/query/builder/windows_test.exs +++ b/test/ecto/query/builder/windows_test.exs @@ -6,6 +6,10 @@ defmodule Ecto.Query.Builder.WindowsTest do import Ecto.Query + defp subquery_sources(%{subqueries: subqueries}) do + Enum.map(subqueries, fn subquery -> elem(subquery.query.from.source, 0) end) + end + defmacrop rows_between_preceding(count) do quote do fragment(unquote("ROWS BETWEEN #{count} PRECEDING AND CURRENT ROW")) @@ -132,6 +136,20 @@ defmodule Ecto.Query.Builder.WindowsTest do assert query.windows[:w].expr[:partition_by] == [{:exists, [], [subquery: 0]}] assert [_] = query.windows[:w].subqueries + partition_by = [ + dynamic([p], exists(from f in "foos", where: f.title == parent_as(:q).title)), + dynamic([p], exists(from b in "bars", where: b.title == parent_as(:q).title)) + ] + + query = "q" |> windows([p], w: [partition_by: ^partition_by]) + + assert query.windows[:w].expr[:partition_by] == [ + {:exists, [], [subquery: 0]}, + {:exists, [], [subquery: 1]} + ] + + assert subquery_sources(query.windows[:w]) == ["foos", "bars"] + query = "q" |> windows([p], @@ -142,6 +160,48 @@ defmodule Ecto.Query.Builder.WindowsTest do assert query.windows[:w].expr[:partition_by] == [{:exists, [], [subquery: 0]}] assert [_] = query.windows[:w].subqueries + + query = + "q" + |> windows([p], + w: [ + partition_by: [ + exists(from f in "foos", where: f.title == parent_as(:q).title), + exists(from b in "bars", where: b.title == parent_as(:q).title) + ] + ] + ) + + assert query.windows[:w].expr[:partition_by] == [ + {:exists, [], [subquery: 0]}, + {:exists, [], [subquery: 1]} + ] + + assert subquery_sources(query.windows[:w]) == ["foos", "bars"] + + order_by = [ + asc: dynamic([p], exists(from baz in "bazs", where: baz.title == parent_as(:q).title)) + ] + + query = + "q" + |> windows([p], + w: [ + partition_by: [ + exists(from f in "foos", where: f.title == parent_as(:q).title), + exists(from b in "bars", where: b.title == parent_as(:q).title) + ], + order_by: ^order_by + ] + ) + + assert query.windows[:w].expr[:partition_by] == [ + {:exists, [], [subquery: 0]}, + {:exists, [], [subquery: 1]} + ] + + assert query.windows[:w].expr[:order_by] == [asc: {:exists, [], [subquery: 2]}] + assert subquery_sources(query.windows[:w]) == ["foos", "bars", "bazs"] end test "raises on invalid partition by" do