Use LIMIT SQL word in first - Closes #2783 #2789

Merged
merged 2 commits into from Sep 5, 2011
@@ -114,7 +114,7 @@ def find(*args)
def first(*args)
if args.any?
if args.first.kind_of?(Integer) || (loaded? && !args.first.kind_of?(Hash))
- to_a.first(*args)
+ limit(*args).to_a
else
apply_finder_options(args.first).first
end
@@ -134,7 +134,11 @@ def first!
def last(*args)
if args.any?
if args.first.kind_of?(Integer) || (loaded? && !args.first.kind_of?(Hash))
- to_a.last(*args)
+ if order_values.empty? && reorder_value.nil?
jonleighton
jonleighton Sep 5, 2011 Member

Sorry to make yet another comment, but we need to check reorder_values too. Sorry, I should have caught that before.

dmathieu
dmathieu Sep 5, 2011 Contributor

No problem. Fixed and pushed.

+ order("#{primary_key} DESC").limit(*args).reverse
+ else
+ to_a.last(*args)
+ end
else
apply_finder_options(args.first).last
end
@@ -243,6 +243,32 @@ def test_model_class_responds_to_last_bang
end
end
+ def test_first_and_last_with_integer_should_use_sql_limit
+ assert_sql(/LIMIT 2/) { Topic.first(2).entries }
+ assert_sql(/LIMIT 5/) { Topic.last(5).entries }
+ end
jonleighton
jonleighton Sep 4, 2011 Member

Please could you test against actual records, rather than the SQL, so we can be sure it works as imagined. Also would be good to assert that the return value is an ActiveRecord::Relation.

dmathieu
dmathieu Sep 5, 2011 Contributor

Well, the value returned by first and last is already tested here.
I'll add a test checking that the value is an ActiveRecord::Relation.

jonleighton
jonleighton Sep 5, 2011 Member

Ok, despite what I said earlier, I now think we should actually convert the values to a flat Array because:

  • last cannot always be a Relation, and I think it is good to have consistency between first and last
  • I can't think of a use-case for wanting first or last to be a Relation
dmathieu
dmathieu Sep 5, 2011 Contributor

Done.

jonleighton
jonleighton Sep 5, 2011 Member

Thanks. Do you want to implement what I suggested above? ("Maybe you could use the above if the relation has no existing ordering, and otherwise fall back to to_a?") If not I am happy to merge this as-is, but if you want to then I'll wait for that.

dmathieu
dmathieu Sep 5, 2011 Contributor

Yes, I'm currently on it.

+
+ def test_last_with_integer_and_order_should_keep_the_order
+ 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)
+ 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_first_and_last_with_integer_should_return_an_array
+ assert_kind_of Array, Topic.first(5)
+ assert_kind_of Array, Topic.last(5)
+ end
+
def test_unexisting_record_exception_handling
assert_raise(ActiveRecord::RecordNotFound) {
Topic.find(1).parent