Skip to content
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.

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

Is there a reason to why ActiveRecord::Associations::Preloader is a nodoc? #11719

Closed
kevinsjoberg opened this issue Aug 2, 2013 · 14 comments
Closed

Comments

@kevinsjoberg
Copy link
Contributor

@kevinsjoberg kevinsjoberg commented Aug 2, 2013

Just recently I stumbled across a problem where I had to a pretty complex query using .find_by_sql. Unfortunately, eager loading in the form of .includes, doesn't work with that finder method.

After asking around, I got introduced to ActiveRecord::Associations::Preloader which came very handy in this particular situation. I was surprised to see that it was marked as a nodoc (see https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/preloader.rb), even though it's very well documented.

Is there any reason to this? Otherwise, I'd suggest to remove the nodoc mark and introduce it to the rest of the community.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 2, 2013

It is marked as nodoc because It is for internal use of the framework and should not be used in user applications.

@andyh
Copy link

@andyh andyh commented Aug 2, 2013

@rafaelfranca is there a better "rails-way" of acheving this that is part of the public api? I've seen this come up a few times recently. The key thing is that we want a way to load the associated objects too so we can't just do the joins in the find_by_sql

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 2, 2013

Using find_by_sql there is no way. But I pretty sure this same query can be written using the Active Record API.

@kevinsjoberg
Copy link
Contributor Author

@kevinsjoberg kevinsjoberg commented Aug 2, 2013

@rafaelfranca I see. But I don't think we should close this issue just yet. Currently this is a problem. Some queries just isn't suited for the Active Record API and I'll assume this is why .find_by_sql was created. I think it's fair to assume there should be an alternative to eager load records when the API isn't sufficient.

Don't you agree?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 2, 2013

Could you show an example where the API is not suited?

I agree that find_by_sql is a low level API, but you can't expect to have the same features of the high level API using it.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 2, 2013

Also I closed this issue since discussion can't be done on the issues tracker and in my opinion this is not an issue since you are expecting a low level API should work as a high level API.

@kevinsjoberg
Copy link
Contributor Author

@kevinsjoberg kevinsjoberg commented Aug 2, 2013

Some complex SQL queries aren't suited for the API, I guess we both can agree on that? For those types of situations I'd assume that if other low level features such as .find_by_sql is documented, why not document the Preloader as well.

It's handy to use the lower level API when the higher level API falls short. It's also easy to explicitly document a feature as low level, or in other ways state that it isn't a part of the higher level API.

I claim that this change is beneficial due to it giving advanced users an easier to overcome "complex" problems, while not getting in the way for ordinary users.

I didn't know that discussions were not allowed, so if this is the case, I'm sorry. I just want to prove my point.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Aug 2, 2013

Some complex SQL-queries isn't suited for the API, I guess we both can agree on that?

I never used find_by_sql in a project, so I can't agree with this.

It's handy to use the lower level API when the higher level API falls short. It's also easy to explicitly document a feature as low level, or in other ways state that it isn't a part of the higher level API.

Yes, agree with this. But like I said, you can't expect the lower level API works in the same way and with the same features that the higher level API.

I claim that this change is beneficial due to it giving advanced user an easier to overcome "complex" problems, while not getting in the way for an ordinary user.

I don't think we should add preloader to the public API. It is a complicated feature and adding it to public API will mean we have to support the actual API across releases removing the flexibility when evolving the framework.

But advanced users can still use it, but we don't guarantee it will not change between releases. It is the same case of arel.

I didn't know that discussions were not allowed, so if this is the case, I'm sorry.

No problem. Discussions should be done on the Rails Core mailing list. We keep the issues tracker for issues and pull requests.

I'd like to hear @jeremy @jonleighton and @tenderlove about this one, so I'm reopening.

I just want to prove my point.

Noted.

@rafaelfranca rafaelfranca reopened this Aug 2, 2013
@senny
Copy link
Member

@senny senny commented Aug 15, 2013

recently there was another ticket where using the Preloader was the best way to solve the issue #10912.

/cc @josevalim

@Bertg
Copy link
Contributor

@Bertg Bertg commented Feb 28, 2014

@senny Thx for mentioning my old issue here.

I do think if the Preloader would be public I would not have spent the time on that pull request and just use the feature that is hidden here in the docs.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 16, 2014

To make preload public API we need a lot of work. Right now the method signature is not easy to use. Also, I'm more inclined to make find_by_sql to work with include than making an internal class public.

That said I'm closing this issue since it is more a discussion, but I would love to see some implementation to make preload possible using custom SQL, or some kind of high level API easier to use than this private class.

@krzkrzkrz
Copy link

@krzkrzkrz krzkrzkrz commented Nov 3, 2015

Might be wise to re-open this issue.

As described at http://cha1tanya.com/2013/10/26/preload-associations-with-find-by-sql.html

includes will not work with find_by_sql. However, ActiveRecord::Associations::Preloader.new.preload(@users, :company) will eager load the respective associations, preventing n+1 queries

@kingdonb
Copy link

@kingdonb kingdonb commented Apr 20, 2017

I also arrived at reading this issue immediately after reading that article from 2013, too.

I'm still on Rails 4, and I guess that Rails 5 has added the "or" method to ActiveRecord scopes which would have solved my problem, so I guess I should upgrade then.

(I didn't upgrade. I just used Preloader and put a note in comments about how bad this code I had to write is, with a reminder to switch to ".or" whenever we get around to upgrading.)

@akaspick
Copy link
Contributor

@akaspick akaspick commented Dec 1, 2017

ActiveRecord::Associations::Preloader just helped me greatly optimize a page of mine that required where conditions on the preloaded associations. Adding the conditions directly to the main query was not the right place for them, and there are no other ways to add conditions to the joins, so this undocumented API was super helpful. I understand why it isn't a public API, but I've been using Rails for over 10 years and I JUST happened to come across it and it saved my day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants