django-ninja icon indicating copy to clipboard operation
django-ninja copied to clipboard

Response schema from annotation

Open fojetin opened this issue 4 years ago • 12 comments

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!

fojetin avatar Dec 04 '20 19:12 fojetin

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

vitalik avatar Dec 04 '20 21:12 vitalik

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 avatar Dec 05 '20 01:12 fojetin

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 avatar Dec 05 '20 01:12 fojetin

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)

fojetin avatar Dec 05 '20 02:12 fojetin

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
  1. Even for one result mypy will not validate it
def foo(request) -> Foo:
    return {'foo': 'bar'}

vitalik avatar Dec 05 '20 16:12 vitalik

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

fojetin avatar Dec 05 '20 17:12 fojetin

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()

kristofgilicze avatar Apr 24 '21 02:04 kristofgilicze

hi @kristofgilicze

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

vitalik avatar Apr 24 '21 06:04 vitalik

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?

kristofgilicze avatar Apr 25 '21 15:04 kristofgilicze

@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

vitalik avatar Apr 26 '21 07:04 vitalik

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

davidszotten avatar Jul 02 '21 15:07 davidszotten

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

mom1 avatar Nov 17 '21 20:11 mom1