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

Use marshmallow under the hood #61

Closed
sloria opened this issue Sep 20, 2015 · 11 comments

Comments

@sloria
Copy link
Member

commented Sep 20, 2015

Proposal

Replace Arg objects with marshmallow fields. Use marshmallow for validation.

Why?

Webargs and marshmallow have redundant functionality for validating input data against a user-defined schema. Marshmallow does a better job of than webargs in a number of respects:

  • Nested validation is much more powerful in marshmallow. marshmallow's Nested field would solve #39 and #51.
  • Marshmallow does error bundling. Solves #58
  • Marshmallow handles null and missing inputs in a more predictable way.
  • Marshmallow has more consistent error messaging.
  • Marshmallow's validation is more stable and well-tested

Proposed API

from webargs import use_kwargs, fields


@use_kwargs({
    'title': fields.Str(required=True, location='json'),
    'description': fields.Str(required=False),
})
def myview(title, description):
    return 'title: {}, description: {}'.format(title, description)


# with nesting
@use_kwargs({
    'data': fields.Nested({
        'title': fields.Str(),
        'description': fields.Str(),
    })
})
def myview(data):
    return 'title: {}, description: {}'.format(**data)


# respects load_from
@use_kwargs({
    'x_custom_header': fields.Str(load_from='X-Custom-Header')
})
def myview(x_custom_header):
    return 'Header: {}'.format(x_custom_header)


# Passing in a Schema
from marshmallow import Schema

class PostSchema(Schema):
    title = fields.Str(required=True, location='json')
    description = fields.Str(required=False)
    id = fields.Int(dump_only=True)  # won't be included in parsed output

@use_kwargs(PostSchema)
def myview(title, description):
    return 'title: {}, description: {}'.format(title, description)
@marcellarius

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2015

This seems like a good solution to me.

On Mon, Sep 21, 2015 at 9:26 AM, Steven Loria notifications@github.com
wrote:

Proposal

Replace Arg objects with marshmallow fields. Use marshmallow for
validation.
Why?

Webargs and marshmallow have redundant functionality for validating input
data against a user-defined schema. Marshmallow does a better job of than
webargs in a number of respects:

  • Nested validation is much more powerful in marshmallow.
    marshmallow's Nested field would solve #39
    #39 and #51
    #51.
  • Marshmallow does error bundling. Solves #58
    #58
  • Marshmallow handles null and missing inputs in a more predictable
    way.
  • Marshmallow has more consistent error messaging.
  • Marshmallow's validation is more stable and well-tested

Proposed API

from webargs import use_kwargs, fields

@use_kwargs({
'title': fields.Str(required=True, location='json'),
'description': fields.Str(required=False),
})def myview(title, description):
return 'title: {}, description: {}'.format(title, description)

with nesting@use_kwargs({

'data': fields.Nested({
    'title': fields.Str(),
    'description': fields.Str(),
})

})def myview(data):
return 'title: {}, description: {}'.format(**data)

respects load_from@use_kwargs({

'x_custom_header': fields.Str(load_from='X-Custom-Header')

})def myview(x_custom_header):
return 'Header: {}'.format(x_custom_header)

Passing in a Schemafrom marshmallow import Schema

class PostSchema(Schema):
title = fields.Str(required=True, location='json')
description = fields.Str(required=False)
id = fields.Int(dump_only=True) # won't be included in parsed output
@use_kwargs(PostSchema)def myview(title, description):
return 'title: {}, description: {}'.format(title, description)


Reply to this email directly or view it on GitHub
#61.

@Trii

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2015

+1

@sloria sloria referenced this issue Sep 21, 2015
12 of 12 tasks complete
@sloria

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2015

Making lots of progress on this.

I haven't yet decided where fields should be imported from.

Here are the alternatives (note that Nested is a webargs-specific field):

(1) Import from webargs.fields

from webargs import fields
from webargs.flaskparser import use_args
from marshmallow import validate

@use_args({
    'data': fields.Nested({
        'id': fields.Int(),
        'email': fields.Str(validate=validate.Email()),
    })
})

(2) Import from webargs

import webargs as wa
from webargs.flaskparser import use_args
from marshmallow import validate

