This project is archived and is in readonly mode.

#3424 ✓stale
Arya Asemanfar

[PATCH] Double loading associations when using preload and an identity map

Reported by Arya Asemanfar | October 25th, 2009 @ 01:44 AM

I'll start off by saying this bug is not reproducible without the use of an in-process identity map plugin. Given that, the bug is in the way ActiveRecord chooses whether or not to preload associations.

Example failure scenario:
class Author
has_many :posts, :include => [:comments] end

class Post
has_many :comments end

post = Post.find(1)
post.comments.to_a # to trigger load_target
posts = Author.find(post.author_id).posts
posts.to_a # to trigger load_target

If and only if you have a identity map-ish plugin, posts's objects will include post. I don't mean equivalent AR objects, but the objects with the same object_id.

Given the include option of has_many :posts, AR will iterate over and load all the comments for each post. But it only checks to see if the first post has its comments loaded before returning:

"return if records.first.send(reflection.name).loaded?"

So if the first element of record is not the post we loaded prior, then the post object has two copies of each of its comments in its association proxy.

Given that the identity map plugin is not included in AR, I should make clear the purpose of the patch is to correct the assumption made by AR's preloader that the either all or none of the records' associations are loaded.

Comments and changes to this ticket

  • Rizwan Reza

    Rizwan Reza January 17th, 2010 @ 02:14 AM

    • Tag changed from 3.0pre, activerecord, patch, preload_associations to 3.0pre, activerecord, patch, preload_associations, review
    • Assigned user set to “Pratik”
    • Title changed from “Double loading associations when using preload and an identity map” to “[PATCH] Double loading associations when using preload and an identity map”

    This applies cleanly on master.

  • Pratik

    Pratik January 17th, 2010 @ 07:50 AM

    Could you add a failing test please ?

    Thanks.

  • Arya Asemanfar

    Arya Asemanfar January 17th, 2010 @ 08:22 AM

    To create a failing test, there would have to be an identity map or the test would have to override AR's instantiate method to shim in identity behavior.

    To clarify, this bug is only reproducible with the presence of an AR identity map. I suppose it could remain unfixed until AR natively supports identity map, but it'd be nice to correct the assumption that all or none of the associated records are loaded.

    It's possible to create another example to break this assumption. Referring to the example in the original ticket: if the Post model implemented an after_initialize that conditionally loaded comments, but not for the record which will appear first in the author's posts.

    What do you think of that as a test?

  • Arya Asemanfar

    Arya Asemanfar January 17th, 2010 @ 08:57 AM

    Here is a patch for a test as described above. It's not the same situation as described in the original ticket, but it does break the same assumption that causes the problem when there is an identity map.

  • rails

    rails April 14th, 2011 @ 01:00 AM

    • Tag changed from 3.0pre, activerecord, patch, preload_associations, review to 30pre, activerecord, patch, preload_associations, review
    • State changed from “new” to “open”

    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 core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.

    Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.

  • rails

    rails April 14th, 2011 @ 01:00 AM

    • State changed from “open” to “stale”

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>

People watching this ticket

Pages