Skip to content

has_one with includes excludes limit 1 from resulting query #10621

Closed
JangoSteve opened this Issue May 14, 2013 · 9 comments

7 participants

@JangoSteve
Ruby on Rails member

Note: This may not actually be a bug, but just a limitation of includes; if so, feel free to close this, but I figured it'd be good to write it down anyway since it's not in the documentation and someone else may search for this issue.

This seems to effect the latest 3-2-stable branch of rails as of right now. I have the following:

class User < ActiveRecord::Base
  has_one :latest_thing, :class_name => 'Thing'
end

class Thing < ActiveRecord::Base
end

If I then run:

User.includes(:latest_thing).first

I get the following SQL (I'm using the postgres adapter, I'm not sure if this is specific to that or not):

User Load (0.2ms)  SELECT "users".* FROM "users" LIMIT 1
  Thing Load (25.3ms)  SELECT "things".* FROM "things" WHERE "things"."user_id" IN (1)

Notice there is no LIMIT 1 included in the Thing query as would be expected. Alternatively, if I do this instead:

User.first.latest_thing

I now get the expected SQL:

  User Load (0.3ms)  SELECT "users".* FROM "users" LIMIT 1
  Thing Load (0.3ms)  SELECT "things".* FROM "things" WHERE "things"."user_id" = 3 LIMIT 1

The reason this may be a limitation rather than a bug is that, it wouldn't be possible have LIMIT 1 included in the SQL when writing the query with the includes method, since at the time in the Arel chain that includes is called, Arel wouldn't yet know that find is about to be called, as opposed to some other query that could be searching for multiple records of the parent association.

For example:

User.includes(:latest_thing).limit(10)

If the above included LIMIT 1 in the Thing SQL, then we'd only have the latest_thing loaded for one of the users.

But the current implementation is problematic because it will actually result in a query that loads all things for those 10 users, and instantiates them into an array of Thing objects for all of those things, which means we now have a deceptive query that could be very slow if those 10 users have something like 10,000 things between them that will all get instantiated now into Ruby objects. For us, it was the difference between a 100ms and 2,000ms in our view rendering.

Again, this may not be something that we'd even want to solve for in Arel, as I would expect it's just a limitation of using includes (considering the correct behavior would be a complex query that groups things on user_id and selects only one per group), meaning it just shouldn't be used in this scenario. But it seems like it should be mentioned in the docs for eager loading. I can add a small note about it, but wanted to do a sanity check before doing it and making sure this wasn't an unintentional bug in Rails first.

@neerajdotname
Ruby on Rails member

@JangoSteve this code fixes this issue . neerajdotname@master...10621

However this might cause quite a bit of confusion. Right now we have tests like this

    post = Post.all.merge!(:includes => :comments, :where => "posts.title = 'Welcome to the weblog'").first
    assert_equal 2, post.comments.size

After this fix only 1 record would be available for post.comments.

Personally I think this is a bit of edge case requirement. In most of the cases if I want eager loading then that means I want all the comments for the post which I am loading.

@JangoSteve
Ruby on Rails member

@neerajdotname Awesome thanks. But I kinda agree that it might be a bit of an edge case and there's not straight forward way to fix it without side effects. That's why I'm thinking maybe the best solution would be to add a note in docrails warning people that they typically shouldn't use eager loading :includes for has_one relations.

@rails-bot rails-bot added the stale label Aug 19, 2014
@rails-bot

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@rails-bot

This issue has been automatically closed because of inactivity.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@rails-bot rails-bot closed this Nov 19, 2014
@chrise86

Sorry to comment on an old issue, but I have to disagree with this

Personally I think this is a bit of edge case requirement. In most of the cases if I want eager loading then that means I want all the comments for the post which I am loading.

I'm building an API that uses Active Model Serializers and am getting lots of N+1 queries when rendering a user. For instance, a user has many followers, followees, photos, comments, likes, reposts, pets - lot's going on.

The default limit applied to the returned associations is 5 (or 21 in some cases), in the UserSerializer:

has_many :followers

def followers
  object.followers.limit(5)
end

However, to combat the N+1 queries I am using User.includes(:followers, :following, etc). This ignores all limits and instead eager loads the users 20K+ followers.

How would I go about combating the N+1 queries, yet still eager loading the associations with sensible limits?

@1st8
1st8 commented Mar 5, 2015

I am facing the same problem right now, will have to revert back to N+1 for now.

@1st8
1st8 commented Mar 5, 2015

