Merge pull request #23377 from bogdan/last-with-sql

Fix AR::Relation#last bugs instroduced in 7705fc

4 files changed, 78 insertions, 17 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 53538f4157..f13a80195a 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,24 @@
+*   Rework `ActiveRecord::Relation#last` 
+    
+    1. Never perform additional SQL on loaded relation
+    2. Use SQL reverse order instead of loading relation if relation doesn't have limit
+    3. Deprecated relation loading when SQL order can not be automatically reversed
+
+        Topic.order("title").load.last(3)
+          # before: SELECT ...
+          # after: No SQL
+
+        Topic.order("title").last
+          # before: SELECT * FROM `topics`
+          # after:  SELECT * FROM `topics` ORDER BY `topics`.`title` DESC LIMIT 1
+
+        Topic.order("coalesce(author, title)").last
+          # before: SELECT * FROM `topics`
+          # after:  Deprecation Warning for irreversible order
+
+    *Bogdan Gusiev*
+
+
 *   Allow `joins` to be unscoped.
 
     Closes #13775.
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index 75482cb381..8b114c6bdf 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -145,15 +145,21 @@ module ActiveRecord
     #
     #   [#<Person id:4>, #<Person id:3>, #<Person id:2>]
     def last(limit = nil)
-      if limit
-        if order_values.empty? && primary_key
-          order(arel_attribute(primary_key).desc).limit(limit).reverse
-        else
-          to_a.last(limit)
-        end
-      else
-        find_last
-      end
+      return find_last(limit) if loaded? || limit_value
+
+      result = limit(limit || 1)
+      result.order!(arel_attribute(primary_key)) if order_values.empty? && primary_key
+      result = result.reverse_order!
+
+      limit ? result.reverse : result.first
+    rescue ActiveRecord::IrreversibleOrderError
+      ActiveSupport::Deprecation.warn(<<-WARNING.squish)
+          Finding a last element by loading the relation when SQL ORDER
+          can not be reversed is deprecated.
+          Rails 5.1 will raise ActiveRecord::IrreversibleOrderError in this case.
+          Please call `to_a.last` if you still want to load the relation.
+      WARNING
+      find_last(limit)
     end
 
     # Same as #last but raises ActiveRecord::RecordNotFound if no record
@@ -578,5 +584,9 @@ module ActiveRecord
         find_nth_with_limit(index, limit)
       end
     end
+
+    def find_last(limit)
+      limit ? to_a.last(limit) : to_a.last
+    end
   end
 end
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 91bfa4d131..91d486e902 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -1112,6 +1112,8 @@ module ActiveRecord
 
       order_query.flat_map do |o|
         case o
+        when Arel::Attribute
+          o.desc
         when Arel::Nodes::Ordering
           o.reverse
         when String
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index ba0400f799..5b41630cd8 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -516,16 +516,44 @@ class FinderTest < ActiveRecord::TestCase
     assert_equal Topic.order("title").to_a.last(2), Topic.order("title").last(2)
   end
 
-  def test_last_with_integer_and_order_should_not_use_sql_limit
-    query = assert_sql { Topic.order("title").last(5).entries }
-    assert_equal 1, query.length
-    assert_no_match(/LIMIT/, query.first)
+  def test_last_with_integer_and_order_should_use_sql_limit
+    relation = Topic.order("title")
+    assert_queries(1) { relation.last(5) }
+    assert !relation.loaded?
   end
 
-  def test_last_with_integer_and_reorder_should_not_use_sql_limit
-    query = assert_sql { Topic.reorder("title").last(5).entries }
-    assert_equal 1, query.length
-    assert_no_match(/LIMIT/, query.first)
+  def test_last_with_integer_and_reorder_should_use_sql_limit
+    relation = Topic.reorder("title")
+    assert_queries(1) { relation.last(5) }
+    assert !relation.loaded?
+  end
+
+  def test_last_on_loaded_relation_should_not_use_sql
+    relation  = Topic.limit(10).load
+    assert_no_queries do
+      relation.last
+      relation.last(2)
+    end
+  end
+
+  def test_last_with_irreversible_order
+    assert_deprecated do
+      Topic.order("coalesce(author_name, title)").last
+    end
+  end  
+
+  def test_last_on_relation_with_limit_and_offset
+    post = posts('sti_comments')
+
+    comments = post.comments.order(id: :asc)
+    assert_equal comments.limit(2).to_a.last, comments.limit(2).last
+    assert_equal comments.limit(2).to_a.last(2), comments.limit(2).last(2)
+    assert_equal comments.limit(2).to_a.last(3), comments.limit(2).last(3)
+
+    comments = comments.offset(1)
+    assert_equal comments.limit(2).to_a.last, comments.limit(2).last
+    assert_equal comments.limit(2).to_a.last(2), comments.limit(2).last(2)
+    assert_equal comments.limit(2).to_a.last(3), comments.limit(2).last(3)
   end
 
   def test_take_and_first_and_last_with_integer_should_return_an_array