This project is archived and is in readonly mode.

#6476 ✓committed
fxposter

[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

    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

    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

    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

    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

    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

    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

    fxposter April 22nd, 2011 @ 09:54 PM

    Thanks. I'll try to test master branch a bit later.

  • fxposter

    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>

Attachments

Pages