Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a way to fluently convert an response with WebClient and add BlockingWebClient #4021

Merged
merged 20 commits into from Jan 27, 2022

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Jan 12, 2022

Motivation:

  • Improved usability
    • REST API uses a unary call(non-streaming request and response) to send
      and respond to a message.
      As WebClient supports streaming requests and responses by default,
      users need to manually aggregate the returned HttpResponse.
    • Testing code needs to wait for the result with .join() or .get()
      to assert the result.
      If we provide BlockingWebClient, users and dev can remove a lot of
      boilerplate code.
  • Possibility of performance optimization

Modifications:

  • Add ResponseAs<T, U> that defines a way to convert a response into another.
    • Decoders for byte[], String, JSON, File are provided by default.
  • Add TransformingRequestPreparation<T, R> that fluently transforms a returns response.
  • Add FutureTransformingRequestPreparation that is a specialized converter for CompletableFuture.
    • Scala's Future will be supported via ScalaResponseAs.
  • Add BlockingWebClient that returns AggregatedHttpResponse by default.
  • Add InvalidHttpResponseException for notifying a failure of JSON decoding.
  • Introduce {Request,Response}Entity<T> for express a decoded type of an HTTP content.
  • Migrate some tests code for proof of concept.

Result:

  • You can now fluently convert a response using WebClient.
    WebClient client = WebClient.of("api.example.com");
    CompletableFuture<ResponseEntity<MyObject>> response =
       client.prepare()
            .get("/v1/items/1")
            .asJson(MyObject.class)
            .execute();
  • You can now use BlockingWebClient to wait for a response to be completed.
    BlockingWebClient client = BlockingWebClient.of("https://api.example.com");
    ResponseEntity<MyObject> response =
        client.prepare()
              .get("/v1/items/1")
              .asJson(MyObject.class)
              .execute();

Motivation:

- Improve usability
  - REST API uses a unary call(non-streaming request and response) to send
    and respond a message.
    As `WebClient` supports streaming request and response by default,
    users need to manually aggregate the returnd `HttpResponse`.
  - Testing code needs to wait for the result with `.join()` or `.get()`.
    You can see them many places in test suites.
    If we provide `BlockingWebClient`, uesrs and dev can remove a lot of
    boilerplate code.
- Possibility of performance optimization
  - If a request and response type are known before sending a call, we
    can optimize the performance for non-streaming calls.

Modifications:

- TBU

Result:

- You can now fluently convert a response using `WebClient`.
  ```java
  WebClient client = WebClient.of("https://api.example.com");
  CompletableFuture<ResponseEntity<MyObject>> response =
     client.prepare()
          .get("/v1/items/1")
          .asJson(MyObject.class)
          .execute();
  ```
- You can now use `BlockingWebClient` to wait for a response to be completed.
  ```java
  BlockingWebClient client = WebClient.of("https://api.example.com").blocking();
  ResponseEntity<MyObject> response =
      client.prepare()
            .get("/v1/items/1")
            .asJson(MyObject.class)
            .execute();
  ```
@codecov
Copy link

codecov bot commented Jan 12, 2022

Codecov Report

Merging #4021 (f7dece4) into master (9e5c65a) will decrease coverage by 0.26%.
The diff coverage is 38.68%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4021      +/-   ##
============================================
- Coverage     73.40%   73.13%   -0.27%     
- Complexity    15931    16056     +125     
============================================
  Files          1386     1404      +18     
  Lines         60869    61433     +564     
  Branches       7715     7731      +16     
============================================
+ Hits          44680    44929     +249     
- Misses        12278    12585     +307     
- Partials       3911     3919       +8     
Impacted Files Coverage Δ
...inecorp/armeria/internal/consul/CatalogClient.java 0.00% <0.00%> (ø)
...main/java/com/linecorp/armeria/client/Clients.java 65.27% <ø> (ø)
...spring/web/reactive/ArmeriaClientConfigurator.java 100.00% <ø> (ø)
...pring/web/reactive/ArmeriaClientHttpConnector.java 79.31% <ø> (ø)
...armeria/client/TransformingRequestPreparation.java 6.25% <6.25%> (ø)
...ia/client/BlockingWebClientRequestPreparation.java 15.55% <15.55%> (ø)
...a/client/FutureTransformingRequestPreparation.java 28.07% <28.07%> (ø)
...inecorp/armeria/common/NoHttpContentException.java 28.57% <28.57%> (ø)
...com/linecorp/armeria/client/BlockingWebClient.java 29.62% <29.62%> (ø)
.../com/linecorp/armeria/client/DefaultWebClient.java 87.71% <33.33%> (-3.19%) ⬇️
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e5c65a...f7dece4. Read the comment docs.

@ikhoon ikhoon marked this pull request as ready for review January 17, 2022 14:28
@ikhoon ikhoon added this to the 1.14.0 milestone Jan 17, 2022
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the build passes. Great work, @ikhoon 🙇

@ikhoon
Copy link
Contributor Author

ikhoon commented Jan 24, 2022

LGTM once the build passes.

Oops... 😅

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nits.
Thanks and great job, @ikhoon!

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Left some minor nits and questions 🙏

…ntRequestPreparation.java

Co-authored-by: jrhee17 <guins_j@guins.org>
@jrhee17
Copy link
Contributor

jrhee17 commented Jan 26, 2022

Can you check the scala test compilation failure? 😅

@ikhoon
Copy link
Contributor Author

ikhoon commented Jan 26, 2022

Can you check the scala test compilation failure? 😅

😱😱😱

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ikhoon ! Looking forward to reviewing the exchangeType PR as well 🙇 👍 🥇

@trustin trustin merged commit ccf630c into line:master Jan 27, 2022
@ikhoon ikhoon deleted the web-client-response-type branch January 27, 2022 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants