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

Nested schema doesn't load nested field if many is not set in both Schema and field constructors #1160

Closed
Kamforka opened this issue Feb 26, 2019 · 8 comments

Comments

@Kamforka
Copy link

commented Feb 26, 2019

I found a strange behaviour of the Nested field when I use instantiated schemas for its nested parameter. To reproduce one can use the following snippet:

from marshmallow import Schema, fields


class BookSchema(Schema):
    id = fields.Int()
    title = fields.String()

    class Meta:
        strict = True


class UserSchema(Schema):
    id = fields.Int()
    name = fields.String()
    books = fields.Nested(BookSchema(many=True)) 

    class Meta:
        strict = True


books = [{'id': 1, 'title': 'First book'}, {'id': 2, 'title': 'Second book'}]
user = {'id': 1, 'name': 'Peter', 'books': books}

user_dump = UserSchema().dump(user).data
# output: {'id': 1, 'books': {}, 'name': 'Peter'}

user_load = UserSchema().load(user_dump).data
# output: marshmallow.exceptions.ValidationError: {'books': {'_schema': ['Invalid input type.']}}

I'd expect it to return a list of books since I set the many=True parameter of the schema constructor.

A workaround is to set many in both the schema and the field like this:

books = fields.Nested(BookSchema(many=True), many=True)

And the outputs are as expected:

user_dump = UserSchema().dump(user).data
# output: {'name': 'Peter', 'books': [{'title': 'First book', 'id': 1}, {'title': 'Second book', 'id': 2}], 'id': 1}
user_load = UserSchema().load(user_dump).data
# output: {'name': 'Peter', 'books': [{'title': 'First book', 'id': 1}, {'title': 'Second book', 'id': 2}], 'id': 1}

Note:
I know I can setup the schema this way and it works:
books = fields.Nested(BookSchema, many=True)
However my initial intention was to use it this way:
books = fields.Nested(BookSchema(custom_arg='A custom arg to be set', many=True))

I'm using marshmallow==2.16.3.

@deckar01

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

I imagine this will also be fixed by #1151.
We may want to consider dropping support for schema instances in the Nested arg since we reconstruct it and loose custom parameters.

@deckar01 deckar01 added the bug label Feb 26, 2019

@Kamforka

This comment has been minimized.

Copy link
Author

commented Feb 26, 2019

@deckar01 This means that in the future one may only use nested fields by passing a schema class or a name reference to the nested parameter of fields.Nested?

@deckar01

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

If we are going to offer a way to fully customize the nested schema construction in #1151, and the the current way only works for some use cases, it seems like a good idea to get rid of the old interface to avoid the pitfall. I wouldn't worry about it in #1151, but I think this is a good issue to have the discussion on.

@Kamforka

This comment has been minimized.

Copy link
Author

commented Feb 27, 2019

In this case I would not rely on the introduced schema_args, but rather pass each parameter defined in the field constructor. Something like this:

fields.Nested('MySchema', many=True, partial=False, only=('this', 'that'), custom_arg='a custom arg')

And I'd instantiate MySchema by passing everything from the field constructor to the schema constructor.

What do you think?

@deckar01

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

Currently extra kwargs go to the base Field constructor. That is how all field args work and probably should not change. Only a few named args are currently passed to the schema constructor. only and exclude are simply passed through, so it would no longer be necessary to explicitly pass them. many is the only argument that is actually needed in both the field and the schema.

Fixing this in v2 would probably require porting #1151 to v2 and adding logic to pull many out of the field args into the new schema args.

books = fields.Nested(
    BookSchema,
    many=True,
    schema_args={'custom_arg': 'A custom arg to be set'},
)

Detour: I think the underlying issue is many being coupled to Schema. It is purely there to allow dumping/loading data that is a list at the top level. If Schema and Field shared the same interface for loading/dumping, then many could be replaced with fields.List and fields.Nested could be replaced by using a schema directly. It could also avoid the need to pass schema args at all and make passing an instance work as expected without the need to rebuild it from the class.

item_schema = ItemSchema(custom_arg='A custom arg to be set')
list_schema = fields.List(item_schema)
list_schema.load([{...}, {...}]

That said, new field interfaces are probably out of scope for a v3 MR. I would probably just drop only and exclude from Nested and add special handling to copy many from the field args into the new schema args.

@Kamforka

This comment has been minimized.

Copy link
Author

commented Feb 28, 2019

I see your point, however the way I like using nested schemas is to refer to them by name, this way I can avoid circular import errors easily.

That was the intention behind the introduction of the schema_args parameter for the fields.Nested field.

@sloria

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

I was able to reproduce this on both 2.x-line and dev. In marshmallow 3, you should use List(Nested(...))), but this should be fixed anyway.

Will release a fix soon.

@sloria

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

Fixed on v2 and v3

@sloria sloria closed this Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.