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

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

Open
parafoxia opened this issue Jun 21, 2024 · 7 comments
Open

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

parafoxia opened this issue Jun 21, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@parafoxia
Copy link
Contributor

parafoxia commented Jun 21, 2024

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

  • Operating system: MacOS
  • Strawberry version (if applicable): 0.235.0

Additional Context

Starlette version 0.37.2.

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@parafoxia parafoxia added the bug Something isn't working label Jun 21, 2024
@patrick91
Copy link
Member

@parafoxia I think the types might be wrong, this was changed a year ago: encode/starlette#2180

but our code still works, no?

@parafoxia
Copy link
Contributor Author

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
Copy link
Contributor Author

parafoxia commented Jul 3, 2024

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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants