Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix unintuitive behavior with multiple order and group clauses
[#4545 state:committed]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
  • Loading branch information
Ernie Miller authored and jeremy committed May 6, 2010
1 parent 6d7f279 commit 902861a
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 8 deletions.
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/relation/calculations.rb
Expand Up @@ -195,7 +195,7 @@ def execute_grouped_calculation(operation, column_name) #:nodoc:

select_statement << ", #{group_field} AS #{group_alias}"

relation = select(select_statement).group(group)
relation = except(:group).select(select_statement).group(group)

calculated_data = @klass.connection.select_all(relation.to_sql)

Expand Down
8 changes: 2 additions & 6 deletions activerecord/lib/active_record/relation/query_methods.rb
Expand Up @@ -162,13 +162,9 @@ def build_arel
arel = arel.take(@limit_value) if @limit_value.present?
arel = arel.skip(@offset_value) if @offset_value.present?

@group_values.uniq.each do |g|
arel = arel.group(g) if g.present?
end
arel = arel.group(*@group_values.uniq.select{|g| g.present?})

@order_values.uniq.each do |o|
arel = arel.order(Arel::SqlLiteral.new(o.to_s)) if o.present?
end
arel = arel.order(*@order_values.uniq.select{|o| o.present?}.map(&:to_s))

selects = @select_values.uniq

Expand Down
7 changes: 6 additions & 1 deletion activerecord/lib/active_record/relation/spawn_methods.rb
Expand Up @@ -80,10 +80,15 @@ def apply_finder_options(options)

options.assert_valid_keys(VALID_FIND_OPTIONS)

[:joins, :select, :group, :having, :order, :limit, :offset, :from, :lock, :readonly].each do |finder|
[:joins, :select, :group, :having, :limit, :offset, :from, :lock, :readonly].each do |finder|
relation = relation.send(finder, options[finder]) if options.has_key?(finder)
end

# Give precedence to newly-applied orders and groups to play nicely with with_scope
[:group, :order].each do |finder|
relation.send("#{finder}_values=", Array.wrap(options[finder]) + relation.send("#{finder}_values")) if options.has_key?(finder)
end

relation = relation.where(options[:conditions]) if options.has_key?(:conditions)
relation = relation.includes(options[:include]) if options.has_key?(:include)
relation = relation.extending(options[:extend]) if options.has_key?(:extend)
Expand Down
10 changes: 10 additions & 0 deletions activerecord/test/cases/base_test.rb
Expand Up @@ -1994,6 +1994,16 @@ def test_find_multiple_ordered_last
assert_equal last, Developer.find(:all, :order => 'developers.name, developers.salary DESC').last
end

def test_find_keeps_multiple_order_values
combined = Developer.find(:all, :order => 'developers.name, developers.salary')
assert_equal combined, Developer.find(:all, :order => ['developers.name', 'developers.salary'])
end

def test_find_keeps_multiple_group_values
combined = Developer.find(:all, :group => 'developers.name, developers.salary, developers.id, developers.created_at, developers.updated_at')
assert_equal combined, Developer.find(:all, :group => ['developers.name', 'developers.salary', 'developers.id', 'developers.created_at', 'developers.updated_at'])
end

def test_find_symbol_ordered_last
last = Developer.find :last, :order => :salary
assert_equal last, Developer.find(:all, :order => :salary).last
Expand Down

0 comments on commit 902861a

Please sign in to comment.