https://kotlinlang.org logo
#graphql-kotlin
Title
# graphql-kotlin
j

Joe

04/07/2021, 10:44 PM
Noticing a couple things with `FlowSubscriptionExecutionStrategy`: 1. it now returns a
Publisher
now instead of a
Flow
. This makes sense for spring webflux, but we're consuming `Flow`s directly and the
Flow
->`Publisher`->`Flow` round trip has caused resource leak problems for us in the past. Would something like a
NativeFlowSubscriptionExecutionStrategy
that returns `Flow`s be welcome in
graphql-kotlin
or should we maintain it externally? 2. instrumentation doesn't appear to be applied to subscription results. Previously fixed in https://github.com/ExpediaGroup/graphql-kotlin/pull/742 but maybe in going to
Publisher
lost that? (or maybe something's changed in graphql-java 16 around it?) other than that, updating package import definitions and some renames in graphql-java 16 (ExecutionPath -> ResultPath) appear to be the main changes that we need to adjust for. There's also some weirdness with the selectionSet duplicating fields that appears to be filed upstream as https://github.com/graphql-java/graphql-java/issues/2275.
d

Dariusz Kuc

04/07/2021, 11:34 PM
If I remember right we tried using flows for subscriptions but it was causing some issues
I'll try to dig up corresponding prs
j

Joe

04/08/2021, 12:12 AM
ok. I'll plan on maintaining NativeFlow version in our own repos for now. I'm surprised by (2) since there's a test for instrumentation being applied, but did find that
result.getData<Publisher<ExecutionResult>>().asFlow().onEach { it.... }.....collect()
is missing the instrumented values in the
it
ExecutionResult.
d

Dariusz Kuc

04/08/2021, 12:53 AM
ah yeah so re #1 -> I've updated it to publisher when integrating with spring server as current subscription beans were based on flux (https://github.com/ExpediaGroup/graphql-kotlin/pull/972)
ideally we should rewrite those spring handlers to use flows but didn't have time to do it
j

Joe

04/08/2021, 1:12 AM
Yeah makes perfect sense from an already existing flux perspective
d

Dariusz Kuc

04/08/2021, 1:44 AM
re: 2 I just checked the unit test and extensions gets populated as expected (https://github.com/ExpediaGroup/graphql-kotlin/blob/master/generator/graphql-kotli[…]ql/generator/execution/FlowSubscriptionExecutionStrategyTest.kt), unsure what might be causing different behavior
j

Joe

04/08/2021, 7:41 PM
ok digging a little deeper on (2) it appears that the Instrumentation is being applied, but the coroutine context is not preserved between the raw flow and the converted publisher. Specifically we return a
flow.flowOn(MDCContext())
and then attempt to pick up the MDC value from our Instrumentation to make a
RequestId
accessible. Will try to create a simple reproduction now that I've figured that out.
👍 1
d

Dariusz Kuc

04/08/2021, 8:05 PM
ah I think it is most likely due to the
flow -> publisher
change if you use the default flow subscription strategy from gql kotlin
i.e. publisher has no concept of contextual data
j

Joe

04/08/2021, 9:58 PM
Yeah, for our specific case it looks like you could do
(publisherOrFlow as? Flow<Any>)?.asPublisher(MDCContext())
but I don't see a good way to do this generically (ie. inherit context that's already there in the calling context, since we've bridged into java/non-coroutine land at this point?). sticking with avoiding flow -> publisher -> flow conversion is the way we'll go.
After a little more thought put this together: https://github.com/ExpediaGroup/graphql-kotlin/pull/1116 not sure if you'd want it or not, but figured worth sharing some thoughts on it.
3 Views