web.reactive.function.server.ServerRequest.awaitBody() exception handling

[GeorgePap-719]

Affects: Spring v6.0.6

What is the recommended approach on handling ServerRequest.awaitBody()

Right now, the API is using under the hood awaitSingle() from kotlinx.coroutines.reactive, which is intentional from
the spring team. Now, the API is throwing either NoSuchElementException or IllegalArgmuentException depending on if
the publisher emits zero or more than one values.

Question: In general code, are we expected to catch them both? Although, the operator is being used on a Mono,
which technically can never throw IllegalArgmuentException?

On the other hand, since we always extract the body (thus we are not directly affected by how a client writes in body)
in either a Flux or Mono if the client emits more than one values, but we use ServerRequest.awaitBody() the body
is converted properly, and we are retrieving a collection. Though, this process breaks in serialization
(as expected).

Usage in general code

The most "blur" part is about throwing IllegalArgmuentException. That is because, when using kotlinx.serialization
for general "catch-all" around deserialization it is recommended to catch IllegalArgmuentException. In that case, we
have to know beforehand if we need to also check if IllegalArgmuentException is type of SerializationException
(so we can distinguish between them).

When I am writing code to handle ServerRequest.awaitBody(), I find it a one-way path to use
ServerRequest.awaitBodyOrNull() API to avoid all the above points.

My exception handling looks like this:

suspend inline fun <reified T : Any> ServerRequest.awaitAndRequireBody(): T {
    val body = try {
        awaitBodyOrNull<T>()
    } catch (e: IllegalArgumentException) { // serialization error
        throw IllegalArgumentException(bodyTypeErrorMessage<T>())
    }
    requireNotNull(body) { bodyTypeErrorMessage<T>() }
    return body
}

Which makes ServerRequest.awaitBody() kind of "obsolete" API (at least imho).

Proposal

I recommend to at least document about the fact that ServerRequest.awaitBody() throws exceptions which are expected to be caught from the user.

Also, happy to provide a pr with the changes.

[sdeleuze]

Thanks for the detailed issue.

IllegalArgumentException is only thrown if the provided context contains a Job instance which is a use case that should not happen when the Coroutine context is defined by Spring (if it does, please raise a related issue), so you can IMO safely assume that for the general use case, IllegalArgumentException won't be thrown for that reason.

Methods returning Mono will usually not throw IllegalArgumentException but can emit error signals with an IllegalArgumentException that will be thrown when exposed as a suspending function.

Kotlin extensions are not documenting every kind of unchecked exception than can be thrown like the related Java Reactive method that are leveraging, there can be a wide range of exception thrown depending on the codecs used, we can't really provide a useful documentation here. And if there is any change, it should be consistent between Java Reactive methods and Coroutines extensions.

That said, I agree that the fact that non-nullable extensions are throwing NoSuchElementException should be documented, I have created #31189 related issue.

So I will decline this Kotlin specific issue, and ping @poutsma in case he thinks we should refine the documentation in ServerRequest and ServerResponse in term of error signals (if we do, we should update the Coroutines extensions as well).