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

Unenveloping None values doesn't work as expected #347

Closed
tdevelioglu opened this Issue Dec 1, 2015 · 7 comments

Comments

Projects
None yet
5 participants
@tdevelioglu

tdevelioglu commented Dec 1, 2015

Trying to unenvelope serialized data when the contained data is None continues deserialization with the original data. This makes it impossible to deserialize nested fields with enveloped data where None is a valid value. (Related: http://jsonapi.org/format/)

Example (let's pretend our system allows for homeless employees):
https://gist.github.com/tdevelioglu/8e433355494ba022f48f
(I expect a deserialized Resource object, with an attribute 'address' with value 'None')

@sloria

This comment has been minimized.

Member

sloria commented Dec 8, 2015

Thanks for the report. This does appear to be a limitation of the pre_/post_ methods. The decorators were meant to allow for in-place modifications of data and no return values (i.e. implicitly returning None).

We'll have to give this more thought to reach a solution.

@sloria

This comment has been minimized.

Member

sloria commented May 17, 2018

@taion @lafrech @deckar01 Any ideas on this?

We could make it so that pre_/post_ methods must return the processed data and allow None to be returned. This would be a breaking change, which is why I'd like to resolve this for 3.0.

@deckar01

This comment has been minimized.

Member

deckar01 commented May 17, 2018

The method receives the deserialized data and returns the processed data.

https://marshmallow.readthedocs.io/en/latest/api_reference.html#marshmallow.decorators.post_load

A return value of None indicating that the data was modified in place is not part of the docs. Anyone not returning a value is relying on undocumented behavior, but I can see how a lot of people could be relying on this unknowingly.

I recommend documenting the None behavior in the 2.x-line docs in addition to fixing it in 3.0.

@lafrech

This comment has been minimized.

Member

lafrech commented May 17, 2018

I also think this is the right thing to do.

The change in the code should be easy: just remove if_none stuff.

I like this. It fixes the use case while stripping unnecessary complication from the code.

@sloria

This comment has been minimized.

Member

sloria commented May 17, 2018

Thanks guys. Let's make it so!

@taion

This comment has been minimized.

Contributor

taion commented May 17, 2018

This was my fault, wasn't it? Yeah, in retrospect trying to handle "not returning" from those hooks was way too cute. Sorry!

@deckar01

This comment has been minimized.

Member

deckar01 commented May 17, 2018

It was discussed at the time, it just wasn't documented. It's never too late to add some docs. 😉

#191 (comment)

lafrech added a commit that referenced this issue May 18, 2018

lafrech added a commit that referenced this issue May 24, 2018

lafrech added a commit that referenced this issue May 24, 2018

@sloria sloria closed this in #819 May 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment