Summary

ServerRequest.awaitBody* APIs have some problems:

  1. ServerRequest.awaitBody forces you to think about the coroutines-bridge exception handling, since it uses kotlinx.coroutines.reactive.awaitSingle. Also, it is not documented that it might throw NoSuchElementException and IllegalArgumentException for coroutines-related errors.

  2. ServerRequest.awaitBody might throw IllegalArgumentException for two different reasons, for serialization-related errors and for coroutines-related errors.

  3. ServerRequest.awaitBodyOrNull should never throw as the signature indicates. The convention established by the stdlib mandate that an operation with the name xxxOrNull returns null instead of throwing in case there is an error.

Proposal

Introduce APIs that address all the above.

Add ServerRequest.awaitReceiveNullable.

  • This API "abstracts" away the coroutine-bridging exception handling from the user
  • Throws only in case of serialization error
  • Returns null in case user is expecting the body to be "missing"

Add ServerRequest.awaitReceive.

  • This API "abstracts" away the coroutine-bridging exception handling from the user
  • Throws only in case of serialization error

Both APIs introduce a "mindset" that only serialization-wise can something go wrong, in other words, user input.

Extensions:

Based on these functions, the user can create their own function for a general catch-all and return null pattern

runCatching { awaitReceiveOrNull }.getOrNull()

Deprecations/Migrations

Deprecate ServerRequest.awaitBodyOrNull which can be replaced with ServerRequest.awaitReceiveNullable, as behavior wise they are pretty close.

Deprecate ServerRequest.awaitBody with not direct replacement, but promote the usage of ServerRequest.awaitReceive.

Closes gh-30926

Comment From: sdeleuze

Thanks for providing this PR, but I don't think we should merge the proposed changes.

ServerRequest.awaitBody forces you to think about the coroutines-bridge exception handling, since it uses kotlinx.coroutines.reactive.awaitSingle. Also, it is not documented that it might throw NoSuchElementException and IllegalArgumentException for coroutines-related errors.

ServerRequest.awaitBody might throw IllegalArgumentException for two different reasons, for serialization-related errors and for coroutines-related errors.

Please refer for this comment for related feedback about current APIs and related documentation and see https://github.com/spring-projects/spring-framework/issues/31189 follow-up issue.

ServerRequest.awaitBodyOrNull should never throw as the signature indicates. The convention established by the stdlib mandate that an operation with the name xxxOrNull returns null instead of throwing in case there is an error.

I don't think this is correct, Mono<T>.awaitSingleOrNull can (and should) throw/emit exception at codec level for example, the null value is used when for example there is no body, which is something different.

For those reasons, I decline this PR, but thanks for the discussion.

Comment From: GeorgePap-719

Hey, thanks for the feedback!

I don't think this is correct, Mono.awaitSingleOrNull can (and should) throw/emit exception at codec level for example, the null value is used when for example there is no body, which is something different.

I agree with this statement.

I tried to raise the issue about the naming in the function itself awaitBodyOrNull. As i see it, i found the problem kinda similar to the old Reactive<T>.awaitSingleOrNull , where it is deprecated for similar reasons, the function was named xxxOrNull but was throwing an exception for a specific error.

So in the same manner, imo, i expect a function named awaitBodyOrNull to return null in case of an error, nevertheless if it is at codec level or coroutines-related error.

Comment From: sdeleuze

As far as I know, Reactive<T>.awaitSingleOrNull was deprecated because there is no garantee to have a single element, and is replaced by Mono<T>.awaitSingleOrNull which keeps the same behavior is term of exception management. See #31127 related issue.

Comment From: GeorgePap-719

I was checking the deprecation section here: Deprecation note.

because there is no guarantee to have a single element

But that might be also the case. I cannot argue here since I'm not expert in coroutines.

Comment From: sdeleuze

Interesting, the deprecation may be partially misleading because as far as I can tell Publisher<T>.awaitFirstOrNull() has a documentation saying if this publisher has produced an error, throws the corresponding exception and that's the case for Mono<T>.awaitSingleOrNull as well. I will discuss that with the Kotlin team.

Comment From: GeorgePap-719

I'm quite interested as well in what the kotlin team has to say.

Nevertheless, I will drop my two cents just for the sake of the discussion:

From my understanding most coroutines operators propagate (and throw) all exceptions in the chain and, also might throw a CancelationException as well (but this is documented always I think). They are not trying to convey these kinds of exceptions in the signatures but exceptions the functions/operators themselves throw for their own semantic value.

Examples:

Publisher<T>.awaitSingle(): Propagates all exceptions and also throws CancellationException. The KDoc states that the function itself throws two additional exceptions for its own semantic value: NoSuchElementException if the publisher does not emit any value, and IllegalArgumentException if the publisher emits more than one value. Which can be translated that the function itself does some validations to ensure some semantics (emits only one value or throws the corresponding exception).

Publisher<T>.awaitSingleOrNull(): Propagates all exceptions and also throws CancellationException. The KDoc states that the function itself for its own semantics throws an IllegalArgumentException if the publisher emits more than one value or returns null in case there were non emitted. The problem here was that all xxxOrNull functions should return null in case of an error for their own semantic value, however here it is not the case.

Mono<T>.awaitSingleOrNull(): Propagates all exceptions and also throws CancellationException. The KDoc states that the function itself for its own semantic value returns null in case of an error, specifically: "If the Mono completed without a value, null is returned".

Publisher<T>.awaitFirstOrNull(): To avoid being repetitive, this function follows the same pattern as the other ones and in case of an error for its own semantic value returns null.

Comment From: sdeleuze

We got confirmation from the Kotlin team the wording from the deprecation notes is misleading.

Comment From: GeorgePap-719

I see, nevertheless, thanks for the discussion.