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

Allow passing lambda functions to Nested #1382

Merged
merged 16 commits into from
Dec 6, 2019
Merged

Allow passing lambda functions to Nested #1382

merged 16 commits into from
Dec 6, 2019

Conversation

sloria
Copy link
Member

@sloria sloria commented Sep 8, 2019

This allows passing a schema factory function to Nested. This enables passing constructor arguments to circularly-referenced schemas and also allows for self-nesting without the 'self' magic.

class ChildSchema(Schema):
    id = fields.Str()
    name = fields.Str()
    parent = fields.Nested(lambda: ParentSchema(enveloped=True, ...))
    siblings = fields.List(fields.Nested(lambda: ChildSchema(...)))

class ParentSchema(Schema):
    ...

Closes #1146
Replaces #1151

@sloria sloria requested review from lafrech and deckar01 September 8, 2019 18:39
@sloria
Copy link
Member Author

sloria commented Sep 9, 2019

Will this cause any problems for apispec? @lafrech

@taion
Copy link
Contributor

taion commented Sep 9, 2019

Oh this is neat. Do you think there's any value to supporting the SQLAlchemy-style syntax of lambda: FooSchema that doesn't construct the instance? I guess not, given the somewhat different semantics here.

@sloria
Copy link
Member Author

sloria commented Sep 9, 2019

It's not explicitly tested because I don't think we need to officially support that usage, but it should work anyway.

@lafrech
Copy link
Member

lafrech commented Sep 9, 2019

Will this cause any problems for apispec? @lafrech

I suppose it wouldn't since the nested field instance is resolved on first call to schema cached property.

sloria added a commit that referenced this pull request Sep 10, 2019
This generalizes slightly better to accommodate #1382
@sloria
Copy link
Member Author

sloria commented Sep 10, 2019

Deprecating only and exclude on Nested sort of deprecates the Nested('MySchema', only=.., exclude=...) usage, indirectly. If the string name usage is deprecated then removed, the class_registry becomes unused within marshmallow. Do we want to keep support for the class_registry moving forward, or should we drop it?

Maybe we hold off on deprecating the string name usage (as well as the only and exclude params, by extension). It's a convenient way to avoid circular imports.

It's worth noting that SQLAlchemy also supports string names to relationship.

@sloria
Copy link
Member Author

sloria commented Sep 10, 2019

We could just document lambda as the preferred way, with a sidenote about using string names for avoiding circular imports.

@lafrech
Copy link
Member

lafrech commented Sep 11, 2019

So finally we'd have

  • the string way to avoid circular imports but without the ability to pass constructor arguments
  • the lambda way to pass constructor arguments but circular imports issues

That's double API surface with still unsupported cases.

Standard case Constructor args Circular import Constructor args + Circular import
string X X
lambda X X

The last case could be covered with string + a constructor_(kw)args parameter. Would we still need lambda if we did that?

@sloria
Copy link
Member Author

sloria commented Sep 11, 2019

The lambda/callable usage can actually handle the circular import, but in a clunky way.

def get_artist_schema():
    from .artists import ArtistSchema
    return ArtistSchema()

class AlbumSchema(Schema):
    artist = fields.Nested(get_artist_schema)

In most cases, lambda is strictly better than string names because it doesn't break static analysis. The string usage is only provided for convenience in the circular import cases. We could decide to deprecate/remove it later.

So it's a bit more like

Standard case Constructor args Circular import
string X (clunky) X
lambda X X X (clunky)

@Kamforka
Copy link

Oh yeah, that was the reason behind my implementation of the string constructor and argument passing, to avoid circular imports, now I remember.

Regarding to static analysis / linting, I guess you are also breaking the import guidelines by not having imports at the top of your modules, but by lazily importing the schemas in your factory function, it's clunky indeed.

Unfortunately both ways have their lows and ups but I guess only one should be supported not both.

@sloria
Copy link
Member Author

sloria commented Sep 11, 2019

If I haaad to choose one way, it'd be the lambda. It meets all use cases and doesn't break go-to in my editor =P

@lafrech @Kamforka What do you think about

  1. Removing the deprecations from this PR
  2. Deprecating Nested('self')
  3. Documenting lambda as the preferred way (updating all existing 2-way nesting examples), with a sidenote about the string way for circular import cases.
  4. Deciding in a future 3.x release whether we want to deprecate string/many/exclude.

@taion
Copy link
Contributor

taion commented Sep 11, 2019

For the lambda case, can you not do:

from . import artists

class AlbumSchema(Schema):
    artist = fields.Nested(lambda: artists.ArtistSchema())

As long as the evaluation is delayed, I think this should be okay? You do get a circular import, but it should still work.

@sloria
Copy link
Member Author

sloria commented Sep 11, 2019

@taion That will work sometimes but will break in cases like

# core.py
from . import artists


class EntityMixin:
    id = fields.UUID()

class AlbumSchema(EntityMixin, Schema):
    artist = fields.Nested(lambda: artists.ArtistSchema())
# artists.py

from marshmallow import Schema, fields
from . import core


class ArtistSchema(core.EntityMixin, Schema):
    name = fields.Str()

@taion
Copy link
Contributor

taion commented Sep 11, 2019

Those cases seem avoidable, though. The string case carries an implicit requirement, too, that .artists gets imported at some point... just not necessarily from there.

@sloria
Copy link
Member Author

sloria commented Sep 11, 2019

Indeed, you can fix the above cases with some code re-org.

I still think we should iteratively move users toward the lambda usage, i.e. defer deprecation of string names until we decide all use cases can be met with lambda without too much finagling.

sloria added a commit that referenced this pull request Sep 12, 2019
* Respect dotted `only` and `exclude` on nested schema instances

* Refactor: use _init_fields to re-initialize fields based on `only` and `exclude`

Also, be explicit about which helper methods are private

* Update changelog

* Refactor only/exclude propagation

This generalizes slightly better to accommodate #1382

* Fix behavior when dumping multiple times

Copy schema to avoid unwanted sharing of `only` and `exclude`
across instances

* Remove unnecessary falsy checks

* Update changelog
@sloria sloria changed the title [WIP] Allow passing lambda functions to Nested Allow passing lambda functions to Nested Sep 13, 2019
@sloria
Copy link
Member Author

sloria commented Sep 13, 2019

Reverted the deprecations and updated the documentation. Shall we move forward with this? @lafrech @Kamforka

@s16h
Copy link

s16h commented Dec 4, 2019

Do we know what the ETA of merging this is?

@sloria
Copy link
Member Author

sloria commented Dec 4, 2019

@lafrech @deckar01 Can you take a look at this?

@lafrech
Copy link
Member

lafrech commented Dec 4, 2019

Sorry for the delay.

LGTM.

src/marshmallow/fields.py Outdated Show resolved Hide resolved
@Kamforka
Copy link

Kamforka commented Dec 4, 2019

I'm a bit late on this but my original solution was introduced to solve the circular import issue with nested schemas, when both schema nests the other.
I guess this one will still crash with a circular import error in that case, or am I missing something?

@sloria
Copy link
Member Author

sloria commented Dec 4, 2019

@Kamforka See #1382 (comment) . It's technically possible to avoid circular imports but it's klunky. The proposal here is to keep support for string names in the short term, but lambda: ... is the preferred API.

@sloria sloria merged commit 31b784f into dev Dec 6, 2019
@sloria sloria deleted the nested-lambda branch December 6, 2019 00:33
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.

How to pass parameters to a nested schema which is referred by name
6 participants