Repository: elixir-ecto/ecto
Pull request #2431 (2018-02-19 00:30:30)

[WIP] Add support for named joins
Refs #2389 

This PR implements support for named joins following the feedback from #2411 and building on top of what was implemented in #2428.

The commits are organized in a logical order with atomic changes, so that the work can be reviewed one-by-one (which may come in handy due to size of the PR).

1. https://github.com/elixir-ecto/ecto/commit/e3796abda9bc1f36d06837faa007f9e4fe44d06b alters `Ecto.Query.Builder.Join.build` to accept opts as the last argument. This change landed here because it didn't make it to #2428 
2. https://github.com/elixir-ecto/ecto/commit/443f3b81cded190d3c67bae85760cf3837c5e67c adds `:as` option support and assures that it's passed from both `Ecto.Query.join` and `Ecto.Query.from`. There's a small inconsistency in how the extra parameters are treated. For `Ecto.Query.from`, `as` has to be always present immediately after `join` DSL construct, while in case of `Ecto.Query.join`, there are no constraints with regards to the position of `as` relative to `on`. While at it, `Ecto.Query.join` does not support multiple `on` occurrences the way `Ecto.Query.from` does. Is it desired to strive for 100% consistency in behavior between those 2?
3. https://github.com/elixir-ecto/ecto/commit/bdff7e78a7c327d4d6e65ec07eec55beac426724 handles storing join name to position mapping on the basis of `as` alias passed when building. It follows the advice of storing those mappings explicitly in top-level query struct for quick access later. I'm not 100% sure whether the approach taken is correct one, however following the structure of the code, I've decided to build/update aliases map in the `apply` step, because that seemed the only place where query is modified in any way (a join expression is added)
4. https://github.com/elixir-ecto/ecto/commit/0c075f74b1fdad17b19d8f0c26c9afd6532cf85a handles named binds in binding lists. It's mostly iterates on the initial attempt made in #2428 with a couple refactorings along the way to make that area of the code a bit clearer (I hope).
5. https://github.com/elixir-ecto/ecto/commit/11c2f8ff5926f6a8f83793f5ad5d47d0e89ffb96 adds an extra integration test so that at least basic use case is tested end to end.

As always, feedback appreciated.

## TODO (there may be more, based on the feedback)

- [ ] add documentation
- [ ] add extra tests around passing opts to `Ecto.Query.from`, especially for failing cases

ebert[bot]: Ebert has finished reviewing this Pull Request and has found:

* 1 possible new issue (including those that may have been commented here).

You can see more details about this review at https://ebertapp.io/github/elixir-ecto/ecto/pulls/2431.

<!-- Ebert auto generated comment -->

josevalim: This is beautiful. I will go ahead and merge this and push some commits on top with small changes. Documentation and more tests can come as we improve this. :)

