Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Why not use the return type annotation for the response_model? #101

Closed
JHSaunders opened this issue Mar 24, 2019 · 12 comments
Closed

Why not use the return type annotation for the response_model? #101

JHSaunders opened this issue Mar 24, 2019 · 12 comments
Labels
question Question or problem question-migrate

Comments

@JHSaunders
Copy link
Contributor

JHSaunders commented Mar 24, 2019

Description

This is a very minor question on chosen syntax. Why not use the return type annotation for the response_model? For example rather than:

@app.post("/items/", response_model=Item)
async def create_item(*, item: Item):
    return item

You could concievably go:

@app.post("/items/") 
async def create_item(*, item: Item) -> Item:
    return item

which just seems like it uses the language facilities better and is a bit more pythonic. There is no real change in feature set, and I am sure i was thought of, but what was the reasoning behind not doing it?

@JHSaunders JHSaunders added the question Question or problem label Mar 24, 2019
@tiangolo
Copy link
Owner

That was the original idea, but then there was no way to put the status code in the same annotation.

Or if the status code was there then the response model would have to be the parameter.

And the same with the response description.

Also, because in many cases the function doesn't really return an Item object but a dict (or database object, etc) that is then serialized using that Item Pydantic model, users would see type errors, for having a function that declared that it would return an Item and it was actually returning a dict.

@Imaclean74
Copy link

thanks for the explanation. Maybe add some of that to the docs ? To be honest - I had the same reaction. Working thru the tutorial - you first see query params and body as type inferred from the hints. So it's a little surprising to see response needing to be marked explicitly. The reasons make sense - so a quick note would help clear things up for new users.

@JHSaunders
Copy link
Contributor Author

Created a pull request at:

#109

tiangolo pushed a commit that referenced this issue Mar 29, 2019
)

* Update response model documentation to explain design choice

Closes #101

* 📝 Update note about return function type annotation
@tiangolo
Copy link
Owner

Thanks for the idea @Imaclean74

Thanks for the PR @JHSaunders 🎉 🌮

@zor-el
Copy link

zor-el commented May 19, 2019

Huh, I'm trying to evaluate/learn in FastAPI, and I have searched like mad for an explanation on this. :)

But I don't quite get this statement, @tiangolo:

Also, because in many cases the function doesn't really return an Item object but a dict (or database object, etc) that is then serialized using that Item Pydantic model, users would see type errors, for having a function that declared that it would return an Item and it was actually returning a dict.

When/where would users see type errors? How is that different from function parameters that initialize an int with a Path? This surely must be a solvable problem, no?

Also, perhaps one should not dismiss the use of function return types for response schema deduction. Return types are the most natural way to describe a response. An idea for a (backward-compatible) approach:

@app.get('/hello')
def hello() -> Greeting: # ...

would behave as if it was written as:

@app.get('/hello', response_model=Greeting)
def hello() -> Greeting: # ...

but

@app.get('/hello', response_model=Greeting)
def hello() -> dict: # ...

would ignore the function's return type (at least for the API documentation).

Any thoughts?

@tiangolo
Copy link
Owner

@zor-el this breaks mypy:

from fastapi import FastAPI
from pydantic import BaseModel


class Item(BaseModel):
    title: str
    description: str = None

fake_db = {
    "foo": {
        "title": "Fighters",
        "description": "There goes someone's hero"
    }
}


app = FastAPI()


@app.get("/item/{item_id}")
def read_item(item_id: str) -> Item:
    return fake_db.get(item_id)

The return fake_db.get(item_id) reports an error "Incompatible return value type (got "Optional[Dict[str, str]]", expected "Item")".


Also, the idea/benefit of type annotations is to help you use them in the rest of your code. To detect errors, have completion, etc. But a path operation function is never called by yourself. It's called by FastAPI. So, there's no real benefit in adding a return annotation to the functions. Even though at first it seems the more intuitive place to declare that.


About this:

[...] function parameters that initialize an int with a Path [...]

In the latest version, when you import and use Path, it is actually a function that returns an instance of a class Path, but has an annotation of Any, so mypy knows that it doesn't have to check its type.

That involved quite some work just to make mypy work correctly and let users expect their code to be correctly typed.

Adding return annotations would bring the same kind of problem.

@teskje
Copy link

