aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord
diff options
context:
space:
mode:
Diffstat (limited to 'activerecord')
-rw-r--r--activerecord/CHANGELOG.md20
-rw-r--r--activerecord/lib/active_record/relation/finder_methods.rb39
-rw-r--r--activerecord/test/cases/finder_test.rb32
3 files changed, 60 insertions, 31 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 50a0b291b3..faab0f9554 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,23 @@
+* Rework `ActiveRecord::Relation#last`
+
+ 1. Always find last with ruby if relation is loaded
+ 2. Always use SQL instead if relation is not loaded.
+ 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*
+
* `ActiveRecord::Relation#reverse_order` throws `ActiveRecord::IrreversibleOrderError`
when the order can not be reversed using current trivial algorithm.
Also raises the same error when `#reverse_order` is called on
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index 3f5d6de78a..5d4a045097 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -145,15 +145,23 @@ 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_table[primary_key].desc).limit(limit).reverse
- else
- to_a.last(limit)
- end
- else
- find_last
- end
+ return find_last(limit) if loaded?
+
+ result = order_values.empty? && primary_key ? order(arel_table[primary_key].desc) : reverse_order
+ result = result.limit!(limit || 1)
+ 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
+
+ def find_last(limit)
+ limit ? to_a.last(limit) : to_a.last
end
# Same as #last but raises ActiveRecord::RecordNotFound if no record
@@ -523,19 +531,6 @@ module ActiveRecord
relation.limit(limit).to_a
end
- def find_last
- if loaded?
- @records.last
- else
- @last ||=
- if limit_value
- to_a.last
- else
- reverse_order.limit(1).to_a.first
- end
- end
- end
-
private
def find_nth_with_limit_and_offset(index, limit, offset:) # :nodoc:
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index 75a74c052d..cbeb37dd22 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -506,7 +506,7 @@ class FinderTest < ActiveRecord::TestCase
end
end
- def test_take_and_first_and_last_with_integer_should_use_sql_limit
+ def test_take_and_first_and_last_with_integer_should_use_sql
assert_sql(/LIMIT|ROWNUM <=/) { Topic.take(3).entries }
assert_sql(/LIMIT|ROWNUM <=/) { Topic.first(2).entries }
assert_sql(/LIMIT|ROWNUM <=/) { Topic.last(5).entries }
@@ -516,16 +516,30 @@ 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
+ 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
+ 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_take_and_first_and_last_with_integer_should_return_an_array