Patch:
diff --git a/lib/ecto/query.ex b/lib/ecto/query.ex
index df741518..85fd1ddf 100644
--- a/lib/ecto/query.ex
+++ b/lib/ecto/query.ex
@@ -305,7 +305,7 @@ defmodule Ecto.Query do
   specially on insert, use `Ecto.put_meta/2`.
   """
 
-  defstruct [prefix: nil, sources: nil, from: nil, joins: [], wheres: [], select: nil,
+  defstruct [prefix: nil, sources: nil, from: nil, joins: [], aliases: nil, wheres: [], select: nil,
              order_bys: [], limit: nil, offset: nil, group_bys: [], updates: [],
              havings: [], preloads: [], assocs: [], distinct: nil, lock: nil]
   @type t :: %__MODULE__{}
@@ -332,7 +332,7 @@ defmodule Ecto.Query do
 
   defmodule JoinExpr do
     @moduledoc false
-    defstruct [:qual, :source, :on, :file, :line, :assoc, :ix, params: []]
+    defstruct [:qual, :source, :on, :file, :line, :assoc, :alias, :ix, params: []]
   end
 
   defmodule Tagged do
@@ -624,11 +624,16 @@ defmodule Ecto.Query do
 
   defp from([{join, expr}|t], env, count_bind, quoted, binds) when join in @joins do
     qual = join_qual(join)
+    {t, as} = pop_join_as(t)
     {t, on} = collect_on(t, nil)
-    {quoted, binds, count_bind} = Join.build(quoted, qual, binds, expr, on, count_bind, env)
+    {quoted, binds, count_bind} = Join.build(quoted, qual, binds, expr, count_bind, env, as: as, on: on)
     from(t, env, count_bind, quoted, to_query_binds(binds))
   end
 
+  defp from([{:as, _value}|_], _env, _count_bind, _quoted, _binds) do
+    Builder.error! "`as` keyword must immediately follow a join"
+  end
+
   defp from([{:on, _value}|_], _env, _count_bind, _quoted, _binds) do
     Builder.error! "`on` keyword must immediately follow a join"
   end
@@ -654,6 +659,9 @@ defmodule Ecto.Query do
   defp join_qual(:left_lateral_join), do: :left_lateral
   defp join_qual(:inner_lateral_join), do: :inner_lateral
 
+  defp pop_join_as([{:as, expr}|t]), do: {t, expr}
+  defp pop_join_as(t), do: {t, nil}
+
   defp collect_on([{:on, expr}|t], nil),
     do: collect_on(t, expr)
   defp collect_on([{:on, expr}|t], acc),
@@ -769,11 +777,11 @@ defmodule Ecto.Query do
       end
 
     query
-    |> Join.build(qual, binding, expr, opts[:on], nil, __CALLER__)
+    |> Join.build(qual, binding, expr, nil, __CALLER__, opts)
     |> elem(0)
   end
 
-  @join_opts [:on]
+  @join_opts [:on, :as]
 
   defp parse_join_opts([]), do: {:opts, []}
   defp parse_join_opts(list) when is_list(list) do
diff --git a/lib/ecto/query/builder.ex b/lib/ecto/query/builder.ex
index 0850aa9f..cc528d40 100644
--- a/lib/ecto/query/builder.ex
+++ b/lib/ecto/query/builder.ex
@@ -519,13 +519,52 @@ defmodule Ecto.Query.Builder do
       ** (Ecto.Query.CompileError) variable `x` is bound twice
 
       iex> escape_binding(%Ecto.Query{}, quote do: [a, b, :foo])
-      ** (Ecto.Query.CompileError) binding list should contain only variables, got: :foo
+      ** (Ecto.Query.CompileError) binding list should contain only variables or `{:bind_name, var}` tuples, got: :foo
 
   """
   @spec escape_binding(Macro.t | Ecto.Query.t, list) :: {Macro.t | Ecto.Query.t, Keyword.t}
   def escape_binding(query, binding) when is_list(binding) do
     vars = binding |> Enum.with_index |> Enum.map(&escape_bind(&1))
-    assert_no_dup_binding!(vars)
+
+    with :ok <- assert_no_duplicate_bind(vars),
+         {positional_vars, named_vars} <- Enum.split_while(vars, & !named_bind?(&1)),
+         :ok <- assert_named_binds_in_tail(named_vars),
+         {query, positional_binds} <- calculate_positional_binds(query, positional_vars),
+         {query, named_binds} <- calculate_named_binds(query, named_vars) do
+      {query, positional_binds ++ named_binds}
+    else
+      {:error, {:duplicate_bind, var}} ->
+        error! "variable `#{var}` is bound twice"
+      {:error, :named_binds_not_in_tail} ->
+        error! "named binds in the form of `{:bind_name, var}` tuples must be at the end " <>
+          "of the binding list, got: #{Macro.to_string(binding)}"
+    end
+   end
+  def escape_binding(_query, bind) do
+    error! "binding should be list of variables and `{:bind_name, var}` tuples " <>
+      "at the end, got: #{Macro.to_string(bind)}"
+  end
+
+  defp assert_named_binds_in_tail(named_vars) do
+    if Enum.all?(named_vars, &named_bind?/1) do
+      :ok
+    else
+      {:error, :named_binds_not_in_tail}
+    end
+  end
+
+  defp named_bind?({_, {:named, _}}), do: true
+  defp named_bind?(_), do: false
+
+  defp assert_no_duplicate_bind(vars) do
+    bound_vars = vars |> Keyword.keys |> Enum.filter(& &1 != :_)
+    case bound_vars -- Enum.uniq(bound_vars) do
+      []  -> :ok
+      dup -> {:error, {:duplicate_bind, hd(dup)}}
+    end
+  end
+
+  defp calculate_positional_binds(query, vars) do
     case Enum.split_while(vars, & elem(&1, 0) != :...) do
       {vars, []} ->
         {query, vars}
