Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix for #371 #451

Closed
wants to merge 1 commit into from

3 participants

Nick Howard José Valim Jon Leighton
Nick Howard

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.

Nick Howard baroquebobcat 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.
95b2590
Jon Leighton
Collaborator

Merged, thanks!

Jon Leighton jonleighton closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 7, 2011
  1. Nick Howard

    Fix for #371

    baroquebobcat authored
    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.
This page is out of date. Refresh to see the latest.
6 activerecord/lib/active_record/relation/finder_methods.rb
View
@@ -375,7 +375,11 @@ 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
+ else
+ reverse_order.limit(1).to_a[0]
+ end
end
end
21 activerecord/test/cases/finder_test.rb
View
@@ -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
Something went wrong with that request. Please try again.