Warn on missing :on in joins (#4074)

lib/ecto/query.ex

@@ -1050,7 +1050,7 @@ defmodule Ecto.Query do
 
   Each join accepts the following options:
 
-    * `:on` - a query expression or keyword list to filter the join
+    * `:on` - a query expression or keyword list to filter the join, defaults to `true`
     * `:as` - a named binding for the join
     * `:prefix` - the prefix to be used for the join when issuing a database query
     * `:hints` - a string or a list of strings to be used as database hints

lib/ecto/query/builder/join.ex

@@ -3,6 +3,7 @@ import Kernel, except: [apply: 2]
 defmodule Ecto.Query.Builder.Join do
   @moduledoc false
 
+  require Logger
   alias Ecto.Query.Builder
   alias Ecto.Query.{JoinExpr, QueryExpr}
 
@@ -184,7 +185,8 @@ defmodule Ecto.Query.Builder.Join do
       hints: hints
     ]
 
-    query = build_on(on || true, join, as, query, binding, count_bind, env)
+    on = ensure_on(on, join_assoc, join_qual, join_source, env)
+    query = build_on(on, join, as, query, binding, count_bind, env)
     {query, binding, next_bind}
   end
 
@@ -229,6 +231,25 @@ defmodule Ecto.Query.Builder.Join do
     end
   end
 
+  defp ensure_on(on, _assoc, _qual, _source, _env) when on != nil, do: on
+
+  defp ensure_on(nil, _assoc = nil, qual, source, env) when qual not in [:cross, :cross_lateral] do
+    maybe_source =
+      with {source, alias} <- source,
+        source when source != nil <- source || alias do
+        " on #{inspect(source)}"
+      else
+        _ -> ""
+      end
+
+    stacktrace = Macro.Env.stacktrace(env)
+    IO.warn("Missing `:on` in join#{maybe_source}, defaulting to `on: true`.", stacktrace)
+
+    true
+  end
+
+  defp ensure_on(nil, _assoc, _qual, _source, _env), do: true
+
   @doc """
   Applies the join expression to the query.
   """

lib/ecto/query/builder/preload.ex

@@ -190,13 +190,13 @@ defmodule Ecto.Query.Builder.Preload do
       ** (ArgumentError) `"external"` is not a valid preload expression, expected an atom or a list.
 
       iex> require Ecto.Query
