strawberry-graphql / strawberry

A GraphQL library for Python that leverages type annotations πŸ“
https://strawberry.rocks
MIT License
4.01k stars 533 forks source link

Using the ASGI app as a Starlette route throws typing errors #3545

Open parafoxia opened 4 months ago

parafoxia commented 4 months ago

Describe the Bug

The following code is more or less a copy of what appears in the docs:

from starlette.applications import Starlette
from strawberry.asgi import GraphQL

from .schema import schema

app = Starlette()
graphql = GraphQL[None, None](schema)
app.add_route("/graphql", route=graphql)

However, it raises the following typing error:

app.py:8: error: Argument 2 to "add_route" of "Starlette" has incompatible type "GraphQL[Any, Any]"; expected "Callable[[Request], Awaitable[Response] | Response]"  [arg-type]
app.py:8: note: "GraphQL[Any, Any].__call__" has type "Callable[[Arg(MutableMapping[str, Any], 'scope'), Arg(Callable[[], Awaitable[MutableMapping[str, Any]]], 'receive'), Arg(Callable[[MutableMapping[str, Any]], Awaitable[None]], 'send')], Coroutine[Any, Any, None]]"

While the code does actually run and appears to function correctly, Mypy is flagging incompatible signatures.

The signature for the route parameter for the add_route method:

(Request) -> (Awaitable[Response] | Response)

The signature of the GraphQL.__call__ method:

(Scope, Receive, Send) -> (None)

The latter signature is the same as the signature for Starlette middleware. I'm presuming Mypy is getting confused, or something isn't properly deliniated to Mypy and it's assuming something it shouldn't be, as the code does work.

It's also worth noting that the documentation should be updated to use full type annotations.

System Information

Additional Context

Starlette version 0.37.2.

Upvote & Fund

Fund with Polar

patrick91 commented 4 months ago

@parafoxia I think the types might be wrong, this was changed a year ago: https://github.com/encode/starlette/pull/2180

but our code still works, no?

parafoxia commented 4 months ago

Yeah it still works. In fact when done like this (on mobile so might be wrong but you get the idea):

app = Starlette(
    routes=[
        ...,
        GraphQL[None, None](schema),
    ]
)

Mypy doesn't complain like it did. I haven't run it on strict though, but at least normal Mypy is fine with it.

parafoxia commented 4 months ago

Btw, slight tangent, would there be something better to use than [None, None]? I'm not really sure what those two types are -- I've seen Context used in the first around the place.

patrick91 commented 4 months ago

@parafoxia I'll make a PR to remove the need for [None, None] that should only be used when subclassing 😊

Yeah it still works. In fact when done like this (on mobile so might be wrong but you get the idea):

app = Starlette(
    routes=[
        ...,
        GraphQL[None, None](schema),
    ]
)

Mypy doesn't complain like it did. I haven't run it on strict though, but at least normal Mypy is fine with it.

I'll ask @Kludex if this is expected tomorrow 😊 if so I can make a PR to adjust the typing on Starlette 😊

parafoxia commented 4 months ago

Awesome, cheers!

Wrt subclassing, I am actually subclassing now to overload the encode_json to use orjson -- would there be something better than [None, None] in this case?

patrick91 commented 4 months ago

something like this:

class MyGraphQL(GraphQL[MyContextType, MyRootType]:
    ...

MyContextType is the return type of get_context and MyRootType is the return type of get_root_value 😊

parafoxia commented 4 months ago

Oooh I see, that makes a lot of sense! That could actually prove useful -- thanks for explaining that!