Migrate motor to pymongo#1705
Conversation
✔️ Changelog found.Thank you for adding a description of the changes |
JrooTJunior
left a comment
JrooTJunior
left a comment
There was a problem hiding this comment.
This is a very small but critical change for developers using this type of storage, as it could break code until the developer rewrites other code that uses Mongo. Also, using multiple drivers for the same purposes can be too expensive.
So, the best way to update this storage is to mark the current implementation as deprecated and add a new storage implementation with a name like PyMongoStorage, and provide migration guidance in the changelog and documentation articles (storages section).
…ition in the previous line
…using update_data method
There was a problem hiding this comment.
Pull Request Overview
This PR migrates MongoDB storage from the deprecated motor package to the async PyMongo package, introducing a new PyMongoStorage class while deprecating the existing MongoStorage class.
- Introduces new
PyMongoStorageclass as replacement for deprecatedMongoStorage - Updates package dependencies to include
pymongo>=4.9.12 - Adds comprehensive test coverage for the new PyMongo storage implementation
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| aiogram/fsm/storage/pymongo.py | New PyMongoStorage class implementing async MongoDB storage using PyMongo |
| aiogram/fsm/storage/mongo.py | Adds deprecation warning to existing MongoStorage class |
| tests/test_fsm/storage/test_pymongo.py | Comprehensive test suite for PyMongoStorage functionality |
| tests/conftest.py | Adds pymongo_storage fixture for testing |
| tests/test_fsm/storage/test_storages.py | Includes pymongo_storage in storage tests |
| pyproject.toml | Adds pymongo dependency |
| docs/dispatcher/finite_state_machine/storages.rst | Documents new PyMongoStorage class |
| CHANGES/1705.misc.rst | Release notes for the migration |
| projection={"_id": 0}, | ||
| ) | ||
| if not update_result: | ||
| await self._collection.delete_one({"_id": document_id}) |
There was a problem hiding this comment.
The return value can be None when update_result is falsy, but the method signature expects Dict[str, Any]. This will cause a TypeError since None.get() will fail. Should return {} when update_result is falsy.
| await self._collection.delete_one({"_id": document_id}) | |
| await self._collection.delete_one({"_id": document_id}) | |
| return {} |
There was a problem hiding this comment.
Nothing returns in this branch, below, where actual value is returned, there is a fallback to {}
| projection={"_id": 0}, | ||
| ) | ||
| if not update_result: | ||
| await self._collection.delete_one({"_id": document_id}) |
There was a problem hiding this comment.
This condition will never be true because find_one_and_update with upsert=True will always return a document (either the existing one or the newly created one). The deletion logic is unreachable.
| await self._collection.delete_one({"_id": document_id}) |
There was a problem hiding this comment.
Actually it will be true. Because of projection={"_id": 0}. When there is no record in database and user tries to call update_data(key, data={}), update_result will be {}. So this condition will be true.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev-3.x #1705 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 578 579 +1
Lines 13531 13593 +62
=========================================
+ Hits 13531 13593 +62
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Some of the tests fails are transient, they should go on tests rerun. |
Vadim-Khristenko
left a comment
Vadim-Khristenko
left a comment
There was a problem hiding this comment.
Everything looks good — great work!
Just one suggestion: I recommend pinning the upper version of pymongo to <4.11 for now.
Why?
pymongo==4.10.1still supports PyPy 3.9.pymongo>=4.11drops support for PyPy 3.9 entirely.
Since we currently run tests on pypy3.9, upgrading beyond 4.10.1 would break CI.
Suggestion:
Let’s create an issue to track this and automatically upgrade the upper bound when CPython 3.9 reaches EOL (October 2025).
This way, we’ll be ready to safely bump pymongo without breaking builds.
Ready for final review from @JrooTJunior before merge.
Description
Migrated
MongoStoragefrom relying on deprecatedmotorpackage to using new asyncPyMongo. Also__init__.pyfile was changed inaiogram/storage/fsmto improve developer experience.Fixes #1695
Type of change
How Has This Been Tested?
It was tested utilizing method that is provided in contributing guide:
pytest --mongo mongodb://<user>:<password>@<host>:<port> testsTest Configuration:
Checklist: