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

Typehints instead of response_model #2296

Closed
9 tasks done
uvicorn opened this issue Nov 4, 2020 · 28 comments
Closed
9 tasks done

Typehints instead of response_model #2296

uvicorn opened this issue Nov 4, 2020 · 28 comments
Labels
answered feature New feature or request reviewed

Comments

@uvicorn
Copy link

uvicorn commented Nov 4, 2020

First check

  • I added a very descriptive title to this issue.
  • I used the GitHub search to find a similar issue and didn't find it.
  • I searched the FastAPI documentation, with the integrated search.
  • I already searched in Google "How to X in FastAPI" and didn't find any information.
  • I already read and followed all the tutorial in the docs and didn't find an answer.
  • I already checked if it is not related to FastAPI but to Pydantic.
  • I already checked if it is not related to FastAPI but to Swagger UI.
  • I already checked if it is not related to FastAPI but to ReDoc.
  • After submitting this, I commit to:
    • Read open issues with questions until I find 2 issues where I can help someone and add a comment to help there.
    • Or, I already hit the "watch" button in this repository to receive notifications and I commit to help at least 2 people that ask questions in the future.
    • Implement a Pull Request for a confirmed bug.

Example

Now:

from fastapi import FastAPI
from typing import Dict, List
app = FastAPI()


@app.get("/", response_model=Dict[str, List[str]])
def read_root():
    return {"Hello": ["tom", "hesus"]}

The solution you would like

I want to do it through typehints:

from fastapi import FastAPI
from typing import Dict, List
app = FastAPI()


@app.get("/")
def read_root() -> Dict[str, List[str]]:
    return {"Hello": ["tom", "hesus"]}
@uvicorn uvicorn added the feature New feature or request label Nov 4, 2020
@Kludex
Copy link
Sponsor Collaborator

Kludex commented Nov 4, 2020

I don't know if it was only a decision or if there's an issue on the suggested approach.

My input here will be related to MyPy:

from typing import Dict, List

from fastapi import FastAPI
from pydantic import BaseModel

app = FastAPI()

class Potato(BaseModel):
    name: str
    color: str

@app.get("/")
def get_potato() -> Potato:
    return {"name": "best potato", "color": "purple"}

Doing as above, MyPy will get sad:

test.py:14: error: Incompatible return value type (got "Dict[str, str]", expected "Potato")
Found 1 error in 1 file (checked 1 source file)

