Skip to content

Commit

Permalink
Stop assuming strings for grouped calculations
Browse files Browse the repository at this point in the history
Execute_grouped_calculation is one of those places where
ActiveRecord forgets that it has ARel underpinnings, and
assumes that the values provided to group_values are
strings. This artificially hobbles otherwise functional
code. This patch stops assuming that incoming values
respond to to_sym, stops using string interpolation for
table aliases on objects that support aliasing, and stops
unnecessarily joining group_values on the relation.

Additionally, it calls to_sql, if available, on objects
sent to column_alias_for, in order to get a more reasonable
alias string than a non-string's default to_str method.
  • Loading branch information
ernie committed Jun 24, 2012
1 parent ebe8a45 commit a1c05dd
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 6 deletions.
23 changes: 17 additions & 6 deletions activerecord/lib/active_record/relation/calculations.rb
Expand Up @@ -262,10 +262,16 @@ def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
end

def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
group_attr = group_values
association = @klass.reflect_on_association(group_attr.first.to_sym)
associated = group_attr.size == 1 && association && association.macro == :belongs_to # only count belongs_to associations
group_fields = Array(associated ? association.foreign_key : group_attr)
group_attrs = group_values

if group_attrs.first.respond_to?(:to_sym)
association = @klass.reflect_on_association(group_attrs.first.to_sym)
associated = group_attrs.size == 1 && association && association.macro == :belongs_to # only count belongs_to associations
group_fields = Array(associated ? association.foreign_key : group_attrs)
else
group_fields = group_attrs
end

group_aliases = group_fields.map { |field| column_alias_for(field) }
group_columns = group_aliases.zip(group_fields).map { |aliaz,field|
[aliaz, column_for(field)]
Expand All @@ -288,10 +294,14 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
select_values += select_values unless having_values.empty?

select_values.concat group_fields.zip(group_aliases).map { |field,aliaz|
"#{field} AS #{aliaz}"
if field.respond_to?(:as)
field.as(aliaz)
else
"#{field} AS #{aliaz}"
end
}

relation = except(:group).group(group.join(','))
relation = except(:group).group(group)
relation.select_values = select_values

calculated_data = @klass.connection.select_all(relation, nil, bind_values)
Expand Down Expand Up @@ -321,6 +331,7 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
# column_alias_for("count(*)") # => "count_all"
# column_alias_for("count", "id") # => "count_id"
def column_alias_for(*keys)
keys.map! {|k| k.respond_to?(:to_sql) ? k.to_sql : k}
table_name = keys.join(' ')
table_name.downcase!
table_name.gsub!(/\*/, 'all')
Expand Down
5 changes: 5 additions & 0 deletions activerecord/test/cases/calculations_test.rb
Expand Up @@ -61,6 +61,11 @@ def test_should_group_by_field
[1,6,2].each { |firm_id| assert c.keys.include?(firm_id) }
end

def test_should_group_by_arel_attribute
c = Account.sum(:credit_limit, :group => Account.arel_table[:firm_id])
[1,6,2].each { |firm_id| assert c.keys.include?(firm_id) }
end

def test_should_group_by_multiple_fields
c = Account.group('firm_id', :credit_limit).count(:all)
[ [nil, 50], [1, 50], [6, 50], [6, 55], [9, 53], [2, 60] ].each { |firm_and_limit| assert c.keys.include?(firm_and_limit) }
Expand Down

0 comments on commit a1c05dd

Please sign in to comment.