Marshmallow: RFC: Remove `fields` attribute from ValidationError

[sloria]

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]

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.

[decaz]

@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]

@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]

@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]

@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]

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

[sloria]

@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]

@decaz The above change is released in webargs 4.0.0.

[decaz]

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

[sloria]

@decaz It's released now.

[lafrech]

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]

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