openapi: remove JSON body second validation and type casting #1170
Conversation
|
I forgot to mention that there was also some not-fully-working default value handling (which worked only for top-most entity). I've removed that too. Currently, the only working default value handling in body is AFAIK using this approach and if using aiohttp, also this fix (which has unfortunately not yet been merged) is needed. |
|
ok, removing the default value handling breaks tests. I was not thinking about form bodies etc. |
p4l1ly
changed the title
p4l1ly
changed the title
|
To increase coverage, I have removed some edge case checking, for the edge cases that I could not achieve in tests. Could you please check it? |
|
You might find #1140 interesting. I was doing similar work, but haven't had any free time to finish it up. |
|
I've found #1140 interesting, but it covers quite a big part of the connexion's code and I'm afraid I will not find time to dive into it in a foreseeable future. I'm not sure if #1140 makes handling of JSON bodies correct, maybe I could transform the example in this pull request into test so that it could be added into #1140? |
|
Hi @p4l1ly |
p4l1ly
force-pushed
the
oneof
branch
2 times, most recently
from
474ee55 to
8fe35a3
Compare
p4l1ly
force-pushed
the
oneof
branch
2 times, most recently
from
534707b to
e70f6e5
Compare
Body is already validated using jsonschema. There was also some type
casting but it was wrong: e.g. not recurring deeply into dicts and lists,
relying on existence of "type" in schema (which is not there e.g. if
oneOf is used). Anyway, the only reason why types should be casted is
converting integer values to float if the type is number. But this is in
most cases irrelevant.
Added an example, which did not work before this commit (echoed `{}`)
e.g. for
```
curl localhost:8080/api/foo -H 'content-type: application/json' -d
'{"foo": 1}'
```
but now the example works (echoes `{"foo": 1}`).
|
Hi @RobbeSneyders , thanks for your response. I've rebased, retargeted and slightly refactored the pull request. |
|
|
||
| if x_body_name in arguments or has_kwargs: | ||
| return {x_body_name: res} | ||
| return self._get_typed_body_values(body_arg, body_props, additional_props) |
There was a problem hiding this comment.
Why do we need this for form types and not json?
There was a problem hiding this comment.
Because for json, strings are already converted to appropriate python types via json.loads. But there's no such conversion for forms, only string splitting of arrays. So I have left forms to be processed the old way and focused on fixing json, which is simpler to fix and more important (oneOf is rare in form data).
In my opinion, proper solution would perform type conversions somehow via jsonschema validators, at one place, close to the start of the request handling pipeline. Currently, types are converted inconsistently here and there and there and there. But please, this was intended to be a quick simple bugfix MR, let's not dive into big refactorings here.
|
@RobbeSneyders, I've just converted the example into a test. |
|
Thanks for the efforts! Indeed, let's release as 2.14.0 :) |
* Fix uri parsing for query parameter with empty brackets (#1501) * Update tests for changed werkzeug behavior in 2.1 (#1506) pallets/werkzeug#2352 * Bugfix/async security check (#1512) * Add failing tests * Use for else construct * openapi: remove JSON body second validation and type casting (#1170) * openapi: remove body preprocessing Body is already validated using jsonschema. There was also some type casting but it was wrong: e.g. not recurring deeply into dicts and lists, relying on existence of "type" in schema (which is not there e.g. if oneOf is used). Anyway, the only reason why types should be casted is converting integer values to float if the type is number. But this is in most cases irrelevant. Added an example, which did not work before this commit (echoed `{}`) e.g. for ``` curl localhost:8080/api/foo -H 'content-type: application/json' -d '{"foo": 1}' ``` but now the example works (echoes `{"foo": 1}`). * test with oneOf in the requestBody * remove oneof examples: superseded by tests Co-authored-by: Pavol Vargovcik <pavol.vargovcik@kiwi.com> Co-authored-by: Ruwann <ruwanlambrichts@gmail.com> Co-authored-by: Pavol Vargovčík <pavol.vargovcik@gmail.com> Co-authored-by: Pavol Vargovcik <pavol.vargovcik@kiwi.com>
…rst#1170) * openapi: remove body preprocessing Body is already validated using jsonschema. There was also some type casting but it was wrong: e.g. not recurring deeply into dicts and lists, relying on existence of "type" in schema (which is not there e.g. if oneOf is used). Anyway, the only reason why types should be casted is converting integer values to float if the type is number. But this is in most cases irrelevant. Added an example, which did not work before this commit (echoed `{}`) e.g. for ``` curl localhost:8080/api/foo -H 'content-type: application/json' -d '{"foo": 1}' ``` but now the example works (echoes `{"foo": 1}`). * test with oneOf in the requestBody * remove oneof examples: superseded by tests Co-authored-by: Pavol Vargovcik <pavol.vargovcik@kiwi.com>
…rst#1170) * openapi: remove body preprocessing Body is already validated using jsonschema. There was also some type casting but it was wrong: e.g. not recurring deeply into dicts and lists, relying on existence of "type" in schema (which is not there e.g. if oneOf is used). Anyway, the only reason why types should be casted is converting integer values to float if the type is number. But this is in most cases irrelevant. Added an example, which did not work before this commit (echoed `{}`) e.g. for ``` curl localhost:8080/api/foo -H 'content-type: application/json' -d '{"foo": 1}' ``` but now the example works (echoes `{"foo": 1}`). * test with oneOf in the requestBody * remove oneof examples: superseded by tests Co-authored-by: Pavol Vargovcik <pavol.vargovcik@kiwi.com>
…operties openapi: remove JSON body second validation and type casting (spec-first#1170)
JSON body is already validated using jsonschema. There was also some type
casting but it was wrong: e.g. not recurring deeply into dicts and lists,
relying on existence of "type" in schema (which is not true e.g. if
oneOf is used). Anyway, the only reason why types should be casted is
converting integer values to float if the type is number. But this is in
most cases irrelevant.
Added an example, which did not work before this commit (echoed
{})e.g. for
but now the example works (echoes
{"foo": 1}).Fixes #691
Changes proposed in this pull request:
From
OpenAPIOperation._get_body_argument:additionalPropertieshandling)numbertype is for both integers and floats so we don't need to forcibly convert int to float (and there's really nothing else to convert in the json-parsed body).