vitalik / django-ninja

💨 Fast, Async-ready, Openapi, type hints based framework for building APIs
https://django-ninja.dev
MIT License
6.84k stars 416 forks source link

Response schema from annotation #46

Closed fojetin closed 1 month ago

fojetin commented 3 years ago

In this moment ninja use decorator param to find response schema.

@api.post('/login', response={200: Token, 401: Message, 402: Message}) @api.get("/tasks", response=List[TaskSchema]) @api.post("/users/", response=UserOut)

I think we should use python annotation like as request schemas def login(request, payload: Auth) -> {200: Token, 401: Message, 402: Message}): def tasks(request) -> List[TaskSchema]: def create_user(request, data: UserIn) -> UserOut:

We can take this schema funcion.__annotations__.get('return')

@vitalik what you think about this enhancement? I want take on this task!

vitalik commented 3 years ago

Hi @fojetin Initially django-ninja had responses done with return annotations... But it was not covering all cases (like multiple response models and codes #16 ) and looked too long in practice

fojetin commented 3 years ago

multiple response models and codes works

>>> def login(request, payload: int) -> {200: str, 401: int, 402: str}:
...  pass
... 
>>> login.__annotations__
{'payload': <class 'int'>, 'return': {200: <class 'str'>, 401: <class 'int'>, 402: <class 'str'>}}
fojetin commented 3 years ago

I think it seems pretty

def some_longlonglonglonglonglonglonglonglonglong_api_method_name(
    some_param: int,
    some_more: str,
    more: int,
    crazy_long_param_name: str,
    one_more_big_param_name_like_snake: int,
) -> {200: int, 400: dict}:
    pass

and of course we should keep the ability to specify response in decorator

fojetin commented 3 years ago

Why i really wants to make it, because i want to defining endpoints and their schemas in many files and register them to API in one other file.

In this moment i should do this:

def add(request, a: int, b: int):
    return {"result": a + b}
import add

router = Router()

router.get("/add", response=t.Dict[str, int])(add)

This splits the request and response schema into different files.

Or i think we should add multiple inheritance using router like router.add_router(func)

vitalik commented 3 years ago

hm.. that's interesting approach

ok, I think I will add both options - with responnse argument and return-type-annotation

but there is a big typing issue if you will use annnotations:

1) mypy will not validate this:

CleanShot 2020-12-05 at 18 18 17@2x

2) Even for one result mypy will not validate it

def foo(request) -> Foo:
    return {'foo': 'bar'}
fojetin commented 3 years ago

Yeah, mypy not validate this, but flake8 returns no errors

kristofgilicze commented 3 years ago

Recent on-boarding experience with django-ninja:

Everything was clean and as expected ( I did not even need the docs, everything was super intuitive. ), up until the return schema handling.

The type hint approach what I was expecting.

So this kind of breaks the flow.

Despite this I really enjoy it so far!

I have checked the corresponding issues with FastAPI, this is still unresolved over there.

I like the idea of a InferringRouter, keeping the backwards compatibility. https://fastapi-utils.davidmontague.xyz/user-guide/inferring-router/

[EDIT]: I'have 'ported' the solution from fastapi-utils - Seems to be working:

I only tested trivial cases, though I don't think this is too invasive.

import uuid
from ninja import Router, Schema
from ninja.constants import NOT_SET
from typing import TYPE_CHECKING, Any, Callable, get_type_hints, List

from apps.users.models import User

class InferringRouter(Router):
    """
    Overrides the route decorator logic to use the annotated return type as the `response_model` if unspecified.
    """

     if not TYPE_CHECKING:  # pragma: no branch

        # as in: https://github.com/dmontagu/fastapi-utils/blob/master/fastapi_utils/inferring_router.py
        # def add_api_operation(
        #         self,
        #         path: str,
        #         methods: List[str],
        #         view_func: Callable,
        #         ** kwargs: Any
        #         ) -> None:
        #     if kwargs.get("response") is NOT_SET:
        #         kwargs["response"] = get_type_hints(view_func).get("return")
        #     return super().add_api_operation(path, methods, view_func, **kwargs)

        def add_api_operation(
            self,
            path: str,
            methods: List[str],
            view_func: Callable,
            *,
            response: Any = NOT_SET,
            **kwargs: Any
        ) -> None:

            # I like this a little better then fiddling around in the kwargs with string keys.
            if response is NOT_SET:
                response = get_type_hints(view_func).get("return") or NOT_SET
            return super().add_api_operation(path, methods, view_func, response=response, **kwargs)

router = InferringRouter()

class UserOut(Schema):
    email: str
    telephone: str
    uuid: uuid.UUID

@router.get('/test/')
def pay(request) -> UserOut:
    return User.objects.first()
vitalik commented 3 years ago

hi @kristofgilicze

Indeed overriding Router.add_api_operation is the recommended approach for making return annotations marked as response schemas

kristofgilicze commented 3 years ago

Ok! Is an InferringRouter something that will be eventually included in the module, or you prefer to keep the module API in sync with FastAPI?

vitalik commented 3 years ago

@kristofgilicze

well this is a very controversial part.. lot of people would like to see this built in and lot of other people are strongly against it (because it just break type-checking )

I think for now I will just add some sort of best practices section to documentation where this example will be outlined and tests covered

davidszotten commented 3 years ago

what about supporting the type annotation in "simple" cases, e.g. when it's a single value, since that is also accepted by mypy?

mom1 commented 2 years ago

for the case when the annotation is not set too

if response is NOT_SET:
    response = get_type_hints(view_func).get("return") or NOT_SET