From ba4de2f933d693e8cae771b8b7755a26700ea11d Mon Sep 17 00:00:00 2001 From: Giovanni Cimolin da Silva Date: Tue, 23 Jul 2019 17:50:54 -0300 Subject: [PATCH] Upgrade pymongo and fix issues This commit upgrades the version of pymongo from 2.x to 3.x, removing usages to deprecated functions usage and fixing tests where necessary. This version of pymongo supports MongoDB 2.x all the way up to 4.2, and this ensures that the platform will be able to run on a supported MongoDB version in the next release. --- .../lib/xmodule/xmodule/contentstore/mongo.py | 25 ++++++++++--------- .../xmodule/xmodule/course_metadata_utils.py | 2 +- .../xmodule/xmodule/modulestore/mongo/base.py | 23 +++++++---------- .../xmodule/modulestore/mongo/draft.py | 6 ++--- .../split_mongo/mongo_connection.py | 16 +++++------- .../xmodule/modulestore/split_mongo/split.py | 6 ----- .../xmodule/modulestore/tests/factories.py | 10 +++----- .../tests/test_mongo_call_count.py | 6 ++--- .../test_split_mongo_mongo_connection.py | 3 ++- common/lib/xmodule/xmodule/mongo_utils.py | 23 ++++++++++++++--- lms/djangoapps/dashboard/git_import.py | 2 +- lms/djangoapps/dashboard/sysadmin.py | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/paver.in | 2 +- requirements/edx/paver.txt | 2 +- requirements/edx/testing.txt | 2 +- 17 files changed, 67 insertions(+), 67 deletions(-) diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index ae747f97fa..1bc89f5cec 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/common/lib/xmodule/xmodule/contentstore/mongo.py @@ -56,7 +56,7 @@ class MongoContentStore(ContentStore): """ Closes any open connections to the underlying databases """ - self.fs_files.database.connection.close() + self.fs_files.database.client.close() def _drop_database(self, database=True, collections=True, connections=True): """ @@ -70,10 +70,10 @@ class MongoContentStore(ContentStore): If connections is True, then close the connection to the database as well. """ - connection = self.fs_files.database.connection + connection = self.fs_files.database.client if database: - connection.drop_database(self.fs_files.database) + connection.drop_database(self.fs_files.database.name) elif collections: self.fs_files.drop() self.chunks.drop() @@ -320,15 +320,16 @@ class MongoContentStore(ContentStore): } }) - items = self.fs_files.aggregate(pipeline_stages) - if items['result']: - result = items['result'][0] - count = result['count'] - assets = list(result['results']) - else: - # no results - count = 0 - assets = [] + cursor = self.fs_files.aggregate(pipeline_stages) + # Set values if result of query is empty + count = 0 + assets = [] + + if cursor.alive: + result = cursor.next() + if result: + count = result['count'] + assets = list(result['results']) # We're constructing the asset key immediately after retrieval from the database so that # callers are insulated from knowing how our identifiers are stored. diff --git a/common/lib/xmodule/xmodule/course_metadata_utils.py b/common/lib/xmodule/xmodule/course_metadata_utils.py index c862356513..3ae03b3418 100644 --- a/common/lib/xmodule/xmodule/course_metadata_utils.py +++ b/common/lib/xmodule/xmodule/course_metadata_utils.py @@ -201,7 +201,7 @@ def sorting_dates(start, advertised_start, announcement): start = dateutil.parser.parse(advertised_start) if start.tzinfo is None: start = start.replace(tzinfo=utc) - except (ValueError, AttributeError): + except (TypeError, ValueError, AttributeError): start = start now = datetime.now(utc) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index bd3c5b5264..ede5b88e33 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -603,14 +603,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo """ Closes any open connections to the underlying database """ - self.collection.database.connection.close() - - def mongo_wire_version(self): - """ - Returns the wire version for mongo. Only used to unit tests which instrument the connection. - """ - self.database.connection._ensure_connected() - return self.database.connection.max_wire_version + self.collection.database.client.close() def _drop_database(self, database=True, collections=True, connections=True): """ @@ -627,14 +620,14 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo # drop the assets super(MongoModuleStore, self)._drop_database(database, collections, connections) - connection = self.collection.database.connection + connection = self.collection.database.client if database: connection.drop_database(self.collection.database.proxied_object) elif collections: self.collection.drop() else: - self.collection.remove({}) + self.collection.delete_many({}) if connections: connection.close() @@ -1913,7 +1906,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo """ source_assets = self._find_course_assets(source_course_key) dest_assets = {'assets': source_assets.asset_md.copy(), 'course_id': six.text_type(dest_course_key)} - self.asset_collection.remove({'course_id': six.text_type(dest_course_key)}) + self.asset_collection.delete_many({'course_id': six.text_type(dest_course_key)}) # Update the document. self.asset_collection.insert(dest_assets) @@ -1985,7 +1978,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo # A single document exists per course to store the course asset metadata. try: course_assets = self._find_course_assets(course_key) - self.asset_collection.remove(course_assets.doc_id) + self.asset_collection.delete_many({'_id': course_assets.doc_id}) except ItemNotFoundError: # When deleting asset metadata, if a course's asset metadata is not present, no big deal. pass @@ -1994,9 +1987,11 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo """ Check that the db is reachable. """ - if self.database.connection.alive(): + try: + # The ismaster command is cheap and does not require auth. + self.database.client.admin.command('ismaster') return {ModuleStoreEnum.Type.mongo: True} - else: + except pymongo.errors.ConnectionFailure: raise HeartbeatFailure("Can't connect to {}".format(self.database.name), 'mongo') def ensure_indexes(self): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 9a264fae1d..2f746f9932 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -178,7 +178,7 @@ class DraftModuleStore(MongoModuleStore): # delete all of the db records for the course course_query = self._course_key_to_son(course_key) - self.collection.remove(course_query, multi=True) + self.collection.delete_many(course_query) self.delete_all_asset_metadata(course_key, user_id) self._emit_course_deleted_signal(course_key) @@ -651,7 +651,7 @@ class DraftModuleStore(MongoModuleStore): if len(to_be_deleted) > 0: bulk_record = self._get_bulk_ops_record(root_usages[0].course_key) bulk_record.dirty = True - self.collection.remove({'_id': {'$in': to_be_deleted}}, safe=self.collection.safe) + self.collection.delete_many({'_id': {'$in': to_be_deleted}}) def has_changes(self, xblock): """ @@ -766,7 +766,7 @@ class DraftModuleStore(MongoModuleStore): bulk_record = self._get_bulk_ops_record(course_key) if len(to_be_deleted) > 0: bulk_record.dirty = True - self.collection.remove({'_id': {'$in': to_be_deleted}}) + self.collection.delete_many({'_id': {'$in': to_be_deleted}}) self._flag_publish_event(course_key) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index cdfd854d09..ec456b85c1 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -296,9 +296,11 @@ class MongoConnection(object): """ Check that the db is reachable. """ - if self.database.connection.alive(): + try: + # The ismaster command is cheap and does not require auth. + self.database.client.admin.command('ismaster') return True - else: + except pymongo.errors.ConnectionFailure: raise HeartbeatFailure("Can't connect to {}".format(self.database.name), 'mongo') def get_structure(self, key, course_context=None): @@ -589,13 +591,7 @@ class MongoConnection(object): """ Closes any open connections to the underlying databases """ - self.database.connection.close() - - def mongo_wire_version(self): - """ - Returns the wire version for mongo. Only used to unit tests which instrument the connection. - """ - return self.database.connection.max_wire_version + self.database.client.close() def _drop_database(self, database=True, collections=True, connections=True): """ @@ -609,7 +605,7 @@ class MongoConnection(object): If connections is True, then close the connection to the database as well. """ - connection = self.database.connection + connection = self.database.client if database: connection.drop_database(self.database.name) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index bd9e387865..aa7e15c699 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -751,12 +751,6 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): """ self.db_connection.close_connections() - def mongo_wire_version(self): - """ - Returns the wire version for mongo. Only used to unit tests which instrument the connection. - """ - return self.db_connection.mongo_wire_version() - def _drop_database(self, database=True, collections=True, connections=True): """ A destructive operation to drop the underlying database and close all connections. diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 531d4082a5..1c208014d1 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -610,8 +610,6 @@ def mongo_uses_error_check(store): """ Does mongo use the error check as a separate message? """ - if hasattr(store, 'mongo_wire_version'): - return store.mongo_wire_version() <= 1 if hasattr(store, 'modulestores'): return any([mongo_uses_error_check(substore) for substore in store.modulestores]) return False @@ -630,16 +628,16 @@ def check_mongo_calls_range(max_finds=float("inf"), min_finds=0, max_sends=None, :param min_sends: If non-none, make sure number of send calls are >=min_sends """ with check_sum_of_calls( - pymongo.message, - ['query', 'get_more'], + pymongo.collection.Collection, + ['find'], max_finds, min_finds, ): if max_sends is not None or min_sends is not None: with check_sum_of_calls( - pymongo.message, + pymongo.collection.Collection, # mongo < 2.6 uses insert, update, delete and _do_batched_insert. >= 2.6 _do_batched_write - ['insert', 'update', 'delete', '_do_batched_write_command', '_do_batched_insert', ], + ['insert', 'update', 'bulk_write', '_delete'], max_sends if max_sends is not None else float("inf"), min_sends if min_sends is not None else 0, ): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py index ebc7b4b852..a86f1df952 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py @@ -155,11 +155,11 @@ class CountMongoCallsCourseTraversal(TestCase): (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 0, True, False, 359), # The line below shows the way this traversal *should* be done # (if you'll eventually access all the fields and load all the definitions anyway). - (MIXED_SPLIT_MODULESTORE_BUILDER, None, False, True, 4), + (MIXED_SPLIT_MODULESTORE_BUILDER, None, False, True, 3), (MIXED_SPLIT_MODULESTORE_BUILDER, None, True, True, 38), (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, True, 38), (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, True, 38), - (MIXED_SPLIT_MODULESTORE_BUILDER, None, False, False, 4), + (MIXED_SPLIT_MODULESTORE_BUILDER, None, False, False, 3), (MIXED_SPLIT_MODULESTORE_BUILDER, None, True, False, 3), (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, False, 3), (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, False, 3) @@ -179,7 +179,7 @@ class CountMongoCallsCourseTraversal(TestCase): @ddt.data( (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 176), - (MIXED_SPLIT_MODULESTORE_BUILDER, 5), + (MIXED_SPLIT_MODULESTORE_BUILDER, 4), ) @ddt.unpack def test_lazy_when_course_previously_cached(self, store_builder, num_mongo_calls): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py index c66d4a4997..e89cf15ad4 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py @@ -4,6 +4,7 @@ from __future__ import absolute_import import unittest from mock import patch +from pymongo.errors import ConnectionFailure from xmodule.exceptions import HeartbeatFailure from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection @@ -17,7 +18,7 @@ class TestHeartbeatFailureException(unittest.TestCase): def test_heartbeat_raises_exception_when_connection_alive_is_false(self, *calls): # pylint: disable=W0613 with patch('mongodb_proxy.MongoProxy') as mock_proxy: - mock_proxy.return_value.alive.return_value = False + mock_proxy.return_value.admin.command.side_effect = ConnectionFailure('Test') useless_conn = MongoConnection('useless', 'useless', 'useless') with self.assertRaises(HeartbeatFailure): diff --git a/common/lib/xmodule/xmodule/mongo_utils.py b/common/lib/xmodule/xmodule/mongo_utils.py index bb70697f5e..b6d5d414f9 100644 --- a/common/lib/xmodule/xmodule/mongo_utils.py +++ b/common/lib/xmodule/xmodule/mongo_utils.py @@ -7,10 +7,19 @@ import logging import pymongo from mongodb_proxy import MongoProxy -from pymongo import ReadPreference +from pymongo.read_preferences import ( + ReadPreference, + read_pref_mode_from_name, + _MONGOS_MODES, + _MODES +) + logger = logging.getLogger(__name__) # pylint: disable=invalid-name +# This will yeld a map of all available Mongo modes and their name +MONGO_READ_PREFERENCE_MAP = dict(zip(_MONGOS_MODES, _MODES)) + # pylint: disable=bad-continuation def connect_to_mongodb( @@ -39,10 +48,16 @@ def connect_to_mongodb( # If the MongoDB server uses a separate authentication database that should be specified here auth_source = kwargs.pop('auth_source', '') or None - # If read_preference is given as a name of a valid ReadPreference. constant - # such as "SECONDARY_PREFERRED", convert it. Otherwise pass it through unchanged. + # If read_preference is given as a name of a valid ReadPreference. + # constant such as "SECONDARY_PREFERRED" or a mongo mode such as + # "secondaryPreferred", convert it. Otherwise pass it through unchanged. if 'read_preference' in kwargs: - read_preference = getattr(ReadPreference, kwargs['read_preference'], None) + read_preference = MONGO_READ_PREFERENCE_MAP.get( + kwargs['read_preference'], + kwargs['read_preference'] + ) + + read_preference = getattr(ReadPreference, read_preference, None) if read_preference is not None: kwargs['read_preference'] = read_preference diff --git a/lms/djangoapps/dashboard/git_import.py b/lms/djangoapps/dashboard/git_import.py index a888f7374c..1d4cf7ba25 100644 --- a/lms/djangoapps/dashboard/git_import.py +++ b/lms/djangoapps/dashboard/git_import.py @@ -350,4 +350,4 @@ def add_repo(repo, rdir_in, branch=None): cil.save() log.debug(u'saved CourseImportLog for %s', cil.course_id) - mdb.disconnect() + mdb.close() diff --git a/lms/djangoapps/dashboard/sysadmin.py b/lms/djangoapps/dashboard/sysadmin.py index f09b6b311e..45b1d5a8ca 100644 --- a/lms/djangoapps/dashboard/sysadmin.py +++ b/lms/djangoapps/dashboard/sysadmin.py @@ -553,7 +553,7 @@ class GitLogs(TemplateView): page = min(max(1, given_page), paginator.num_pages) logs = paginator.page(page) - mdb.disconnect() + mdb.close() context = { 'logs': logs, 'course_id': text_type(course_id) if course_id else None, diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 35696a3f3f..877353d6c7 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -192,7 +192,7 @@ pygments==2.5.2 pygraphviz==1.5 pyjwkest==1.3.2 pyjwt==1.5.2 -pymongo==2.9.1 +pymongo==3.9.0 pynliner==0.8.0 pyparsing==2.2.0 # via pycontracts pysrt==1.1.1 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index f68f340a62..55d4153085 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -255,7 +255,7 @@ pylint-celery==0.3 pylint-django==0.7.2 pylint-plugin-utils==0.3 pylint==1.7.6 -pymongo==2.9.1 +pymongo==3.9.0 pynliner==0.8.0 pyparsing==2.2.0 pyquery==1.4.1 diff --git a/requirements/edx/paver.in b/requirements/edx/paver.in index a8fb579c86..e0e7d05b03 100644 --- a/requirements/edx/paver.in +++ b/requirements/edx/paver.in @@ -18,7 +18,7 @@ mock # Stub out code with mock objects and make a path.py==8.2.1 # Easier manipulation of filesystem paths paver # Build, distribution and deployment scripting tool psutil==1.2.1 # Library for retrieving information on running processes and system utilization -pymongo==2.9.1 # via edx-opaque-keys +pymongo==3.9.0 # via edx-opaque-keys python-memcached # Python interface to the memcached memory cache daemon requests # Simple interface for making HTTP requests stevedore # Support for runtime plugins, used for XBlocks and edx-platform Django app plugins diff --git a/requirements/edx/paver.txt b/requirements/edx/paver.txt index 47f048fc7f..c02248a221 100644 --- a/requirements/edx/paver.txt +++ b/requirements/edx/paver.txt @@ -19,7 +19,7 @@ pathtools==0.1.2 # via watchdog paver==1.3.4 pbr==5.4.4 # via stevedore psutil==1.2.1 -pymongo==2.9.1 +pymongo==3.9.0 python-memcached==1.59 pyyaml==5.2 # via watchdog requests==2.22.0 diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index d474a71397..d9419c7e5b 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -244,7 +244,7 @@ pylint-celery==0.3 # via edx-lint pylint-django==0.7.2 # via edx-lint pylint-plugin-utils==0.3 # via pylint-celery, pylint-django pylint==1.7.6 # via edx-lint, pylint-celery, pylint-django -pymongo==2.9.1 +pymongo==3.9.0 pynliner==0.8.0 pyparsing==2.2.0 pyquery==1.4.1