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

my4-dev · 2022-09-04T10:25:07Z

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 Provide a way to automatically deserialize non-OK JSON response using WebClient and RestClient #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)

…ssue4382

… WebClient and RestClient

CLAassistant · 2022-09-04T10:25:13Z

All committers have signed the CLA.

my4-dev · 2022-09-07T13:35:09Z

I have read the CLA document and already signed it.

codecov · 2022-09-08T04:00:49Z

Codecov Report

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

❗ 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

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

minwoox · 2022-09-08T14:43:29Z

core/src/main/java/com/linecorp/armeria/client/ResponseAs.java

@@ -105,6 +105,22 @@ static <T> FutureResponseAs<ResponseEntity<T>> json(Class<? extends T> clazz) {
        return aggregateAndConvert(AggregatedResponseAs.json(clazz));
    }

+    @UnstableApi
+    static <T> FutureResponseAs<ResponseEntity<T>> json(Class<? extends T> clazz,
+                                                        HttpStatusPredicate predicate) {


Global comment:
We cannot expose HttpStatusPredicate and HttpStatusClassPredicate to the public API because it's package private classes.
Instead, could you add method variants that take HttpStatus, HttpStatusClass and Predicate<HttpStatus>, please?

I'm sorry, I was careless.
I'll address your comments quickly.
Thank you for your comments!

my4-dev · 2022-09-10T09:04:23Z

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

my4-dev · 2022-09-10T09:20:19Z

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

jrhee17 · 2022-09-14T14:29:30Z

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

ikhoon · 2022-09-15T03:30:42Z

core/src/main/java/com/linecorp/armeria/client/AggregatedResponseAs.java

@@ -41,26 +44,73 @@ static <T> ResponseAs<AggregatedHttpResponse, ResponseEntity<T>> json(Class<? ex
        return response -> newJsonResponseEntity(response, bytes -> JacksonUtil.readValue(bytes, clazz));
    }

+    static <T> ResponseAs<AggregatedHttpResponse, ResponseEntity<T>> json(Class<? extends T> clazz,
+                                                                          Predicate<HttpStatus> predicate) {


Global comment: Should we add ? super for the type parameter of Predicate?

Suggested change

Predicate<HttpStatus> predicate) {

Predicate<? super HttpStatus> predicate) {

ikhoon · 2022-09-15T04:04:22Z

core/src/main/java/com/linecorp/armeria/client/AggregatedResponseAs.java

@@ -41,26 +44,73 @@ static <T> ResponseAs<AggregatedHttpResponse, ResponseEntity<T>> json(Class<? ex
        return response -> newJsonResponseEntity(response, bytes -> JacksonUtil.readValue(bytes, clazz));


Should we also use HttpStatusClassPredicate for this?

private static final HttpStatusClassPredicate SUCCESS_PREDICATE = ...; ... return json(clazz, SUCCESS_PREDICATE);

ikhoon · 2022-09-15T04:09:46Z

core/src/main/java/com/linecorp/armeria/client/WebClientRequestPreparation.java

+    public <T> FutureTransformingRequestPreparation<ResponseEntity<T>> asJson(Class<? extends T> clazz,
+                                                                              HttpStatusClass httpStatusClass) {
+        requireNonNull(httpStatusClass, "httpStatusClass");
+        return asJson(clazz, new HttpStatusClassPredicate(httpStatusClass));


HttpStatusClass is an enum so we can statistically create HttpStatusClassPredicates.
How about adding a static factory method for HttpStatusClassPredicate?

// Returns a pre-populated object. HttpStatusClassPredicate.of(httpStatusClass)

We may apply a similar approach to HttpStatusPrediate

…ientRequestPreparation

my4-dev · 2022-09-24T13:20:18Z

I have addressed your comment @ikhoon .

ikhoon · 2022-09-26T07:40:02Z

core/src/main/java/com/linecorp/armeria/client/HttpStatusClassPredicates.java

+            new HttpStatusClassPredicates(HttpStatusClass.UNKNOWN);
+
+    static HttpStatusClassPredicates of(HttpStatusClass httpStatusClass) {
+        if (httpStatusClass == HttpStatusClass.INFORMATIONAL) {


nit: Should we use switch statement?

ikhoon · 2022-09-26T07:44:42Z

core/src/main/java/com/linecorp/armeria/client/AggregatedResponseAs.java

            AggregatedHttpResponse response) {
        return new InvalidHttpResponseException(
                response, "status: " + response.status() +
-                          " (expect: the success class (2xx). response: " + response, null);
+                          " is not expected by predicate method. response: " + response, null);


Should we add predicate to the error message. Some users might want to override toString() for debugging purposes instead of using lambda expressions.

Suggested change

" is not expected by predicate method. response: " + response, null);

" is not expected by predicate method. response: " + response ", predicate: " + predicate, null);

ikhoon · 2022-09-26T07:46:54Z

core/src/main/java/com/linecorp/armeria/client/HttpStatusPredicate.java

+    private static final Map<HttpStatus, HttpStatusPredicate> httpStatusPredicateMap;
+
+    static {
+        httpStatusPredicateMap = new HashMap<>();


We prefer to use immutable collections if possible. Should we use Guavas ImmutableMap.Builderto createhttpStatusPredicateMap`?

ikhoon · 2022-09-26T07:48:24Z

core/src/main/java/com/linecorp/armeria/client/HttpStatusPredicate.java

+    }
+
+    static HttpStatusPredicate of(HttpStatus httpStatus) {
+        return httpStatusPredicateMap.get(httpStatus);


A custom HttpStatus can have a status code larger than 1000. Could we create a new HttpStatusPredicate if httpStatusPredicateMap.get(httpStatus) returns null?

Oops, it's my mistake.
I'm going to revise this as static factory method returns a new HttpStatusPredicate object when a status code is larger than 1000.

ikhoon · 2022-09-26T08:05:16Z

core/src/main/java/com/linecorp/armeria/client/BlockingWebClientRequestPreparation.java

+    /**
+     * Deserializes the JSON response content into the specified non-container type
+     * using the default {@link ObjectMapper}.
+     * {@link HttpStatus} type argument specify what type of response is allowed.


nit:

Suggested change

* {@link HttpStatus} type argument specify what type of response is allowed.

* {@link HttpStatus} type argument specifies what type of response is allowed.

ikhoon · 2022-09-26T08:23:34Z

core/src/main/java/com/linecorp/armeria/client/BlockingWebClientRequestPreparation.java

+    /**
+     * Deserializes the JSON response content into the specified non-container type
+     * using the default {@link ObjectMapper}.
+     * {@link HttpStatusClass} type argument specify what type of response is allowed.


Suggested change

* {@link HttpStatusClass} type argument specify what type of response is allowed.

* {@link HttpStatusClass} type argument specifies what type of response is allowed.

ikhoon · 2022-09-26T08:36:18Z

core/src/main/java/com/linecorp/armeria/client/WebClientRequestPreparation.java

+    /**
+     * Deserializes the JSON content of the {@link HttpResponse} into the specified non-container type using
+     * the default {@link ObjectMapper}.
+     * {@link HttpStatus} type argument specify what type of response is allowed.


How about?

Suggested change

* {@link HttpStatus} type argument specify what type of response is allowed.

* Note that if the specified {@link HttpStatus} is different from the {@link HttpStatus}

* of the response, an {@link InvalidHttpResponseException} is raised.

ikhoon · 2022-09-26T08:39:00Z

core/src/test/java/com/linecorp/armeria/client/HttpStatusClassPredicatesTest.java

+class HttpStatusClassPredicatesTest {
+
+    @Test
+    public void httpStatusClassIsEqualToTestArgument() {


nit: Remove public from the methods?

ikhoon · 2022-09-26T08:39:49Z

core/src/test/java/com/linecorp/armeria/client/HttpStatusPredicateTest.java

+class HttpStatusPredicateTest {
+
+    @Test
+    public void httpStatusIsEqualToTestArgument() {


nit: Remove public from the methods?

ikhoon · 2022-09-27T07:20:25Z

core/src/main/java/com/linecorp/armeria/client/AggregatedResponseAs.java

+    private static final HttpStatusClassPredicates SUCCESS_PREDICATE
+            = HttpStatusClassPredicates.of(HttpStatusClass.SUCCESS);


style: Should we place = at the end of the first line?

Suggested change

private static final HttpStatusClassPredicates SUCCESS_PREDICATE

= HttpStatusClassPredicates.of(HttpStatusClass.SUCCESS);

private static final HttpStatusClassPredicates SUCCESS_PREDICATE =

HttpStatusClassPredicates.of(HttpStatusClass.SUCCESS);

my4-dev · 2022-10-02T08:51:02Z

I have addressed your all comments @ikhoon again.

my4-dev · 2023-01-26T13:56:39Z

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

…ssue4382

my4-dev · 2023-01-27T14:59:15Z

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

ikhoon · 2023-01-30T06:25:10Z

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 · 2023-02-03T02:30:57Z

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 · 2023-03-08T14:13:18Z

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

jrhee17 · 2023-04-12T13:47:22Z

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 · 2023-04-13T09:13:18Z

@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 · 2023-04-15T14:57:14Z

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 · 2023-05-23T14:10:28Z

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. aed5603

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 · 2023-06-11T05:48:48Z

@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 · 2023-06-14T02:51:44Z

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:

less intrusive to core APIs
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 · 2023-07-01T09:02:45Z

This PR was closed and the new PR was suggested blow.
#5002

my4-dev added 4 commits September 4, 2022 19:11

Add Predicate Class

695ea35

Merge branch 'issue4382' of https://github.com/my4-dev/armeria into i…

0f91dbc

…ssue4382

Add New Line at the end of HttpStatusClassPredicate

cf50abc

Provide a way to automatically deserialize non-OK JSON response using…

d4bb602

… WebClient and RestClient

my4-dev requested review from ikhoon, jrhee17, minwoox and trustin as code owners September 4, 2022 10:25

my4-dev changed the title ~~Issue4382~~ Provide a way to automatically deserialize non-OK JSON response using WebClient and RestClient Sep 4, 2022

my4-dev mentioned this pull request Sep 4, 2022

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

Closed

ikhoon added the new feature label Sep 5, 2022

minwoox reviewed Sep 8, 2022

View reviewed changes

Address the comments by @minwoox

9364498

my4-dev requested review from minwoox and removed request for trustin, ikhoon and jrhee17 September 10, 2022 09:14

ikhoon reviewed Sep 15, 2022

View reviewed changes

Address the comments by @ikhoon and additionally change BlockingWebCl…

0355e90

…ientRequestPreparation

ikhoon reviewed Sep 27, 2022

View reviewed changes

minwoox added this to the 1.21.0 milestone Sep 29, 2022

revise the HttpStatusPredicate static factory method and code comments

d20d004

add test cases to WebClientPreparationExchangeTypeTest

14ba13e

jrhee17 and others added 3 commits January 27, 2023 17:55

Merge branch 'master' of github.com:line/armeria into issue4382

2e347a9

fix style

d93a2a4

Merge branch 'issue4382' of https://github.com/my4-dev/armeria into i…

8016f67

…ssue4382

ikhoon modified the milestones: 1.22.0, 1.23.0 Feb 5, 2023

ikhoon modified the milestones: 1.23.0, 1.24.0 Mar 15, 2023

Merge branch 'main' of github.com:line/armeria into issue4382

746dd88

ikhoon modified the milestones: 1.24.0, 1.25.0 Jun 2, 2023

Merge branch 'main' into issue4382

25f27f9

trustin self-requested a review June 14, 2023 10:38

my4-dev mentioned this pull request Jul 1, 2023

Provide a way to automatically deserialize non-OK JSON response #5002

Merged

my4-dev closed this Jul 1, 2023

jrhee17 modified the milestones: 1.25.0, 1.26.0 Aug 20, 2023

minwoox removed this from the 1.26.0 milestone Oct 20, 2023

	Predicate<HttpStatus> predicate) {
	Predicate<? super HttpStatus> predicate) {

		@@ -41,26 +44,73 @@ static <T> ResponseAs<AggregatedHttpResponse, ResponseEntity<T>> json(Class<? ex
		return response -> newJsonResponseEntity(response, bytes -> JacksonUtil.readValue(bytes, clazz));

	" is not expected by predicate method. response: " + response, null);
	" is not expected by predicate method. response: " + response ", predicate: " + predicate, null);

	* {@link HttpStatus} type argument specify what type of response is allowed.
	* {@link HttpStatus} type argument specifies what type of response is allowed.

	* {@link HttpStatusClass} type argument specify what type of response is allowed.
	* {@link HttpStatusClass} type argument specifies what type of response is allowed.

	* {@link HttpStatus} type argument specify what type of response is allowed.
	* Note that if the specified {@link HttpStatus} is different from the {@link HttpStatus}
	* of the response, an {@link InvalidHttpResponseException} is raised.

		private static final HttpStatusClassPredicates SUCCESS_PREDICATE
		= HttpStatusClassPredicates.of(HttpStatusClass.SUCCESS);

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

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

Conversation

my4-dev commented Sep 4, 2022

CLAassistant commented Sep 4, 2022 • edited

my4-dev commented Sep 7, 2022

codecov bot commented Sep 8, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

my4-dev commented Sep 10, 2022

my4-dev commented Sep 10, 2022

jrhee17 commented Sep 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

my4-dev commented Sep 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

my4-dev commented Oct 2, 2022

my4-dev commented Jan 26, 2023

my4-dev commented Jan 27, 2023

ikhoon commented Jan 30, 2023

minwoox commented Feb 3, 2023

my4-dev commented Mar 8, 2023

jrhee17 commented Apr 12, 2023 • edited

jrhee17 commented Apr 13, 2023

my4-dev commented Apr 15, 2023

jrhee17 commented May 23, 2023

my4-dev commented Jun 11, 2023

jrhee17 commented Jun 14, 2023 • edited

my4-dev commented Jul 1, 2023

CLAassistant commented Sep 4, 2022 •

edited

codecov bot commented Sep 8, 2022 •

edited

jrhee17 commented Apr 12, 2023 •

edited

jrhee17 commented Jun 14, 2023 •

edited