MONGOID-4998 deprecate :id_sort and implement limit positional argument on #first/#last (#5358)

* MONGOID-4998 add limit to #first

* MOGNOID-4998 add none implementation

* MONGOID-4998 add caching and limit to last, fix first tests

* MONGOID-4998 pass options through on memory, none, findable

* MONGOID-4998 add last findable tests

* Apply suggestions from code review

* MONGOID-4998 add mongoid warning module

* MONGOID-4998 make limit a positional argument

* MONGOID-4998 update last signatures

* MONGOID-4998 fix last sig

* MONGOID-4998 fix mongo tests

* MONGOID-4998 fix tests

* MONGOID-4998 fix enumerable test

* MONGOID-4998 add release note

* MONGOID-4998 refactor numbered_cache

* MONGOID-4998 add back id_sort functionality

* MONGOID-4998 update queries.txt

* MONGOID-4998 np docs

* MONGOID-4998 fix test

* Update docs/reference/queries.txt

Co-authored-by: Oleg Pudeyev <39304720+p-mongo@users.noreply.github.com>

* Update docs/reference/queries.txt

Co-authored-by: Oleg Pudeyev <39304720+p-mongo@users.noreply.github.com>

* Update docs/release-notes/mongoid-7.5.txt

Co-authored-by: Oleg Pudeyev <39304720+p-mongo@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Oleg Pudeyev <39304720+p-mongo@users.noreply.github.com>

* Update docs/release-notes/mongoid-7.5.txt

Co-authored-by: Oleg Pudeyev <39304720+p-mongo@users.noreply.github.com>

* Update lib/mongoid/warnings.rb

Co-authored-by: Oleg Pudeyev <39304720+p-mongo@users.noreply.github.com>

* MONGOID-4998 fix up suggestions

* MONGOID-4998 use nil?

* MONGOID-4998 use nil? on last

Co-authored-by: Oleg Pudeyev <39304720+p-mongo@users.noreply.github.com>

Author: neilshweky

docs/reference/queries.txt

@@ -1301,15 +1301,17 @@ Mongoid also has some helpful methods on criteria.
 
    * - ``Criteria#first|last``
 
-       *Finds a single document given the provided criteria. This automatically adds a sort on id.
-       Opt out of adding the id sort with the {id_sort: :none} option.*
+       *Finds a single document given the provided criteria. This automatically adds a sort on _id.
+       Opt out of adding the id sort with the {id_sort: :none} option. This option is deprecated -
+       please call ``take`` instead. Get a list of documents by passing in a limit argument.*
 
      -
         .. code-block:: ruby
 
           Band.first
           Band.where(:members.with_size => 3).first
           Band.where(:members.with_size => 3).last
+          Band.first(2)
 
    * - ``Criteria#first_or_create``
 

docs/release-notes/mongoid-7.5.txt

@@ -188,3 +188,37 @@ calling ``#first`` or ``#last`` with the ``{ id_sort: :none }`` option. This
 option has been deprecated in Mongoid 7.5 and it is recommended to use ``#take``
 instead going forward. Support for the ``:id_sort`` option will be dropped in
 Mongoid 8.
+
+
+Deprecate ``:id_sort`` Option and Support ``limit`` on ``#first/last``
+----------------------------------------------------------------------
+
+Mongoid 7.5 deprecates the ``:id_sort`` keyword argument for the
+``Criteria#first`` and ``Criteria#last`` methods. Please use ``Criteria#take``
+to retrieve documents without sorting by id.
+
+The ``first`` and ``last`` methods now take the number of documents to return
+as a positional argument, mirroring the functionality of Ruby's ``Enumerable``
+method and ActiveRecord's ``first`` and ``last`` methods. Both invocations
+(with limit as a positional arguments and with the ``:id_sort`` option) remain
+supported in Mongoid 7.x, but the ``:id_sort`` invocation will be removed in
+Mongoid 8.
+
+.. code:: ruby
+
+  Band.first
+  # => #<Band _id: 62c835813282a4470c07d530, >
+  Band.first(2)
+  # => [ #<Band _id: 62c835813282a4470c07d530, >, #<Band _id: 62c835823282a4470c07d531, > ]
+  Band.last
+  # => #<Band _id: 62c835823282a4470c07d531, >
+  Band.last(2)
+  # => [#<Band _id: 62c835813282a4470c07d530, >, #<Band _id: 62c835823282a4470c07d531, >]
+
+When providing a limit, ``#first/last`` will return a list of documents, and
+when not providing a limit (or providing ``nil``), a single document will be
+returned.
+
+Note that the ``#first/last`` methods apply a sort on ``_id``, which can
+cause performance issues. To get a document without sorting first, use the
+``Critera#take`` method.

