From a9900054fd59334acfa7e2714df23a33ab374fde Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Fri, 4 Sep 2020 19:34:34 +0000 Subject: [PATCH] Make unknown propagation override child schemas In order to simplify, the value of `unknown` will not be respected in child schemas if `PROPAGATE` is used. This removes any need for the `auto_unknown` tracking, so there's less complexity added to schemas in order to implement this behavior. Adjust tests and changelog to match. Also make minor tweaks to ensure UnknownParam usage is consistent and keep the diff with the dev branch smaller. --- CHANGELOG.rst | 20 ++++++-------------- src/marshmallow/base.py | 4 +--- src/marshmallow/fields.py | 17 +++-------------- src/marshmallow/schema.py | 25 ++++--------------------- src/marshmallow/utils.py | 5 ++++- tests/test_schema.py | 36 +++++++++++++++--------------------- 6 files changed, 33 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 19d2f68e8..64ec9d987 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -8,24 +8,16 @@ Features: - The behavior of the ``unknown`` option can be further customized with a new value, ``PROPAGATE``. If ``unknown=EXCLUDE | PROPAGATE`` is set, then the - value of ``unknown=EXCLUDE | PROPAGATE`` will be passed to any nested - schemas which do not explicitly set ``unknown`` in ``Nested`` or schema - options. This works for ``INCLUDE | PROPAGATE`` and ``RAISE | PROPAGATE`` as - well. + value of ``unknown=EXCLUDE | PROPAGATE`` will be passed to any nested schemas. + This works for ``INCLUDE | PROPAGATE`` and ``RAISE | PROPAGATE`` as well. (:issue:`1490`, :issue:`1428`) - Thanks :user:`lmignon` and :user:`mahenzon`. .. note:: - When a schema is being loaded with ``unknown=... | PROPAGATE``, you can still - set ``unknown`` explicitly on child schemas. However, be aware that such a - value may turn off propagation at that point in the schema hierarchy. - - For example, a schema which specifies ``unknown=EXCLUDE`` will set - ``EXCLUDE`` for itself. But because the value is ``EXCLUDE`` rather than - ``EXCLUDE | PROPAGATE``, that setting will not be propagated to its - children, even if there is a parent schema which sets - ``unknown=EXCLUDE | PROPAGATE``. + When a schema is being loaded with ``unknown=... | PROPAGATE``, this will + override any values set for ``unknown`` in child schemas. Therefore, + ``PROPAGATE`` should only be used in cases in which you want to change + the behavior of an entire schema heirarchy. 3.7.0 (2020-07-08) ****************** diff --git a/src/marshmallow/base.py b/src/marshmallow/base.py index e3cbde749..6e739b350 100644 --- a/src/marshmallow/base.py +++ b/src/marshmallow/base.py @@ -38,9 +38,7 @@ def dump(self, obj, *, many: bool = None): def dumps(self, obj, *, many: bool = None): raise NotImplementedError - def load( - self, data, *, many: bool = None, partial=None, unknown=None - ): + def load(self, data, *, many: bool = None, partial=None, unknown=None): raise NotImplementedError def loads( diff --git a/src/marshmallow/fields.py b/src/marshmallow/fields.py index 808d59eb5..31026a316 100644 --- a/src/marshmallow/fields.py +++ b/src/marshmallow/fields.py @@ -494,7 +494,7 @@ def __init__( self.only = only self.exclude = exclude self.many = many - self.unknown = UnknownParam.parse_if_str(unknown) if unknown else None + self.unknown = UnknownParam.parse_if_str(unknown) self._schema = None # Cached Schema instance super().__init__(default=default, **kwargs) @@ -575,7 +575,7 @@ def _test_collection(self, value): def _load(self, value, data, partial=None, unknown=None): try: valid_data = self.schema.load( - value, unknown=unknown if unknown else self.unknown, partial=partial, + value, unknown=unknown or self.unknown, partial=partial, ) except ValidationError as error: raise ValidationError( @@ -593,18 +593,7 @@ def _deserialize(self, value, attr, data, partial=None, unknown=None, **kwargs): Add ``partial`` parameter. """ self._test_collection(value) - # check if self.unknown or self.schema.unknown is set - # however, we should only respect `self.schema.unknown` if - # `auto_unknown` is False, meaning that it was set explicitly on the - # schema class or instance - explicit_unknown = ( - self.unknown - if self.unknown - else (self.schema.unknown if not self.schema.auto_unknown else None) - ) - if explicit_unknown: - unknown = explicit_unknown - return self._load(value, data, partial=partial, unknown=unknown,) + return self._load(value, data, partial=partial, unknown=unknown) class Pluck(Nested): diff --git a/src/marshmallow/schema.py b/src/marshmallow/schema.py index 4770fd533..a2b2d0f7e 100644 --- a/src/marshmallow/schema.py +++ b/src/marshmallow/schema.py @@ -228,15 +228,7 @@ def __init__(self, meta, ordered: bool = False): self.include = getattr(meta, "include", {}) self.load_only = getattr(meta, "load_only", ()) self.dump_only = getattr(meta, "dump_only", ()) - # self.unknown defaults to "RAISE", but note whether it was explicit or - # not, so that when we're handling "propagate" we can decide - # whether or not to propagate based on whether or not it was set - # explicitly - self.unknown = getattr(meta, "unknown", None) - self.auto_unknown = self.unknown is None - if self.auto_unknown: - self.unknown = RAISE - self.unknown = UnknownParam.parse_if_str(self.unknown) + self.unknown = UnknownParam.parse_if_str(getattr(meta, "unknown", RAISE)) self.register = getattr(meta, "register", True) @@ -397,14 +389,7 @@ def __init__( self.load_only = set(load_only) or set(self.opts.load_only) self.dump_only = set(dump_only) or set(self.opts.dump_only) self.partial = partial - self.unknown = ( - UnknownParam.parse_if_str(unknown) - if unknown is not None - else self.opts.unknown - ) - # if unknown was not set explicitly AND self.opts.auto_unknown is true, - # then the value should be considered "automatic" - self.auto_unknown = (not unknown) and self.opts.auto_unknown + self.unknown = UnknownParam.parse_if_str(unknown) or self.opts.unknown self.context = context or {} self._normalize_nested_options() #: Dictionary mapping field_names -> :class:`Field` objects @@ -775,7 +760,7 @@ def loads( if invalid data are passed. """ data = self.opts.render_module.loads(json_data, **kwargs) - return self.load(data, many=many, partial=partial, unknown=unknown,) + return self.load(data, many=many, partial=partial, unknown=unknown) def _run_validator( self, @@ -857,9 +842,7 @@ def _do_load( error_store = ErrorStore() errors = {} # type: typing.Dict[str, typing.List[str]] many = self.many if many is None else bool(many) - unknown = UnknownParam.parse_if_str( - unknown if unknown is not None else self.unknown - ) + unknown = UnknownParam.parse_if_str(unknown or self.unknown) if partial is None: partial = self.partial # Run preprocessors diff --git a/src/marshmallow/utils.py b/src/marshmallow/utils.py index 3eab1f46e..3a99a015f 100644 --- a/src/marshmallow/utils.py +++ b/src/marshmallow/utils.py @@ -51,7 +51,10 @@ def __repr__(self): @classmethod def parse_if_str(cls, value): - """Given a string or UnknownParam, convert to an UnknownParam""" + """Given a string or UnknownParam, convert to an UnknownParam + + Preserves None, which is important for making sure that it can be used + blindly on `unknown` which may be a user-supplied value or a default""" if isinstance(value, str): return cls(value) return value diff --git a/tests/test_schema.py b/tests/test_schema.py index 7915ba178..59e166222 100644 --- a/tests/test_schema.py +++ b/tests/test_schema.py @@ -2894,12 +2894,11 @@ class DefinitelyUniqueSchema(Schema): assert SchemaClass is DefinitelyUniqueSchema -def test_propagate_unknown_stops_at_explicit_value_for_nested(): - # PROPAGATE should traverse any "auto_unknown" values and - # replace them with the "unknown" value from the parent context (schema or - # load arguments) - # this test makes sure that it stops when a nested field or schema has - # "unknown" set explicitly (so auto_unknown=False) +def test_propagate_unknown_overrides_explicit_value_for_nested(): + # PROPAGATE should traverse any schemas and replace them with the + # "unknown" value from the parent context (schema or load arguments) + # this test makes sure that it takes precedence when a nested field + # or schema has "unknown" set explicitly class Bottom(Schema): x = fields.Str() @@ -2923,14 +2922,13 @@ class Top(Schema): assert result == { "x": "hi", "y": "bye", - "child": {"x": "hi", "y": "bye", "child": {"x": "hi"}}, + "child": {"x": "hi", "y": "bye", "child": {"x": "hi", "y": "bye"}}, } -def test_propagate_unknown_stops_at_explicit_value_for_meta(): - # this is the same as the above test of unknown propagation stopping where - # auto_unknown=False, but it checks that this applies when `unknown` is set - # by means of `Meta` +def test_propagate_unknown_overrides_explicit_value_for_meta(): + # this is the same as the above test of unknown propagation, but it checks that + # this applies when `unknown` is set by means of `Meta` as well class Bottom(Schema): x = fields.Str() @@ -2939,25 +2937,21 @@ class Middle(Schema): x = fields.Str() child = fields.Nested(Bottom) - # set unknown explicitly here, so auto_unknown will be - # false going into Bottom, and also set propagate to make it propagate - # in this case class Meta: - unknown = EXCLUDE | PROPAGATE + unknown = EXCLUDE class Top(Schema): x = fields.Str() child = fields.Nested(Middle) - # sanity-check that auto-unknown is being set correctly - assert Top().auto_unknown - assert not Top(unknown=INCLUDE | PROPAGATE).auto_unknown - assert not Middle().auto_unknown - data = { "x": "hi", "y": "bye", "child": {"x": "hi", "y": "bye", "child": {"x": "hi", "y": "bye"}}, } result = Top(unknown=INCLUDE | PROPAGATE).load(data) - assert result == {"x": "hi", "y": "bye", "child": {"x": "hi", "child": {"x": "hi"}}} + assert result == { + "x": "hi", + "y": "bye", + "child": {"x": "hi", "y": "bye", "child": {"x": "hi", "y": "bye"}}, + }