More work on removing plain SQL from associations and use ARel instead.

3 files changed, 43 insertions, 35 deletions

diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 60da632b3b..7c445f5618 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -1662,40 +1662,39 @@ module ActiveRecord
         def construct_finder_sql_with_included_associations(options, join_dependency)
           scope = scope(:find)
 
+          relation = arel_table((scope && scope[:from]) || options[:from])
+
           joins = join_dependency.join_associations.collect{|join| join.association_join }.join
           joins << construct_join(options[:joins], scope)
+          relation.join(joins)
 
-          conditions = construct_conditions(options[:conditions], scope) || ''
-          conditions << construct_limited_ids_condition(conditions, options, join_dependency) if !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit])
-
-          relation = arel_table((scope && scope[:from]) || options[:from]).
-            join(joins).
-            where(conditions).
-            project(column_aliases(join_dependency)).
-            group(construct_group(options[:group], options[:having], scope)).
-            order(construct_order(options[:order], scope)
-          )
+          relation.where(construct_conditions(options[:conditions], scope))
+          relation.where(construct_arel_limited_ids_condition(options, join_dependency)) if join_dependency && !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit])
 
-          relation = relation.take(construct_limit(options[:limit], scope)) if using_limitable_reflections?(join_dependency.reflections)
+          relation.project(column_aliases(join_dependency))
+          relation.group(construct_group(options[:group], options[:having], scope))
+          relation.order(construct_order(options[:order], scope))
+          relation.take(construct_limit(options[:limit], scope)) if using_limitable_reflections?(join_dependency.reflections)
 
-          return sanitize_sql(relation.to_sql)
+          sanitize_sql(relation.to_sql)
         end
 
-         def construct_limited_ids_condition(where, options, join_dependency)
-          unless (id_list = select_limited_ids_list(options, join_dependency)).empty?
-            "#{where.blank? ? '' : ' AND '} #{connection.quote_table_name table_name}.#{primary_key} IN (#{id_list}) "
-          else
+        def construct_arel_limited_ids_condition(options, join_dependency)
+          if (ids_array = select_limited_ids_array(options, join_dependency)).empty?
             throw :invalid_query
+          else
+            Arel::In.new(
+              Arel::SqlLiteral.new("#{connection.quote_table_name table_name}.#{primary_key}"),
+              ids_array
+            )
           end
         end
 
-        def select_limited_ids_list(options, join_dependency)
-          pk = columns_hash[primary_key]
-
+        def select_limited_ids_array(options, join_dependency)
           connection.select_all(
             construct_finder_sql_for_association_limiting(options, join_dependency),
             "#{name} Load IDs For Limited Eager Loading"
-          ).collect { |row| connection.quote(row[primary_key], pk) }.join(", ")
+          ).collect { |row| row[primary_key] }
         end
 
         def construct_finder_sql_for_association_limiting(options, join_dependency)
diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb
index 5c7247b9c0..3c75200f82 100644
--- a/activerecord/lib/active_record/calculations.rb
+++ b/activerecord/lib/active_record/calculations.rb
@@ -148,19 +148,27 @@ module ActiveRecord
         end
 
         catch :invalid_query do
-          conditions = construct_conditions(options[:conditions], scope)
-          conditions << construct_limited_ids_condition(conditions, options, join_dependency) if join_dependency && !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit])
+          relation = arel_table((scope && scope[:from]) || options[:from])
+
+          relation.join(joins)
+
+          relation.where(construct_conditions(options[:conditions], scope))
+          relation.where(construct_arel_limited_ids_condition(options, join_dependency)) if join_dependency && !using_limitable_reflections?(join_dependency.reflections) && ((scope && scope[:limit]) || options[:limit])
+
+          relation.order(construct_order(options[:order], scope))
+          relation.take(options[:limit])
+          relation.skip(options[:offset])
 
           if options[:group]
-            return execute_grouped_calculation(operation, column_name, options.merge(:conditions => conditions, :joins => joins, :distinct => distinct))
+            return execute_grouped_calculation(operation, column_name, options, relation)
           else
-            return execute_simple_calculation(operation, column_name, options.merge(:conditions => conditions, :joins => joins, :distinct => distinct))
+            return execute_simple_calculation(operation, column_name, options.merge(:distinct => distinct), relation)
           end
         end
         0
       end
 
-      def execute_simple_calculation(operation, column_name, options) #:nodoc:
+      def execute_simple_calculation(operation, column_name, options, relation) #:nodoc:
         column = if column_names.include?(column_name.to_s)
           Arel::Attribute.new(arel_table(options[:from] || table_name),
                               options[:select] || column_name)
@@ -169,14 +177,12 @@ module ActiveRecord
                                (column_name == :all ? "*" : column_name.to_s))
         end
 
-        value = construct_finder_sql(options.merge(
-          :select => operation == 'count' ? column.count(options[:distinct]) : column.send(operation)
-        ), nil)
+        relation.project(operation == 'count' ? column.count(options[:distinct]) : column.send(operation))
 
-        type_cast_calculated_value(connection.select_value(value), column_for(column_name), operation)
+        type_cast_calculated_value(connection.select_value(relation.to_sql), column_for(column_name), operation)
       end
 
-      def execute_grouped_calculation(operation, column_name, options) #:nodoc:
+      def execute_grouped_calculation(operation, column_name, options, relation) #:nodoc:
         group_attr      = options[:group].to_s
         association     = reflect_on_association(group_attr.to_sym)
         associated      = association && association.macro == :belongs_to # only count belongs_to associations
@@ -194,7 +200,10 @@ module ActiveRecord
 
         options[:select] <<  ", #{group_field} AS #{group_alias}"
 
-        calculated_data = connection.select_all(construct_finder_sql(options, nil))
+        relation.project(options[:select])
+        relation.group(construct_group(options[:group], options[:having], nil))
+
+        calculated_data = connection.select_all(relation.to_sql)
 
         if association
           key_ids     = calculated_data.collect { |row| row[group_alias] }
diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
index 14b96caaae..9c915ab010 100644
--- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
@@ -730,7 +730,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
     assert_equal [projects(:active_record), projects(:action_controller)].map(&:id).sort, developer.project_ids.sort
   end
 
-  def test_select_limited_ids_list
+  def test_select_limited_ids_array
     # Set timestamps
     Developer.transaction do
       Developer.find(:all, :order => 'id').each_with_index do |record, i|
@@ -740,9 +740,9 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
 
     join_base = ActiveRecord::Associations::ClassMethods::JoinDependency::JoinBase.new(Project)
     join_dep  = ActiveRecord::Associations::ClassMethods::JoinDependency.new(join_base, :developers, nil)
-    projects  = Project.send(:select_limited_ids_list, {:order => 'developers.created_at'}, join_dep)
+    projects  = Project.send(:select_limited_ids_array, {:order => 'developers.created_at'}, join_dep)
     assert !projects.include?("'"), projects
-    assert_equal %w(1 2), projects.scan(/\d/).sort
+    assert_equal ["1", "2"], projects.sort
   end
 
   def test_scoped_find_on_through_association_doesnt_return_read_only_records
@@ -768,7 +768,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase
     assert_equal developer, project.developers.find(:first)
     assert_equal project, developer.projects.find(:first)
   end
-  
+
   def test_self_referential_habtm_without_foreign_key_set_should_raise_exception
     assert_raise(ActiveRecord::HasAndBelongsToManyAssociationForeignKeyNeeded) {
       Member.class_eval do