aiohttpparser: Fix 500 error with JSON content-type and empty body #297

fmqa · 2018-09-25T22:20:59Z

Fixes 500 errors when Content-Type is JSON and the request body is empty.
This is done by catching the resulting JSONDecodeError and checking whether the input is empty.

This should make the PR pass the style check

sloria · 2018-10-22T23:11:22Z

Sorry for the delay in getting to this. I wasn't sure if this was correct given that aiohttp decided not to fix the issue on their end: aio-libs/aiohttp#3302 .

But I think it is correct to handle an empty body in webargs since it has its own missing value and the decorator API.

@Kochab Would you mind adding a test for this? If you don't have time, let me know, and I can do it when I have some free time.

Once the test is added, this is good to merge.

…SON bodies

fmqa · 2018-10-23T19:10:58Z

Hi @sloria. No problem! I have added a test for this as requested. It doesn't look idiomatic, but I did it this way because webtest.TestApp.post() adds an unwanted Content-Length header. webtest.TestRequest.blank() can be used to build a minimal request with just a Content-Type header and an empty/zero-length body.

Fixes as the location loading code undergoes refactoring for the flaskparser: - Restore missing mimetype check for JSON payloads - Treat form data as a multidict without extraneous AttributeError check (after testing, flask will return '{}' for form data when it is missing or empty -- just respect/allow this) - req.headers is a multidict type as well - fix some mistakes with new argument ordering Changes to the base test app at src/webargs/testing.py of note: - Because each route can only specify that it loads data from a single location (unless you pull extra tricks), several testing cases have had their routes modified, e.g. "/echo" -> "/echo_json" - New test, test_parse_json_list_error_malformed_data added to check that webargs does not incorrectly treat non-list data as singleton lists when passing data to a schema with list fields. The previous behavior was questionable before, but with full data going to schemas it definitely becomes incorrect. - Many tests which previously expected success with malformed data ignored now expect 422 errors. For example, sending a non-dict JSON value (e.g. `1`) should fail because it cannot be loaded by the schema. It should *not* be silently ignored because data was provided and failed to parse. This is a major behavior change for webargs. - Reverse the decision on marshmallow-code#297 . After reading the referent material, I agree with the aiohttp maintainers in aio-libs/aiohttp#3302 . "" and "{}" should not be treated equivalently -- they are different inputs. Additionally, I foresee that supporting "" as "{}" will pose an issue if the proposal for "json_or_form" in marshmallow-code#419 is accepted. More detailed explanation is provided in the inline comment, and the test for this case is preserved with its expectations reversed Supporting modifications were made in tests/apps/flask_app.py , including the addition of `ma.EXCLUDE` when the detected marshmallow version is >= 3. Minor fix in core: restore the cache clearing step. This is apparently used by the flaskparser. Possibly this only shows up under test usage? The main parse function should always be cloning the parser before its cache could be populated.

Fixes as the location loading code undergoes refactoring for the flaskparser: - Restore missing mimetype check for JSON payloads - Treat form data as a multidict without extraneous AttributeError check (after testing, flask will return '{}' for form data when it is missing or empty -- just respect/allow this) - req.headers is a multidict type as well - fix some mistakes with new argument ordering Changes to the base test app at src/webargs/testing.py of note: - Because each route can only specify that it loads data from a single location (unless you pull extra tricks), several testing cases have had their routes modified, e.g. "/echo" -> "/echo_json" - New test, test_parse_json_list_error_malformed_data added to check that webargs does not incorrectly treat non-list data as singleton lists when passing data to a schema with list fields. The previous behavior was questionable before, but with full data going to schemas it definitely becomes incorrect. - Many tests which previously expected success with malformed data ignored now expect 422 errors. For example, sending a non-dict JSON value (e.g. `1`) should fail because it cannot be loaded by the schema. It should *not* be silently ignored because data was provided and failed to parse. This is a major behavior change for webargs. - Reverse the decision on marshmallow-code#297 . After reading the referent material, I agree with the aiohttp maintainers in aio-libs/aiohttp#3302 . "" and "{}" should not be treated equivalently -- they are different inputs. Additionally, I foresee that supporting "" as "{}" will pose an issue if the proposal for "json_or_form" in marshmallow-code#419 is accepted. More detailed explanation is provided in the inline comment, and the test for this case is preserved with its expectations reversed Supporting modifications were made in tests/apps/flask_app.py , including the addition of `ma.EXCLUDE` when the detected marshmallow version is >= 3.

