Made first finder consistent among database engines by adding a default order

activerecord/CHANGELOG.md

@@ -1,5 +1,11 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   Added default order to `first` to assure consistent results among
+    diferent database engines. Introduced `take` as a replacement to
+    the old behavior of `first`.
+
+    *Marcelo Silveira*
+
 *   Added an :index option to automatically create indexes for references
     and belongs_to statements in migrations.
 

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

@@ -1233,7 +1233,10 @@ def distinct(columns, orders) #:nodoc:
 
         # Construct a clean list of column names from the ORDER BY clause, removing
         # any ASC/DESC modifiers
-        order_columns = orders.collect { |s| s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '') }
+        order_columns = orders.collect do |s|
+          s = s.to_sql unless s.is_a?(String)
+          s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '')
+        end
         order_columns.delete_if { |c| c.blank? }
         order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
 

activerecord/lib/active_record/querying.rb

@@ -3,7 +3,7 @@
 
 module ActiveRecord
   module Querying
-    delegate :find, :first, :first!, :last, :last!, :all, :exists?, :any?, :many?, :to => :scoped
+    delegate :find, :take, :take!, :first, :first!, :last, :last!, :all, :exists?, :any?, :many?, :to => :scoped
     delegate :first_or_create, :first_or_create!, :first_or_initialize, :to => :scoped
     delegate :find_by, :find_by!, :to => :scoped
     delegate :destroy, :destroy_all, :delete, :delete_all, :update, :update_all, :to => :scoped

activerecord/lib/active_record/relation/finder_methods.rb

@@ -60,14 +60,44 @@ def find_by!(*args)
       where(*args).first!
     end
 
+    # Gives a record (or N records if a parameter is supplied) without any implied
+    # order. The order will depend on the database implementation.
+    # If an order is supplied it will be respected.
+    #
+    # Examples:
+    #
+    #   Person.take # returns an object fetched by SELECT * FROM people
+    #   Person.take(5) # returns 5 objects fetched by SELECT * FROM people LIMIT 5
+    #   Person.where(["name LIKE '%?'", name]).take
+    def take(limit = nil)
+      limit ? limit(limit).to_a : find_take
+    end
+
+    # Same as +take+ but raises <tt>ActiveRecord::RecordNotFound</tt> if no record
+    # is found. Note that <tt>take!</tt> accepts no arguments.
+    def take!
+      take or raise RecordNotFound
+    end
+
+    # Find the first record (or first N records if a parameter is supplied).
+    # If no order is defined it will order by primary key.
+    #
     # Examples:
     #
     #   Person.first # returns the first object fetched by SELECT * FROM people
     #   Person.where(["user_name = ?", user_name]).first
     #   Person.where(["user_name = :u", { :u => user_name }]).first
     #   Person.order("created_on DESC").offset(5).first
     def first(limit = nil)
-      limit ? limit(limit).to_a : find_first
+      if limit
+        if order_values.empty? && primary_key
+          order(arel_table[primary_key].asc).limit(limit).to_a
+        else
+          limit(limit).to_a
+        end
+      else
+        find_first
+      end
     end
 
     # Same as +first+ but raises <tt>ActiveRecord::RecordNotFound</tt> if no record
@@ -76,15 +106,18 @@ def first!
       first or raise RecordNotFound
     end
 
+    # Find the last record (or last N records if a parameter is supplied).
+    # If no order is defined it will order by primary key.
+    #
     # Examples:
     #
     #   Person.last # returns the last object fetched by SELECT * FROM people
     #   Person.where(["user_name = ?", user_name]).last
     #   Person.order("created_on DESC").offset(5).last
     def last(limit = nil)
       if limit
-        if order_values.empty?
-          order("#{primary_key} DESC").limit(limit).reverse
+        if order_values.empty? && primary_key
+          order(arel_table[primary_key].desc).limit(limit).reverse
         else
           to_a.last(limit)
         end
@@ -315,11 +348,24 @@ def find_some(ids)
       end
     end
 
+    def find_take
+      if loaded?
+        @records.take(1).first
+      else
+        @take ||= limit(1).to_a.first
+      end
+    end
+
     def find_first
       if loaded?
         @records.first
       else
-        @first ||= limit(1).to_a[0]
+        @first ||=
+          if order_values.empty? && primary_key
+            order(arel_table[primary_key].asc).limit(1).to_a.first
+          else
+            limit(1).to_a.first
+          end
       end
     end
 
@@ -331,7 +377,7 @@ def find_last
           if offset_value || limit_value
             to_a.last
           else
-            reverse_order.limit(1).to_a[0]
+            reverse_order.limit(1).to_a.first
           end
       end
     end

activerecord/test/cases/finder_test.rb

@@ -143,6 +143,26 @@ def test_find_by_sql_with_sti_on_joined_table
     assert_equal [Account], accounts.collect(&:class).uniq
   end
 
+  def test_take
+    assert_equal topics(:first), Topic.take
+  end
+
+  def test_take_failing
+    assert_nil Topic.where("title = 'This title does not exist'").take
+  end
+
+  def test_take_bang_present
+    assert_nothing_raised do
+      assert_equal topics(:second), Topic.where("title = 'The Second Topic of the day'").take!
+    end
+  end
+
+  def test_take_bang_missing
+    assert_raises ActiveRecord::RecordNotFound do
+      Topic.where("title = 'This title does not exist'").take!
+    end
+  end
+
   def test_first
     assert_equal topics(:second).title, Topic.where("title = 'The Second Topic of the day'").first.title
   end
@@ -163,6 +183,12 @@ def test_first_bang_missing
     end
   end
 
+  def test_first_have_primary_key_order_by_default
+    expected = topics(:first)
+    expected.touch # PostgreSQL changes the default order if no order clause is used
+    assert_equal expected, Topic.first
+  end
+
   def test_model_class_responds_to_first_bang
     assert Topic.first!
     Topic.delete_all
@@ -191,7 +217,8 @@ def test_model_class_responds_to_last_bang
     end
   end
 
-  def test_first_and_last_with_integer_should_use_sql_limit
+  def test_take_and_first_and_last_with_integer_should_use_sql_limit
+    assert_sql(/LIMIT 3|ROWNUM <= 3/) { Topic.take(3).entries }
     assert_sql(/LIMIT 2|ROWNUM <= 2/) { Topic.first(2).entries }
     assert_sql(/LIMIT 5|ROWNUM <= 5/) { Topic.last(5).entries }
   end
@@ -212,7 +239,8 @@ def test_last_with_integer_and_reorder_should_not_use_sql_limit
     assert_no_match(/LIMIT/, query.first)
   end
 
-  def test_first_and_last_with_integer_should_return_an_array
+  def test_take_and_first_and_last_with_integer_should_return_an_array
+    assert_kind_of Array, Topic.take(5)
     assert_kind_of Array, Topic.first(5)
     assert_kind_of Array, Topic.last(5)
   end