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

RFC: Don't pass a status_code as kwargs in a Field validator #327

Closed
lafrech opened this issue Nov 22, 2018 · 9 comments

Comments

@lafrech
Copy link
Member

commented Nov 22, 2018

webargs's ValidationError adds status_code and headers to marshmallow's ValidationError. This allows to specify a default code for input validation error. The webserver uses this attribute to return the proper error code.

The tests show another use of this that I find a bit problematic.

It is possible to define a validator on a Field of the input validation Schema that raises a ValidationError with a status_code. The deserialization logic in marshmallow adds this attribute to the ValidationError that is raised at the end of the validation process.

The issue with this is the case of several fields raising with a different status code. The last one to deserialize has priority. This is probably a corner case, but it shows the feature is not 100% reliable. It looks underterministic.

In practice I don't see the use case for this in the first place. I generally use the default 422 for all validation errors and return specific codes from flask abort in view function or in specific decorator (authentication decorator,...).

Related marshmallow issue: marshmallow-code/marshmallow#996.

My suggestion is to remove the logic that passes the kwargs in marshmallow and delete the test case here that exhibit the case of specifying a status code in a field validator.

@sloria

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

I think you're right about this, @lafrech . In retrospect, the status_code kwarg was the wrong solution to #85 . The use case is met by using flask.abort, as you said.

Modified from the OP in #85:

from flask import Flask, abort, Response
from webargs import fields
from webargs.flaskparser import use_args

app = Flask(__name__)


def dummy_validator(val):
    abort(Response("Error!", status=500))


@app.route("/")
@use_args({"test_field": fields.String(validate=dummy_validator)})
def hello_world(args):
    return "Hello World!"

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

So let's move forward with this. It'd be good to have a minor release deprecating status_code and DEFAULT_VALIDATION_STATUS before we remove it in 5.0.0. I might have some time to work on this over the next few days.

@sloria

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

One use case that would be affected by this change would be reusing validators.

For example, I may want to use the same validator for multiple fields and have the error response include the formatted error messages.

Right now, you can do

from flask import Flask, jsonify
from webargs import fields, ValidationError
from webargs.flaskparser import use_args

app = Flask(__name__)


def auth_validator(val):
    raise ValidationError(message="Auth failed", status_code=401)


@app.route("/auth")
@use_args(
    {
        "password": fields.Str(validate=auth_validator),
        "token": fields.Str(validate=auth_validator),
    }
)
def auth(args):
    return "access granted"


@app.errorhandler(422)
@app.errorhandler(401)
def handle_validation_error(err):
    exc = err.exc
    return jsonify({"errors": exc.messages}), exc.kwargs["status_code"]


if __name__ == "__main__":
    app.run("0.0.0.0", debug=True)
webargs ❯ http ':5000/auth?token=foo'
HTTP/1.0 401 UNAUTHORIZED
Content-Length: 63
Content-Type: application/json
Date: Sun, 23 Dec 2018 18:51:13 GMT
Server: Werkzeug/0.14.1 Python/3.6.5

{
    "errors": {
        "token": [
            "Auth failed"
        ]
    }
}

Do we want to continue supporting this use case?

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Dec 23, 2018

You're passing 401 in handle_validation_error, so you don't need it in the ValidationError. Did you mean

    return jsonify({"errors": exc.messages}), exc.kwargs['status_code']

?

I don't think I'd have a single function for different auth mechanisms, and I don't think I'd use a Schema validator to do this king of validation.

Besides, 422 could be something else. A validation error not triggered by the auth_validator function. Unlikely, because it's hard to fail validation on a string field, but theoretically.

I remember we already discussed that once in marshmallow when dealing with field uniqueness validation, and there seemed to be consensus that validation needing I/O, like DB access, was better done in the view function. Not Truth, just a general rule. And actually, the best argument IMHO about uniqueness was to avoid race conditions, and it doesn't apply here.

Maybe the example is too simplistic or too far-fetched, but for this use case, I'd let the auth functions do the abort. Or more likely, I'd have a part of the app - it could be an extension, maybe a separate lib - that would raise its own exception, and I'd catch it in the view to call abort. Either in the view itself in a single /auth resource, or in a view function decorator when checking credentials in every view.

All in all, the case above seems more complicated to me in real life than what I'm describing.

@sloria

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

Did you mean...

Good catch. Fixed.

I'd let the auth functions do the abort...

Fair enough. But then you'll need to duplicate the error formatting logic in two places if you want to keep the error objects consistent.

Perhaps we could support passing an error status code to parse &c.

@use_args(user_args)  # default 422
@use_args(auth_args, error_status_code=401)
def myview(args):
    return "foo"

Thoughts?

@sloria

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

We should give the same treatment to headers.

sloria added a commit that referenced this issue Dec 24, 2018

