Provide a way to automatically deserialize non-OK JSON response using WebClient and RestClient

[my4-dev]

Motivation:

Please refer to #4382

Modifications:

• Add HttpStatusPredicate and HttpStatusClassPredicate class to deserialize non-OK JSON response
• Add some methods to specify what type of response is allowed

Result:

• Closes #4382
• Users can deserialize non-OK JSON response like the following

// WebClient
CompletableFuture<ResponseEntity<MyObject>> response =
    client.prepare()
          .get("/v1/items/1")
          .asJson(MyObject.class, HttpStatus.INTERNAL_SERVER_ERROR)
          .execute();

// RestClient
CompletableFuture<ResponseEntity<MyObject>> response = 
    client.get("/v1/items/1")
          .execute(MyObject.class, HttpStatus.INTERNAL_SERVER_ERROR)

[CLAassistant]

All committers have signed the CLA.

[my4-dev]

I have read the CLA document and already signed it.

[codecov[bot]]

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@a88ca59). Click here to learn what that means. Patch has no changes to coverable lines.

:exclamation: Current head ff299ba differs from pull request most recent head 25f27f9. Consider uploading reports for the commit 25f27f9 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4412   +/-   ##
=======================================
  Coverage        ?   73.97%           
  Complexity      ?    18210           
=======================================
  Files           ?     1538           
  Lines           ?    67592           
  Branches        ?     8523           
=======================================
  Hits            ?    50000           
  Misses          ?    13524           
  Partials        ?     4068           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[my4-dev]

I addressed your comments, @minwoox . Please review again.

[my4-dev]

I'm sorry, I accidentally push re-request review btn.

[jrhee17]

Can you check the CLA message and ensure that the email for the commits are registered with github?

[my4-dev]

I have addressed your comment @ikhoon .

[my4-dev]

I have addressed your all comments @ikhoon again.

[my4-dev]

Thank you for your reviews! I have fixed code styles as you advised @ikhoon.

[my4-dev]

Hi @ikhoon ! Are you very busy for now? Please review my code when you have a empty time.

[my4-dev]

Hi, @ikhoon! I have added some test cases and satisfied coverages which is required to exceed. So, the workflow pipeline will be passed.

[my4-dev]

I'm sorry, @jrhee17. I have revised the build error.

[ikhoon]

Sorry for the late review. 😅 I was told that @minwoox has a different idea of the API design on this PR. He is busy dealing with some LINE internal work. Thanks for your patience.

[minwoox]

Sorry about the delay. 😅 @jrhee17 has a better idea so he will propose it in a couple of days. Thanks a lot for your patience. 🙇

[my4-dev]

Hi, @jrhee17 ! I want to hear about a better idea of the API design you have!

[jrhee17]

Hi @my4-dev , really sorry about the late comment 😅

I think one of the concerns that was pointed out is that it is difficult to specify multiple mappings.

For instance, it is possible that users want to apply a mapping in the following way:

• Status 400: ClientErrorMessage.class
• Status 500: ServerErrorMessage.class
• Status 200: NormalMessage.class

With the current setup, it would be difficult to satisfy this kind of requirement.

---

