-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Rest Client Reactive: globally register providers annotated with @Provider #18957
Rest Client Reactive: globally register providers annotated with @Provider #18957
Conversation
@@ -217,14 +213,34 @@ void registerProvidersFromAnnotations(CombinedIndexBuildItem indexBuildItem, | |||
constructor.invokeSpecialMethod(MethodDescriptor.ofConstructor(AnnotationRegisteredProviders.class), | |||
constructor.getThis()); | |||
|
|||
for (AnnotationInstance instance : index.getAnnotations(ResteasyReactiveDotNames.PROVIDER)) { | |||
// TODO: we may want to filter out stuff that is server side only |
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.
Here's the part I'm not really sure about.
@geoand do you think registering everything here can have some side effects?
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.
You need to honor this: https://quarkus.io/guides/rest-client#rest-client-and-resteasy-interactions (or at least have an equivalent).
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.
Exactly. We already do the equivalent on the server side
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 don't know how I could be unaware of this :) thank you! Fixing
4c717be
to
b32cf91
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building b32cf91
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/oidc-client-reactive✖
⚙️ JVM Tests - JDK 16 #📦 integration-tests/oidc-client-reactive✖
⚙️ MicroProfile TCKs Tests #📦 tcks/microprofile-rest-client-reactive✖
✖
✖
✖
|
b32cf91
to
dd69a26
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building dd69a26
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/oidc-client-reactive✖
⚙️ JVM Tests - JDK 16 #📦 integration-tests/oidc-client-reactive✖
⚙️ MicroProfile TCKs Tests #📦 tcks/microprofile-rest-client-reactive✖
⚙️ Native Tests - Security2 #📦 integration-tests/oidc-client-reactive✖
|
@michalszynkiewicz as we've discussed, Note, with ResteasyClassic,
Note there I'm concerned that just picking up all IMHO it makes sense to address the issue of the duplicates as part of this PR, i.e if you have |
Please also check what the MP RestClient expectations around
See the note that at the client side only the programmatic registration is expected |
thanks @sberyozkin ! MicroProfile Rest Client specification does not mention Re the duplicates, what I currently have stores the providers to register (either registered I can add a switch to disable automatic discovery for |
dd69a26
to
43a7611
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 43a7611
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/oidc-client-reactive✖
⚙️ JVM Tests - JDK 16 #📦 integration-tests/oidc-client-reactive✖
⚙️ Native Tests - Security2 #📦 integration-tests/oidc-client-reactive✖
|
6e0af54
to
27160d0
Compare
27160d0
to
2b52269
Compare
@geoand I added the switch, can you take another look if you still approve? |
fixes #18560
This change also adds support for sorting providers by
@Priority