-
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
Add @ClientFormParam to Reactive REST Client #34535
Conversation
06800ad
to
61c2ea8
Compare
Seems reasonable, but I would like @Sgitario to take a look |
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.
The changes look amazing to me. Many thanks!
The only missing part is to update the documentation similar to what is mentioned to @ClientQueryParam
: https://quarkus.io/guides/rest-client-reactive#using-clientqueryparam
I'm also running the CI.
This comment has been minimized.
This comment has been minimized.
The test failures do seem related. |
@geoand @Sgitario Do you know why tests fail? I cloned ClientQueryParamFromMethodTest to ClientFormParamFromMethodTest, but it fails.
|
Added documentation, but still I was not able to fix test failures. |
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
I'm having a look into the test failures |
The message was indeed misleading, though when running the test on debug mode, you add a breakpoint here, you will see the actual errors. The problem is that the issue is found in the SubClient and that's why it could not identify the errors. Anyway, the actual errors are:
private static final MethodDescriptor MULTI_VALUED_MAP_ADD_ALL_METHOD = MethodDescriptor.ofMethod(MultivaluedMap.class,
"addAll", void.class, Object.class, List.class); After making these changes, still didn't work for me because the following exception at runtime:
But I hope you can continue investigating after my findings. |
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.
See my inline comments and last comment to fix the test failures.
...src/main/java/io/quarkus/rest/client/reactive/deployment/MicroProfileRestClientEnricher.java
Outdated
Show resolved
Hide resolved
...src/main/java/io/quarkus/rest/client/reactive/deployment/MicroProfileRestClientEnricher.java
Outdated
Show resolved
Hide resolved
Merged with latest commit from main branch, and now I am getting new error:
|
You need to rebased on main and update the code |
Thanks for your help. I applied all fixes you mentioned. I will try to figure out how to fix |
My bad. I had to recompile the project again. |
This comment has been minimized.
This comment has been minimized.
Ok, I fixed
It seems that the form params are not passed in the request. |
Fixed. All tests should pass now. |
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.
There are several concerns about the implementation:
- Generally speaking, we should be very careful when changing the signature of interfaces like JaxrsClientReactiveEnricher. In this case, we can avoid it (see the following points)
JaxrsClientReactiveEnricher.forWebTarget
andJaxrsClientReactiveEnricher.forSubResourceWebTarget
is meant to enrich the webTarget and when handling the form params, we are not updating the web target.- You're using an array to hold a single instance of formParams (as if you were passing a reference parameter).
To avoid these three points, I would create a new method in JaxrsClientReactiveEnricher like handleFormParams
that returns the assignable result handle (to avoid the usage of arrays for doing this).
Other comments:
- The method
MicroProfileRestClientEnricher.addFormParam
is too long and I think it can be refactored (eg reusing variables, etc). - There are several conditions like
if (!multipart)
, I don't fully understand why you ended up with this condition, but form params are not incompatible with multipart. I guess your intention here is not to parse the annotations that are used in Multipart classes? If this scenario is not supported, we should throw an exception to let users that this is not a valid use case. If this was not your intention, I would need further clarification about this to better understand the condition.
I agree with you, using separate method
Do you mean like merging
In case of multipart,
However, with Edit: I think I can get
Is this correct? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@Sgitario is on PTO, so let's wait for him to have the final say |
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.
Back from my PTO. Only two comments to address.
The rest of the changes look good to me. Thanks!
...src/main/java/io/quarkus/rest/client/reactive/deployment/MicroProfileRestClientEnricher.java
Outdated
Show resolved
Hide resolved
...src/main/java/io/quarkus/rest/client/reactive/deployment/MicroProfileRestClientEnricher.java
Outdated
Show resolved
Hide resolved
Welcome back. I updated the code according to your comments. Thanks. |
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. |
Resolves #34477