From fa2c61fc636f958c274692f2a0f3062859797790 Mon Sep 17 00:00:00 2001
From: jvillarejo <arzivian87@gmail.com>
Date: Thu, 21 Feb 2019 17:55:21 -0300
Subject: fixes different `count` calculation when using `size` manual `select`
 with DISTINCT

When using `select` with `'DISTINCT( ... )'` if you use method `size` on a non loaded relation it overrides the column selected by passing `:all` so it returns different value than count.

This fixes #35214
---
 activerecord/CHANGELOG.md                              |  6 ++++++
 .../lib/active_record/relation/calculations.rb         | 11 ++++++++---
 activerecord/test/cases/calculations_test.rb           | 18 ++++++++++++++++++
 activerecord/test/cases/relations_test.rb              |  7 +++++++
 4 files changed, 39 insertions(+), 3 deletions(-)

(limited to 'activerecord')

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 77cb167e7c..c8fa387b42 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Fix different `count` calculation when using `size` with manual `select` with DISTINCT.
+    
+    Fixes #35214.
+
+    *Juani Villarejo*
+
 ## Rails 6.0.0.beta2 (February 25, 2019) ##
 
 *   Fix prepared statements caching to be enabled even when query caching is enabled.
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index bdd3c540bb..4f9ddf302e 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -221,7 +221,6 @@ module ActiveRecord
     end
 
     private
-
       def has_include?(column_name)
         eager_loading? || (includes_values.present? && column_name && column_name != :all)
       end
@@ -236,10 +235,12 @@ module ActiveRecord
         if operation == "count"
           column_name ||= select_for_count
           if column_name == :all
-            if distinct && (group_values.any? || select_values.empty? && order_values.empty?)
+            if !distinct
+              distinct = distinct_select?(select_for_count) if group_values.empty?
+            elsif group_values.any? || select_values.empty? && order_values.empty?
               column_name = primary_key
             end
-          elsif column_name.is_a?(::String) && /\bDISTINCT[\s(]/i.match?(column_name)
+          elsif distinct_select?(column_name)
             distinct = nil
           end
         end
@@ -251,6 +252,10 @@ module ActiveRecord
         end
       end
 
+      def distinct_select?(column_name)
+        column_name.is_a?(::String) && /\bDISTINCT[\s(]/i.match?(column_name)
+      end
+
       def aggregate_column(column_name)
         return column_name if Arel::Expressions === column_name
 
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index 8ac2d55218..840920ccb6 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -473,6 +473,24 @@ class CalculationsTest < ActiveRecord::TestCase
     assert_equal 6, Account.select("DISTINCT accounts.id").includes(:firm).count
   end
 
+  def test_should_count_manual_select_with_count_all
+    assert_equal 5, Account.select("DISTINCT accounts.firm_id").count(:all)
+  end
+
+  def test_should_count_with_manual_distinct_select_and_distinct
+    assert_equal 4, Account.select("DISTINCT accounts.firm_id").distinct(:id).count
+  end
+
+  def test_should_count_manual_select_with_group_with_count_all
+    expected = { nil => 1, 1 => 1, 2 => 1, 6 => 2, 9 => 1 }
+    actual = Account.select("DISTINCT accounts.firm_id").group("accounts.firm_id").count(:all)
+    assert_equal expected, actual
+  end
+
+  def test_should_count_manual_with_count_all
+    assert_equal 6, Account.count(:all)
+  end
+
   def test_count_selected_arel_attribute
     assert_equal 5, Account.select(Account.arel_table[:firm_id]).count
     assert_equal 4, Account.distinct.select(Account.arel_table[:firm_id]).count
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 2de0a81c99..5a3bd1bd4a 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -972,6 +972,13 @@ class RelationTest < ActiveRecord::TestCase
     assert_queries(1) { assert_equal 11, posts.load.size }
   end
 
+  def test_size_with_eager_loading_and_manual_distinct_select_and_custom_order
+    accounts = Account.select("DISTINCT accounts.firm_id").order("accounts.firm_id")
+
+    assert_queries(1) { assert_equal 5, accounts.size }
+    assert_queries(1) { assert_equal 5, accounts.load.size }
+  end
+
   def test_count_explicit_columns
     Post.update_all(comments_count: nil)
     posts = Post.all
-- 
cgit v1.2.3