Summary

When an association with conditions is chained together with additional conditions using Relations, ActiveRecord and Arel shifts the association's conditions from the WHERE clause to the ON clause. But in doing so it drops the foreign key relationship and improperly quotes conditions. Quick example:

   Post.includes(:special_tags).where(:user_id => 10).all

will result in the following SQL (using sqlite3):

  SELECT "posts"."id" as t0_r0, "posts"."title" AS t0_r0, "user_id" AS t0_r1, "tags"."id" AS t1_r0, "tags"."post_id" AS t1_r1, "tags"."name" AS t1_r2, "tags"."context_type" AS t1_r3
  FROM "posts" 
  LEFT OUTER JOIN "taggings" ON "posts"."id" = "taggings"."post_id" 
  LEFT OUTER JOIN "tags" ON "tags"."name" = ''special''
  WHERE "posts"."id" = 10

There are two issues with the second LEFT OUTER JOIN statement. First its missing the foreign key relationship condition "taggings"."tag_id" = "tags"."id". Second it's improperly double quoting (two single quotes on each side) the word "special" with "tags"."name" = ''special''

Recreating the issue

This is a modified version of our code at work where we originally discovered the issue.

Schema:

create_table "recipes", :force => true do |t|
  t.string   "name"    
end
create_table "taggings", :force => true do |t|
  t.integer  "taggable_id"
  t.string   "taggable_type"
  t.integer  "position"
  t.integer  "tag_id"
  t.datetime "created_at"
  t.datetime "updated_at"
end
create_table "tags", :force => true do |t|
  t.string   "name"
  t.string   "context_type"
  t.datetime "created_at"
  t.datetime "updated_at"
end

models:

class Recipe < ActiveRecord::Base
  has_many :taggings, :as => :taggable, :dependent => :destroy
  has_many :courses, :through => :taggings, :source => :tag, :conditions => {:tags => {:context_type =>  'course'}}
  scope :ordered_by_course, includes(:courses).where(:taggings => {:position => 0}).order("UPPER(tags.name) ASC").order("UPPER(recipes.name) ASC")
end

class Tagging < ActiveRecord::Base
  belongs_to :taggable, :polymorphic => true
  belongs_to :tag

  before_create do
    self.position = self.taggable.send(self.tag.context_type.to_s.pluralize).count + 1
  end
end

class Tag < ActiveRecord::Base
  has_many :taggings, :dependent => :destroy

  class << self
    ["course", "main_ingredient", "cuisine", "occasion", "convenience", "cooking_method"].each do |context|
      define_method context.pluralize do
        where("tags.context_type = '#{context}'")
      end
    end
  end

end

Create some dummy data with IRB:

recipe = Recipe.create!(:name => "foo")
tag = Tag.create!(:name => "Main Dish", :context_type => "course")
recipe.courses << tag

Now using IRB and do a normal load of the association:

recipe = Recipe.first
recipe.courses # => [#<Tag id: 1, name: "Main Dish", context_type: "course", created_at: "2010-11-19 17:04:14", updated_at: "2010-11-19 17:04:14">]

SQL in the log:

 SELECT "tags".* FROM "tags" INNER JOIN "taggings" ON "tags".id = "taggings".tag_id WHERE (("taggings".taggable_id = 1) AND ("taggings".taggable_type = 'Recipe') AND (("tags"."context_type" = 'course')))

Back to IRB now lets try the scope:

recipes = Recipe.ordered_by_course
recipes.first.courses # => []

That's wrong check the SQL in the log:

SELECT "recipes"."id" AS t0_r0, "recipes"."name" AS t0_r1,"tags"."id" AS t1_r0, "tags"."name" AS t1_r1, "tags"."context_type" AS t1_r2, "tags"."created_at" AS t1_r3, "tags"."updated_at" AS t1_r4
FROM "recipes" 
LEFT OUTER JOIN "taggings" ON "recipes"."id" = "taggings"."taggable_id" AND "taggings"."taggable_type" = 'Recipe' 
LEFT OUTER JOIN "tags" ON '"tags"."context_type" = ''course'' 
WHERE (recipes.deleted_at IS NULL) AND ("taggings"."position" = 1) 
ORDER BY UPPER(tags.name) ASC, UPPER(recipes.name) ASC

Wow this is a bit of an issue.

The Two Nasty Issues

Dirty Nasty Issue 1 - Where did the foreign keys go?

