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

Add extra argument to load/dump for extensions #1725

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

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Jan 21, 2021

resolves #1726

I want to make some changes in marshmallow-oneofschema to support hooks (currently unsupported) and simplify the code.
Although it's just a PR, one of the issues I see with it is that it will not play nicely with marshmallow-sqlalchemy and there isn't a trivial way to get the information I need in _deserialize and _serialize.

I've been thinking about this on and off for a while and this approach is what I came up with.
It involves marshmallow being aware that someone might want to extend Schema by overloading _serialize and _deserialize, and a single new parameter to load(s) and dump(s) called extra. (Name inspired by stdlib logging's use of extra.)

Although neither marshmallow-oneofschema nor marshmallow-sqlalchemy are part of the core project, they're both important parts of a healthy ecosystem of marshmallow extension packages. Supporting their interplay is valuable.


The purpose of extra is to allow libraries built on top of marshmallow to interact with one another cleanly by providing a space for additional data to be passed which marshmallow knows about.

In particular, at present the functionality of marshmallow-sqlalchemy relies upon adding keyword arguments to load, and a proposed refactor of marshmallow-oneofschema would not be able to pass through these additional arguments to child schemas.
At the crux of this is a desire in marshmallow-oneofschema to override the behavior of _serialize and _deserialize to take advantage of the hooks provided by marshmallow. This would simplify OneOfSchema and provide additional features, but cannot handle the extra arguments added by marshmallow-sqlalchemy.

One option is for these external libraries to coordinate in order to support one another via some other (indeterminate) mechanism. However, in order for marshmallow to support such 3rd-party libraries, the key issue here is that there is no way to take extensible data from load() calls and pass it down through to the inner (de)serialization methods.

extra could be exposed in additional contexts, for example in field loading, hook evaluation, error handling, and so forth. However, the current issue in these third party libraries only requires the addition of extra to be passed down to _deserialize and _serialize. Therefore, this feature addition is restricted to only this context for now.

For this particular problem, this will allow a new major version of marshmallow-oneofschema to support usage with marshmallow-sqlalchemy something like so:

load(..., extra={"child_load_kwargs": {"transient": True}})

which is then unpacked and passed to a child sqlalchemy schema as load(..., transient=True).

In a future version, if marshmallow-sqlalchemy started using extra instead of added keyword arguments, marshmallow-oneofschema could pass through the extra data it received or define other sophisticated behaviors.

The purpose of `extra` is to allow libraries built on top of
`marshmallow` to interact with one another cleanly by providing a
space for additional data to be passed which `marshmallow` knows
about.

In particular, at present the functionality of
`marshmallow-sqlalchemy` relies upon adding keyword arguments to
`load`, and a proposed refactor of `marshmallow-oneofschema` would not
be able to pass through these additional arguments to child schemas.
At the crux of this is a desire in `marshmallow-oneofschema` to
override the behavior of `_serialize` and `_deserialize` to take
advantage of the hooks provided by marshmallow. This would simplify
OneOfSchema and provide additional features, but cannot handle the
extra arguments added by `marshmallow-sqlalchemy`.

One option is for these external libraries to coordinate in order to
support one another via some other (indeterminate) mechanism. However,
in order for `marshmallow` to support such 3rd-party libraries, the
key issue here is that there is no way to take extensible data from
`load()` calls and pass it down through to the inner
(de)serialization methods.

`extra` could be exposed in additional contexts, for example in field
loading, hook evaluation, error handling, and so forth. However, the
current issue in these third party libraries only requires the
addition of `extra` to be passed down to `_deserialize` and
`_serialize`. Therefore, this feature addition is restricted to only
this context for now.

For this particular problem, this will allow a new major version of
`marshmallow-oneofschema` to support usage with marshmallow-sqlalchemy
something like so:

    load(..., extra={"child_load_kwargs": {"transient": True}})

which is then unpacked and passed to a child sqlalchemy schema as
`load(..., transient=True)`.
Copy link
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't inspect thoroughly but the rationale makes sense.

loaded = MySchema().load(data, extra={"side_effect": "foo"})
assert MySchema.last_deserialize_side_effect == "foo"
assert MySchema.last_serialize_side_effect is None

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd clear last_deserialize_side_effect here to make next test more selective.

@deckar01
Copy link
Member

deckar01 commented Jan 21, 2021

Although it's just a PR, ... there isn't a trivial way to get the information I need in _deserialize and _serialize.

This seems like a problem that would benefit from opening an issue in marshmallow-oneofschema to explore possible solutions.

@sirosen
Copy link
Contributor Author

sirosen commented Jan 21, 2021

I almost opened a marshmallow issue to discuss, but thought it would be better to come to the table with some concrete option for how to handle this. If you prefer to discuss in an issue to a PR, I can open one.

marshmallow-oneofschema isn't the right place to tackle this because that library isn't really capable of solving the problem. I want to define a type of schema with special meanings for load and dump, but still inheriting all of the goodness of the main marshmallow codebase -- namely, support for pre- and post-processing hooks.
That will only work if marshmallow itself supports it, and this is the best way I've thought of to do that.

@sirosen
Copy link
Contributor Author

sirosen commented Jan 22, 2021

I've opened #1726 to lay out the issue and discuss potential avenues for supporting extensions of marshmallow which may want to customize Schema._deserialize and Schema._serialize. This PR now can be said to fix #1726 .

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.

Allow customized Schema _serialize and _deserialize to get custom data from load/dump calls
3 participants