Allow :preload_order association option to be specified as MFA and …

…add `:mode` option to `Ecto.Query.order_by` (#4219)

integration_test/cases/preload.exs

@@ -575,6 +575,16 @@ defmodule Ecto.Integration.PreloadTest do
     assert [%{name: "foo"}, %{name: "bar"}] = post.ordered_users
   end
 
+  test "custom preload_order with mfa" do
+    post1 = TestRepo.insert!(%Post{users: [%User{name: "bar"}, %User{name: "foo"}], title: "1"})
+    post2 = TestRepo.insert!(%Post{users: [%User{name: "baz"}, %User{name: "foz"}], title: "2"})
+
+    [post1, post2] = TestRepo.preload([post1, post2], [:ordered_users_by_join_table], log: :error)
+
+    assert [%{name: "foo"}, %{name: "bar"}] = post1.ordered_users_by_join_table
+    assert [%{name: "foz"}, %{name: "baz"}] = post2.ordered_users_by_join_table
+  end
+
   ## Others
 
   @tag :invalid_prefix

integration_test/support/schemas.exs

@@ -28,6 +28,7 @@ defmodule Ecto.Integration.Post do
   """
   use Ecto.Integration.Schema
   import Ecto.Changeset
+  import Ecto.Query, only: [dynamic: 2]
 
   schema "posts" do
     field :counter, :id # Same as integer
@@ -57,6 +58,8 @@ defmodule Ecto.Integration.Post do
     many_to_many :users, Ecto.Integration.User,
       join_through: "posts_users", on_delete: :delete_all, on_replace: :delete
     many_to_many :ordered_users, Ecto.Integration.User, join_through: "posts_users", preload_order: [desc: :name]
+    many_to_many :ordered_users_by_join_table, Ecto.Integration.User,
+      join_through: "posts_users", preload_order: {__MODULE__, :preload_order, []}
     many_to_many :unique_users, Ecto.Integration.User,
       join_through: "posts_users", unique: true
     many_to_many :constraint_users, Ecto.Integration.User,
@@ -67,6 +70,10 @@ defmodule Ecto.Integration.Post do
     timestamps()
   end
 
+  def preload_order() do
+    [desc: dynamic([assoc, join], join.user_id)]
+  end
+
   def changeset(schema, params) do
     cast(schema, params, ~w(counter title blob temp public cost visits
                            intensity bid uuid meta posted)a)

lib/ecto/association.ex

@@ -503,6 +503,10 @@ defmodule Ecto.Association do
   @doc """
   Validates `preload_order` for association named `name`.
   """
+  def validate_preload_order!(_name, {mod, fun, args} = preload_order)
+      when is_atom(mod) and is_atom(fun) and is_list(args),
+      do: preload_order
+
   def validate_preload_order!(name, preload_order) when is_list(preload_order) do
     Enum.map(preload_order, fn
       field when is_atom(field) ->
@@ -528,8 +532,8 @@ defmodule Ecto.Association do
 
   def validate_preload_order!(name, preload_order) do
     raise ArgumentError,
-          "expected `:preload_order` for #{inspect(name)} to be a keyword list or a list of atoms/fields, " <>
-            "got: `#{inspect(preload_order)}`"
+          "expected `:preload_order` for #{inspect(name)} to be a keyword list, a list of atoms/fields " <>
+            "or a {Mod, fun, args} tuple, got: `#{inspect(preload_order)}`"
   end
 
   @doc """
@@ -1318,6 +1322,7 @@ defmodule Ecto.Association.ManyToMany do
   @behaviour Ecto.Association
   @on_delete_opts [:nothing, :delete_all]
   @on_replace_opts [:raise, :mark_as_invalid, :delete]
+
   defstruct [
     :field,
     :owner,

lib/ecto/query.ex

@@ -736,7 +736,7 @@ defmodule Ecto.Query do
 
       windows: [ordered_names: [order_by: e.name]]
 
-  It works exactly as the keyword query version of `order_by/3`.
+  It works exactly as the keyword query version of `order_by/4`.
 
   ### :frame
 
@@ -1775,12 +1775,20 @@ defmodule Ecto.Query do
   "nulls first" or "nulls last" is specific to each database implementation.
 
   `order_by` may be invoked or listed in a query many times. New expressions
-  are always appended to the previous ones.
+  can be appended or preprended to the existing ones. The behaviour is controlled
+  through the `:mode` option. By default, new expressions are appended.
 
   `order_by` also accepts a list of atoms where each atom refers to a field in
   source or a keyword list where the direction is given as key and the field
   to order as value.
 
+  ## Options
+
+    * `:mode` - where to place the order expression relative to the
+      ones that already exist. Can be `:append`, to place it after the
+      current orderings, or `:prepend`, to place it before the current
+      orderings. Defaults to `:append`
+
   ## Keywords examples
 
       from(c in City, order_by: c.name, order_by: c.population)
@@ -1829,8 +1837,8 @@ defmodule Ecto.Query do
       City |> order_by(^order_by_param) # Keyword list
 
   """
-  defmacro order_by(query, binding \\ [], expr) do
-    Builder.OrderBy.build(query, binding, expr, __CALLER__)
+  defmacro order_by(query, binding \\ [], expr, opts \\ []) do
+    Builder.OrderBy.build(query, binding, expr, opts, __CALLER__)
   end
 
   @doc """

lib/ecto/query/api.ex

@@ -755,7 +755,7 @@ defmodule Ecto.Query.API do
   referencing it outside of select statements.
 
   This comes in handy when, for instance, you would like to use the calculated
-  value in `Ecto.Query.group_by/3` or `Ecto.Query.order_by/3`:
+  value in `Ecto.Query.group_by/3` or `Ecto.Query.order_by/4`:
 
       from p in Post,
         select: %{

lib/ecto/query/builder/order_by.ex

@@ -1,4 +1,4 @@
-import Kernel, except: [apply: 2]
+import Kernel, except: [apply: 3]
 
 defmodule Ecto.Query.Builder.OrderBy do
   @moduledoc false
@@ -14,6 +14,9 @@ defmodule Ecto.Query.Builder.OrderBy do
     :desc_nulls_first
   ]
 
+  @modes [:append, :prepend]
+  @default_mode :append
+
   @doc """
   Returns `true` if term is a valid order_by direction; otherwise returns `false`.
 
@@ -128,6 +131,23 @@ defmodule Ecto.Query.Builder.OrderBy do
 
   defp to_field(field), do: {{:., [], [{:&, [], [0]}, field]}, [], []}
 
+  @doc """
+  Called at runtime to verify the ordering mode
+  """
+  def mode!(order_opts) do
+    case order_opts[:mode] do
+      nil ->
+        @default_mode
+
+      mode when mode in @modes ->
+        mode
+
+      other ->
+        raise ArgumentError,
+              "expected `:mode` to be `:append` or `:prepend`, got: #{inspect(other)}"
+    end
+  end
+
   @doc """
   Shared between order_by and distinct.
   """
@@ -148,10 +168,11 @@ defmodule Ecto.Query.Builder.OrderBy do
   @doc """
   Called at runtime to assemble order_by.
   """
-  def order_by!(query, exprs, file, line) do
+  def order_by!(query, exprs, order_opts, file, line) do
+    mode = Ecto.Query.Builder.OrderBy.mode!(order_opts)
     {expr, params} = order_by_or_distinct!(:order_by, query, exprs, [])
     expr = %Ecto.Query.QueryExpr{expr: expr, params: Enum.reverse(params), line: line, file: file}
-    apply(query, expr)
+    apply(query, expr, mode)
   end
 
   defp dynamic_or_field!(kind, %Ecto.Query.DynamicExpr{} = dynamic, query, {params, count}) do
@@ -176,34 +197,47 @@ defmodule Ecto.Query.Builder.OrderBy do
   If possible, it does all calculations at compile time to avoid
   runtime work.
   """
-  @spec build(Macro.t, [Macro.t], Macro.t, Macro.Env.t) :: Macro.t
-  def build(query, _binding, {:^, _, [var]}, env) do
+  @spec build(Macro.t, [Macro.t], Macro.t, Macro.t, Macro.Env.t) :: Macro.t
+  def build(query, _binding, {:^, _, [var]}, order_opts, env) do
     quote do
-      Ecto.Query.Builder.OrderBy.order_by!(unquote(query), unquote(var), unquote(env.file), unquote(env.line))
+      Ecto.Query.Builder.OrderBy.order_by!(unquote(query), unquote(var), unquote(order_opts), unquote(env.file), unquote(env.line))
     end
   end
 
-  def build(query, binding, expr, env) do
+  def build(query, binding, expr, order_opts, env) do
     {query, binding} = Builder.escape_binding(query, binding, env)
     {expr, {params, _acc}} = escape(:order_by, expr, {[], %{}}, binding, env)
     params = Builder.escape_params(params)
+    mode = quote do: Ecto.Query.Builder.OrderBy.mode!(unquote(order_opts))
 
     order_by = quote do: %Ecto.Query.QueryExpr{
                            expr: unquote(expr),
                            params: unquote(params),
                            file: unquote(env.file),
                            line: unquote(env.line)}
-    Builder.apply_query(query, __MODULE__, [order_by], env)
+    Builder.apply_query(query, __MODULE__, [order_by, mode], env)
   end
 
   @doc """
   The callback applied by `build/4` to build the query.
   """
-  @spec apply(Ecto.Queryable.t, term) :: Ecto.Query.t
-  def apply(%Ecto.Query{order_bys: order_bys} = query, expr) do
-    %{query | order_bys: order_bys ++ [expr]}
+  @spec apply(Ecto.Queryable.t, term, term) :: Ecto.Query.t
+  def apply(%Ecto.Query{order_bys: orders} = query, expr, mode) do
+    %{query | order_bys: update_order_bys(orders, expr, mode)}
   end
-  def apply(query, expr) do
-    apply(Ecto.Queryable.to_query(query), expr)
+  def apply(query, expr, mode) do
+    apply(Ecto.Queryable.to_query(query), expr, mode)
+  end
+
+  @doc """
+  Updates the `order_bys` value for a query.
+  """
+  def update_order_bys(orders, expr, :append), do: orders ++ [expr]
+  def update_order_bys(orders, expr, :prepend), do: [expr | orders]
+
+  def update_order_bys(orders, expr, mode) do
+    quote do
+      Ecto.Query.Builder.OrderBy.update_order_bys(unquote(orders), unquote(expr), unquote(mode))
+    end
   end
 end

lib/ecto/repo/preloader.ex

@@ -6,6 +6,8 @@ defmodule Ecto.Repo.Preloader do
   require Ecto.Query
   require Logger
 
+  alias Ecto.Query.DynamicExpr
+
   @doc """
   Transforms a result set based on query preloads, loading
   the associations onto their parent schema.
@@ -242,13 +244,13 @@ defmodule Ecto.Repo.Preloader do
 
   defp fetch_query(ids, %{cardinality: card} = assoc, repo_name, query, prefix, related_key, take, tuplet) do
     query = assoc.__struct__.assoc_query(assoc, query, Enum.uniq(ids))
-    field = related_key_to_field(query, related_key)
+    related_field_ast = related_key_to_field(query, related_key)
 
     # Normalize query
     query = %{Ecto.Query.Planner.ensure_select(query, take || true) | prefix: prefix}
 
     # Add the related key to the query results
-    query = update_in query.select.expr, &{:{}, [], [field, &1]}
+    query = update_in query.select.expr, &{:{}, [], [related_field_ast, &1]}
 
     # If we are returning many results, we must sort by the key too
     query =
@@ -261,10 +263,13 @@ defmodule Ecto.Repo.Preloader do
                   "select the parent's foreign key"
 
         {:many, _} ->
+          query = add_preload_order(assoc.preload_order, query)
+
           update_in query.order_bys, fn order_bys ->
-            [%Ecto.Query.QueryExpr{expr: preload_order(assoc, query, field), params: [],
+            [%Ecto.Query.QueryExpr{expr: [asc: related_field_ast], params: [],
                                    file: __ENV__.file, line: __ENV__.line}|order_bys]
           end
+
         {:one, _} ->
           query
       end
@@ -315,17 +320,6 @@ defmodule Ecto.Repo.Preloader do
     Expected a tuple with ID and struct, got: #{inspect(entry)}
     """
 
-  defp preload_order(assoc, query, related_field) do
-    custom_order_by = Enum.map(assoc.preload_order, fn
-      {direction, field} ->
-        {direction, related_key_to_field(query, {0, field})}
-      field ->
-        {:asc, related_key_to_field(query, {0, field})}
-    end)
-
-    [{:asc, related_field} | custom_order_by]
-  end
-
   defp related_key_to_field(query, {pos, key, field_type}) do
     field_ast = related_key_to_field(query, {pos, key})
 
@@ -339,6 +333,46 @@ defmodule Ecto.Repo.Preloader do
   defp related_key_pos(_query, pos) when pos >= 0, do: pos
   defp related_key_pos(query, pos), do: Ecto.Query.Builder.count_binds(query) + pos
 
+  defp add_preload_order([], query), do: query
+
+  defp add_preload_order(order, query) when is_list(order) do
+    Ecto.Query.order_by(query, [q], ^order, mode: :prepend)
+  end
+
+  defp add_preload_order({m, f, a}, query) do
+    order =
+      case apply(m, f, a) do
+        order when is_list(order) ->
+          order
+
+        other ->
+          raise ArgumentError,
+                "`:preload_order` must resolve to a keyword list or a list of atoms/fields, " <>
+                  "got: `#{inspect(other)}`"
+      end
+
+    Enum.each(order, fn
+      {direction, field} when is_atom(field) or is_struct(field, DynamicExpr) ->
+        unless Ecto.Query.Builder.OrderBy.valid_direction?(direction) do
+          raise ArgumentError,
+                "`:preload_order` must specify valid directions, " <>
+                  "got: `#{inspect(order)}`, `#{inspect(direction)}` is not a valid direction"
+        end
+
+        :ok
+
+      field when is_atom(field) or is_struct(field, DynamicExpr) ->
+        :ok
+
+      other ->
+        raise ArgumentError,
+              "`:preload_order` must resolve to a keyword list or a list of atoms/fields, " <>
+                "got: `#{inspect(order)}`, `#{inspect(other)}` is not valid"
+    end)
+
+    add_preload_order(order, query)
+  end
+
   defp unzip_ids([{k, v}|t], acc1, acc2), do: unzip_ids(t, [k|acc1], [v|acc2])
   defp unzip_ids([], acc1, acc2), do: {acc1, acc2}