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 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 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 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 September 30th, 2008 @ 09:45 AM
Yeah, he wrote the preloading code and is probably the best person to fix this.
-
Michael Koziarski September 30th, 2008 @ 09:45 AM
but fundamentally, why are you doing that duplicate :including anyway :)
-
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 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 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 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 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?
-
-
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 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 October 9th, 2008 @ 10:37 AM
- → Tag changed from “"2.1" activerecord bug eager_loading edge has_many patch” to “2.1 activerecord bug bug eager_loading edge has_many patch”
Is that one looking OK?
-
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...
Please Login or create a free account to add a new comment.
You can update this ticket by sending an email to from your email client. (help)
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
Source available from github
The Git repository resides at http://github.com/rails
Check out the current development trunk (Edge Rails) with:
git clone git://github.com/rails/rails.git
Creating or reviewing a patch
See the contributor guide.
Creating a feature request
Please don't. If you want a new feature in Rails, you'll have to pull up your sleeves and get busy yourself. Or convince someone else to do it. See the contributor guide on how to get going. But posting them here is just going to lead to ticket root.
Creating a bug report
When creating a bug report, be sure to include as much relevant information as possible. Post the code sample that causes the problem. Preferably, alter the unit tests and show through either changed or added tests how the expected behavior is not occuring.
Security vulnerabilities should be reported via an email to security@rubyonrails.org, do not use trac for reporting security vulnerabilities. All content in trac is publicly available as soon as it is posted.
Then don't get your hopes up. Unless you have a "Code Red, Mission Critical, The World is Coming to an End" kinda bug, you're creating this ticket in the hope that others with the same problem will be able to collaborate with you on solving it. Do not expect that the ticket automatically will see any activity or that others will jump to fix it. Creating a ticket like this is mostly to help yourself start on the path of fixing the problem and for others to sign on to with a "I'm having this problem too".