I would like to propose that we introduce a conditional at the ResponseAs level. (I'm using andThen here, but there may be a better naming)

ResponseAs.java

<V> ConditionalResponseAs<T, R, V> andThen(ResponseAs<R, V> responseAs, Predicate<R> predicate)

which would mean a ResponseAs is applied only if the accompanying predicate is satisifed.

Then we may chain conditions like the following:

WebClient.of().prepare()
     .as(ResponseAs.blocking()
                   .andThen(res -> "Unexpected server error", res -> res.status().isServerError())
                   .andThen(res -> "missing header", res -> !res.headers().contains("x-header"))
                   .orElse(AggregatedHttpObject::contentUtf8))
     .execute();

We can do something similar for the blocking counterpart, and introduce a BlockingResponseAs as well. BlockingResponseAs.java

<V> BlockingConditionalResponseAs<V> andThenJson(Class<? extends V> clazz, Predicate<AggregatedHttpResponse> predicate)

and apply json specific conditionals as well

WebClient.of()
         .prepare()
         .as(ResponseAs.blocking()
                       .<MyResponse>andThenJson(MyError.class, res -> res.status().isError())
                       .andThenJson(EmptyMessage.class, res -> res.status().isInformational())
                       .orElseJson(MyMessage.class))
         .execute();

Let me know if this doesn't make sense and sorry about the change in direction 😅

[jrhee17]

@ikhoon also gave the good idea that we may want to accept the predicate first when designing the API

<V> ConditionalResponseAs<T, R, V> andThen(Predicate<R> predicate, ResponseAs<R, V> responseAs)

[my4-dev]

Hi @jrhee17 , thank you for your suggestion! It gives me a new perspective. andThen method which is used like the following seems to be more versatile than asJson.

WebClient.of().prepare()
     .as(ResponseAs.blocking()
                   .andThen(res -> "Unexpected server error", res -> res.status().isServerError())
                   .andThen(res -> "missing header", res -> !res.headers().contains("x-header"))
                   .orElse(AggregatedHttpObject::contentUtf8))
     .execute();

But I think it would be difficult to specify a lambda function converting res.content into user defined clazz as a first argument of addThen. In this issue, I think it would be better to extend the method intended to convert response contents to user defined clazz like asJson to use predicate.

On the other hand, I believe that the following points you made should be addressed. I will try to figure out how to implement this.

I think one of the concerns that was pointed out is that it is difficult to specify multiple mappings.

For instance, it is possible that users want to apply a mapping in the following way:

Status 400: ClientErrorMessage.class Status 500: ServerErrorMessage.class Status 200: NormalMessage.class

If you have an opinion on the above, I would love to hear it👍

[jrhee17]

But I think it would be difficult to specify a lambda function converting res.content into user defined clazz as a first argument of addThen.

To supplement what I had in mind, this is a poc I had worked on when leaving the above review. https://github.com/line/armeria/commit/aed560361f042f8ff1b43b4dfb3164be83947774

I don't think this is necessarily the "correct" way, nor do I think the commit is refined enough for review, but this is just to give a basic idea of what I was thinking

In this issue, I think it would be better to extend the method intended to convert response contents to user defined clazz like asJson to use predicate.

I'm not sure I understood the intention of adding a predicate to the parameters of asJson() (am I understanding correctly? 😅 ). Can you give an example implentation of what you have in mind?

[my4-dev]

@jrhee17 : Thank you for sharing your poc! I could understand your idea and it would be better than before.

I have some questions and please share your opinion.

In your poc, we couldn't apply the mapping by HttpStatus in RestClient. The intention of this PR is to produce the mapping functions to users in only tests. So, not providing this functions in RestClient probably isn't a problem. What do you think? (If needed, I haven't come up with a way to implement this functions to RestClient at this time.)

What do you think the necessity of HttpStatusPredicate and HttpStatusClassPredicate? For me, it would be more convinient if we could use those classes.

[jrhee17]

In your poc, we couldn't apply the mapping by HttpStatus in RestClient.

I thought we could do something like the following:

final ResponseEntity<MyResponse> res2 =
        RestClient.of(server.httpUri()).get("/")
                  .execute(ResponseAs.blocking()
                                     .<MyResponse>andThenJson(MyError.class, res -> res.status().isClientError())
                                     .andThenJson(EmptyMessage.class, res -> res.status().isInformational())
                                     .orElseJson(MyMessage.class));

https://github.com/jrhee17/armeria/tree/poc/responseas-if-3

The intention of this PR is to produce the mapping functions to users in only tests. So, not providing this functions in RestClient probably isn't a problem. What do you think? (If needed, I haven't come up with a way to implement this functions to RestClient at this time.)

I see. I think the issue is the current PR introduces new APIs in the core APIs (RestClient, BlockingClient), which makes us more cautious (since we don't want to introduce breaking changes).

I'm fine with an alternative approach which is either:

1. less intrusive to core APIs
2. or provides a way to extend later to support multiple predicates

What do you think the necessity of HttpStatusPredicate and HttpStatusClassPredicate? For me, it would be more convinient if we could use those classes.

Sure, I think the predicates can be used for more shortcut methods.

[my4-dev]

This PR was closed and the new PR was suggested blow. https://github.com/line/armeria/pull/5002