Fix count which would sometimes force a DISTINCT

The current behaviour of checking if there is a LEFT OUTER JOIN arel
node to detect if we are doing eager_loading is wrong. This problem
wasn't frequent before as only some pretty specific cases would add
a LEFT OUTER JOIN arel node. However, the recent new feature
left_outer_joins also add this node and made this problem happen
frequently.

Since in the perform_calculation function, we don't have access to
eager_loading information, I had to extract the logic for the distinct
out to the calculate method.

As I was in the file for left_outer_join tests, I fixed a few that had
bugs and I replaced some that were really weak with something that
will catch more issues.

In relation tests, the first test I changed would have failed if it
had validated the hash returned by count instead of just checking how
many pairs were in it. This is because this merge of join currently
transforms the join node into an outer join node, which then made
count do a distinct. So before this change, the return was
{1=>1, 4=>1, 5=>1}.

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.
 

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

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
 

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