@@ -543,24 +582,38 @@ defmodule Ecto.Query.Builder do
         {query, vars ++ tail}
     end
   end
-  def escape_binding(_query, bind) do
-    error! "binding should be list of variables, got: #{Macro.to_string(bind)}"
-  end
 
-  defp assert_no_dup_binding!(vars) do
-    bound_vars = vars |> Keyword.keys |> Enum.filter(& &1 != :_)
-    case bound_vars -- Enum.uniq(bound_vars) do
-      []  -> :ok
-      dup -> error! "variable `#{hd dup}` is bound twice"
-    end
+  defp calculate_named_binds(query, []), do: {query, []}
+  defp calculate_named_binds(query, vars) do
+    query =
+      quote do
+        query = Ecto.Queryable.to_query(unquote(query))
+        aliases = query.aliases || %{}
+        query
+      end
+
+    vars = Enum.map(vars, fn {k, {_, name}} ->
+      ix = quote do
+        case Map.fetch(aliases, unquote(name)) do
+          {:ok, ix} -> ix
+          :error -> Ecto.Query.Builder.error! "there is no bind named `#{inspect unquote(name)}`"
+        end
+      end
+      {k, ix}
+    end)
+
+    {query, vars}
   end
 
   defp escape_bind({{{var, _, context}, ix}, _}) when is_atom(var) and is_atom(context),
     do: {var, ix}
   defp escape_bind({{var, _, context}, ix}) when is_atom(var) and is_atom(context),
     do: {var, ix}
+  defp escape_bind({{name, {var, _, context}}, _ix}) when is_atom(name) and is_atom(var) and is_atom(context),
+    do: {var, {:named, name}}
   defp escape_bind({bind, _ix}),
-    do: error!("binding list should contain only variables, got: #{Macro.to_string(bind)}")
+    do: error!("binding list should contain only variables or " <>
+          "`{:bind_name, var}` tuples, got: #{Macro.to_string(bind)}")
 
   defp try_expansion(expr, type, params, vars, %Macro.Env{} = env) do
     try_expansion(expr, type, params, vars, {env, &escape/5})
diff --git a/lib/ecto/query/builder/from.ex b/lib/ecto/query/builder/from.ex
index af54d16d..ca93e907 100644
--- a/lib/ecto/query/builder/from.ex
+++ b/lib/ecto/query/builder/from.ex
@@ -30,7 +30,7 @@ defmodule Ecto.Query.Builder.From do
       {quote(do: other), []}
 
       iex> escape(quote do: x() in other)
-      ** (Ecto.Query.CompileError) binding list should contain only variables, got: x()
+      ** (Ecto.Query.CompileError) binding list should contain only variables or `{:bind_name, var}` tuples, got: x()
 
   """
   @spec escape(Macro.t) :: {Macro.t, Keyword.t}
diff --git a/lib/ecto/query/builder/join.ex b/lib/ecto/query/builder/join.ex
index 5a8e5e60..0d4b7e0d 100644
--- a/lib/ecto/query/builder/join.ex
+++ b/lib/ecto/query/builder/join.ex
@@ -117,9 +117,11 @@ defmodule Ecto.Query.Builder.Join do
   If possible, it does all calculations at compile time to avoid
   runtime work.
   """
-  @spec build(Macro.t, atom, [Macro.t], Macro.t, Macro.t, Macro.t, Macro.Env.t) ::
+  @spec build(Macro.t, atom, [Macro.t], Macro.t, Macro.t, Macro.Env.t, Keyword.t) ::
               {Macro.t, Keyword.t, non_neg_integer | nil}
-  def build(query, qual, binding, expr, on, count_bind, env) do
+  def build(query, qual, binding, expr, count_bind, env, opts) do
+    on = opts[:on]
+    alias_name = opts[:as]
     {query, binding} = Builder.escape_binding(query, binding)
     {join_bind, join_source, join_assoc, join_params} = escape(expr, binding, env)
     join_params = Builder.escape_params(join_params)
