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

Use return type annotation for response_model by default. #875

Closed
wants to merge 1 commit into from
Closed

Use return type annotation for response_model by default. #875

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 17, 2020

This shouldn't be merged as-is, because I haven't added related tests yet, but I wanted to send this up for discussion. In my use case I am basically already returning the correct models from the endpoint functions already, and have return types enforced through mypy, so it feels quite unnecessary to duplicate the return type to the response_model argument to get the correct OpenAPI docs.

Basically I ended up creating a custom router that does similar logic as in this PR, but I am curious why it is not the default, since this behaviour would feel intuitive to me. I'm not offended at all if this gets rejected, but thought it's easier to explain what I'm asking through a PR instead of trying to explain it verbally in an issue.

@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #875 into master will decrease coverage by 61.51%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #875       +/-   ##
===========================================
- Coverage     100%   38.48%   -61.52%     
===========================================
  Files         289      287        -2     
  Lines        7595     7574       -21     
===========================================
- Hits         7595     2915     -4680     
- Misses          0     4659     +4659
Impacted Files Coverage Δ
fastapi/routing.py 69.13% <50%> (-30.87%) ⬇️
docs/src/sql_databases_peewee/sql_app/crud.py 0% <0%> (-100%) ⬇️
docs/src/sql_databases_peewee/sql_app/database.py 0% <0%> (-100%) ⬇️
docs/src/sql_databases/sql_app/database.py 0% <0%> (-100%) ⬇️
docs/src/sql_databases_peewee/sql_app/schemas.py 0% <0%> (-100%) ⬇️
docs/src/sql_databases_peewee/sql_app/main.py 0% <0%> (-100%) ⬇️
docs/src/sql_databases/sql_app/main.py 0% <0%> (-100%) ⬇️
docs/src/sql_databases/sql_app/schemas.py 0% <0%> (-100%) ⬇️
docs/src/sql_databases/sql_app/models.py 0% <0%> (-100%) ⬇️
docs/src/bigger_applications/app/routers/users.py 0% <0%> (-100%) ⬇️
... and 270 more

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 3eca945...b181a52. Read the comment docs.

@tiangolo
Copy link
Owner

Thanks for your interest @juhovh-aiven ! ✨

Check the note here in the "Technical Details": https://fastapi.tiangolo.com/tutorial/response-model/#response_model_include-and-response_model_exclude

The response model is declared in this parameter instead of as a function return type annotation, because the path function may not actually return that response model but rather return a dict, database object or some other model, and then use the response_model to perform the field limiting and serialization.

It also means that if you return a dict but declare the return as a Pydantic model, mypy would rightfully complain, because you are not really returning the data type you declared your function to return.

It would be quite easy that in many cases the response_model doesn't really match the type of data that you are actually returning from your function, so it easily end up leading to confussion and mypy errors.

I'll close this PR, but feel free to add more comments, issues, or new PRs.

@tiangolo tiangolo closed this Jan 17, 2020
@ghost
Copy link
Author

ghost commented Jan 17, 2020

Check the note here in the "Technical Details": https://fastapi.tiangolo.com/tutorial/response-model/#response_model_include-and-response_model_exclude

Thanks for a quick response!

I had actually already checked that note in the "Technical Details", and I also use that feature quite heavily elsewhere (returning a dict and then validating/limiting fields through a pydantic model). However, my suggestion was simply about removing duplication, i.e. if the response_model and return annotation type are identical, one could skip the response_model part and simply go with the return annotation. If they are not identical one could still return a dict from the callable and simply set the response_model manually as before, the response_model value would always override whatever return annotation type is given.

But as mentioned, it's pretty easy to do with a custom route class with its own constructor, so I'll just keep going with that. Thank you for your good work with FastAPI, I must say I quite like it.

@ghost ghost deleted the default-response-model branch January 17, 2020 13:07
@uriyyo uriyyo mentioned this pull request Nov 8, 2020
9 tasks
@tiangolo
Copy link
Owner

tiangolo commented Jan 7, 2023

This was added in #1436 🚀 It's available in FastAPI 0.89.0.

The new docs are here: https://fastapi.tiangolo.com/tutorial/response-model/ 🤓

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

1 participant