ExpediaGroup / graphql-kotlin

Libraries for running GraphQL in Kotlin
https://opensource.expediagroup.com/graphql-kotlin/
Apache License 2.0
1.73k stars 345 forks source link

native subscription support for Kotlin Flow #358

Closed dariuszkuc closed 4 years ago

dariuszkuc commented 5 years ago

Is your feature request related to a problem? Please describe. As per graphql-java currently we only support subscription functions that return some Publisher (e.g. Reactor Flux). Since we support coroutines for queries/mutations we should also support Flow for subscriptions.

Describe the solution you'd like Support Kotlin Flow type for subscriptions.

Describe alternatives you've considered N/A

Additional context N/A

smyrick commented 5 years ago

@dariuszkuc graphql-java supports Publishers. https://www.graphql-java.com/documentation/v13/subscriptions/

Flow can map to Publisher with Flow.asPublisher.

https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/-flow/#

Do we want to have this mapping in the schema generator code or require schema developers to do this mapping for us?

dariuszkuc commented 5 years ago

@smyrick yes coroutines have nice interop with many different libs and definitely devs could do this conversion manually but the whole point of this issue is to support it natively. As a dev writing my graphql service I just want to be able to write it as

class MySubscription {
  fun foo(): Flow<Bar> { ... }
}

If possible we should do the mapping within the schema generator. Unsure if it will work though with current graphql-java model.

smyrick commented 5 years ago

Tested locally, It does not work with graphql-java unless we change the type to Publisher so we can do this in the generator but I am not sure if that is any better that just supporting Publisher and having devs do that

smyrick commented 5 years ago

@dariuszkuc Are we sure that we want to have this mapping in the library and not just support the types that graphql-java does (only publisher)

dariuszkuc commented 5 years ago

I think having native support would be great but yes until we move away from graphql-java this might not be feasible. Lets keep it open for now.

josephlbarnett commented 5 years ago

Would it make sense to have graphql-kotllin provide a class that extends SubscriptionExecutionStrategy to treat Flows similar to how it currently treats Publishers? I think doing so to only support Flows would be somewhat straightforward port of the class, but probably want to be able to support either Flow or Publisher from the same strategy, which may be a little less straighforward to figure out?

dariuszkuc commented 5 years ago

Would it make sense to have graphql-kotllin provide a class that extends SubscriptionExecutionStrategy to treat Flows similar to how it currently treats Publishers? I think doing so to only support Flows would be somewhat straightforward port of the class, but probably want to be able to support either Flow or Publisher from the same strategy, which may be a little less straighforward to figure out?

Sounds like a good idea. Since Flow can be easily converted to reactor Flux this can be definitely done in custom execution strategy. Ideally both Flow and Publisher should be handled from single strategy but if it is not possible then having separate strategies should be fine as well.

josephlbarnett commented 4 years ago

note that due to issues like https://github.com/Kotlin/kotlinx.coroutines/issues/1825 doing flow.asPublisher() in the schema function and then consuming the ExecutionResult's publisher .asFlow() can have unexpected side effects