Skip to content
Browse files

Fix for #371

if a query contains a limit or an offset, ActiveRecord::FinderMethods#find_last had inconsistent behavior.

If the records were loaded, it returned the last record in the cached list.
If they were not, it reversed the order of the query and changed the limit to one.
If the earlier limit was less than the total matching the query in the db,
it would return a different record than if the records had been cached.

This commit changes find_last so that it loads the records when getting the
last record on a query containing a limit or an offset, which makes the behavior consistent.
  • Loading branch information...
1 parent 820b6f3 commit 92c10760d7f5e1d308e492d5fd3d551df41bfabb @baroquebobcat baroquebobcat committed with jonleighton
View
7 activerecord/lib/active_record/relation/finder_methods.rb
@@ -375,7 +375,12 @@ def find_last
if loaded?
@records.last
else
- @last ||= reverse_order.limit(1).to_a[0]
+ @last ||=
+ if offset_value || limit_value
+ to_a.last
@tekin
tekin added a note

Isn't this terribly inefficient? You're essentially loading and instantiating all the records to return just one?

The problem is you can't know before querying, what the last record will be for a query including a limit or offset.

There are certainly bad cases like if you have a large limit value, or an offset without a limit on a query that returns a large number of records.

@tekin
tekin added a note

Ah right, of course, I was naively thinking that you simply had to run the correct query. Clearly time to call it a day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ else
+ reverse_order.limit(1).to_a[0]
+ end
end
end
View
21 activerecord/test/cases/finder_test.rb
@@ -683,6 +683,27 @@ def test_find_last_by_two_attributes
assert_nil Topic.find_last_by_title_and_author_name(topic.title, "Anonymous")
end
+ def test_find_last_with_limit_gives_same_result_when_loaded_and_unloaded
+ scope = Topic.limit(2)
+ unloaded_last = scope.last
+ loaded_last = scope.all.last
+ assert_equal loaded_last, unloaded_last
+ end
+
+ def test_find_last_with_limit_and_offset_gives_same_result_when_loaded_and_unloaded
+ scope = Topic.offset(2).limit(2)
+ unloaded_last = scope.last
+ loaded_last = scope.all.last
+ assert_equal loaded_last, unloaded_last
+ end
+
+ def test_find_last_with_offset_gives_same_result_when_loaded_and_unloaded
+ scope = Topic.offset(3)
+ unloaded_last = scope.last
+ loaded_last = scope.all.last
+ assert_equal loaded_last, unloaded_last
+ end
+
def test_find_all_by_one_attribute
topics = Topic.find_all_by_content("Have a nice day")
assert_equal 2, topics.size

0 comments on commit 92c1076

Please sign in to comment.
Something went wrong with that request. Please try again.