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

my4-dev · 2023-07-01T08:56:55Z

Motivation:

Please refer to #4382

Modifications:

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

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
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();

// RestClient
final ResponseEntity<MyResponse> res =
        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));

my4-dev · 2023-07-01T09:01:21Z

Thank you @jrhee17 !
I almost quoted from your poc and added some test cases.
I think this would be better than my previous PR.

I haven't completed some JavaDocs yet.
But I want you to review the code at first.
I will fix them after reviewing.

jrhee17

Left some minor comments, but the overall direction looks good to me 👍

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

jrhee17 · 2023-07-03T09:03:08Z

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

+import com.linecorp.armeria.common.HttpResponse;
+import com.linecorp.armeria.common.util.Exceptions;
+
+public class BlockingResponseAs implements ResponseAs<HttpResponse, AggregatedHttpResponse> {


Sorry about the misdirection, but what do you think of moving the methods here to overload methods in ResponseAs.java?

static <T> BlockingConditionalResponseAs<T> json(Class<? extends T> clazz, Predicate<AggregatedHttpResponse> predicate) { requireNonNull(clazz, "clazz"); return new BlockingConditionalResponseAs<>(ResponseAs.blocking(), AggregatedResponseAs.json(clazz), predicate); }

And then users can just do this

WebClient.of(server.httpUri()).prepare().get("/json_generic_server_error") .as(ResponseAs.json(new TypeReference<List<MyObject>>() {}, res -> res.status().isServerError())

By doing so, I don't think users have to type out blocking() and just chain conditions immediately

I think it's good because users can do the same thing more simply. Thanks!

…er for users to chain conditions more simply

…alse

jrhee17

Looks good so far! Left some minor comments 🙇

jrhee17 · 2023-07-07T05:25:36Z

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

-                    return true;
-                }
-            };
+    static final BlockingResponseAs BLOCKING = new BlockingResponseAs();


I think we can just remove BlockingResponseAs and revert this change now

jrhee17 · 2023-07-07T05:51:45Z

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

+                                                   AggregatedResponseAs.json(clazz, predicate),
+                                                   predicate);


I think it might be better to just use a predicate that always returns true in this case.
I think

We can't guarantee that the predicate will always return the same result for a given input

The predicate is executed twice, which may not be expected

Suggested change

AggregatedResponseAs.json(clazz, predicate),

predicate);

AggregatedResponseAs.json(clazz, unused -> true),

predicate);

I also think the unused -> true predicate can be a static singleton.
Ditto for the other variants

jrhee17 · 2023-07-07T05:59:44Z

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

