fastapi icon indicating copy to clipboard operation
fastapi copied to clipboard

Ability to define response_model with native typing

Open AbstractiveNord opened this issue 1 year ago • 14 comments

First Check

  • [X] I added a very descriptive title to this issue.
  • [X] I used the GitHub search to find a similar issue and didn't find it.
  • [X] I searched the FastAPI documentation, with the integrated search.
  • [X] I already searched in Google "How to X in FastAPI" and didn't find any information.
  • [X] I already read and followed all the tutorial in the docs and didn't find an answer.
  • [X] I already checked if it is not related to FastAPI but to Pydantic.
  • [X] I already checked if it is not related to FastAPI but to Swagger UI.
  • [X] I already checked if it is not related to FastAPI but to ReDoc.

Commit to Help

  • [X] I commit to help with one of those options 👆

Example Code

from fastapi import FastAPI

from pydantic import BaseModel

app = FastAPI()


class TypicalResponse(BaseModel):
    msg: str
    data: int


@app.get("/add/{x}/{y}")
async def add(x: int, y: int) -> TypicalResponse:
    return TypicalResponse(msg="Done!", data=x + y)

Description

Open the automatic doc on this handler See no output schema defined I would like to use typing "-> Type" for annotate response model. This is good for simple handlers.

Wanted Solution

I would like to autodoc parse this typing same as "response_model=TypicalResponse"

Wanted Code

from fastapi import FastAPI

from pydantic import BaseModel

app = FastAPI()

# Same
class TypicalResponse(BaseModel):
    msg: str
    data: int


@app.get("/add/{x}/{y}")
async def add(x: int, y: int) -> TypicalResponse:
    return TypicalResponse(msg="Done!", data=x + y)

Alternatives

I use kwarg "response_model" as single alternative But typing already defines the output model, necessary duplicate as result.

Operating System

Linux

Operating System Details

No response

FastAPI Version

0.79.0

Python Version

Python 3.9.0

Additional Context

Without patch: изображение

With patch: изображение

I think second is better by avoid information duplication

Meanwhile, solution is quite simple. One of the ways to implement (don't judge too much, that's first idea):

  1. Open APIRouter class
  2. Modify self.add_api_route by adding изображение

May be dirty, so welcome to search another ways.

AbstractiveNord avatar Jul 31 '22 18:07 AbstractiveNord

May be dirty, so welcome to search another ways.

Not at all dirty! I like the idea, it is more 'pythonic' and best of all its additional to current practice, so in the future it could be decided to phase it out. If you want, I can put a pull request together.

JarroVGIT avatar Jul 31 '22 18:07 JarroVGIT

Not at all dirty! I like the idea, it is more 'pythonic' and best of all its additional to current practice, so in the future it could be decided to phase it out. If you want, I can put a pull request together.

I am highly interested, why this "essential" feautre didn't imlemented earlier. Maybe some hidden problems in corner cases or something else.

P.S. I think it's essential

AbstractiveNord avatar Jul 31 '22 18:07 AbstractiveNord

Yeah I am just thinking about that, for example when you have multiple return types (anyOf situation in OpenAPI), but that would still work actually. I'll run the test suite, see what happens.

I don't agree that it is "essential" as the functionality is now already there, well documented and in use by literally thousands of projects, but I do think it is more idiomatic.

JarroVGIT avatar Jul 31 '22 18:07 JarroVGIT

Well the test work, if you take into account one edge case: if the endpoint is a functools.partial (of which I am of opinion is not really a use case IRL), one of the tests will fail as that type doesn't have an __annotations__ dunder. Circumventing, this is code that is working:

# self.response_model = response_model
if type(endpoint) != functools.partial:
    self.response_model = response_model or endpoint.__annotations__.get("return")
else:
    self.response_model = response_model or endpoint.func.__annotations__.get("return")

It's not a crazy idea. Though I am wondering, why is this in your opinion an essential feature?

JarroVGIT avatar Jul 31 '22 18:07 JarroVGIT

Well the test work, if you take into account one edge case: if the endpoint is a functools.partial (of which I am of opinion is not really a use case IRL), one of the tests will fail as that type doesn't have an __annotations__ dunder. Circumventing, this is code that is working:

# self.response_model = response_model
if type(endpoint) != functools.partial:
    self.response_model = response_model or endpoint.__annotations__.get("return")
else:
    self.response_model = response_model or endpoint.func.__annotations__.get("return")

It's not a crazy idea. Though I am wondering, why is this in your opinion an essential feature?

Thanks for help!

Essential... Well, i mean it's because this functionality expected to be as basic (by any newbie at least).

FastAPI have Pydantic native intergration, very pythonic style, you may think "wow, nice framework!" And boom! - This typing isn't working.

Not critical, but still.

AbstractiveNord avatar Jul 31 '22 19:07 AbstractiveNord

Well the test work, if you take into account one edge case: if the endpoint is a functools.partial (of which I am of opinion is not really a use case IRL), one of the tests will fail as that type doesn't have an __annotations__ dunder. Circumventing, this is code that is working:

# self.response_model = response_model
if type(endpoint) != functools.partial:
    self.response_model = response_model or endpoint.__annotations__.get("return")
else:
    self.response_model = response_model or endpoint.func.__annotations__.get("return")

It's not a crazy idea. Though I am wondering, why is this in your opinion an essential feature?

You found great corner case!

I consider this as more solid solve:

def discover(c: Callable) -> Dict[str, str]:
    while issubclass(type(c), functools.partial):
        c = c.func
    return c__annotations__

But there another problem - this check make patch much bigger.

AbstractiveNord avatar Jul 31 '22 19:07 AbstractiveNord

this check make patch much bigger.

What do you mean by this?

JarroVGIT avatar Jul 31 '22 19:07 JarroVGIT

this check make patch much bigger.

What do you mean by this?

Probable need to separate this functionality from self.add_api_route For now it's not necessary, but in future it can be a problem.

AbstractiveNord avatar Jul 31 '22 19:07 AbstractiveNord

At the current moment i have this:

if response_model is None:
    handler: Callable= endpoint
    while issubclass(type(handler), functools.partial):
        handler= handler.func
    response_model = handler.__annotations__.get('return')

AbstractiveNord avatar Jul 31 '22 19:07 AbstractiveNord

I created a PR and wrote some tests, I really like the idea :) But. the PR's are mounting up and I noticed that the maintainer of this package only updates every now and then, and mostly when a change he himself needs warrants a new release. So no promises on timelines or anything

JarroVGIT avatar Jul 31 '22 19:07 JarroVGIT

I created a PR and wrote some tests, I really like the idea :)

Thanks a lot

AbstractiveNord avatar Jul 31 '22 19:07 AbstractiveNord

This idea has been rejected previously: https://github.com/tiangolo/fastapi/issues/101

chbndrhnns avatar Jul 31 '22 20:07 chbndrhnns