@use_args({
    'data': wa.Nested({
        'id': wa.Int(),
        'email': wa.Str(validate=validate.Email()),
    })
})

(3) Import from marshmallow

from webargs import Nested  # or from webargs.fields import Nested
from webargs.flaskparser import use_args
from marshmallow import fields, validate

@use_args({
    'data': Nested({
        'id': fields.Int(),
        'email': fields.Str(validate=validate.Email()),
    })
})

Any preferences?

@marcellarius

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

  1. or 3). I think option 3) makes it clear that it's using marshmallow.

On Tue, Sep 22, 2015 at 2:51 PM, Steven Loria notifications@github.com
wrote:

Making lots of progress on this.

I haven't yet decided where fields should be imported from.

Here are the alternatives (note that Nested is a webargs-specific field):

(1) Import from webargs.fields

from webargs import fieldsfrom webargs.flaskparser import use_argsfrom marshmallow import validate
@use_args({
'data': fields.Nested({
'id': fields.Int(),
'email': fields.Str(validate=validate.Email()),
})
})

(2) Import from webargs

import webargs as wafrom webargs.flaskparser import use_argsfrom marshmallow import validate
@use_args({
'data': wa.Nested({
'id': wa.Int(),
'email': wa.Str(validate=validate.Email()),
})
})

(3) Import from marshmallow

from webargs import Nestedfrom webargs.flaskparser import use_argsfrom marshmallow import fields, validate
@use_args({
'data': Nested({
'id': fields.Int(),
'email': fields.Str(validate=validate.Email()),
})
})

Any preferences?


Reply to this email directly or view it on GitHub
#61 (comment).

@sloria

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2015

Fair point. On the other hand, perhaps we don't want to "leak" the marshmallow abstraction? As a user, I don't really like being required to import a 3rd-party library's (marshmallow) modules in the common case, but that's personal preference.

@sloria

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2015

From discussion with @jmcarp : "I think i prefer 1 or 2 just so it's harder to use the wrong Nested".

@Trii

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

I would argue that you shouldn't import from Marshmallow at all. If you want to use Marshmallow under the good, go all the way.

from webargs import fields, validate

@sloria

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2015

To play devil's advocate: there's a risk of hiding too much. If users import Schema from webargs, they might think that webargs modifies behavior of vanilla marshmallow Schemas in some way.

That said, I think importing fields and validate from webargs is fine.

@sloria

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2015

There's also an argument for future-proofing: we can always add imports to webargs without breaking backwards-compat; we cannot remove them without breaking compat.

sloria added a commit that referenced this issue Sep 25, 2015

sloria added a commit that referenced this issue Sep 25, 2015

@sloria

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2015

The marshmallow branch is merged. We'll have a release out later today.

@sloria sloria closed this Sep 27, 2015

@Trii

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2015

👍

sloria added a commit to CenterForOpenScience/django-elasticsearch-metrics that referenced this issue Aug 20, 2018

Make fields importable from top-level module and add Date
This adds a custom `Date` field which respects
`settings.TIMEZONE`.

This also exposes elasticsearch_dsl's fields from the top-level
module, so that all fields are imported from the same place:

```python
from elasticsearch_metrics import Keyword, Date
```

This approach was the same used in webargs, which uses
marshmallow fields under the hood but overrides a single
field. Discussion: marshmallow-code/webargs#61 (comment)

close #18

sloria added a commit to CenterForOpenScience/django-elasticsearch-metrics that referenced this issue Aug 20, 2018

Make fields importable from top-level module and add Date
This adds a custom `Date` field which respects
`settings.TIMEZONE`.

This also exposes elasticsearch_dsl's fields from the top-level
module, so that all fields are imported from the same place:

```python
from elasticsearch_metrics import Keyword, Date
```

This approach was the same used in webargs, which uses
marshmallow fields under the hood but overrides a single
field. Discussion: marshmallow-code/webargs#61 (comment)

close #18

sloria added a commit to CenterForOpenScience/django-elasticsearch-metrics that referenced this issue Aug 20, 2018

Make fields importable from top-level module and add Date
This adds a custom `Date` field which respects
`settings.TIMEZONE`.

