Rails Ruby on Rails → 3.x

Comments and changes to this ticket

• 

  Pratik March 10th, 2009 @ 12:19 PM

  • Assigned user set to “Frederick Cheung”

• You flagged this item as spam.
  

  Jonathan Monahan March 26th, 2009 @ 10:27 AM

  • Tag changed from “:include, eager_loading, has_one_through” to “:include, :through_conditions, eager_loading, has_one_through”

  Is this possibly related to the fact that there is no way to apply conditions to the :through association from the source association?

  I have monkey-patched this on 2.2.2 by adding support for :through_conditions, i.e. conditions that are applied to the join table join rather than to the target table join. Does that make sense?

• You flagged this item as spam.
  

  Joseph Palermo March 26th, 2009 @ 04:22 PM

  No, because the two associations are loaded completely separately in the eager loading code, there is no way to apply the conditions on the :through association. I think the only way to fix it is to rewrite it so it loads the two associations in a single query like it does everywhere else.

• 

  Frederick Cheung March 26th, 2009 @ 04:39 PM

  I've been wanting for it to be that way (and for has many through) for a long long time.

  I think a precondition is some refactoring some of the condition, join etc. generation for hmt/hot so that it is shared between eager loading and 'normal' loading (you could duplicate most of it but that wouldn't be very nice.

  When I last looked at this the bit where it got non obvious was when you had

  a has_many :bars a has_many :foos, :through => bars, :include => :yet_another_association

  and the include of yet_another_association required falling back to the old join code

  (because some preload stuff requires you to be able to add to the select clause, but the old include overwrites select, and we decided against allowing old include to support a limited form of select that would have sufficed here)

  This may have gotten better because one change that went into 2.3 reduced the number of occasions we need to fall back to the old code because we now scan the joins clause. Unfortunately I don't really have time to investigate right now.

• You flagged this item as spam.
  

  Jonathan Monahan March 26th, 2009 @ 04:58 PM

  Here is some of what I monkey-patched into association_join() in associations.rb:

  
                        " #{join_type} %s ON (%s.%s = %s.%s%s%s%s%s%s) " % [
                          table_alias_for(through_reflection.klass.table_name, aliased_join_table_name),
                          connection.quote_table_name(parent.aliased_table_name),
                          connection.quote_column_name(parent.primary_key),
                          connection.quote_table_name(aliased_join_table_name),
                          connection.quote_column_name(jt_foreign_key),
                          jt_as_extra, jt_source_extra, jt_sti_extra,
                          through_reflection ? reflection_conditions(through_reflection.options[:conditions], aliased_join_table_name, through_reflection.klass) : '',
                          reflection ? reflection_conditions(reflection.options[:through_conditions], aliased_join_table_name, through_reflection.klass) : ''
                        ] +
                        " #{join_type} %s ON (%s.%s = %s.%s%s) " % [
                          table_name_and_alias,
                          connection.quote_table_name(aliased_table_name),
                          connection.quote_column_name(first_key),
                          connection.quote_table_name(aliased_join_table_name),
                          connection.quote_column_name(second_key),
                          as_extra
                        ]
  

  which puts the through reflections conditions on the right join, as well as putting my new :through_conditions from the outer association on there too.

  Also, at the bottom of join_association(), I modified the code that attaches the conditions to not include the conditions from the :through reflection (having included them directly in the join above:

  
  # Only include reflection conditions here, not the through_reflection conditions
  # as they have already been rolled into the join clause above.
  # Also, use appropriate aliasing and handle Hash-style conditions.
  #   Previous lines:
  #             [through_reflection, reflection].each do |ref|
  #               join << "AND #{interpolate_sql(sanitize_sql(ref.options[:conditions]))} " if ref && ref.options[:conditions]
  #             end
                [reflection].each do |ref|
                  join << reflection_conditions(ref.options[:conditions]) if ref
                end
  

  And, finally, to handle table aliasing correctly, and to support hash conditions, I abstracted the reflection condition handling:

  
                # Generates reflection condition clause with appropriate table aliasing.
                # By default, it uses aliased_table_name on reflection condition Hashes to avoid
                # errors such as:
                # Mysql::Error: Unknown column 'table_name.column_name' in 'on clause'.
                def reflection_conditions(conditions, reflection_aliased_table_name = aliased_table_name, reflection_klass = active_record)
                  if conditions
                    if conditions.is_a?(Hash)
                      return " AND #{interpolate_sql(reflection_klass.send(:sanitize_sql_hash_for_conditions, conditions, reflection_aliased_table_name))} "
                    else
                      return " AND #{interpolate_sql(reflection_klass.send(:sanitize_sql, conditions))} "
                    end
                  else
                    return ""
                  end
                end
  

  (Obviously there's also the minor changes to allow :through_conditions as an attribute on has_one and has_many).

  I see that 2.3 has added support for hash conditions in associations, but it does not cope with aliasing when you join to the same table several times, which I think I've fixed above by passing the alias to sanitize_sql_hash_for_conditions().

• 

  Frederick Cheung March 26th, 2009 @ 06:39 PM

  I think we may be talking at cross purposes I (and I think Joseph) are talking about the other :include mechanism (in association_preload.rb)

• You flagged this item as spam.
  

  Jonathan Monahan March 26th, 2009 @ 11:29 PM

  Ah, OK. In that case, do my changes make any sense to you? Do they relate to another ticket that already exists perhaps? If not, should I raise a new ticket for it?

• 

  Jeremy Kemper May 4th, 2010 @ 06:48 PM

  • Milestone changed from “2.x” to “3.x”

  [bulk edit]

• 

  Jon Leighton December 12th, 2010 @ 01:46 PM

  • Assigned user changed from “Frederick Cheung” to “Aaron Patterson”
  • Importance changed from “” to “”

  This is no longer an issue because eager loading now has two different behaviours (JOIN or a separate query) depending on whether conditions are present.

  I've applied the attached test and verified that it does in fact pass on current master.

  So this ticket can be closed. Thanks.

• 

  Aaron Patterson December 15th, 2010 @ 07:24 PM

  • State changed from “new” to “resolved”

  I've added the test to master so we don't break anything in the future. Thanks for Checking this Jon.