That is too bad, it seems intuitive but I do follow the reasoning that it could break mypy. Or at least, how FastAPI is now allowing users to implement path operations functions, makes that this change could break mypy as the return type of your POF is not necessarily the same as the actual returned object (as FastAPI does the serialisation afterwards.

One could argue that if one would want to do that (returning an object other than type response_model, to let FastAPI render the response) it could be made explicit that providing a return type to the POF would break mypy.

JarroVGIT avatar Jul 31 '22 20:07 JarroVGIT

This idea has been rejected previously: #101

was it really rejected? BTW, this issue also duplicates: #2296

all of which had a lot of support

rooterkyberian avatar Aug 03 '22 07:08 rooterkyberian

Essential... Well, i mean it's because this functionality expected to be as basic (by any newbie at least).

FastAPI have Pydantic native intergration, very pythonic style, you may think "wow, nice framework!" And boom! - This typing isn't working.

Not critical, but still.

Exactly - I literally went through this! Would be great if we can somehow make this work. If this indeed breaks something, can this configuration be used as an input for those who wants to go use the return types instead of duplicating it in reponse_model?

STKarthikAbiram avatar Oct 03 '22 10:10 STKarthikAbiram

Essential... Well, i mean it's because this functionality expected to be as basic (by any newbie at least). FastAPI have Pydantic native intergration, very pythonic style, you may think "wow, nice framework!" And boom! - This typing isn't working. Not critical, but still.

Exactly - I literally went through this! Would be great if we can somehow make this work. If this indeed breaks something, can this configuration be used as an input for those who wants to go use the return types instead of duplicating it in reponse_model?

My patch is very soft.It checks response_model to be None before grabbing model from annotations.I can agree with problems, mypy, everything, but for newbies it's great, meanwhile everyone can directly set response_model and my patch will not do a thing.

Welcome to test

AbstractiveNord avatar Oct 03 '22 14:10 AbstractiveNord

I think this proposal might also have performance benefits, but it would need to be implemented a bit differently.

Currently when you specify a response model in the routing decorator, the following things happen:

  • duplicate validation of the response when the returned response is already a valid pydantic model and gets validated again; see #1359
  • create_response_field and create_cloned_field have a negative performance impact on big APIs; see #4644

When you don't specify the response_model parameter in the routing decorator and the return type is a pydantic model, you would still want it to be documented in the OpenAPI spec.

That is why I think handling exactly the below case differently throughout FastAPI might be worth investigating:

class MyModel(BaseModel) ...

@router.get("/")
def x() -> MyModel:
  return MyModel(...)

mathiasburger avatar Nov 14 '22 13:11 mathiasburger

I think this proposal might also have performance benefits, but it would need to be implemented a bit differently.

Currently when you specify a response model in the routing decorator, the following things happen:

* duplicate validation of the response when the returned response is already a valid pydantic model and gets validated again; see [[QUESTION] Validation in the FastAPI response handler is a lot heavier than expected #1359](https://github.com/tiangolo/fastapi/issues/1359)

* `create_response_field` and `create_cloned_field` have a negative performance impact on big APIs; see [`created_cloned_field` — slow performance with many models #4644](https://github.com/tiangolo/fastapi/issues/4644)

When you don't specify the response_model parameter in the routing decorator and the return type is a pydantic model, you would still want it to be documented in the OpenAPI spec.

That is why I think handling exactly the below case differently throughout FastAPI might be worth investigating:

class MyModel(BaseModel) ...

@router.get("/")
def x() -> MyModel:
  return MyModel(...)

Quite interesting. I'm going to look it closely.

AbstractiveNord avatar Nov 14 '22 16:11 AbstractiveNord

I think this proposal might also have performance benefits, but it would need to be implemented a bit differently.

Currently when you specify a response model in the routing decorator, the following things happen:

* duplicate validation of the response when the returned response is already a valid pydantic model and gets validated again; see [[QUESTION] Validation in the FastAPI response handler is a lot heavier than expected #1359](https://github.com/tiangolo/fastapi/issues/1359)

* `create_response_field` and `create_cloned_field` have a negative performance impact on big APIs; see [`created_cloned_field` — slow performance with many models #4644](https://github.com/tiangolo/fastapi/issues/4644)

When you don't specify the response_model parameter in the routing decorator and the return type is a pydantic model, you would still want it to be documented in the OpenAPI spec.

That is why I think handling exactly the below case differently throughout FastAPI might be worth investigating:

class MyModel(BaseModel) ...

@router.get("/")
def x() -> MyModel:
  return MyModel(...)

If return annotation should be passed as early as possible due to speed, probably good idea is decorator modification (fastapi/routing.py, line 629). Can anyone test performance properly in both variants?

AbstractiveNord avatar Nov 26 '22 18:11 AbstractiveNord

I found a corner case with Any return annotation, so code block getting bigger, and now it's should be separated for sure.

Raw&Dirty implementation: (fastapi/routing.py, line 600)

    from typing import get_type_hints

    def api_route(self, ...) -> Callable[[DecoratedCallable], DecoratedCallable]:
        def decorator(func: DecoratedCallable) -> DecoratedCallable:
            nonlocal response_model
            if response_model is None:
                handler: Callable = func
                while issubclass(type(handler), functools.partial):
                    handler = handler.func
                return_type = get_type_hints(handler).get('return')
                if return_type is not Any:
                    response_model = return_type

            self.add_api_route(
            ...

Issue found by test_invalid_response_model_raises (thanks to @patrickmckenna). I think it's potential to be a whole pool of "ignore these typings, pass 'None' instead of Any, etc."

AbstractiveNord avatar Nov 26 '22 19:11 AbstractiveNord