teskje commented Jun 1, 2019

Also, the idea/benefit of type annotations is to help you use them in the rest of your code. To detect errors, have completion, etc. But a path operation function is never called by yourself. It's called by FastAPI. So, there's no real benefit in adding a return annotation to the functions. Even though at first it seems the more intuitive place to declare that.

Even if the function is only called by framework code, I still find specifying a return type useful to statically ensure that I return the correct value from my request handler. Specifying that in the response_model will only ensure that at runtime, which is less ideal IMO.

Another reason for adding an explicit return value for my request handlers is that my mypy settings simply force me to annotate every function. I find that useful to ensure I don't forget it sometimes. But with the current FastAPI behavior, this forces me to repeat myself, e.g.:

@app.get("/", response_model=Result)
def handle() -> Result:
    ...

So in my mind it would be valuable (and, as you mention, more intuitive) if FastAPI would pick up the return type as a default response_model. It should still be possible to override that by specifying an explicit response_model if needed of course. That would be similar to how we can already specify simple parameters without needing Query, Body, etc.

Just my two cents. Btw, thanks for your great work on this project!

@feluxe
Copy link

feluxe commented Jun 24, 2019

If this snippet (from @tiangolo example above):

@app.get("/item/{item_id}")
def read_item(item_id: str) -> Item:
    return fake_db.get(item_id)

raises Incompatible return value type (got "Optional[Dict[str, str]]", expected "Item"). Would it not be clearer to let the user return Item explicitly like in this snippet:

@app.get("/item/{item_id}")
def read_item(item_id: str) -> Item:
    return Item(**fake_db.get(item_id))

and catch pydantic exceptions from within the decorator function? I don't know the implications, but I guess this could solve the mypy/typing issues and it could make a cleaner and clearer API compared to:

@app.get("/item/{item_id}", response_model=Item)
def read_item(item_id: str):
    return fake_db.get(item_id)

@tiangolo
Copy link
Owner

For completeness, if mypy is complaining about functions without types, you can always:

@app.get("/item/{item_id}", response_model=Item)
def read_item(item_id: str) -> Any:
    return fake_db.get(item_id)

@septatrix
Copy link

I think having the response_model automatically inferred from the annotation as an alternative to explicitly passing it would be beneficial. In the case where response_model is not set fastapi would simply use get_type_hints(handler).get("return"). Having both be set (and especially be set to different types) should still be allowed.

This allows people who prefer explicitly converting the types themselves to use mypy and benefit from static analysis. Everybody else could still use the argument to the decorator. Furthermore this would allow for checks like the following:

from myapp.models import UserModel
from myapp.db import fake_db, Manager, Employee

@app.get("/managers/{manager_id}", response_model=UserModel)
def get_manager(manager_id: str) -> Manager:
    return fake_db.Managers.get(manager_id)

@app.get("/employees/{employee_id}", response_model=UserModel)
def get_user(employee_id: str) -> Employee:
    return fake_db.Employees.get(employee_id)

This way they get the checks from mypy which ensure the are returning the correct thing while simultaneously benefiting from pydantic reducing them to a common user model.

@AntonVucinic
Copy link

I think having the response_model automatically inferred from the annotation as an alternative to explicitly passing it would be beneficial.

I also think it would be more consistent with the rest of the framework. The Depends function has a similar functionality when used with classes. This is explicitly mentioned in the docs

@tiangolo
Copy link
Owner

tiangolo commented Jan 7, 2023

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

You can read the new docs here: https://fastapi.tiangolo.com/tutorial/response-model/ 🤓

baoliay2008 added a commit to baoliay2008/lccn_predictor that referenced this issue Feb 16, 2023
given that FastAPI `response_model` now support return type annotation

see more details at:
1. tiangolo/fastapi#101
2. tiangolo/fastapi#1436
@tiangolo tiangolo changed the title [QUESTION] Why not use the return type annotation for the response_model? Why not use the return type annotation for the response_model? Feb 24, 2023
@tiangolo tiangolo reopened this Feb 28, 2023
Repository owner locked and limited conversation to collaborators Feb 28, 2023
@tiangolo tiangolo converted this issue into discussion #8247 Feb 28, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
question Question or problem question-migrate
Projects
None yet
Development

No branches or pull requests

8 participants