-
Notifications
You must be signed in to change notification settings - Fork 924
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
Conversation
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]() ```
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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 🙇
core/src/main/java/com/linecorp/armeria/client/RequestPreparationSetters.java
Show resolved
Hide resolved
|
||
/** | ||
* Sets the content as UTF_8 for this request. | ||
*/ | ||
@Override |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
core/src/main/java/com/linecorp/armeria/common/AbstractHttpMessageBuilder.java
Show resolved
Hide resolved
@@ -285,11 +274,10 @@ final HttpHeadersBuilder headersBuilder() { | |||
|
|||
private RequestHeaders requestHeaders() { | |||
requestHeadersBuilder.path(buildPath()); | |||
if (cookies != null) { | |||
requestHeadersBuilder.set(COOKIE, Cookie.toCookieHeader(cookies)); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 👍 👍 👍
There was a problem hiding this 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 🚀 🚀 🚀
There was a problem hiding this 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 🙇
core/src/main/java/com/linecorp/armeria/client/RequestPreparationSetters.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/AbstractHttpMessageBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/PathAndQueryParamSetters.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/RequestMethodSetters.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/client/RestClientTest.java
Outdated
Show resolved
Hide resolved
junit5/src/main/java/com/linecorp/armeria/internal/testing/ServerRuleDelegate.java
Show resolved
Hide resolved
scala/scala_2.13/src/main/scala/com/linecorp/armeria/scala/ClientConversions.scala
Outdated
Show resolved
Hide resolved
@@ -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"/> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, @ikhoon 🚀
@minwoox Care to review? |
There was a problem hiding this 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! 👍 👍 👍
core/src/main/java/com/linecorp/armeria/client/RequestPreparationSetters.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/RestClientPreparation.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/retry/BackoffWrapper.java
Outdated
Show resolved
Hide resolved
final HttpData content = content(); | ||
if (content == null || content.isEmpty()) { | ||
//noinspection ReactiveStreamsUnusedPublisher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cruft?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core/src/main/java/com/linecorp/armeria/common/PathAndQueryParamSetters.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/RequestMethodSetters.java
Outdated
Show resolved
Hide resolved
…ionSetters.java Co-authored-by: minux <songmw725@gmail.com>
…apper.java Co-authored-by: minux <songmw725@gmail.com>
…amSetters.java Co-authored-by: minux <songmw725@gmail.com>
…tters.java Co-authored-by: minux <songmw725@gmail.com>
Thanks for adding this nice feature, @ikhoon! |
Motivation:
WebClient
andWebClientPreparation
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 alsoa stream response and a file response.
The API design of
WebClient
, in turn, is not optimized for REST API.See #4250
Modifications:
RestClient
for exchange RESTful APIs.RestClientBuilder
so as to fluently build aRestClient
.RestClientPreparation
.RequestMethodSetters
.PathAndQueryParamSetters
HttpMessageSetters
for settingHttpMessage
properties.execute()
extension methods for Kotlin Coroutine.ScalaRestClient
for Scala.compiler failed to find the overloaded method which has a generic
parameter.
WebClient
toScalaRestClient
.restClient()
toServerExtension
andServerRule
.Result:
You can now easily send and receive RESTful APIs using
RestClient