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

Return error response for invalid JSON payloads #334

Merged
merged 5 commits into from Dec 31, 2018

Conversation

@sloria
Copy link
Member

commented Dec 24, 2018

Previously, invalid JSON payloads were allowed to pass silently.
With this change, a HTTPError with status code 400 and a payload of {"json": ["Invalid JSON body."]} is raised instead.

close #329

Note: Adds simplejson as a dependency on Python 2.

@sloria sloria force-pushed the invalid-json-handling branch from b537eb8 to 1deaf82 Dec 24, 2018

@sloria sloria added this to the 5.0.0 milestone Dec 25, 2018

CHANGELOG.rst Outdated Show resolved Hide resolved

@sloria sloria force-pushed the invalid-json-handling branch 2 times, most recently from 29a2817 to 381ba5b Dec 27, 2018

@lafrech

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

I didn't inspect thoroughly so this is a bit of a lazy question, but wouldn't it be more in line with the validation error triggering a 422 if the parser aborted with 400 rather than raising an exception for the user to catch?

I find it a bit dodgy that we raise an exception that will trigger a 500 out of the box. Even with a major release and a warning.

I can live with that change. I'll just need to add a handler as you suggest, but is there a reason not to return a 400 error directly? The message could be personalized with a parser attribute if that's a concern. And the formatting can be done by catching the HTTPError just like with the 422. At least in the use case I'm familiar with (Flask + flask_rest_api).

@sloria

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2018

You raise a fair point. I think I went the 500 route for consistency with the current behavior of aiohttpparser and for configurability. But in retrospect, it's more user-friendly to raise an HTTPError. I'll try this out when I have some time.

sloria added a commit that referenced this pull request Dec 30, 2018

@sloria sloria force-pushed the invalid-json-handling branch from 8a961fd to b80beed Dec 30, 2018

sloria added a commit that referenced this pull request Dec 30, 2018

@sloria

This comment has been minimized.

Copy link
Member Author

commented Dec 30, 2018

Rebased and modified so that a 400 response is returned with the payload:

{"json": ["Invalid JSON body."]}

A handle_invalid_json_error hook was added to the relevant parsers to make this behavior configurable.

sloria added some commits Dec 24, 2018

Allow JSONDecodeError to be raised when invalid JSON is passed
Previously, invalid JSON payloads were allowed to pass silently.
With this change, the JSONDecodeError is raised.
Therefore, users are responsible for handling these errors.

Example with flask:

```python
@app.errorhandler(json.JSONDecodeError)
def handle_invalid_json(err):
    return jsonify(["Invalid JSON."]), 400
```

close #329
Depend on simplejson and use it consistently
Ensures consistency of behavior across Python 2 and 3
Only require simplejson for Python 2.7
Import json with

```
from webargs.core import json
```

to ensure that we use the same implementation everywhere

@sloria sloria force-pushed the invalid-json-handling branch from b80beed to 1976af6 Dec 30, 2018

@@ -58,6 +58,7 @@ def dateadd(value, addend, unit):


# Return validation errors as JSON
@error(400)
@error(422)
def error422(err):

This comment has been minimized.

Copy link
@lafrech

lafrech Dec 30, 2018

Member

Should this be renamed (remove 422 in the name)?

This comment has been minimized.

Copy link
@sloria

sloria Dec 31, 2018

Author Member

Good catch. Fixed.

setup.py Outdated
import re
from setuptools import setup, find_packages

INSTALL_REQUIRES = ["marshmallow>=2.15.2"]
if sys.version_info[0] < 3:
INSTALL_REQUIRES.append("simplejson")

This comment has been minimized.

Copy link
@lafrech

lafrech Dec 30, 2018

Member

Add a minimal version? "simplejson>=x.x.x"?

@@ -115,7 +115,8 @@ def echo_nested_many():
return parser.parse(args)


@error(422)
@app.error(400)
@app.error(422)
def handle_422(err):

This comment has been minimized.

Copy link
@lafrech

lafrech Dec 30, 2018

Member

Likewise, 422 in the name.

@lafrech

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

Looks good to me! 👍

@lafrech

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

Great. This is good to go.

@sloria sloria changed the title Allow JSONDecodeError to be raised when invalid JSON is passed Return error response for invalid JSON payloads Dec 31, 2018

@sloria sloria merged commit 48a3b7e into dev Dec 31, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@sloria sloria deleted the invalid-json-handling branch Dec 31, 2018

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.