Add missing :on keys to queries with joins

[joeljuca]

It's mostly a no-op when it comes to features, etc. – but it eliminates these annoying warnings that get in the way of important mix test outputs (tests breaking with insightful information).

Before this patch, mix test looks like this:

$ mix test
.........................warning: missing `:on` in join, defaulting to `on: true`.
  test/ecto/adapters/sqlite3/connection_test.exs:307: Ecto.Adapters.SQLite3.ConnectionTest."test parent binding subquery and CTE"/1

warning: missing `:on` in join on Ecto.Adapters.SQLite3.ConnectionTest.Schema2, defaulting to `on: true`.
  test/ecto/adapters/sqlite3/connection_test.exs:1412: Ecto.Adapters.SQLite3.ConnectionTest."test join with hints"/1

warning: missing `:on` in join, defaulting to `on: true`.
  test/ecto/adapters/sqlite3/connection_test.exs:1478: Ecto.Adapters.SQLite3.ConnectionTest."test join with subquery"/1

warning: missing `:on` in join, defaulting to `on: true`.
  test/ecto/adapters/sqlite3/connection_test.exs:1515: Ecto.Adapters.SQLite3.ConnectionTest."test join with fragment"/1

warning: missing `:on` in join, defaulting to `on: true`.
  test/ecto/adapters/sqlite3/connection_test.exs:1548: Ecto.Adapters.SQLite3.ConnectionTest."test join with query interpolation"/1

warning: missing `:on` in join, defaulting to `on: true`.
  test/ecto/adapters/sqlite3/connection_test.exs:1557: Ecto.Adapters.SQLite3.ConnectionTest."test lateral join with fragment"/1

.............................................................................................................................................................................................................
Finished in 1.4 seconds (1.3s async, 0.09s sync)
230 tests, 0 failures

Randomized with seed 200492

After this patch:

$ mix test
......................................................................................................................................................................................................................................
Finished in 1.0 seconds (0.9s async, 0.1s sync)
230 tests, 0 failures

Randomized with seed 89209

[warmwaffles]

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

[joeljuca]

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).

[warmwaffles]

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

[joeljuca]

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.

[warmwaffles]

@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 😄

[joeljuca]

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?

[warmwaffles]

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

[warmwaffles]

@joeljuca I was comparing these tests to what is in the postgres adapter and what you changed to is roughly what they do. So I just merged this and am cleaning up some tests now.