-
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
Update MP REST Client to 4.0 #43959
Update MP REST Client to 4.0 #43959
Conversation
🎊 PR Preview de27583 has been successfully built and deployed to https://quarkus-pr-main-43959-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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! It looks mostly good to me except for some minor suggestions.
I would prefer for @geoand to have a look though.
...src/main/java/io/quarkus/rest/client/reactive/runtime/DefaultClientHeadersRequestFilter.java
Outdated
Show resolved
Hide resolved
...time/src/main/java/org/jboss/resteasy/reactive/client/handlers/ClientSendRequestHandler.java
Outdated
Show resolved
Hide resolved
...time/src/main/java/org/jboss/resteasy/reactive/client/handlers/ClientSendRequestHandler.java
Outdated
Show resolved
Hide resolved
...asy-client/runtime/src/main/java/io/quarkus/restclient/runtime/QuarkusRestClientBuilder.java
Show resolved
Hide resolved
2e2996e
to
95b1469
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
95b1469
to
9028324
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Guillaume Smet <[email protected]>
9028324
to
6c4a5b6
Compare
<dependency> | ||
<groupId>org.jboss.resteasy</groupId> | ||
<artifactId>resteasy-multipart-provider</artifactId> | ||
</dependency> |
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.
Not that I mind, but I would like to know why this is now necessary
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.
Also if it's necessary, we might have to depend on the quarkus-resteasy-multipart
extension instead.
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.
This is the dependency from RESTEasy that provides the EntityPart.Builder
implementation. Note that for the REST Client Classic, I've tried to keep it as the original implementation: resteasy/resteasy-microprofile@bb8b433
return Stream.of(type.getMethods()) | ||
.filter(method -> !IGNORED_METHODS.contains(method)) | ||
.toArray(Method[]::new); |
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'm definitely no fan of this, but we can let it go in :)
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.
Yes, this is a copy from the original RESTEasy Builder code:
resteasy/resteasy-microprofile@cb58ceb
Initially, we used the one provided by RESTEasy, but we made a copy to remove references to their CDI integration and also to remove the CDI dependency. They diverged a bit, so I've tried to reconcile them again for consistency.
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!!
Status for workflow
|
Status for workflow
|
No description, noteworthy label, or indication if there are implications on end-user applications ... |
I have opened issue here #44067 because it cost me time to find this. Please document this. There is no PR description, there is no migration guide change, there is no documentation change. Thank you |
The reported issue in #44067 is not caused by this change, which is a very simple one with zero impact on the end user. It is caused by #43345, which completely describes the changes, including the breaking changes. |
I apologize for my mistake. I don't think any of comments requesting documentation and PR description were wrong and it is true that there were no migration guide note, but again, my bad. I didn't find #43345 with my GH query, I will have to adjust my filter or rebuild Quarkus next time. |
No worries, all good. I usually include a Breaking Changes description in the PR, but only in the Migration Guide after the PR is merged because we can't be sure which version the PR will merge. #43345 was merged recently, so it was still on my TODO list to copy the breaking changes to the Migration Guide. |
About stuff mentioned in #43959 (comment), noteworthy label should be applied, wdyt? |
Updated! |
MicroProfile Rest Client Spec PDF
MicroProfile Rest Client Spec HTML
MicroProfile Rest Client Spec Javadocs