Merge pull request #512 from pivotalneutron/fix_eager_load_with_calculations

Bug fixes for calculations with includes

3 files changed, 18 insertions, 6 deletions

diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index a8a52867ce..0fcae92d51 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -146,7 +146,7 @@ module ActiveRecord
       if options.except(:distinct).present?
         apply_finder_options(options.except(:distinct)).calculate(operation, column_name, :distinct => options[:distinct])
       else
-        if eager_loading? || includes_values.present?
+        if eager_loading? || (includes_values.present? && references_eager_loaded_tables?)
           construct_relation_for_association_calculations.calculate(operation, column_name, options)
         else
           perform_calculation(operation, column_name, options)
@@ -161,21 +161,20 @@ module ActiveRecord
     def perform_calculation(operation, column_name, options = {})
       operation = operation.to_s.downcase
 
-      distinct = nil
+      distinct = options[:distinct]
 
       if operation == "count"
         column_name ||= (select_for_count || :all)
 
         unless arel.ast.grep(Arel::Nodes::OuterJoin).empty?
           distinct = true
-          column_name = primary_key if column_name == :all
         end
 
+        column_name = primary_key if column_name == :all && distinct
+
         distinct = nil if column_name =~ /\s*DISTINCT\s+/i
       end
 
-      distinct = options[:distinct] || distinct
-
       if @group_values.any?
         execute_grouped_calculation(operation, column_name, distinct)
       else
diff --git a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb
index 39e8a7960a..49d8722aff 100644
--- a/activerecord/test/cases/associations/cascaded_eager_loading_test.rb
+++ b/activerecord/test/cases/associations/cascaded_eager_loading_test.rb
@@ -51,7 +51,9 @@ class CascadedEagerLoadingTest < ActiveRecord::TestCase
     categories = Category.joins(:categorizations).includes([{:posts=>:comments}, :authors])
 
     assert_nothing_raised do
-      assert_equal 3, categories.count
+      assert_equal 4, categories.count
+      assert_equal 4, categories.all.count
+      assert_equal 3, categories.count(:distinct => true)
       assert_equal 3, categories.all.uniq.size # Must uniq since instantiating with inner joins will get dupes
     end
   end
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index 654c4c9010..56f6d795b6 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -319,6 +319,17 @@ class CalculationsTest < ActiveRecord::TestCase
     assert_equal 4, Account.count(:distinct => true, :include => :firm, :select => :credit_limit)
   end
 
+  def test_should_not_perform_joined_include_by_default
+    assert_equal Account.count, Account.includes(:firm).count
+    queries = assert_sql { Account.includes(:firm).count }
+    assert_no_match(/join/i, queries.last)
+  end
+
+  def test_should_perform_joined_include_when_referencing_included_tables
+    joined_count = Account.includes(:firm).where(:companies => {:name => '37signals'}).count
+    assert_equal 1, joined_count
+  end
+
   def test_should_count_scoped_select
     Account.update_all("credit_limit = NULL")
     assert_equal 0, Account.scoped(:select => "credit_limit").count