Deprecate status and headers params of ValidationErrors
* `status` and `headers` kwargs are deprecated (close #336)
* Add `error_status_code` and `error_headers`:
  See #327 (comment)

```python
def validate_foo(val):  # Before
    ...
    raise ValidationError("Error!", status_code=400)

@use_args({"foo": fields.Str(validate=validate_foo)})
def my_view(args):
    ...

def validate_foo(val):  # After
    ...
    raise ValidationError("Error!")

@use_args(
    {"foo": fields.Str(validate=validate_foo)},
    error_status_code=400
)
def my_view(args):
    ...
```

* error_handler functions and handle_error methods now take
  error_status_code and error_headers arguments. Backwards
  compatibility was maintained by checking the the number of
  arguments in the signatures.

```python
@parser.error_handler
def handle_error(error, req):  # Before
    raise CustomError(error.messages)

@parser.error_handler
def handle_error(error, req, status_code, headers):  # After
    raise CustomError(error.messages)

@parser.error_handler
def handle_error(error, **kwargs):  # Also works
    raise CustomError(error.messages)
```
@sloria

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

I've implemented my proposal above and deprecated status_code and headers in #338.

sloria added a commit that referenced this issue Dec 25, 2018

Deprecate status and headers params of ValidationErrors
* `status` and `headers` kwargs are deprecated (close #336)
* Add `error_status_code` and `error_headers`:
  See #327 (comment)

```python
def validate_foo(val):  # Before
    ...
    raise ValidationError("Error!", status_code=400)

@use_args({"foo": fields.Str(validate=validate_foo)})
def my_view(args):
    ...

def validate_foo(val):  # After
    ...
    raise ValidationError("Error!")

@use_args(
    {"foo": fields.Str(validate=validate_foo)},
    error_status_code=400
)
def my_view(args):
    ...
```

* error_handler functions and handle_error methods now take
  error_status_code and error_headers arguments. Backwards
  compatibility was maintained by checking the the number of
  arguments in the signatures.

```python
@parser.error_handler
def handle_error(error, req):  # Before
    raise CustomError(error.messages)

@parser.error_handler
def handle_error(error, req, status_code, headers):  # After
    raise CustomError(error.messages)

@parser.error_handler
def handle_error(error, **kwargs):  # Also works
    raise CustomError(error.messages)
```

sloria added a commit that referenced this issue Dec 25, 2018

Deprecate status and headers params of ValidationErrors
* `status` and `headers` kwargs are deprecated (close #336)
* Add `error_status_code` and `error_headers`:
  See #327 (comment)

```python
def validate_foo(val):  # Before
    ...
    raise ValidationError("Error!", status_code=400)

@use_args({"foo": fields.Str(validate=validate_foo)})
def my_view(args):
    ...

def validate_foo(val):  # After
    ...
    raise ValidationError("Error!")

@use_args(
    {"foo": fields.Str(validate=validate_foo)},
    error_status_code=400
)
def my_view(args):
    ...
```

* error_handler functions and handle_error methods now take
  error_status_code and error_headers arguments. Backwards
  compatibility was maintained by checking the the number of
  arguments in the signatures.

```python
@parser.error_handler
def handle_error(error, req):  # Before
    raise CustomError(error.messages)

@parser.error_handler
def handle_error(error, req, status_code, headers):  # After
    raise CustomError(error.messages)

@parser.error_handler
def handle_error(error, **kwargs):  # Also works
    raise CustomError(error.messages)
```

sloria added a commit that referenced this issue Dec 25, 2018

Deprecate status and headers params of ValidationErrors
* `status` and `headers` kwargs are deprecated (close #336)
* Add `error_status_code` and `error_headers`:
  See #327 (comment)

```python
def validate_foo(val):  # Before
    ...
    raise ValidationError("Error!", status_code=400)

@use_args({"foo": fields.Str(validate=validate_foo)})
def my_view(args):
    ...

def validate_foo(val):  # After
    ...
    raise ValidationError("Error!")

@use_args(
    {"foo": fields.Str(validate=validate_foo)},
    error_status_code=400
)
def my_view(args):
    ...
```

* error_handler functions and handle_error methods now take
  error_status_code and error_headers arguments. Backwards
  compatibility was maintained by checking the the number of
  arguments in the signatures.

```python
@parser.error_handler
def handle_error(error, req):  # Before
    raise CustomError(error.messages)

@parser.error_handler
def handle_error(error, req, status_code, headers):  # After
    raise CustomError(error.messages)

@parser.error_handler
def handle_error(error, **kwargs):  # Also works
    raise CustomError(error.messages)
```
@lafrech

This comment has been minimized.

Copy link
Member Author

commented Dec 27, 2018

But then you'll need to duplicate the error formatting logic in two places if you want to keep the error objects consistent.

OK, I didn't think about that because I use flask-rest-api, which registers a generic error handler that formats all error messages, so the logic is not duplicated and all I need to do is use abort. I'd recommend doing something like this. At least in Flask, I don't know the other frameworks.

This said, I have no objection to your proposal.

@sloria

This comment has been minimized.

Copy link
Member

commented Dec 27, 2018

@lafrech Even with a generic error handler, abort will cause validation to exit early, so the error handler won't receive the full messages dict.

With my implementation in #338, you can do

@app.route('/auth')
@use_args(non_auth_args, error_status_code=422)
@use_args(auth_args, error_status_code=401)
def myview():
    #...

@app.errorhandler(422)
@app.errorhandler(401)
def handle_validation_error(err):
    exc = getattr(err, "exc", None)
    if exc:
        # Get validation messages from the ValidationError object
        headers = err.data["headers"]
        messages = exc.messages
    else:
        messages = ["Invalid request"]
    return jsonify({"messages": messages}), err.code
        headers = None
        messages = ["Invalid request."]
    if headers:
        return jsonify({"errors": messages}), err.code, headers
    else:
        return jsonify({"errors": messages}), err.code

And get the full errors dict, like so:

webargs ❯ http ':5000/auth?token=foo'
HTTP/1.0 401 UNAUTHORIZED
Content-Length: 63
Content-Type: application/json
Date: Sun, 23 Dec 2018 18:51:13 GMT
Server: Werkzeug/0.14.1 Python/3.6.5

{
    "errors": {
        "token": [
            "Auth failed"
        ]
    }
}

But I digress. If you have no objections, let's go ahead with #338

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Dec 27, 2018

We're using webargs'abort. It accepts kwargs it stores in exception data attribute, so I can pass a message in abort.

That's two very different ways to address the problem, so it is hard to compare.

But I digress. If you have no objections, let's go ahead with #338

Yep.

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.