From 69353270f70b8204f478344e617bd34022e46e01 Mon Sep 17 00:00:00 2001 From: Shane Harvey Date: Fri, 12 Apr 2019 17:13:10 -0700 Subject: [PATCH] PYTHON-1811 Deprecate running min/max queries without hint Starting in MongoDB 4.2 a hint will be required when using min/max. (cherry picked from commit ea8941ef5d3f60a227cb89021ef7d65d7b06c6e1) --- doc/changelog.rst | 5 ++++ pymongo/collection.py | 6 ++-- pymongo/cursor.py | 24 +++++++++++++-- test/__init__.py | 6 ++++ test/test_collection.py | 11 ++++--- test/test_cursor.py | 65 ++++++++++++++++++++++++++++++++--------- 6 files changed, 95 insertions(+), 22 deletions(-) diff --git a/doc/changelog.rst b/doc/changelog.rst index f3b32a91e..0cc80bb54 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -47,6 +47,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 ............... diff --git a/pymongo/collection.py b/pymongo/collection.py index 0f7fe5368..cd9ba9070 100644 --- a/pymongo/collection.py +++ b/pymongo/collection.py @@ -1367,11 +1367,13 @@ class Collection(common.BaseObject): - `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 diff --git a/pymongo/cursor.py b/pymongo/cursor.py index b1b89a7e7..ee8b0255e 100644 --- a/pymongo/cursor.py +++ b/pymongo/cursor.py @@ -634,12 +634,19 @@ class Cursor(object): 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 @@ class Cursor(object): 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 @@ class Cursor(object): 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, diff --git a/test/__init__.py b/test/__init__.py index 975c355bb..49030017f 100644 --- a/test/__init__.py +++ b/test/__init__.py @@ -604,6 +604,12 @@ class ClientContext(object): """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() diff --git a/test/test_collection.py b/test/test_collection.py index 1bffa19de..2f3358e9b 100644 --- a/test/test_collection.py +++ b/test/test_collection.py @@ -2000,10 +2000,13 @@ class TestCollection(IntegrationTest): 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. diff --git a/test/test_cursor.py b/test/test_cursor.py index ce85a1bd3..9b456d051 100644 --- a/test/test_cursor.py +++ b/test/test_cursor.py @@ -21,6 +21,7 @@ import re import sys import time import threading +import warnings sys.path[0:0] = [""] @@ -449,66 +450,102 @@ class TestCursor(IntegrationTest): 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()