Allow many_to_many to order preloads by join table

[greg-rychlewski]

This was inspired by this comment: #3509 (comment).

It seems useful to be able to order by the join table, especially if you are adding metadata to the relationship.

The proposal here was the best one I could think of. It feels a little awkward having two options but the other ideas I thought of didn't seem too great:

1. Mixing the source table option into preload_order seems a bit messy.
2. I don't think we want to allow bindings or anything like that in the schema definition. It seems complicated and messy.

[josevalim]

What about adding join_preload_order, to mirror join_defaults and join_where?

[greg-rychlewski]

That sounds like a good option to me. Do you think we should raise if both join_preload_order and preload_order are specified? I think otherwise the order to apply them would be ambiguous.

Maybe this is overkill, but given ^ do you think we should considering giving users the option to order across the tables with :preload_order? Something like: order by join_table.col_a, assoc_table.col_b, .... The API is maybe not so clean though I think you'd have to do a nested keyword list: [{:join, asc: :col_a}, {:assoc, asc: :col_b}]

[josevalim]

How hard would it be to check the columns and, if missing, we assume it is from join?

[greg-rychlewski]

It would be contained to a relatively small place. Just this function: https://github.com/elixir-ecto/ecto/blob/master/lib/ecto/repo/preloader.ex#L318

defp preload_order(assoc, query, related_field) do
  custom_order_by = Enum.map(assoc.preload_order, fn
    {direction, field} ->
      {direction, related_key_to_field(query, {0, field})}
    field ->
      {:asc, related_key_to_field(query, {0, field})}
  end)

  [{:asc, related_field} | custom_order_by]
end

You have the assoc so you can check if it is many to many. And you have the query so you can get the from source. Then I think you'd just need to use the reflection functions to see if the field exist. Like checking if the type is nil or something like that.

The one issue I can see though is you wouldn't be able to distinguish if both tables had the same column name.

[josevalim]

Yeah, i am not sure what is the best API. :(

[greg-rychlewski]

Would it be bad to have pseudo-bindings. For instance:

[asc: join.col_a, desc: col_b, asc: assoc.col_c]

[josevalim]

I would avoid adding them at compile-time. Maybe we could allow preload_order as a MFA and in there you should have access to both?

[greg-rychlewski]

Sorry if I'm misunderstanding, were you mentioning compile-time due to generating dependencies between modules? If so, what I meant was to have "dummy" bindings. Something like this is supplied by the user:

preload_order: [asc: join.col_a]

where join is not related to any schema. It's just a fixed binding we understand. And then it gets compiled to {:join, :asc, :col_a}. And if there is no dummy binding specified it gets compiled to {:assoc, dir, col}.

I was trying to think of how to use a function too. Though I think we'd have to give them access to modify the entire query rather than generate just the new order_by clause. Unless I am not seeing something.

[josevalim]

Sorry, what I meant is that extracting a macro expression from an option will be tricky and confusing, especially because none of the other options have macro-like behaviour.

Though I think we'd have to give them access to modify the entire query rather than generate just the new order_by clause. Unless I am not seeing something.

Gah, good point. I guess the best API so far is the one you proposed as preload_order: [through: [...], assoc: [...]]. But I would wait a bit more... maybe inspiration will strike later?

[greg-rychlewski]

Yeah let's wait :).

[greg-rychlewski]

I was trying to get something like this to work:

   preload_order: fn -> dynamic([join, assoc], assoc.field) end

But couldn't get it to work. I always get compiler errors either about dynamic not existing or the binding not existing.

Sorry to bug you but I'm hitting my limits a bit =). Do you know if this is a definite no-no or if there should be something that can make it work?

[josevalim]

I am almost sure it has to be written using MFAs (which is why we use MFAs everywhere).

[greg-rychlewski]

I think the MFA works pretty nicely. WDYT of this change?

[greg-rychlewski]

Thanks for the discussion/feedback. I finished the changes we talked about . Some callouts:

1. I added the new order_by option to this PR because it was a small change. I could separate it out if you prefer.
2. I didn't add the new order_by option to the keyword queries because it's a bit more work and distracts a bit from the other changes. But I could follow up quickly with a PR for that.
3. I changed it so that any :preload_order is now using the new order_by option even if it's a compile-time keyword list/list.
4. There is a default ordering Ecto applies that goes right at the beginning of the query. This is so the preloaded structs can be placed into their parents in the right order. This still has to use the old way because the ordering is coming from the field definition in the association struct. The field definition has the binding position in it and there is also some custom logic for negative indices. So I thought I better not touch that.

[josevalim]

I didn't add the new order_by option to the keyword queries because it's a bit more work and distracts a bit from the other changes. But I could follow up quickly with a PR for that.

I agree it is not necessary. It is fine to depend on the keyword one for these cases.

[josevalim]

I have added some minor nits, overall it looks great!

[josevalim]

Just one minor nit to do less work inside the quote and ship it!