Permalink
Browse files

Remove fall back and column restrictions for `count`.

  • Loading branch information...
1 parent db519c0 commit da9b5d4a8435b744fcf278fffd6d7f1e36d4a4f2 @senny senny committed May 13, 2013
View
@@ -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.
@@ -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')
@@ -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]

8 comments on commit da9b5d4

Will this be backported to 3.2-stable?

Member

senny replied Jun 12, 2013

I don't think so, it changes the behavior and as such should not go back to older releases.

Contributor

ernie replied Jul 14, 2013

This should (imho) be reverted before 4.0.1 is released. Calling join on select_values makes assumptions that the values in select_values are strings. This is not always the case, and will break if Arel nodes are used in the select.

If we must introduce the behavior, we should keep select_for_count in place so libraries have a place to hook in and prevent this sort of assumption from causing breakage. For more, see https://github.com/ernie/squeel/blob/master/lib/squeel/adapters/active_record/relation_extensions.rb#L76-L103.

Member

senny replied Jul 14, 2013

@ernie I see your point but I don't think the old functionality was reasonable behavior either. Let me bring select_for_count back but keep the current behavior. Is that ok?

Member

senny replied Jul 14, 2013

In the long term we should refactor the code to not make the same assumptions about select_values.

Member

senny replied Jul 14, 2013

@ernie would that work for you: senny/rails@f147092

Contributor

ernie replied Jul 14, 2013

Member

senny replied Jul 14, 2013

@ernie the method is back on master and 4-0-stable. Please ping me if you have the time to work on a PR to make the count code path not completely string focused.

Please sign in to comment.