This project is archived and is in readonly mode.

#1723 ✓resolved
Joseph Palermo

Eager load of has_one :through with conditions on the :through table fails

Reported by Joseph Palermo | January 9th, 2009 @ 08:22 PM | in 3.x

The first problem is that has_one :through eager loading doesn't pass through :conditions or :order like a has_many :through does. But once you pass those down, you have the problem that the through association is preloaded separately from the base association, so at that point the conditions throw a SQL error because the table can't be found.

has_many :through has the same problem.

The patch has a test that shows the broken functionality.

Comments and changes to this ticket

  • Pratik

    Pratik March 10th, 2009 @ 12:19 PM

    • Assigned user set to “Frederick Cheung”
  • Jonathan Monahan

    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?

  • Joseph Palermo

    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

    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.

  • Jonathan Monahan

    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

    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)

  • Jonathan Monahan

    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

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

    • Milestone changed from 2.x to 3.x
  • Jon Leighton

    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

    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.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Pages