Add Relation#construct_relation_for_association_calculations for calculations with includes

4 files changed, 17 insertions, 42 deletions

diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb
index 8a44dc7df1..fc6cb793ab 100644
--- a/activerecord/lib/active_record/calculations.rb
+++ b/activerecord/lib/active_record/calculations.rb
@@ -2,8 +2,6 @@ module ActiveRecord
   module Calculations #:nodoc:
     extend ActiveSupport::Concern
 
-    CALCULATIONS_OPTIONS = [:conditions, :joins, :order, :select, :group, :having, :distinct, :limit, :offset, :include, :from]
-
     module ClassMethods
       # Count operates using three different approaches.
       #
@@ -147,26 +145,11 @@ module ActiveRecord
       end
 
       private
-        def validate_calculation_options(options = {})
-          options.assert_valid_keys(CALCULATIONS_OPTIONS)
-        end
-
-        def construct_calculation_arel(options = {})
-          validate_calculation_options(options)
-          options = options.except(:distinct)
-
-          merge_with_includes = current_scoped_methods ? current_scoped_methods.includes_values : []
-          includes = (merge_with_includes + Array.wrap(options[:include])).uniq
 
-          if includes.any?
-            merge_with_joins = current_scoped_methods ? current_scoped_methods.joins_values : []
-            joins = (merge_with_joins + Array.wrap(options[:joins])).uniq
-            join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(self, includes, construct_join(joins))
-            construct_finder_arel_with_included_associations(options, join_dependency)
-          else
-            scoped.apply_finder_options(options)
-          end
-        end
+      def construct_calculation_arel(options = {})
+        relation = scoped.apply_finder_options(options.except(:distinct))
+        (relation.eager_loading? || relation.includes_values.present?) ? relation.send(:construct_relation_for_association_calculations) : relation
+      end
 
     end
   end
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index e37e692a97..19f91f4278 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -45,12 +45,10 @@ module ActiveRecord
     def to_a
       return @records if loaded?
 
-      eager_loading = @eager_load_values.any? || (@includes_values.any? && references_eager_loaded_tables?)
-
-      @records = eager_loading ? find_with_associations : @klass.find_by_sql(arel.to_sql)
+      @records = eager_loading? ? find_with_associations : @klass.find_by_sql(arel.to_sql)
 
       preload = @preload_values
-      preload +=  @includes_values unless eager_loading
+      preload +=  @includes_values unless eager_loading?
       preload.each {|associations| @klass.send(:preload_associations, @records, associations) } 
 
       # @readonly_value is true only if set explicity. @implicit_readonly is true if there are JOINS and no explicit SELECT.
@@ -112,6 +110,7 @@ module ActiveRecord
 
     def reset
       @first = @last = @to_sql = @order_clause = @scope_for_create = @arel = @loaded = nil
+      @should_eager_load = @join_dependency = nil
       @records = []
       self
     end
@@ -133,6 +132,10 @@ module ActiveRecord
       end
     end
 
+    def eager_loading?
+      @should_eager_load ||= (@eager_load_values.any? || (@includes_values.any? && references_eager_loaded_tables?))
+    end
+
     protected
 
     def method_missing(method, *args, &block)
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index 980c5796f3..c48c8fe828 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -53,6 +53,12 @@ module ActiveRecord
       []
     end
 
+    def construct_relation_for_association_calculations
+      including = (@eager_load_values + @includes_values).uniq
+      join_dependency = ActiveRecord::Associations::ClassMethods::JoinDependency.new(@klass, including, arel.joins(arel))
+      construct_relation_for_association_find(join_dependency)
+    end
+
     def construct_relation_for_association_find(join_dependency)
       relation = except(:includes, :eager_load, :preload, :select).select(@klass.send(:column_aliases, join_dependency))
 
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index bd2d471fc7..c3b2e56387 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -246,23 +246,6 @@ class CalculationsTest < ActiveRecord::TestCase
     assert_equal 8, c['Jadedpixel']
   end
 
-  def test_should_reject_invalid_options
-    assert_nothing_raised do
-      # empty options are valid
-      Company.send(:validate_calculation_options)
-      # these options are valid for all calculations
-      [:select, :conditions, :joins, :order, :group, :having, :distinct].each do |opt|
-        Company.send(:validate_calculation_options, opt => true)
-      end
-
-      # :include is only valid on :count
-      Company.send(:validate_calculation_options, :include => true)
-    end
-
-    assert_raise(ArgumentError) { Company.send(:validate_calculation_options, :sum,   :foo => :bar) }
-    assert_raise(ArgumentError) { Company.send(:validate_calculation_options, :count, :foo => :bar) }
-  end
-
   def test_should_count_selected_field_with_include
     assert_equal 6, Account.count(:distinct => true, :include => :firm)
     assert_equal 4, Account.count(:distinct => true, :include => :firm, :select => :credit_limit)