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
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---
On 25 Sep 2008, at 12:51, will.bryant wrote:> > 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: >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> Thoughts? > > Cheers, > Will > > >--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Fri, Sep 26, 2008 at 12:38 AM, Frederick Cheung < frederick.cheung@gmail.com> wrote:> > > > 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 ? >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 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---