search

strawberry-graphql / strawberry

A GraphQL library for Python that leverages type annotations 🍓
https://strawberry.rocks
MIT License
4.01k stars 533 forks source link

Move request and response out of context #2687

Open patrick91 opened 1 year ago

patrick91 commented 1 year ago

While working on #2681 I noticed that the way we pass the request and response param is a bit brittle, if someone updates the context to remove either then things will break (especially when removing response)

I propose we move request and response on Info, so they are always available (when doing HTTP requests).

The annoying thing of this is that we'll have two more Generic TypeVars on the Info parameter, but it is probably for the best?

Upvote & Fund

Fund with Polar

DoctorJohn commented 1 year ago

Alternatively we could discuss standardizing context by having a base context class that requires response and request to be present. That way we could also enforce a connection_params property which would allow us to get rid of code like this:

https://github.com/strawberry-graphql/strawberry/blob/bfb2f1759d3b9ba39ece742be48be601ca939a43/strawberry/subscriptions/protocols/graphql_transport_ws/handlers.py#L181-L183

The new awesome integration base classes already have a TypeVar for context which we would then bind to said new base context class (Context = TypeVar("Context", bound=NewBaseContextClass)). At least that's what I had in mind :thinking:

Unfortunately that would be a breaking change since we don't know what people currently use for context. Could be worth it tho since it makes the context much more predictable and thus usable by integrations.

patrick91 commented 1 year ago

one issue that I have around that is that context is either a class or a (typed) dict in different integrations :)

I think it might be easier to not include the request in the context at all (or leave it up to the user)

DoctorJohn commented 1 year ago

one issue that I have around that is that context is either a class or a (typed) dict in different integrations :)

I'm aware, I was trying to propose that we require it to derive from a new base class. But I agree it would be less disruptive to move request to info since we didn't have any restrictions on the context so far. I'm happy either way, but here are my original thoughts on this:

  1. Whatever happens to request should happen to connection_params too since I want to get rid of the guessing code shown in my previous comment (it's present in some integrations as well).
  2. I remember there was confusion in the past about how to access context (dot vs bracket notation). Introducing a base context class could be our chance to clearify how to access context no matter what integration is used.
  3. I like how the Apollo docs describe the purpose of info vs context (https://www.apollographql.com/docs/apollo-server/data/resolvers/#resolver-arguments) and I feel like context would be more suitable for requests according to their description. Obviously we don't have to follow their interpretation but it'll could be less suprising for newcomers.

Edit: Just talked with Patrick about this on Discord. We kinda came to the conclusion that introducing a base class for context would be a really disruptive change since we didn't have any restrictions on the context so far. Leaving my original thoughts up here so anyone wondering can see we did consider them.