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

Add note about Body parameters #900

Merged
merged 2 commits into from Feb 4, 2020
Merged

Add note about Body parameters #900

merged 2 commits into from Feb 4, 2020

Conversation

pawamoy
Copy link
Sponsor Contributor

@pawamoy pawamoy commented Jan 21, 2020

Add a link to section containing the documentation for Body parameters, emphasizing that using Pydantic models is not mandatory.

It's a follow-up for issue #895. Me and another colleague had the same trouble, thinking that Pydantic models were mandatory to accept params in the request body.

Waiting for your feedback of course as I wrote this note quickly, just to initiate the PR.

Add a link to section containing the documentation for Body parameters,
emphasizing that using Pydantic models is not mandatory.
@codecov
Copy link

codecov bot commented Jan 21, 2020

Codecov Report

Merging #900 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #900   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         293    293           
  Lines        7692   7701    +9     
=====================================
+ Hits         7692   7701    +9
Impacted Files Coverage Δ
tests/test_filter_pydantic_sub_model.py 100% <0%> (ø) ⬆️
fastapi/utils.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55afb70...2fef7ed. Read the comment docs.

@tiangolo tiangolo merged commit 68723d5 into tiangolo:master Feb 4, 2020
@tiangolo
Copy link
Owner

tiangolo commented Feb 4, 2020

Thanks for your contribution! 🎉 🍰

@mbello
Copy link

mbello commented Feb 26, 2020

I tripped on this as well. And I really think it would be awesome to have FastAPI work with properly annotated but otherwise "unmodified" python functions and this "Body(...)" departs from that and is quite ugly I would say.

May I suggest we change the @app.post to:

- Suggestion 1: add a "param-lookup" argument to the post decorator so that we could specify the order in which FastAPI will look for 'non-path' and non BaseModel descendants parameters. It could be "query, body", "body, query", "query" or "body". Current default is "query", but should be changed to "query, body".

- Suggestion 2: add a "fallback-to-body" argument to the decorator, so if a parameter is missing after looking for them in the query string, FastAPI would then check the request body for them.

If any of these suggestions sound ok, I could try developing it and submitting a pull request. Just let me know if it would be welcomed.

@mbello
Copy link

mbello commented Feb 26, 2020

The "Body(...)" workaround also has the limitation described here: #1011
(the parameters will be considered optional)

@pawamoy
Copy link
Sponsor Contributor Author

pawamoy commented Feb 26, 2020

I think it's quite consistent to have Path, Query, Body, Header and Cookie parameters and to have only one default when none of the above class/function is used, which is to consider it's a query parameter.

In any case, since the implicit behavior can become quite confusing with lots of parameters (default to query, pydantic models as body, params in the route as path, etc.), I think it's best to always be explicit and wrap your parameters in Query, Body, etc. (just my personal opinion!).

But I'm concerned by the issue your references, as I thought using Body(...) (with an ellipsis) would make the parameter mandatory, just as what is done for Query: https://fastapi.tiangolo.com/tutorial/query-params-str-validations/#make-it-required.

@mbello
Copy link

mbello commented Feb 27, 2020

While pydantic and FastAPI will understand the parameter as being mandatory, linters and cpython will see it as optional and it is an error in python to have optional parameters preceeding non-optional parameters so it may force one to reorder parameters around.
Hence, we get into the business of "converting methods" to become FastAPI-friendly when I always thought that being able to use existing methods 'as-is' was a great appeal of FastAPI.

Also, I really do not understand why it would be a bad idea to fallback to checking for parameters in the body, even if you choose to limit this behavior to post method and when the body content type is form or json.

May I suggest two more alternatives?

Suggestion 3: Have the decorator accept kwargs like this:

@app.post('/', a = Body(...), b = Path(...), c = Query(...))
async def test(a: str, b: int, c: bool):
pass

Suggestion 4: create type alias to indicate where the parameters will be found

somewhere on FastAPI codebase:

TPath = typing.Union
TBody = typing.Union
TQuery = typing.Union
THeader = typing.Union
TCookie = typing.Union

@app.post('/')
async def test(a: TBody[str], b: TPath[int], c: TQuery[bool]):
pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants