Improve BaseDocument.from_json tests and documentation (#2127)

mongoengine/base/document.py

@@ -463,13 +463,21 @@ class BaseDocument(object):
     def from_json(cls, json_data, created=False):
         """Converts json data to a Document instance
 
-        :param json_data: The json data to load into the Document
+        :param str json_data: The json data to load into the Document
-        :param created: If True, the document will be considered as a brand new document
+        :param bool created: Boolean defining whether to consider the newly
-                        If False and an id is provided, it will consider that the data being
+            instantiated document as brand new or as persisted already:
-                        loaded corresponds to what's already in the database (This has an impact of subsequent call to .save())
+            * If True, consider the document as brand new, no matter what data
-                        If False and no id is provided, it will consider the data as a new document
+              it's loaded with (i.e. even if an ID is loaded).
-                        (default ``False``)
+            * If False and an ID is NOT provided, consider the document as
+              brand new.
+            * If False and an ID is provided, assume that the object has
+              already been persisted (this has an impact on the subsequent
+              call to .save()).
+            * Defaults to ``False``.
         """
+        # TODO should `created` default to False? If the object already exists
+        # in the DB, you would likely retrieve it from MongoDB itself through
+        # a query, not load it from JSON data.
         return cls._from_son(json_util.loads(json_data), created=created)
 
     def __expand_dynamic_values(self, name, value):

tests/document/instance.py

@@ -3429,83 +3429,114 @@ class InstanceTest(MongoDBTestCase):
 
         self.assertEqual(Person.objects(height=189).count(), 1)
 
-    def test_from_son(self):
+    def test_shard_key_mutability_after_from_json(self):
-        # 771
+        """Ensure that a document ID can be modified after from_json.
-        class MyPerson(self.Person):
-            meta = dict(shard_key=["id"])
 
-        p = MyPerson.from_json('{"name": "name", "age": 27}', created=True)
+        If you instantiate a document by using from_json/_from_son and you
-        self.assertEqual(p.id, None)
+        indicate that this should be considered a new document (vs a doc that
-        p.id = (
+        already exists in the database), then you should be able to modify
-            "12345"
+        fields that are part of its shard key (note that this is not permitted
-        )  # in case it is not working: "OperationError: Shard Keys are immutable..." will be raised here
+        on docs that are already persisted).
-        p = MyPerson._from_son({"name": "name", "age": 27}, created=True)
-        self.assertEqual(p.id, None)
-        p.id = (
-            "12345"
-        )  # in case it is not working: "OperationError: Shard Keys are immutable..." will be raised here
 
-    def test_from_son_created_False_without_id(self):
+        See https://github.com/mongoengine/mongoengine/issues/771 for details.
-        class MyPerson(Document):
+        """
+
+        class Person(Document):
+            name = StringField()
+            age = IntField()
+            meta = {"shard_key": ("id", "name")}
+
+        p = Person.from_json('{"name": "name", "age": 27}', created=True)
+        self.assertEqual(p._created, True)
+        p.name = "new name"
+        p.id = "12345"
+        self.assertEqual(p.name, "new name")
+        self.assertEqual(p.id, "12345")
+
+    def test_shard_key_mutability_after_from_son(self):
+        """Ensure that a document ID can be modified after _from_son.
+
+        See `test_shard_key_mutability_after_from_json` above for more details.
+        """
+
+        class Person(Document):
+            name = StringField()
+            age = IntField()
+            meta = {"shard_key": ("id", "name")}
+
+        p = Person._from_son({"name": "name", "age": 27}, created=True)
+        self.assertEqual(p._created, True)
+        p.name = "new name"
+        p.id = "12345"
+        self.assertEqual(p.name, "new name")
+        self.assertEqual(p.id, "12345")
+
+    def test_from_json_created_false_without_an_id(self):
+        class Person(Document):
             name = StringField()
 
-        MyPerson.objects.delete()
+        Person.objects.delete()
 
-        p = MyPerson.from_json('{"name": "a_fancy_name"}', created=False)
+        p = Person.from_json('{"name": "name"}', created=False)
-        self.assertFalse(p._created)
+        self.assertEqual(p._created, False)
-        self.assertIsNone(p.id)
+        self.assertEqual(p.id, None)
+
+        # Make sure the document is subsequently persisted correctly.
         p.save()
-        self.assertIsNotNone(p.id)
+        self.assertTrue(p.id is not None)
-        saved_p = MyPerson.objects.get(id=p.id)
+        saved_p = Person.objects.get(id=p.id)
-        self.assertEqual(saved_p.name, "a_fancy_name")
+        self.assertEqual(saved_p.name, "name")
 
-    def test_from_son_created_False_with_id(self):
+    def test_from_json_created_false_with_an_id(self):
-        # 1854
+        """See https://github.com/mongoengine/mongoengine/issues/1854"""
-        class MyPerson(Document):
+
+        class Person(Document):
             name = StringField()
 
-        MyPerson.objects.delete()
+        Person.objects.delete()
 
-        p = MyPerson.from_json(
+        p = Person.from_json(
-            '{"_id": "5b85a8b04ec5dc2da388296e", "name": "a_fancy_name"}', created=False
+            '{"_id": "5b85a8b04ec5dc2da388296e", "name": "name"}', created=False
         )
-        self.assertFalse(p._created)
+        self.assertEqual(p._created, False)
         self.assertEqual(p._changed_fields, [])
-        self.assertEqual(p.name, "a_fancy_name")
+        self.assertEqual(p.name, "name")
         self.assertEqual(p.id, ObjectId("5b85a8b04ec5dc2da388296e"))
         p.save()
 
         with self.assertRaises(DoesNotExist):
-            # Since created=False and we gave an id in the json and _changed_fields is empty
+            # Since the object is considered as already persisted (thanks to
-            # mongoengine assumes that the document exits with that structure already
+            # `created=False` and an existing ID), and we haven't changed any
-            # and calling .save() didn't save anything
+            # fields (i.e. `_changed_fields` is empty), the document is
-            MyPerson.objects.get(id=p.id)
+            # considered unchanged and hence the `save()` call above did
+            # nothing.
+            Person.objects.get(id=p.id)
 
         self.assertFalse(p._created)
-        p.name = "a new fancy name"
+        p.name = "a new name"
         self.assertEqual(p._changed_fields, ["name"])
         p.save()
-        saved_p = MyPerson.objects.get(id=p.id)
+        saved_p = Person.objects.get(id=p.id)
         self.assertEqual(saved_p.name, p.name)
 
-    def test_from_son_created_True_with_an_id(self):
+    def test_from_json_created_true_with_an_id(self):
-        class MyPerson(Document):
+        class Person(Document):
             name = StringField()
 
-        MyPerson.objects.delete()
+        Person.objects.delete()
 
-        p = MyPerson.from_json(
+        p = Person.from_json(
-            '{"_id": "5b85a8b04ec5dc2da388296e", "name": "a_fancy_name"}', created=True
+            '{"_id": "5b85a8b04ec5dc2da388296e", "name": "name"}', created=True
         )
         self.assertTrue(p._created)
         self.assertEqual(p._changed_fields, [])
-        self.assertEqual(p.name, "a_fancy_name")
+        self.assertEqual(p.name, "name")
         self.assertEqual(p.id, ObjectId("5b85a8b04ec5dc2da388296e"))
         p.save()
 
-        saved_p = MyPerson.objects.get(id=p.id)
+        saved_p = Person.objects.get(id=p.id)
         self.assertEqual(saved_p, p)
-        self.assertEqual(p.name, "a_fancy_name")
+        self.assertEqual(saved_p.name, "name")
 
     def test_null_field(self):
         # 734