#6476 [PATCH] Eager loading don't work properly on nested includes - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#6476 ✓committed

[PATCH] Eager loading don't work properly on nested includes

Reported by fxposter | February 27th, 2011 @ 03:42 AM

If we have a following models:

class Article < ActiveRecord::Base
  has_and_belongs_to_many :tags
end

class Tag < ActiveRecord::Base
  has_and_belongs_to_many :articles
  has_many :translations
end

class Translation < ActiveRecord::Base
  belongs_to :tag
end

and some seed data:

tag1_translation = Translation.create!
tag2_translation = Translation.create!
tag3_translation = Translation.create!
tag4_translation = Translation.create!
tag1 = Tag.create! :translations => [tag1_translation]
tag2 = Tag.create! :translations => [tag2_translation]
tag3 = Tag.create! :translations => [tag3_translation]
tag4 = Tag.create! :translations => [tag4_translation]
article1 = Article.create! :tags => [tag1, tag2]
article2 = Article.create! :tags => [tag2, tag3]
article3 = Article.create! :tags => [tag1, tag2, tag3]
article4 = Article.create! :tags => [tag4]

Then this query:

Article.includes(:tags => :translations)

will not preload :translations to all tag objects

 > Article.includes(:tags => :translations).map { |a| a.tags.first.id }
=> [1, 1, 1, 4] 
 > Article.includes(:tags => :translations).map { |a| a.tags.first }.map(&:translations).map(&:loaded?)
=> [true, false, false, true]

The bug appears when we have one record mapped into several ruby objects - then only one object of those will get next "includes()".

After some investigations, I've come to this line:
https://github.com/rails/rails/blob/master/activerecord/lib/active_...

If I remove ".uniq" call - everything works just fine except one failing test - test_including_duplicate_objects_from_belongs_to.

Comments and changes to this ticket

You flagged this item as spam.
fxposter February 27th, 2011 @ 04:16 AM
It seems that removing ".uniq" ActiveRecord::AssociationPreload::ClassMethods#preload_associations and adding "records = Array.wrap(records).compact.uniq" to ActiveRecord::AssociationPreload::ClassMethods#preload_has_many_association fixes that bug and doesn't break anything (according to tests).

fxposter February 27th, 2011 @ 04:33 AM

No, I was wrong. It doesn't fix the problem.

  def test_including_duplicate_objects_from_has_many
    post = Post.create!(:title => 'foo', :body => "I like cars!")
    comment = SpecialComment.create!(:body => 'Come on!', :post => post)
    first_category = Category.create! :name => 'First!', :posts => [post]
    second_category = Category.create! :name => 'Second!', :posts => [post]
    
    categories = Category.where(:id => [first_category.id, second_category.id]).includes(:posts => :special_comments)
    assert_equal categories.map { |category| category.posts.first.special_comments.loaded? }, [true, true]
  end

That's the test case, showing the bug.

You flagged this item as spam.
fxposter February 27th, 2011 @ 09:41 PM
- Tag changed from “eager_loading” to “bug, eager_loading, patch”
- Assigned user set to “Aaron Patterson”
Added patch with fix and test case (I'm not sure if my fix is 100% right, but it works + doesn't break any tests).
- eager_loading_patch.diff 2.7 KB
You flagged this item as spam.
fxposter March 2nd, 2011 @ 09:06 AM
- Title changed from “Eager loading don't work properly on nested includes” to “[PATCH] Eager loading don't work properly on nested includes”
You flagged this item as spam.
Peter Bui March 2nd, 2011 @ 10:01 PM
+1

I applied this patch successfully to the 3-0-stable branch and ran the tests successfully.

I went back and looked at why the Array#uniq call was there in the first place (https://github.com/rails/rails/commit/5dee6ce9e0e31c86c4319c17a1ee5...) and it looks like it's residue from 3 years ago. The current code base (before patch) already ensures there are no duplicate records loaded with eager loading, so you're right that the Array#uniq is not needed.
Aaron Patterson April 22nd, 2011 @ 06:07 PM
- State changed from “new” to “committed”
- Importance changed from “” to “Low”
I've pulled this commit in to 3-0-stable. I tried porting to master, but the test didn't fail on master. I'm closing this, but can you verify that master is OK?
You flagged this item as spam.
fxposter April 22nd, 2011 @ 09:54 PM
Thanks. I'll try to test master branch a bit later.
You flagged this item as spam.
fxposter May 1st, 2011 @ 10:20 AM
It seems like rails edge doesn't have this problem - everything works fine.