load_* methods are now expected to return `missing` or {} when data is missing for the given location. e.g. When a JSON body is "" it should be `missing`. This is still considered correct. However, in terms of the external behavior of webargs, go back to the webargs v5.x stance on marshmallow-code#297 and optional JSON data: no data should be interpreted as `{}`. In order to achieve this, the wrapper which calls the various "loader_func"s to load location data explicitly checks the result for `missing` and converts it to `{}` if it is missing. This makes it easy to use webargs to implement an API with optional JSON data, but internally preserves the ability to detect a missing JSON payload and distinguish it from a payload containing `{}`. Revert changes to the regression test for marshmallow-code#297 , and a similar test.

Stated goals in the form of CHANGELOG update Gut significant chunks of webargs.core - replace "parse_{location}" with "load_{location}" "locations=<collection>" with "location=<string>" "location_handler" with "location_loader" - remove "parse_arg" "get_value" - add webargs.multidictproxy which uses a Schema to handle MultiDict types and behave similarly to webargs.core.get_value As a proving ground for this implementation/approach, get flaskparser tests to pass under ma3 Fixes as the location loading code undergoes refactoring for the flaskparser: - Restore missing mimetype check for JSON payloads - Treat form data as a multidict without extraneous AttributeError check (after testing, flask will return '{}' for form data when it is missing or empty -- just respect/allow this) - req.headers is a multidict type as well - fix some mistakes with new argument ordering Changes to the base test app at src/webargs/testing.py of note: - Because each route can only specify that it loads data from a single location (unless you pull extra tricks), several testing cases have had their routes modified, e.g. "/echo" -> "/echo_json" - New test, test_parse_json_list_error_malformed_data added to check that webargs does not incorrectly treat non-list data as singleton lists when passing data to a schema with list fields. The previous behavior was questionable before, but with full data going to schemas it definitely becomes incorrect. - Many tests which previously expected success with malformed data ignored now expect 422 errors. For example, sending a non-dict JSON value (e.g. `1`) should fail because it cannot be loaded by the schema. It should *not* be silently ignored because data was provided and failed to parse. This is a major behavior change for webargs. Supporting modifications were made in tests/apps/flask_app.py , including the addition of `ma.EXCLUDE` when the detected marshmallow version is >= 3. After work on the relevant PR and trying things both ways, the current handling of empty JSON bodies (interpreting them as {}) is being kept externally, but changed internally. This relates to the decision made in marshmallow-code#297 . Originally in the course of work on marshmallow-code#419, it seemed like it might be necessary to change the behavior to make missing JSON data behave as `missing` externally. However, making the change internal and wrapping it to present the same external interface is best. load_* methods are expected to return `missing` or {} when data is missing for the given location. e.g. When a JSON body is "" it should be `missing`. This is considered correct. However, in terms of the external behavior of webargs, no data should be interpreted as `{}`. In order to achieve this, the wrapper which calls the various "loader_func"s to load location data explicitly checks the result for `missing` and converts it to `{}` if it is missing. This makes it easy to use webargs to implement an API with optional JSON data, but internally preserves the ability to detect a missing JSON payload and distinguish it from a payload containing `{}`.

aiohttpparser: Fix 500 error with JSON content-type and empty body

1ddb55d

fmqa mentioned this pull request Sep 25, 2018

500 when content-type is 'application/json' and body is None #229

Closed

aiohttpparser: Use double quoted empty string

c0cee7d

This should make the PR pass the style check

test_aiohttpparser: Add a test to ensure correct behaviour on empty J…

7454c35

…SON bodies

sloria merged commit 1019518 into marshmallow-code:dev Oct 25, 2018

sirosen mentioned this pull request Sep 4, 2019

Only allow users to specify a single location per parse call & pass full location data to schema.load #420

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aiohttpparser: Fix 500 error with JSON content-type and empty body #297

aiohttpparser: Fix 500 error with JSON content-type and empty body #297

fmqa commented Sep 25, 2018

sloria commented Oct 22, 2018

fmqa commented Oct 23, 2018

Navigation Menu

aiohttpparser: Fix 500 error with JSON content-type and empty body #297

aiohttpparser: Fix 500 error with JSON content-type and empty body #297

Conversation

fmqa commented Sep 25, 2018

sloria commented Oct 22, 2018

fmqa commented Oct 23, 2018