The first problem is within ActiveRecord::QueryMethods#build_joins and ActiveRecord::Associations::ClassMethods::JoinDependency::JoinAssociation#association_join. ActiveRecord::QueryMethods#build_joins is responsible for building the joins for the query (lib/active_record/relation/query_methods.rb:215). Check out this snippet here at line 235-243

to_join = []
join_dependency.join_associations.each do |association|
  if (association_relation = association.relation).is_a?(Array)
    to_join << [association_relation.first, association.join_type, association.association_join.first]
    to_join << [association_relation.last, association.join_type, association.association_join.last]
  else
    to_join << [association_relation, association.join_type, association.association_join]
  end
end

Essentially if the association relation is an array (array of ARel Nodes) we are dealing some type of has_many :through relationship. And this snippet assumes that there are only two items in the array (the first should be all the joins for the recipes to taggings and the secound should be for the taggings to tags).

But now take a look at ActiveRecord::Associations::ClassMethods::JoinDependency::JoinAssociation#association_join (in the rails 3-0-stable branch that would be activerecord/lib/active_record/associations.rb:2135). The first part of the function (lines 2136-2207) will set @join to be an array of two length:

def association_join
    return @join if @join

    aliased_table = Arel::Table.new(table_name, :as      => @aliased_table_name,
                                                :engine  => arel_engine,
                                                :columns => klass.columns)

    parent_table = Arel::Table.new(parent.table_name, :as      => parent.aliased_table_name,
                                                      :engine  => arel_engine,
                                                      :columns => parent.active_record.columns)

    @join = case reflection.macro
    when :has_and_belongs_to_many
      join_table = Arel::Table.new(options[:join_table], :as => aliased_join_table_name, :engine => arel_engine)
      fk = options[:foreign_key] || reflection.active_record.to_s.foreign_key
      klass_fk = options[:association_foreign_key] || klass.to_s.foreign_key

      [
        join_table[fk].eq(parent_table[reflection.active_record.primary_key]),
        aliased_table[klass.primary_key].eq(join_table[klass_fk])
      ]
    when :has_many, :has_one
      if reflection.options[:through]
        join_table = Arel::Table.new(through_reflection.klass.table_name, :as => aliased_join_table_name, :engine => arel_engine)
        jt_foreign_key = jt_as_extra = jt_source_extra = jt_sti_extra = nil
        first_key = second_key = as_extra = nil

        if through_reflection.options[:as] # has_many :through against a polymorphic join
          jt_foreign_key = through_reflection.options[:as].to_s + '_id'
          jt_as_extra = join_table[through_reflection.options[:as].to_s + '_type'].eq(parent.active_record.base_class.name)
        else
          jt_foreign_key = through_reflection.primary_key_name
        end

        case source_reflection.macro
        when :has_many
          if source_reflection.options[:as]
            first_key   = "#{source_reflection.options[:as]}_id"
            second_key  = options[:foreign_key] || primary_key
            as_extra    = aliased_table["#{source_reflection.options[:as]}_type"].eq(source_reflection.active_record.base_class.name)
          else
            first_key   = through_reflection.klass.base_class.to_s.foreign_key
            second_key  = options[:foreign_key] || primary_key
          end

          unless through_reflection.klass.descends_from_active_record?
            jt_sti_extra = join_table[through_reflection.active_record.inheritance_column].eq(through_reflection.klass.sti_name)
          end
        when :belongs_to
          first_key = primary_key
          if reflection.options[:source_type]
            second_key = source_reflection.association_foreign_key
            jt_source_extra = join_table[reflection.source_reflection.options[:foreign_type]].eq(reflection.options[:source_type])
          else
            second_key = source_reflection.primary_key_name
          end
        end

        [
          [parent_table[parent.primary_key].eq(join_table[jt_foreign_key]), jt_as_extra, jt_source_extra, jt_sti_extra].reject{|x| x.blank? },
          aliased_table[first_key].eq(join_table[second_key])
        ]
      elsif reflection.options[:as]
        id_rel = aliased_table["#{reflection.options[:as]}_id"].eq(parent_table[parent.primary_key])
        type_rel = aliased_table["#{reflection.options[:as]}_type"].eq(parent.active_record.base_class.name)
        [id_rel, type_rel]
      else
        foreign_key = options[:foreign_key] || reflection.active_record.name.foreign_key
        [aliased_table[foreign_key].eq(parent_table[reflection.options[:primary_key] || parent.primary_key])]
      end
    when :belongs_to
      [aliased_table[options[:primary_key] || reflection.klass.primary_key].eq(parent_table[options[:foreign_key] || reflection.primary_key_name])]
    end

