Ecto: Add :order_by opt to has_many

[gjaldon]

Encountered a problem today where I wanted to order a has_many association by a field other than its id. Let's say I have a User model that has_many :feedbacks, Feedback. I preload :feedbacks in user so I can display them on the user's profile.

Since the most relevant feedback is the latest one, I want to arrange all feedback to display the newest one first and the oldest one last. There doesn't seem like a way to do this on a preloaded association unless I manually order them myself after the query. Was thinking of adding an :order_by option to has_many/3 which accepts the same args as Query.order_by/3.

Is that okay?

[ericmj]

If we go down this road we should make it more general, maybe each association should be a queryable or you should be able to specify how to query the association when preloading it.

[gjaldon]

That approach is definitely more flexible and gives the user more power over how to query the associations. :+1:

[josevalim]

So this is tougher than I have originally thought. I have thought about first tackling this as:

has_many :public_comments, from(c in Comment, where: c.public == true, order_by: ...)

However, this has an issue in that from(c in Comment, ...) will try to load the Comment model at compile time, so this is not a viable option. One alternative I have in mind is to allow the following:

has_many :public_comments, c in Comment, where: c.public == true, order_by: ...

has_many/3 is already a macro, so we would need to add some sugar in order to support where and order_by, but we are mostly fine. Furthermore, the option above would allow us to at least sanity check, because we truly can't support much more than where and order_by without going into all sorts of edge cases.

Thoughts?

[josevalim]

@gjaldon I would like to tackle this one because it is very complex and there are still some moving parts I have not solved yet.

[gjaldon]

@josevalim sure, please go ahead. :)

Passing the queryable to has_many would have been the ideal since that way we could possibly pass functions that return a query like has_many :public_comments, ^Comment.scope. No idea how if it's possible though given the issue of the Comment model being loaded at compile-time.

If not possible, then the next option sounds great too!

Btw, would also be great to have preload/3 have a 'scope query' so we can customize how associations are queried on a per-query basis rather than per-model like above.

[josevalim]

We can't have preloads because if you do assoc(p, :public_comments) in a query there is no way we can get preloads from that.

[foxnewsnetwork]

Sorry if I'm noob and this is a bad idea, but what about just making it easier for users to create their own has_many derivative in the schema section? For example, my current solution for this is the following:

In my model web/models/dock.ex is to straight-up use the Schema.association macro

schema "docks" do
...
association :many,
  :on_site_trucks,
  App.HasManyOnSite,
  foreign_key: :dock_id,
  queryable: App.Truck
...
end

and in web/scopes/has_many_on_site.ex I just sort of "inherit" Ecto's has_many association by defining my own :

defmodule App.HasManyOnSite do
...
  def assoc_query(refl, values) do
    from x in refl.queryable,
      where: x.is_on_site == true and is_nil(x.deleted_at),
      where: field(x, ^refl.assoc_key) in ^values
  end
...
end

And copy+pasting a ton of code from https://github.com/elixir-lang/ecto/blob/master/lib/ecto/association.ex#L240

I mean, I realize the copy+paste portion isn't ideal (o rly?), but what if we made it easier to copy+paste through a __using__ in Ecto.Association.Has? Lowly end users like me would be empowered by having the much coveted ability to customize our association queries, but at the same time, we would avoid having to fiddle too much within Ecto's internals.

[josevalim]

@foxnewsnetwork this is a very good suggestion. The trouble in support those things though is not the assoc_query but the joins_query which needs to be expanded and analyzer by Ecto's query planner as in this example:

join: p in assoc(p, :comments)

That's the root of the issue. For example, if :comments above has a limit, how would that limit be applied when used as a join? we literally can't solve this, so we need to maintain associations small so they can be used properly throughout Ecto. If you need more than this then you should just build and extend a regular query.

[josevalim]

I have been thinking more about this issue. One of the things I really liked on Rails associations is that conditions given to the association was also used when building it. For example, if I declared:

has_many :public_comments, Comment, conditions: [public: true]

When I did:

build(post, :public_comments)

The returned post would have public: true:

%Comment{public: true}

I would love to have this behaviour in Ecto. However, if we are willing to support this feature via the syntax I mentioned above:

has_many :public_comments, c in Comment, where: c.public == true, order_by: ...

We would need to traverse the where and extract those expressions. Which I am not a fan of. Right now I am thinking about supporting this syntax:

has_many :public_comments, Comment, where: [public: true], order_by: [:published_at]

However, we don't support it in queries (which we may or may not need to do too). Thoughts?

/cc @drewolson @jeregrine (pinging more folks because this one has been tough)

[michalmuskala]

If we're talking about rails - the ability to pass procs to association instead of a clunky hash with strange attributes was a great improvement. The old behaviour was very limited in scope and usage, the new one is much better (I know this as I was migrating two apps from rails 3 to rails 4 when this change was made - it simplified things a lot).
My main problem with a keyword is that you can't use it for relations other then equality - <, >, etc, are all unusable. I'm still thinking that ability to pass a function name or a fun would be the best one. You could do something like:

