Add a requiredBody() extension to RestClient.ResponseSpec

[Donghh0221]

Description

In WebClientExtensions.kt, developers have the flexibility to handle responses with methods that either guarantee a non-null return type or can return null. This design caters to various nullability requirements, enhancing code safety and expressiveness. However, RestClientExtensionKt currently offers only a nullable return type method, which may lead to unnecessary null checks and potentially less idiomatic Kotlin code.

Given Kotlin's emphasis on type safety and explicit nullability, extending RestClientExtensionKt to include a non-nullable return type method would align with these principles, offering a more robust and intuitive toolset for REST client development.

Proposal

I propose the addition of a bodyNonNull method to RestClientExtensionKt:

inline fun <reified T : Any> RestClient.ResponseSpec.bodyNotNull(): T =
    body(object : ParameterizedTypeReference<T>() {}) ?: throw NoSuchElementException("Expected a non-null response body")

This method complements the existing nullable variant, providing a straightforward way to enforce non-null responses, thereby aligning with Kotlin's null safety features and reducing boilerplate code.

Impact and Use Cases

This enhancement introduces no breaking changes, enriching the API without affecting existing code. It particularly benefits scenarios where a non-null response is confidently expected, making error handling more explicit and idiomatic in Kotlin.

Seeking Feedback

I welcome feedback on this proposal, including alternative approaches, potential impacts on usability, and any concerns regarding implementation details.

Conclusion

By adopting this change, RestClientExtensionKt would not only align more closely with Kotlin's design principles but also offer developers a more expressive and safe way to handle REST responses. This proposal upholds the Kotlin ethos of safety and clarity, enhancing developer experience and code quality in REST client applications.

[sdeleuze]

Kotlin extensions try to stay as close as possible to the Java API, so I think I would not be in favor of a Kotlin specific API like the one proposed here.

@poutsma Could you please let me know what you think of the idea of introducing a non-nullable return value variant of ResponseSpec#body.

[poutsma]

I am not sure such a method is necessary in Java, where the use of null is more idiomatic than in Kotlin. For me, having a use case in Java is a requirement for adding any code on the Java side.

Instead, from a Java perspective, I think having a method that returns an Optional of the body would be much more useful, and more fitting within the functional API of RestClient. Using an Optional also gives the user the choice of returning a default value instead of throwing a NoSuchElementException, to filter out values, to map on them, etc.

Unless I am mistaken, such a optional-returning-method could then be used to implement the Kotlin extension described above, since I believe use of Optional is less idiomatic in Kotlin (?).

How does that sound, @sdeleuze?

[sdeleuze]

I think the use case here is "I know this endpoint is expected to return a body 99.99% of the time and I don't want any "noise" in my code like !! in Kotlin to turn a nullable return value to a non-nullable one or optional unwrapping", so I am in not in favor of introducing an optional variant which typically won't be used from Kotlin side to avoid doing unnecessary wrapping/unwrapping (we would instead just leverage the current nullable variant with !!).

So I am leaning toward declining this PR and just recommend to use body()!! or other null-safety constructs in Kotlin like ?: to handle that. It is pretty concise and give the right level of flexibility to the Kotlin developer. Any thoughts @Donghh0221?

[Donghh0221]

Thank you for your review, @sdeleuze @poutsma.

As you mentioned, using Optional as a return type does not seem to be a suitable way to solve our current issue.

Indeed, when calling the body method, it is typically presumed that a response body exists. Given this, I believe a method like bodyNotNull should be provided to handle responses without requiring additional !! or explicit exception throwing. Without such a method, developers must give extra consideration to which error should be thrown. Where a custom error is necessary, sticking with the existing body method would be more appropriate.

From a practical perspective, many teams in the existing MVC ecosystem have been using WebClient with runBlocking. These teams, which have been pragmatically handling nulls through WebClientExtensions.kt's awaitBody and awaitBodyOrNull, now need to reconsider whether to throw NoSuchElementException everywhere, or an NPE with !!, and change their error handling approach.

For these reasons, please reconsider adding an interface like bodyNotNull for Kotlin users.

[sdeleuze]

@Donghh0221 I propose that we use requiredBody() instead of bodyNotNull() which seems conceptually a better fit with the fact it will throw a NoSuchElementException, could you please update the PR accordingly?

[Donghh0221]

@sdeleuze Of course! I've added a commit to respond to your review.

[sdeleuze]

Polished and merged, thanks for your contribution.