Custom context_getter function overrides default context behaviour thus removing 'request'

[tsmith023]

Hello strawberry devs. I've recently come across this package when desiring to move away from Graphene+Flask to Strawberry+FastAPI.

However, when I define a custom context_getter function like:

async def get_context(cache: Redis = Depends(yield_cache), g: AsyncGraphTraversal = Depends(yield_traversal)) -> Dict[str, Redis | AsyncGraphTraversal]:
    return {
        'cache': cache,
        'g': g
    }

and call it in:

from strawberry.fastapi import GraphQLRouter
gql_app = GraphQLRouter(
    schema = schema,
    context_getter = get_context,
)

then the default context values of 'request' and 'background_tasks' are overwritten by the custom context. Since I see no other way of passing request down to the info.context object outside of the GraphQLRouter instance through the custom context_getter function, is this potentially a bug?

I looked at the source code and saw that line 65 of strawberry/fastapi/router.py reads self.context_getter = context_getter or self._get_context where:

@staticmethod
async def __get_context(
    background_tasks: BackgroundTasks, request: Request = None, ws: WebSocket = None
) -> Optional[Any]:
    return {"request": request or ws, "background_tasks": background_tasks}

which will naturally ignore the addition of 'request' into info.context when there is a custom context_getter function.

Since these are functions at this point, it is not simple to merge their results. However, further down on lines 88 and 136, these functions are injected as dependencies and passed to handlers on lines 123, 153, and 163. If separate default_context and custom_context variables were defined, could their results be merged at this point?

[patrick91]

@tsmith023 sounds like something we should do, especially since we don't have a way to get the request from the context getter.

Do you agree @Kludex?

[Kludex]

We'll need to perform some magic here if we want to inject information.

    @staticmethod
    def __get_context_getter(
        custom_getter: Callable[..., Optional[Dict[str, Any]]]
    ) -> Callable[..., Dict[str, Any]]:
        async def dependency(
            background_tasks: BackgroundTasks,
            request: Request = None,
            ws: WebSocket = None,
            custom_getter: Optional[Dict[str, Any]] = None
        )
            return {"request": request or ws, "background_tasks": background_tasks, **custom_getter}
        
        sig = signature(dependency)
        sig = sig.replace(
            parameters=[
                *list(sig.parameters.values())[:-1],
                sig.parameters["custom_getter"].replace(default=Depends(custom_getter))
            ]
        )
        dependency.__signature__ = sig
        return dependency

Then on the __init__():

self.context_getter = self.__get_context_getter(context_getter)

Notice that context_getter needs to be a callable, so we need to either change the default to lambda: None, or internally map None -> lambda: None, or on the __get_context_getter make the change before injecting into the signature.

FastAPI will look at the signature to resolve the dependencies. That's why it looks "weird".

A minimal working snippet with this solution:

from fastapi import FastAPI, Request, Depends
from inspect import signature
from functools import partial

app = FastAPI()

def dependency(request: Request, context_getter: dict):
    return {"request": request, **context_getter}


def context_getter():
    return {"hi": "there"}


sig = signature(dependency)
sig = sig.replace(
    parameters=[
        *list(sig.parameters.values())[:-1],
        sig.parameters["context_getter"].replace(default=Depends(context_getter))
    ]
)
dependency.__signature__ = sig


@app.get("/")
def home(dep = Depends(dependency)):
    print(dep)

[tsmith023]

@Kludex and @patrick91, thank you for your rapid response to this issue, especially on a Saturday evening!

@Kludex, your solution works well in my application and I will leave that minimal solution for now while I proceed with the rest of the migration. Can I assume that this change will be integrated into the main package in a further feature update?

[Kludex]

Can I assume that this change will be integrated into the main package in a further feature update?

@patrick91 is the one to ask that.

[patrick91]

Can I assume that this change will be integrated into the main package in a further feature update?

I don't see why not 😊 would you be able to send a PR for this? 😊

[tsmith023]

Absolutely, I'd love to! I'll make sure the above solution works on my fork and then open the PR to be reviewed :)