Repository: Scille/umongo
Pull request #195 (2019-05-15 13:42:27)

Fix is_modified
Various fixes about `is_modified`:

- Include modified `BaseDataObject`s in `BaseDataProxy.get_modified_fields` and `BaseDataProxy.get_modified_fields_by_mongo_name`. While fixing this, I factorized the logic in `get_modified_fields`. I don't think it is much worse in terms of performance. BTW, there was a type mismatch in those two methods, as the first returned a `list` and the second returned a `set`. Now, both return a `set`. I don't think this qualifies as a breaking change since it is not public API. `BaseDataProxy.get_modified_fields_by_mongo_name` was not even tested.

- Always return a boolean in `List.is_modified`. Previously, it could return `None`.

- `List`: set modified when using `del my_list_field [i]` (Issue reported in https://github.com/Scille/umongo/issues/71#issuecomment-489995372).



Patch:
diff --git a/umongo/data_objects.py b/umongo/data_objects.py
index eb3f028..3c468c9 100644
--- a/umongo/data_objects.py
+++ b/umongo/data_objects.py
@@ -21,6 +21,10 @@ 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)
@@ -71,6 +75,7 @@ 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 230e628..d322f46 100644
--- a/umongo/data_proxy.py
+++ b/umongo/data_proxy.py
@@ -45,16 +45,14 @@ class BaseDataProxy:
         mongo_data = {}
         set_data = {}
         unset_data = []
-        for name, field in self._fields.items():
+        for name in self.get_modified_fields():
+            field = self._fields[name]
             name = field.attribute or name
-            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
+            v = field.serialize_to_mongo(self._data[name])
+            if v is missing:
+                unset_data.append(name)
+            else:
+                set_data[name] = v
         if set_data:
             mongo_data['$set'] = set_data
         if unset_data:
@@ -172,14 +170,16 @@ class BaseDataProxy:
         return NotImplemented
 
     def get_modified_fields_by_mongo_name(self):
-        return self._modified_data
+        return {self._fields[name].attribute or name for name in self.get_modified_fields()}
 
     def get_modified_fields(self):
-        modified = []
+        modified = set()
         for name, field in self._fields.items():
             value_name = field.attribute or name
-            if value_name in self._modified_data:
-                modified.append(name)
+            value = self._data[value_name]
+            if value_name in self._modified_data or (
+                    isinstance(value, BaseDataObject) and value.is_modified()):
+                modified.add(name)
         return modified
 
     def clear_modified(self):


Test patch:
diff --git a/tests/test_data_proxy.py b/tests/test_data_proxy.py
index 77aadda..64aa2ae 100644
--- a/tests/test_data_proxy.py
+++ b/tests/test_data_proxy.py
@@ -72,22 +72,75 @@ class TestDataProxy(BaseTest):
 
         MyDataProxy = data_proxy_factory('My', MySchema())
         d = MyDataProxy()
-        assert d.get_modified_fields() == []
+        assert d.get_modified_fields() == set()
+        assert d.get_modified_fields_by_mongo_name() == set()
         d.load({'a': 1, 'b': 2})
-        assert list(sorted(d.get_modified_fields())) == ['a', 'b']
+        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})
-        assert d.get_modified_fields() == []
+        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}
         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() == []
+        assert d.get_modified_fields() == set()
+        assert d.get_modified_fields_by_mongo_name() == set()
         assert d.to_mongo(update=True) is None
         assert d.to_mongo() == {'a': 3}
 
-    def test_complexe_field_clear_modified(self):
+    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):
 
         @self.instance.register
         class MyEmbedded(EmbeddedDocument):