Merge pull request #6173 from mhfs/takes_instead_first

Use `take` instead of `first` to avoid unwanted implicit ordering. Closes #6147.

3 files changed, 17 insertions, 5 deletions

diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index a78e3d08e4..cc716bbfd1 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -51,13 +51,13 @@ module ActiveRecord
     #   Post.find_by "published_at < ?", 2.weeks.ago
     #
     def find_by(*args)
-      where(*args).first
+      where(*args).take
     end
 
     # Like <tt>find_by</tt>, except that if no record is found, raises
     # an <tt>ActiveRecord::RecordNotFound</tt> error.
     def find_by!(*args)
-      where(*args).first!
+      where(*args).take!
     end
 
     # Gives a record (or N records if a parameter is supplied) without any implied
@@ -269,7 +269,7 @@ module ActiveRecord
       substitute = connection.substitute_at(column, bind_values.length)
       relation = where(table[primary_key].eq(substitute))
       relation.bind_values += [[column, id]]
-      record = relation.first
+      record = relation.take
 
       unless record
         conditions = arel.where_sql
@@ -309,7 +309,7 @@ module ActiveRecord
 
     def find_take
       if loaded?
-        @records.take(1).first
+        @records.first
       else
         @take ||= limit(1).to_a.first
       end
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index 54801bd101..630acdbc46 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -113,6 +113,10 @@ class FinderTest < ActiveRecord::TestCase
     assert_equal [], Topic.find([])
   end
 
+  def test_find_doesnt_have_implicit_ordering
+    assert_sql(/^((?!ORDER).)*$/) { Topic.find(1) }
+  end
+
   def test_find_by_ids_missing_one
     assert_raise(ActiveRecord::RecordNotFound) { Topic.find(1, 2, 45) }
   end
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 3ef357e297..8cef4423c5 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -255,7 +255,7 @@ class RelationTest < ActiveRecord::TestCase
       assert_equal nil,   Developer.none.calculate(:average, 'salary')
     end
   end
-  
+
   def test_null_relation_metadata_methods
     assert_equal "", Developer.none.to_sql
     assert_equal({}, Developer.none.where_values_hash)
@@ -1287,6 +1287,10 @@ class RelationTest < ActiveRecord::TestCase
     assert_equal nil, Post.scoped.find_by("1 = 0")
   end
 
+  test "find_by doesn't have implicit ordering" do
+    assert_sql(/^((?!ORDER).)*$/) { Post.find_by(author_id: 2) }
+  end
+
   test "find_by! with hash conditions returns the first matching record" do
     assert_equal posts(:eager_other), Post.order(:id).find_by!(author_id: 2)
   end
@@ -1299,6 +1303,10 @@ class RelationTest < ActiveRecord::TestCase
     assert_equal posts(:eager_other), Post.order(:id).find_by!('author_id = ?', 2)
   end
 
+  test "find_by! doesn't have implicit ordering" do
+    assert_sql(/^((?!ORDER).)*$/) { Post.find_by!(author_id: 2) }
+  end
+
   test "find_by! raises RecordNotFound if the record is missing" do
     assert_raises(ActiveRecord::RecordNotFound) do
       Post.scoped.find_by!("1 = 0")