From b67991614349d68e0d8ad489c8c84c3254ac847b Mon Sep 17 00:00:00 2001 From: Bogdan Gusiev Date: Sat, 13 Feb 2016 00:11:18 +0200 Subject: Make ActiveRecord::Relation#last to reverse SQL order instead of loading the relation into memory --- activerecord/CHANGELOG.md | 21 +++++++++++ .../lib/active_record/relation/finder_methods.rb | 28 +++++++++----- .../lib/active_record/relation/query_methods.rb | 2 + activerecord/test/cases/finder_test.rb | 44 ++++++++++++++++++---- 4 files changed, 78 insertions(+), 17 deletions(-) (limited to 'activerecord') diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 45c123d943..5bbaf0cff2 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 a4280c5f33..ba64e3ca60 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 # # [#, #, #] 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 75a74c052d..0004c98c8a 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 -- cgit v1.2.3