Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• You flagged this item as spam.
  

  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?

• You flagged this item as spam.
  

  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 :)

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

  • fix_association_preload_double_include.diff 7.2 KB
• 

  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?

• You flagged this item as spam.
  

  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).

  • additional_habtm_patch__requires_1060_first.diff 2.9 KB
• 

  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?

• You flagged this item as spam.
  

  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).

• You flagged this item as spam.
  

  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 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 November 8th, 2010 @ 08:26 AM

  • Tag cleared.

  Automatic cleanup of spam.