This project is archived and is in readonly mode.

#1110 ✓committed
Will Bryant

Eager loading regression for find with :includes of associations already :included by the parent association

Reported by Will Bryant | September 25th, 2008 @ 12:50 PM | in 2.x

If you have a has_many defined with :includes:


class Author < ActiveRecord::Base
  has_many :posts_with_comments, :include => :comments, :class_name => "Post"
end

And then you load that with a find that includes the same thing:


Author.find(author_id, :include => {:posts_with_comments => :comments})

(or {:posts_with_comments => {:comments => :something_else}}, for example)

Then in 2.1.x/edge you'll get two copies of each Comment record in the author's post_with_comments.comments collection. This is a regression from 2.0 caused by the way the new preloading code works down through the associations.

Attached file includes a testcase demonstrating the problem.

Comments and changes to this ticket

  • Will Bryant

    Will Bryant September 27th, 2008 @ 04:54 AM

    Patch and tests attached for has_many, has_one, and belongs_to. HABTM and has_many :though have unrelated issues so no change needed there.

  • Michael Koziarski

    Michael Koziarski September 29th, 2008 @ 04:52 PM

    • Tag changed from 2.1, activerecord, bug, eager_loading, edge, has_many to 2.1, activerecord, bug, eager_loading, edge, has_many, patch

    I can't find fred's account in lighthouse (i.e. it doesn't exist).

    But do you know if he made any progress with this?

  • Will Bryant

    Will Bryant September 30th, 2008 @ 09:01 AM

    Fred as in Frederick Cheung from the mailing list? AFAIK he hasn't had this issue himself.

  • Michael Koziarski

    Michael Koziarski September 30th, 2008 @ 09:45 AM

    Yeah, he wrote the preloading code and is probably the best person to fix this.

  • Michael Koziarski

    Michael Koziarski September 30th, 2008 @ 09:45 AM

    but fundamentally, why are you doing that duplicate :including anyway :)

  • Will Bryant

    Will Bryant September 30th, 2008 @ 09:57 AM

    Righto, I'll drop Fred an email.

    I'm not doing it, but my clients are :).

    They wanted the association to always load the child association, hence the :include on the has_many declaration itself. But in one part of the app they wanted not just that child but a grandchild, so they had a two-deep :include on their find call, which hits this problem.

    (It also showed up in another place where I am guessing that originally people were putting in includes on the find calls, and later on someone else decided to make that global and put it on the association - but didn't remove it from the find calls.)

    It's something that worked just fine with Rails 2.0, and broke when they tried to upgrade to Rails 2.1, and it definitely isn't expected behaviour, so I'm hoping we can get the fix included... Will see what Fred thinks.

  • Michael Koziarski

    Michael Koziarski September 30th, 2008 @ 10:01 AM

    Yeah, a regression isn't cool.

    At the very least a way to work around this would be good to have documented.

  • Frederick Cheung

    Frederick Cheung September 30th, 2008 @ 10:24 AM

    I do have an account here :-).

    I've sort of run into this, albeit not in the context of an association with an :include option (I've been playing with the ability to add an :include at a later date, and doing that twice resulted in duplicate objects).

    This approach looks fine to me ( presumably you would also need to implement the same check for preload_has_and_belongs_to_many_association) but if there is one thing I've learnt with this stuff is that it is hard to test well, so I'd double check you aren't introducing regressions.

    One case I'd check is whether the case of a belongs_to where the foo_id is NULL and that sort of edge case.

  • Will Bryant

    Will Bryant October 6th, 2008 @ 11:13 PM

    Fred, no patch there for HABTMs as they don't work at all with includes defined on the association - the :select passed to find gets ignored due to the options[:include], so the the_parent_record_id gets wiped out. Since that's an unrelated bug it shouldn't block inclusion of this patch. (Does anyone use HABTMs now anyway?)

    I've added a test for the belongs_to case you suggest. There's quite a few regression tests elsewhere which all pass fine=

    I've updated the attached patch to apply to current master.

  • Frederick Cheung

    Frederick Cheung October 7th, 2008 @ 12:00 AM

    All yes, I'm getting ahead of myself. There is http://rails.lighthouseapp.com/p... which means that rails falls back to the join based include more often than it should and also my own patch

    http://rails.lighthouseapp.com/p... which allows :select to work with :include (as much as it can do) which both make fixing habtm in this way possible, but I guess that has to wait until those paches worm there way in.

    That aside, looks good to me?

  • Will Bryant

    Will Bryant October 7th, 2008 @ 01:56 AM

    If Fred's #1060 patch gets merged, please merge this as well as my patch above - it fixes this #1110 bug for HABTM also. (You can actually merge it safely even if the #1060 patch doesn't get applied, but the test case would fail due to the #1060 bug).

  • Michael Koziarski

    Michael Koziarski October 7th, 2008 @ 08:29 PM

    Hmm, I don't like #1060 for 2.2 just because it makes :select mean something different to what it currently does.

    So what should we do with this one?

  • Will Bryant

    Will Bryant October 7th, 2008 @ 10:51 PM

    OK, just merge the first patch then, and leave out the additional-...diff (or at least, leave out the test case in it).

  • Will Bryant

    Will Bryant October 9th, 2008 @ 10:37 AM

    • Tag changed from 2.1, activerecord, bug, eager_loading, edge, has_many, patch to 2.1, activerecord, bug, eager_loading, edge, has_many, patch

    Is that one looking OK?

  • Repository

    Repository October 12th, 2008 @ 05:42 PM

    • State changed from “new” to “committed”

    (from [4c05055487e149bfa4152c1b42f3519671ca22ac]) explicitly including child associations that are also included in the parent association definition should not result in double records in the collection/double loads (#1110)

    Signed-off-by: Michael Koziarski michael@koziarski.com [#1110 state:committed] http://github.com/rails/rails/co...

  • Jeff Kreeftmeijer

    Jeff Kreeftmeijer November 8th, 2010 @ 08:26 AM

    • Tag cleared.

    Automatic cleanup of spam.

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>

Referenced by

Pages