standardize coroutine context propagation in the execution #1349
Conversation
dariuszkuc
added
changes: major
| @@ -16,23 +16,16 @@ | |||
|
|
|||
| package com.expediagroup.graphql.server.execution | |||
|
|
|||
| import com.expediagroup.graphql.generator.execution.GraphQLContext | |||
| const val GRAPHQL_COROUTINE_CONTEXT_KEY = "GRAPHQL_COROUTINE_CONTEXT" | |||
There was a problem hiding this comment.
Is this supposed to be GRAPHQL_COROUTINE_SCOPE? Maybe we should just use the value defined in the generator package so we don't have it defined in two locations
There was a problem hiding this comment.
Actually I'm thinking of dropping the String key in favor of just using classes as keys.
...otlin/com/expediagroup/graphql/server/spring/subscriptions/ApolloSubscriptionSessionState.kt
Outdated
Show resolved
Hide resolved
| val contextMap = contextFactory.generateContextMap(request) ?: emptyMap<Any, Any?>() | ||
|
|
||
| val customContext: CoroutineContext = contextMap[GRAPHQL_COROUTINE_CONTEXT_KEY] as? CoroutineContext ?: EmptyCoroutineContext | ||
| val graphQLExecutionScope = CoroutineScope(coroutineContext + customContext + SupervisorJob()) |
There was a problem hiding this comment.
Not an expert on coroutines, but does this need to be cancelled at the end of request processing? Was just reading through this: https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-coroutine-scope/index.html
There was a problem hiding this comment.
When we return the value from this method then all children coroutines will already complete so there is nothing to cancel.
dariuszkuc
force-pushed
the
coroutine_context
branch
2 times, most recently
from
7515e76
to
f99d268
Compare
dariuszkuc
changed the title
|
Ran into #1300 testing this library in a Kotlin project and was able to confirm that this PR resolves the issues we were seeing with the spring security context being empty when coming from the GraphQL endpoints! |
to be even more specific we were able to get things working with manually built jars specifically from this commit 87fc89c |
📝 Description Attempts to standardize how we can propagate the coroutine/reactor context created by the server when processing incoming requests to the data fetchers that would resolve the underlying fields. `GraphQLContext` map was introduced in `graphql-java` 17 as means to standardize usage of GraphQL context (previously it could be any object). We can store the original GraphQL request context in the map and then use it from within the data fetchers. This is a breaking change as we are changing signatures of `GraphQLContextFactory` and `FunctionDataFetcher`. 🔗 Related Issues * ExpediaGroup#1336 * ExpediaGroup#1318 * ExpediaGroup#1300 * ExpediaGroup#1257
dariuszkuc
force-pushed
the
coroutine_context
branch
from
87fc89c
to
126e826
Compare
|
Updated to keep the deprecated custom GraphQL context around (will drop it in v7) to provide easier migration path. |
.../main/kotlin/com/expediagroup/graphql/server/spring/execution/SpringGraphQLContextFactory.kt
Show resolved
Hide resolved
…oup#1349) 📝 Description Attempts to standardize how we can propagate the coroutine/reactor context created by the server when processing incoming requests to the data fetchers that would resolve the underlying fields. `GraphQLContext` map was introduced in `graphql-java` 17 as means to standardize usage of GraphQL context (previously it could be any object). We can store the original GraphQL request context in the map and then use it from within the data fetchers. This is a breaking change as we are changing signatures of `GraphQLContextFactory` and `FunctionDataFetcher`. 🔗 Related Issues * ExpediaGroup#1336 * ExpediaGroup#1318 * ExpediaGroup#1300 * ExpediaGroup#1257
|
I'm interested in trying this out. I don't want to build and publish it somewhere for myself and our CI. @dariuszkuc Do you publish snapshots of the |
|
@frozenice we just released |
…oup#1349) 📝 Description Attempts to standardize how we can propagate the coroutine/reactor context created by the server when processing incoming requests to the data fetchers that would resolve the underlying fields. `GraphQLContext` map was introduced in `graphql-java` 17 as means to standardize usage of GraphQL context (previously it could be any object). We can store the original GraphQL request context in the map and then use it from within the data fetchers. This is a breaking change as we are changing signatures of `GraphQLContextFactory` and `FunctionDataFetcher`. 🔗 Related Issues * ExpediaGroup#1336 * ExpediaGroup#1318 * ExpediaGroup#1300 * ExpediaGroup#1257
📝 Description
Attempts to standardize how we can propagate the coroutine/reactor context created by the server when processing incoming requests to the data fetchers that would resolve the underlying fields.
GraphQLContextmap was introduced ingraphql-java17 as means to standardize usage of GraphQL context (previously it could be any object). We can store the original GraphQL request context in the map and then use it from within the data fetchers.This is a breaking change as we are changing signatures of
GraphQLContextFactoryandFunctionDataFetcher.🔗 Related Issues