Maybe a "FastAPI MyPy plugin" can be created to allow it (in case the suggestion is accepted and if is possible to create a plugin like that, I've never tried).

@uvicorn
Copy link
Author

uvicorn commented Nov 4, 2020

No, i want use annotations instead of response_model. i want fastapi to understand typehints

@Kludex
Copy link
Sponsor Collaborator

Kludex commented Nov 4, 2020

I understand what you said... I want that too. 👀

@uvicorn
Copy link
Author

uvicorn commented Nov 4, 2020

i think to do something like this:

from typing import get_type_hints

@app.get("/")
async def root() -> dict:
    return {"hello": "world"}

hints = get_type_hints(root)
if "return" in hints:
    response_model  = hints["return"]

# or
# response_model = hints.get("return")

@Mause
Copy link
Contributor

Mause commented Nov 4, 2020

There is already a library that allows you to do this:

https://fastapi-utils.davidmontague.xyz/user-guide/inferring-router/

@uvicorn
Copy link
Author

uvicorn commented Nov 4, 2020

thx! @tiangolo , how about do it in APIRoter by default?

@Mause
Copy link
Contributor

Mause commented Nov 5, 2020

That would break existing usage patterns, as right now you can, for example, return a dictionary, and have fastapi validate and transform it into a pydantic model. If the pattern was changed this would no longer be possible without breaking type validation

@uriyyo
Copy link
Contributor

uriyyo commented Nov 8, 2020

@peach-lasagna There were multiple discussions and implementation of this feature.

PR #1436, #875, and issue #101 they all about this.

Basically, I was an author of #1436 and after reading discussion thread #101, I understood the main idea why this feature isn't implemented.

In my opinion, the best solution to this problem will be to implement this feature but not to break the existing code base.
One of the possible solutions is:

  1. In case if response_model is set then use it.
  2. In case if response_model isn't set use return type annotation as response_model.
# response model - Dict[str, List[str]]
@app.get("/", response_model=Dict[str, List[str]])
def read_root():
    return {"Hello": ["tom", "hesus"]}

# response model - Dict[str, List[str]]
@app.get("/")
def read_root() -> Dict[str, List[str]]:
    return {"Hello": ["tom", "hesus"]}

# response model - Dict[str, List[str]]
@app.get("/", response_model=Dict[str, List[str]])
def read_root() -> Dict[str, Set[str]]:
    return {"Hello": {"tom", "hesus"}}

In case if we accept the proposal above we can simply reopen and merge PR #1436 because it has already implemented the logic described above.

I really like this idea, and a lot of peoples want this feature.

@johnthagen
Copy link
Contributor

This would make FastAPI's usage of type annotations much more natural, especially for new users familiar with how type annotations work outside of FastAPI. As a new user, it was very surprising to need to put the return type into response_model.

@uriyyo
Copy link
Contributor

uriyyo commented Nov 9, 2020

@johnthagen Totally agree with you, I faced the same problem when have started working with FastAPI.

@tiangolo What do you think about this feature? We can implement this feature fully backward compatible.

@matt1484
Copy link

I really like this feature and have implemented it locally as well and even with support for multiple different responses. If Im honest though I think this should be a non-backwards compatible change. I feel like (even with the way FastAPI is currently) if you allow too many different ways to specify responses it can allow code that is less clear and I think FastAPI should converge on a single way to specify responses (while it is still < v1.0.0). An example being:

@app.post('/api/do_stuff', status_code=200, response_model=Y, responses={200: {'model': Z}})
def do_stuff(body: Any) -> X:
    return {}

If I was just starting to work with FastAPI I would be confused as to what this is actually going to try to return. Would it be X, Y, or Z? Obviously, there is a single answer (and no one would write something like this), but I think that with this many options it allows devs to create something not immediately clear (and quite ugly). If it were instead only looking at the return type annotations, it would make it so you could only specify one response_model per status_code. Something like:

@app.post('/api/do_stuff', default_status_code=200)
def do_stuff(body: Any) -> X:
    return {}

which is pretty clear that it returns a 200 of type X. And it could even work further like this:

@app.post('/api/do_stuff', default_status_code=200)
def do_stuff(body: Any) -> {200: X, 404: Y, 400: {'model': Z, 'description': 'this is a Z'}}:
    return {}

which is effective enough to say it returns X on 200 (the default), Y on 404, and Z on 400.

@antonagestam
Copy link

@uriyyo Looking at #1436, it looks like it implements this in a backwards-compatible way, would it be worthwhile to re-open that? It seems like the sum of the critique of this feature is that it's not compatible with the current response_model behavior, but like you, I fail to see how they're incompatible. Defaulting to using response_model if given and falling back to the return type annotation should maintain backwards compatibility, right?

@uriyyo
Copy link
Contributor

uriyyo commented Apr 5, 2022

@antonagestam Yes, it's fully backward compatible. I can reopen it, but I am not sure that it will be merged. We should hear the opinion of @tiangolo.

More details from previous discussion #875

@antonagestam
Copy link

antonagestam commented Apr 5, 2022

@uriyyo Cool, I think it could be worthwhile to have an open and fresh PR to make the case? @tiangolo mentions confusion as a reason to not implement this, perhaps that can be alleviated by improving documentation?

As a seasoned Python developer I don't really see that this should cause confusion. The annotated return type of the function needs to be accurate, and isn't always the same type as what will be advertised in the API schema. But, when it is, response_model can be omitted to allow less duplication.

I think wording along those lines in documentation should alleviate the confusion?

@tiangolo
Copy link
Owner

Thanks for the discussion everyone! Yep, as some have said, this is particularly important when returning something different than what you want FastAPI to send in the response. Maybe you are returning a dictionary and you want it documented with a Pydantic model. Or maybe you want to return an object directly from the database, and you want it serialized to JSON and filtered by Pydantic, for example, filtering sensitive data, which is precisely explained in the docs about response_model: https://fastapi.tiangolo.com/tutorial/response-model/#add-an-output-model

Nevertheless, I should also document that the simplest way to document path operations is with -> Any:, as none of your own code would normally call that function, it's called by FastAPI, so you normally don't get that much benefit from annotating it. But still, I totally get that for some it would be better to annotate it strictly.

And I agree that if there's no response_model defined and there's a return type, the type checks (mypy, editors) would catch the error, and the response is probably already the same type as declared, so it makes sense to just use that as the response model.

@uriyyo, would you like to re-open your PR, please? 🙏 🚀

@johnthagen
Copy link
Contributor

For reference for others (took me a little bit to find) the PR @tiangolo is mentioning is:

@uriyyo
Copy link
Contributor

uriyyo commented Oct 24, 2022

@tiangolo I have re-opened PR, please, take a look.

@AbstractiveNord
Copy link

Thanks for the discussion everyone! Yep, as some have said, this is particularly important when returning something different than what you want FastAPI to send in the response. Maybe you are returning a dictionary and you want it documented with a Pydantic model. Or maybe you want to return an object directly from the database, and you want it serialized to JSON and filtered by Pydantic, for example, filtering sensitive data, which is precisely explained in the docs about response_model: https://fastapi.tiangolo.com/tutorial/response-model/#add-an-output-model

Nevertheless, I should also document that the simplest way to document path operations is with -> Any:, as none of your own code would normally call that function, it's called by FastAPI, so you normally don't get that much benefit from annotating it. But still, I totally get that for some it would be better to annotate it strictly.

And I agree that if there's no response_model defined and there's a return type, the type checks (mypy, editors) would catch the error, and the response is probably already the same type as declared, so it makes sense to just use that as the response model.

@uriyyo, would you like to re-open your PR, please? pray rocket

Any is good until you set Mypy in strict mode.

@tiangolo
Copy link
Owner

tiangolo commented Jan 7, 2023

This was added in #1436 (thanks @uriyyo!) 🚀 It's available in FastAPI 0.89.0.

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

@yanyongyu
Copy link

yanyongyu commented Jan 8, 2023

Hi, this new feature released in 0.89.0 breaks my code with return type annotation of custom Response class as follows:

Example code

@app.get("/")
async def route() -> Response:
    return Response(xxx)

FastAPI reads the return type annotation when response_model is not set:

fastapi/fastapi/routing.py

Lines 358 to 359 in 69bd7d8

if isinstance(response_model, DefaultPlaceholder):
response_model = get_typed_return_annotation(endpoint)

for workaround, remove the return type annotation or set the response_model with None.

it could be better to add a assertion if annotation is Response subclass.

@solarjoe
Copy link

solarjoe commented Jan 8, 2023

The new feature also sortof breaks my code :)
I had some endpoints with no response_model defined and an incorrect type hint for the returned value.
So far this had no effect but now it throws a ValidationError.