@@ -152,29 +154,30 @@ defmodule Ecto.Query.Builder.Join do
       end
 
     query = build_on(on || true, query, binding, count_bind, qual,
-                     join_source, join_assoc, join_params, env)
+                     join_source, join_assoc, join_params, alias_name, env)
     {query, binding, next_bind}
   end
 
   def build_on({:^, _, [var]}, query, _binding, count_bind,
-               join_qual, join_source, join_assoc, join_params, env) do
+               join_qual, join_source, join_assoc, join_params, alias_name, env) do
     quote do
       query = unquote(query)
       Ecto.Query.Builder.Join.join!(query, unquote(var), unquote(count_bind),
                                     unquote(join_qual), unquote(join_source), unquote(join_assoc),
-                                    unquote(join_params), unquote(env.file), unquote(env.line))
+                                    unquote(alias_name), unquote(join_params), unquote(env.file),
+                                    unquote(env.line))
     end
   end
 
   def build_on(on, query, binding, count_bind,
-               join_qual, join_source, join_assoc, join_params, env) do
+               join_qual, join_source, join_assoc, join_params, alias_name, env) do
     {on_expr, on_params} = Ecto.Query.Builder.Filter.escape(:on, on, count_bind, binding, env)
     on_params = Builder.escape_params(on_params)
 
     join =
       quote do
         %JoinExpr{qual: unquote(join_qual), source: unquote(join_source),
-                  assoc: unquote(join_assoc), file: unquote(env.file),
+                  assoc: unquote(join_assoc), alias: unquote(alias_name), file: unquote(env.file),
                   line: unquote(env.line), params: unquote(join_params),
                   on: %QueryExpr{expr: unquote(on_expr), params: unquote(on_params),
                                  line: unquote(env.line), file: unquote(env.file)}}
@@ -186,21 +189,40 @@ defmodule Ecto.Query.Builder.Join do
   @doc """
   Applies the join expression to the query.
   """
-  def apply(%Ecto.Query{joins: joins} = query, expr) do
-    %{query | joins: joins ++ [expr]}
+  def apply(%Ecto.Query{joins: joins, aliases: aliases} = query, expr) do
+    join_count = Builder.count_binds(query)
+    aliases = update_aliases(aliases, expr, join_count)
+
+    %{query | joins: joins ++ [expr], aliases: aliases}
   end
   def apply(query, expr) do
     apply(Ecto.Queryable.to_query(query), expr)
   end
 
+  def update_aliases(aliases, %JoinExpr{alias: nil}, _), do: aliases
+  def update_aliases(aliases, %JoinExpr{alias: name}, join_count) do
+    aliases = aliases || %{}
+
+    if Map.has_key?(aliases, name) do
+      Builder.error! "alias `#{inspect name}` already exists"
+    else
+      Map.put(aliases, name, join_count)
+    end
+  end
+  def update_aliases(aliases, expr, join_count) do
+    quote do
+      Ecto.Query.Builder.Join.update_aliases(unquote(aliases), unquote(expr), unquote(join_count))
+    end
+  end
+
   @doc """
   Called at runtime to build a join.
   """
-  def join!(query, expr, count_bind, join_qual, join_source, join_assoc, join_params, file, line) do
+  def join!(query, expr, count_bind, join_qual, join_source, join_assoc, alias_name, join_params, file, line) do
     {on_expr, on_params, on_file, on_line} =
       Ecto.Query.Builder.Filter.filter!(:on, query, expr, count_bind, file, line)
 
