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.

Sign up for GitHub

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

Jump to bottom

FEATURE: base feign-vertx on Vertx WebClient instead of bare vertx #2756

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

FEATURE: base feign-vertx on Vertx WebClient instead of bare vertx #2756

wants to merge 2 commits into from

Conversation

hosuaby
Copy link
Contributor

@hosuaby hosuaby commented Feb 6, 2025

I made a number of changes to feign-vertx module:

  • Switch from bare vertx to high level WebClient. It will give the user more configuration options, including the possibility of enabling OAuth2/OIDC authentication (cause feign-vertx and feign-oauth2 are not and cannot be compatible).
  • More reliable tests.
  • Ensured compatibility with Vertx 5.

@hosuaby hosuaby force-pushed the feature/vertxOauth2 branch 5 times, most recently from f971942 to d7adeba Compare February 7, 2025 18:53
velo
velo requested changes Feb 9, 2025
View reviewed changes
Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

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

vertx/pom.xml Outdated
<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>
Copy link
Member

Choose a reason for hiding this comment

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

is this needed?!

11 is the min version already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will check for it. Thanks

vertx/pom.xml Outdated
Comment on lines 32 to 34
<module>tests-vertx4</module>
<module>tests-vertx-common</module>
<module>tests-vertx5</module>
Copy link
Member

Choose a reason for hiding this comment

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

nice effort on test coverage 🥇

Copy link
Member

Choose a reason for hiding this comment

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

I think we can just call them feign-vertx*-test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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 think we can just call them feign-vertx*-test

done

vertx/tests-vertx5/src/test/java/feign/vertx/RetryingTest.java Outdated
import org.junit.jupiter.api.Test;

@DisplayName("When server ask client to retry")
class RetryingTest extends AbstractFeignVertxTest {
Copy link
Member

Choose a reason for hiding this comment

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

These look to be exactly the same

https://github.com/OpenFeign/feign/pull/2756/files#diff-618b10051163eb35607cd662bc192620411ccce4da0be7a99ee6dfb0b0ee4ad9L50

Why not move to test commons and just have a simplified version here?

github-actions[bot]
github-actions bot reviewed Feb 9, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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

@hosuaby hosuaby force-pushed the feature/vertxOauth2 branch from d7adeba to 580874b Compare February 11, 2025 14:35
@hosuaby
Copy link
Contributor Author

hosuaby commented Feb 11, 2025 •
edited
Loading

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

So here for the tests. What I managed and what not to achieve.

Achieved:

  1. Moved all tests into the main module with application code feign-vertx.
  2. Got rid of the module tests-vertx-common.
  3. The main module feign-vertx now produces a test-jar to be imported in test modules feign-vertx4-test and feign-vertx5-test.
  4. The module feign-vertx5-test contains no code. It will only be used to test binary compatibility with different versions of Vert.x 5.x.
  5. The module feign-vertx4-test contains tests rewritten using the API of Vert.x 4.x. It is used to test binary compatibility with different versions of Vert.x 4.x.

What I could not achieve:

  1. You suggested getting rid of the parent module, moving all the production code and tests to the root folder vertx, and producing two jars from it: one with production code and another with tests. By default, this is not possible with Maven and requires a ton of plugins to achieve. Due to architectural complexity, I did not pursue it.
  2. I moved common test code used by both modules feign-vertx4-test and feign-vertx5-test into the test jar produced by feign-vertx, but I could not factorize it further. Vert.x 4.x and 5.x have different APIs, so I need to write different code to create a WebClient with the required configuration. I just decided to copy test classes, even if they are similar. Otherwise, it would require a complex test architecture, making them completely unreadable.

@velo Please tell me what you think and what you would like to improve.

@hosuaby hosuaby force-pushed the feature/vertxOauth2 branch from 580874b to 1bdacbe Compare February 18, 2025 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@velo velo velo requested changes

@github-actions github-actions[bot] github-actions[bot] left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hosuaby @velo