+    }
+
+    @UnstableApi
+    static <V> BlockingConditionalResponseAs<V> andThenJson(


Suggested change

static <V> BlockingConditionalResponseAs<V> andThenJson(

static <V> BlockingConditionalResponseAs<V> json(

I was rather thinking we could name this json to minimize confusion:

ResponseAs.json(Object.class, res -> true) .andThenJson(Object.class, res -> true) .orElseJson(Object.class);

Users can use json as just same as before.
On the other hand, they also can use andThenJson in case they want to apply a mapping by several conditions like this.

ResponseAs.<MyResponse>andThenJson(MyMessage.class, res -> res.status().isServerError()) .andThenJson(MyMessage.class, res -> res.status().isClientError()) .orElseJson(MyMessage.class)

In current implementation, I couldn't intend to chain andThenJson after json.

Oh, I just meant that I think ResponseAs#andThenJson can be renamed to ResponseAs#json (this would mean ResponseAs#json would be overloaded depending on the predicate parameter).

The reasoning was that uses can type ResponseAs.json( and auto-complete suggests the API, which results in more exposure. Also, users don't have to remember ResponseAs.andThenJson() and can just type .json().
This is a minor suggestion though 😄

jrhee17 · 2023-07-07T06:21:28Z

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

+import com.linecorp.armeria.common.HttpStatus;
+import com.linecorp.armeria.common.HttpStatusClass;
+
+final class HttpStatusClassPredicates implements Predicate<HttpStatus> {


I wasn't sure how this predicate plays into the current APIs as nothing is public 😅
I'm also curious how the HttpStatus[Class]Predicates tie into the new APIs since. Can you show me an example?

I think the same too.
I'm not sure how to use HttpStatus[Class]Predicate in the new APIs.
It would be unnecessary so that I would remove this class.

jrhee17 · 2023-07-07T06:22:53Z

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

+     * {@link Predicate} is evaluated as true.
+     */
+    public ResponseAs<HttpResponse, ResponseEntity<V>> orElseJson(Class<? extends V> clazz) {
+        return orElse(AggregatedResponseAs.json(clazz, SUCCESS_PREDICATE));


Since we're already in a conditional, I think it's fine to just try and parse without checking if the response is successful or not. What do you think of just inputting an always true predicate?

Suggested change

return orElse(AggregatedResponseAs.json(clazz, SUCCESS_PREDICATE));

return orElse(AggregatedResponseAs.json(clazz,() -> true));

It makes sense more! Thanks!
Users might use orElse to map json response which isn't Success.

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

jrhee17 · 2023-07-07T06:23:20Z

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

+/**
+ * Adds the mapping of {@link ResponseAs} and {@link Predicate} in order to return {@link ResponseAs} whose
+ * {@link Predicate} is evaluated as true.
+ */


Suggested change

*/

@UnstableApi

jrhee17

I think this will be the last of my comments 👍 Can you check the lint failures as well?

jrhee17 · 2023-07-20T07:43:01Z

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

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


nit; I think it's fine to revert this method since:

The only way this exception is thrown if due to the status predicate (users can't set the predicate using our public API)

I think the predicate method will likely not be toString friendly, which doesn't provide much information.

jrhee17 · 2023-07-20T07:44:26Z

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

+        final ResponseEntity<List<MyObject>> ServerErrorResponseEntity =
+                WebClient.of(server.httpUri()).prepare().get("/json_generic_mapper_server_error")
+                         .as(ResponseAs.json(new TypeReference<List<MyObject>>() {}, mapper,
+                                                    res -> res.status().isServerError())


nit; do you mind re-indenting this file once?

jrhee17 · 2023-07-20T08:55:30Z

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

@@ -182,4 +222,8 @@ public boolean requiresAggregation() {
            }
        };
    }
+
+    default <V> ConditionalResponseAs<T, R, V> andThen(ResponseAs<R, V> responseAs, Predicate<R> predicate) {


Taking a second look, I think it's better if we don't expose the concrete class ConditionalResponseAs and expose an interface instead.

What do you think of:

Renaming ConditionalResponseAs to DefaultConditionalResponseAs

Modifying DefaultConditionalResponseAs to be package private

Create an interface ConditionalResponseAs and define andThen, orElse methods

Modify DefaultConditionalResponseAs to implement ConditionalResponseAs

Just curious.
In what situations do we consider renaming class to Default... and make an interface?

The motivation of my comment was to avoid making breaking changes if we wanted to change the implementation hierarchy.

my4-dev · 2023-07-21T00:30:28Z

@jrhee17 : Thank you for reviewing! I've applied your comments.

jrhee17

Thanks! Pushed a minor commit on documentation but looks good to me 👍 🙇 🚀

codecov · 2023-08-16T13:43:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.12%. Comparing base (b8eb810) to head (6f1cf3a).
Report is 136 commits behind head on main.

❗ Current head 6f1cf3a differs from pull request most recent head 146ba11. Consider uploading reports for the commit 146ba11 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5002      +/-   ##
============================================
+ Coverage     73.95%   74.12%   +0.17%     
- Complexity    20115    21030     +915     
============================================
  Files          1730     1820      +90     
  Lines         74161    77439    +3278     
  Branches       9465     9891     +426     
============================================
+ Hits          54847    57404    +2557     
- Misses        14837    15377     +540     
- Partials       4477     4658     +181

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

minwoox · 2023-10-13T10:24:31Z

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

+     * Adds a mapping such that the response content will be deserialized
+     * to the specified {@link Class} if the {@link Predicate} is satisfied.
+     */
+    public BlockingConditionalResponseAs<V> andThenJson(


I think we should rename this to orElseJson. andThen is used when both instances are used. However, if predicate returns false, then it isn't converted.
Let me send a patch to rename these methods and address comments.

Thanks.
Please send me your suggestion for these method's name.

Hi, @my4-dev! I made a few changes:

Do not block the inside of the converter

I realized that this chaining converts also can be used by the WebClient. We must not block the inside of the converters.

Rename andThenJson to orElseJson

andThen is used when both instances are applied. e.g.

armeria/core/src/main/java/com/linecorp/armeria/server/logging/AccessLogWriter.java

Lines 79 to 83 in ed16bf6

try {

AccessLogWriter.this.log(log);

} finally {

after.log(log);

}

We apply the converter only when the Predicate passes and if it passes, the latter one is not applied. So we should rename it to orElse instead.

Remove ConditionalResponseAs interface because we don't need it at the moment.

PTAL and let me know if there are changes that I need to revert. 😉

…ckingJson

github-actions · 2023-10-14T06:21:46Z

🔍 Build Scan® (commit: `146ba11`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/d6awoqg2i4gcy
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/hy7qeg64ncyby
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/ismb4iintrdg4
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/36eisq2obaemk
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/wjgtlzzf3avkk
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/r34fh6k2lahmi
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/uuoerfszrgnpg
build-macos-12-jdk-19	❌ (failure)	https://ge.armeria.dev/s/nxe7dccy7jwk4

jrhee17

Changes still look good 👍 👍 👍

ikhoon

Looks useful. Thanks, @my4-dev and @jrhee17!

my4-dev added 2 commits June 10, 2023 15:23

add the poc code of @jrhee17

476442a

Provide a way to automatically deserialize non-OK JSON response

30bc8aa

my4-dev requested review from ikhoon, jrhee17, minwoox and trustin as code owners July 1, 2023 08:56

my4-dev mentioned this pull request Jul 1, 2023

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

Closed

Merge branch 'main' into issue-4382-new

f6c8c80

jrhee17 reviewed Jul 3, 2023

View reviewed changes

my4-dev added 4 commits July 6, 2023 08:37

move andThenJson methods from BlockingResponseAs to ResponseAs in ord…

3182531

…er for users to chain conditions more simply

fix to throw InvalidHttpResponseException in case predicate.test is f…

a228219

…alse

fix to throw InvalidHttpResponseException in case predicate.test is f…

89bd77a

…alse

Merge branch 'main' into issue-4382-new

2b9c62c

jrhee17 reviewed Jul 7, 2023

View reviewed changes

my4-dev added 4 commits July 8, 2023 20:40

remove BlockingResponseAs and revert

a800fe9

apply comments

284d469

delete HttpStatus[Class]Predicate

e2ac43c

rename andThenJson to json in ResponseAs

256acf3

jrhee17 added this to the 1.25.0 milestone Jul 20, 2023

jrhee17 added the new feature label Jul 20, 2023

jrhee17 reviewed Jul 20, 2023

View reviewed changes

my4-dev added 3 commits July 21, 2023 08:12

revert and format

8f428cc

apply comments

532526a

add JavaDoc and header

f6e8c4a

my4-dev and others added 4 commits July 22, 2023 18:05

Merge branch 'main' into issue-4382-new

2b36e1e

Merge branch 'main' into issue-4382-new

7617569

fix failing test

c6617b8

minor documentation updates

9abae16

jrhee17 approved these changes Aug 14, 2023

View reviewed changes

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

lint

c2c0f48

minwoox modified the milestones: 1.26.0, 1.27.0 Oct 13, 2023

minwoox reviewed Oct 13, 2023

View reviewed changes

my4-dev added 3 commits October 14, 2023 13:32

Merge branch 'main' into issue-4382-new

89b15a2

apply minor comments

8be91f8

rename json methods which return BlockingConditionalResponseAs to blo…

37b809d

…ckingJson

my4-dev and others added 6 commits October 14, 2023 15:37

re-defined DefaultConditionalResponseAs as the public class

889f5c7

add method variants of json

ff99ed6

lint

a434b8c

Use orElseJson

c944bd3

Fix docs

10e7b45

More fix

a6660c8

minwoox modified the milestones: 1.27.0, 1.26.0 Oct 19, 2023

ikhoon modified the milestones: 1.27.0, 1.28.0 Jan 16, 2024

Merge branch 'main' into issue-4382-new

6f1cf3a

jrhee17 approved these changes Apr 8, 2024

View reviewed changes

ikhoon approved these changes Apr 9, 2024

View reviewed changes

Merge branch 'main' into issue-4382-new

146ba11

jrhee17 merged commit b4e1f58 into line:main Apr 11, 2024
15 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

my4-dev commented Jul 1, 2023

my4-dev commented Jul 1, 2023

jrhee17 left a comment

jrhee17 Jul 3, 2023

my4-dev Jul 5, 2023

jrhee17 left a comment

jrhee17 Jul 7, 2023

jrhee17 Jul 7, 2023

jrhee17 Jul 7, 2023

my4-dev Jul 8, 2023

jrhee17 Jul 10, 2023

jrhee17 Jul 7, 2023

my4-dev Jul 8, 2023

jrhee17 Jul 7, 2023

my4-dev Jul 8, 2023

jrhee17 Jul 7, 2023

jrhee17 left a comment

jrhee17 Jul 20, 2023

jrhee17 Jul 20, 2023

jrhee17 Jul 20, 2023

my4-dev Jul 20, 2023

jrhee17 Aug 14, 2023

my4-dev commented Jul 21, 2023

jrhee17 left a comment

codecov bot commented Aug 16, 2023 •

edited

minwoox Oct 13, 2023 •

edited

my4-dev Oct 14, 2023

minwoox Oct 17, 2023 •

edited

github-actions bot commented Oct 14, 2023 •

edited

jrhee17 left a comment

ikhoon left a comment

	static <V> BlockingConditionalResponseAs<V> andThenJson(
	static <V> BlockingConditionalResponseAs<V> json(

	return orElse(AggregatedResponseAs.json(clazz, SUCCESS_PREDICATE));
	return orElse(AggregatedResponseAs.json(clazz,() -> true));

	try {
	AccessLogWriter.this.log(log);
	} finally {
	after.log(log);
	}

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

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

Conversation

my4-dev commented Jul 1, 2023

my4-dev commented Jul 1, 2023

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a 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

Choose a reason for hiding this comment

jrhee17 left a 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 Jul 21, 2023

jrhee17 left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2023 • edited

Codecov Report

minwoox Oct 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Oct 17, 2023 • edited

Choose a reason for hiding this comment

github-actions bot commented Oct 14, 2023 • edited

🔍 Build Scan® (commit: 146ba11)

jrhee17 left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 16, 2023 •

edited

minwoox Oct 13, 2023 •

edited

minwoox Oct 17, 2023 •

edited

github-actions bot commented Oct 14, 2023 •

edited

🔍 Build Scan® (commit: `146ba11`)