"unknown" option should apply to nested fields so that "exclude" works

[psverkada]

The fact that exclude takes dotted notation for nested fields, but unknown does not apply to nested fields, means that exclude doesn't work for nested fields in some common use cases unless the nested schema(s) have the desired Meta.unknown setting, and then they are not runtime-flexible, which is the great benefit of Schema and load taking exclude. Somewhat new to marshmallow so maybe there's a way to accomplish this (setting exclude on nested fields at load time) in a different way?

Happy to contribute a PR if you guys think this is a good idea.

[deckar01]

Can you provide a reproducible example of this issue? Are you getting errors about unknown nested fields when you are expecting them to be excluded?

[deckar01]

Schema and load taking exclude

The Schema constructor takes an exclude argument, but the load() method doesn't.

https://marshmallow.readthedocs.io/en/stable/api_reference.html#marshmallow.Schema.load

---

I tried building a repro case from the description. Let me know if I missed anything.

from marshmallow import Schema, fields, EXCLUDE

class Foo(Schema):
    data = fields.Str()

class Test(Schema):
    foo = fields.Nested(Foo)

schema = Test(exclude=['foo.data'], unknown=EXCLUDE)
schema.load({'foo': {'data': 'bar'}})
# marshmallow.exceptions.ValidationError: {'foo': {'data': ['Unknown field.']}}

In this example, unknown=EXCLUDE does not get inherited by Foo, so it will continue to raise when it encounters unknown keys. exclude=['foo.data'] propagates down to Foo and removes data from its internal list of fields. When Foo tries to load data it is treated as an unknown key and raises an error.

I initially though this was expected behavior, but the docs for exclude say it contains "fields to exclude in the serialized result". This seems to indicate that the excluded field shouldn't be treated as unknown, but just be excluded from the serialized object. It seems like exclude should take precedence over unknown.

https://marshmallow.readthedocs.io/en/stable/api_reference.html#marshmallow.Schema

A minimal repro case for this can be constructed without Nested or unknown.

from marshmallow import Schema, fields

class Foo(Schema):
    data = fields.Str()

schema = Foo(exclude=['data'])
schema.load({'data': 'bar'})
# marshmallow.exceptions.ValidationError: {'data': ['Unknown field.']}

[lafrech]

IIUC, behaviour is correct and conforms to the documentation:

exclude – Blacklist of the declared fields to exclude when instantiating the Schema.

The field is excluded, so treated as unknown.

Indeed, the "fields to exclude in the serialized result" phrasing is not precise enough and could be reworded. The part I quote above is clearer.

The issue seems to be that unknown does not propagate. This was already discussed here and we settled on "don't propagate". Generally, people will use a base schema for that.

[deckar01]

I copied from Meta.exclude, but linked to Schema(exclude). The constructor parameter definition is clear and matches the implementation. The Meta.exclude definition should probably be fixed to match that.

[psverkada]

Hi Jared Sorry for the slow reply. Yes, your first test case reproduces the issue I'm describing. Jerome also describes it correctly below:

The issue seems to be that unknown does not propagate.

Jerome's proposed solution ("Generally, people will use a base schema for that"), which, I think means setting Meta.unknown on the child schema(?) is what I ended up using. This was the solution I ended up on, but my comment was that this kind of obviates the usefulness of using `exclude` and `unknown` at schema instantiation or load time. If you've already deliberated on this and this is an intentional design decision, that's fine. Would be good to make it more explicit in the docs however. Thanks for looking at this so quickly.

…

On Wed, Jan 15, 2020 at 7:20 AM Jared Deckard ***@***.***> wrote: I copied from Meta.exclude, but linked to Schema(exclude). The schema definition is clear and matches the implementation. The description for Meta.exclude should probably be fixed to match that. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1490?email_source=notifications&email_token=AN36JUKMQ2RA6ACZGG7ASCTQ54SVVA5CNFSM4KG2CYZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJAVT2A#issuecomment-574708200>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN36JUJ2VVLCAPIB5PGU5DLQ54SVVANCNFSM4KG2CYZA> .

-- Pattabi Seshadri Engineering (Access)

[deckar01]

Discussion on proposals to recursively inherit unknown can be found on #980 and #1428. Any suggestions or PRs for updating the docs would be appreciated.

[psverkada]

