From 0cbcae59dd402ff27f6dbf76659b67a77776fe37 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Mon, 1 Feb 2016 14:03:12 -0700 Subject: [PATCH] Revert "Merge pull request #16400 from bogdan/last-with-sql" This reverts commit 9f3730a516f30beb0050caea9539f8d6b808e58a, reversing changes made to 2637fb75d82e1c69333855abd58c2470994995d3. There are additional issues with this commit that need to be addressed before this change is ready (see #23377). This is a temporary revert in order for us to have more time to address the issues with that PR, without blocking the release of beta2. --- activerecord/CHANGELOG.md | 20 ---------- .../active_record/relation/finder_methods.rb | 39 +++++++++++-------- activerecord/test/cases/finder_test.rb | 32 +++++---------- 3 files changed, 31 insertions(+), 60 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index faab0f9554..50a0b291b3 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,23 +1,3 @@ -* 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 5d4a045097..3f5d6de78a 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -145,23 +145,15 @@ module ActiveRecord # # [#, #, #] def last(limit = nil) - 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 + 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 end # Same as #last but raises ActiveRecord::RecordNotFound if no record @@ -531,6 +523,19 @@ 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 cbeb37dd22..75a74c052d 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 + def test_take_and_first_and_last_with_integer_should_use_sql_limit 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,30 +516,16 @@ 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_use_sql - relation = Topic.order("title") - assert_queries(1) { relation.last(5) } - assert !relation.loaded? + 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) end - 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 + 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) end def test_take_and_first_and_last_with_integer_should_return_an_array