@johnthagen
Copy link
Contributor

an incorrect type hint for the returned value.

If the type annotation is incorrect, isn't this feature identifying a bug for you that you can now fix?

@solarjoe
Copy link

solarjoe commented Jan 8, 2023

Yes, that's what I meant with "sortof breaks my code". I am actually quite happy that this happened, already fixed on my side.

@Odame
Copy link

Odame commented Jan 9, 2023

Hi, this new feature released in 0.89.0 breaks my code with return type annotation of custom Response class as follows:

Example code

@app.get("/")
async def route() -> Response:
    return Response(xxx)

FastAPI reads the return type annotation when response_model is not set:

fastapi/fastapi/routing.py

Lines 358 to 359 in 69bd7d8

if isinstance(response_model, DefaultPlaceholder):
response_model = get_typed_return_annotation(endpoint)

for workaround, remove the return type annotation or set the response_model with None.

it could be better to add a assertion if annotation is Response subclass.

As a matter of fact, this breaks for any non-Pydantic model used as a return type annotation.

@johnthagen
Copy link
Contributor

Another small real world example that fails the new return type annotation checking:

from starlette.responses import RedirectResponse

@app.get("/", include_in_schema=False)
async def docs_redirect() -> RedirectResponse:
    return RedirectResponse(url="/docs")
fastapi.exceptions.FastAPIError: Invalid args for response field! Hint: check that <class 'starlette.responses.RedirectResponse'> is a valid pydantic field type

@tiangolo
Copy link
Owner

Thanks everyone for the additional reports! 🍰

These simple use cases with a single Response return type are now handled thanks to @Kludex's #5855 😎

And there's a lot of new docs with details here: https://fastapi.tiangolo.com/tutorial/response-model/#other-return-type-annotations

This is part of FastAPI 0.89.1 just released 🎉

@johnthagen
Copy link
Contributor

Confirmed that 0.89.1 fixed our issue. Thanks!

@JacobHayes
Copy link
Contributor

Quite happy to see this change! One snag I noticed when upgrading (aside from some -> Response | list[...] endpoints) was that it seems any models already in the return hint (eg: -> MyModel) seem to be subclassed, rather than referenced directly. I was using a subclass based registry pattern (using some __init_subclass__ magic) which automatically checked for dup names - so these autogenerated subclasses caused dups/errors.

Stacktrace
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File ".../api.py", line 87, in <module>
    def get_amplicon_structure_from_benchling(benchling_entity_id: str) -> BarcodeSchema:
  File "...venv/lib/python3.10/site-packages/fastapi/routing.py", line 638, in decorator
    self.add_api_route(
  File "...venv/lib/python3.10/site-packages/fastapi/routing.py", line 577, in add_api_route
    route = route_class(
  File "...venv/lib/python3.10/site-packages/fastapi/routing.py", line 417, in __init__
    ] = create_cloned_field(self.response_field)
  File "...venv/lib/python3.10/site-packages/fastapi/utils.py", line 117, in create_cloned_field
    use_type = create_model(original_type.__name__, __base__=original_type)
  File "pydantic/main.py", line 1027, in pydantic.main.create_model
  File "pydantic/main.py", line 283, in pydantic.main.ModelMetaclass.__new__
  File "...venv/lib/python3.10/abc.py", line 106, in __new__
    cls = super().__new__(mcls, name, bases, namespace, **kwargs)
  File ".../some_models.py", line 87, in __init_subclass__
    raise ValueError(f"Duplicate class with feature_class={name}: {dup}")
ValueError: Duplicate class with feature_class=...

This is a pretty esoteric edge case so fine if it's not worth fixing (the well documented response_model=None workaround works!), but thought I might flag in case it'd be easy to avoid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered feature New feature or request reviewed
Projects
None yet
Development

No branches or pull requests