Hi guys,
There's two regressions in the 2.1.x series (including current edge) blocking one of my clients from upgrading from 2.0.
The first one (lighthouse #1101, includes fix) is pretty easy, whenever you have an association with :conditions => {:some => 'hash'}, it'll work fine if you use the association without eager loading, it'll work fine if you use the old eager loading system, but assuming you fall onto the new preloading path as usual, the conditions get sanitized against the parent table rather than the child and so the fully-qualified column names in the SQL have the wrong table name.
The patch for that's really trivial (diffs for edge on the ticket - it's also really simple to backport to 2.1, I've attached it for frozen-in 2.1 rails on the ticket as well for ppl's convenience, let me know if anyone would like it against rails git for 2.1).
The second bug (lighthouse #1110, testcase but no fix) seems much messier to fix. I was having weird problems with records showing up twice in some of the application's associations when loaded. It turned out that you can actually reproduce the problem relatively simply, if you have say a has_many which is defined with some :includes on it (Client has_many :projects, :include => :goals), and you later do a find that does the same include (Client.find(:all, :include => {:project => {:goals => :tasks}}) or even just :include => {:projects => :goals} the same as in the association itself), then the associations get loaded twice - and you get two of every object in the target array.
Suddenly, all our hours and total invoice prices double...
I need some help fixing this one. Merging duplicate :includes in AssociationPreload's preload_associations works for the above example (and the test case on the ticket) but not the general problem, so it looks to me like checking to see if we've already loaded the association as we go is probably a better approach. Doing it that way for say has_many is pretty easy, you could even just:
--- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -129,6 +129,7 @@ module ActiveRecord end
def preload_has_many_association(records, reflection, preload_options={}) + return if records.first.send(reflection.name).loaded? id_to_record_map, ids = construct_id_map(records) records.each {|record| record.send(reflection.name).loaded} options = reflection.options
But fixing it for has_many :throughs and has_ones is harder.
Thoughts?
Cheers, Will