Document serialization uses field order to ensure a strict order is set (#296)

docs/changelog.rst

@@ -4,6 +4,7 @@ Changelog
 
 Changes in 0.8.X
 ================
+- Document serialization uses field order to ensure a strict order is set (#296)
 - DecimalField now stores as float not string (#289)
 - UUIDField now stores as a binary by default (#292)
 - Added Custom User Model for Django 1.5 (#285)

docs/guide/defining-documents.rst

@@ -24,6 +24,9 @@ objects** as class attributes to the document class::
         title = StringField(max_length=200, required=True)
         date_modified = DateTimeField(default=datetime.datetime.now)
 
+As BSON (the binary format for storing data in mongodb) is order dependent,
+documents are serialized based on their field order.
+
 Dynamic document schemas
 ========================
 One of the benefits of MongoDb is dynamic schemas for a collection, whilst data
@@ -51,6 +54,7 @@ be saved ::
 
    There is one caveat on Dynamic Documents: fields cannot start with `_`
 
+Dynamic fields are stored in alphabetical order *after* any declared fields.
 
 Fields
 ======

docs/guide/document-instances.rst

@@ -30,11 +30,14 @@ already exist, then any changes will be updated atomically.  For example::
 
 .. note::
 
-    Changes to documents are tracked and on the whole perform `set` operations.
+    Changes to documents are tracked and on the whole perform ``set`` operations.
 
-    * ``list_field.pop(0)`` - *sets* the resulting list
+    * ``list_field.push(0)`` - *sets* the resulting list
     * ``del(list_field)``   - *unsets* whole list
 
+    With lists its preferable to use ``Doc.update(push__list_field=0)`` as
+    this stops the whole list being updated - stopping any race conditions.
+
 .. seealso::
     :ref:`guide-atomic-updates`
 
@@ -70,9 +73,10 @@ Cascading Saves
 ---------------
 If your document contains :class:`~mongoengine.fields.ReferenceField` or
 :class:`~mongoengine.fields.GenericReferenceField` objects, then by default the
-:meth:`~mongoengine.Document.save` method will automatically save any changes to
-those objects as well.  If this is not desired passing :attr:`cascade` as False
-to the save method turns this feature off.
+:meth:`~mongoengine.Document.save` method will not save any changes to
+those objects.  If you want all references to also be saved also, noting each
+save is a separate query, then passing :attr:`cascade` as True
+to the save method will cascade any saves.
 
 Deleting documents
 ------------------

docs/upgrade.rst

@@ -120,7 +120,7 @@ eg::
         p._mark_as_dirty('friends')
         p.save()
 
-`An example test migration is available on github
+`An example test migration for ReferenceFields is available on github
 <https://github.com/MongoEngine/mongoengine/blob/master/tests/migration/refrencefield_dbref_to_object_id.py>`_.
 
 UUIDField
@@ -148,7 +148,7 @@ eg::
         a._mark_as_dirty('uuid')
         a.save()
 
-`An example test migration is available on github
+`An example test migration for UUIDFields is available on github
 <https://github.com/MongoEngine/mongoengine/blob/master/tests/migration/uuidfield_to_binary.py>`_.
 
 DecimalField
@@ -180,7 +180,7 @@ eg::
 .. note:: DecimalField's have also been improved with the addition of precision
     and rounding.  See :class:`~mongoengine.fields.DecimalField` for more information.
 
-`An example test migration is available on github
+`An example test migration for DecimalFields is available on github
 <https://github.com/MongoEngine/mongoengine/blob/master/tests/migration/decimalfield_as_float.py>`_.
 
 Cascading Saves
@@ -196,6 +196,19 @@ you will have to explicitly tell it to cascade on save::
     # Or on save:
     my_document.save(cascade=True)
 
+Storage
+-------
+
+Document and Embedded Documents are now serialized based on declared field order.
+Previously, the data was passed to mongodb as a dictionary and which meant that
+order wasn't guaranteed - so things like ``$addToSet`` operations on
+:class:`~mongoengine.EmbeddedDocument` could potentially fail in unexpected
+ways.
+
+If this impacts you, you may want to rewrite the objects using the
+``doc.mark_as_dirty('field')`` pattern described above.  If you are using a
+compound primary key then you will need to ensure the order is fixed and match
+your EmbeddedDocument to that order.
 
 Querysets
 =========

mongoengine/base/document.py

@@ -6,6 +6,7 @@ from functools import partial
 import pymongo
 from bson import json_util
 from bson.dbref import DBRef
+from bson.son import SON
 
 from mongoengine import signals
 from mongoengine.common import _import_class
@@ -228,11 +229,16 @@ class BaseDocument(object):
         pass
 
     def to_mongo(self):
-        """Return data dictionary ready for use with MongoDB.
+        """Return as SON data ready for use with MongoDB.
         """
-        data = {}
-        for field_name, field in self._fields.iteritems():
+        data = SON()
+        data["_id"] = None
+        data['_cls'] = self._class_name
+
+        for field_name in self:
             value = self._data.get(field_name, None)
+            field = self._fields.get(field_name)
+
             if value is not None:
                 value = field.to_mongo(value)
 
@@ -244,19 +250,27 @@ class BaseDocument(object):
             if value is not None:
                 data[field.db_field] = value
 
-        # Only add _cls if allow_inheritance is True
-        if (hasattr(self, '_meta') and
-            self._meta.get('allow_inheritance', ALLOW_INHERITANCE) == True):
-            data['_cls'] = self._class_name
+        # If "_id" has not been set, then try and set it
+        if data["_id"] is None:
+            data["_id"] = self._data.get("id", None)
 
-        if '_id' in data and data['_id'] is None:
-            del data['_id']
+        if data['_id'] is None:
+            data.pop('_id')
+
+        # Only add _cls if allow_inheritance is True
+        if (not hasattr(self, '_meta') or
+           not self._meta.get('allow_inheritance', ALLOW_INHERITANCE)):
+            data.pop('_cls')
 
         if not self._dynamic:
             return data
 
-        for name, field in self._dynamic_fields.items():
+        # Sort dynamic fields by key
+        dynamic_fields = sorted(self._dynamic_fields.iteritems(),
+                                key=operator.itemgetter(0))
+        for name, field in dynamic_fields:
             data[name] = field.to_mongo(self._data.get(name, None))
+
         return data
 
     def validate(self, clean=True):

tests/document/dynamic.py

@@ -31,8 +31,9 @@ class DynamicTest(unittest.TestCase):
 
         self.assertEqual(p.to_mongo(), {"_cls": "Person", "name": "James",
                                         "age": 34})
-
+        self.assertEqual(p.to_mongo().keys(), ["_cls", "name", "age"])
         p.save()
+        self.assertEqual(p.to_mongo().keys(), ["_id", "_cls", "name", "age"])
 
         self.assertEqual(self.Person.objects.first().age, 34)
 

tests/document/inheritance.py

@@ -168,6 +168,26 @@ class InheritanceTest(unittest.TestCase):
         self.assertEqual(Employee._get_collection_name(),
                          Person._get_collection_name())
 
+    def test_inheritance_to_mongo_keys(self):
+        """Ensure that document may inherit fields from a superclass document.
+        """
+        class Person(Document):
+            name = StringField()
+            age = IntField()
+
+            meta = {'allow_inheritance': True}
+
+        class Employee(Person):
+            salary = IntField()
+
+        self.assertEqual(['age', 'id', 'name', 'salary'],
+                         sorted(Employee._fields.keys()))
+        self.assertEqual(Person(name="Bob", age=35).to_mongo().keys(),
+                         ['_cls', 'name', 'age'])
+        self.assertEqual(Employee(name="Bob", age=35, salary=0).to_mongo().keys(),
+                         ['_cls', 'name', 'age', 'salary'])
+        self.assertEqual(Employee._get_collection_name(),
+                         Person._get_collection_name())
 
     def test_polymorphic_queries(self):
         """Ensure that the correct subclasses are returned from a query
@@ -197,7 +217,6 @@ class InheritanceTest(unittest.TestCase):
         classes = [obj.__class__ for obj in Human.objects]
         self.assertEqual(classes, [Human])
 
-
     def test_allow_inheritance(self):
         """Ensure that inheritance may be disabled on simple classes and that
         _cls and _subclasses will not be used.
@@ -213,8 +232,8 @@ class InheritanceTest(unittest.TestCase):
         self.assertRaises(ValueError, create_dog_class)
 
         # Check that _cls etc aren't present on simple documents
-        dog = Animal(name='dog')
-        dog.save()
+        dog = Animal(name='dog').save()
+        self.assertEqual(dog.to_mongo().keys(), ['_id', 'name'])
 
         collection = self.db[Animal._get_collection_name()]
         obj = collection.find_one()

tests/document/instance.py

@@ -428,6 +428,21 @@ class InstanceTest(unittest.TestCase):
         self.assertFalse('age' in person)
         self.assertFalse('nationality' in person)
 
+    def test_embedded_document_to_mongo(self):
+        class Person(EmbeddedDocument):
+            name = StringField()
+            age = IntField()
+
+            meta = {"allow_inheritance": True}
+
+        class Employee(Person):
+            salary = IntField()
+
+        self.assertEqual(Person(name="Bob", age=35).to_mongo().keys(),
+                         ['_cls', 'name', 'age'])
+        self.assertEqual(Employee(name="Bob", age=35, salary=0).to_mongo().keys(),
+                         ['_cls', 'name', 'age', 'salary'])
+
     def test_embedded_document(self):
         """Ensure that embedded documents are set up correctly.
         """