lib/mongoid.rb

@@ -24,6 +24,7 @@
 require "mongoid/document"
 require "mongoid/tasks/database"
 require "mongoid/query_cache"
+require "mongoid/warnings"
 
 # If we are using Rails then we will include the Mongoid railtie. This has all
 # the nifty initializers that Mongoid needs.

lib/mongoid/association/referenced/has_many/enumerable.rb

@@ -243,14 +243,16 @@ def any?(*args)
           #   use the option { id_sort: :none }.
           #   Be aware that #first/#last won't guarantee order in this case.
           #
-          # @param [ Hash ] opts The options for the query returning the first document.
+          # @param [ Integer | Hash ] limit_or_opts The number of documents to
+          #   return, or a hash of options.
           #
-          # @option opts [ :none ] :id_sort Don't apply a sort on _id.
+          # @option limit_or_opts [ :none ] :id_sort This option is deprecated.
+          #   Don't apply a sort on _id if no other sort is defined on the criteria.
           #
           # @return [ Document ] The first document found.
-          def first(opts = {})
+          def first(limit_or_opts = nil)
             _loaded.try(:values).try(:first) ||
-                _added[(ul = _unloaded.try(:first, opts)).try(:_id)] ||
+                _added[(ul = _unloaded.try(:first, limit_or_opts)).try(:_id)] ||
                 ul ||
                 _added.values.try(:first)
           end
@@ -330,15 +332,17 @@ def in_memory
           #   use the option { id_sort: :none }.
           #   Be aware that #first/#last won't guarantee order in this case.
           #
-          # @param [ Hash ] opts The options for the query returning the first document.
+          # @param [ Integer | Hash ] limit_or_opts The number of documents to
+          #   return, or a hash of options.
           #
-          # @option opts [ :none ] :id_sort Don't apply a sort on _id.
+          # @option limit_or_opts [ :none ] :id_sort This option is deprecated.
+          #   Don't apply a sort on _id if no other sort is defined on the criteria.
           #
           # @return [ Document ] The last document found.
-          def last(opts = {})
+          def last(limit_or_opts = nil)
             _added.values.try(:last) ||
                 _loaded.try(:values).try(:last) ||
-                _added[(ul = _unloaded.try(:last, opts)).try(:_id)] ||
+                _added[(ul = _unloaded.try(:last, limit_or_opts)).try(:_id)] ||
                 ul
           end
 

lib/mongoid/contextual/memory.rb

@@ -116,9 +116,16 @@ def exists?
       # @example Get the first document.
       #   context.first
       #
+      # @param [ Integer | Hash ] limit_or_opts The number of documents to
+      #   return, or a hash of options.
+      #
       # @return [ Document ] The first document.
-      def first(*args)
-        eager_load([documents.first]).first
+      def first(limit_or_opts = nil)
+        if limit_or_opts.nil? || limit_or_opts.is_a?(Hash)
+          eager_load([documents.first]).first
+        else
+          eager_load(documents.first(limit_or_opts))
+        end
       end
       alias :one :first
       alias :find_first :first
@@ -159,9 +166,19 @@ def inc(incs)
       # @example Get the last document.
       #   context.last
       #
+      # @param [ Integer | Hash ] limit_or_opts The number of documents to
+      #   return, or a hash of options.
+      #
+      # @option limit_or_opts [ :none ] :id_sort This option is deprecated.
+      #   Don't apply a sort on _id if no other sort is defined on the criteria.
+      #
       # @return [ Document ] The last document.
-      def last
-        eager_load([documents.last]).first
+      def last(limit_or_opts = nil)
+        if limit_or_opts.nil? || limit_or_opts.is_a?(Hash)
+          eager_load([documents.last]).first
+        else
+          eager_load(documents.last(limit_or_opts))
+        end
       end
 
       # Take the given number of documents from the database.

lib/mongoid/contextual/mongo.rb

