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

Add RestClient for Java, Kotlin Coroutine and Scala #4263

Merged
merged 15 commits into from
Jul 1, 2022

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented May 20, 2022

Motivation:

WebClient and WebClientPreparation has powerful and fluent features.
RESTful API is classic but it is still the dominant protocol for sharing
web resources.
The WebClient is able to handle not only REST responses but also
a stream response and a file response.
The API design of WebClient, in turn, is not optimized for REST API.
See #4250

Modifications:

  • Add RestClient for exchange RESTful APIs.
    • It assume that the response has a JSON body.
  • Add RestClientBuilder so as to fluently build a RestClient.
  • Refactor the intefaces for request preparations to accommodate
    RestClientPreparation.
    • Method and path setters are extracted to RequestMethodSetters.
    • Query and path param setters are moved to PathAndQueryParamSetters
    • Added HttpMessageSetters for setting HttpMessage properties.
  • Add execute() extension methods for Kotlin Coroutine.
  • Add ScalaRestClient for Scala.
    • Tried to add extension methods using an implicit class but Scala
      compiler failed to find the overloaded method which has a generic
      parameter.
  • Provide a extension method to convert a WebClient to
    ScalaRestClient.
  • Add restClient() to ServerExtension and ServerRule.

Result:

You can now easily send and receive RESTful APIs using RestClient

  • Java
    RestClient restClient = RestClient.of("...");
    CompletableFuture<ResponseEntity<Customer>> response =
        restClient.get("/api/v1/customers/{customerId}")
                  .pathParam("customerId", "0000001")
                  .execute(Customer.class);
  • Kotlin
    val restClient: RestClient = RestClient.of("...");
    val response: ResponseEntity<Customer> =
       restClient
         .get("/api/v1/customers/{customerId}")
         .pathParam("customerId", "0000001")
         .execute<Customer>()  // a suspend function
  • Scala
    val restClient: ScalaRestClient = ScalaRestClient("...")
    val response: Future[ResponseEntity[Result]] =
      restClient.post("/api/v1/customers")
                .contentJson(new Customer(...))
                .execute[Result]()

Motivation:

`WebClient` and `WebClientPreparation` has powerful and fluent features.
RESTful API is classic but it is still dominant protocol to share
web resources.
The `WebClient` is able to handle not only REST response but also
a stream response and a file response.
The API design of `WebClient`, in turn, is not optimized for REST API.

Modifications:

- Add `RestClient` for exchange RESTful APIs.
  - It assume that the response has a JSON body.
- Add `RestClientBuilder` so as to fluently build a `RestClient`.
- Refactory the intefaces for request preparations to accommodate
  `RestClientPreparation`.
  - Method and path setters are extracted to `RequestMethodSetters`.
  - Query and path param setters are moved to `PathAndQueryParamSetters`
  - Added `HttpMessageSetters` for setting `HttpMessage` properties.
- Add `execute()` extension methods for Kotlin Coroutine.
- Add `ScalaRestClient` for Scala.
  - Tried to add extension methods using an implicit class but Scala
    compiler failed to find the overloaded method which has a generic
    parameter.
- Provide a extension method to convert a `WebClient` to
  `ScalaRestClient`.
- Add `restClient()` to `ServerExtension` and `ServerRule`.

Result:

You can now easily exchange RESTful APIs using `RestClient`
- Java
  ```java
  RestClient restClient = RestClient.of("...");
  CompletableFuture<ResponseEntity<Customer>> response =
      restClient.get("/api/v1/customers/{customerId}")
                .pathParam("customerId", "0000001")
                .execute(Customer.class);
  ```
- Kotlin
  ```kt
  RestClient restClient = RestClient.of("...");
  val response: ResponseEntity<Customer> =
     restClient
       .get("/api/v1/customers/{customerId}")
       .pathParam("customerId", "0000001")
       .execute<Customer>()  // a suspend function
  ```
- Scala
  ```scala
  val restClient: ScalaRestClient = ScalaRestClient("...")
  val response: Future[ResponseEntity[Result]] =
    restClient.post("/api/v1/customers")
              .contentJson(new Customer(...))
              .execute[Result]()
  ```