-      iex> expand([b: Ecto.Query.dynamic([_a, b], b)], Ecto.Query.from(a in "a", join: b in "b"))
+      iex> expand([b: Ecto.Query.dynamic([_a, b], b)], Ecto.Query.from(a in "a", join: b in "b", on: true))
       {[], [b: {1, []}]}
 
       iex> require Ecto.Query
       iex> expand(
       ...>   [b: {Ecto.Query.dynamic([_a, b], b), c: Ecto.Query.dynamic([_a, _b, c], c)}],
-      ...>   Ecto.Query.from(a in "a", join: b in "b", join: c in "c")
+      ...>   Ecto.Query.from(a in "a", join: b in "b", on: true, join: c in "c", on: true)
       ...> )
       {[], [b: {1, [c: {2, []}]}]}
   """

test/ecto/query/inspect_test.exs

@@ -117,7 +117,7 @@ defmodule Ecto.Query.InspectTest do
   end
 
   test "join" do
-    assert i(from(x in Post, join: y in Comment)) ==
+    assert i(from(x in Post, join: y in Comment, on: true)) ==
            ~s{from p0 in Inspect.Post, join: c1 in Inspect.Comment, on: true}
 
     assert i(from(x in Post, join: y in Comment, on: x.id == y.id)) ==
@@ -132,7 +132,7 @@ defmodule Ecto.Query.InspectTest do
     binding = :comments
     assert i(from(x in Post, left_join: y in assoc(x, ^binding), as: ^binding)) ==
            ~s{from p0 in Inspect.Post, left_join: c1 in assoc(p0, :comments), as: :comments}
-    
+
     assert i(from(x in Post, left_join: y in assoc(x, :comments))) ==
            ~s{from p0 in Inspect.Post, left_join: c1 in assoc(p0, :comments)}
 
@@ -470,7 +470,7 @@ defmodule Ecto.Query.InspectTest do
     def equal?(_, _, _), do: false
     def embed_as(_, _), do: :self
   end
-   
+
   test "parameterized types" do
     query = from(x in Post, select: type(^"foo", ^Ecto.ParameterizedType.init(MyParameterizedType, param: :foo)))
     assert i(query) == ~s<from p0 in Inspect.Post, select: type(^"foo", {:parameterized, Ecto.Query.InspectTest.MyParameterizedType, :foo})>

test/ecto/query/planner_test.exs

@@ -290,13 +290,13 @@ defmodule Ecto.Query.PlannerTest do
   end
 
   test "plan: joins" do
-    query = from(p in Post, join: c in "comments") |> plan |> elem(0)
+    query = from(p in Post, join: c in "comments", on: true) |> plan |> elem(0)
     assert hd(query.joins).source == {"comments", nil}
 
-    query = from(p in Post, join: c in Comment) |> plan |> elem(0)
+    query = from(p in Post, join: c in Comment, on: true) |> plan |> elem(0)
     assert hd(query.joins).source == {"comments", Comment}
 
-    query = from(p in Post, join: c in {"post_comments", Comment}) |> plan |> elem(0)
+    query = from(p in Post, join: c in {"post_comments", Comment}, on: true) |> plan |> elem(0)
     assert hd(query.joins).source == {"post_comments", Comment}
   end
 
@@ -472,6 +472,7 @@ defmodule Ecto.Query.PlannerTest do
         where: is_nil(nil),
         or_where: is_nil(nil),
         join: c in Comment,
+        on: true,
         hints: ["join hint"],
         prefix: "world",
         preload: :comments
@@ -531,23 +532,23 @@ defmodule Ecto.Query.PlannerTest do
     assert query.sources == {{"posts", Post, "local"}}
 
     # Schema prefix in join
-    {query, _, _, _} = from(c in Comment, join: Post) |> plan()
+    {query, _, _, _} = from(c in Comment, join: Post, on: true) |> plan()
     assert query.sources == {{"comments", Comment, nil}, {"posts", Post, "my_prefix"}}
 
-    {query, _, _, _} = from(c in Comment, join: Post) |> Map.put(:prefix, "global") |> plan()
+    {query, _, _, _} = from(c in Comment, join: Post, on: true) |> Map.put(:prefix, "global") |> plan()
     assert query.sources == {{"comments", Comment, "global"}, {"posts", Post, "my_prefix"}}
 
-    {query, _, _, _} = from(c in Comment, join: Post, prefix: "local") |> Map.put(:prefix, "global") |> plan()
+    {query, _, _, _} = from(c in Comment, join: Post, on: true, prefix: "local") |> Map.put(:prefix, "global") |> plan()
     assert query.sources == {{"comments", Comment, "global"}, {"posts", Post, "local"}}
 
     # Schema prefix in query join
-    {query, _, _, _} = from(p in Post, join: ^from(c in Comment)) |> plan()
+    {query, _, _, _} = from(p in Post, join: ^from(c in Comment), on: true) |> plan()
     assert query.sources == {{"posts", Post, "my_prefix"}, {"comments", Comment, nil}}
 
-    {query, _, _, _} = from(p in Post, join: ^from(c in Comment)) |> Map.put(:prefix, "global") |> plan()
+    {query, _, _, _} = from(p in Post, join: ^from(c in Comment), on: true) |> Map.put(:prefix, "global") |> plan()
     assert query.sources == {{"posts", Post, "my_prefix"}, {"comments", Comment, "global"}}
 
-    {query, _, _, _} = from(p in Post, join: ^from(c in Comment), prefix: "local") |> Map.put(:prefix, "global") |> plan()
+    {query, _, _, _} = from(p in Post, join: ^from(c in Comment), on: true, prefix: "local") |> Map.put(:prefix, "global") |> plan()
     assert query.sources == {{"posts", Post, "my_prefix"}, {"comments", Comment, "local"}}
 
     # No schema prefix in assoc join
@@ -945,20 +946,20 @@ defmodule Ecto.Query.PlannerTest do
 
   test "normalize: late parent bindings with as" do
     child = from(c in Comment, where: parent_as(:posts).posted == c.posted)
-    query = from(Post, as: :posts, join: c in subquery(child)) |> normalize()
+    query = from(Post, as: :posts, join: c in subquery(child), on: true) |> normalize()
     assert Macro.to_string(hd(hd(query.joins).source.query.wheres).expr) == "parent_as(:posts).posted() == &0.posted()"
 
     child = from(c in Comment, select: %{map: parent_as(:posts).posted})
-    query = from(Post, as: :posts, join: c in subquery(child)) |> normalize()
+    query = from(Post, as: :posts, join: c in subquery(child), on: true) |> normalize()
     assert Macro.to_string(hd(query.joins).source.query.select.expr) == "%{map: parent_as(:posts).posted()}"
 
     assert_raise Ecto.SubQueryError, ~r/the parent_as in a subquery select used as a join can only access the `from` binding in query/, fn ->
       child = from(c in Comment, select: %{map: parent_as(:itself).posted})
-      from(Post, as: :posts, join: c in subquery(child), as: :itself) |> normalize()
+      from(Post, as: :posts, join: c in subquery(child), on: true, as: :itself) |> normalize()
     end
 
     assert_raise Ecto.SubQueryError, ~r/could not find named binding `parent_as\(:posts\)`/, fn ->
-      from(Post, join: c in subquery(child)) |> normalize()
+      from(Post, join: c in subquery(child), on: true) |> normalize()
     end
 
     assert_raise Ecto.QueryError, ~r/could not find named binding `parent_as\(:posts\)`/, fn ->
@@ -970,11 +971,11 @@ defmodule Ecto.Query.PlannerTest do
     as = :posts
 
     child = from(c in Comment, where: parent_as(^as).posted == c.posted)
-    query = from(Post, as: :posts, join: c in subquery(child)) |> normalize()
+    query = from(Post, as: :posts, join: c in subquery(child), on: true) |> normalize()
     assert Macro.to_string(hd(hd(query.joins).source.query.wheres).expr) == "parent_as(:posts).posted() == &0.posted()"
 
     child = from(c in Comment, select: %{map: field(parent_as(^as), :posted)})
-    query = from(Post, as: :posts, join: c in subquery(child)) |> normalize()
+    query = from(Post, as: :posts, join: c in subquery(child), on: true) |> normalize()
     assert Macro.to_string(hd(query.joins).source.query.select.expr) == "%{map: parent_as(:posts).posted()}"
   end
 
@@ -983,7 +984,7 @@ defmodule Ecto.Query.PlannerTest do
     child2 = from(c in Comment, where: c.id in subquery(child3), select: c.id)
     child = from(c in Comment, where: parent_as(:posts).posted == c.posted and c.id in subquery(child2))
 
-    query = from(Post, as: :posts, join: c in subquery(child)) |> normalize()
+    query = from(Post, as: :posts, join: c in subquery(child), on: true) |> normalize()
     assert Macro.to_string(hd(hd(query.joins).source.query.wheres).expr) =~ "parent_as(:posts).posted() == &0.posted()"
     assert Macro.to_string(hd(hd(query.joins).source.query.wheres).expr) =~ "in %Ecto.SubQuery{"
   end
@@ -994,7 +995,7 @@ defmodule Ecto.Query.PlannerTest do
     child2 = from(c in Comment, where: c.id in subquery(child3), select: c.id)
     child = from(c in Comment, where: field(parent_as(^as), :posted) == c.posted and c.id in subquery(child2))
 
-    query = from(Post, as: :posts, join: c in subquery(child)) |> normalize()
+    query = from(Post, as: :posts, join: c in subquery(child), on: true) |> normalize()
     assert Macro.to_string(hd(hd(query.joins).source.query.wheres).expr) =~ "parent_as(:posts).posted() == &0.posted()"
     assert Macro.to_string(hd(hd(query.joins).source.query.wheres).expr) =~ "in %Ecto.SubQuery{"
   end
@@ -1117,7 +1118,7 @@ defmodule Ecto.Query.PlannerTest do
 
   test "normalize: parent_as/1 in type/2" do
     child = from c in Comment, where: type(parent_as(:posts).id, :string) == c.text
-    query = from(Post, as: :posts, inner_lateral_join: c in subquery(child)) |> normalize()
+    query = from(Post, as: :posts, inner_lateral_join: c in subquery(child), on: true) |> normalize()
 
     assert inspect(query) =~ "where: type(parent_as(:posts).id, :string) == c0.text"
   end
@@ -1286,7 +1287,7 @@ defmodule Ecto.Query.PlannerTest do
       agg_values =
         "values"
         |> with_cte("sensors_cte", as: ^sensors)
-        |> join(:inner, [v], s in "sensors_cte")
+        |> join(:inner, [v], s in "sensors_cte", on: true)
         |> select([v, s], %{bucket: ^123 + v.number})
 
       query =
@@ -1330,7 +1331,7 @@ defmodule Ecto.Query.PlannerTest do
 
     query =
       from(Post, [])
-      |> join(:inner, [_], c in Comment)
+      |> join(:inner, [_], c in Comment, on: true)
       |> preload([_, c], comments: c)
       |> select([p, _], {p.title, p})
       |> normalize()
@@ -1393,7 +1394,7 @@ defmodule Ecto.Query.PlannerTest do
 
     query =
       Post
-      |> join(:inner, [_], c in Comment)
+      |> join(:inner, [_], c in Comment, on: true)
       |> select([p, c], {p, struct(c, [:id, :text])})
       |> normalize()
     assert query.select.fields ==
@@ -1404,7 +1405,7 @@ defmodule Ecto.Query.PlannerTest do
   test "normalize: select with struct/2 on assoc" do
     query =
       Post
-      |> join(:inner, [_], c in Comment)
+      |> join(:inner, [_], c in Comment, on: true)
       |> select([p, c], struct(p, [:id, :title, comments: [:id, :text]]))
       |> preload([p, c], comments: c)
       |> normalize()
@@ -1415,7 +1416,7 @@ defmodule Ecto.Query.PlannerTest do
 
     query =
       Post
-      |> join(:inner, [_], c in Comment)
+      |> join(:inner, [_], c in Comment, on: true)
       |> select([p, c], struct(p, [:id, :title, comments: [:id, :text, post: :id], extra_comments: :id]))
       |> preload([p, c], comments: {c, post: p}, extra_comments: c)
       |> normalize()
@@ -1430,7 +1431,7 @@ defmodule Ecto.Query.PlannerTest do
   test "normalize: select with struct/2 on fragment" do
     assert_raise Ecto.QueryError, ~r"it is not possible to return a struct subset of a fragment", fn ->
       Post
-      |> join(:inner, [_], c in fragment("comments"))
+      |> join(:inner, [_], c in fragment("comments"), on: true)
       |> select([_, c], struct(c, [:id]))
       |> normalize()
     end
@@ -1448,7 +1449,7 @@ defmodule Ecto.Query.PlannerTest do
 
     query =
       Post
-      |> join(:inner, [_], c in Comment)
+      |> join(:inner, [_], c in Comment, on: true)
       |> select([p, c], {p, map(c, [:id, :text])})
       |> normalize()
     assert query.select.fields ==
@@ -1459,7 +1460,7 @@ defmodule Ecto.Query.PlannerTest do
   test "normalize: select with map/2 on assoc" do
     query =
       Post
-      |> join(:inner, [_], c in Comment)
+      |> join(:inner, [_], c in Comment, on: true)
       |> select([p, c], map(p, [:id, :title, comments: [:id, :text]]))
       |> preload([p, c], comments: c)
       |> normalize()
@@ -1470,7 +1471,7 @@ defmodule Ecto.Query.PlannerTest do
 
     query =
       Post
-      |> join(:inner, [_], c in Comment)
+      |> join(:inner, [_], c in Comment, on: true)
       |> select([p, c], map(p, [:id, :title, comments: [:id, :text, post: :id], extra_comments: :id]))
       |> preload([p, c], comments: {c, post: p}, extra_comments: c)
       |> normalize()
@@ -1485,7 +1486,7 @@ defmodule Ecto.Query.PlannerTest do
   test "normalize: select with map/2 on fragment" do
     query =
       Post
-      |> join(:inner, [_], f in fragment("select 1 as a, 2 as b"))
+      |> join(:inner, [_], f in fragment("select 1 as a, 2 as b"), on: true)
       |> select([_, f], map(f, [:a, :b]))
       |> normalize()
 
@@ -1509,7 +1510,7 @@ defmodule Ecto.Query.PlannerTest do
 
     query =
       Post
-      |> join(:inner, [_], c in Comment)
+      |> join(:inner, [_], c in Comment, on: true)
       |> select([p, c], {p, %{c | text: "bar"}})
       |> normalize()
     assert query.select.fields ==
@@ -1619,7 +1620,7 @@ defmodule Ecto.Query.PlannerTest do
   end
 
   test "normalize: fragments do not support preloads" do
-    query = from p in Post, join: c in fragment("..."), preload: [comments: c]
+    query = from p in Post, join: c in fragment("..."), on: true, preload: [comments: c]
     assert_raise Ecto.QueryError, ~r/can only preload sources with a schema/, fn ->
       normalize(query)
     end

test/ecto/query/subquery_test.exs

@@ -421,10 +421,10 @@ defmodule Ecto.Query.SubqueryTest do
       query = normalize(from(p in subquery(subquery), select: p.title))
       assert query.select.fields == [{{:., [type: :string], [{:&, [], [0]}, :title]}, [], []}]
 
-      query = normalize(from(c in Comment, join: p in subquery(subquery), select: p))
+      query = normalize(from(c in Comment, join: p in subquery(subquery), on: true, select: p))
       assert query.select.fields == select_fields([:id, :title], 1)
 
-      query = normalize(from(c in Comment, join: p in subquery(subquery), select: p.title))
+      query = normalize(from(c in Comment, join: p in subquery(subquery), on: true, select: p.title))
       assert query.select.fields == [{{:., [type: :string], [{:&, [], [1]}, :title]}, [], []}]
 
       subquery = from p in Post, select: %{id: p.id, title: p.title}