Use association's :conditions when eager loading. [jeremyevans0@gmail.com] closes #4144

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@3897 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

13 files changed, 93 insertions, 18 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index e0803611cb..b02626f4aa 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Use association's :conditions when eager loading. [jeremyevans0@gmail.com] #4144
+
 * Alias the has_and_belongs_to_many join table on eager includes. #4106 [jeremyevans0@gmail.com]
 
   This statement would normally error because the projects_developers table is joined twice, and therefore joined_on would be ambiguous.
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 072e35aadd..34fba0414c 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -1157,7 +1157,7 @@ module ActiveRecord
 
           class JoinBase
             attr_reader :active_record
-            delegate    :table_name, :column_names, :primary_key, :reflections, :to=>:active_record
+            delegate    :table_name, :column_names, :primary_key, :reflections, :to => :active_record
 
             def initialize(active_record)
               @active_record = active_record
@@ -1227,19 +1227,19 @@ module ActiveRecord
             def association_join
               join = case reflection.macro
                 when :has_and_belongs_to_many
-                  join_table_name    =
                   " LEFT OUTER JOIN %s %s ON %s.%s = %s.%s " % [
                      options[:join_table], aliased_join_table_name, aliased_join_table_name,
                      options[:foreign_key] || reflection.active_record.to_s.classify.foreign_key,
                      reflection.active_record.table_name, reflection.active_record.primary_key] +
                   " LEFT OUTER JOIN %s %s ON %s.%s = %s.%s " % [
                      table_name, aliased_table_name, aliased_table_name, klass.primary_key,
-                     options[:join_table], options[:association_foreign_key] || klass.table_name.classify.foreign_key
+                     aliased_join_table_name, options[:association_foreign_key] || klass.table_name.classify.foreign_key
                      ]
                 when :has_many, :has_one
                   case
                     when reflection.macro == :has_many && reflection.options[:through]
-                      through_reflection       = parent.active_record.reflect_on_association(reflection.options[:through])
+                      through_reflection = parent.active_record.reflect_on_association(reflection.options[:through])
+                      through_conditions = through_reflection.options[:conditions] ? "AND #{eval("%(#{through_reflection.options[:conditions]})")}" : ''
                       if through_reflection.options[:as] # has_many :through against a polymorphic join
                         polymorphic_foreign_key  = through_reflection.options[:as].to_s + '_id'
                         polymorphic_foreign_type = through_reflection.options[:as].to_s + '_type'
@@ -1284,11 +1284,12 @@ module ActiveRecord
                     ]
                 else
                   ""
