Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Merge pull request #10710 from senny/5554_let_the_database_raise_on_c…

…ounts

Remove column restrictions for `#count`, let the database raise if the SQL is invalid.
  • Loading branch information...
commit f5e133e830940731b74c4e12118eab90054d32ec 2 parents ae6e6d9 + da9b5d4
@jonleighton jonleighton authored
View
16 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.
View
2  activerecord/lib/active_record/relation.rb
@@ -247,7 +247,7 @@ def size
def empty?
return @records.empty? if loaded?
- c = count
+ c = count(:all)
c.respond_to?(:zero?) ? c.zero? : c.empty?
end
View
16 activerecord/lib/active_record/relation/calculations.rb
@@ -207,15 +207,18 @@ def perform_calculation(operation, column_name, options = {})
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 @@ def type_cast_using_column(value, column)
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')
View
9 activerecord/test/cases/calculations_test.rb
@@ -167,6 +167,15 @@ def test_no_limit_no_offset
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]
Please sign in to comment.
Something went wrong with that request. Please try again.