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

have request.get_json() return an error message #2339

Closed
ctiller15 opened this issue Feb 15, 2022 · 9 comments · Fixed by #2355
Closed

have request.get_json() return an error message #2339

ctiller15 opened this issue Feb 15, 2022 · 9 comments · Fixed by #2355
Assignees
Milestone

Comments

@ctiller15
Copy link

ctiller15 commented Feb 15, 2022

Request.get_json(), (or a new function), should return some sort of error message if the force parameter is not used notifying the user that this method only functions if the mime type is application/json unless the force option is used.

  • in endpoint:
parsed_json = request.get_json() # Currently returns None
parsed_json.get("key") # Errors out

Alternatively, current usage can look like this:

parsed_json = request.get_json() # None
key_value = parsed_json.get("key", None)
if key_value:
    # do stuff
else:
    # do other stuff

In most scenarios there would be a failure at some stage or some form of custom handling for the current None behavior. Given that the function expects that application/json is in use it seems reasonable to enforce that expectation at some level.

To be totally fair, this would be a breaking change to anyone relying on the current None behavior so it would also be great if some sort of get_json_strict() existed with similar functionality. The end goal here is to just have some default visibility around this particular behavior of get_json().

This would affect

@davidism
Copy link
Member

It is documented to return None. I don't plan to change that behavior. You can subclass and override Request.get_json to use whatever behavior you want.

@davidism
Copy link
Member

Hmm, on second thought, maybe I could transition to force=True with a deprecation period. During deprecation, if get_json is called without a correct content type, and force=False wasn't explicitly set, then it would show a DeprecationWarning. I'll have to think about how disruptive this might be though.

@davidism davidism reopened this Feb 15, 2022
@davidism davidism added this to the 2.1.0 milestone Feb 15, 2022
@davidism
Copy link
Member

I'll try it out in 2.1 and see how it goes.

@davidism
Copy link
Member

davidism commented Feb 15, 2022

Never mind, setting force=True is not the right thing, that would always allow users to make requests without the right content type, which is a step backwards. You're asking for a new thing where force=False becomes an error, so basically removing force completely and not returning None on missing content type.

@davidism
Copy link
Member

davidism commented Feb 15, 2022

I could remove force, and make silent=True mean "return None if wrong content type". But that doesn't feel right either, there is a difference between ignoring an incorrect content type and silencing parsing errors (which also seems like a bad option to ever make available).

@davidism
Copy link
Member

This works the same for form data, if the content type isn't application/json or application/x-www-form-urlencoded, then an empty dict is returned for request.files. The problem seems to be that it is much easier to forget to pass the application/json content type with JavaScript, since jQuery, fetch, XHR, etc. don't have special support for sending JSON data. Python tools like httpx or requests, and even the Werkzeug test client, have a json parameter that does this automatically.

I think the most we can do is log a warning if the content type is incorrect.

@davidism davidism self-assigned this Mar 16, 2022
@davidism
Copy link
Member

I think that it's ok to have a difference between form parsing returning silently and JSON parsing failing with a 400 error. As I pointed out, it's much more common for clients to not set a JSON content type. Since that behavior is such a common source of confusion, returning an informative 400 error will make it easier to understand and fix the issue.

stacywsmith pushed a commit to stacywsmith/flask-restx that referenced this issue Mar 29, 2022
pallets/werkzeug#2339 changed the behavior of Request.get_json()
and the Request.json property to raise a BadRequest if Request.json
is accessed and the content type is not `application/json`.

Argument parsing allows parsing from multiple locations, but if the
locations include `json` and the content type is not `application/json`,
a `BadRequest` is now raised with Werkzeug >= 2.1.0.

Handle this case and continue argument parsing.
@jantonacci
Copy link

This feature change caused all API endpoints to return 400 with Connexion 2.13.0 and Flask 2.1.0. Pinning Werkzeug to release 2.0.* resolved it.

@jwoytek
Copy link

jwoytek commented Mar 30, 2022

This change breaks Flask-RESTful, which attempts to get both JSON and value arguments from any request while trying to parse arguments (reqparse.py line 125). The call to getattr() results in a call to get_json() in werkzeug, which bombs with the BadRequest exception. There is no way to pass the force option here. Flask-RESTful was leveraging the return of None when called on a non-JSON request.

stacywsmith pushed a commit to stacywsmith/flask-restx that referenced this issue Apr 1, 2022
pallets/werkzeug#2339 changed the behavior of `Request.get_json()`
and the `Request.json` property to raise a `BadRequest` if `Request.get_json()`
is called without `silent=True`, or the `Request.json` property is accessed,
and the content type is not `"application/json"`.

Argument parsing allows parsing from multiple locations, and defaults to
`["json", "values"]`, but if the locations include `"json"` and the content
type is not `"application/json"`, a `BadRequest` is now raised with Werkzeug >= 2.1.0.

Invoking `Request.get_json()` with the `silent=True` parameter now handles
the situation where `"json"` is included in the locations, but the content type
is not `"application/json"`.
stacywsmith pushed a commit to stacywsmith/flask-restx that referenced this issue Apr 1, 2022
pallets/werkzeug#2339 changed the behavior of `Request.get_json()`
and the `Request.json` property to raise a `BadRequest` if `Request.get_json()`
is called without `silent=True`, or the `Request.json` property is accessed,
and the content type is not `"application/json"`.

Argument parsing allows parsing from multiple locations, and defaults to
`["json", "values"]`, but if the locations include `"json"` and the content
type is not `"application/json"`, a `BadRequest` is now raised with Werkzeug >= 2.1.0.

Invoking `Request.get_json()` with the `silent=True` parameter now handles
the situation where `"json"` is included in the locations, but the content type
is not `"application/json"`.
stacywsmith pushed a commit to stacywsmith/flask-restx that referenced this issue Apr 1, 2022
pallets/werkzeug#2339 changed the behavior of `Request.get_json()`
and the `Request.json` property to raise a `BadRequest` if `Request.get_json()`
is called without `silent=True`, or the `Request.json` property is accessed,
and the content type is not `"application/json"`.

Argument parsing allows parsing from multiple locations, and defaults to
`["json", "values"]`, but if the locations include `"json"` and the content
type is not `"application/json"`, a `BadRequest` is now raised with Werkzeug >= 2.1.0.

Invoking `Request.get_json()` with the `silent=True` parameter now handles
the situation where `"json"` is included in the locations, but the content type
is not `"application/json"`.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants