Repository: Scille/umongo
Pull request #184 (2019-03-19 20:00:39)

Add count_documents support to motor asyncio Documents
Update API to support v2.0.0 of motor. This involves changing tests
that use the deprecated count() cursor method to use either
count_documents() or to_list() instead, and removing the callback
keyword argument from the to_list wrapper.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/22277982/badge)](https://coveralls.io/builds/22277982)

Coverage remained the same at 96.175% when pulling **2a4de514942afca6b8d1a0ad8165d1176389433f on wfscheper:motor-2.0** into **042477237ff707ea02df6fb562df140aa5afc84e on Scille:master**.

lafrech: I'm not familiar with motor and the async part of umongo, but I gave it a look and it looks like it would be easy to support both motor 1.x and motor 2.x with the same umongo codebase.

Dropping currently supported versions of motor would be a breaking change, but supporting new versions while keeping support for older ones would just be a minor change. We can drop motor 1.x in a later major version.

Do you think you could change your PR to keep compatibility with old motor versions?

Basically, that means

- use conditions when the code depends on the motor version (most of it would be in the tests)
- modify setup.py to read `['motor>=1.1,<3.0']`
- modify tox.ini to define motor1 and motor2 instead of just motor
- modify .travis.yml to test motor1 and motor2 (maybe we can test motor1 on python3.5 and motor2 on python 3.7 rather than all combinations, to limit CI load/time)

Thanks.
wfscheper: @lafrech, you're correct about it being easy to support both versions of motor, so I'm updating my pull request. 

I can see two ways to address this. One is that I don't change the public API of umongo and just make the changes you suggested. The other way is to add a new Document classmethod `count(filter, *, with_limit_and_skip=False, **kwargs)` that does the version handling for the consumer. What are your thoughts?

Patch:
diff --git a/.travis.yml b/.travis.yml
index d1265c6..7aab055 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -29,10 +29,10 @@ jobs:
 
     - { python: '3.7', env: TOXENV=lint }
     - { python: '3.5', env: TOXENV=py35-pymongo }
-    - { python: '3.5', env: TOXENV=py35-motor }
+    - { python: '3.5', env: TOXENV=py35-motor1 }
     - { python: '3.5', env: TOXENV=py35-txmongo }
     - { python: '3.7', env: TOXENV=py37-pymongo }
-    - { python: '3.7', env: TOXENV=py37-motor }
+    - { python: '3.7', env: TOXENV=py37-motor2 }
     - { python: '3.7', env: TOXENV=py37-txmongo }
 
     - stage: PyPI Release
diff --git a/HISTORY.rst b/HISTORY.rst
index b545f23..034dfbe 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -2,24 +2,6 @@
 History
 =======
 
-2.0.5 (2019-06-13)
-------------------
-
-Bug fixes:
-
-* Ensure ``Reference`` and ``GenericReference`` fields round-trip (see #200).
-
-2.0.4 (2019-05-28)
-------------------
-
-Bug fixes:
-
-* Include modified ``BaseDataObject`` in ``BaseDataProxy.get_modified_fields``
-  and ``BaseDataProxy.get_modified_fields_by_mongo_name`` (see #195).
-* Always return a boolean in ``List.is_modified`` (see #195).
-* ``List``: call ``set_modified`` when deleting an element using the ``del``
-  builtin (see #195).
-
 2.0.3 (2019-04-10)
 ------------------
 
diff --git a/setup.cfg b/setup.cfg
index 4aff281..e06bc38 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -1,5 +1,5 @@
 [bumpversion]
-current_version = 2.0.5
+current_version = 2.0.3
 commit = True
 tag = True
 
diff --git a/setup.py b/setup.py
index c6ed354..1850283 100755
--- a/setup.py
+++ b/setup.py
@@ -22,7 +22,7 @@ requirements = [
 
 setup(
     name='umongo',
-    version='2.0.5',
+    version='2.0.3',
     description="sync/async MongoDB ODM, yes.",
     long_description=readme + '\n\n' + history,
     author="Emmanuel Leblond",
@@ -32,7 +32,7 @@ setup(
     include_package_data=True,
     install_requires=requirements,
     extras_require={
-        'motor': ['motor>=1.1,<2.0'],
+        'motor': ['motor>=1.1,<3.0'],
         'txmongo': ['txmongo>=16.0.1'],
         'mongomock': ['mongomock'],
     },
diff --git a/tox.ini b/tox.ini
index d30b3cb..a1a6443 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,5 +1,5 @@
 [tox]
-envlist = lint,{py35,py36,py37}-{motor,pymongo,txmongo}
+envlist = lint,{py35,py36,py37}-{motor1,motor2,pymongo,txmongo}
 
 [testenv]
 setenv =
@@ -7,7 +7,8 @@ setenv =
 deps =
     pytest>=4.0.0
     pytest-cov>=2.6.0
-    motor: motor>=1.0,<2.0
+    motor1: motor>=1.0,<2.0
+    motor2: motor>=2.0,<3.0
     pymongo: mongomock>=3.5.0
     txmongo: txmongo>=16.0.1
     txmongo: pytest_twisted>=1.9
diff --git a/umongo/__init__.py b/umongo/__init__.py
index dd0ccff..1d3ffcb 100644
--- a/umongo/__init__.py
+++ b/umongo/__init__.py
@@ -37,7 +37,7 @@ from .i18n import set_gettext
 
 __author__ = 'Emmanuel Leblond'
 __email__ = 'emmanuel.leblond@gmail.com'
-__version__ = '2.0.5'
+__version__ = '2.0.3'
 __all__ = (
     'missing',
 
diff --git a/umongo/data_objects.py b/umongo/data_objects.py
index 3c468c9..eb3f028 100644
--- a/umongo/data_objects.py
+++ b/umongo/data_objects.py
@@ -21,10 +21,6 @@ class List(BaseDataObject, list):
         super().__setitem__(key, obj)
         self.set_modified()
 
-    def __delitem__(self, key):
-        super().__delitem__(key)
-        self.set_modified()
-
     def append(self, obj):
         obj = self.container_field.deserialize(obj)
         ret = super().append(obj)
@@ -75,7 +71,6 @@ class List(BaseDataObject, list):
         if len(self) and isinstance(self[0], BaseDataObject):
             # Recursive handling needed
             return any(obj.is_modified() for obj in self)
-        return False
 
     def clear_modified(self):
         self._modified = False
diff --git a/umongo/data_proxy.py b/umongo/data_proxy.py
index d322f46..230e628 100644
--- a/umongo/data_proxy.py
+++ b/umongo/data_proxy.py
@@ -45,14 +45,16 @@ class BaseDataProxy:
         mongo_data = {}
         set_data = {}
         unset_data = []
-        for name in self.get_modified_fields():
-            field = self._fields[name]
+        for name, field in self._fields.items():
             name = field.attribute or name
-            v = field.serialize_to_mongo(self._data[name])
-            if v is missing:
-                unset_data.append(name)
-            else:
-                set_data[name] = v
+            v = self._data[name]
+            if name in self._modified_data or (
+                    isinstance(v, BaseDataObject) and v.is_modified()):
+                v = field.serialize_to_mongo(v)
+                if v is missing:
+                    unset_data.append(name)
+                else:
+                    set_data[name] = v
         if set_data:
             mongo_data['$set'] = set_data
         if unset_data:
@@ -170,16 +172,14 @@ class BaseDataProxy:
         return NotImplemented
 
     def get_modified_fields_by_mongo_name(self):
-        return {self._fields[name].attribute or name for name in self.get_modified_fields()}
+        return self._modified_data
 
     def get_modified_fields(self):
-        modified = set()
+        modified = []
         for name, field in self._fields.items():
             value_name = field.attribute or name
-            value = self._data[value_name]
-            if value_name in self._modified_data or (
-                    isinstance(value, BaseDataObject) and value.is_modified()):
-                modified.add(name)
+            if value_name in self._modified_data:
+                modified.append(name)
         return modified
 
     def clear_modified(self):
diff --git a/umongo/frameworks/motor_asyncio.py b/umongo/frameworks/motor_asyncio.py
index 19b0bd4..f69ef32 100644
--- a/umongo/frameworks/motor_asyncio.py
+++ b/umongo/frameworks/motor_asyncio.py
@@ -1,6 +1,7 @@
 import asyncio
 
 from motor.motor_asyncio import AsyncIOMotorDatabase, AsyncIOMotorCursor
+from motor import version_tuple as MOTOR_VERSION
 from pymongo.errors import DuplicateKeyError
 from inspect import iscoroutine
 
@@ -47,7 +48,8 @@ class WrappedCursor(AsyncIOMotorCursor):
         return self.raw_cursor.each(wrapped_callback)
 
     def to_list(self, length, callback=None):
-        raw_future = self.raw_cursor.to_list(length, callback=callback)
+        kwargs = {"callback": callback} if callback else {}
+        raw_future = self.raw_cursor.to_list(length, **kwargs)
         cooked_future = asyncio.Future()
         builder = self.document_cls.build_from_mongo
 
@@ -255,6 +257,18 @@ class MotorAsyncIODocument(DocumentImplementation):
         filter = cook_find_filter(cls, filter)
         return WrappedCursor(cls, cls.collection.find(*args, filter=filter, **kwargs))
 
+    @classmethod
+    async def count_documents(cls, filter=None, *, with_limit_and_skip=False, **kwargs):
+        """
+        Return a count of the documents in a collection.
+        """
+        filter = cook_find_filter(cls, filter or {})
+        if MOTOR_VERSION < (2, 0):
+            count = await cls.find(filter, **kwargs).count(with_limit_and_skip)
+            return count
+        count = await cls.collection.count_documents(filter, **kwargs)
+        return count
+
     @classmethod
     async def ensure_indexes(cls):
         """
diff --git a/umongo/marshmallow_bonus.py b/umongo/marshmallow_bonus.py
index 669c6b7..55ad3b7 100644
--- a/umongo/marshmallow_bonus.py
+++ b/umongo/marshmallow_bonus.py
@@ -149,9 +149,6 @@ class Reference(ObjectId):
             return str(value)
         else:
             # In OO world, value is a :class:`umongo.data_object.Reference`
-            # or an ObjectId before being loaded into a Document
-            if isinstance(value, bson.ObjectId):
-                return str(value)
             return str(value.pk)
 
 
@@ -172,9 +169,6 @@ class GenericReference(ma_fields.Field):
             return {'id': str(value['_id']), 'cls': value['_cls']}
         else:
             # In OO world, value is a :class:`umongo.data_object.Reference`
-            # or a dict before being loaded into a Document
-            if isinstance(value, dict):
-                return {'id': str(value['id']), 'cls': value['cls']}
             return {'id': str(value.pk), 'cls': value.document_cls.__name__}
 
     def _deserialize(self, value, attr, data):


Test patch:
diff --git a/tests/frameworks/test_motor_asyncio.py b/tests/frameworks/test_motor_asyncio.py
index 503b220..d48deed 100644
--- a/tests/frameworks/test_motor_asyncio.py
+++ b/tests/frameworks/test_motor_asyncio.py
@@ -8,6 +8,7 @@ from bson import ObjectId
 # Check if the required dependancies are met to run this driver's tests
 try:
     from motor.motor_asyncio import AsyncIOMotorClient
+    from motor import version_tuple as MOTOR_VERSION
 except ImportError as e:
     dep_error = str(e)
 else:
@@ -114,17 +115,17 @@ class TestMotorAsyncio(BaseDBTest):
             with pytest.raises(exceptions.NotCreatedError):
                 await john.remove()
             await john.commit()
-            assert (await Student.find().count()) == 1
+            assert (await Student.count_documents()) == 1
             ret = await john.remove()
             assert isinstance(ret, DeleteResult)
             assert not john.is_created
-            assert (await Student.find().count()) == 0
+            assert (await Student.count_documents()) == 0
             with pytest.raises(exceptions.NotCreatedError):
                 await john.remove()
             # Can re-commit the document in database
             await john.commit()
             assert john.is_created
-            assert (await Student.find().count()) == 1
+            assert (await Student.count_documents()) == 1
             # Test conditional delete
             with pytest.raises(exceptions.DeleteError):
                 await john.remove(conditions={'name': 'Bad Name'})
@@ -163,10 +164,21 @@ class TestMotorAsyncio(BaseDBTest):
             for i in range(10):
                 await Student(name='student-%s' % i).commit()
             cursor = Student.find(limit=5, skip=6)
-            count = await cursor.count()
-            assert count == 10
-            count_with_limit_and_skip = await cursor.count(with_limit_and_skip=True)
-            assert count_with_limit_and_skip == 4
+            if MOTOR_VERSION < (2, 0):
+                # this test doesn't make as much sense in motor>2.0, since the cursor no
+                # longer supports a count() method
+                count = await cursor.count()
+                assert count == 10
+                count_with_limit_and_skip = await cursor.count(with_limit_and_skip=True)
+                assert count_with_limit_and_skip == 4
+            else:
+                assert (await Student.count_documents()) == 10
+                assert (await Student.count_documents(limit=5, skip=6)) == 4
+
+            # to_list with callback should fail
+            error = NotImplementedError if MOTOR_VERSION < (2, 0, 0) else TypeError
+            with pytest.raises(error):
+                await cursor.to_list(length=100, callback=lambda r, e: r if r else e)
 
             # Make sure returned documents are wrapped
             names = []
@@ -734,10 +746,10 @@ class TestMotorAsyncio(BaseDBTest):
             await InheritanceSearchChild1Child(pf=1, sc1f=1).commit()
             await InheritanceSearchChild2(pf=2, c2f=2).commit()
 
-            assert (await InheritanceSearchParent.find().count()) == 4
-            assert (await InheritanceSearchChild1.find().count()) == 2
-            assert (await InheritanceSearchChild1Child.find().count()) == 1
-            assert (await InheritanceSearchChild2.find().count()) == 1
+            assert (await InheritanceSearchParent.count_documents()) == 4
+            assert (await InheritanceSearchChild1.count_documents()) == 2
+            assert (await InheritanceSearchChild1Child.count_documents()) == 1
+            assert (await InheritanceSearchChild2.count_documents()) == 1
 
             res = await InheritanceSearchParent.find_one({'sc1f': 1})
             assert isinstance(res, InheritanceSearchChild1Child)
@@ -807,13 +819,13 @@ class TestMotorAsyncio(BaseDBTest):
                 ]
             ).commit()
 
-            res = await Book.find({'title': 'The Hobbit'}).count()
+            res = await Book.count_documents({'title': 'The Hobbit'})
             assert res == 1
-            res = await Book.find({'author.name': {'$in': ['JK Rowling', 'JRR Tolkien']}}).count()
+            res = await Book.count_documents({'author.name': {'$in': ['JK Rowling', 'JRR Tolkien']}})
             assert res == 2
-            res = await Book.find({'$and': [{'chapters.name': 'Roast Mutton'}, {'title': 'The Hobbit'}]}).count()
+            res = await Book.count_documents({'$and': [{'chapters.name': 'Roast Mutton'}, {'title': 'The Hobbit'}]})
             assert res == 1
-            res = await Book.find({'chapters.name': {'$all': ['Roast Mutton', 'A Short Rest']}}).count()
+            res = await Book.count_documents({'chapters.name': {'$all': ['Roast Mutton', 'A Short Rest']}})
             assert res == 1
 
         loop.run_until_complete(do_test())
diff --git a/tests/test_data_proxy.py b/tests/test_data_proxy.py
index 64aa2ae..77aadda 100644
--- a/tests/test_data_proxy.py
+++ b/tests/test_data_proxy.py
@@ -72,75 +72,22 @@ class TestDataProxy(BaseTest):
 
         MyDataProxy = data_proxy_factory('My', MySchema())
         d = MyDataProxy()
-        assert d.get_modified_fields() == set()
-        assert d.get_modified_fields_by_mongo_name() == set()
+        assert d.get_modified_fields() == []
         d.load({'a': 1, 'b': 2})
-        assert d.get_modified_fields() == {'a', 'b'}
-        assert d.get_modified_fields_by_mongo_name() == {'a', 'in_mongo_b'}
+        assert list(sorted(d.get_modified_fields())) == ['a', 'b']
         d.from_mongo({'a': 1, 'in_mongo_b': 2})
-        assert d.get_modified_fields() == set()
-        assert d.get_modified_fields_by_mongo_name() == set()
+        assert d.get_modified_fields() == []
         assert d.to_mongo() == {'a': 1, 'in_mongo_b': 2}
         assert d.to_mongo(update=True) is None
         d.set('a', 3)
         d.delete('b')
         assert d.to_mongo(update=True) == {'$set': {'a': 3}, '$unset': {'in_mongo_b': ''}}
         d.clear_modified()
-        assert d.get_modified_fields() == set()
-        assert d.get_modified_fields_by_mongo_name() == set()
+        assert d.get_modified_fields() == []
         assert d.to_mongo(update=True) is None
         assert d.to_mongo() == {'a': 3}
 
-    def test_list_field_modify(self):
-
-        class MySchema(EmbeddedSchema):
-            a = fields.ListField(fields.IntField())
-            b = fields.ListField(fields.IntField(), attribute='in_mongo_b')
-
-        MyDataProxy = data_proxy_factory('My', MySchema())
-        d = MyDataProxy()
-        assert d.get_modified_fields() == set()
-        assert d.get_modified_fields_by_mongo_name() == set()
-        d.load({'a': [1], 'b': [2, 2]})
-        assert d.get_modified_fields() == {'a', 'b'}
-        assert d.get_modified_fields_by_mongo_name() == {'a', 'in_mongo_b'}
-        d.from_mongo({'a': [1], 'in_mongo_b': [2, 2]})
-        assert d.get_modified_fields() == set()
-        assert d.get_modified_fields_by_mongo_name() == set()
-        assert d.to_mongo() == {'a': [1], 'in_mongo_b': [2, 2]}
-        assert d.to_mongo(update=True) is None
-        d.set('a', [3, 3, 3])
-        d.delete('b')
-        assert d.to_mongo(update=True) == {'$set': {'a': [3, 3, 3]}, '$unset': {'in_mongo_b': ''}}
-        d.clear_modified()
-        assert d.get_modified_fields() == set()
-        assert d.get_modified_fields_by_mongo_name() == set()
-        assert d.to_mongo() == {'a': [3, 3, 3]}
-        assert d.to_mongo(update=True) is None
-        d.clear_modified()
-        d.load({'a': [1], 'b': [2, 2]})
-        d._data['a'].append(1)
-        d._data['in_mongo_b'].append(2)
-        assert d.get_modified_fields() == {'a', 'b'}
-        assert d.get_modified_fields_by_mongo_name() == {'a', 'in_mongo_b'}
-        assert d.to_mongo() == {'a': [1, 1], 'in_mongo_b': [2, 2, 2]}
-        assert d.to_mongo(update=True) == {'$set': {'a': [1, 1], 'in_mongo_b': [2, 2, 2]}}
-        d.clear_modified()
-        del d._data['a'][0]
-        del d._data['in_mongo_b'][0]
-        assert d.get_modified_fields() == {'a', 'b'}
-        assert d.get_modified_fields_by_mongo_name() == {'a', 'in_mongo_b'}
-        assert d.to_mongo() == {'a': [1], 'in_mongo_b': [2, 2]}
-        assert d.to_mongo(update=True) == {'$set': {'a': [1], 'in_mongo_b': [2, 2]}}
-        d.clear_modified()
-        d._data['a'].clear()
-        d._data['in_mongo_b'].clear()
-        assert d.get_modified_fields() == {'a', 'b'}
-        assert d.get_modified_fields_by_mongo_name() == {'a', 'in_mongo_b'}
-        assert d.to_mongo() == {'a': [], 'in_mongo_b': []}
-        assert d.to_mongo(update=True) == {'$set': {'a': [], 'in_mongo_b': []}}
-
-    def test_complex_field_clear_modified(self):
+    def test_complexe_field_clear_modified(self):
 
         @self.instance.register
         class MyEmbedded(EmbeddedDocument):
diff --git a/tests/test_marshmallow.py b/tests/test_marshmallow.py
index ec8a9ca..918ea74 100644
--- a/tests/test_marshmallow.py
+++ b/tests/test_marshmallow.py
@@ -432,15 +432,9 @@ class TestMarshmallow(BaseTest):
         # schema to OO world
         ma_schema_cls = Doc.schema.as_marshmallow_schema()
         ma_schema = ma_schema_cls()
-        # Dump uMongo object
         ret = ma_schema.dump(doc)
         assert not ret.errors
         assert ret.data == serialized
-        # Dump OO data (not uMongo object) to ensure bonus fields round-trip
-        ret = ma_schema.dump(oo_data)
-        assert not ret.errors
-        assert ret.data == serialized
-        # Load serialized data
         ret = ma_schema.load(serialized)
         assert not ret.errors
         assert ret.data == oo_data