-              end
+              end || ''
               join << %(AND %s.%s = %s ) % [
                 aliased_table_name, 
                 reflection.active_record.connection.quote_column_name(reflection.active_record.inheritance_column), 
-                klass.quote(klass.name)] unless join.blank? || klass.descends_from_active_record?
+                klass.quote(klass.name)] unless klass.descends_from_active_record?
+              join << "AND #{eval("%(#{reflection.options[:conditions]})")} " if reflection.options[:conditions]
               join
             end
           end
diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb
index d340cc30e3..4ddeb84a58 100644
--- a/activerecord/lib/active_record/associations/association_proxy.rb
+++ b/activerecord/lib/active_record/associations/association_proxy.rb
@@ -14,14 +14,23 @@ module ActiveRecord
       def respond_to?(symbol, include_priv = false)
         proxy_respond_to?(symbol, include_priv) || (load_target && @target.respond_to?(symbol, include_priv))
       end
-
+      
       # Explicitly proxy === because the instance method removal above
       # doesn't catch it.
       def ===(other)
         load_target
         other === @target
       end
-
+      
+      def aliased_table_name
+        @reflection.klass.table_name
+      end
+      
+      def conditions
+        @conditions ||= eval("%(#{@reflection.options[:conditions]})") if @reflection.options[:conditions]
+      end
+      alias :sql_conditions :conditions
+      
       def reset
         @target = nil
         @loaded = false
diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
index 8955c0dc31..1752678cbd 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -43,7 +43,7 @@ module ActiveRecord
         def find_target
           @reflection.klass.find(
             @owner[@reflection.primary_key_name], 
-            :conditions => @reflection.options[:conditions] ? interpolate_sql(@reflection.options[:conditions]) : nil,
+            :conditions => conditions,
             :include    => @reflection.options[:include]
           )
         end
diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
index 6d0704db74..9549b959fc 100644
--- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
@@ -30,7 +30,7 @@ module ActiveRecord
           if @reflection.options[:conditions]
             association_class.find(
               @owner[@reflection.primary_key_name], 
-              :conditions => interpolate_sql(@reflection.options[:conditions]),
+              :conditions => conditions,
               :include    => @reflection.options[:include]
             )
           else
diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
index 5b203dcc57..25c23c63b7 100644
--- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -144,7 +144,11 @@ module ActiveRecord
             @owner.connection.execute(sql)
           end
         end
-
+        
+        #def aliased_join_table_name
+        #  @reflection.options[:join_table]
+        #end
+        
         def construct_sql
           interpolate_sql_options!(@reflection.options, :finder_sql)
 
@@ -152,7 +156,7 @@ module ActiveRecord
             @finder_sql = @reflection.options[:finder_sql]
           else
             @finder_sql = "#{@reflection.options[:join_table]}.#{@reflection.primary_key_name} = #{@owner.quoted_id} "
-            @finder_sql << " AND (#{interpolate_sql(@reflection.options[:conditions])})" if @reflection.options[:conditions]
+            @finder_sql << " AND (#{conditions})" if conditions
           end
 
           @join_sql = "JOIN #{@reflection.options[:join_table]} ON #{@reflection.klass.table_name}.#{@reflection.klass.primary_key} = #{@reflection.options[:join_table]}.#{@reflection.association_foreign_key}"
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 8b471859c1..8d82de0d01 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -3,7 +3,6 @@ module ActiveRecord
     class HasManyAssociation < AssociationCollection #:nodoc:
       def initialize(owner, reflection)
         super
-        @conditions = sanitize_sql(reflection.options[:conditions])
         construct_sql
       end
 
@@ -169,11 +168,11 @@ module ActiveRecord
               @finder_sql = 
                 "#{@reflection.klass.table_name}.#{@reflection.options[:as]}_id = #{@owner.quoted_id} AND " + 
                 "#{@reflection.klass.table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote @owner.class.base_class.name.to_s}"
-              @finder_sql << " AND (#{interpolate_sql(@conditions)})" if @conditions
+              @finder_sql << " AND (#{conditions})" if conditions
             
             else
               @finder_sql = "#{@reflection.klass.table_name}.#{@reflection.primary_key_name} = #{@owner.quoted_id}"
-              @finder_sql << " AND (#{interpolate_sql(@conditions)})" if @conditions
+              @finder_sql << " AND (#{conditions})" if conditions
           end
 
           if @reflection.options[:counter_sql]
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index 6db5eae0bc..009ac7c53a 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -74,7 +74,7 @@ module ActiveRecord
               "AND #{through_reflection.table_name}.#{through_reflection.primary_key_name} = #{@owner.quoted_id}"
           end
           
-          conditions << " AND (#{interpolate_sql(sanitize_sql(@reflection.options[:conditions]))})" if @reflection.options[:conditions]
+          conditions << " AND (#{sql_conditions})" if sql_conditions
           
           return conditions
         end
@@ -100,7 +100,7 @@ module ActiveRecord
               @finder_sql = interpolate_sql(@reflection.options[:finder_sql])
 
               @finder_sql = "#{@reflection.klass.table_name}.#{@reflection.primary_key_name} = #{@owner.quoted_id}"
-              @finder_sql << " AND (#{interpolate_sql(@conditions)})" if @conditions
+              @finder_sql << " AND (#{conditions})" if conditions
           end
 
           if @reflection.options[:counter_sql]
diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index e0d6f64b27..e881d49420 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -73,7 +73,7 @@ module ActiveRecord
             else
               @finder_sql = "#{@reflection.table_name}.#{@reflection.primary_key_name} = #{@owner.quoted_id}"
           end
-          @finder_sql << " AND (#{sanitize_sql(@reflection.options[:conditions])})" if @reflection.options[:conditions]
+          @finder_sql << " AND (#{conditions})" if conditions
         end
     end
   end
diff --git a/activerecord/test/associations_go_eager_test.rb b/activerecord/test/associations_go_eager_test.rb
index 50cb9bec59..9eb1b96baf 100644
--- a/activerecord/test/associations_go_eager_test.rb
+++ b/activerecord/test/associations_go_eager_test.rb
@@ -224,7 +224,57 @@ class EagerAssociationTest < Test::Unit::TestCase
       post = Post.find(6, :include=>[ :monkeys, :elephants ])
     }
   end
+  
+  def find_all_ordered(className, include=nil)
+    className.find(:all, :order=>"#{className.table_name}.#{className.primary_key}", :include=>include)
+  end
 
