[PROPOSAL] Require explicit join .. on: true

[Daniel Kukula]

Currently you can do:

    join(SchemaA, :left, [a], b in SchemaB)

This is very similar to:

    join(SchemaA, [a], b in assoc(a, SchemaB))

and it works similar in tests when you have only 1 of each rows, so the tests may be passing when I change from assoc to left join having only minimal setup.
My proposal is to require to specify the `on: true` otherwise log a warning when it's missing:

    join(SchemaA, :left, [a], b in SchemaB, on: true)

I saw some production failures due to this in my short career already.
I searched but could not find if this was discussed before.

[Greg Rychlewski]

This is a very good callout, imo. I wonder if this warrants a breaking change to raise when `:on` is not specified for non-cross joins. I can't think of a legitimate use case for leaving out `:on`, though I might be missing something. Seems like this might be falling into "introduce a breaking change to fix erroneous behaviour" territory.

[José Valim]

I am not sure.

There are likely many examples of functions throughout Elixir where passing an argument or not changes its meaning.

Plus the benefit of assoc/2 is exactly not having to pass the :on option.

Another suggestion is to introduce inner_join/left_join/etc as macros and make the atom argument required in the join one.

--
You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/7d65bbb4-19cb-4d0f-a391-280a159fb417n%40googlegroups.com.

[danie...@gmail.com]

I imagine we should not require this with assoc because assoc sets the on by default.
I think only when joining 2 schemas this should be specified otherwise ecto generates on: true and it can be a problem.
This is hard to spot unless you experienced it before and know to check for queries that are missing on when you get oom kills.

[José Valim]

Oh, I get it now. I think emitting a warning if the `on` option is missing and there is no assoc is a good compromise.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/d8dff8c3-5e4b-4904-885c-5adc9634352en%40googlegroups.com.