-
Notifications
You must be signed in to change notification settings - Fork 927
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
Support for Spring 6 HTTP interface #5370
Conversation
be3d38d
to
6a8253c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5370 +/- ##
===========================================
+ Coverage 0 74.04% +74.04%
- Complexity 0 20839 +20839
===========================================
Files 0 1805 +1805
Lines 0 76697 +76697
Branches 0 9776 +9776
===========================================
+ Hits 0 56794 +56794
- Misses 0 15279 +15279
- Partials 0 4624 +4624 ☔ View full report in Codecov by Sentry. |
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.
Looks pretty good. 👍
Left some questions.
spring/spring6/src/main/java/com/linecorp/armeria/spring/client/ArmeriaHttpExchangeAdapter.java
Outdated
Show resolved
Hide resolved
spring/spring6/src/main/java/com/linecorp/armeria/spring/client/ArmeriaHttpExchangeAdapter.java
Outdated
Show resolved
Hide resolved
spring/spring6/src/main/java/com/linecorp/armeria/spring/client/ArmeriaHttpExchangeAdapter.java
Outdated
Show resolved
Hide resolved
spring/spring6/src/main/java/com/linecorp/armeria/spring/client/ArmeriaHttpExchangeAdapter.java
Outdated
Show resolved
Hide resolved
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 a question but it looks really great! Thanks!
...ing6/src/main/java/com/linecorp/armeria/spring/client/ArmeriaHttpExchangeAdapterBuilder.java
Outdated
Show resolved
Hide resolved
* | ||
* GreetingService greetingService = ... | ||
* // The request attribute will be set to the "X-Request-Id" header. | ||
* greetingService.hello("123"); |
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.
Question: So the decorator is not added to the client, the 123
will be ignored. Is that correct?
If so, is there any way to prevent this situation?
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.
Right. It's up to the user how to use the request attribute.
In Spring, the value of @RequestAttribute
is propagated to Filter
.
It is an optional feature for which additional validation may not be needed.
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.
Thanks! Changes look straightforward to me 👍 👍 👍
Thanks a lot, @ikhoon! |
Motivation:
The Spring Framework 6 supports defining declarative HTTP services using Java interfaces like Feign.
That could be an alternative choice for Spring users instead of Retrofit.
Modifications:
spring6
module to provideArmeriaHttpExchangeAdapter
.ArmeriaClientHttpRequest
andArmeriaClientHttpRequest
are moved to Spring 6 module from the WebFlux module.ClientRequest
andClientResponse
in Spring 6 are used as a bridge to encode an object intoHttpData
and vice versa.Result:
You can now create Spring 6 HTTP interface with Armeria
WebClient
.