This also exposes elasticsearch_dsl's fields from the top-level
module, so that all fields are imported from the same place:

```python
from elasticsearch_metrics import Keyword, Date
```

This approach was the same used in webargs, which uses
marshmallow fields under the hood but overrides a single
field. Discussion: marshmallow-code/webargs#61 (comment)

close #18

sloria added a commit to CenterForOpenScience/django-elasticsearch-metrics that referenced this issue Aug 20, 2018

Make fields importable from metrics module and add Date
This adds a custom `Date` field which respects
`settings.TIMEZONE`.

This also exposes elasticsearch_dsl's fields from the metrics
module, so that all fields are imported from the same place:

```python
from elasticsearch_metrics import Keyword, Date
```

This approach was the same used in webargs, which uses
marshmallow fields under the hood but overrides a single
field. Discussion: marshmallow-code/webargs#61 (comment)

close #18

sloria added a commit to CenterForOpenScience/django-elasticsearch-metrics that referenced this issue Aug 21, 2018

Make fields importable from metrics module and add Date
This adds a custom `Date` field which respects
`settings.TIMEZONE`.

This also exposes elasticsearch_dsl's fields from the metrics
module, so that all fields are imported from the same place:

```python
from elasticsearch_metrics import Keyword, Date
```

This approach was the same used in webargs, which uses
marshmallow fields under the hood but overrides a single
field. Discussion: marshmallow-code/webargs#61 (comment)

close #18

sloria added a commit to CenterForOpenScience/django-elasticsearch-metrics that referenced this issue Aug 21, 2018

Make fields importable from metrics module and add Date
This adds a custom `Date` field which respects
`settings.TIMEZONE`.

This also exposes elasticsearch_dsl's fields from the metrics
module, so that all fields are imported from the same place:

```python
from elasticsearch_metrics import metrics

class MyMetric(metrics.Metric):
  my_keyword = metrics.Keyword()
  my_date = metrics.Date()
```

This approach was the same used in webargs, which uses
marshmallow fields under the hood but overrides a single
field. Discussion: marshmallow-code/webargs#61 (comment)

close #18

sloria added a commit to CenterForOpenScience/django-elasticsearch-metrics that referenced this issue Aug 21, 2018

Make fields importable from metrics module and add Date
This adds a custom `Date` field which respects
`settings.TIMEZONE`.

This also exposes elasticsearch_dsl's fields from the metrics
module, so that all fields are imported from the same place:

```python
from elasticsearch_metrics import metrics

class MyMetric(metrics.Metric):
  my_keyword = metrics.Keyword()
  my_date = metrics.Date()
```

This approach was the same used in webargs, which uses
marshmallow fields under the hood but overrides a single
field. Discussion: marshmallow-code/webargs#61 (comment)

close #18

sloria added a commit to CenterForOpenScience/django-elasticsearch-metrics that referenced this issue Aug 21, 2018

Make fields importable from metrics module and add Date
This adds a custom `Date` field which respects
`settings.TIMEZONE`.

This also exposes elasticsearch_dsl's fields from the metrics
module, so that all fields are imported from the same place:

```python
from elasticsearch_metrics import metrics

class MyMetric(metrics.Metric):
  my_keyword = metrics.Keyword()
  my_date = metrics.Date()
```

This approach was the same used in webargs, which uses
marshmallow fields under the hood but overrides a single
field. Discussion: marshmallow-code/webargs#61 (comment)

close #18

sloria added a commit to CenterForOpenScience/django-elasticsearch-metrics that referenced this issue Aug 21, 2018

Make fields importable from metrics module and add Date
This adds a custom `Date` field which respects
`settings.TIMEZONE`.

This also exposes elasticsearch_dsl's fields from the metrics
module, so that all fields are imported from the same place:

```python
from elasticsearch_metrics import metrics

class MyMetric(metrics.Metric):
  my_keyword = metrics.Keyword()
  my_date = metrics.Date()
```

This approach was the same used in webargs, which uses
marshmallow fields under the hood but overrides a single
field. Discussion: marshmallow-code/webargs#61 (comment)

close #18
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.