PYTHON-1811 Deprecate running min/max queries without hint

Starting in MongoDB 4.2 a hint will be required when using min/max.

Author: ShaneHarvey

doc/changelog.rst

@@ -89,6 +89,11 @@ Changes in Version 3.8.0.dev0
   :meth:`pymongo.change_stream.ChangeStream.try_next` method.
 - Fixed a reference leak bug when splitting a batched write command based on
   maxWriteBatchSize or the max message size.
+- Deprecated running find queries that set :meth:`~pymongo.cursor.Cursor.min`
+  and/or :meth:`~pymongo.cursor.Cursor.max` but do not also set a
+  :meth:`~pymongo.cursor.Cursor.hint` of which index to use. The find command
+  is expected to require a :meth:`~pymongo.cursor.Cursor.hint` when using
+  min/max starting in MongoDB 4.2.
 
 Issues Resolved
 ...............

pymongo/collection.py

@@ -1368,11 +1368,13 @@ def find(self, *args, **kwargs):
           - `min` (optional): A list of field, limit pairs specifying the
             inclusive lower bound for all keys of a specific index in order.
             Pass this as an alternative to calling
-            :meth:`~pymongo.cursor.Cursor.min` on the cursor.
+            :meth:`~pymongo.cursor.Cursor.min` on the cursor. ``hint`` must
+            also be passed to ensure the query utilizes the correct index.
           - `max` (optional): A list of field, limit pairs specifying the
             exclusive upper bound for all keys of a specific index in order.
             Pass this as an alternative to calling
-            :meth:`~pymongo.cursor.Cursor.max` on the cursor.
+            :meth:`~pymongo.cursor.Cursor.max` on the cursor. ``hint`` must
+            also be passed to ensure the query utilizes the correct index.
           - `comment` (optional): A string to attach to the query to help
             interpret and trace the operation in the server logs and in profile
             data. Pass this as an alternative to calling

pymongo/cursor.py

@@ -634,12 +634,19 @@ def max_scan(self, max_scan):
         return self
 
     def max(self, spec):
-        """Adds `max` operator that specifies upper bound for specific index.
+        """Adds ``max`` operator that specifies upper bound for specific index.
+
+        When using ``max``, :meth:`~hint` should also be configured to ensure
+        the query uses the expected index and starting in MongoDB 4.2
+        :meth:`~hint` will be required.
 
         :Parameters:
           - `spec`: a list of field, limit pairs specifying the exclusive
             upper bound for all keys of a specific index in order.
 
+        .. versionchanged:: 3.8
+           Deprecated cursors that use ``max`` without a :meth:`~hint`.
+
         .. versionadded:: 2.7
         """
         if not isinstance(spec, (list, tuple)):
@@ -650,12 +657,19 @@ def max(self, spec):
         return self
 
     def min(self, spec):
-        """Adds `min` operator that specifies lower bound for specific index.
+        """Adds ``min`` operator that specifies lower bound for specific index.
+
+        When using ``min``, :meth:`~hint` should also be configured to ensure
+        the query uses the expected index and starting in MongoDB 4.2
+        :meth:`~hint` will be required.
 
         :Parameters:
           - `spec`: a list of field, limit pairs specifying the inclusive
             lower bound for all keys of a specific index in order.
 
+        .. versionchanged:: 3.8
+           Deprecated cursors that use ``min`` without a :meth:`~hint`.
+
         .. versionadded:: 2.7
         """
         if not isinstance(spec, (list, tuple)):
@@ -1095,6 +1109,12 @@ def _refresh(self):
             self.__session = self.__collection.database.client._ensure_session()
 
         if self.__id is None:  # Query
