Repository: marshmallow-code/marshmallow

Issue:
JSON deserialization converts dots in field names to deep dictionaries.
**Summary**
The marshmallow JSON deserializer interprets dots as meaning a sub-object in user-provided input, violating the JSON standard.

**Minimal example**
```py
from marshmallow import Schema, INCLUDE

class MySchema(Schema):
    class Meta:
        unknown = INCLUDE

> s = MySchema()
> s.load({"01.12.2019": 1, "01.01.2020": 0})
{'01': {'01': {'2020': 0}, '12': {'2019': 1}}}

> s.loads("""{"01.12.2019": 1, "01.01.2020": 0}""")
{'01': {'01': {'2020': 0}, '12': {'2019': 1}}}
```

**Discussion**
While I appreciate the idea of allowing "compact notation" using dotted paths, this even causes problems in attribute/data_key specifications, such as #1225 (I like the idea discussed there of using lists of strings instead of in-band signaling using path notation). Either way, applying the dot path transformation *on the data* is counter-intuitive, undocumented and dangerous as it's hard to discover during testing.

**Motivation**
My schema allows somewhat "free form" key-value attributes for certain objects (think os.environ, or some other meta-attribute table). The schema (in my case) enforces that all field values are numeric, like so:
```
class MetaAttributeSchema(Schema):
    class Meta:
        unknown = INCLUDE

    @validates_schema
    def validate_types(self, data, **kwargs):
        errors = {}
        for key, value in data.items():
            try:
                NumberValidator.deserialize(value)
            except ValidationError as e:
                errors[key] = e.messages
        if errors:
            raise ValidationError(errors)
```


Discussion:
deckar01: > In #1506, @deckar01 suggested using Field to do this, but I'm not sure I understood properly.

The underlying issue is that only certain field/meta arguments are allowed to parse dotted syntax, but this bug is parsing the dotted syntax of unknown data keys, which is undocumented (and undesired) behavior. The other `set_value()` call I was referring to is actually another use case where data keys can be parsed as dotted syntax. This bug is hard to reach, because dots are not valid Python class attribute names, but I suspect there is a way to hit it, possibly when generating a schema from a dict. The alternate solution only really applied to that use case, because the responsibility could be pushed down to the field. That wouldn't work for unknown keys, which by definition don't have a field declared, so I think this fix is good.
deckar01: > But I could easily be convinced if one of the maintainers feels strongly about being really strictly backwards compatible.

I think this is safe for a patch release without a feature flag. I don't think SEMVER is intended to protect building on undefined behavior. :shipit: 
sirosen: I've simplified it to just `ret_d[key] = value` and gotten rid of the changes in `set_value`. (I amended because I fussed with a type comment and broke py3.5 compat, so I reverted that.)

> I don't think SEMVER is intended to protect building on undefined behavior.

👍 That's how I see it, but if someone wanted to make the case I wasn't going to resist.

> I suspect there is a way to hit it [dotted attribute name for a field], possibly when generating a schema from a dict.

Yeah, I bet that would do it. I can `class A: pass ; setattr(A, "foo.bar", 1)` and `dir(A)` shows `foo.bar` just fine. You're not able to access such an attribute with the normal dot-based syntax, but `getattr` and other methods should work fine.
I'll see if I can add a test or two for this to confirm the behavior is right, or -- if there is a bug -- I might try to put together a fix.
sirosen: I think we should merge this as-is. I looked into dotted attributes and it seems like something so niche and strange that I think we could hang our hats on "the behavior is undefined".

<details><summary>Here's a diff from me playing with this.</summary>

```diff
diff --git a/src/marshmallow/schema.py b/src/marshmallow/schema.py
index ea030c1..3800dd6 100644
--- a/src/marshmallow/schema.py
+++ b/src/marshmallow/schema.py
@@ -673,7 +673,14 @@ class Schema(base.SchemaABC, metaclass=SchemaMeta):
                 )
                 if value is not missing:
                     key = field_obj.attribute or attr_name
-                    set_value(ret_d, key, value)
+                    if (
+                        field_obj.data_key is None
+                        and field_obj.attribute is None
+                        and "." in key
+                    ):
+                        ret_d[key] = value
+                    else:
+                        set_value(ret_d, key, value)
             if unknown != EXCLUDE:
                 fields = {
                     field_obj.data_key if field_obj.data_key is not None else field_name
diff --git a/tests/test_deserialization.py b/tests/test_deserialization.py
index dda9c59..4f71e25 100644
--- a/tests/test_deserialization.py
+++ b/tests/test_deserialization.py
@@ -1745,6 +1745,11 @@ class TestSchemaDeserialization:
         data = MySchema().load({"foo": "hi", "bar.baz": "okay", "alpha.beta": "woah!"})
         assert data == {"foo": "hi", "bar": "okay", "alpha.beta": "woah!"}

+    def test_field_with_dotted_attribute_name(self):
+        MySchema = Schema.from_dict({"foo.bar": fields.Str()})
+        data = MySchema().load({"foo.bar": "hi"})
+        assert data == {"foo.bar": "hi"}
+

 validators_gen = (func for func in [lambda x: x <= 24, lambda x: 18 <= x])
```

