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

Throwing error on invalid JSON string… #329

Closed
zedrdave opened this issue Nov 24, 2018 · 3 comments

Comments

@zedrdave
Copy link

commented Nov 24, 2018

This seems too big to be an oversight, so apologies in advance if this is a design choice and it's been covered before (I looked in the issues and didn't see anything):

When sending an invalid JSON string, eg (note the stray comma):

curl -X PUT "http://localhost:5000/bar/0001" -H "accept: application/json" -H "Content-Type: application/json" -d "{ \"foo\": \"bar\" , }"

… it seems that parsers are expected to simply return an empty dict {}, while silencing any error. Eg flask_parser:

        # Fail silently so that the webargs parser can handle the error
        if hasattr(req, "get_json"):
            # Flask >= 0.10.x
            json_data = req.get_json(force=force, silent=True)
        else:
            # Flask <= 0.9.x
            json_data = req.json
        if json_data is None:
            return core.missing

Despite what the comment implies, I don't think there is any way later on to distinguish between a real missing value, and one resulting from an invalid JSON string…

As expected, assuming an empty dict is compatible with Schema validation options, no error is thrown (and adding an error handler does not seem like it would help).

Am I missing something? Shouldn't JSON parsing errors be caught and trigger a 400 or 422 automatically?

@sloria

This comment has been minimized.

Copy link
Member

commented Dec 16, 2018

Thanks for the report, @zedrdave . I think you're right about not handling JSON correctly. I would also expect an error response to be returned.

I won't have time to look at this in the immediate future. Would you like to send a PR for this? Even a partial PR, e.g. a failing test, would be a big help.

@sloria

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

After further investigation, all the parsers behave the same way (ignoring invalid JSON and treating it as missing).

I added this to webargs.testing.CommonTestCase:

    def test_invalid_json(self, testapp):
        res = testapp.post(
            "/echo",
            '{"foo": "bar", }',
            headers={"Accept": "application/json", "Content-Type": "application/json"},
            expect_errors=True,
        )
        assert res.status_code == 400

and it failed for every parser. So it appears this behavior is expected (though perhaps not desired).

I'm still up for changing this, but the behavior should be changed across all parsers.

@sloria sloria added backwards incompatible and removed bug labels Dec 23, 2018

@sloria sloria self-assigned this Dec 24, 2018

@sloria sloria removed the help wanted label Dec 24, 2018

sloria added a commit that referenced this issue 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
@sloria

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

This issue is resolved by #334, which allows JSONDecodeError to be raised returns a 400 error response when the client passes an invalid JSON payload.

Your code will still need to handle the error appropriately.

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

sloria added a commit that referenced this issue Dec 27, 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

sloria added a commit that referenced this issue Dec 28, 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

sloria added a commit that referenced this issue Dec 30, 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

sloria added a commit that referenced this issue Dec 30, 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

@sloria sloria closed this in #334 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.