Ok, thanks for those links. It's clear now why you made the decision not to have unknown propagate. My suggestion for updating the docs would be to add a sentence in every description of unknown for Schema(), load, and loads. See the blue text below as an example: Whether to exclude, include, or raise an error for unknown fields in the data. (Note: does not propagate to nested fields.) Use EXCLUDE, INCLUDE or RAISE.

…

On Wed, Jan 15, 2020 at 10:28 AM Jared Deckard ***@***.***> wrote: Discussion on proposals to recursively inherit unknown can be found on #980 <#980> and #1428 <#1428>. Any suggestions or PRs for updating the docs would be appreciated. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1490?email_source=notifications&email_token=AN36JUNLWT6S6GFFSN4IS5TQ55IVXA5CNFSM4KG2CYZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJBJZIA#issuecomment-574790816>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN36JUK2NTMYUPELALWZUKTQ55IVXANCNFSM4KG2CYZA> .

-- Pattabi Seshadri Engineering (Access)

[deckar01]

Seeing how many times this definition is duplicated, and considering how we just noticed the definitions for exclude were out of sync, we might want to consider defining once, and referencing with links. The restructured text code reference syntax should make this fairly painless.

[mahenzon]

Hi there! We definitely need to implement this functionality. Here's a live example:

from marshmallow import Schema, fields, EXCLUDE, INCLUDE


class SpamSchema(Schema):
    meat = fields.String()


class CanSchema(Schema):
    spam = fields.Nested(SpamSchema)


def main():
    can = CanSchema(unknown=EXCLUDE)

    # just passing expected data
    data = can.load({
        'spam': {'meat': 'pork'},
    })
    print(data)
    # prints {'spam': {'meat': 'pork'}}

    # passing unknown field at the top level
    data = can.load({
        'spam': {'meat': 'pork'},
        'foo': 'bar'
    })
    print(data)
    # prints {'spam': {'meat': 'pork'}}

    # or with include
    data = can.load(
        {
            'spam': {'meat': 'pork'},
            'foo': 'bar'
        },
        unknown=INCLUDE,
    )
    print(data)
    # prints {'spam': {'meat': 'pork'}, 'foo': 'bar'}

    # now passing unknown in a nested field
    can.load({
        'spam': {
            'meat': 'pork',
            'add-on': 'eggs',
        },
    })
    # fails with marshmallow.exceptions.ValidationError: {'spam': {'add-on': ['Unknown field.']}}

    # even if stating directly, it does not help
    can.load({
        'spam': {
            'meat': 'pork',
            'add-on': 'eggs',
        },
    }, unknown=EXCLUDE)
    # fails with marshmallow.exceptions.ValidationError: {'spam': {'add-on': ['Unknown field.']}}


if __name__ == '__main__':
    main()

[sirosen]

Looking at this issue from the perspective of webargs, I see two (related) issues with #1575. I think I have a good idea of how to resolve them, but want to see if people like the results before I try to write it.

• If unknown propagates automatically in any context (as in Propagate unknown parameter to nested fields #1575), it will be backwards incompatible
• If unknown behaves inconsistently between Meta, schema instantiation, and load, it will cause confusion

And then, as an ancillary concern, if someone is writing a library on top of marshmallow (👋 hi guys!), they'll want to be able to control this or offer control to their users.

Maybe backwards incompatibility isn't an issue in this case -- if it's considered a bug that's being fixed -- but I think it will be a problem for someone.

---

So my idea:

• add a new parameter in all of the same places as unknown, propagate_unknown
• propagate_unknown can be True or False (by default, for backwards compat) and can be removed (True everywhere) in marshmallow v4
• if propagate_unknown is True, behave as in Propagate unknown parameter to nested fields #1575 when unknown is passed to load, and if False, behave as today

I would take #1575 as a basis for this work. It looks solid, has nice tests, and credit where it's due -- I just don't quite agree with the behavior.

The downsides are that we have a new attribute to control unknown, which might feel messy, and that we don't get the desired behavior by default. To me, those are worthwhile in the first case and a necessary evil in the second.

You can do it all with just one attribute, but I promise that it's worse.

This is just for fun. Don't take this suggestion seriously. Please. 😄

We could change the constants a bit...

RAISE = 1
INCLUDE = 2
EXCLUDE = 4
PROPAGATE = 8
...
schema.load(data, unknown=EXCLUDE|PROPAGATE)

but then we're just asking for trouble because you have to check for INCLUDE|EXCLUDE, which makes no sense.