has_many :current_entries, Entry, fn -> 
  from e in Entry, where: e.entered_at >= ^beginning_of_day and is_nil(e.left_at)
end
has_many :current_visitors, Person, through: [:current_entries, :visitor]

[josevalim]

Good points. Maybe we should allow an on_build option that sets a parameter on build?

has_many :public_comments, c in Comment, where: c.public == true, order_by: ..., on_build: [public: true]

Thoughts?

[gjaldon]

I like the function approach for setting a 'default scope' for the association since we could do more powerful queries. It would be a challenge though to build an association with default values from conditions like Rails does.

How 'bout we have a :defaults option for setting default values for the built association and use the fun for preloading the association? Something like:

has_many :public_comments, Comment, fn (query) -> 
  from c in query, where: c.public == true
end, defaults: %{public: true}

Was about to post, before your :on_build opt suggestion popped up. :) But yeah, that sounds great! :on_build or :defaults is more explicit than what Rails provides.

[josevalim]

I can't support anonymous functions because of the reasons I mentioned above. If you do a limit, how does that even behave when we do a join in that association? So I would rather whitelist the supported operations.

@gjaldon I like :defaults more than :on_build, let's go with it.

[gjaldon]

Ah, I see. Ok sounds good to me! :)

[jeregrine]

:+1: on defaults.

Could a solution exist where association queries exist but certain actions
don't apply like limit? And we just document or even log a warning when
used as a preload on a join?

On Tue, Jul 14, 2015 at 6:57 AM, Gabriel Jaldon [email protected]
wrote:

Ah, I see. Ok sounds good to me! :)

—
Reply to this email directly or view it on GitHub
https://github.com/elixir-lang/ecto/issues/659#issuecomment-121215194.

[drewolson]

I personally feel like I'd steer clear of using a feature like this and rather just use functions on my models. The asymmetry of the API (a difference syntax for association conditions and queries) is worrying to me.

I do think :defaults is the best compromise, but again, I'd probably just use functions to accomplish the same (either fetching the association with a scope on a separate query or doing the join + scope).

[josevalim]

@drewolson the last proposal would still have a symmetric API:

has_many :public_comments, c in Comment, where: c.public == true, order_by: ..., defaults: [public: true]

Would you still steer away from this?

[jeregrine]

I am with Drew, I don't super use build at all. Just set the post_id
directly :P

I like the scoped queries but currently in elixir I just use functions and
its working out okay

Comment.for_post(post) => from(c in Comments, where: c.post_id == ^post.id)

On Tue, Jul 14, 2015 at 7:57 AM, José Valim [email protected]
wrote:

@drewolson https://github.com/drewolson the last proposal would still
have a symmetric API:

has_many :public_comments, c in Comment, where: c.public == true, order_by: ..., defaults: [public: true]

Would you still steer away from this?

—
Reply to this email directly or view it on GitHub
https://github.com/elixir-lang/ecto/issues/659#issuecomment-121231476.

[jeregrine]

or I just use preload all over. :P

On Tue, Jul 14, 2015 at 8:02 AM, Jason S [email protected] wrote:

I am with Drew, I don't super use build at all. Just set the post_id
directly :P

I like the scoped queries but currently in elixir I just use functions and
its working out okay

Comment.for_post(post) => from(c in Comments, where: c.post_id == ^post.id
)

On Tue, Jul 14, 2015 at 7:57 AM, José Valim [email protected]
wrote:

@drewolson https://github.com/drewolson the last proposal would still
have a symmetric API:

has_many :public_comments, c in Comment, where: c.public == true, order_by: ..., defaults: [public: true]

Would you still steer away from this?

—
Reply to this email directly or view it on GitHub
https://github.com/elixir-lang/ecto/issues/659#issuecomment-121231476.

[drewolson]

@josevalim yeah, I understand that it's symmetric now in the proposal, but it feels like we kind of tacked it on to the "standard" query syntax for the purposes of symmetry. I'd probably still just use functions. That said, if I'm alone on this I'd happily see it as a feature in Ecto. I don't think it takes anything away from the current API, I'm just generally wary of increasing the surface area of the (currently lovely) API.

[josevalim]

Ok, let's close this. We can add a guideline that associations are meant to work on data, with the help of the upcoming support for associations in changeset and it doesn't make sense to pollute your structs with complex associations if you are mostly worried about querying. Given the complexity of all the discussions in here, it does sound like the most sensible decision to make.

[foxnewsnetwork]

...add a guideline ...

Yes, so much this! From a lowly plebeian end-user point of view, it isn't that Ecto is missing features or that the has_many doesn't support scopes and defaults. It's more that there is no Ecto homepage with a reasonably comprehensive guide to the canonical use patterns of Ecto's existing features. The Simple example and the existing guide attached to the Phoenix framework are a great start, but they don't (yet) cover all the more "intermediate-to-advanced" patterns required in building a more ambitious app such as many-to-many polymorphism, association scopes, where to even put queries (should I be polluting my controller? My model? In a separate file?), etc.

