Query param converters for lists & sets are not handled correctly in Resteasy reactive

[pravussum]

Describe the bug

This is a follow-up of #23486.

Now that custom parameter converters are used for query parameters, there are some cases, where this fails:

According to the JAX-RS resources documentation a valid method parameter has to fulfill one of the following:

In general the Java type of the method parameter may:

1) Be a primitive type;

2) Have a constructor that accepts a single String argument;

3) Have a static method named valueOf or fromString that accepts a single String argument (see, for example, Integer.valueOf(String) and java.util.UUID.fromString(String));

4) Have a registered implementation of jakarta.ws.rs.ext.ParamConverterProvider JAX-RS extension SPI that returns a jakarta.ws.rs.ext.ParamConverter instance capable of a "from string" conversion for the type. or

5) Be List<T>, Set<T> or SortedSet<T>, where T satisfies 2 or 3 above. The resulting collection is read-only. 

In cases where 4) AND 5) are fulfilled, the current implementation extracts the parameters according to bullet point 5 and then calls the parameter converter with the result, which in turn fails.

So for example if the there is a parameter converter which is able to handle List and a request is made with param = [{...pojo field here}] then the parameter converter get called with [[{...}]] (square brackets duplicated).

This is due to the way ResteasyReactiveRequestContext extracts the query parameters here and RuntimeResolvedConverter here.
The single parameter controls whether the parameter object will be a collection or not and the toString() methods adds the additional brackets, because single in QueryParamExtractor is not correctly set during processing.

I added some additional tests to demonstrate the problematic cases in MapWithParamConverterTest.

I tried to fix this in ServerEndpointIndexer, but again face some challenges:

The extractConverter() code in ServerEndpointIndexer is not aware of the runtime param converters they way that it is build right now. Thus, it cannot be decided whether to handle e. g. a List type according to 5) or as 4), using a parameter converter. Currently, if there is ANY converter, a RuntimeResolvedConverter is created, which then later cannot be resolved during runtime

The existingProviders map didn't use the full type, thus with the better support for generic types we have now there are clashes.

I am unsure how to continue from here. Should the param converter providers be made available during processor time, so they can be used in ServerEndpointIndexer to decide?
Or should the parameter handling during runtime be adapted, so it creates a generated converter on-the-fly?
Or am I completely on the wrong track?

/cc @geoand

Expected behavior

MapWithParamConverterTest are all green.

Actual behavior

See description

How to Reproduce?

MapWithParamConverterTest

Output of uname -a or ver

5.13.0-27-generic #29-Ubuntu SMP Wed Jan 12 17:36:47 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Output of java -version

openjdk-15.0.2

GraalVM version (if different from Java)

No response

Quarkus version or git rev

https://github.com/pravussum/quarkus

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

[quarkus-bot]

/cc @FroMage, @geoand, @stuartwdouglas

[geoand]

I am not really sure how this should be handled to be honest.

AFAIK, the List and Set types for parameters are meant to capture multiple values of the same parameter (and that is what the TCK tests).

[pravussum]

Yes, exactly, but still both could be true and by the order of the possibilities in the JAX-RS resource documentation I would take it, that it should be checked for a param converter first, and only then be handled as a list.
Otherwise it would never be possible to have a converter for anything that is a List (or Set) - but this is what our OpenAPI generator produces from the spec.

[geoand]

Yeah that makes sense.

[Plawn]

up on this, I got the same issue aha

[u6f6o]

I created a rest-client like this:

    @GET
    @Path("/api/channel-assets")
    fun getAssets(
        @QueryParam("sales-bundle") salesBundle: String,
        @QueryParam("options") options: Collection<String>
    ): Uni<List<ChannelAsset>>
}

The options resolve to: /api/channel-assets?sales-bundle=Some_Bundle&options=%5BMobile%5D. I suppose this is the same issue?

[geoand]

I am going to close this in favor of #29528