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

Marshmallow Schema construction should be handled by Parser #73

Closed
DamianHeard opened this issue Oct 21, 2015 · 8 comments

Comments

@DamianHeard
Copy link
Contributor

commented Oct 21, 2015

Hi @sloria. Loving the new marshmallow integration! Thanks so much.

I had a suggested tweak to the API I was hoping to run past you. Presently marshmallow schemas are used like:

# example A
@use_args(UserSchema())
def my_pryamid_view(request, args):
    pass

I suggest it makes sense to push the construction of the schema instance into the core.Parser becoming:

# example B
@use_args(UserSchema)
def my_pyramid_view(request, args):
    pass

This has a number of benefits:

  • The current approach requires API users to explicitly set strict=True either in the schema constructor or the schema Meta class. This could be ensured internally if core.Parser handles construction.
  • Since webargs presently requires schema instances be constructed at module scope and usage of the marshmallow context will persist between requests. Re-constructing the schema each requests ensures no state can persist.
  • Presently core.Parser is already responsible for schema construction with dictionary based webargs usage. The proposed change would bring the two usages more into line.

Of course example A over simplifies the construction of the marshmallow schemas since there are a number of arguments that could be supplied with marshmallow construction. What we probably would require is either:

# example C
@use_args(UserSchema, only=('name', 'email'))
def my_pyramid_view(request, args):
    pass

or

# example D
@use_args(lambda: UserSchema(only=('name', 'email'))
def my_pyramid_view(request, args):
    pass

or

# example E
@use_args(UserSchema, schema_args={'only': ('name', 'email')})
def my_pyramid_view(request, args):
    pass

I dislike C because it utilizes kargs which I think have limitations should the use_args decorator, for example, require additional arguments be passed to core.Parser in later versions of the API.

I favour E over D as it allows us to address the issue of setting strict=True during schema construction.

This would also be compatible with webargs's dictionary based usage. So with dictionaries E becomes:

# example F
user_dict = {
    'name': field.String(),
    'email': field.Email(),
    'password': field.String(),
}

@use_args(user_dict, schema_args={'only': ('name', 'email')})
def my_pyramid_view(request, args):
    pass

I'm happy to put a PR together should there be interest in this change.

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 21, 2015

Thanks for the suggestion, @DamianHeard . I think this is worth considering. You make good points about the API here. One alternative I can think of would be to pass a callable that takes the request. That would enable use cases such as:

def make_user_schema(request):
    only = request.query['fields']
     # Once marshmallow 2.2 is released...
    partial = request.method == 'PATCH'
    return UserSchema(only=only, partial=partial)

@use_args(make_user_schema)
def my_view(request, args):
    #...

Thoughts?

@DamianHeard

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2015

Thanks for you're prompt reply.

So yes I think some sort of setup callable would definitely be useful. In particular, this would solve one of my up coming feature requests in advice: the ability to provide the request in the marshmallow context.

The downside to this suggestion is that it's less declarative as the schema associated with the view is now slightly harder to find in the callable. This might sound like a small thing but this is one of the things me and my co-workers enjoy most about using webargs.

Can I suggest the following iteration on you're suggestion. The callable should accept the schema class along with the request:

def  make_schema(cls, request):
    only = request.queryu['fields']
    partial = request.method == 'PATCH'
    return cls(only=only, partial=partial)


@use_args(UserSchema, callable=make_schema)
def my_view(request, args):
    # ...

This is more declarative and allows callables to be composed with schemas rather than be hard wired which should promote re-use.

I could for example do:

def use_args_with(cls, schema_args, **kwargs):
    return use_args(cls, lambda kls, request: kls(context={'request': request}, **schema_args), **kwargs)


 @use_args_with(UserSchema, schema_args={'only': ('name', 'email')})
def my_view(request, args):
    # ...

In the event a callable isn't provided I would suggest the Parser would just fall back on the schemas constructor.

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

I'm not sure adding a callable param is the most intuitive solution. What happens if a schema instance is passed? Would the callable have to handle both instances or classes? A function that returns an instance of a Schema seems more predictable.

You could still easily implement use_args_with :

def use_args_with(schema_cls, schema_kwargs=None, **kwargs):
    schema_kwargs = schema_kwargs or {}
    def maker(request):
        return schema_cls(only, partial, **schema_kwargs)
    return use_args(maker, **kwargs)

@use_args_with(UserSchema, schema_kwargs={'only': ('name', 'email')})
def my_view(request, args):
@DamianHeard

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2015

How would you modify this approach to work when the schema is specified as a dict? Since the Parser is presently responsible for constructing the schema it wouldn't be available in the closure.

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

How would you modify this approach to work when the schema is specified as a dict?

def use_args_with(schema_cls, schema_kwargs=None, **kwargs):
    if not issubclass(schema_cls, Schema):
        schema_cls = argmap2schema(schema_cls)
    schema_kwargs = schema_kwargs or {}
    def maker(request):
        return schema_cls(only, partial, **schema_kwargs)
    return use_args(maker, **kwargs)

Edit: Fix conditional.

@DamianHeard

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2015

Thanks for your speedy response.

While I don't love that it will certainly produce the required result. The main downside I see is that between the various parser implemention and the decorators there's already a lot of places converting dicts objects to ma.Schema and this would continue that proliferation rather than taking advantage of what's already been implemented.

I agree that in my suggestion supplying callable a schema has already been instantiated is far from clearly defined. If you recall my earlier points I was leaning towards ultimately removing support for passing in instantiated schemas for the following reasons:

  • The current approach requires API users to explicitly set strict=True either in the schema constructor or the schema Meta class. This could be ensured internally if core.Parser handles construction.
  • Since webargs presently requires schema instances be constructed at module scope and usage of the marshmallow context will persist between requests. Re-constructing the schema each requests ensures no state can persist.
  • Presently core.Parser is already responsible for schema construction with dictionary based webargs usage. The proposed change would bring the two usages more into line.

That said I can see this might be a major departure from your thoughts and maybe entirely unpalatable. In addition to this, I don't for example have a solution of how one would easily supply constructor arguments to the schema without also using a callable or making further modifications to the decorators.

I'm still happy to put a PR together if you're happy for me to push it over the line in my own time. If I come up with an alternative approach I'll be sure to slap it up here for discussion prior to then.

@sloria

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

Good points in favor of passing a Schema class over an instance. However, I'm not sure we want to completely drop support for passing instances. Passing an instance can be useful (1) when the constructor args don't need to be dynamic and (2) for performance reasons (avoiding the overhead of instantiating a Schema).

The main advantage of allowing a callable is that it enforces the mutual exclusivity of passing a dict vs. Schema class vs. Schema instance.

@sloria

This comment has been minimized.

Copy link
Member

commented Nov 7, 2015

Added in f77b288

@sloria sloria closed this Nov 7, 2015

sloria added a commit that referenced this issue Nov 7, 2015

Update docs and bump dev version
- Update changelog
- Add @DamianHeard to AUTHORS
- Add Schema Factories section to Quickstart
- Document `use_args_with` pattern from
  #73 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.