Merge pull request #26121 from MaxLap/fix_count_with_left_joins

Fix count which would sometimes force a DISTINCT

4 files changed, 28 insertions, 16 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index bf81c23036..6b996fd6bd 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   Doing count on relations that contain LEFT OUTER JOIN Arel node no longer
+    force a DISTINCT. This solves issues when using count after a left_joins.
+
+    *Maxime Handfield Lapointe*
+
 *   RecordNotFound raised by association.find exposes `id`, `primary_key` and
     `model` methods to be consistent with RecordNotFound raised by Record.find.
 
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index ecf3700aab..a4962879ab 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -117,7 +117,10 @@ module ActiveRecord
       end
 
       if has_include?(column_name)
-        construct_relation_for_association_calculations.calculate(operation, column_name)
+        relation = construct_relation_for_association_calculations
+        relation = relation.distinct if operation.to_s.downcase == "count"
+
+        relation.calculate(operation, column_name)
       else
         perform_calculation(operation, column_name)
       end
@@ -198,11 +201,6 @@ module ActiveRecord
 
         if operation == "count"
           column_name ||= select_for_count
-
-          unless arel.ast.grep(Arel::Nodes::OuterJoin).empty?
-            distinct = true
-          end
-
           column_name = primary_key if column_name == :all && distinct
           distinct = nil if column_name =~ /\s*DISTINCT[\s(]+/i
         end
diff --git a/activerecord/test/cases/associations/left_outer_join_association_test.rb b/activerecord/test/cases/associations/left_outer_join_association_test.rb
index d814229f64..2cc6468827 100644
--- a/activerecord/test/cases/associations/left_outer_join_association_test.rb
+++ b/activerecord/test/cases/associations/left_outer_join_association_test.rb
@@ -25,23 +25,32 @@ class LeftOuterJoinAssociationTest < ActiveRecord::TestCase
     end
   end
 
-  def test_construct_finder_sql_executes_a_left_outer_join
-    assert_not_equal Author.count, Author.joins(:posts).count
-    assert_equal Author.count, Author.left_outer_joins(:posts).count
+  def test_left_outer_joins_count_is_same_as_size_of_loaded_results
+    assert_equal 17, Post.left_outer_joins(:comments).to_a.size
+    assert_equal 17, Post.left_outer_joins(:comments).count
   end
 
-  def test_left_outer_join_by_left_joins
-    assert_not_equal Author.count, Author.joins(:posts).count
-    assert_equal Author.count, Author.left_joins(:posts).count
+  def test_left_joins_aliases_left_outer_joins
+    assert_equal Post.left_outer_joins(:comments).to_sql, Post.left_joins(:comments).to_sql
+  end
+
+  def test_left_outer_joins_return_has_value_for_every_comment
+    all_post_ids = Post.pluck(:id)
+    assert_equal all_post_ids, all_post_ids & Post.left_outer_joins(:comments).pluck(:id)
+  end
+
+  def test_left_outer_joins_actually_does_a_left_outer_join
+    queries = capture_sql { Author.left_outer_joins(:posts).to_a }
+    assert queries.any? { |sql| /LEFT OUTER JOIN/i.match?(sql) }
   end
 
   def test_construct_finder_sql_ignores_empty_left_outer_joins_hash
-    queries = capture_sql { Author.left_outer_joins({}) }
+    queries = capture_sql { Author.left_outer_joins({}).to_a }
     assert queries.none? { |sql| /LEFT OUTER JOIN/i.match?(sql) }
   end
 
   def test_construct_finder_sql_ignores_empty_left_outer_joins_array
-    queries = capture_sql { Author.left_outer_joins([]) }
+    queries = capture_sql { Author.left_outer_joins([]).to_a }
     assert queries.none? { |sql| /LEFT OUTER JOIN/i.match?(sql) }
   end
 
diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb
index 951b83e87b..103075d09c 100644
--- a/activerecord/test/cases/relation_test.rb
+++ b/activerecord/test/cases/relation_test.rb
@@ -223,7 +223,7 @@ module ActiveRecord
     def test_relation_merging_with_merged_joins_as_symbols
       special_comments_with_ratings = SpecialComment.joins(:ratings)
       posts_with_special_comments_with_ratings = Post.group("posts.id").joins(:special_comments).merge(special_comments_with_ratings)
-      assert_equal 3, authors(:david).posts.merge(posts_with_special_comments_with_ratings).count.length
+      assert_equal({ 2=>1, 4=>3, 5=>1 }, authors(:david).posts.merge(posts_with_special_comments_with_ratings).count)
     end
 
     def test_relation_merging_with_joins_as_join_dependency_pick_proper_parent
@@ -273,7 +273,7 @@ module ActiveRecord
       join_string = "LEFT OUTER JOIN #{Rating.quoted_table_name} ON #{SpecialComment.quoted_table_name}.id = #{Rating.quoted_table_name}.comment_id"
       special_comments_with_ratings = SpecialComment.joins join_string
       posts_with_special_comments_with_ratings = Post.group("posts.id").joins(:special_comments).merge(special_comments_with_ratings)
-      assert_equal 3, authors(:david).posts.merge(posts_with_special_comments_with_ratings).count.length
+      assert_equal({ 2=>1, 4=>3, 5=>1 }, authors(:david).posts.merge(posts_with_special_comments_with_ratings).count)
     end
 
     class EnsureRoundTripTypeCasting < ActiveRecord::Type::Value