Skip to content

Commit

Permalink
order should always be concatenated.
Browse files Browse the repository at this point in the history
order that is declared first has highest priority in all cases.

Here are some examples.

Car.order('name desc').find(:first, :order => 'id').name

Car.named_scope_with_order.named_scope_with_another_order

Car.order('id DESC').scoping do
  Car.find(:first, :order => 'id asc')
end

No special treatment to with_scope or scoping.

Also note that if default_scope declares an order then the order
declared in default_scope has the highest priority unless
with_exclusive_scope is used.

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
  • Loading branch information
Neeraj Singh authored and spastorino committed Sep 5, 2010
1 parent 2988751 commit 1c7a754
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 8 deletions.
3 changes: 1 addition & 2 deletions activerecord/lib/active_record/relation/spawn_methods.rb
Expand Up @@ -99,9 +99,8 @@ def apply_finder_options(options)

relation = relation.readonly(options[:readonly]) if options.key? :readonly

# 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)
relation.send("#{finder}_values=", relation.send("#{finder}_values") + Array.wrap(options[finder])) if options.has_key?(finder)
end

relation = relation.where(options[:conditions]) if options.has_key?(:conditions)
Expand Down
8 changes: 4 additions & 4 deletions activerecord/test/cases/base_test.rb
Expand Up @@ -1139,18 +1139,18 @@ def test_scoped_find_order
scoped_developers = Developer.send(:with_scope, :find => { :limit => 1 }) do
Developer.find(:all, :order => 'salary DESC')
end
# Test scope order + find order, find has priority
# Test scope order + find order, order has priority
scoped_developers = Developer.send(:with_scope, :find => { :limit => 3, :order => 'id DESC' }) do
Developer.find(:all, :order => 'salary ASC')
end
assert scoped_developers.include?(developers(:poor_jamis))
assert scoped_developers.include?(developers(:david))
assert ! scoped_developers.include?(developers(:david))
assert ! scoped_developers.include?(developers(:jamis))
assert_equal 3, scoped_developers.size

# Test without scoped find conditions to ensure we get the right thing
developers = Developer.find(:all, :order => 'id', :limit => 1)
assert scoped_developers.include?(developers(:david))
assert ! scoped_developers.include?(Developer.find(1))
assert scoped_developers.include?(Developer.find(11))
end

def test_scoped_find_limit_offset_including_has_many_association
Expand Down
4 changes: 2 additions & 2 deletions activerecord/test/cases/relation_scoping_test.rb
Expand Up @@ -398,8 +398,8 @@ def test_nested_exclusive_scope
assert_equal expected, received
end

def test_overwriting_default_scope
expected = Developer.find(:all, :order => 'salary').collect { |dev| dev.salary }
def test_order_in_default_scope_should_prevail
expected = Developer.find(:all, :order => 'salary desc').collect { |dev| dev.salary }
received = DeveloperOrderedBySalary.find(:all, :order => 'salary').collect { |dev| dev.salary }
assert_equal expected, received
end
Expand Down
18 changes: 18 additions & 0 deletions activerecord/test/cases/relations_test.rb
Expand Up @@ -667,4 +667,22 @@ def test_order_by_relation_attribute
def test_relations_limit_with_conditions_or_limit
assert_equal Post.limit(2).size, Post.limit(2).all.size
end

def test_order_with_find_with_order
assert_equal 'zyke', Car.order('name desc').find(:first, :order => 'id').name
end

def test_default_scope_order_with_named_scope_order
assert_equal 'zyke', Car.order_using_new_style.limit(1).first.name
assert_equal 'zyke', Car.order_using_old_style.limit(1).first.name
end

def test_order_using_scoping
car = Car.order('id DESC').scoping do
Car.find(:first, :order => 'id asc')
end
assert_equal 'zyke', car.name
end


end
5 changes: 5 additions & 0 deletions activerecord/test/fixtures/cars.yml
Expand Up @@ -2,3 +2,8 @@ honda:
id: 1
name: honda
engines_count: 0

zyke:
id: 2
name: zyke
engines_count: 0
5 changes: 5 additions & 0 deletions activerecord/test/models/car.rb
Expand Up @@ -7,4 +7,9 @@ class Car < ActiveRecord::Base
scope :incl_tyres, includes(:tyres)
scope :incl_engines, includes(:engines)

default_scope :order => 'name desc'

scope :order_using_new_style, order('name asc')
scope :order_using_old_style, :order => 'name asc'

end

0 comments on commit 1c7a754

Please sign in to comment.