@@ -253,25 +253,28 @@ def find_one_and_delete
       #   and have no sort defined on the criteria, use the option { id_sort: :none }.
       #   Be aware that #first/#last won't guarantee order in this case.
       #
-      # @param [ Hash ] opts The options for the query returning the first document.
+      # @param [ Integer | Hash ] limit_or_opts The number of documents to
+      #   return, or a hash of options.
       #
-      # @option opts [ :none ] :id_sort Don't apply a sort on _id if no other sort
-      #   is defined on the criteria.
+      # @option limit_or_opts [ :none ] :id_sort This option is deprecated.
+      #   Don't apply a sort on _id if no other sort is defined on the criteria.
       #
       # @return [ Document ] The first document.
-      def first(opts = {})
-        return documents.first if cached? && cache_loaded?
-        try_cache(:first) do
-          if sort = view.sort || ({ _id: 1 } unless opts[:id_sort] == :none)
-            if raw_doc = view.sort(sort).limit(1).first
-              doc = Factory.from_db(klass, raw_doc, criteria)
-              eager_load([doc]).first
-            end
-          else
-            if raw_doc = view.limit(1).first
-              doc = Factory.from_db(klass, raw_doc, criteria)
-              eager_load([doc]).first
-            end
+      def first(limit_or_opts = nil)
+        limit = limit_or_opts unless limit_or_opts.is_a?(Hash)
+        if cached? && cache_loaded?
+          return limit ? documents.first(limit) : documents.first
+        end
+        try_numbered_cache(:first, limit) do
+          if limit_or_opts.try(:key?, :id_sort)
+            Mongoid::Warnings.warn_id_sort_deprecated
+          end
+          sorted_view = view
+          if sort = view.sort || ({ _id: 1 } unless limit_or_opts.try(:fetch, :id_sort) == :none)
+            sorted_view = view.sort(sort)
+          end
+          if raw_docs = sorted_view.limit(limit || 1).to_a
+            process_raw_docs(raw_docs, limit)
           end
         end
       end
@@ -362,19 +365,26 @@ def initialize(criteria)
       #   and have no sort defined on the criteria, use the option { id_sort: :none }.
       #   Be aware that #first/#last won't guarantee order in this case.
       #
-      # @param [ Hash ] opts The options for the query returning the first document.
+      # @param [ Integer | Hash ] limit_or_opts The number of documents to
+      #   return, or a hash of options.
       #
-      # @option opts [ :none ] :id_sort Don't apply a sort on _id if no other sort
-      #   is defined on the criteria.
-      def last(opts = {})
-        try_cache(:last) do
-          with_inverse_sorting(opts) do
-            if raw_doc = view.limit(1).first
-              doc = Factory.from_db(klass, raw_doc, criteria)
-              eager_load([doc]).first
+      # @option limit_or_opts [ :none ] :id_sort This option is deprecated.
+      #   Don't apply a sort on _id if no other sort is defined on the criteria.
+      #
+      # @return [ Document ] The last document.
+      def last(limit_or_opts = nil)
+        limit = limit_or_opts unless limit_or_opts.is_a?(Hash)
+        if cached? && cache_loaded?
+          return limit ? documents.last(limit) : documents.last
+        end
+        res = try_numbered_cache(:last, limit) do
+          with_inverse_sorting(limit_or_opts) do
+            if raw_docs = view.limit(limit || 1).to_a
+              process_raw_docs(raw_docs, limit)
             end
           end
         end
+        res.is_a?(Array) ? res.reverse : res
       end
 
       # Returns the number of documents in the database matching
@@ -644,6 +654,31 @@ def try_cache(key, &block)
         end
       end
 
+      # yield the block given or return the cached value
+      #
+      # @param [ String, Symbol ] key The instance variable name
+      # @param [ Integer | nil ] n The number of documents requested or nil
+      #   if none is requested.
+      #
+      # @return [ Object ] The result of the block.
+      def try_numbered_cache(key, n, &block)
+        unless cached?
+          yield if block_given?
+        else
+          len = n || 1
+          ret = instance_variable_get("@#{key}")
+          if !ret || ret.length < len
+            instance_variable_set("@#{key}", ret = Array.wrap(yield))
+          elsif !n
+            ret.is_a?(Array) ? ret.first : ret
+          elsif ret.length > len
+            ret.first(n)
+          else
+            ret
+          end
+        end
+      end
+
       # Update the documents for the provided method.
       #
       # @api private
