:include preloading bugs

rubyonrails-core
1

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... :slight_smile:

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

2

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... :slight_smile:

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:

Oops. I'd noticed that in a slightly different context (i've been
fiddling with stuff you can eager load extra associations after the
main load). Didn't occur to me that it could happen in this way though.

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

I'm not sure why that is ? (well with has_one you test for the
existance of the appropriate ivar rather than calling loaded? but
surely it's basically the same ?

Fred

3

It was just a bit messy as we need to test for the ivar, then call it’s loaded? - it seems like an unnecessary intrusion of implementation details of the association proxy classes. So I’ve introduced a loaded_#{my_association}? to try and keep it clean.

Incidentally, could someone fill me in on why the associations code does this:

association = instance_variable_get(ivar) if instance_variable_defined?(ivar)

instead of just using the ifness of instance_variable_get directly? #instance_variable_get doesn’t seem to define the requested instance variable as a side-effect, so why do we need to check instance_variable_defined? explicitly?

Anyway, I’ve written the patch for has_many, has_one, and belongs_to; HABTM has unrelated :include bugs, and meanwhile has_many with :through doesn’t seem to do the :includes anyway, so don’t need to do anything to them at the mo.

Could my patches for #1101 and #1110 please be reviewed?

Thanks,

Will