Repository: marshmallow-code/marshmallow
Pull request #1685 (2020-11-02 10:30:27)

Cast to mapping type in Mapping.serialize/deserialize
When no key/value fields are specified in a `Dict` field, the value is passed as is when loading or dumping.

This means that the dump is still related to the object and modifying the dump result modifies the object. My understanding is that marshmallow is meant to prevent that and guarantee that the dumped/loaded value is of proper type and not linked to the original value.

This triggered [this issue](https://github.com/Scille/umongo/issues/306).

This PR fixes this by always casting to mapping type even if no key/value fields are specified.

We could remove the line and rely on the generic logic below but I suppose it makes sense to keep this line as a shortcut for the "loose dict" case (no field specified), as a slight performance improvement. I don't mind either way. Dropping code is nice as well and the performance improvement might be limited.

Also, I had to remove two broken tests. Those tests check that `Dict` field pass invalid dicts transparently. I don't see the point of this. In practice, it only works in the "loose dict" case. Besides, `List` chokes on invalid iterables, for instance. I don't understand why we added those tests in the first place. I didn't dig in the history to find out.

------------------------------------------------------

~Finally, mypy complained about unused ignores, so I removed them.~

Edit: Looks like there's a mypy/pre-commit issue. When committing, mypy (launched by pre-commit) complained about unused ignores, so I removed the ignores, but then it would complain when run on the whole code. I put the ignores back and committed with `--no-verify`. Could it be mypy is executed by pre-commit on modified files, not all the files, and therefore it complains about unused stuff?

sloria: > Could it be mypy is executed by pre-commit on modified files, not all the files, and therefore it complains about unused stuff?

Yes, I've also seen this strange behavior when mypy executes on single files vs all files. Not sure what the issue is ¯\_(ツ)_/¯

Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index fe10f80c..6b45926e 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -1,6 +1,16 @@
 Changelog
 ---------
 
+3.9.1 (unreleased)
+******************
+
+Bug fixes:
+
+- Cast to mapping type in ``Mapping.serialize`` and ``Mapping.deserialize``
+  (:pr:`1685`).
+- Fix bug letting ``Dict`` pass invalid dict on deserialization when no key or
+  value ``Field`` is specified (:pr:`1685`).
+
 3.9.0 (2020-10-31)
 ******************
 
diff --git a/src/marshmallow/fields.py b/src/marshmallow/fields.py
index 9d1142bf..dc73a418 100644
--- a/src/marshmallow/fields.py
+++ b/src/marshmallow/fields.py
@@ -1505,7 +1505,7 @@ class Mapping(Field):
         if value is None:
             return None
         if not self.value_field and not self.key_field:
-            return value
+            return self.mapping_type(value)
 
         #  Serialize keys
         if self.key_field is None:
@@ -1532,7 +1532,7 @@ class Mapping(Field):
         if not isinstance(value, _Mapping):
             raise self.make_error("invalid")
         if not self.value_field and not self.key_field:
-            return value
+            return self.mapping_type(value)
 
         errors = collections.defaultdict(dict)
 


Test patch:
diff --git a/tests/test_deserialization.py b/tests/test_deserialization.py
index 1c2dca25..084a966f 100644
--- a/tests/test_deserialization.py
+++ b/tests/test_deserialization.py
@@ -719,8 +719,13 @@ class TestFieldDeserialization:
         assert excinfo.value.args[0] == msg
 
     def test_dict_field_deserialization(self):
+        data = {"foo": "bar"}
         field = fields.Dict()
-        assert field.deserialize({"foo": "bar"}) == {"foo": "bar"}
+        load = field.deserialize(data)
+        assert load == {"foo": "bar"}
+        # Check load is a distinct object
+        load["foo"] = "baz"
+        assert data["foo"] == "bar"
         with pytest.raises(ValidationError) as excinfo:
             field.deserialize("baddict")
         assert excinfo.value.args[0] == "Not a valid mapping type."
diff --git a/tests/test_schema.py b/tests/test_schema.py
index 2a720374..4f5b871c 100644
--- a/tests/test_schema.py
+++ b/tests/test_schema.py
@@ -641,11 +641,6 @@ def test_can_serialize_time(user, serialized_user):
     assert serialized_user["time_registered"] == expected
 
 
-def test_invalid_dict_but_okay():
-    u = User("Joe", various_data="baddict")
-    UserSchema().dump(u)
-
-
 def test_json_module_is_deprecated():
     with pytest.deprecated_call():
 
diff --git a/tests/test_serialization.py b/tests/test_serialization.py
index e4859c8d..cdd7d253 100644
--- a/tests/test_serialization.py
+++ b/tests/test_serialization.py
@@ -369,16 +369,14 @@ class TestFieldSerialization:
         field = fields.Dict()
         assert field.serialize("various_data", user) is None
 
-    def test_dict_field_invalid_dict_but_okay(self, user):
-        user.various_data = "okaydict"
-        field = fields.Dict()
-        field.serialize("various_data", user)
-        assert field.serialize("various_data", user) == "okaydict"
-
     def test_dict_field_serialize(self, user):
         user.various_data = {"foo": "bar"}
         field = fields.Dict()
-        assert field.serialize("various_data", user) == {"foo": "bar"}
+        dump = field.serialize("various_data", user)
+        assert dump == {"foo": "bar"}
+        # Check dump is a distinct object
+        dump["foo"] = "baz"
+        assert user.various_data["foo"] == "bar"
 
     def test_dict_field_serialize_ordereddict(self, user):
         user.various_data = OrderedDict([("foo", "bar"), ("bar", "baz")])