mirumee / ariadne

Python library for implementing GraphQL servers using schema-first approach.
https://ariadnegraphql.org
BSD 3-Clause "New" or "Revised" License
2.21k stars 179 forks source link

Using middlewares with ASGI GraphQL as documented doesn't work #1000

Closed blazewicz closed 1 year ago

blazewicz commented 1 year ago

Adding Middlewares to ASGI GraphQL as documented in https://ariadnegraphql.org/docs/middleware#custom-middleware-example doesn't work.

This is the example from docs at the time of writing this issue:

from ariadne.asgi import GraphQL
from ariadne.asgi.handlers import GraphQLHTTPHandler
from graphql import MiddlewareManager

app = GrapqhQL(
    schema,
    http_handler=GraphQLHTTPHandler(
        middleware=MiddlewareManager(lowercase_middleware),
    ),
)

Firts, mypy will throw following error:

error: Argument "middleware" to "GraphQLHTTPHandler" has incompatible type "MiddlewareManager"; expected
"Union[Callable[[Any, Union[Any, Callable[[Any], Any], None]], Optional[List[Union[Tuple[Any, ...], List[Any], MiddlewareManager, None]]]], Optional[List[Union[Tuple[Any, ...], List[Any], MiddlewareManager, None]]], None]"

Second, running this code will throw following error:

self = <ariadne.asgi.handlers.http.GraphQLHTTPHandler object at 0x127605390>, request = <starlette.requests.Request object at 0x1279b5600>
context = {'request': <starlette.requests.Request object at 0x1279b5600>}

    async def get_middleware_for_request(
        self, request: Any, context: Optional[ContextValue]
    ) -> Optional[MiddlewareManager]:
        middleware = self.middleware
        if callable(middleware):
            middleware = middleware(request, context)
            if isawaitable(middleware):
                middleware = await middleware  # type: ignore
        if middleware:
            middleware = cast(list, middleware)
>           return MiddlewareManager(*middleware)
E           TypeError: graphql.execution.middleware.MiddlewareManager() argument after * must be an iterable, not MiddlewareManager

By looking at the expected type I've tried:

from ariadne.asgi import GraphQL
from ariadne.asgi.handlers import GraphQLHTTPHandler
from graphql import MiddlewareManager

app = GrapqhQL(
    schema,
    http_handler=GraphQLHTTPHandler(
        middleware=[
            MiddlewareManager(lowercase_middleware),
        ],
    ),
)

It does satisfy mypy, but when the code is run the middlewares aren't ever getting called.

By looking at the test suite:

https://github.com/mirumee/ariadne/blob/9a6cd62c9172b5e43cc62cccdec9f28294d7238d/tests/asgi/test_configuration.py#L589..L621

For example in this part:

def test_middlewares_are_passed_to_query_executor(schema):
    http_handler = GraphQLHTTPHandler(middleware=[middleware])
    app = GraphQL(schema, http_handler=http_handler)
    client = TestClient(app)
    response = client.post("/", json={"query": '{ hello(name: "BOB") }'})
    assert response.json() == {"data": {"hello": "**Hello, BOB!**"}}

I've noted, that you use a different syntax. This one is working, but it contradicts the doc and doesn't work with static type checking.

From mypy you will get following error:

error: List item 0 has incompatible type "Callable[[Callable[..., Any], Any, GraphQLResolveInfo, KwArg(Any)], Any]"; expected
"Union[Tuple[Any, ...], List[Any], MiddlewareManager, None]"  [list-item]

Workaround that I've found is to do:

from ariadne.asgi import GraphQL
from ariadne.asgi.handlers import GraphQLHTTPHandler

app = GrapqhQL(
    schema,
    http_handler=GraphQLHTTPHandler(
        middleware=[
            lowercase_middleware,  # type: ignore[list-item]
        ],
    ),
)

I'd suggest adding type annotations to the test suite and run mypy on it.

rafalp commented 1 year ago

Correct, documentation should be fixed to show that middlewares should be passed to manager as list. Could you please contribute a PR fixing this?

Starting with Ariadne 0.18 we split middleware option into middlewares (which we expect to be a list of middlewares) and middleware manager class. I'll put this for that release so I remember to check if MyPy is complaining after changes.

rafalp commented 1 year ago

I've got this fixed in the PR, but there's a gotcha in mypy that it compares argument names on Middleware prototype:

class Middleware(Protocol):
    def __call__(
        self,
        resolver: Resolver,
        obj: Any,
        info: GraphQLResolveInfo,
        **kwargs: Any,
    ) -> Any:
        ...

# Works
def example_middleware(resolver, obj: Any, info: GraphQLResolveInfo, **kwargs: Any):
    return resolver(obj, info, **kwargs)

# Produces error:
def example_middleware(next_, parent: Any, info: GraphQLResolveInfo, **kwargs: Any):
    return resolver(obj, info, **kwargs)
incompatible type "Callable[[Any, Any, GraphQLResolveInfo, KwArg(Any)], Any]"; expected "Middleware"

Mypy has a way for specifying arg as positional only with double underscore, but if you mix it with **kwargs its ignored 😞