Add native subscription support for coroutine Flows #629
Conversation
...ator/src/main/kotlin/com/expediagroup/graphql/execution/FlowSubscriptionExecutionStrategy.kt
Show resolved
Hide resolved
| * that is a [Flow] instead of a [Publisher], and allows schema subscription functions | ||
| * to return either a [Flow] or a [Publisher]. | ||
| */ | ||
| class FlowSubscriptionExecutionStrategy(dfe: DataFetcherExceptionHandler) : ExecutionStrategy(dfe) { |
There was a problem hiding this comment.
I think you can make it much simpler -> extend CompletionStageMappingPublisher and then you only need to create createSourceEventStream that returns CompletableFuture<Publisher<Object>> (i.e. convert from Flow to Publisher). Unfortunately createSourceEventStream is private so you would also need to override the execute method but it would basically be copy and paste.
There was a problem hiding this comment.
Yeah, this class is mostly copy paste already. I think what you're suggesting would work for allowing schemas to specify Flow and then consuming a Publisher from the ExecutionResult's data. I think an execution strategy that did that could make sense for some cases. In our case we want to consume a Flow out of the ExecutionResult's data and avoid the Publisher conversion due to Kotlin/kotlinx.coroutines#1825
There was a problem hiding this comment.
@dariuszkuc What do you think about this implementation then? Do we want to have this copy paste execution strategy in the library or just provide more hooks for other developers to customize the supported return types?
There was a problem hiding this comment.
I'm leaning towards both - @josephlbarnett can you update FlowSubscriptionExecutionStrategy kdoc to indicate this is a copy of SubscriptionExecutionStrategy that adds support for Flow?
There was a problem hiding this comment.
👍 updated KDoc.
josephlbarnett
force-pushed
the
subscription-flow
branch
from
4955231
to
183b5d6
Compare
|
Also, if this ExecutionStrategy doesn't make sense for graphql-kotlin, would appreciate a way to implement it externally, either by the rest of the changes in this PR or some other/pluggable means. Right now I don't think the check in generateSubscription.kt is overridable, although I think the functionReturnTypes.kt diff can be implemented via schemaHooks.willResolveMonad() |
josephlbarnett
force-pushed
the
subscription-flow
branch
from
183b5d6
to
0cf059c
Compare
|
@josephlbarnett This code changes in this PR looks good to me, I think if you wanted to provide a custom subscription execution strategy you can just provide your own As far as checking the types, we can add more hooks that would allow you to do this without merging a execution strategy for the entire library. |
smyrick
added
changes: minor
josephlbarnett
force-pushed
the
subscription-flow
branch
4 times, most recently
from
7b732f9
to
d5269d6
Compare
Implement a SubscriptionExecutionStrategy that allows for `Flow`s and `Publisher`s to be returned from graphql schema elements, and can be processed as a `Flow` by subscription consumers. Relax restrictions that look for `Publisher`s to also allow `Flow`s. Fixes ExpediaGroup#358
josephlbarnett
force-pushed
the
subscription-flow
branch
from
d5269d6
to
c797d78
Compare
|
Rebased off master to use the new subscription return type hooks, thanks those look good! |
Implement a SubscriptionExecutionStrategy that allows for `Flow`s and `Publisher`s to be returned from graphql schema elements, and can be processed as a `Flow` by subscription consumers. Relax restrictions that look for `Publisher`s to also allow `Flow`s. Fixes ExpediaGroup#358
Implement a SubscriptionExecutionStrategy that allows
for
Flows andPublishers to be returned fromgraphql schema elements, and can be processed as a
Flowby subscription consumers. Relax restrictionsthat look for
Publishers to also allowFlows.Fixes #358