[PATCH] Eager loading has_many association that provides :class_name of STI superclass doesn't filter for subclasses

Is it weird that has_many( :items, ... etc without :class_name ) doesn't make sure that it loads only Item (and subclasses)?

In addition to the above classes, I also have a Comment class that extends Content (I have my reasons, namely that they use the same "containership" method). The has_many :items also returns Comments erroneously (because the SQL isn't generated to filter the left side of the join if I leave out :class_name => 'Item' on the :items association).

I know the docs say that with a :through association that the "options for :class_name and :foreign_key are ignored," but they're obviously not since it kinda performs as expected when you specify the class_name (just doesn't in the eager loading).

Since you're doing something non-standard (specifying class_name) it really would be best if you attach the full set of model classes (and schema if you're doing something odd there). I don't know where that "contents.type = 'Photo'" in the where clause came from, unless it's in that omitted code. Please include all the stuff and I'll take a look.

I really would like to not specify class_name and believe I shouldn't have to since my model is Item and the association is :items. But if I don't, then Rails doesn't add the type = ? to the ON clause of the outer join.

I'm really doing something common: STI with a many-to-many join table.

The relationship is Content has_many :containerships which belongs to a Content as :parent and a Content as :child. Very similar to a STI nested tree.

contents.type = 'Photo' comes about because Photo subclasses Item. A full set of models would be helpful. I'll have to recreate a simplified set that demonstrates the issue.

create_table :comments do |t|
      t.column :commented_item_id, :integer
      t.column :content, :text
    end
    create_table :containers do |t|
      t.column :content, :text
    end
    create_table :items do |t|
      t.column :type, :string, :null => false
      t.column :container_id, :integer, :null => false
    end


class CommentedItem < Item
  has_many :comments
end
class Comment < ActiveRecord::Base
  belongs_to :commented_item
end
class Container < ActiveRecord::Base
  has_many :commented_items
  has_many :comments,
    :through => :commented_items
end
class Item < ActiveRecord::Base
  belongs_to :container
end
class SubCommentedItem < CommentedItem
end

Fixtures:
>> y Container.find(:all)
--- 
- !ruby/object:Container 
  attributes: 
    id: "1"
    content: asd

>> y Item.find(:all)
--- 
- !ruby/object:CommentedItem 
  attributes: 
    container_id: "1"
    type: CommentedItem
    id: "1"
- !ruby/object:SubCommentedItem 
  attributes: 
    container_id: "1"
    type: SubCommentedItem
    id: "2"

>> y Comment.find(:all)
--- 
- !ruby/object:Comment 
  attributes: 
    id: "1"
    content: asd2
    commented_item_id: "1"
- !ruby/object:Comment 
  attributes: 
    id: "2"
    content: asd3
    commented_item_id: "2"

# Description:
# Two comments, one attached to a CommentedItem, another to a SubCommentedItem.
# Both the CommentedItem and SubCommentedItem are attached to one Container.
# Accessing the comments through the container has_many :commented_items association
# fails to find the comment attached through the SubCommentedItem even though
# the SubCommentedItem is found through that :commented_items association.

The problem is demonstrated by (notice that only one comment is found):

>> SubCommentedItem # make sure the class is loaded
>> Container.find(1).comments
=> [#<Comment:0xb74cb820 @attributes={"id"=>"1", "content"=>"asd2", "commented_item_id"=>"1"}]

The other comment is missing because of the SQL which is missing the type condition for SubCommentedItem:

  Comment Load (0.001116)   SELECT comments.* FROM comments INNER JOIN items ON comments.commented_item_id = items.id WHERE ((items.container_id = 1) AND ((items.type = 'CommentedItem'))) 

I realize this is a quite contrived example but it is the simplest I could find that demonstrates the behaviour.

The problem is quite simple, ActiveRecord::Base has "type_condition" method for generating conditions for STI, if you call CommentedItem.find(:all) that method generates: (items."type" = 'CommentedItem' OR items."type" = 'SubCommentedItem' ).

But has_many :through does not use "type_condition" for generating conditions, instead it does the following in it's "conditions" method (in associations/has_many_association.rb):

("#{@reflection.through_reflection.table_name}.#{@reflection.through_reflection.klass.inheritance_column} = #{@reflection.klass.quote_value(@reflection.through_reflection.klass.name.demodulize)}" unless @reflection.through_reflection.klass.descends_from_active_record?)

A solution that seems to be to use this (kind of ugly) line instead:

(@reflection.through_reflection.klass.send(:type_condition) unless @reflection.through_reflection.klass.descends_from_active_record?)

I'll attach a patch.

I'm not entirely sure that the problem in this ticket is caused by this limitation as part of the code seems to be missing, but what I described certainly is quite likely a related problem.

prefix the summary with [PATCH] so TRAC can manage this ticket properly.

I just ran into what I think the real bug reported by orangechicken is, and wrote a patch for it, should I create a new ticket for the other patch or is it ok to keep it here as it's a bit related?

btw. the first patch does not include a test, but it does pass all rails tests, I'll see if I can write a test case for the first one. But for the second patch it was easy to write a test.

As for the first patch the second also happens because while activerecord has the "type_condition" method for generating conditions that filter for not only the parent but also the STI subclasses it is not used so if you :include an association which is against an STI parent class you will only get records that are instances of that parent class not any of it's children.

In the second patch I had to add the ability to specify a table name to the "type_condition" method because eager loading sometimes aliases the table name.

I've updated the first patch which now includes a test. Not sure if all the changes are according to Rails standards but the meaning should be clear.