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.

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

Default parameter conversion for REST Client Reactive #27430

Closed
michalszynkiewicz opened this issue Aug 23, 2022 · 4 comments · Fixed by #27474
Closed

Default parameter conversion for REST Client Reactive #27430

michalszynkiewicz opened this issue Aug 23, 2022 · 4 comments · Fixed by #27474
Labels
Milestone

Comments

@michalszynkiewicz
Copy link
Member

Description

At the moment, REST Client Reactive, supports only String parameters or parameters that have registered custom parameter converters.
Anything else results in

Form element 'io.quarkus.it.rest.client.multipart.MultipartClient$WithBufferAsTextFile.number' could not be converted to 'String' for REST Client interface 'io.quarkus.it.rest.client.multipart.MultipartClient'. A proper implementation of 'javax.ws.rs.ext.ParamConverter' needs to be returned by a 'javax.ws.rs.ext.ParamConverterProvider' that is registered with the client via the @RegisterProvider annotation on the REST Client interface.

On the server side, we convert parameters according to the JAXRS spec:

Valid parameter types for each of the above annotations are listed in the corresponding Javadoc, however in general (excluding @Context) the following types are supported:

Types for which a ParamConverter is available via a registered ParamConverterProvider. See Javadoc for these classes for more information.

Primitive types.

Types that have a constructor that accepts a single String argument.

Types that have a static method named valueOf or fromString with a single String argument that return an instance of the type. If both methods are present then valueOf MUST be used unless the type is an enum in which case fromString MUST be used[1].

List<T>, Set<T>, or SortedSet<T>, where T satisfies 1,3 or 4 above.

We should support similar conversions on the client side (but the client side needs to convert from some type, e.g. primitive, to string).

CC @FroMage @Sgitario @geoand

Origin: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/RESTEasy.20Reactive.20client.20ParamConverter

Implementation ideas

No response

@michalszynkiewicz michalszynkiewicz added the kind/enhancement New feature or request label Aug 23, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 23, 2022

You added a link to a Zulip discussion, please make sure the description of the issue is comprehensive and doesn't require accessing Zulip.

This message is automatically generated by a bot.

@FroMage
Copy link
Member

FroMage commented Aug 23, 2022

Thanks.

@Sgitario
Copy link
Contributor

So, what we're trying to cover here is the following use case:

public interface FormClient {

        @POST
        @Consumes(MediaType.APPLICATION_FORM_URLENCODED)
        @Produces(MediaType.TEXT_PLAIN)
        String formWithTypes(@FormParam("text") String text, // Already working
                @FormParam("number") int number, // not working
                @FormParam("param") @PartType(MediaType.APPLICATION_JSON) Person person); // not working
    }

And the right solution should be to reuse the message readers to parse integer or any other type (like the Person class) to String, plus taking into account the media type defined in the PartType annotation.

Is this right?

@FroMage
Copy link
Member

FroMage commented Aug 26, 2022

Sorry, I don't think this is enough. I didn't have time to review your PR or I would have mentioned it, sorry :(

I tweaked your code in #27526 as well, since I had to have a workaround at the time I worked on the PR, and I did it by implementing ParamConverter for primitives. I moved the Enum check to RestClientBase.convertParam because it's always easier to hack stuff in our libs than in gizmo, plus you added a dynamic instanceof Enum instead of conditionally adding that code for enums we know at build-time (by looking at the parameter type).

But this issue is larger, because it's also about the other cases, using valueOf and fromString and the constructor, and also supporting the collection types, and what's worse, is that because we also support returning multipart values from the client, I expect that the opposite convertion needs to happen too (the String->Enum for example). I really didn't look at this part of the code.

Anyway, reopening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants