Skip to content

Add missing :on keys to queries with joins #113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 11, 2023
Merged

Add missing :on keys to queries with joins #113

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions test/ecto/adapters/sqlite3/connection_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ defmodule Ecto.Adapters.SQLite3.ConnectionTest do
from(c in "categories",
as: :parent_category,
left_lateral_join: b in subquery(breadcrumbs_query),
on: true,
select: %{id: c.id, breadcrumbs: b.breadcrumbs}
)
|> plan()
Expand Down Expand Up @@ -1409,7 +1410,10 @@ defmodule Ecto.Adapters.SQLite3.ConnectionTest do
test "join with hints" do
assert_raise Ecto.QueryError, ~r/join hints are not supported by SQLite3/, fn ->
Schema
|> join(:inner, [p], q in Schema2, hints: ["USE INDEX FOO", "USE INDEX BAR"])
|> join(:inner, [p], q in Schema2,
hints: ["USE INDEX FOO", "USE INDEX BAR"],
on: true
)
|> select([], true)
|> plan()
|> all()
Expand Down Expand Up @@ -1475,7 +1479,7 @@ defmodule Ecto.Adapters.SQLite3.ConnectionTest do
query =
"comments"
|> from(as: :comment)
|> join(:inner, [c], p in subquery(posts))
|> join(:inner, [c], p in subquery(posts), on: true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty sure this should just be on: p.id == c.post_id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I changed it, the test failed:

$ mix test
..........................................................................................................................................

  1) test join with subquery (Ecto.Adapters.SQLite3.ConnectionTest)
     test/ecto/adapters/sqlite3/connection_test.exs:1445
     Assertion with == failed
     code:  assert all(query) ==
              ~s"SELECT s1.\"title\" FROM \"comments\" AS c0 " <>
                ~s"INNER JOIN (SELECT sp0.\"title\" AS \"title\" FROM \"posts\" AS sp0 WHERE (sp0.\"title\" = c0.\"subtitle\")) AS s1 ON 1"
     left:  "SELECT s1.\"title\" FROM \"comments\" AS c0 INNER JOIN (SELECT sp0.\"title\" AS \"title\" FROM \"posts\" AS sp0 WHERE (sp0.\"title\" = c0.\"subtitle\")) AS s1 ON s1.\"id\" = c0.\"post_id\""
     right: "SELECT s1.\"title\" FROM \"comments\" AS c0 INNER JOIN (SELECT sp0.\"title\" AS \"title\" FROM \"posts\" AS sp0 WHERE (sp0.\"title\" = c0.\"subtitle\")) AS s1 ON 1"
     stacktrace:
       test/ecto/adapters/sqlite3/connection_test.exs:1486: (test)

...........................................................................................
Finished in 0.7 seconds (0.6s async, 0.09s sync)
230 tests, 1 failure

Randomized with seed 382835

I avoided changing things in test cases to keep it focused on eliminating the warnings. If we should visit these tests, we better do it in another PR (we'll probably touch some other things too).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we should fix the tests here too. I don't see why we can't fix them here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test file has 2.4K lines of code, with multiple modules defined inline and lots of functions defined directly in the test module. I'm not familiar enough with the Ecto.Adapters.SQL.Connection behaviour just yet, so I can't step up to do any refactoring for now, that's why I kept the PR focused on cleaning up the warnings only.

I've been diving into the Ecto documentation to be able to actively contribute to ecto_sqlite3 since last week. I expect to be able to contribute at a higher level soon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joeljuca I'm thinking we should just fix the tests now and those warnings are good indicators of the places we should alter them. When I get some time, I'll either copy your changes or just push into this branch with the SQL updates. It's a good thing to clean up as we go.

Originally I just copied a lot of tests from other adapters and threw together frankenstien tests from all of them to give this library a fighting chance 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, got it. So we leave it as it is for now? What should we do with this PR? Leave it hanging till we're able to refactor these tests, or we just close it and move on?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave this open. I can attack this and use the changes you made to hunt down and improve the tests.

|> select([_, p], p)
|> plan()

Expand Down Expand Up @@ -1519,7 +1523,8 @@ defmodule Ecto.Adapters.SQLite3.ConnectionTest do
"SELECT * FROM schema2 AS s2 WHERE s2.id = ? AND s2.field = ?",
p.x,
^10
)
),
on: true
)
|> select([p], {p.id, ^0})
|> where([p], p.id > 0 and p.id < ^100)
Expand All @@ -1545,7 +1550,10 @@ defmodule Ecto.Adapters.SQLite3.ConnectionTest do

test "join with query interpolation" do
inner = Ecto.Queryable.to_query(Schema2)
query = from(p in Schema, left_join: c in ^inner, select: {p.id, c.id}) |> plan()

query =
from(p in Schema, left_join: c in ^inner, on: true, select: {p.id, c.id})
|> plan()

assert all(query) ==
"SELECT s0.\"id\", s1.\"id\" FROM \"schema\" AS s0 LEFT OUTER JOIN \"schema2\" AS s1 ON 1"
Expand All @@ -1561,7 +1569,8 @@ defmodule Ecto.Adapters.SQLite3.ConnectionTest do
"SELECT * FROM schema2 AS s2 WHERE s2.id = ? AND s2.field = ?",
p.x,
^10
)
),
on: true
)
|> select([p, q], {p.id, q.z})
|> where([p], p.id > 0 and p.id < ^100)
Expand Down