+  def test_eager_with_multiple_associations_with_same_table_has_many_and_habtm
+    # Eager includes of has many and habtm associations aren't necessarily sorted in the same way
+    def assert_equal_after_sort(item1, item2, item3 = nil)
+      assert_equal(item1.sort{|a,b| a.id <=> b.id}, item2.sort{|a,b| a.id <=> b.id})
+      assert_equal(item3.sort{|a,b| a.id <=> b.id}, item2.sort{|a,b| a.id <=> b.id}) if item3
+    end
+    # Test regular association, association with conditions, association with
+    # STI, and association with conditions assured not to be true
+    post_types = [:posts, :hello_posts, :special_posts, :nonexistent_posts]
+    # test both has_many and has_and_belongs_to_many
+    [Author, Category].each do |className|
+      d1 = find_all_ordered(className)
+      # test including all post types at once
+      d2 = find_all_ordered(className, post_types) 
+      d1.each_index do |i| 
+        assert_equal(d1[i], d2[i])
+        assert_equal_after_sort(d1[i].posts, d2[i].posts)
+        post_types[1..-1].each do |post_type|
+          # test including post_types together
+          d3 = find_all_ordered(className, [:posts, post_type])
+          assert_equal(d1[i], d3[i])
+          assert_equal_after_sort(d1[i].posts, d3[i].posts)
+          assert_equal_after_sort(d1[i].send(post_type), d2[i].send(post_type), d3[i].send(post_type))
+        end
+      end
+    end
+  end
+  
+  def test_eager_with_multiple_associations_with_same_table_has_one
+    d1 = find_all_ordered(Firm)
+    d2 = find_all_ordered(Firm, :account)
+    d1.each_index do |i| 
+      assert_equal(d1[i], d2[i])
+      assert_equal(d1[i].account, d2[i].account)
+    end
+  end
+  
+  def test_eager_with_multiple_associations_with_same_table_belongs_to
+    firm_types = [:firm, :firm_with_basic_id, :firm_with_other_name, :firm_with_condition]
+    d1 = find_all_ordered(Client)
+    d2 = find_all_ordered(Client, firm_types)
+    d1.each_index do |i| 
+      assert_equal(d1[i], d2[i])
+      firm_types.each { |type| assert_equal(d1[i].send(type), d2[i].send(type)) }
+    end
+  end
   def test_eager_with_valid_association_as_string_not_symbol
     assert_nothing_raised { Post.find(:all, :include => 'comments') }
   end
diff --git a/activerecord/test/fixtures/author.rb b/activerecord/test/fixtures/author.rb
index 3138dfca57..8c6cee51e4 100644
--- a/activerecord/test/fixtures/author.rb
+++ b/activerecord/test/fixtures/author.rb
@@ -4,6 +4,9 @@ class Author < ActiveRecord::Base
   has_many :posts_with_categories, :include => :categories, :class_name => "Post"
   has_many :posts_with_comments_and_categories, :include => [ :comments, :categories ], :order => "posts.id", :class_name => "Post"
 
+  has_many :special_posts, :class_name => "Post"
+  has_many :hello_posts, :class_name => "Post", :conditions=>"\#{aliased_table_name}.body = 'hello'"
+  has_many :nonexistent_posts, :class_name => "Post", :conditions=>"\#{aliased_table_name}.body = 'nonexistent'"
   has_many :posts_with_callbacks, :class_name => "Post", :before_add => :log_before_adding,
            :after_add => :log_after_adding, :before_remove => :log_before_removing,
            :after_remove => :log_after_removing
diff --git a/activerecord/test/fixtures/categories_posts.yml b/activerecord/test/fixtures/categories_posts.yml
index 0d77d8c05f..9b67ab4fa4 100644
--- a/activerecord/test/fixtures/categories_posts.yml
+++ b/activerecord/test/fixtures/categories_posts.yml
@@ -17,3 +17,7 @@ general_sti_habtm:
 sti_test_sti_habtm:
   category_id: 3
   post_id: 6
+
+general_hello:
+  category_id: 1
+  post_id: 4
diff --git a/activerecord/test/fixtures/category.rb b/activerecord/test/fixtures/category.rb
index 9be459a80f..6917c51d34 100644
--- a/activerecord/test/fixtures/category.rb
+++ b/activerecord/test/fixtures/category.rb
@@ -1,5 +1,8 @@
 class Category < ActiveRecord::Base
   has_and_belongs_to_many :posts
+  has_and_belongs_to_many :special_posts, :class_name => "Post"
+  has_and_belongs_to_many :hello_posts, :class_name => "Post", :conditions => "\#{aliased_table_name}.body = 'hello'"
+  has_and_belongs_to_many :nonexistent_posts, :class_name => "Post", :conditions=>"\#{aliased_table_name}.body = 'nonexistent'"
   
   def self.what_are_you
     'a category...'