Heh, I worked around the problem by forcing ActiveRecord to build a LEFT OUTER JOIN using the #eager_load method.

Instead of

# Loads multiple rows from my market_snapshots_table instead of two
# MarketSnapshot Load (5723.5ms)  SELECT "market_snapshots".* FROM "market_snapshots"  WHERE "market_snapshots"."listing_id" IN (55, 5)  ORDER BY "market_snapshots"."created_at" DESC
merchant.listings.includes(:current_market_snapshot)

do this

# Joins each row with single row from market_snapshots table instead of two
# SELECT  "listings"."id" AS t0_r0, ..., "market_snapshots"."id" AS t1_r0 FROM "listings" LEFT OUTER JOIN "market_snapshots" ON "market_snapshots"."listing_id" = "listings"."id" WHERE "listings"."merchant_id" = $1  ORDER BY "listings"."item_name" ASC LIMIT 25 OFFSET 0  [["merchant_id", 1]]
merchant.listings.eager_load(:current_market_snapshot)

The only thing missing right now is joining the most recent market_snapshot (ORDER BY "market_snapshots"."created_at" DESC in the first), but it should give a starting point.

edit: As expected, adding .order('market_snapshots.created_at DESC') produces the expected query.

@ajspera
ajspera commented Nov 5, 2015

Note, the eager_load solution will nuke the query if you have any has_many collection includes or eager loads happening simultaneously.

@thehenster thehenster added a commit to alphagov/service-manual-publisher that referenced this issue Apr 17, 2016
@thehenster thehenster Replace Guide#latest_edition association
 * Replace Guide#latest_edition association with an alias to #latest_persisted_edition

The #latest_edition association was causing frustration. It was a has_one association with a scope and it was easy to forget it was a has_one and try and use it as a pure scope. You would call it the first time and the scope would be used to load the latest edition but when you called it a second time it would return the loaded model. We decided to take the time to remove it.

The first step was removing the nested attributes which relied on the association which has been done.

 * Replace use of #latest_edition in factories

 * Replace use of #latest_edition in factory overrides

 * Remove the includes for :latest_edition

They weren't working anyway. Limits are ignored when preloading and don't work with has_ones either rails/rails#10621.

This post talks about a way to get it working using a subquery https://ksylvest.com/posts/2014-12-20/advanced-eager-loading-in-rails

For the moment it's safer to not preload because we are saving so many editions now.

 * Replace use of Edition#build_latest_edition

Now the association is gone the method doesn't exist anymore.

 * Fix a couple of Publisher specs for removal of #latest_edition

 * Reload editions to fix a test for removal of #latest_edition

 * Test fixes for removal of #latest_edition

 * Fix SearchIndexer test for removal of #latest_edition

 * Validate new Guide editions have a content_owner

Previously we could check latest_edition to see if it was valid when it was an attribute. Now we need to search through for the unpersisted edition in the editions association collection.

 * Rename Guide#latest_persisted_edition to #latest_edition
998bcea
@thehenster thehenster added a commit to alphagov/service-manual-publisher that referenced this issue Apr 18, 2016
@thehenster thehenster Replace Guide#latest_edition association
 * Replace Guide#latest_edition association with an alias to #latest_persisted_edition

The #latest_edition association was causing frustration. It was a has_one association with a scope and it was easy to forget it was a has_one and try and use it as a pure scope. You would call it the first time and the scope would be used to load the latest edition but when you called it a second time it would return the loaded model. We decided to take the time to remove it.

The first step was removing the nested attributes which relied on the association which has been done.

 * Replace use of #latest_edition in factories

 * Replace use of #latest_edition in factory overrides

 * Remove the includes for :latest_edition

They weren't working anyway. Limits are ignored when preloading and don't work with has_ones either rails/rails#10621.

This post talks about a way to get it working using a subquery https://ksylvest.com/posts/2014-12-20/advanced-eager-loading-in-rails

For the moment it's safer to not preload because we are saving so many editions now.

 * Replace use of Edition#build_latest_edition

Now the association is gone the method doesn't exist anymore.

 * Fix a couple of Publisher specs for removal of #latest_edition

 * Reload editions to fix a test for removal of #latest_edition

 * Test fixes for removal of #latest_edition

 * Fix SearchIndexer test for removal of #latest_edition

 * Validate new Guide editions have a content_owner

Previously we could check latest_edition to see if it was valid when it was an attribute. Now we need to search through for the unpersisted edition in the editions association collection.

 * Rename Guide#latest_persisted_edition to #latest_edition
405aece
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.