Generate context with suspend and nullable in subscriptions #1053
Conversation
smyrick
added
changes: major
| counter = "BRANCH" | ||
| value = "COVEREDRATIO" | ||
| minimum = "0.74".toBigDecimal() | ||
| minimum = "0.70".toBigDecimal() |
There was a problem hiding this comment.
With removal of the default context factory we don't have code we are testing much anymore. We could separate some of the routers to internal functions but there is not much going on anymore in the spring module other than defining the beans and subscriptions which are covered.
| graphQLContext: GraphQLContext | ||
| ): Mono<GraphQLContext> = Mono.just(graphQLContext) | ||
| graphQLContext: GraphQLContext? | ||
| ): GraphQLContext? = graphQLContext |
There was a problem hiding this comment.
There is no need for a mono wrapper anymore. We should just cache the context to be used later
|
|
||
| private fun saveContext(operationMessage: SubscriptionOperationMessage, session: WebSocketSession) { | ||
| val connectionParams = getConnectionParams(operationMessage.payload) | ||
| val context = runBlocking { contextFactory.generateContext(session) } |
There was a problem hiding this comment.
This line here is still ambiguous to me. If we wrapping this in mono { } then the code is not run unless the client subscribes and then it might not run first before the operations are executed
| val context = sessionState.getContext(session) | ||
|
|
||
| // If we do not have a context, that means the init message was never sent | ||
| return if (context != null) { |
There was a problem hiding this comment.
The context could be null. Since we have to handle this case this is what sparked all the other changes in subscriptions
...lin/com/expediagroup/graphql/server/spring/subscriptions/SpringGraphQLSubscriptionHandler.kt
Outdated
Show resolved
Hide resolved
|
Can we also update the docs to reflect those changes? |
smyrick
force-pushed
the
suspend-context-factory
branch
from
3f6da31 to
a945bdc
Compare
smyrick
force-pushed
the
suspend-context-factory
branch
from
6d77f77 to
86b032f
Compare
…roup#1053) * Generate context with suspend and nullable in subscriptions * Update tests * Do not cache null context for subscriptions * Rename internal methods * WIP * Update subsctiption javadocs * Use expression body * Update docs for context Co-authored-by: Shane Myrick <accounts@shanemyrick.com>
…roup#1053) * Generate context with suspend and nullable in subscriptions * Update tests * Do not cache null context for subscriptions * Rename internal methods * WIP * Update subsctiption javadocs * Use expression body * Update docs for context Co-authored-by: Shane Myrick <accounts@shanemyrick.com>
2 participants
📝 Description
There are two issues we are fixing here:
GraphQLContextFactoryshould supportsuspendfunctionsSupporting
suspendfunctions is easy enough. We just updated the interface. However that also means inApolloSubscriptionProtocolHandlerwe need to be able to run the suspend function in a class that deals with reactorFlux. Once we generate the context it could come back asnull. If it does our old logic was to return a classDefaultGraphQLContext. However, this was now blowing up because if you had a custom context of typeMyCustomContextand used that as an argument in your subscription functions, graphql-java was throwing an illegal argument exception because we were trying to passDefaultGraphQLContextto the typeMyCustomContext.So what that means is that we really can't have a
DefaultGraphQLContextinstead, if theGraphQLContextFactoryimplementation could possibly return null, the schema functions need to have a parameter type defined as nullable🔗 Related Issues
Default Context added here: #955
Resolves: #945
Resolves: #1052