It definitely makes sense to nicely support the behaviour, I'm just wondering, since records is a true ActiveRecord::Relation instance, isn't it possible to just call eg. Category.search('one').records.includes(:articles).to_a -- if I work with the code in the associations example?

@karmi

You can technically call includes on records, but here's my issue:

records = Search::MapSearch.new(params).search.page(page).per(per_page).records
records.total_count
# => 740
records = Search::MapSearch.new(params).search.page(page).per(per_page).records.includes(map_items: :item)
records.total_count
# undefined method `total_count' for #<Map::ActiveRecord_Relation:0x007fd968e50878>

I guess I could do

records = Search::MapSearch.new(params).search.page(page).per(per_page).records
total_count = records.total_count
records = records.includes(map_items: :item)

It seems like it works fine, but that's not ideal.

Actually, it's not working as I expected...

search = ES::MapSearch.new(params).search
@total_count = search.total_count # performs the ES query
@took = search.took
@maps = search.page(page).per(per_page).records
@maps = @maps.includes(:user, { map_items: :item }) # performs the ES query a second time! 

The Elasticsearch query is performed twice!
I'm not sure how to get pagination, includes, and get some raw data about the query (took).

Having same issue, this PR would help a lot!

I've used this approach - but still, elasticsearch could support this feature, since approach I've used is hacky.

That seems like a pretty nice solution, well done. What I had done personally is just give up on kaminari pagination, using size and from in the ES query, and then getting the models via the result ids, since I just needed a "Load more" type of pagination.

Hi, @karmi. Any plans to get this merged?

My apologies to all, and first of all to you, @dabit, this took too long. Thanks for pinging me, @brianstorti!

I've added an integration test in the attached commit, and merged. I wanted to explore if we couldn't fix the more natural way of expressing the same, Category.search('one').records.includes(:articles).to_a, but I couldn't put much time into it, so let's start with merging @dabit's patch, and maybe work further in the future.

Thanks for the link to the decorator article, @donce!

@karmi no worries, this is great timing as precisely yesterday I had to patch one of my apps with this change and I did get a considerable performance boost. When there's a lot of data showing up in a page from different related models, that N+1 situation can kill you.

Thanks!

Awesome. Thanks, @karmi and @dabit. Is this going to be released soon?

@brianstorti, I'd like to accumulate more changes before a release to Rubygems, but if you need a release for your environment and cannot just use Git in Gemfile, I can do a release.

Actually, there has been a lot accumulated already :), so just released 0.1.9. There's some kind of weird unit test failures depending on whole test suite running or not, which I investigated for a bit, but didn't chew through it, will have a look at it later.

Was excited to find this PR, but having some issues implementing:
Gemfile

gem 'elasticsearch-rails', '~> 0.1.9'

/models/product.rb

 has_many :product_images, dependent: :destroy

controller

results = Product.search(query)
results = results.page(params[:page]).records(includes: [:product_images])

I get an argument error: wrong number of arguments (1 for 0)

Have I misunderstood how to implement this? Thanks in advance!

I don't think you need to call results if you're calling records. What about just Product.search(query).page(...).records(includes: [:product_images]).