Allow WebFlux ApiVersionResolver to return a Mono

[JKaplanEmpty-Nes]

The current WebFlux API resolution returned a nullable String, this prevented custom API resolvers to be based on values which would need to be blocked.
For example, creating an API version resolver based on the Authorized Principal. Since this is returned as a Mono in a server exchange.

[bclozel]

It is unfortunately too late for this type of change. This would break existing public contracts in a spectacular fashion. I will leave this open for now so we can discuss the use case.

[JKaplanEmpty-Nes]

It is unfortunately too late for this type of change. This would break existing public contracts in a spectacular fashion. I will leave this open for now so we can discuss the use case.

What if I changed the public contracts so that the old return was there, but added a new reactive one and by default it would enclose the old result in a mono?

[bclozel]

We can always consider solutions like that. Let us discuss this matter first.

[JKaplanEmpty-Nes]

We can always consider solutions like that. Let us discuss this matter first.

Absolutely I would love to discuss my use case, just in case I made the change to not break the previous contracts.

The specific case is that we set the version (alpha, beta, GA) based on the Authorized Principal of the request.
However using WebFlux we get that wrapped in a Mono, which we obviously can't block to resolve.

[rstoyanchev]

This was unfortunately an oversight, but we need to consider the best way to get there with the least disruption.

I do like the idea of allowing an ApiVersionResolver to be sync or async, so the resolveVersionReactively method with a default implementation is good, but I would call it resolveVersionAsync.

However, it doesn't make sense to keep both sync and async variants at the level of ApiVersionStrategy, so we should deprecate the sync version for removal, and again call the new method resolveVersionAsync. A similar pattern for resolveParseAndValidate.

During the deprecation phase, we do need to continue to call the sync resolveParseAndValidate first, suppressing any MissingApiVersionException, and then conditionally call the new async variant if the sync version did not produce a version.

Let me know if you plan to make those updates.

[JKaplanEmpty-Nes]

I pushed the following changes:
ApiVersionResolver.resolveVersionReactively renamed to resolveVersionAsync
ApiVersionStrategy.resolveVersionReactively renamed to resolveVersionAsync

Deprecated ApiVersionStrategy.resolveVersion since 7.0.3

Instead of defaulting the async resolver to sync resolver in the ApiVersionStrategy, both methods are being called when calling resolveParseAndValidate. If the sync method throws a MissingApiVersionException then it is suppressed.

[rstoyanchev]

@JKaplanEmpty-Nes, thinking further about the use case:

The specific case is that we set the version (alpha, beta, GA) based on the Authorized Principal of the request.

Could you clarify why the version depends on the logged in user? It seems to me the version to use should match the one requested by the client, which knows best what their capability and compatibility level is.

[JKaplanEmpty-Nes]

@JKaplanEmpty-Nes, thinking further about the use case:

The specific case is that we set the version (alpha, beta, GA) based on the Authorized Principal of the request.

Could you clarify why the version depends on the logged in user? It seems to me the version to use should match the one requested by the client, which knows best what their capability and compatibility level is.

Sure, I can go into a bit more detail. My use case is very specific, but allowing async API version resolution will solve for it and other possible cases.

We use a 3rd party service called LaunchDarkly. It allows us to select the correct version of an API based on a supplied context. I created a Launch Darkly API Version resolver for WebMvc just fine however I was unable to do it for WebFlux. The context we send to Launch Darkly includes information from the JWT.

This allows us to switch versions for specific segments of users and allows us to change the routing at any time.

[rstoyanchev]

Thanks for elaborating.

[rstoyanchev]

@JKaplanEmpty-Nes as I mentioned before, I'm already making changes locally. Please, don't push any more changes. All of those will not be included.

[JKaplanEmpty-Nes]

@JKaplanEmpty-Nes as I mentioned before, I'm already making changes locally. Please, don't push any more changes. All of those will not be included.

Sure, I misunderstood. I thought you said you were processing it locally not updating the code.

[rstoyanchev]

Yeah, processing as in processing the PR, not digesting it.

I've merged the changes and also evolved the solution a bit further. The main thing to point out are the changes in ApiVersionResolver, deprecating the sync method and making the async method the main one instead (for @FunctionalInterface), and creating an SyncApiVersionResolver extension. This is a better setup since otherwise if you implement the async method, the sync one remains without an obvious implementation. It also matches a pattern we've used elsewhere in WebFlux. This does break lambda implementations, but that's inevitable with the new setup after the deprecation phase, so it's better to absorb that now. In any case the expectation is for concrete implementations so I think this should have limited impact at this point in time early in the 7.0.x phase.

Hopefully the rest is self explanatory and makes sense, but feel free to comment.

[JKaplanEmpty-Nes]

Yeah, processing as in processing the PR, not digesting it.

I've merged the changes and also evolved the solution a bit further. The main thing to point out are the changes in ApiVersionResolver, deprecating the sync method and making the async method the main one instead (for @FunctionalInterface), and creating an SyncApiVersionResolver extension. This is a better setup since otherwise if you implement the async method, the sync one remains without an obvious implementation. It also matches a pattern we've used elsewhere in WebFlux. This does break lambda implementations, but that's inevitable with the new setup after the deprecation phase, so it's better to absorb that now. In any case the expectation is for concrete implementations so I think this should have limited impact at this point in time early in the 7.0.x phase.

Hopefully the rest is self explanatory and makes sense, but feel free to comment.

As long as we have an async resolver then I am absolutely delighted. My first commit also changed the defaults to the asynced version but I changed it to the synced version based on the first suggestion.

[rstoyanchev]

I've done one more iteration, restoring ApiVersionResolver to be exactly the way it was, and having AsyncApiVersionResolver as the sub-interface that can be implemented when necessary. In effect, the overall version resolution is still async first, but the sync resolver contract remains front and center as the more common way to resolve a version.