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

RFC: Remove `fields` attribute from ValidationError #840

Closed
sloria opened this Issue Jun 13, 2018 · 12 comments

Comments

Projects
None yet
3 participants
@sloria
Member

sloria commented Jun 13, 2018

Currently, ValidationError stores the field name and field instances associated with the error.

from marshmallow import Schema, fields

class MySchema(Schema):
    foo = fields.Int()

schema = MySchema()
try:
    schema.load({'foo': 'invalid'})
except Exception as error:
    print(error.fields)  # [<fields.Integer(...)>]

I propose removing the fields because it unnecessarily complicates the validation logic.

fields and field_names were added at a time (version 1.1.0 😓 ) when it was impossible to access all the error messages for a schema (see #63). It wasn't until 2.0 that it became possible to get the full error messages dict when using strict mode.

At this point, I don't see a use case for accessing ValidationError.fields.

@sloria sloria added this to the 3.0 milestone Jun 13, 2018

@sloria sloria changed the title from RFC: Remove `field_name` and `fields` attributes from ValidationError to RFC: Remove `fields` attribute from ValidationError Jun 16, 2018

@sloria

This comment has been minimized.

Member

sloria commented Jun 16, 2018

Actually, we should keep field_names, as it allows users to specify which fields to store ValidationErrors on: https://marshmallow.readthedocs.io/en/latest/extending.html#storing-errors-on-specific-fields .

I've modified proposal to only remove ValidationError.fields.

sloria added a commit that referenced this issue Jun 16, 2018

sloria added a commit that referenced this issue Jun 16, 2018

sloria added a commit that referenced this issue Jun 23, 2018

@sloria sloria closed this in #845 Jun 23, 2018

@decaz

This comment has been minimized.

decaz commented Jul 9, 2018

@sloria I use ValidationError.fields because it gives me access to field instances of schema the error was raised at. For instance:

from flask import Flask, Response
from flask_apispec import use_kwargs
from marshmallow import fields


app = Flask(__name__)


@app.errorhandler(422)
def handle_422(error):
    errors = []
    for field in error.exc.fields:
        error_msg = f'Invalid field {field.name}'
        location = field.metadata.get('location')
        if location:
            error_msg += f' at {location}'
        errors.append(error_msg)
    return Response('\n'.join(errors))


@app.route('/')
@use_kwargs({'x': fields.Int(location='query')})
def index(x):
    return x

Here we have global Flask error handler for 422 error and abstract kwargs schema. There can be a lot of schemes across project and the handler should be universal and return information from invalid fields metadata. So I can't instantiate these schemes, pass them to the handler and then get metadata from propagated instance.

If you don't want to keep field instances, maybe it worth to keep schema the error relates to (ValidationError.schema) ?

@sloria

This comment has been minimized.

Member

sloria commented Jul 10, 2018

@decaz I think your use case can (should?) be solved by a change in webargs. We could attach the schema to the HTTPException.

This could look something like:

@app.errorhandler(422)
def handle_422(error):
    errors = []
    schema = error.data['schema']
    for field_name in error.exc.field_names:
        field = schema.fields[field_name]
        error_msg = f'Invalid field {field.name}'
        location = field.metadata.get('location')
        if location:
            error_msg += f' at {location}'
        errors.append(error_msg)
    return Response('\n'.join(errors))
@decaz

This comment has been minimized.

decaz commented Jul 10, 2018

@sloria I guess ValidationError.data doesn't fit for storing schema as it contains fields data and there can be a field called "schema".

Unfortunately ValidationError.schema also may not be good idea as kwargs in my example is dict but not schema instance, so it won't be possible to access field instances by ValidationError.schema.fields - the user will have to check type of the schema explicitly =/

@sloria

This comment has been minimized.

Member

sloria commented Jul 10, 2018

@decaz In the code example, error is the HTTPException raised by Flask's abort function--not the ValidationError raised. webargs stores additional data in the data attribute. Validation messages are stored in error.data['messages']. So I still think error.data['schema'] is a fine place to store the schema instance.

Note: There's not yet a way to achieve the above without a change to webargs. I was just posting it for feedback.

@decaz

This comment has been minimized.

decaz commented Jul 10, 2018

@sloria ok, I understand and agree - error.data['schema'] would be a good solution. What is the next step?

@sloria

This comment has been minimized.

Member

sloria commented Jul 10, 2018

@decaz If you'd like, feel free to send a PR to webargs implementing this.

If you don't have time now, I'll try to find some time over the weekend.

sloria added a commit to sloria/webargs that referenced this issue Jul 15, 2018

Pass schema instance to error handlers
* @error_handler and handle_error receive schema instance
* In Flask, the Schema instance is attached to HTTPExceptions and
  is accessible from error.data["schema"]

See marshmallow-code/marshmallow#840 (comment)
@sloria

This comment has been minimized.

Member

sloria commented Jul 15, 2018

@decaz The above change is released in webargs 4.0.0.

@decaz

This comment has been minimized.

decaz commented Jul 16, 2018

@sloria great, thanks! When will it be released at PyPI?

@sloria

This comment has been minimized.

Member

sloria commented Jul 16, 2018

@decaz It's released now.

lipis added a commit to gae-init/gae-init that referenced this issue Sep 15, 2018

Bump webargs from 3.0.2 to 4.0.0 (#911)
Bumps [webargs](https://github.com/sloria/webargs) from 3.0.2 to 4.0.0.
<details>
<summary>Changelog</summary>

*Sourced from [webargs's changelog](https://github.com/sloria/webargs/blob/dev/CHANGELOG.rst).*

> 4.0.0 (2018-07-15)
> ******************
> 
> Features:
> 
> * *Backwards-incompatible*: Custom error handlers receive the
>   `marshmallow.Schema` instance as the third argument. Update any
>   functions decorated with `Parser.error_handler` to take a ``schema``
>   argument, like so:
> 
> .. code-block:: python
> 
>     # 3.x
>     [**parser**](https://github.com/parser).error_handler
>     def handle_error(error, req):
>         raise CustomError(error.messages)
> 
> 
>     # 4.x
>     [**parser**](https://github.com/parser).error_handler
>     def handle_error(error, req, schema):
>         raise CustomError(error.messages)
> 
> 
> See `marshmallow-code/marshmallow#840 (comment) <https://github-redirect.dependabot.com/marshmallow-code/marshmallow/issues/840#issuecomment-403481686>`_
> for more information about this change.
> 
> Bug fixes:
> 
> * *Backwards-incompatible*: Rename ``webargs.async`` to
>   ``webargs.asyncparser`` to fix compatibility with Python 3.7
>   (:issue:`240`). Thanks :user:`Reskov` for the catch and patch.
> 
> 
> Other changes:
> 
> * *Backwards-incompatible*: Drop support for Python 3.4 (:pr:`243`). Python 2.7 and
>   >=3.5 are supported.
> * *Backwards-incompatible*: Drop support for marshmallow<2.15.0.
>   marshmallow>=2.15.0 and >=3.0.0b12 are officially supported.
> * Use `black <https://github.com/ambv/black>`_ with `pre-commit <https://pre-commit.com/>`_
>   for code formatting (:pr:`244`).
</details>
<details>
<summary>Commits</summary>

- [`a864099`](sloria/webargs@a864099) Bump version and update changelog
- [`90b822f`](sloria/webargs@90b822f) Merge pull request [#250](https://github-redirect.dependabot.com/sloria/webargs/issues/250) from sloria/flask-attach-schema-to-error
- [`3ead80a`](sloria/webargs@3ead80a) Pass schema instance to error handlers
- [`ee5ee8c`](sloria/webargs@ee5ee8c) Use :doc: instead of :ref: to refer to pages...
- [`a721931`](sloria/webargs@a721931) Merge pull request [#249](https://github-redirect.dependabot.com/sloria/webargs/issues/249) from sloria/pyup-update-sphinx-issues-0.4.0-to-1.0.0
- [`c432c2c`](sloria/webargs@c432c2c) Update sphinx-issues from 0.4.0 to 1.0.0
- [`dcd67a9`](sloria/webargs@dcd67a9) Merge pull request [#248](https://github-redirect.dependabot.com/sloria/webargs/issues/248) from sloria/pyup-update-tox-3.1.1-to-3.1.2
- [`802c434`](sloria/webargs@802c434) Merge pull request [#247](https://github-redirect.dependabot.com/sloria/webargs/issues/247) from sloria/pyup-update-invoke-1.0.0-to-1.1.0
- [`6c946cc`](sloria/webargs@6c946cc) Update tox from 3.1.1 to 3.1.2
- [`1c54e1b`](sloria/webargs@1c54e1b) Update invoke from 1.0.0 to 1.1.0
- Additional commits viewable in [compare view](sloria/webargs@3.0.2...4.0.0)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=webargs&package-manager=pip&previous-version=3.0.2&new-version=4.0.0)](https://dependabot.com/compatibility-score.html?dependency-name=webargs&package-manager=pip&previous-version=3.0.2&new-version=4.0.0)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>

lipis added a commit to gae-init/gae-init-magic that referenced this issue Oct 26, 2018

Bump webargs from 1.9.0 to 4.1.1 (#86)
Bumps [webargs](https://github.com/sloria/webargs) from 1.9.0 to 4.1.1.
<details>
<summary>Changelog</summary>

*Sourced from [webargs's changelog](https://github.com/sloria/webargs/blob/dev/CHANGELOG.rst).*

> 4.1.1 (2018-10-25)
> ******************
> 
> Bug fixes:
> 
> * Fix bug in ``AIOHTTPParser`` that caused a ``JSONDecode`` error
>   when parsing empty payloads (:issue:`229`). Thanks :user:`explosic4`
>   for reporting and thanks user :user:`kochab` for the PR.
> 
> 4.1.0 (2018-09-17)
> ******************
> 
> Features:
> 
> * Add ``webargs.testing`` module, which exposes ``CommonTestCase``
>   to third-party parser libraries (see comments in :pr:`287`).
> 
> 4.0.0 (2018-07-15)
> ******************
> 
> Features:
> 
> * *Backwards-incompatible*: Custom error handlers receive the
>   `marshmallow.Schema` instance as the third argument. Update any
>   functions decorated with `Parser.error_handler` to take a ``schema``
>   argument, like so:
> 
> .. code-block:: python
> 
>     # 3.x
>     [**parser**](https://github.com/parser).error_handler
>     def handle_error(error, req):
>         raise CustomError(error.messages)
> 
> 
>     # 4.x
>     [**parser**](https://github.com/parser).error_handler
>     def handle_error(error, req, schema):
>         raise CustomError(error.messages)
> 
> 
> See `marshmallow-code/marshmallow#840 (comment) <https://github-redirect.dependabot.com/marshmallow-code/marshmallow/issues/840#issuecomment-403481686>`_
> for more information about this change.
> 
> Bug fixes:
> 
> * *Backwards-incompatible*: Rename ``webargs.async`` to
>   ``webargs.asyncparser`` to fix compatibility with Python 3.7
>   (:issue:`240`). Thanks :user:`Reskov` for the catch and patch.
> 
></table> ... (truncated)
</details>
<details>
<summary>Commits</summary>

- [`969f2fc`](sloria/webargs@969f2fc) Bump version and update changelog
- [`db3c459`](sloria/webargs@db3c459) Update pre-commit hooks
- [`80b6bc4`](sloria/webargs@80b6bc4) Update changelog and add kochab to authors
- [`1019518`](sloria/webargs@1019518) Merge pull request [#297](https://github-redirect.dependabot.com/sloria/webargs/issues/297) from kochab/fix-229
- [`2c265a8`](sloria/webargs@2c265a8) Merge pull request [#310](https://github-redirect.dependabot.com/sloria/webargs/issues/310) from sloria/dependabot/pip/pre-commit-1.12.0
- [`2cb4ddd`](sloria/webargs@2cb4ddd) Bump pre-commit from 1.11.2 to 1.12.0
- [`7454c35`](sloria/webargs@7454c35) test_aiohttpparser: Add a test to ensure correct behaviour on empty JSON bodies
- [`8327796`](sloria/webargs@8327796) Bump pytest from 3.9.1 to 3.9.2
- [`5d8fbe7`](sloria/webargs@5d8fbe7) Merge pull request [#306](https://github-redirect.dependabot.com/sloria/webargs/issues/306) from sloria/dependabot/pip/pytest-3.9.1
- [`b0e55a4`](sloria/webargs@b0e55a4) Bump pytest from 3.8.2 to 3.9.1
- Additional commits viewable in [compare view](sloria/webargs@1.9.0...4.1.1)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=webargs&package-manager=pip&previous-version=1.9.0&new-version=4.1.1)](https://dependabot.com/compatibility-score.html?dependency-name=webargs&package-manager=pip&previous-version=1.9.0&new-version=4.1.1)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>

rbubley pushed a commit to rbubley/gae-init that referenced this issue Oct 30, 2018

Bump webargs from 3.0.2 to 4.0.0 (gae-init#911)
Bumps [webargs](https://github.com/sloria/webargs) from 3.0.2 to 4.0.0.
<details>
<summary>Changelog</summary>

*Sourced from [webargs's changelog](https://github.com/sloria/webargs/blob/dev/CHANGELOG.rst).*

> 4.0.0 (2018-07-15)
> ******************
> 
> Features:
> 
> * *Backwards-incompatible*: Custom error handlers receive the
>   `marshmallow.Schema` instance as the third argument. Update any
>   functions decorated with `Parser.error_handler` to take a ``schema``
>   argument, like so:
> 
> .. code-block:: python
> 
>     # 3.x
>     [**parser**](https://github.com/parser).error_handler
>     def handle_error(error, req):
>         raise CustomError(error.messages)
> 
> 
>     # 4.x
>     [**parser**](https://github.com/parser).error_handler
>     def handle_error(error, req, schema):
>         raise CustomError(error.messages)
> 
> 
> See `marshmallow-code/marshmallow#840 (comment) <https://github-redirect.dependabot.com/marshmallow-code/marshmallow/issues/840#issuecomment-403481686>`_
> for more information about this change.
> 
> Bug fixes:
> 
> * *Backwards-incompatible*: Rename ``webargs.async`` to
>   ``webargs.asyncparser`` to fix compatibility with Python 3.7
>   (:issue:`240`). Thanks :user:`Reskov` for the catch and patch.
> 
> 
> Other changes:
> 
> * *Backwards-incompatible*: Drop support for Python 3.4 (:pr:`243`). Python 2.7 and
>   >=3.5 are supported.
> * *Backwards-incompatible*: Drop support for marshmallow<2.15.0.
>   marshmallow>=2.15.0 and >=3.0.0b12 are officially supported.
> * Use `black <https://github.com/ambv/black>`_ with `pre-commit <https://pre-commit.com/>`_
>   for code formatting (:pr:`244`).
</details>
<details>
<summary>Commits</summary>

- [`a864099`](sloria/webargs@a864099) Bump version and update changelog
- [`90b822f`](sloria/webargs@90b822f) Merge pull request [gae-init#250](https://github-redirect.dependabot.com/sloria/webargs/issues/250) from sloria/flask-attach-schema-to-error
- [`3ead80a`](sloria/webargs@3ead80a) Pass schema instance to error handlers
- [`ee5ee8c`](sloria/webargs@ee5ee8c) Use :doc: instead of :ref: to refer to pages...
- [`a721931`](sloria/webargs@a721931) Merge pull request [gae-init#249](https://github-redirect.dependabot.com/sloria/webargs/issues/249) from sloria/pyup-update-sphinx-issues-0.4.0-to-1.0.0
- [`c432c2c`](sloria/webargs@c432c2c) Update sphinx-issues from 0.4.0 to 1.0.0
- [`dcd67a9`](sloria/webargs@dcd67a9) Merge pull request [gae-init#248](https://github-redirect.dependabot.com/sloria/webargs/issues/248) from sloria/pyup-update-tox-3.1.1-to-3.1.2
- [`802c434`](sloria/webargs@802c434) Merge pull request [gae-init#247](https://github-redirect.dependabot.com/sloria/webargs/issues/247) from sloria/pyup-update-invoke-1.0.0-to-1.1.0
- [`6c946cc`](sloria/webargs@6c946cc) Update tox from 3.1.1 to 3.1.2
- [`1c54e1b`](sloria/webargs@1c54e1b) Update invoke from 1.0.0 to 1.1.0
- Additional commits viewable in [compare view](sloria/webargs@3.0.2...4.0.0)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=webargs&package-manager=pip&previous-version=3.0.2&new-version=4.0.0)](https://dependabot.com/compatibility-score.html?dependency-name=webargs&package-manager=pip&previous-version=3.0.2&new-version=4.0.0)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
@lafrech

This comment has been minimized.

Member

lafrech commented Nov 30, 2018

I'm afraid @decaz's use case is broken again.

The latest error management rework removed field_names from ValidationError. My intention was to remove the possibility to raise the same error for multiple fields at once because it complicates the logic and the new message deep merge feature makes it possible in a more powerful way.

I just realized from broken webargs tests that it also removes the possibility to store the names of faulty fields. The faulty inputs are still available in the messages, but the keys are not the field names if data_key is used.

Note that the example provided above was a bit wrong IMHO because it returned field names from object space, not client space (it ignored data_key).

From the schema and the error messages containing client space names, it should be possible to find the fields, but this requires a bit of trickery. For each key, you'll need to find the field that has the same name or data_key attribute and is not dump_only (there may be several fields with the same data_key but only one is not dump_only).

@sloria

This comment has been minimized.

Member

sloria commented Nov 30, 2018

Good catch, @lafrech . I'll get your webargs patch merged and released.

Once that's in, I think @decaz 's use case can be met with something like

from flask import Flask, jsonify
from webargs.flaskparser import use_kwargs
from marshmallow import fields


app = Flask(__name__)


@app.errorhandler(422)
def handle_422(error):
    errors = []
    schema = error.data['schema']
    client_keys_to_fields = {
        field.data_key or field_name: field
        for field_name, field in schema.fields.items()
        if not field.dump_only
    }
    for field_name in error.exc.messages:
        field = client_keys_to_fields[field_name]
        error_msg = f'Invalid field {field.name}'
        location = field.metadata.get('location')
        if location:
            error_msg += f' at {location}'
        errors.append(error_msg)
    return jsonify(errors)


@app.route('/')
@use_kwargs({'x': fields.Int(location='query')})
def index(x):
    return jsonify(x)


if __name__ == "__main__":
    app.debug = True
    app.run()

Edit: clean up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment