Use kotlin coroutine return types in query extensions instead of CompletableFuture & Optional

[matthewadams]

Enhancement Description

The current query extension methods return values using Java's CompletableFuture and Optional types. The usage of these types seems awkward to me in modern Kotlin that uses coroutines. This enhancement request is to encapsulate the use of Java types in favor of pure Kotlin idioms.

Current Behaviour

Note the use of Java types here, for example:

inline fun <reified R, reified Q> QueryGateway.queryOptional(query: Q): CompletableFuture<Optional<R>> {
    return this.query(query, ResponseTypes.optionalInstanceOf(R::class.java))
}

This requires the developer to adapt CompletableFuture to Kotlin's Deferred or Flow types (depending on single vs multiple response types), as well as adapting Optional to Kotlin's nullable type system.

Wanted Behaviour

I've been trying to create new extension methods that only expose Kotlin types.

Consider the following:

import kotlinx.coroutines.Deferred
import kotlinx.coroutines.future.asDeferred
import java.util.Optional
import java.util.concurrent.CompletableFuture

fun <T> Optional<T>.orNull(): T? = orElse(null)

fun <T> CompletableFuture<Optional<T>>.asDeferredOfNullable(): Deferred<T?> = thenApply { it.orNull() }.asDeferred()

Now, notice how I use them in the extension method queryNullableAsDeferred below, where Schedule is a Spring Data MongoDB persistent entity on the read side:

import kotlinx.coroutines.Deferred
import kotlinx.coroutines.future.asDeferred
import org.axonframework.commandhandling.gateway.CommandGateway
import org.axonframework.extensions.kotlin.queryOptional
import org.axonframework.queryhandling.QueryGateway
import org.springframework.stereotype.Component

// note use of Kotlin async types here

inline fun <reified R, reified Q> QueryGateway.queryNullableAsDeferred(q: Q): Deferred<R?> =
    queryOptional<R, Q>(q).asDeferredOfNullable()

@Component
class CqrsSchedulingService(val cgw: CommandGateway, val qgw: QueryGateway) {
    fun createSchedule(cmd: CreateScheduleCommand): Deferred<Unit> {
        return cgw.send<Unit>(cmd).asDeferred() // note conversion to Deferred here
    }

    fun updateSchedule(cmd: UpdateScheduleCommand): Deferred<Unit> {
        return cgw.send<Nothing>(cmd).asDeferred() // note conversion to Deferred here
    }

    fun findScheduleById(q: FindScheduleByIdQuery): Deferred<Schedule?> { // note use of Deferred and Kotlin's ? nullable type operator
        return qgw.queryNullableAsDeferred(q) // note use of extension method here
    }
}

I'm no Kotlin expert yet, so maybe this needs some fine-tuning, but this seems more natural to me. I think this issue could be expanded to other parts of the codebase as well (like CommandGateway.send(..), as they also return CompletableFuture.

Possible Workarounds

[smcvb]

It seems like a reasonable addition to me @matthewadams and straightforward enough too. So, a question for you then: Would you be up for making a pull request for this?

[matthewadams]

@smcvb I would, however, I consider myself a Kotlin noob right now. I'm coming along quickly. Perhaps I can enlist the help of the commenters on my SO question related to this issue. I've asked them if they'd be willing to help me out. We'll see what they say. Meantime, I'll fork and take a first stab.

[matthewadams]

Perhaps we should expand the scope of this issue to be the overhaul of the entire module to use current Kotlin idioms & conventions. What do you think, @smcvb? If so, I propose renaming the issue Enhance module to use current Kotlin idioms & conventions. Let me know.

[joffrey-bion]

I would be happy to review the PR or even help with the code. Don't hesitate to ping me!

[matthewadams]

Ok. I’ve already forked the repo. I’m a little busy, but I’ll take a crack at it by the middle of next week. If you want to fork and do it, that’s cool too. Just let me know so we’re not duplicating effort! 😊

On Wed, Jul 28, 2021 at 11:22 AM Joffrey Bion @.***> wrote:

I would be happy to review the PR or even help with the code. Don't hesitate to ping me!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AxonFramework/extension-kotlin/issues/139#issuecomment-888484439, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADN3IDXAF2BRH5GHQG45ZDT2A4FPANCNFSM5BCM7BUA .

[joffrey-bion]

@matthewadams Same here, a bit busy until next week. I'll let you know if I start anything before you ;)

[joffrey-bion]

@smcvb what are the current backwards compatibility constraints for these extensions? I see the version is 0.1.0 so I'm expecting it's ok to just change the signature directly from fun X(...): CompletableFuture<Y> to a suspend equivalent without deprecation cycle (and in particular without inventing new function names).

Also, is it ok for you if we bring in the dependency on kotlinx.coroutines? I guess this will be necessary for any user using a coroutine-based extension anyway, but still I feel this is worth asking. If this is a problem, I guess we will need to just replace Optional<T> with kotlin nullable types and leave CompletableFuture unchanged. Users will have to bring in the coroutines dependency on their own and use await() on the futures manually.

[smcvb]

As it stands, the Kotlin Extension is free to change its API. So, backward compatibility is not an issue yet. You are thus free to change the API however that fits the current Kotlin stance of the world.

Just for my understanding, though, any chance either of you guys could hook me up with a decent article/description on why we would move away entirely from the current format? Although I generally trust those with more knowledge on a subject, I'd rather not make a wrong decision entirely. Some write-up on whether we should keep one or the other, or both, would be valuable.

As far as the dependency goes, if it's experimental, I'd have to do some internal debate. I do believe it's recently seen its full release, though...again not certain here, however. Same question as earlier: any chance any of you could point me to some specifics that it's no longer experimental?

Apart from all of that, happy to see some people jumping on this! Any contributions are always much appreciated.

[smcvb]

Ah, I see it's already finalized since Kotlin 1.3, whilst we're on 1.5. So, go ahead and add the dependency @joffrey-bion.

For my understanding, though, all you'd be looking at is transforming the types, correct? Stating this as having Axon completely support co-routines internally is something we've been looking into but is easier said than done at the time.

[joffrey-bion]

@smcvb sorry for the delay, and sorry for the confusion. As you noticed, Kotlin coroutines are quite stable now, they have been for a while. They are still a "kotlinx" library, which means it's separate from Kotlin's stdlib.

There were basically 2 issues in one here:

1. moving from Optional to Kotlin nullables. This doesn't seem very controversial, as I believe most Kotlin users will prefer working with T? over Optional<T>
2. moving from a CompletableFuture API to Kotlin coroutines API (either using Deferred return type, which I find non-idiomatic, or moving to suspend functions, which I believe is more natural).

This second point is not necessary per se, but is a welcome change for coroutine users. That being said, after such change, the library becomes opinionated on using Kotlin coroutines, and the users of the lib will (sort of) have to use coroutines to call the lib (regardless of whether we use Deferred or suspend functions). The question about the dependency was rather whether you wanted to move away from Java's CompletableFuture to a coroutine-based API (which feels more natural in Kotlin, but is not absolutely necessary).

Note that if we keep CompletableFuture, all it takes for users of coroutines is a call to CompletableFuture.await() (provided in kotlinx-coroutines-jdk8) to make it a suspend call, as you can see in the stackoverflow answer.

For my understanding, though, all you'd be looking at is transforming the types, correct? Stating this as having Axon completely support co-routines internally is something we've been looking into but is easier said than done at the time.

I was talking about changing the methods to be suspend functions rather than returning Deferred, but yes it's essentially about changing the way the methods are exposed to the users, not about changing the internals of the library.

[matthewadams]

I don't see a reason to break backwards compatibility with 0.1.0. We could leave everything as it is, and simply add the suspend apis. I agree with @joffrey-bion about using suspend apis instead of Deferred/Flow. If we encounter some kind of conflict with the CompletableFuture & Optional stuff when implementing the suspend apis, only then would we need to break backward compatibility. We'll just see how it goes.

[matthewadams]

With regard to dependencies, I propose that we make the dependencies on the coroutine libraries be provided, and that this project only use test dependencies for the various coroutine libraries. @joffrey-bion It's confusing to me exactly which coroutine libraries this project should use for testing, and which ones to use as provided dependencies. Can you provide a recommendation here please?

[joffrey-bion]

I don't see a reason to break backwards compatibility with 0.1.0. We could leave everything as it is, and simply add the suspend apis.

@matthewadams I agree it is possible, but it doubles the API surface of the library and requires deciding on a naming convention. Since it's a 0.1.0 and it's ok, changing it instead saves some maintenance burden, but I agree keeping both could be interesting as well, I just don't like having 2 different names for the same thing. If we want to keep both APIs, what would you suggest as a naming convention for the future-based VS suspending equivalent? The usual convention would be suspend fun doSomething(): T and fun doSomethingAsync(): Deferred<T>. So here we could use doSomethingAsync() for the future-based version, but it will become different from the initial Java API (and still break backwards compat).

I agree with @joffrey-bion about using suspend apis instead of Deferred/Flow.

I mentioned using suspend functions instead of Deferred, but Flow has its place for asynchronous streams of results.

I propose that we make the dependencies on the coroutine libraries be provided

What exactly would be the benefit of using provided dependencies in this case? Since this library is distributed with maven, the dependencies are not embedded anyway so they don't take up any extra space.

It's confusing to me exactly which coroutine libraries this project should use for testing, and which ones to use as provided dependencies

kotlinx-coroutines-jdk8 for both I guess, since we'll need the CompletionStage.await extension. What are you hesitating with?

[joffrey-bion]

I'm trying to have a stab at it before the weekend, starting with Optional, which I guess can be handled as a separate PR (first step).

@smcvb I realized with great surprise by reading the tests that query is supposed to allow null results, which means its signature is wrong.

To illustrate my point, here is an example:

val queryResult: CompletableFuture<String> = gateway.query<String, ExampleQuery>(queryName, exampleQuery)

This return type is lying, because the string inside the completable future could be null if the underlying Java call returned a null inside the future.

Is there any way in the regular Java Axon framework to specify queries that forbid null values? More specifically, are there query methods that generate validation/runtime exceptions in case of null responses? From what I can see, if you specify ResponseTypes.instanceOf(someClass), you cannot be guaranteed non-null values. Is this correct?

If that is the case, then we should make the return types nullable for non-optional query methods, or we should fail with exceptions in case of nulls inside those methods.

One option is to define both query (which fails) and queryNullable (which returns nullable types). That's a backwards-incompatible change. What do you think @smcvb?

[matthewadams]

I don't see a reason to break backwards compatibility with 0.1.0. We could leave everything as it is, and simply add the suspend apis.

@matthewadams I agree it is possible, but it doubles the API surface of the library and requires deciding on a naming convention. Since it's a 0.1.0 and it's ok, changing it instead saves some maintenance burden, but I agree keeping both could be interesting as well, I just don't like having 2 different names for the same thing. If we want to keep both APIs, what would you suggest as a naming convention for the future-based VS suspending equivalent? The usual convention would be suspend fun doSomething(): T and fun doSomethingAsync(): Deferred<T>. So here we could use doSomethingAsync() for the future-based version, but it will become different from the initial Java API (and still break backwards compat).

Fair enough, @joffrey-bion. We can just stick with Kotlin idioms moving forward. Less to maintain.

I agree with @joffrey-bion about using suspend apis instead of Deferred/Flow.

I mentioned using suspend functions instead of Deferred, but Flow has its place for asynchronous streams of results.

I also agree.

I propose that we make the dependencies on the coroutine libraries be provided

What exactly would be the benefit of using provided dependencies in this case? Since this library is distributed with maven, the dependencies are not embedded anyway so they don't take up any extra space.

It's confusing to me exactly which coroutine libraries this project should use for testing, and which ones to use as provided dependencies

kotlinx-coroutines-jdk8 for both I guess, since we'll need the CompletionStage.await extension. What are you hesitating with?

Since kotlinx-coroutines-jdk8 doesn't have variants (multiplatform or native), then we can depend normally on that. I was thinking about the other coroutine dependencies, about which I'm admittedly confused because I'm a noob.

[matthewadams]

I'm trying to have a stab at it before the weekend, starting with Optional, which I guess can be handled as a separate PR (first step).

Sounds good. I'm kind of on vacation right now, anyway, so have at it, @joffrey-bion!

[sandjelkovic]

Hey, @joffrey-bion and @matthewadams thanks for taking this on! Maybe I can help clear out a few things as well.

In the case of Optional discussion, as you have already noticed, you can use both nullable and non-nullable references and types in regular query methods. So to add to the example: val queryResult: CompletableFuture<String> = gateway.query<String, ExampleQuery>(queryName, exampleQuery) will require a non-nullable String in your code after the query. If a null is returned by a Query Handler, Kotlin will blow up here.

However if you do expect nulls as a possible response from that query, you can use: val queryResult: CompletableFuture<String?> = gateway.query<String?, ExampleQuery>(queryName, exampleQuery) which will give you null if the query handler returned one.

In the end, it all boils down to requesting the proper type based on the Query handler's responses. We can have pairs of methods like orNull ones, and force the main ones to use non-nullable types. I'm just not sure if it adds enough value compared to doubling the API surface area to maintain.

Having the above points in mind, that is what you were trying to convert the Optional extensions to if I understand correctly? Isn't that already supported by regular *query methods? If you can use those extensions with a nullable reference instead of Optional ones and you'd get Kotlin idiomatic code.

Flow would only make sense in the case of Subscription queries to replace the Reactor stream. These extensions are not yet implemented though, there's an issue for it: https://github.com/AxonFramework/extension-kotlin/issues/17. None of the other query methods work with real streams, but only with Futures or single response values.

For Deferred and suspend there's a discussion on https://github.com/AxonFramework/extension-kotlin/pull/107 that might be helpful and give you a few ideas/hints. It also ties into properly supporting Flow and coroutines via Reactor gateway, but all of that is open for discussion.

About breaking backwards compatibility, as @smcvb already mentioned, the extension is still experimental and the API can freely change.

[joffrey-bion]

@sandjelkovic Thanks a lot for all these precisions and links, I will definitely take a look at the related issues!

In the case of Optional discussion, as you have already noticed, you can use both nullable and non-nullable references and types in regular query methods. So to add to the example:

val queryResult: CompletableFuture<String> = gateway.query<String, ExampleQuery>(queryName, exampleQuery)

will require a non-nullable String in your code after the query. If a null is returned by a Query Handler, Kotlin will blow up here.

@sandjelkovic No it won't blow up here. This is why I believe it's a problem. This is an unchecked cast, and will only fail when the value retrieved from the CompletableFuture is assigned to a variable with explicit type String, or passed to a method accepting a String. The following test passes:

@Test
fun `Query should handle nullable responses`() {
    val nullInstanceReturnValue: CompletableFuture<String?> = CompletableFuture.completedFuture(null)
    val nullableQueryGateway = mockk<QueryGateway> {
        every { query(queryName, exampleQuery, matchInstanceResponseType<String?>() ) } returns nullInstanceReturnValue
    }

    val queryResult = nullableQueryGateway.query<String, ExampleQuery>(queryName = queryName, query = exampleQuery)
    val result = queryResult.get() // the inferred type of result is String (non-nullable)
    assertSame(result, nullInstanceReturnValue.get()) // passes!
    assertEquals(nullInstanceReturnValue.get(), null)
    verify(exactly = 1) { nullableQueryGateway.query(queryName, exampleQuery, matchExpectedResponseType(String::class.java)) }
}

Specifying a non-nullable type doesn't add any checks inside the extension function, and nulls can still escape from Java. We don't have to double the API surface, though. If it's enough in your opinion to add a check depending on the generic type, we can take advantage of the reified generic and check if it's nullable (for instance with if (null is R)), and add a null check on the future result in that case.

In the end, it all boils down to requesting the proper type based on the Query handler's responses.

Yes, but if the developer expects a non-nullable value and null is returned by the handler, I believe it would be much better to fail right on the query call.

Having the above points in mind, that is what you were trying to convert the Optional extensions to if I understand correctly Isn't that already supported by regular *query methods? If you can use those extensions with a nullable reference instead of Optional ones and you'd get Kotlin idiomatic code.

Agreed, that's actually my question to @matthewadams. When you asked on stackoverflow for how to convert the Optional<T> types to nullables, were you aware that you could use the regular query methods with nullable types?

I'm just not sure if it adds enough value compared to doubling the API surface area to maintain.

I think this will depend on @matthewadams's answer. Having 2 APIs might help inform the user that there is a strict and a nullable API, and avoid confusion. Currently having query and queryOptional tends to suggest the former is strict, which is not the case.

Flow would only make sense in the case of Subscription queries to replace the Reactor stream.

Agreed. I guess to replace Java Streams in scatterGather methods, Kotlin's Sequence would be more appropriate than Flow, because they are synchronous as well.

[matthewadams]

Agreed, that's actually my question to @matthewadams. When you asked on stackoverflow for how to convert the Optional<T> types to nullables, were you aware that you could use the regular query methods with nullable types?

No, I guess I wasn't aware. I think I was viewing the Kotlin API as strict with regards to nullable types. query would only work with non-nullable types, and whatever we replace queryOptional with (queryNullable) would allow nullable types. I see how that would increase the number of methods, but it feels like better idiomatic Kotlin to me. Don't forget, though, that I'm still at noob status with Kotlin, so take that into account when considering my input, @joffrey-bion :)

[joffrey-bion]

I would personally also vote for having a separate method for querying nullable types. Here are my reasons:

1. Kotlin has a very strong and reliable type inference, so I rarely specify generic types. When I do, it's usually for disambiguation (to help the compiler). I have rarely seen a usage of reified generic type to drive the actual behaviour/validation inside the method, so I would find it quite surprising as I mentioned before (especially if we keep queryOptional). It could lead to strange bugs based on the types of variables that receive the value.
2. It would improve discoverability of the mechanism. If we keep queryOptional that returns Optional-based stuff, it really suggests (at least IMO) that query is strict. Adding a queryNullable version seems to really clarify the API (and self document it).
3. It would improve composability by allowing method references. If we drive the actual behaviour of the method via the type param (which again I find weird, as described in the first point), we can't really construct a reliable method reference for query because of KT-12140.

To give a bit more substance to reason 1, one puzzling scenario would be the following:

• I have a local var or property of nullable type
• I want to assign to it the result of a query call
• I'm using null to represent the uninitialized state, but I'm not using null as a valid result for query, so I want the strict non-nullable query call

class State

class Test(val gateway: QueryGateway) {
    private var someState: State? = null

    suspend fun initializeSomething() {
        // this assumes query() is now a suspend function returning the result directly (not wrapped in CompletableFuture)
        // the type is inferred from the type of someState
        someState = gateway.query("MyQueryName", query = "query")
    }
}

Here it would really not be obvious why it makes any difference to specify types explicitly. With 2 methods, it's clear. With one method driven by type parameters, it would lead to very strange situations (almost undetectable). If someone with more experience comes across this code, they may replace someState with a lateinit var of non-nullable type, and this will unexpectedly change the behaviour of query.

@sandjelkovic / @smcvb what do you think?

Another question, do you believe it's useful to keep the Optional-based extensions in Kotlin? I guess most people would use the nullable versions for optional values, unless we can actually differentiate between absent value and present null value (which I don't believe the underlying Java implementation can do). Not having the convenient extensions wouldn't actually prevent users from using Optional (they can still do it by calling the regular Java API).

[joffrey-bion]

@sandjelkovic please let me backtrack on what I said about Flow and Sequence in my initial response.

None of the other query methods work with real streams, but only with Futures or single response values.

What about scatterGather methods? They return Java Streams that are internally blocking on a bunch of futures. I guess it could make sense to use Kotlin's Sequence instead of Flow here, since the Java stream is blocking on the futures behind the scenes, but using Flow on Dispatchers.IO would also be really nice and hide the problem from users. Later optimizations could actually happen behind the scenes by using true asynchronous handling of these things. My main issue is that the errors are swallowed inside the query bus implementation.

[matthewadams]

Thinking about @joffrey-bion's comments above, I really think it would make the interface much more obvious IMHO with regard to nullability & multiplicity if we refactored to the following (plus their overloads, of course):

• suspend fun <T> QueryGateway.querySingle(q: Query): T: returns a single, nonnullable T
• suspend fun <T> QueryGateway.queryNullable(q: Query): T?: returns a single, nullable T
• fun <T> QueryGateway.queryMultiple(q: Query): Flow<T>: returns a Flow of T, using @joffrey-bion's awesome suggestion of "using Flow on Dispatchers.IO would also be really nice and hide the problem from users"

Lastly, I feel like Optional should be removed entirely from the Kotlin API, given Kotlin's compile-time nullability features. This would result in the complete removal of the methods named just query.

@joffrey-bion: Is my omission of suspend from queryMultiple above correct?

[joffrey-bion]

@matthewadams it is correct to not use the suspend keyword for functions that return a Flow most of the time (when representing cold flows).

However, my suggestion of using Flow does not apply to queryMultiple but to scatterGather (and overloads). I believe queryMultiple (or queryMany as it's named now) just returns a collection of elements, but doesn't stream anything, so we should honor that by also just returning a collection (and using suspend).

Regarding naming, I am actually fine with the current query and queryMany, but querySingle and queryMultiple look ok too. I don't have a strong opinion on this. I'm not entirely convinced by queryNullable to be honest, but I don't have a much nicer alternative. We could actually keep the name queryOptional which still conveys that the value can be absent even if we don't return an actual Optional type. The Kotlin stdlib tends to add the suffix -OrNull for non-failing methods that return null, but querySingleOrNull would be a bit too verbose here IMO. I think it would be nice to have the opinion of the maintainers here.

[matthewadams]

@joffrey-bion:

Could we give the illusion of asynchrony by having queryMultiple return a Flow<T>, similar to your suggestion with scatterGather, then change the implementation later? I really am grooving on your suggestion, as it gives a really nice experience for a Kotlin user, and gives the API a direction to head to in the future. Please advise if I'm missing something.

How does the notion of hot v. cold Flow affect this API? Is it that, for a hot Flow, the implementation would call a terminal method to start the flow, whereas for a cold Flow, the API client would call the terminal method to activate it?

[joffrey-bion]

Could we give the illusion of asynchrony by having queryMultiple return a Flow, similar to your suggestion with scatterGather, then change the implementation later?

I think it really depends on the contract of the method. If the behaviour is really to suspend and then get all elements at once (like an API call that gets a collection), then I don't believe there is any reason to use a Flow to represent it. It's really just a suspend function waiting for a collection. If elements were streamed one by one from the server, things would be different. I don't believe this semantics is going to change, that's why for that case I don't think Flow would be appropriate.

How does the notion of hot v. cold Flow affect this API?

Hot vs cold flow depends on the source of the data. Returning a cold flow means that nothing happens as long as no-one applies a terminal operation on the flow (basically as long as no-one collects the flow). Returning a hot flow means that something is going on regardless of whether a collector is collecting the flow or not. It's the case for instance if you have an already open websocket connection and you return a flow of events: events are coming regardless of collectors. In both cases, it's still always the client that calls terminal operators, but the source of the flow behaves differently.

[sandjelkovic]

@sandjelkovic No it won't blow up here. This is why I believe it's a problem. This is an unchecked cast, and will only fail when the value retrieved from the CompletableFuture is assigned to a variable with explicit type String, or passed to a method accepting a String.

You are correct, I didn't make myself clear enough. Yes, it will blow up when the value is received from the server. However, before the value is received there is no way to know if that's going to be null or not. To really support non-nullable types on that level, the core framework components for query handling must differentiate between the two with some sort of mechanism. Right now I'm not sure if there is a difference in compiled JVM bytecode between nullable and non-nullable types. Even if we require non-nullable generic, I don't see a way how to assert non-nullability on the result before it is retrieved from the server in the completable future. If you have an idea on how to do it, sure, I'm all for type safety.

I'm using null to represent the uninitialized state, but I'm not using null as a valid result for query, so I want the strict non-nullable query call

I would actually expect the return type to be nullable if it's assigned to a nullable value unless a non-nullable one is specifically requested. Keep in mind that the Kotlin stdlib, at least the collections, also allow for

val list: List<String?> = listOf<String?>()

and

val first: String? = list.first()

Would also get you a nullable reference even though there is firstOrNull. In the collection's case, the -orNull methods are used to indicate that the value might not be present or calculable.

Regarding naming, I am actually fine with the current query and queryMany, but querySingle and queryMultiple look ok too.

To give some perspective, querySingle/queryMultiple were the first version names, which changed to query/queryMany for both simplicity and to match Reactor extension's gateway which uses Reactor naming.

Agreed. I guess to replace Java Streams in scatterGather methods, Kotlin's Sequence would be more appropriate than Flow, because they are synchronous as well.

Could we give the illusion of asynchrony by having queryMultiple return a Flow, similar to your suggestion with scatterGather, then change the implementation later?

Honestly, Flow will just make it more confusing for the users. scatterGather is already blocking, and I agree, Kotlin's Sequence is the way to go here. Subscription query updates and the initial result are a different story, they are actually pushed from the server to the client and there it makes perfect sense to use Flow instead of Flux.

There are 2 very important differences between Sequence/Stream(Java8) and Flow/Flux

• In the case of Sequence/Stream all the data is already present in the memory (or be generated right away) and there is no waiting for updates from somewhere. These are intended to be used as a collection processing mechanism. Flow/Flux expect that not all data is present right away and will be pushed when available. They are intended for true Stream processing, which can be cancelled, completed, and flow controlled.
• Future<Sequence>/Future<Stream> are eagerly invoked, as opposed to Flow/Flux which are lazily invoked on subscribe. This can be a huge difference in some cases as you expect just a cold pipeline from a method returning Flow/Flux, which will be only triggered (in this case query sent) when you subscribe. When you see a Future/Deferred you expect the request to have already been sent and you are just waiting for the result.

Lastly, I feel like Optional should be removed entirely from the Kotlin API, given Kotlin's compile-time nullability features. This would result in the complete removal of the methods named just query.

I don't see why would these need to be removed, they provide utility to users by bypassing the class parameter with a generic. Maybe not all Kotlin users will use it, sure, but it's not hurting anyone and it's still a utility if you use Java code in the codebase.

I would also like to mention that the discussion is starting to blur two APIs and concepts together. These extension methods are intended to make the original Gateway as much Kotlin and user friendly as possible and are usually simple utilities. As such they are always going to be limited by the underlying Gateway interface and what they can do to wrap those regular Java friendly method calls. So while Java's Optional might not be originally present in the Kotlin-only API, those methods are already present on the base interface. Well, the ResponseTypes are anyway.

For a true Kotlin asynchronous API these extensions on the regular Gateway probably won't work. I say probably as there might be a good way to integrate those API, but either extensions to ReactorQueryGateway or a new interface based on coroutines suspending functions and types make more sense for true asynchronous API.

[joffrey-bion]

@sandjelkovic thanks again for taking the time to respond. Please let me address the nullability typing problem, as it's sort-of independent from the Optional/coroutines discussion.

The problem

Yes, it will blow up when the value is received from the server

No it won't blow up when we receive the value from the server (at least not with the current code), and it will not even blow up when we access the value from the completable future. People can call .get() and get a null from a CompletableFuture<String>, which is unexpected and breaks the type system (this is only possible because of Java interop). This is what the test I provided shows, and that is why I think it's a problem to declare CompletableFuture<R> (with possibly non-nullable R) as return type with the current code.

The only moment it fails is when we assign the result of .get() to a variable with explicit non-nullable type, or if we pass this value to a function that accepts a non-nullable type. IMO this is too late and we can do better by failing on get().

// test gateway returning null inside the completed future despite the non-null R==String
val queryResult = gateway.query<String, ExampleQuery>(queryName = queryName, query = exampleQuery)
val result = queryResult.get() // doesn't fail, the inferred type of result is String (non-nullable) but holds a null
val result2: String = queryResult.get() // correctly fails, but users are unlikely to declare types explicitly
someFunExpectingNonNullString(result) // correctly fails, but that's too late

The solutions

Even if we require non-nullable generic, I don't see a way how to assert non-nullability on the result before it is retrieved from the server in the completable future.

I'm sorry if it sounded like I was suggesting this was possible. You're right, we can't have an error before we get the result from the server of course, just like you won't get a compile-time error when calling list.first() on an empty list, because you only know at runtime. However, we can make the future fail once we know it contains a null, just like first() fails on empty collections.

What I was suggesting is to add .thenApply { it ?: error("Expected non-null value in query '$queryName', but received null") } before returning the future in query. This means people calling .get() on the returned future will never get a null, they will get an exception, which is what we want in order to respect the return type CompletableFuture<R>.

My previous messages might have been unclear, so please let me clarify the 2 solutions I had talked about with actual code (leaving aside the whole coroutine stuff for now), and the 3rd solution just for completeness.

Solution 1 (with 2 separate methods):

// strict query method with non-nullable generic type param, and non-nullable `R` in the returned future
inline fun <reified R : Any, reified Q> QueryGateway.query(queryName: String, query: Q): CompletableFuture<R> {
    return query(queryName, query, ResponseTypes.instanceOf(R::class.java))
        .thenApply { it ?: error("Expected non-null value in query '$queryName'") }
}

// lenient query method (possibly with a better name) that accepts nullable type param and returns nullable `R`
inline fun <reified R, reified Q> QueryGateway.queryNullable(queryName: String, query: Q): CompletableFuture<R?> {
    return query(queryName, query, ResponseTypes.instanceOf(R::class.java))
}

(Note that if the strict query function were converted to a suspend function, it would even fail right on the call site, just like calling list.first() on an empty list, which would be nice. This is why I was mentioning "failing on the query call", but that didn't mean it fails before receiving the server response.)

Solution 2 (all-in-one):

inline fun <reified R, reified Q> QueryGateway.query(queryName: String, query: Q): CompletableFuture<R> {
    val result = query(queryName, query, ResponseTypes.instanceOf(R::class.java))
    return if (null is R) result else result.thenApply { it ?: error("Expected non-null value in query '$queryName'") }
}

This second solution would be strict if the type parameter is non-null, but allow nulls if the type parameter allows it. In a previous message, I detailed why I would prefer 2 separate methods (as a user) instead of this "combo" one.

Solution 3 (just fix the type):

// exactly the same as the current query method that accepts nullable type param but properly returns nullable R?
// no matter the nullability of the type parameter R
inline fun <reified R, reified Q> QueryGateway.query(queryName: String, query: Q): CompletableFuture<R?> {
    return query(queryName, query, ResponseTypes.instanceOf(R::class.java))
}

If nothing else is changed, at least this has to be done in order to match the current code, because calling .get() on this future can definitely return null depending on what the server returns, regardless of the nullability of the type parameter. Of course this leads to pretty bad UX when the user is sure that s/he won't get a null here. It's better to propose a non-nullable variant IMO.

About why I find solution 2 undesirable

I would actually expect the return type to be nullable if it's assigned to a nullable value unless a non-nullable one is specifically requested.

Note that while val first: String? = list.first() fails right here, val result: String? = query<String>(...).get() does NOT fail (with the current code), even though we explicitly specify non-nullable String here. This would be fixed by solution 1 and 2. My point was simply that it's a bad idea to rely on type parameters nullability (solution 2) because we don't know where the nullability comes from in the type inference and maybe it's not used for what we think it is. I find it better to let the users express their intent on strict-ness via the appropriate method choice (like calling first() vs firstOrNull), and let them use type inference as much as they please.

Keep in mind that the Kotlin stdlib, at least the collections, also allow for val list: List<String?> = listOf<String?>() and then val first: String? = list.first()

I'm not sure what you're getting at here. The list is properly declared as List<String?>, so we know at compile time that it can contain null elements. In our case, CompletableFuture<String> is incorrectly declaring that it cannot contain nulls, while a call to get() will surprise the user. But I think this is besides the point, because the "unknown runtime behaviour" of first is about the emptiness of the list, not the nullability of the elements. first() will never return successfully if there is no elements in the list, but it can definitely return null if there is a null element inside.

The example I had constructed here was an argument against solution 2, and in favor of solution 1. Note that the first/firstOrNull duo is actually an example of solution 1: a strict and a lenient version of the method, the behaviour of which doesn't depend on type parameters.

A closer analogy here would be the following: if the stdlib was designed with solution 2, we would only have a single first() method that would, in case of empty list, return null if the type parameter is nullable or fail if the type parameter is not nullable (the example is still not perfect because the receiver list already provides a type parameter, but bear with me). So, val first: String? = list.first() would behave like firstOrNull, while val first: String = list.first() would behave like first(). This would be quite weird to deal with.

---

I'm really sorry for the long message, I just wanted to be very clear about what I meant in all previous messages.

[joffrey-bion]

Apart from the problem discussed above, which I'm thinking I should open as a separate issue, I agree with @sandjelkovic on almost everything else.

Lastly, I feel like Optional should be removed entirely from the Kotlin API, given Kotlin's compile-time nullability features. This would result in the complete removal of the methods named just query.

I don't see why would these need to be removed, they provide utility to users by bypassing the class parameter with a generic. > Maybe not all Kotlin users will use it, sure, but it's not hurting anyone and it's still a utility if you use Java code in the codebase.

I don't have a strong opinion against Optional-based variants. The possible reasons to remove them would be:

1. to free up the queryOptional name for the nullable variant described in solution 1 above, but that's ok if we're ok with another name
2. to save some maintenance burden given the low chance of it being useful

These are pretty weak arguments to be honest, and these extensions could indeed be useful to some users using mixed Java/Kotlin projects. So honestly, no strong opinion from me 😄 We can keep them.

For a true Kotlin asynchronous API these extensions on the regular Gateway probably won't work. I say probably as there might be a good way to integrate those API, but either extensions to ReactorQueryGateway or a new interface based on coroutines suspending functions and types make more sense for true asynchronous API.

It's very easy to adapt future-based APIs to coroutines by just using CompletionStage.await() from kotlinx-coroutines-jdk8, so the extensions would definitely work fine as suspend functions. That being said, it's also very easy to do for users that want to use coroutines, so honestly it's very much OK to keep these extensions coroutines-agnostic by still returning futures.