Awesome. But the rest of the function now breaks the assumption that its only going to return that it will return an array of length 2 (lines 2209-2225):

  unless klass.descends_from_active_record?
    sti_column = aliased_table[klass.inheritance_column]
    sti_condition = sti_column.eq(klass.sti_name)
    klass.descendants.each {|subclass| sti_condition = sti_condition.or(sti_column.eq(subclass.sti_name)) }

    @join << sti_condition
  end


  [through_reflection, reflection].each do |ref|
    if ref && ref.options[:conditions]
      @join << interpolate_sql(sanitize_sql(ref.options[:conditions], aliased_table_name))
    end
  end

  @join
end

If the reflection has conditions or if the underlying klass is an STI you now have 3 or more items in your array. In the example I am using we would have three:
1. the Arel AST nodes for the recipes to taggings table
2. the Arel AST nodes for the foreign key of taggings to tags
3. the condition clauses checking the context type of the tag.

Now look back at ActiveRecord::QueryMethods#build_joins (lib/active_record/relation/query_methods.rb:235-243).

to_join = []
join_dependency.join_associations.each do |association|
  if (association_relation = association.relation).is_a?(Array)
    to_join << [association_relation.first, association.join_type, association.association_join.first]
    to_join << [association_relation.last, association.join_type, association.association_join.last]
  else
    to_join << [association_relation, association.join_type, association.association_join]
  end
end

Yup its totally going to drop the middle condition. So we need to patch ActiveRecord::Associations::ClassMethods::JoinDependency::JoinAssociation#association_join to only return an array of length two (since its grouping the joins for each relationship in an array). I'm just not sure what else this is going to break.

Dirty Nasty Issue 2 - What's with the double single quotes?

Recall that the generated SQL had this little snippet LEFT OUTER JOIN "tags" ON '"tags"."context_type" = ''course''. Why is it doing this? Well the problem turns out to be ActiveRecord::Associations::ClassMethods::JoinDependency::JoinAssociation#association_join again. Re-examine this snippet (activerecord/lib/active_record/associations.rb:218-223)

[through_reflection, reflection].each do |ref|
  if ref && ref.options[:conditions]
     @join << interpolate_sql(sanitize_sql(ref.options[:conditions], aliased_table_name))
  end
end

The #sanitize_sql (which is delegated to the active_record) returns a sql string. That is its not passing ARel AST nodes but actual SQL. That might not seem like much of an issue until you realize that ARel will treat this a string value, NOT a sql expression. We have to look at the ARel code to see what happens (I am using the 2.0.6 gem as reference). Let's look at how ARel's ToSql visitor handles the ON node (lib/arel/visitors/to_sql.rb:190-192):

def visit_Arel_Nodes_On o
  "ON #{visit o.expr}"
end

Now remember that ActiveRecord::Associations::ClassMethods::JoinDependency::JoinAssociation#association_join set one the condition here to be a string. That means o.expr # => "\"tags\".\"content_type\" = 'course'". Well look what happens next (lib/arel/visitors/to_sql.rb:278):

def visit_String o; quote(o, @last_column) end

It's going to quote the string as if its a String value to be passed into the database, not the actual sql to be run (lib/arel/visitors/to_sql.rb:294-296)

def quote value, column = nil
  @connection.quote value, column
end

And sure enough when we trace this down in activerecord we find (lib/active_record/connection_adapters/abstract/quoting.rb:40-42)

def quote_string(s)
  s.gsub(/\\/, '\&\&').gsub(/'/, "''") # ' (for ruby-mode)
end

Wow. At least we know where the issue is. The solution seems to be to patch ActiveRecord::Associations::ClassMethods::JoinDependency::JoinAssociation#association_join so that it does not just give a string, but actual ARel ast nodes. But its going to require some work.

Patches

I am currently working on the patches for this issue, but it may take me a while to test. Not to mention that I currently do not have MySQL or PostgreSQL installed on my mac and I've already spent way too much time tracking this down. But I will give it my best shot and hopefully whoever can check as needed. I'll post patches as soon as I have them. I am working off the Rails 3-0-stable branch.

Side Note

I checked out 'master' of activerecord, and its gone through a major rewrite. I am not sure if the rewrite addressed this issue, but once I complete my patches for the 3.0 line I can port the tests over to double check