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

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.

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

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

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

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.

Yeah, a regression isn't cool.

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

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.

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.

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?

If Fred's #1060 Allow :select option with join-based include patch gets merged, please merge this as well as my patch above - it fixes this #1110 Eager loading regression for find with :includes of associations already :inc... bug for HABTM also. (You can actually merge it safely even if the #1060 Allow :select option with join-based include patch doesn't get applied, but the test case would fail due to the #1060 Allow :select option with join-based include bug).

Hmm, I don't like #1060 Allow :select option with join-based include for 2.2 just because it makes :select mean something different to what it currently does.

So what should we do with this one?

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