</details>

It works... but I'm not sure if that toy example is a realistic use-case? At the very least, I think we can follow-up on it separately.

Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 18f514e2..1b796e22 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -1,6 +1,14 @@
 Changelog
 ---------
 
+3.11.1 (unreleased)
+*******************
+
+Bug fixes:
+
+- Fix treatment of dotted keys when unknown=INCLUDE (:issue:`1506`).
+  Thanks :user:`rbu` for reporting and thanks :user:`sirosen` for the fix (:pr:`1745`).
+
 3.11.0 (2021-03-28)
 *******************
 
diff --git a/src/marshmallow/schema.py b/src/marshmallow/schema.py
index 93259d74..ea030c16 100644
--- a/src/marshmallow/schema.py
+++ b/src/marshmallow/schema.py
@@ -616,9 +616,9 @@ class Schema(base.SchemaABC, metaclass=SchemaMeta):
         if many:
             if not is_collection(data):
                 error_store.store_error([self.error_messages["type"]], index=index)
-                ret = []  # type: typing.List[_T]
+                ret_l = []  # type: typing.List[_T]
             else:
-                ret = [
+                ret_l = [
                     typing.cast(
                         _T,
                         self._deserialize(
@@ -632,8 +632,8 @@ class Schema(base.SchemaABC, metaclass=SchemaMeta):
                     )
                     for idx, d in enumerate(data)
                 ]
-            return ret
-        ret = self.dict_class()
+            return ret_l
+        ret_d = self.dict_class()
         # Check data is a dict
         if not isinstance(data, Mapping):
             error_store.store_error([self.error_messages["type"]], index=index)
@@ -673,7 +673,7 @@ class Schema(base.SchemaABC, metaclass=SchemaMeta):
                 )
                 if value is not missing:
                     key = field_obj.attribute or attr_name
-                    set_value(typing.cast(typing.Dict, ret), key, value)
+                    set_value(ret_d, key, value)
             if unknown != EXCLUDE:
                 fields = {
                     field_obj.data_key if field_obj.data_key is not None else field_name
@@ -682,14 +682,14 @@ class Schema(base.SchemaABC, metaclass=SchemaMeta):
                 for key in set(data) - fields:
                     value = data[key]
                     if unknown == INCLUDE:
-                        set_value(typing.cast(typing.Dict, ret), key, value)
+                        ret_d[key] = value
                     elif unknown == RAISE:
                         error_store.store_error(
                             [self.error_messages["unknown"]],
                             key,
                             (index if index_errors else None),
                         )
-        return ret
+        return ret_d
 
     def load(
         self,


Test patch:
diff --git a/tests/test_deserialization.py b/tests/test_deserialization.py
index 6690e429..f79ee96e 100644
--- a/tests/test_deserialization.py
+++ b/tests/test_deserialization.py
@@ -1808,6 +1808,22 @@ class TestSchemaDeserialization:
         data = MySchema(unknown=INCLUDE).load({"foo": "LOL"})
         assert data["foo"] == "LOL"
 
+    def test_unknown_fields_do_not_unpack_dotted_names(self):
+        class MySchema(Schema):
+            class Meta:
+                unknown = INCLUDE
+
+            foo = fields.Str()
+            bar = fields.Str(data_key="bar.baz")
+
+        # dotted names are still supported
+        data = MySchema().load({"foo": "hi", "bar.baz": "okay"})
+        assert data == {"foo": "hi", "bar": "okay"}
+
+        # but extra keys included via unknown=INCLUDE are not transformed into nested dicts
+        data = MySchema().load({"foo": "hi", "bar.baz": "okay", "alpha.beta": "woah!"})
+        assert data == {"foo": "hi", "bar": "okay", "alpha.beta": "woah!"}
+
 
 validators_gen = (func for func in [lambda x: x <= 24, lambda x: 18 <= x])