@@ -708,8 +743,10 @@ def apply_option(name)
       # @example Apply the inverse sorting params to the given block
       #   context.with_inverse_sorting
       def with_inverse_sorting(opts = {})
+        Mongoid::Warnings.warn_id_sort_deprecated if opts.try(:key?, :id_sort)
+
         begin
-          if sort = criteria.options[:sort] || ( { _id: 1 } unless opts[:id_sort] == :none )
+          if sort = criteria.options[:sort] || ( { _id: 1 } unless opts.try(:fetch, :id_sort) == :none )
             @view = view.sort(Hash[sort.map{|k, v| [k, -1*v]}])
           end
           yield
@@ -917,6 +954,18 @@ def demongoize_with_field(field, value, is_translation)
           value.class.demongoize(value)
         end
       end
+
+      # Process the raw documents retrieved for #first/#last.
+      #
+      # @return [ Array<Document> | Document ] The list of documents or a
+      #   single document.
+      def process_raw_docs(raw_docs, limit)
+        docs = raw_docs.map do |d|
+          Factory.from_db(klass, d, criteria)
+        end
+        docs = eager_load(docs)
+        limit ? docs : docs.first
+      end
     end
   end
 end

lib/mongoid/contextual/none.rb

@@ -117,13 +117,35 @@ def initialize(criteria)
         @criteria, @klass = criteria, criteria.klass
       end
 
+      # Always returns nil.
+      #
+      # @example Get the first document in null context.
+      #   context.first
+      #
+      # @param [ Integer | Hash ] limit_or_opts The number of documents to
+      #   return, or a hash of options.
+      #
+      # @return [ nil ] Always nil.
+      def first(limit_or_opts = nil)
+        if !limit_or_opts.nil? && !limit_or_opts.is_a?(Hash)
+          []
+        end
+      end
+
       # Always returns nil.
       #
       # @example Get the last document in null context.
       #   context.last
       #
+      # @param [ Integer | Hash ] limit_or_opts The number of documents to
+      #   return, or a hash of options.
+      #
       # @return [ nil ] Always nil.
-      def last; nil; end
+      def last(limit_or_opts = nil)
+        if !limit_or_opts.nil? && !limit_or_opts.is_a?(Hash)
+          []
+        end
+      end
 
       # Returns nil or empty array.
       #

lib/mongoid/findable.rb

@@ -194,9 +194,15 @@ def find_by!(attrs = {})
     # @example Find the first document.
     #   Person.first
     #
+    # @param [ Integer | Hash ] limit_or_opts The number of documents to
+    #   return, or a hash of options.
+    #
+    # @option limit_or_opts [ :none ] :id_sort This option is deprecated.
+    #   Don't apply a sort on _id if no other sort is defined on the criteria.
+    #
     # @return [ Document ] The first matching document.
-    def first
-      with_default_scope.first
+    def first(limit_or_opts = nil)
+      with_default_scope.first(limit_or_opts)
     end
     alias :one :first
 
@@ -205,9 +211,15 @@ def first
     # @example Find the last document.
     #   Person.last
     #
+    # @param [ Integer | Hash ] limit_or_opts The number of documents to
+    #   return, or a hash of options.
+    #
+    # @option limit_or_opts [ :none ] :id_sort This option is deprecated.
+    #   Don't apply a sort on _id if no other sort is defined on the criteria.
+    #
     # @return [ Document ] The last matching document.
-    def last
-      with_default_scope.last
+    def last(limit_or_opts = nil)
+      with_default_scope.last(limit_or_opts)
     end
   end
 end

lib/mongoid/warnings.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module Mongoid
+
+  # Encapsulates behavior around logging and caching warnings so they are only
+  # logged once.
+  #
+  # @api private
+  module Warnings
+
+    class << self
+      def warning(id, message)
+        singleton_class.class_eval do
+          define_method("warn_#{id}") do
+            unless instance_variable_get("@#{id}")
+              Mongoid.logger.warn(message)
+              instance_variable_set("@#{id}", true)
+            end
+          end
+        end
+      end
+    end
+
+    warning :id_sort_deprecated, 'The :id_sort option has been deprecated. Use Mongo#take to get a document without a sort on _id.'
+  end
+end
+