Merge pull request #1949 from bagerard/fix_delta_bug

Fix bug in _delta method - setting ListField to empty in DynamicDocument is faulty

Author: bagerard

docs/changelog.rst

@@ -5,6 +5,7 @@ Changelog
 Development
 ===========
 - (Fill this out as you fix issues and develop your features).
+- Fix bug in _delta method - Update of a ListField depends on an unrelated dynamic field update #1733
 - Remove deprecated `save()` method and used `insert_one()` #1899
 
 =================

mongoengine/base/document.py

@@ -579,7 +579,6 @@ def _delta(self):
 
         set_fields = self._get_changed_fields()
         unset_data = {}
-        parts = []
         if hasattr(self, '_changed_fields'):
             set_data = {}
             # Fetch each set item from its path
@@ -589,15 +588,13 @@ def _delta(self):
                 new_path = []
                 for p in parts:
                     if isinstance(d, (ObjectId, DBRef)):
+                        # Don't dig in the references
                         break
-                    elif isinstance(d, list) and p.lstrip('-').isdigit():
-                        if p[0] == '-':
-                            p = str(len(d) + int(p))
-                        try:
-                            d = d[int(p)]
-                        except IndexError:
-                            d = None
+                    elif isinstance(d, list) and p.isdigit():
+                        # An item of a list (identified by its index) is updated
+                        d = d[int(p)]
                     elif hasattr(d, 'get'):
+                        # dict-like (dict, embedded document)
                         d = d.get(p)
                     new_path.append(p)
                 path = '.'.join(new_path)
@@ -609,26 +606,26 @@ def _delta(self):
 
         # Determine if any changed items were actually unset.
         for path, value in set_data.items():
-            if value or isinstance(value, (numbers.Number, bool)):
+            if value or isinstance(value, (numbers.Number, bool)):  # Account for 0 and True that are truthy
                 continue
 
-            # If we've set a value that ain't the default value don't unset it.
-            default = None
+            parts = path.split('.')
+
             if (self._dynamic and len(parts) and parts[0] in
                     self._dynamic_fields):
                 del set_data[path]
                 unset_data[path] = 1
                 continue
-            elif path in self._fields:
+
+            # If we've set a value that ain't the default value don't unset it.
+            default = None
+            if path in self._fields:
                 default = self._fields[path].default
             else:  # Perform a full lookup for lists / embedded lookups
                 d = self
-                parts = path.split('.')
                 db_field_name = parts.pop()
                 for p in parts:
-                    if isinstance(d, list) and p.lstrip('-').isdigit():
-                        if p[0] == '-':
-                            p = str(len(d) + int(p))
+                    if isinstance(d, list) and p.isdigit():
                         d = d[int(p)]
                     elif (hasattr(d, '__getattribute__') and
                           not isinstance(d, dict)):
@@ -646,10 +643,9 @@ def _delta(self):
                         default = None
 
             if default is not None:
-                if callable(default):
-                    default = default()
+                default = default() if callable(default) else default
 
-            if default != value:
+            if value != default:
                 continue
 
             del set_data[path]

tests/fields/fields.py

@@ -186,6 +186,31 @@ class Person(Document):
         data_to_be_saved = sorted(person.to_mongo().keys())
         self.assertEqual(data_to_be_saved, ['age', 'created', 'userid'])
 
+    def test_default_value_is_not_used_when_changing_value_to_empty_list_for_strict_doc(self):
+        """List field with default can be set to the empty list (strict)"""
+        # Issue #1733
+        class Doc(Document):
+            x = ListField(IntField(), default=lambda: [42])
+
+        doc = Doc(x=[1]).save()
+        doc.x = []
+        doc.save()
+        reloaded = Doc.objects.get(id=doc.id)
+        self.assertEqual(reloaded.x, [])
+
+    def test_default_value_is_not_used_when_changing_value_to_empty_list_for_dyn_doc(self):
+        """List field with default can be set to the empty list (dynamic)"""
+        # Issue #1733
+        class Doc(DynamicDocument):
+            x = ListField(IntField(), default=lambda: [42])
+
+        doc = Doc(x=[1]).save()
+        doc.x = []
+        doc.y = 2   # Was triggering the bug
+        doc.save()
+        reloaded = Doc.objects.get(id=doc.id)
+        self.assertEqual(reloaded.x, [])
+
     def test_default_values_when_deleting_value(self):
         """Ensure that default field values are used after non-default
         values are explicitly deleted.