But, as a new-to-Ecto rails refugee, a lot of the problems that I used to solve simply by following existing convention now must be solved by explicitly making design decisions. And this isn't good because, as end-users, my feeble brain-power is hilariously limited and I would prefer to focus on solving my business-logic problems rather than having to figure out framework implementation. In other words, it's not so much that I _must_ have rails convention in Ecto, it's more that I would really like _some_ sort of convention that I could follow like a mindless lemming.

So, TL;DR: if you good folks on core are already building a guide website for Ecto, great, I'd love to contribute. If not, I can scrap together something like Phoenix has.

[josevalim]

That's very insightful @foxnewsnetwork. I am not sure how guidelines would work for Ecto but let's indeed try to get something running, maybe start a github repo with some patterns so we start sketching what makes sense.

[gjaldon]

Sounds good! Would be glad to contribute there too.

Btw, @josevalim how to preload comments for user and order them at the same time(just one visit to the DB)? Doesn't seem possible at the moment. It would be helpful for us to be able to customize how associations are queried. Will this be made possible with associations in changeset?

[josevalim]

@gjaldon I think this will support it: https://github.com/elixir-lang/ecto/issues/784

[gjaldon]

@josevalim but doesn't Repo.preload/2 expect a fetched model so would take 2 trips to the db?

Repo.get(User, 1) |> Repo.preload(:comments)

[josevalim]

@gjaldon it is actually better to make two trips in the majority of case. The issue with assoc(...) in queries is that if you have posts and comments, the amount of data returned from the database will be posts_size x comments_size because it uses a join. So we end-up sending more data and spending more time processing this data in Ecto.

With two queries, the result set is posts_size + comments_size.

[gjaldon]

Sorry if I'm being persistent here, just want to know what's the best way to go about this. In the context of a Phoenix show action, I load a User who has many comments. I want to be able to get just the active comments. I was initially thinking to do something like:

User |> Ecto.Query.preload(:comments) |> Repo.get(1)

Problem is I can't set how the comments are preloaded. In this case, I could use the queryable support in Repo.preload/2:

Repo.get(User, 1) |> Repo.preload(:comments, Comment.active_comments)

But this would result in 2 trips to the DB which may be negligible, but isn't the ideal.

Shouldn't Ecto make it easy for users to do the more ideal/performant query?

[josevalim]

The first case is also doing two trips to the database too. Again, it is actually better to make two trips in the majority of cases, see my answer above. If you really care about doing one trip:

query = from u in User,
  join: c in assoc(c, :comments),
  where: c.active,
  preload: [comments: c]
Repo.get(query, 1)

[gjaldon]

I see. Thanks for the explanations and the patience. :) Just wondered how best to do preloading for the case I mentioned. It looks like Repo.preload/2 is the way to go.

[romul]

What is status of this issue in Ecto 2? Has a syntax support for order_by in has_many been introduced?

[josevalim]

I don't think it has but now we support custom queries when preloading which can be used to solve this issue.

[josevalim]

I am back on a computer. Today you will be able to do:

Repo.preload(posts, comments: from(Comment, order_by: [desc: :inserted_at]))

And that's going to work. You can also specify it as part of a query:

def with_recent_comments(query) do
  from p in query,
    preload: [comments: from(Comment, order_by: [desc: :inserted_at]))]
end

[romul]

@josevalim Thank you for the answer.
But how can I set a default order? For example, I have position column and want associated entities to be always sorted by position. Setting this sort order manually in each preload or introducing a new function (and therefore a new way of preload for each such a case) looks tedious and error-prone for such the common task.

[nathany]

Well, since preload(posts, :comments) isn't going to apply a default order, I guess the only reusable solution is to write a function to preload_comments and use that everywhere instead.

[romul]

@nathany Correct! But it doesn't look like a reasonable solution, more like a workaround.

[romul]

I like the idea to be able to use something like

has_many :public_comments, Comment, fn (query) -> 
  from c in query, where: c.public == true
end, defaults: %{public: true}

It would be much more convenient and consistent.

[smeevil]

The above would be quite nice, I think this would also allow for polymorphic associations.

For example :

has_many :comments, MyApp.Comment, foreign_key: :commentable_id,  fn (query) -> 
  from c in query, where: c.commentable_type == "articles"
end

[abitdodgy]

I think it's a shame that we can't get defaults: [public: true]. This is a handy feature.

In my case, which I don't think is unusual, I have a Membership schema that links Account and User. The Membership schema has a role column, staff or admin. An Account will always define its associations based on that column. So...

has_many staff_users, Membership, defaults: [role: :staff]
has_many admin_users, Membership, defaults: [role: :admin]

There's no need here for a generic has_many :users. So the lack of ability to define this condition seems to make the association rather redundant.

[jfrolich]

It would be great to have something like this. Is there any update on this with regards to Ecto 2? (Maybe it is already possible in some way).

[programisti]

Since this issue gets bumped by search engines I need to leave this here:
I have more efficient way of @smeevil 's solution

has_many :comments, MyApp.Comment, foreign_key: :commentable_id, where: [commentable_type: "articles"]