Fix issue #1272

Set reverse_order_value when asked to reverse_order().
Do the actual reversal in build_arel.

4 files changed, 24 insertions, 10 deletions

diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index e852f50d86..317af8a15d 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -6,7 +6,7 @@ module ActiveRecord
     JoinOperation = Struct.new(:relation, :join_class, :on)
     ASSOCIATION_METHODS = [:includes, :eager_load, :preload]
     MULTI_VALUE_METHODS = [:select, :group, :order, :joins, :where, :having, :bind]
-    SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :create_with, :from, :reorder]
+    SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :create_with, :from, :reorder, :reverse_order]
 
     include FinderMethods, Calculations, SpawnMethods, QueryMethods, Batches
 
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 94aa999715..739363415c 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -9,7 +9,7 @@ module ActiveRecord
                   :select_values, :group_values, :order_values, :joins_values,
                   :where_values, :having_values, :bind_values,
                   :limit_value, :offset_value, :lock_value, :readonly_value, :create_with_value,
-                  :from_value, :reorder_value
+                  :from_value, :reorder_value, :reverse_order_value
 
     def includes(*args)
       args.reject! {|a| a.blank? }
@@ -158,13 +158,9 @@ module ActiveRecord
     end
 
     def reverse_order
-      order_clause = arel.order_clauses
-
-      order = order_clause.empty? ?
-        "#{table_name}.#{primary_key} DESC" :
-        reverse_sql_order(order_clause).join(', ')
-
-      except(:order).order(Arel.sql(order))
+      relation = clone
+      relation.reverse_order_value = !relation.reverse_order_value
+      relation
     end
 
     def arel
@@ -186,6 +182,7 @@ module ActiveRecord
       arel.group(*@group_values.uniq.reject{|g| g.blank?}) unless @group_values.empty?
 
       order = @reorder_value ? @reorder_value : @order_values
+      order = reverse_sql_order(order) if @reverse_order_value
       arel.order(*order.uniq.reject{|o| o.blank?}) unless order.empty?
 
       build_select(arel, @select_values.uniq)
@@ -306,6 +303,8 @@ module ActiveRecord
     end
 
     def reverse_sql_order(order_query)
+      order_query = ["#{quoted_table_name}.#{quoted_primary_key} ASC"] if order_query.empty?
+
       order_query.join(', ').split(',').collect do |s|
         s.gsub!(/\sasc\Z/i, ' DESC') || s.gsub!(/\sdesc\Z/i, ' ASC') || s.concat(' DESC')
       end
diff --git a/activerecord/test/cases/relation_scoping_test.rb b/activerecord/test/cases/relation_scoping_test.rb
index 83bc472764..a6e08f95d0 100644
--- a/activerecord/test/cases/relation_scoping_test.rb
+++ b/activerecord/test/cases/relation_scoping_test.rb
@@ -11,6 +11,14 @@ require 'models/reference'
 class RelationScopingTest < ActiveRecord::TestCase
   fixtures :authors, :developers, :projects, :comments, :posts, :developers_projects
 
+  def test_reverse_order
+    assert_equal Developer.order("id DESC").to_a.reverse, Developer.order("id DESC").reverse_order
+  end
+
+  def test_double_reverse_order_produces_original_order
+    assert_equal Developer.order("name DESC"), Developer.order("name DESC").reverse_order.reverse_order
+  end
+
   def test_scoped_find
     Developer.where("name = 'David'").scoping do
       assert_nothing_raised { Developer.find(1) }
@@ -483,4 +491,11 @@ class DefaultScopingTest < ActiveRecord::TestCase
   def test_default_scope_order_ignored_by_aggregations
     assert_equal DeveloperOrderedBySalary.all.count, DeveloperOrderedBySalary.count
   end
+
+  def test_default_scope_find_last
+    assert DeveloperOrderedBySalary.count > 1, "need more than one row for test"
+
+    lowest_salary_dev = DeveloperOrderedBySalary.find(developers(:poor_jamis).id)
+    assert_equal lowest_salary_dev, DeveloperOrderedBySalary.last
+  end
 end
diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb
index 6874bd18f8..12e58bd340 100644
--- a/activerecord/test/cases/relation_test.rb
+++ b/activerecord/test/cases/relation_test.rb
@@ -20,7 +20,7 @@ module ActiveRecord
     end
 
     def test_single_values
-      assert_equal [:limit, :offset, :lock, :readonly, :create_with, :from, :reorder].map(&:to_s).sort,
+      assert_equal [:limit, :offset, :lock, :readonly, :create_with, :from, :reorder, :reverse_order].map(&:to_s).sort,
         Relation::SINGLE_VALUE_METHODS.map(&:to_s).sort
     end