Skip to content
This repository has been archived by the owner on Jul 21, 2022. It is now read-only.

Handle generating field for schema that isn't defined yet #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

justanr
Copy link
Owner

@justanr justanr commented Jan 31, 2021

Closes #5

Adding a new parameter to AbstractConverter is probably a breaking change :/ -- but can use the upgrade to Marshmallow 3 as an excuse to also bump major version. 👀

Anyways, idea here is that when we are generating a schema if we encounter a field that we don't know about yet, we can just create a kind of thunk field (appropriately named ThunkedField) that delays creating the actual field until it's requested, and then caches that field.

The ThunkedField will proxy all operations to the underlying field, creating it if necessary. This is potentially dangerous if there's a marshmallow plugin that'll poke at the created field BEFORE the other schema is created.

This first pass implementation actually improperly handles lists, optionals, etc by just generating thunks for them all since it happens pretty early in _get_field_from_typehint, but as a first pass works pretty well. Fixing this might require additionally updating TypeRegistry.get to accept an additional argument or adding a get_or_thunk method (which is probably better) in which case the ThunkedField can carry less information, just what the FieldFactory delegate declares

Risks (setting aside the new parameter thing):

The following would now generate a schema but fail at first serialization attempt -- rather than eagerly fail:

class Foo:
    bar: List["bar"] # WHOOPS!

class Bar:
    pass

Possible solution: Push a __THUNKED__: bool into the field settings and make the end user explicitly declare that a field is to be thunked:

class FooSchema(AnnotationSchema):
    class Meta:
        target = Foo

        class Fields:
            # explicitly opt into thunkiness rather than as the default for everything
            bar = {"__THUNKED__": True}

I like the explicit opt-in better, even if it's slightly cumbersome, since it prevents unexpected surprises down the line (leaving only expected surprises). And means that only convert needs to change since convert_all would receive the opt-in via the field configurations.

@justanr justanr self-assigned this Jan 31, 2021
@coveralls
Copy link

Pull Request Test Coverage Report for Build 124

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-97.5%) to 0.0%

Totals Coverage Status
Change from base Build 119: -97.5%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle forward references
2 participants