Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow use of Schema hooks on OneOfSchema #130

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Nov 10, 2020

NOTE: this change is very mildly backwards-incompatible, but since 3.0 is coming up for this lib, I think it's okay.

Rather than overriding the dump() and load() methods of the Schema class, override _serialize and _deserialize, which are the "concrete" steps in schema loading and dumping which handle loading or dumping on a field-by-field basis.

This still uses load() and dump() methods of the type schemas being used, but it happens between the various hooks which may run on the OneOfSchema instance.

Add a test that checks that a post_dump hook to remove the type field works.

The most significant downside of this change is that it makes use of several private APIs within marshmallow. Not only are _serialize and _deserialize private methods, but the error_store object which is used here is also considered private (per marshmallow docs).

In order to better guarantee behavior near-identical to marshmallow, several methods from marshmallow.utils have been copied in-tree here.

One notable API change here is that arbitrary keyword arguments are no longer being passed from OneOfSchema.load() and OneOfSchema.dump() down into the type schemas' load and dump methods. As a result, you cannot specify a load or dump parameter here and expect it to take effect.
With the switch to overriding _serialize and _deserialize, there is no practical way to pass parameters like that.

closes #4

Rather than overriding the `dump()` and `load()` methods of the Schema
class, override `_serialize` and `_deserialize`, which are the
"concrete" steps in schema loading and dumping which handle loading or
dumping on a field-by-field basis.

This still uses load() and dump() methods of the type schemas being
used, but it happens between the various hooks which may run on the
OneOfSchema instance.

Add a test that checks that a `post_dump` hook to remove the `type`
field works.

The most significant downside of this change is that it makes use of
several private APIs within marshmallow. Not only are `_serialize` and
`_deserialize` private methods, but the error_store object which is
used here is also considered private (per marshmallow docs).

In order to better guarantee behavior near-identical to marshmallow,
several methods from marshmallow.utils have been copied in-tree here.

One notable API change here is that arbitrary keyword arguments are no
longer being passed from `OneOfSchema.load()` and `OneOfSchema.dump()`
down into the type schemas' load and dump methods. As a result, you
cannot specify a load or dump parameter here and expect it to take
effect.
With the switch to overriding `_serialize` and `_deserialize`, there
is no practical way to pass parameters like that.

closes marshmallow-code#4
@sirosen
Copy link
Contributor Author

sirosen commented Nov 10, 2020

Reviewing existing issues, I believe this will also resolve #74 , since it allows marshmallow.Schema._do_load to be called.

@sirosen
Copy link
Contributor Author

sirosen commented Nov 23, 2020

Something which has been bothering me about this since I opened it is that load and dump were passing through arbitrary kwargs, which seemed odd. I just noticed that this was added in #111 to allow the arguments to load and dump to get to the child schemas. This change will break that use-case, and I'm not sure what to do about it.

On the one hand, this implementation strikes me as much simpler and makes all of the hooks work for free. On the other hand, I don't see how to make this work for the case cited in #111 without additional upstream changes in marshmallow. (If kwargs to load passed all the way down to _deserialize, we could use them that way.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OneOfSchema can't use pre/post_dump/load decorators
1 participant