:include preloading bugs

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

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

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