Prepare for pymongo 4 upgrade #60
Conversation
…and 'safe' args to them
…th list_database_names() and list_collection_names()
dill0wn
force-pushed
the
dw/pymongo4-prep
branch
from
d9b4cbe
to
6d0ee6e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #60 +/- ##
==========================================
+ Coverage 90.19% 91.11% +0.91%
==========================================
Files 43 43
Lines 6724 6682 -42
==========================================
+ Hits 6065 6088 +23
+ Misses 659 594 -65
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -358,7 +359,7 @@ def _ensure_indexes(self): | |||
| try: | |||
| with self._lock: | |||
| for idx in self.manager.indexes: | |||
| collection.ensure_index(idx.index_spec, background=True, | |||
| collection.create_index(idx.index_spec, background=True, | |||
There was a problem hiding this comment.
https://pymongo.readthedocs.io/en/stable/migrate-to-pymongo4.html#collection-ensure-index-is-removed recommends we should avoid frequent calls to ensure_index Are we okay because of the self.indexes_ensured flag? Seems like it
| def replace_one(self, filter, replacement, upsert=False): | ||
| return self._update(filter, replacement, upsert) | ||
| return self.__update(filter, replacement, upsert) | ||
|
|
||
| def __update(self, spec, updates, upsert=False, multi=False): |
There was a problem hiding this comment.
def __update returns a dict with some values like err n nModified etc. That is old. Now pymongo returns a UpdateResult https://pymongo.readthedocs.io/en/stable/api/pymongo/results.html#pymongo.results.UpdateResult
MIM should be updated to match
Ming could be an abstraction layer and return the old format, but I think there's probably not that many places that use the old result dict, and we could require them to be updated.
ming/session.py
Outdated
| data = {arg: data[arg] for arg in args} | ||
| result = self._impl(doc).update_one( | ||
| dict(_id=doc._id), {'$set': data}, | ||
| upsert=True, **fix_write_concern(kwargs) |
There was a problem hiding this comment.
the old update() call didn't pass upsert=True, do we need it here?
There was a problem hiding this comment.
My thinking was that we probably don't ever want to "fail" on a save. We probably always want to just... do the save. An upsert guarantees that. At least that was my thinking. I wasn't necessarily thinking about how we're currently using this fn, more about conceptually how I would expect it to work as a user.
…pdate/replace/delete
…pdate/replace/delete
…e_and_update/replace/delete
| matched_count=0, | ||
| modified_count=0, | ||
| raw_result=None, | ||
| upsert_id=None, | ||
| ) |
There was a problem hiding this comment.
seems like this should be an UpdateResult instance rather than a dict? Although you have some comments about backwards-compatibillity with ['n'] Maybe it won't be possible to be backwards compatible if we want to match the new return type.
| self._deindex(doc) | ||
| else: | ||
| new_data[id] = doc | ||
| self._data = new_data | ||
| # TODO: this is needed for backcompat, but can drop 'n' in SF-9544 | ||
| result['n'] = result['deleted_count'] | ||
| return result |
There was a problem hiding this comment.
should this be DeleteResult rather than a dict?
This was motivated by this issue #48 requesting pymongo v4 support. This PR lays the groundwork for the upgrade to pymongo4, but it stops short of actually bumping the package.
I followed pymongo's migration guide to see what was removed or deprecated then updated everything relevant in Ming. pymongo has backported many of their API changes to pymongo3 therefore I think this work should be representative.
That said, I'm sure more issues will crop up once we actually update to pymongo4, but that can be a separate chunk of work.