get_context() is only resolved once per WS connection for FastAPI

[kristjanvalur]

Unlike for http operations, the get_context() handler is set up as a FastAPI dependency for the websocket endpoint on GraphQLRouter and resolved when the websocket connection is initialized.
So, even though the protocol handler's get_context() member is evaluated for each subscription, or any other operation, it will ultimately return this single cached value for the duration of the websocket connection.

This may be by design, but it is a bit unexpected and is not mentioned in the documentation. It also means that the dependencies cannot be used via the yield pattern to acquire and release resources for the operation.

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.

[tsmith023]

This is an interesting side-effect of the current implementation!

In my understanding, the problem arises due to https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/fastapi/router.py#L216 where, as you say, any yield functionality in the self.context_getter is resolved once by Depends() and thus never again subsequently.

Would the solution then be to simply remove the Depends() here? Would doing this have any unintended consequences? @Kludex, do you have thoughts regarding this issue?

[Kludex]

Can you show me a code snippet on which this is a problem for strawberry?

[kristjanvalur]

Well, I noticied this when many months ago when I was looking at authentication. Obviously, authentication needs happen only once per connection. But authorization might change.
Anyway, here is a trivial example:

def custom_context_dependency() -> float:
    return time.time()


async def get_context(
    custom_value=Depends(custom_context_dependency),
):
    return {
        "custom_value": custom_value,
    }


@strawberry.type
class Query:
    @strawberry.field
    def example(self, info: Info) -> str:
        return f"Hello at {info.context['custom_value']}"

A more concrete example might be this. Note that I didn't test the yield functionality, so I am making a guess, but my guess is that the transaction will be kept open for the entire connection, which might have unintended consequences:

def custom_context_dependency():
    with db.get_session() as sesssion:
        yield check_authorization(session)

Depending on the type of connection made, these queries will work differently.

[Kludex]

I see what you mean, but the way I see it, this is the intended behavior.

A note on the documentation with a meaningful example clarifying this may be helpful.

[kristjanvalur]

In other words, dependencies cannot be expected to be evaluated for every request.
That's fine, except that is not how it works in FastAPI and that's not what those coming from FastAPI may expect. They may expect a graphql operation to correspond to a single "request" in terms of dependencies. And the different behaviour of the graphql code depending on the type of connection a client makes may be perplexing.
I

[Kludex]

This last message says something different from the ones above.

I don't see how this is different from what is expected from FastAPI users. Would you mind sharing a minimal reproducible example, a comment on the line on which you'd expect a different result, and the client command you used to call the application?

[tsmith023]

I guess the misunderstanding that you're highlighting here is about clarifying the scope of the behaviour of Depends.

I would say that, since it is part of the FastAPI functionality, its scope should remain within the scope of FastAPI. That is to say, FastAPI dependencies should evaluate per request in the context of FastAPI, which is the overall HTTP request that contains within it the GraphQL request. I think it would be unreasonable to require Strawberry to implement the same Dependency Injection functionality of FastAPI per resolver, which is what I believe you are describing.

[kristjanvalur]

Possibly. But what I am describing is that the behaviour is different, denpending on how the client decides to invoke the resolver. The GraphQL schema is designed without respect to the physical connection model, be it over HTTP, or using WebSockets. And the scope of the Depends and the behaviour of the GraphQL api, will be different, depending on this choice of client protocol, which is out of the control of the Strawberry application.

Without changing strawberry code, the only way to resolve this is to require that Dependencies do not perform any request-specific actions, for example, make Database queries or other such things that need to return information that is up-to-date when the request is invoked. And of course, document this explicitly.

[kristjanvalur]

I guess that what I'm trying to say is that the designer of the Strawberry Schema using FastAPI integration should not need to be aware of the different modes of invocation, whether invoked via a http (1.1, cached, perhaps) request, or over WebSocket, which is out of his control. Or, barring that, explicitly warning him not to return any request-specific information using dependency injection.

[tsmith023]

I'm afraid you will need to provide a concrete example of some code that is behaving in a way that you do not expect as our discussion so far is too vague for any detailed understanding.

However, from the FastAPI docs https://fastapi.tiangolo.com/advanced/websockets/#using-depends-and-others, FastAPI's DI does provide per-request functionality on every websocket message in the example. (Repeated return of "Session cookie or query token value is: some-key-token" in https://fastapi.tiangolo.com/advanced/websockets/#try-the-websockets-with-dependencies).

Since https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/fastapi/router.py#L227 is coded in the same way as the FastAPI example, I do not see how the behaviour could be different from that provided by FastAPI itself.

[tsmith023]

@kristjanvalur, revisiting this issue in the context of the linked PR is helping my understanding with regards to the problem. Indeed, it seems that this issue may be closely related to the one here, since that one also occurs due to a confusion around the underlying protocol.