+            if (self.__min or self.__max) and not self.__hint:
+                warnings.warn("using a min/max query operator without "
+                              "specifying a Cursor.hint is deprecated. A "
+                              "hint will be required when using min/max in "
+                              "PyMongo 4.0",
+                              DeprecationWarning, stacklevel=3)
             q = self._query_class(self.__query_flags,
                                   self.__collection.database.name,
                                   self.__collection.name,

test/__init__.py

@@ -637,6 +637,12 @@ def supports_getpreverror(self):
         """Does the connected server support getpreverror?"""
         return not (self.version.at_least(4, 1, 0) or self.is_mongos)
 
+    @property
+    def requires_hint_with_min_max_queries(self):
+        """Does the server require a hint with min/max queries."""
+        # Changed in SERVER-39567.
+        return self.version.at_least(4, 1, 10)
+
 
 # Reusable client context
 client_context = ClientContext()

test/test_collection.py

@@ -2000,10 +2000,13 @@ def test_min_query(self):
         self.db.test.insert_many([{"x": 1}, {"x": 2}])
         self.db.test.create_index("x")
 
-        self.assertEqual(1, len(list(self.db.test.find({"$min": {"x": 2},
-                                                        "$query": {}}))))
-        self.assertEqual(2, self.db.test.find({"$min": {"x": 2},
-                                               "$query": {}})[0]["x"])
+        cursor = self.db.test.find({"$min": {"x": 2}, "$query": {}})
+        if client_context.requires_hint_with_min_max_queries:
+            cursor = cursor.hint("x_1")
+
+        docs = list(cursor)
+        self.assertEqual(1, len(docs))
+        self.assertEqual(2, docs[0]["x"])
 
     def test_numerous_inserts(self):
         # Ensure we don't exceed server's 1000-document batch size limit.

test/test_cursor.py

@@ -21,6 +21,7 @@
 import sys
 import time
 import threading
+import warnings
 
 sys.path[0:0] = [""]
 
@@ -449,66 +450,102 @@ def test_limit(self):
             break
         self.assertRaises(InvalidOperation, a.limit, 5)
 
+    @ignore_deprecations  # Ignore max without hint.
     def test_max(self):
         db = self.db
         db.test.drop()
-        db.test.create_index([("j", ASCENDING)])
+        j_index = [("j", ASCENDING)]
+        db.test.create_index(j_index)
 
         db.test.insert_many([{"j": j, "k": j} for j in range(10)])
 
-        cursor = db.test.find().max([("j", 3)])
+        def find(max_spec, expected_index):
+            cursor = db.test.find().max(max_spec)
+            if client_context.requires_hint_with_min_max_queries:
+                cursor = cursor.hint(expected_index)
+            return cursor
+
+        cursor = find([("j", 3)], j_index)
         self.assertEqual(len(list(cursor)), 3)
 
         # Tuple.
-        cursor = db.test.find().max((("j", 3), ))
+        cursor = find((("j", 3),), j_index)
         self.assertEqual(len(list(cursor)), 3)
 
         # Compound index.
-        db.test.create_index([("j", ASCENDING), ("k", ASCENDING)])
-        cursor = db.test.find().max([("j", 3), ("k", 3)])
+        index_keys = [("j", ASCENDING), ("k", ASCENDING)]
+        db.test.create_index(index_keys)
+        cursor = find([("j", 3), ("k", 3)], index_keys)
         self.assertEqual(len(list(cursor)), 3)
 
         # Wrong order.
-        cursor = db.test.find().max([("k", 3), ("j", 3)])
+        cursor = find([("k", 3), ("j", 3)], index_keys)
         self.assertRaises(OperationFailure, list, cursor)
 
         # No such index.
-        cursor = db.test.find().max([("k", 3)])
+        cursor = find([("k", 3)], "k")
         self.assertRaises(OperationFailure, list, cursor)
 
         self.assertRaises(TypeError, db.test.find().max, 10)
         self.assertRaises(TypeError, db.test.find().max, {"j": 10})
 
+    @ignore_deprecations  # Ignore min without hint.
     def test_min(self):
         db = self.db
         db.test.drop()
-        db.test.create_index([("j", ASCENDING)])
+        j_index = [("j", ASCENDING)]
+        db.test.create_index(j_index)
 
         db.test.insert_many([{"j": j, "k": j} for j in range(10)])
 
-        cursor = db.test.find().min([("j", 3)])
+        def find(min_spec, expected_index):
+            cursor = db.test.find().min(min_spec)
+            if client_context.requires_hint_with_min_max_queries:
+                cursor = cursor.hint(expected_index)
+            return cursor
+
+        cursor = find([("j", 3)], j_index)
         self.assertEqual(len(list(cursor)), 7)
 
         # Tuple.
-        cursor = db.test.find().min((("j", 3), ))
+        cursor = find((("j", 3),), j_index)
         self.assertEqual(len(list(cursor)), 7)
 
         # Compound index.
-        db.test.create_index([("j", ASCENDING), ("k", ASCENDING)])
-        cursor = db.test.find().min([("j", 3), ("k", 3)])
+        index_keys = [("j", ASCENDING), ("k", ASCENDING)]
+        db.test.create_index(index_keys)
+        cursor = find([("j", 3), ("k", 3)], index_keys)
         self.assertEqual(len(list(cursor)), 7)
 
         # Wrong order.
-        cursor = db.test.find().min([("k", 3), ("j", 3)])
+        cursor = find([("k", 3), ("j", 3)], index_keys)
         self.assertRaises(OperationFailure, list, cursor)
 
         # No such index.
-        cursor = db.test.find().min([("k", 3)])
+        cursor = find([("k", 3)], "k")
         self.assertRaises(OperationFailure, list, cursor)
 
         self.assertRaises(TypeError, db.test.find().min, 10)
         self.assertRaises(TypeError, db.test.find().min, {"j": 10})
 
+    @client_context.require_version_max(4, 1, -1)
+    def test_min_max_without_hint(self):
+        coll = self.db.test
+        j_index = [("j", ASCENDING)]
+        coll.create_index(j_index)
+
+        with warnings.catch_warnings(record=True) as warns:
+            warnings.simplefilter("default", DeprecationWarning)
+            list(coll.find().min([("j", 3)]))
+            self.assertIn('using a min/max query operator', str(warns[0]))
+            # Ensure the warning is raised with the proper stack level.
+            del warns[:]
+            list(coll.find().min([("j", 3)]))
+            self.assertIn('using a min/max query operator', str(warns[0]))
+            del warns[:]
+            list(coll.find().max([("j", 3)]))
+            self.assertIn('using a min/max query operator', str(warns[0]))
+
     def test_batch_size(self):
         db = self.db
         db.test.drop()