Skip to content

Commit 682d7c7

Browse files
committed
Revert "Merge pull request rails#10710 from senny/5554_let_the_database_raise_on_counts"
This reverts commit b8e2978. Conflicts: activerecord/CHANGELOG.md activerecord/lib/active_record/relation/calculations.rb Reason: This change is not backward compatible and it was reverted before 4.0.0 at 2ad168e so we can't include in 4-0-stable. Closes rails#12417
1 parent fbf5ece commit 682d7c7

File tree

4 files changed

+6
-31
lines changed

4 files changed

+6
-31
lines changed

activerecord/CHANGELOG.md

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -196,22 +196,6 @@
196196

197197
*Yves Senn*
198198

199-
* Remove column restrictions for `count`, let the database raise if the SQL is
200-
invalid. The previos behavior was untested and surprising for the user.
201-
202-
Fixes #5554.
203-
204-
Example:
205-
206-
User.select("name, username").count
207-
# Before => SELECT count(*) FROM users
208-
# After => ActiveRecord::StatementInvalid
209-
210-
# you can still use `count(:all)` to perform a query unrelated to the
211-
# selected columns
212-
User.select("name, username").count(:all) # => SELECT count(*) FROM users
213-
214-
*Yves Senn*
215199

216200
* Fix the `:primary_key` option for `has_many` associations.
217201

activerecord/lib/active_record/relation.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ def size
234234
def empty?
235235
return @records.empty? if loaded?
236236

237-
c = count(:all)
237+
c = count
238238
c.respond_to?(:zero?) ? c.zero? : c.empty?
239239
end
240240

activerecord/lib/active_record/relation/calculations.rb

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -207,14 +207,15 @@ def perform_calculation(operation, column_name, options = {})
207207
end
208208

209209
if operation == "count"
210-
column_name ||= select_for_count
210+
column_name ||= (select_for_count || :all)
211211

212212
unless arel.ast.grep(Arel::Nodes::OuterJoin).empty?
213213
distinct = true
214214
end
215215

216216
column_name = primary_key if column_name == :all && distinct
217-
distinct = nil if column_name =~ /\s*DISTINCT[\s(]+/i
217+
218+
distinct = nil if column_name =~ /\s*DISTINCT\s+/i
218219
end
219220

220221
if group_values.any?
@@ -378,9 +379,8 @@ def type_cast_using_column(value, column)
378379
# TODO: refactor to allow non-string `select_values` (eg. Arel nodes).
379380
def select_for_count
380381
if select_values.present?
381-
select_values.join(", ")
382-
else
383-
:all
382+
select = select_values.join(", ")
383+
select if select !~ /[,*]/
384384
end
385385
end
386386

activerecord/test/cases/calculations_test.rb

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -162,15 +162,6 @@ def test_no_limit_no_offset
162162
assert_no_match(/OFFSET/, queries.first)
163163
end
164164

165-
def test_count_on_invalid_columns_raises
166-
e = assert_raises(ActiveRecord::StatementInvalid) {
167-
Account.select("credit_limit, firm_name").count
168-
}
169-
170-
assert_match "accounts", e.message
171-
assert_match "credit_limit, firm_name", e.message
172-
end
173-
174165
def test_should_group_by_summed_field_having_condition
175166
c = Account.group(:firm_id).having('sum(credit_limit) > 50').sum(:credit_limit)
176167
assert_nil c[1]

0 commit comments

Comments
 (0)