Here is an MRE that shows the context is cached at an initial value that never updates:

import asyncio
import strawberry
from datetime import datetime
from strawberry.fastapi import BaseContext, GraphQLRouter
from strawberry.types import Info
from fastapi import Depends, FastAPI, Request, WebSocket
from typing import AsyncGenerator

def get_request(request: Request = None, websocket: WebSocket = None) -> Request:
    return request or websocket

class Context(BaseContext):
    request: Request
    def __init__(
        self,
        request: Request = Depends(get_request),
    ) -> None:
        self.request = request
        self.now = datetime.now().strftime("%H:%M:%S")


async def get_context(context: Context = Depends()) -> Context:
    return context

#-----------------------------------------------------------------------------
@strawberry.type
class Subscription:
    @strawberry.subscription
    async def echo_time(self, info: Info, target: int = 100) -> AsyncGenerator[str, None]:
        for i in range(target):
            yield f"It's currently {info.context.now} (iteration {i})"
            await asyncio.sleep(0.5)

@strawberry.type
class Query:
    @strawberry.field
    async def say_hi(self, info: Info, name: str) -> str:
        return f"Hi {name}"

#-----------------------------------------------------------------------------
schema = strawberry.Schema(
    query=Query,
    subscription=Subscription,
)
graphql_api = GraphQLRouter(
    schema,
    graphiql=True,
    context_getter=get_context,
)

app = FastAPI(title='GraphQL API')
app.include_router(graphql_api, prefix='/graphql')

info.context.now never updates while the iteration count i does when operating the echo_time Subscription.

@kristjanvalur, is your point that this is unexpected and that the context should be regenerated on every subsequent websocket message? Or is it that, due to the way WS works compared to HTTP, the context is not regenerated on each subsequent WS request but is regenerated on each subsequent HTTP request? And, as you've clarified, GraphQL cares not for the underlying protocol so this is confusing to the user?

[kristjanvalur]

I originally was confused and the behaviour didn't match my expectations. At the time I was not very familiar with the internals of FastAPI (I am a bit more now, after submitting a number of PRs there).

GraphQL mentioned that the context geter is a FastAPI dependency and as such, can rely on other depencies. In the context of regular FastAPI api development, dependencies have a clearly defined scope, which is the context of the evaluation of the endpoint. They are therefore typically used for Authentication, Authorization, and for example database session management.

The documentation, by advertising that it is a dependency, seemed to promise the same kind of scope to the Strawberry resolvers. And in fact, it does for regular HTTP based resolvers. But then, it doesn't, if the resolver is run from a Websockets connection.

I understand now that the dependency is actually injected to the FastAPI websocket endpoint (via some hackery) and as such evaluated outside the control of Strawberry, when the original protocol upgrade is performed. FastAPI does not offer any documented ways of manually resolving dependencies.

I guess it would be technically possible to expand FastAPI's dependency graph mechanism for manual invocations by custom code. It would require a PR and some changes, but would be kind of nifty. It would allow us to add such dependencies directly onto the resolvers. Or, to manually evaluate the getcontext dependency before running resolvers.

At the moment, thouh, I think the documentation should be updated to reflect that the scope of the dependency is not strictly defined, or that it depends on the protocol, and that one should not assume per-request-scope. It sort of makes the usefulness of the FastAPI dependency mechanism useless (their main value is that they are cached and can be re-used between different dependencies for an endpoint, e.g. a "GetUser" dependency can be used for various other dependencies which perform various functions).

[kristjanvalur]

Please note that the above code is not the one which is causing confusion. It is wholly expected that info.context.now stays fixed within the context of the echo_time resolver. After all, it is yielding from a loop inside a single method invocation, one expects "info" to only be updated when the function is called.

The confusion is this: If you change say_hi to return info.context.now in its response, if say_hi query is repeatedly called from a WebSocket connection, then it will return the same timestamp for each call. If you call it over HTTP, it will return changing timestamp.

[tsmith023]

Your comment on dependencies being essentially pointless, I've seen echoed elsewhere. So I'd like to get into the weeds on this problem and hopefully find a nice solution. Potentially leveraging extensions if the FastAPI integration itself cannot be refactored.

[kristjanvalur]

FastAPI context injection is interesting. I have a PR in place to streamline it. I had also thought about making a spearate one to make it more flexible for extensions to extend. That way extensions could provide their own "magic" dependency types and could use the Depends and Security types in a function signature.

We'd add some APIs:

1. One to process a function signature and convert it into a "Dependency" if dependencies are found. Provide a registry of builtin dependencies (similar to how a Request param is identified in FastAPI)
2. Provide a method to invoke such a "Dependency", automatically injecting and evaluating recursive dependencies.

But it is significant work, and not guaranteed to jive with the vision of FastAPI maintainers. Considering how long my PR fastapi/fastapi#5554 has lingered, I'm not sure its worth it.