Merge pull request #26200 from kamipo/remove_unnecessary_ordinal_methods

Remove unnecessary ordinal methods for collection association

3 files changed, 84 insertions, 93 deletions

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 7c688d663c..b8d1e0cea4 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -111,50 +111,6 @@ module ActiveRecord
         end
       end
 
-      def first(limit = nil)
-        find_nth_or_last(:first, limit)
-      end
-
-      def second
-        find_nth_or_last(:second)
-      end
-
-      def third
-        find_nth_or_last(:third)
-      end
-
-      def fourth
-        find_nth_or_last(:fourth)
-      end
-
-      def fifth
-        find_nth_or_last(:fifth)
-      end
-
-      def forty_two
-        find_nth_or_last(:forty_two)
-      end
-
-      def third_to_last
-        find_nth_or_last(:third_to_last)
-      end
-
-      def second_to_last
-        find_nth_or_last(:second_to_last)
-      end
-
-      def last(limit = nil)
-        find_nth_or_last(:last, limit)
-      end
-
-      def take(limit = nil)
-        if find_from_target?
-          limit ? load_target.take(limit) : load_target.first
-        else
-          scope.take(limit)
-        end
-      end
-
       def build(attributes = {}, &block)
         if attributes.is_a?(Array)
           attributes.collect { |attr| build(attr, &block) }
@@ -453,6 +409,12 @@ module ActiveRecord
         owner.new_record? && !foreign_key_present?
       end
 
+      def find_from_target?
+        loaded? ||
+          owner.new_record? ||
+          target.any? { |record| record.new_record? || record.changed? }
+      end
+
       private
 
         def find_target
@@ -599,21 +561,6 @@ module ActiveRecord
           owner.class.send(full_callback_name)
         end
 
-        # Should we deal with assoc.first or assoc.last by issuing an independent query to
-        # the database, or by getting the target, and then taking the first/last item from that?
-        #
-        # If the args is just a non-empty options hash, go to the database.
-        #
-        # Otherwise, go to the database only if none of the following are true:
-        #   * target already loaded
-        #   * owner is new record
-        #   * target contains new or changed record(s)
-        def find_from_target?
-          loaded? ||
-            owner.new_record? ||
-            target.any? { |record| record.new_record? || record.changed? }
-        end
-
         def include_in_memory?(record)
           if reflection.is_a?(ActiveRecord::Reflection::ThroughReflection)
             assoc = owner.association(reflection.through_reflection.name)
@@ -640,12 +587,6 @@ module ActiveRecord
             load_target.select { |r| ids.include?(r.id.to_s) }
           end
         end
-
-        # Fetches the first/last using SQL if possible, otherwise from the target array.
-        def find_nth_or_last(ordinal, limit = nil)
-          collection = find_from_target? ? load_target : scope
-          limit ? collection.send(ordinal, limit) : collection.send(ordinal)
-        end
     end
   end
 end
diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb
index 36f6c1b9c3..4634afdf69 100644
--- a/activerecord/lib/active_record/associations/collection_proxy.rb
+++ b/activerecord/lib/active_record/associations/collection_proxy.rb
@@ -140,6 +140,12 @@ module ActiveRecord
         @association.find(*args, &block)
       end
 
+      ##
+      # :method: first
+      #
+      # :call-seq:
+      #   first(limit = nil)
+      #
       # Returns the first record, or the first +n+ records, from the collection.
       # If the collection is empty, the first form returns +nil+, and the second
       # form returns an empty array.
@@ -166,45 +172,63 @@ module ActiveRecord
       #   another_person_without.pets          # => []
       #   another_person_without.pets.first    # => nil
       #   another_person_without.pets.first(3) # => []
-      def first(limit = nil)
-        @association.first(limit)
-      end
 
+      ##
+      # :method: second
+      #
+      # :call-seq:
+      #   second()
+      #
       # Same as #first except returns only the second record.
-      def second
-        @association.second
-      end
 
+      ##
+      # :method: third
+      #
+      # :call-seq:
+      #   third()
+      #
       # Same as #first except returns only the third record.
-      def third
-        @association.third
-      end
 
+      ##
+      # :method: fourth
+      #
+      # :call-seq:
+      #   fourth()
+      #
       # Same as #first except returns only the fourth record.
-      def fourth
-        @association.fourth
-      end
 
+      ##
+      # :method: fifth
+      #
+      # :call-seq:
+      #   fifth()
+      #
       # Same as #first except returns only the fifth record.
-      def fifth
-        @association.fifth
-      end
 
+      ##
+      # :method: forty_two
+      #
+      # :call-seq:
+      #   forty_two()
+      #
       # Same as #first except returns only the forty second record.
       # Also known as accessing "the reddit".
-      def forty_two
-        @association.forty_two
-      end
 
+      ##
+      # :method: third_to_last
+      #
+      # :call-seq:
+      #   third_to_last()
+      #
       # Same as #first except returns only the third-to-last record.
-      def third_to_last
-        @association.third_to_last
-      end
 
+      ##
+      # :method: second_to_last
+      #
+      # :call-seq:
+      #   second_to_last()
+      #
       # Same as #first except returns only the second-to-last record.
-      def second_to_last
-        @association.second_to_last
-      end
 
       # Returns the last record, or the last +n+ records, from the collection.
       # If the collection is empty, the first form returns +nil+, and the second
@@ -233,7 +257,8 @@ module ActiveRecord
       #   another_person_without.pets.last    # => nil
       #   another_person_without.pets.last(3) # => []
       def last(limit = nil)
-        @association.last(limit)
+        load_target if find_from_target?
+        super
       end
 
       # Gives a record (or N records if a parameter is supplied) from the collection
@@ -262,7 +287,8 @@ module ActiveRecord
       #   another_person_without.pets.take    # => nil
       #   another_person_without.pets.take(2) # => []
       def take(limit = nil)
-        @association.take(limit)
+        load_target if find_from_target?
+        super
       end
 
       # Returns a new object of the collection type that has been instantiated
@@ -1078,12 +1104,28 @@ module ActiveRecord
         self
       end
 
+      protected
+
+        def find_nth_with_limit(index, limit)
+          load_target if find_from_target?
+          super
+        end
+
+        def find_nth_from_last(index)
+          load_target if find_from_target?
+          super
+        end
+
       private
 
         def null_scope?
           @association.null_scope?
         end
 
+        def find_from_target?
+          @association.find_from_target?
+        end
+
         def exec_queries
           load_target
         end
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index ff43def901..376867675b 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -97,7 +97,7 @@ module ActiveRecord
     #   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
+      limit ? find_take_with_limit(limit) : find_take
     end
 
     # Same as #take but raises ActiveRecord::RecordNotFound if no record
@@ -526,13 +526,21 @@ module ActiveRecord
         end
       end
 
+      def find_take_with_limit(limit)
+        if loaded?
+          records.take(limit)
+        else
+          limit(limit).to_a
+        end
+      end
+
       def find_nth(index)
         @offsets[offset_index + index] ||= find_nth_with_limit(index, 1).first
       end
 
       def find_nth_with_limit(index, limit)
         if loaded?
-          records[index, limit]
+          records[index, limit] || []
         else
           relation = if order_values.empty? && primary_key
             order(arel_attribute(primary_key).asc)