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 middlewares with ASGI GraphQL as documented doesn't work #1000

Closed
blazewicz opened this issue Dec 21, 2022 · 2 comments · Fixed by #1009
Closed

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

blazewicz opened this issue Dec 21, 2022 · 2 comments · Fixed by #1009
Assignees
Labels
bug Something isn't working docs Focused on documentation contents
Milestone

Comments

@blazewicz
Copy link

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

rafalp commented Dec 21, 2022

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 rafalp modified the milestones: Ariadne 0.17, Ariadne 0.18 Dec 21, 2022
@rafalp rafalp added the docs Focused on documentation contents label Dec 21, 2022
@rafalp rafalp self-assigned this Jan 18, 2023
@rafalp rafalp added the bug Something isn't working label Jan 18, 2023
@rafalp
Copy link
Contributor

rafalp commented Jan 18, 2023

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 😞

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

Successfully merging a pull request may close this issue.

2 participants