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

Modify Nested field to handle the combination of many=True and allow_… #1498

Closed
wants to merge 1 commit into from

Conversation

Meallia
Copy link

@Meallia Meallia commented Jan 31, 2020

Fix for #1497
Hi,

I tried to fix the issue by modifying Schema as suggested in the issue discussion but it would fail to detect validation errors when [None] is passed to a Nested(many=True, allow_none=False) field as the schema is not aware of allow_none.

Instead, I made the Nested field check when (de-)serializing if many and allow_none are both set to True. in that case, we simply pass the non-None values to the underlying schema then insert None values back in the resulting list.

@deckar01
Copy link
Member

deckar01 commented Feb 3, 2020

it would fail to detect validation errors when [None] is passed to a Nested(many=True, allow_none=False)

That is existing behavior and orthogonal to #1497.


def test_nested_multiple_allow_none_deserialize_none_to_none(self):
field = fields.Nested(Schema(), allow_none=True, many=True)
assert field.deserialize([None, {}]) == [None, {}]
Copy link
Member

Choose a reason for hiding this comment

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

The allow_none argument on Nested applies to the Nested container, not its children. Depending on many this is either a List, or a pass through container.

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

Use List(Nested(allow_none)) instead. #779 is an RFC that is amassing reasons Nested(many) should be deprecated. I will add this to the list.

@Meallia
Copy link
Author

Meallia commented Feb 4, 2020

Oh, yes, you're absolutely right about allow_none for nested attributes. So I guess we only need to revert the optimisation on lists of nested attributes.

Is this merge request still relevant then ?

Sorry for the delay, I could not find the time to follow up on the related issue but I will I'm the coming days.

@lafrech
Copy link
Member

lafrech commented Feb 19, 2020

#1497 was fixed by #1503.

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.

3 participants