-    join = %JoinExpr{qual: join_qual, source: join_source, assoc: join_assoc,
+    join = %JoinExpr{qual: join_qual, source: join_source, assoc: join_assoc, alias: alias_name,
                      file: file, line: line, params: join_params,
                      on: %QueryExpr{expr: on_expr, params: on_params,
                                     line: on_line, file: on_file}}
diff --git a/lib/ecto/query/planner.ex b/lib/ecto/query/planner.ex
index 014c0308..c8a36919 100644
--- a/lib/ecto/query/planner.ex
+++ b/lib/ecto/query/planner.ex
@@ -4,7 +4,7 @@ defmodule Ecto.Query.Planner do
 
   alias Ecto.Query.{BooleanExpr, DynamicExpr, JoinExpr, QueryExpr, SelectExpr}
 
-  if map_size(%Ecto.Query{}) != 17 do
+  if map_size(%Ecto.Query{}) != 18 do
     raise "Ecto.Query match out of date in builder"
   end
 


Test patch:
diff --git a/integration_test/cases/joins.exs b/integration_test/cases/joins.exs
index d265cfc5..54911225 100644
--- a/integration_test/cases/joins.exs
+++ b/integration_test/cases/joins.exs
@@ -77,6 +77,18 @@ defmodule Ecto.Integration.JoinsTest do
     assert [{^p1, ^c1}, {^p2, ^c1}] = TestRepo.all(query)
   end
 
+  test "named joins" do
+    _p = TestRepo.insert!(%Post{title: "1"})
+    p2 = TestRepo.insert!(%Post{title: "2"})
+    c1 = TestRepo.insert!(%Permalink{url: "1", post_id: p2.id})
+
+    query =
+      from(p in Post, join: c in assoc(p, :permalink), as: :permalink, order_by: p.id)
+      |> select([p, permalink: c], {p, c})
+
+    assert [{^p2, ^c1}] = TestRepo.all(query)
+  end
+
   @tag :left_join
   test "left joins with missing entries" do
     p1 = TestRepo.insert!(%Post{title: "1"})
diff --git a/test/ecto/query_test.exs b/test/ecto/query_test.exs
index 58dd7fca..6f930ba7 100644
--- a/test/ecto/query_test.exs
+++ b/test/ecto/query_test.exs
@@ -120,7 +120,7 @@ defmodule Ecto.QueryTest do
 
     test "must be a list of variables" do
       assert_raise Ecto.Query.CompileError,
-                   "binding list should contain only variables, got: 0", fn ->
+                   "binding list should contain only variables or `{:bind_name, var}` tuples, got: 0", fn ->
         quote_and_eval select(%Query{}, [0], 1)
       end
     end
@@ -216,6 +216,69 @@ defmodule Ecto.QueryTest do
     end
   end
 
+  describe "named bindings" do
+    test "assigns a name to a join" do
+      query =
+        from(p in "posts",
+          join: b in "blogs",
+          join: c in "comments", as: :comment,
+          join: l in "links", as: :link)
+
+      assert %{comment: 2, link: 3} == query.aliases
+      assert [_, %{alias: :comment}, %{alias: :link}] = query.joins
+    end
+
+    test "crashes on assigning the same name twice" do
+      message = ~r"alias `:foo` already exists"
+      assert_raise Ecto.Query.CompileError, message, fn ->
+        from(p in "posts",
+          join: b in "blogs", as: :foo,
+          join: c in "comments", as: :foo)
+      end
+    end
+
+    test "match on binding by name" do
+      query =
+        "posts"
+        |> join(:inner, [p], c in "comments", as: :comment)
+        |> where([comment: c], c.id == 0)
+
+      assert inspect(query) ==
+        ~s[#Ecto.Query<from p in \"posts\", join: c in \"comments\", on: true, where: c.id == 0>]
+    end
+
+    test "match on binding by name with ... in the middle" do
+      query =
+        "posts"
+        |> join(:inner, [p], c in "comments")
+        |> join(:inner, [], a in "authors", as: :authors)
+        |> where([p, ..., authors: a], a.id == 0)
+
+      assert inspect(query) ==
+        ~s[#Ecto.Query<from p in \"posts\", join: c in \"comments\", on: true, join: a in \"authors\", on: true, where: a.id == 0>]
+    end
+
+    test "referring to non-existing binding" do
+      message = ~r"there is no bind named `:nope`"
+      assert_raise Ecto.Query.CompileError, message, fn ->
+        "posts"
+        |> join(:inner, [p], c in "comments", as: :comment)
+        |> where([nope: c], c.id == 0)
+      end
+    end
+
+    test "named bind not in tail of the list" do
+      message = ~r"tuples must be at the end of the binding list"
+      assert_raise Ecto.Query.CompileError, message, fn ->
+      quote_and_eval(
+        "posts"
+        |> join(:inner, [p], c in "comments", as: :comment)
+        |> where([{:comment, c}, p], c.id == 0)
+      )
+      end
+    end
+  end
+
   describe "keyword queries" do
     test "are supported through from/2" do
       # queries need to be on the same line or == wont work