This project is archived and is in readonly mode.
[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
-
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.
-
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).
-
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
-
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?
-
fxposter May 1st, 2011 @ 10:20 AM
It seems like rails edge doesn't have this problem - everything works fine.
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>