Merge pull request #10710 from senny/5554_let_the_database_raise_on_counts

Remove column restrictions for `#count`, let the database raise if the SQL is invalid.

4 files changed, 32 insertions, 11 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 98033e904e..170084cf67 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,19 @@
+*   Remove column restrictions for `count`, let the database raise if the SQL is
+    invalid. The previos behavior was untested and surprising for the user.
+    Fixes #5554.
+
+    Example:
+
+        User.select("name, username").count
+        # Before => SELECT count(*) FROM users
+        # After => ActiveRecord::StatementInvalid
+
+        # you can still use `count(:all)` to perform a query unrelated to the
+        # selected columns
+        User.select("name, username").count(:all) # => SELECT count(*) FROM users
+
+    *Yves Senn*
+
 *   Rails now automatically detects inverse associations. If you do not set the
     `:inverse_of` option on the association, then Active Record will guess the
     inverse association based on heuristics.
diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index d54479edbb..d37471e9ad 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -247,7 +247,7 @@ module ActiveRecord
     def empty?
       return @records.empty? if loaded?
 
-      c = count
+      c = count(:all)
       c.respond_to?(:zero?) ? c.zero? : c.empty?
     end
 
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index ccb48247b7..4becf3980d 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -207,15 +207,18 @@ module ActiveRecord
       end
 
       if operation == "count"
-        column_name ||= (select_for_count || :all)
+        if select_values.present?
+          column_name ||= select_values.join(", ")
+        else
+          column_name ||= :all
+        end
 
         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
+        distinct = nil if column_name =~ /\s*DISTINCT[\s(]+/i
       end
 
       if group_values.any?
@@ -376,13 +379,6 @@ module ActiveRecord
       column ? column.type_cast(value) : value
     end
 
-    def select_for_count
-      if select_values.present?
-        select = select_values.join(", ")
-        select if select !~ /[,*]/
-      end
-    end
-
     def build_count_subquery(relation, column_name, distinct)
       column_alias = Arel.sql('count_column')
       subquery_alias = Arel.sql('subquery_for_count')
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index c8ac77984f..0f3f9aecfc 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -167,6 +167,15 @@ class CalculationsTest < ActiveRecord::TestCase
     assert_no_match(/OFFSET/, queries.first)
   end
 
+  def test_count_on_invalid_columns_raises
+    e = assert_raises(ActiveRecord::StatementInvalid) {
+      Account.select("credit_limit, firm_name").count
+    }
+
+    assert_match "accounts", e.message
+    assert_match "credit_limit, firm_name", e.message
+  end
+
   def test_should_group_by_summed_field_having_condition
     c = Account.group(:firm_id).having('sum(credit_limit) > 50').sum(:credit_limit)
     assert_nil        c[1]