FEATURE: base feign-vertx on Vertx WebClient instead of bare vertx #2756
Conversation
hosuaby
force-pushed
the
feature/vertxOauth2
branch
4 times, most recently
from
8c66ab1 to
f971942
Compare
hosuaby
force-pushed
the
feature/vertxOauth2
branch
from
f971942 to
d7adeba
Compare
There was a problem hiding this comment.
I think for tests you could keep them all on feign-vertx, generate the test-jar there, and on feign-vertx5-test you just extend the same tests under feign.vertx5 package....
Then you can make the module a top level project and if eventually vext breaks compatibility we can have multiples like we do with jax-rs
| * @return this builder | ||
| */ | ||
| public Builder vertx(final Vertx vertx) { |
There was a problem hiding this comment.
My main concern about this change is breaking compatibility with previous version.
Would it be possible to keep this (probably as deprecate) and run WebClient.create(vertx)
Then we can instruct to use the new method and mark for removal on next major release
There was a problem hiding this comment.
No need for that. Because we never released the current version. The previous is still available, by the way, in the separate project that was merged into the main project recently.
| * Sets request options using Vert.x {@link HttpClientOptions}. | ||
| * | ||
| * @param options {@code HttpClientOptions} for full customization of the underlying Vert.x | ||
| * {@link HttpClient} | ||
| * @return this builder | ||
| */ | ||
| public Builder options(final HttpClientOptions options) { | ||
| this.options = checkNotNull(options, "Argument options must be not null"); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Sets request options using Feign {@link Request.Options}. | ||
| * | ||
| * @param options Feign {@code Request.Options} object | ||
| * @return this builder | ||
| */ | ||
| @Override | ||
| public Builder options(final Request.Options options) { | ||
| checkNotNull(options, "Argument options must be not null"); | ||
| this.options = | ||
| new HttpClientOptions() | ||
| .setConnectTimeout(options.connectTimeoutMillis()) | ||
| .setIdleTimeout(options.readTimeoutMillis()); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Same as above, is it possible to keep as deprecated!?
There was a problem hiding this comment.
Idem. Current version from core project was never released.
| * </pre> | ||
| * | ||
| * @param requestPreProcessor operation to execute on each request | ||
| * @return updated request | ||
| */ | ||
| public Builder requestPreProcessor(UnaryOperator<HttpClientRequest> requestPreProcessor) { |
There was a problem hiding this comment.
the ask here would be the same, not sure if viable at all
There was a problem hiding this comment.
Idem. Current version from core project was never released.
| final Vertx vertx, | ||
| final HttpClientOptions options, | ||
| final WebClient webClient, | ||
| final long timeout, | ||
| final UnaryOperator<HttpClientRequest> requestPreProcessor) { |
There was a problem hiding this comment.
And lastly here too, keep old constructor, deprecate it, add new one
There was a problem hiding this comment.
Idem. Current version from core project was never released.
| <jackson.version>2.18.2</jackson.version> | ||
| <slf4j-log4j12.version>2.0.0-alpha6</slf4j-log4j12.version> | ||
| <wiremock.version>2.35.2</wiremock.version> | ||
| <main.java.version>11</main.java.version> |
There was a problem hiding this comment.
is this needed?!
11 is the min version already
There was a problem hiding this comment.
Will check for it. Thanks
| <module>tests-vertx4</module> | ||
| <module>tests-vertx-common</module> | ||
| <module>tests-vertx5</module> |
There was a problem hiding this comment.
I think we can just call them feign-vertx*-test
There was a problem hiding this comment.
Thanks for the suggestion. I will study them carefully and see how I can improve tests. The most important is to avoid to produce the jar and run tests at same build. By doing so we will not be able to detect runtime binary uncompatibilities with different versions of Vertx (that arise quite offten).
There was a problem hiding this comment.
nice effort on test coverage 🥇
Thanks! feign-vertx is a 10-year-old project. It had plenty of time to be tested in productions and covered with tests for the most of frequent problems.
| import org.junit.jupiter.api.Test; | ||
|
|
||
| @DisplayName("When server ask client to retry") | ||
| class RetryingTest extends AbstractFeignVertxTest { |
There was a problem hiding this comment.
These look to be exactly the same
Why not move to test commons and just have a simplified version here?
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Some suggestions could not be made:
- annotation-error-decoder/pom.xml
- lines 49-48
- apt-test-generator/pom.xml
- lines 62-61
- benchmark/pom.xml
- lines 70-69
- core/pom.xml
- lines 37-36
- googlehttpclient/pom.xml
- lines 54-53
- hc5/pom.xml
- lines 67-66
- httpclient/pom.xml
- lines 75-74
- hystrix/pom.xml
- lines 78-77
- jackson-jaxb/pom.xml
- lines 82-83
- java11/pom.xml
- lines 50-49
- jaxb-jakarta/pom.xml
- lines 55-57
- jaxb/pom.xml
- lines 45-49
- jaxrs2/pom.xml
- lines 103-102
- jaxrs3/pom.xml
- lines 89-88
- jaxrs4/pom.xml
- lines 83-82
- kotlin/pom.xml
- lines 71-70
- okhttp/pom.xml
- lines 59-58
- pom.xml
- lines 601-602
- reactive/pom.xml
- lines 98-97
- ribbon/pom.xml
- lines 83-82
- soap-jakarta/pom.xml
- lines 63-62
- lines 79-81
- soap/pom.xml
- lines 55-60
- lines 70-71
I made a number of changes to feign-vertx module: