Marshmallow: Allow callables for fields.Nested

[taion]

Suggested earlier here: https://github.com/marshmallow-code/marshmallow/issues/19#issuecomment-43685498.

The suggestion would be to allow:

foo = fields.Nested(lambda: Foo)

in addition to

foo = fields.Nested('Foo')

Seems a bit cleaner in general.

[sloria]

I'm -0 on this. I think allowing a callable is fine, but I don't think it justifies the cost of an extra typecheck and a redundant API.

[taion]

Thinking about it a bit more, I probably wouldn't actually use this. This could potentially be slightly cleaner if there were name conflicts or something, because it doesn't require a global registry, but in practice it's just not that big a deal.

[sloria]

Fair enough. FWIW, you can provide the full import path if there are name conflicts:

foo = fields.Nested('path.to.Foo')
foo2 = fields.Nested('path.to.other.Foo')

[taion]

Yup - but seems a little cleaner to use whatever you have in scope. But in practice I'm just not seeing the use case I was imagining, and the lambda: just looks a bit uglier.

[sloria]

Reopening this because I think this would enable valid use cases that are currently unmet.

(1) Passing arbitrary arguments to the nested Schema (only many, exclude, and only are currently supported. This could solve https://github.com/marshmallow-code/marshmallow/issues/285#issuecomment-152577723.

# Pass arbitrary arguments
foo = fields.Nested(lambda: FooSchema(partial=True))

# https://github.com/marshmallow-code/marshmallow/issues/285#issuecomment-152577723
class TreeSchema(Schema):
    tree = fields.Nested(lambda: TreeSchema(many=True, strict=True))

(2) With a slight modification to the OP's proposed API, users could easily implement a polymorphic nested field.

def make_schema(obj):
    if isinstance(obj, Foo):
        return FooSchema()
    elif isinstance(obj, Bar):
        return BarSchema()

class MySchema(Schema):
    poly = fields.Nested(make_schema)

Feedback welcome.

[justanr]

Seems like a better twist on the polymorphic nested field. Would it be practical to apply this to List as well?

[sloria]

@justanr Definitely a possibility.

[justanr]

I'll play with an implementation either when I get home or in the morning, but I have a general idea of what it'll look like. Unless @taion wants to tackle it.

[taion]

This is interesting - would you require the callable to return a schema object, or should this still support the lambda returning a schema class? The latter is less useful but having it would keep consistency with e.g. SQL Alchemy.

[taion]

I don't have time to code this up this weekend - I'm on front-end duty for a bit.

[justanr]

I think it should return an object, that way it's not kicked through the check again. Possibly require a certain signature that accepts things like obj, only, etc, but I'm not sure on this. I'll mull it over some.

[taion]

Just for reference, I'm referring to this: http://docs.sqlalchemy.org/en/rel_1_0/orm/relationship_api.html#sqlalchemy.orm.relationship

[sloria]

Yeah, returning an instance makes the most sense to me. I don't see any advantage to returning a class, unless there's some use case I'm not thinking of.

[taion]

I can't think of any beyond "the SQLAlchemy relationship API looks that way, so maybe users will try to return the schema class and expect it to work".

[taion]

Yeah, scratch that, there's no way that's worth expanding the API surface area.

[justanr]

Hm, playing with this now and I'm not seeing a good way to implement the polymorphic idea, since the schema is built in Nested.schema well before the object is even passed to it, there's a check against the return type because I found without it, a few other tests break.

Any thoughts?

Also, as an aside, is there any desire to bring the singledispatch library in as a dependency? I understand the desire for no outside dependencies, but it'd probably go a long way towards cleaning up some of the if isinstance(thing, ThisType)/elif isinstance(thing, ThatType).

[taion]

I think it's separable - the API in https://github.com/marshmallow-code/marshmallow/issues/302#issuecomment-152747046 (2) is actually incomplete, because it doesn't specify how to deserialize objects at all. Probably easiest to get the non-polymorphic version in place first.

Arguably something like a polymorphic schema should be expressed at the schema level rather than at the fields.Nested level anyway. But I'm not entirely sure what such a thing would be for.

[justanr]

I've made use of a polymorphic field. Consider an online music player. Tracklists can belong to Artists (then they're called albums) or users (then they're playlists). I found it simpler to use a polymorphic field than do type checking when pulling all the tracklists out.

I only mentioned that because @sloria used it as an example.

[taion]

That makes sense for serialization, but I don't think the API proposed above gives you a way to handle this for deserialization. But I think regardless there's a more general solution here that doesn't tie this polymorphism to the Nested field type.

[justanr]

True, I'll leave it be. There's already external examples of Polymorphic fields and @sloria has expressed desire to keep it that way, specifically because of the deserialization issue.

[justanr]

318 didn't back reference for some reason, just making a note here.

[taion]

I think GH only picks up references in the body of a PR message, not in the title.

[taion]

If #318 is out, should this issue be closed again?

[sloria]

Yes, thanks.

[cancan101]

Is there a TL; DR why this was closed again?

[taion]

See https://github.com/marshmallow-code/marshmallow/pull/318#issuecomment-153345283.

I'm not entirely convinced, though. In practice the string was fine. A lambda is cleaner, a bit, but it could be dynamic, which is odd.

[cancan101]

I would be +1 on using lambda with no args to forward reference Schemas along the lines of what sqla allows. In addition to cleaner, refactoring tools like the reference rather than the string.

[deckar01]

Slightly off topic: PEP 563 landed in python 3.7, which adds deferred resolution to type annotations. The annotations can't being accessed by a metaclass at declaration time though. Probably not a viable solution any time soon, but something to think about in the __future__.

from __future__ import annotations

class Foo:
    foo: Foo

[taion]

There's no real API for using that in this context, though, is there?

[deckar01]

Not in marshmallow currently, but it is a schema syntax I have been investigating. Also, https://github.com/justanr/marshmallow-annotations/issues/5.

Back on track: I don't think Method and Function are viable substitutes for a complete Nested field. The suggested workaround is not something I would expect developers to discover on their own or something I would want to document. Strings work well and are easy to document/use for recursively schema references. If a callable is needed for more complex behavior, a custom field is probably the best tool for the job.

[cancan101]

To be clear, I'm not suggesting using the callable for complex behavior; rather I just want it to allow me to reference a Schema before it is declared. My usage case is entirely syntactical.

[deckar01]

https://github.com/marshmallow-code/marshmallow/issues/302#issuecomment-149735890

That was the original motivation for the issue and it was not considered a strong use case for adding a second way to achieve existing functionality. A custom field can be made to achieve this syntax.

https://marshmallow.readthedocs.io/en/3.0/custom_fields.html

[cancan101]

I agree that this code is a bit rough, though it can probably be cleaned up to make adding the callable easier:

            # Inherit context from parent.
            context = getattr(self.parent, 'context', {})
            if isinstance(self.nested, SchemaABC):
                self.__schema = self.nested
                self.__schema.context.update(context)
            elif isinstance(self.nested, type) and \
                    issubclass(self.nested, SchemaABC):
                self.__schema = self.nested(many=self.many,
                        only=only, exclude=self.exclude, context=context,
                        load_only=self._nested_normalized_option('load_only'),
                        dump_only=self._nested_normalized_option('dump_only'))
            elif isinstance(self.nested, basestring):
                if self.nested == _RECURSIVE_NESTED:
                    parent_class = self.parent.__class__
                    self.__schema = parent_class(many=self.many, only=only,
                            exclude=self.exclude, context=context,
                            load_only=self._nested_normalized_option('load_only'),
                            dump_only=self._nested_normalized_option('dump_only'))
                else:
                    schema_class = class_registry.get_class(self.nested)
                    self.__schema = schema_class(many=self.many,
                            only=only, exclude=self.exclude, context=context,
                            load_only=self._nested_normalized_option('load_only'),
                            dump_only=self._nested_normalized_option('dump_only'))
            else:
                raise ValueError('Nested fields must be passed a '
                                 'Schema, not {0}.'.format(self.nested.__class__))

[taion]

The motivation here wasn't actually to allow dynamic nested schemas – it was to have a more static-analysis friendly way to resolve dependency issues.

Something like:

child = fields.Nested(lambda: ChildSchema)

is more amenable to checks from e.g. Flake8 than using 'ChildSchema'.

As with SQLAlchemy, non-determinism here shouldn't work.

[deckar01]

from marshmallow import Schema, fields

class CallableNested(fields.Nested):
    @property
    def nested(self):
        return self.callable()

    @nested.setter
    def nested(self, callable):
        self.callable = callable

class Test(Schema):
    test = CallableNested(lambda: Test)

Test().declared_fields['test'].schema
# <Test(many=False, strict=False)>

[taion]

sure – that's the only thing we're asking for here. it would be convenient if this syntax were available out-of-the-box. coming from being used to sqlalchemy, it's a little surprising that the string is the _only_ way.