@ikhoon ikhoon added this to the 1.17.0 milestone May 20, 2022
@codecov
Copy link

codecov bot commented May 22, 2022

Codecov Report

Merging #4263 (a828ab2) into master (4f85eb9) will decrease coverage by 0.10%.
The diff coverage is 36.96%.

❗ Current head a828ab2 differs from pull request most recent head 12b9f8b. Consider uploading reports for the commit 12b9f8b to get more accurate results

@@             Coverage Diff              @@
##             master    #4263      +/-   ##
============================================
- Coverage     73.30%   73.19%   -0.11%     
- Complexity    17071    17146      +75     
============================================
  Files          1458     1466       +8     
  Lines         64431    64719     +288     
  Branches       8126     8127       +1     
============================================
+ Hits          47228    47371     +143     
- Misses        13071    13216     +145     
  Partials       4132     4132              
Impacted Files Coverage Δ
...ia/client/BlockingWebClientRequestPreparation.java 21.73% <0.00%> (-0.49%) ⬇️
...a/client/FutureTransformingRequestPreparation.java 27.58% <0.00%> (-0.49%) ⬇️
...in/java/com/linecorp/armeria/client/WebClient.java 60.81% <ø> (ø)
.../com/linecorp/armeria/client/WebClientBuilder.java 69.44% <ø> (ø)
...rp/armeria/client/WebClientRequestPreparation.java 68.53% <ø> (+6.74%) ⬆️
.../linecorp/armeria/client/retry/BackoffWrapper.java 100.00% <ø> (ø)
...m/linecorp/armeria/common/HttpResponseBuilder.java 70.21% <0.00%> (-6.54%) ⬇️
...common/metric/AbstractMetricCollectingBuilder.java 100.00% <ø> (ø)
...ecorp/armeria/common/util/SettableIntSupplier.java 100.00% <ø> (ø)
...necorp/armeria/server/docs/DocStringExtractor.java 64.86% <ø> (ø)
... and 36 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 4f85eb9...12b9f8b. Read the comment docs.

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.

Overall looks really nice! 👍 Left some super nit comments 🙇


/**
* Sets the content as UTF_8 for this request.
*/
@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional; I think we can also remove javadocs if they are duplicated so that they are more easily maintainable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand what you are concerned about.
But there is a small difference in the Javadoc. It changed message to request.

@@ -285,11 +274,10 @@ final HttpHeadersBuilder headersBuilder() {

private RequestHeaders requestHeaders() {
requestHeadersBuilder.path(buildPath());
if (cookies != null) {
requestHeadersBuilder.set(COOKIE, Cookie.toCookieHeader(cookies));
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I believe with this change duplicate cookie headers are no longer allowed - I think there is no issue with this

}
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

We need more of these two tests 👍 👍 👍

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.

Changes look good to me 👍 Thanks @ikhoon 🚀 🚀 🚀

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.

Excellent work, @ikhoon. Left some minor comments 🙇

@@ -208,7 +208,9 @@
<property name="allowMissingReturnTag" value="true"/>
<property name="allowedAnnotations" value="Override, Test"/>
</module>
<module name="MissingJavadocMethod"/>
<module name="MissingJavadocMethod">
<property name="scope" value="protected"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note to self: protected scope covers both public and protected
https://checkstyle.sourceforge.io/property_types.html#Scope

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.

Nice work, @ikhoon 🚀

@trustin
Copy link
Member

trustin commented Jun 8, 2022

@minwoox Care to review?

Copy link
Contributor

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

Left only nits.
Thanks! 👍 👍 👍

final HttpData content = content();
if (content == null || content.isEmpty()) {
//noinspection ReactiveStreamsUnusedPublisher
Copy link
Contributor

Choose a reason for hiding this comment

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

cruft?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it wasn't. I intentionally added it to suppress the warning.
image

@minwoox minwoox merged commit f407e1f into line:master Jul 1, 2022
@minwoox
Copy link
Contributor

minwoox commented Jul 1, 2022

Thanks for adding this nice feature, @ikhoon!

@ikhoon ikhoon mentioned this pull request Jul 6, 2022
@ikhoon ikhoon deleted the rest-client branch July 6, 2022 03:40
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.

4 participants