quarkus-rest-client-reactive does not URL encode some @QueryParam values

[plevart]

Describe the bug

When the value of a query parameter contains a pattern { ... anything ...} or %<hex-digit><hex-digit> that part of the value is left untouched and not URL encoded. This can then result in wrong invocations of REST service or in exceptions such as this:

javax.ws.rs.core.UriBuilderException: failed to create URI
	at org.jboss.resteasy.reactive.common.jaxrs.UriBuilderImpl.buildFromValues(UriBuilderImpl.java:736)
	at org.jboss.resteasy.reactive.common.jaxrs.UriBuilderImpl.build(UriBuilderImpl.java:724)
	at org.jboss.resteasy.reactive.client.impl.WebTargetImpl.createQuarkusRestInvocationBuilder(WebTargetImpl.java:310)
	at org.jboss.resteasy.reactive.client.impl.WebTargetImpl.request(WebTargetImpl.java:281)
	at org.acme.MyRemoteService$$QuarkusRestClientInterface.getExtensionsById(Unknown Source)
	at org.acme.MyRemoteService$$CDIWrapper.getExtensionsById(Unknown Source)
...
Caused by: java.net.URISyntaxException: Illegal character in query at index 48: https://stage.code.quarkus.io/api/extensions?id={foo&bar}
	at java.base/java.net.URI$Parser.fail(URI.java:2974)
	at java.base/java.net.URI$Parser.checkChars(URI.java:3145)
	at java.base/java.net.URI$Parser.parseHierarchical(URI.java:3233)
	at java.base/java.net.URI$Parser.parse(URI.java:3175)
	at java.base/java.net.URI.<init>(URI.java:623)
	at org.jboss.resteasy.reactive.common.jaxrs.UriBuilderImpl.buildFromValues(UriBuilderImpl.java:731)
	... 78 more

The culprit is the class org.jboss.resteasy.reactive.client.impl.WebTargetImpl which uses its embedded uriBuilder in the wrong way. When adding query parameters to it, it uses method: javax.ws.rs.core.UriBuilder#queryParam:

queryParam:895, UriBuilderImpl (org.jboss.resteasy.reactive.common.jaxrs)
queryParam:229, WebTargetImpl (org.jboss.resteasy.reactive.client.impl)
queryParam:21, WebTargetImpl (org.jboss.resteasy.reactive.client.impl)
getExtensionsById:-1, MyRemoteService$$QuarkusRestClientInterface (org.acme)
getExtensionsById:-1, MyRemoteService$$CDIWrapper (org.acme)
...

But there is another method on the implementation class of the UriBuilder, the org.jboss.resteasy.reactive.common.jaxrs.UriBuilderImpl#clientQueryParam (not part of UriBuilder interface unfortunately) which should be more appropriate for the client and states the following:

Called by ClientRequest.getUri() to add a query parameter for @QueryParam parameters. We do not use UriBuilder.queryParam() because
queryParam() supports URI template processing and this method must always encode braces (for parameter substitution is not possible for @QueryParam parameters).
queryParam() supports "contextual URI encoding" (i.e., it does not encode % characters that are followed by two hex characters). The JavaDoc for @QueryParam.value() explicitly states that the value is specified in decoded format and that "any percent encoded literals within the value will not be decoded and will instead be treated as literal text". This means that it is an explicit bug to perform contextual URI encoding of this method's name parameter; hence, we must always encode said parameter. This method also foregoes contextual URI encoding on this method's values parameter because it represents arbitrary data passed to a QueryParam parameter of a client proxy (since the client proxy is nothing more than a transport layer, it should not be "interpreting" such data; instead, it should faithfully transmit this data over the wire).

Expected behavior

Passing any string as @QueryParam should be possible as URL encoding should be used.

Actual behavior

When the value of a query parameter contains a pattern { ... anything ...} or %<hex-digit><hex-digit> that part of the value is left untouched and not URL encoded.

How to Reproduce?

package org.acme;

import org.eclipse.microprofile.rest.client.inject.RegisterRestClient;

import javax.ws.rs.GET;
import javax.ws.rs.Path;
import javax.ws.rs.QueryParam;
import java.util.List;
import java.util.Set;

@RegisterRestClient(baseUri = "https://stage.code.quarkus.io/api")
public interface MyRemoteService {

    @GET
    @Path("/extensions")
    Set<Extension> getExtensionsById(@QueryParam("id") String id);

    class Extension {
        public String id;
        public String name;
        public String shortName;
        public List<String> keywords;
    }
}

test:

package org.acme;

import io.quarkus.test.junit.QuarkusTest;
import org.eclipse.microprofile.rest.client.inject.RestClient;
import org.junit.jupiter.api.Test;

import javax.inject.Inject;

@QuarkusTest
public class MyRemoteServiceTest {

    @Inject
    @RestClient
    MyRemoteService myRemoteService;

    @Test
    public void testExtensionsRestClientEndpoint() throws Exception {
        myRemoteService.getExtensionsById("{foo&bar}");
    }
}

Output of uname -a or ver

Linux sun 5.14.18-100.fc33.x86_64 #1 SMP Fri Nov 12 17:38:44 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Output of java -version

openjdk version "17.0.1" 2021-10-19 OpenJDK Runtime Environment 21.9 (build 17.0.1+12) OpenJDK 64-Bit Server VM 21.9 (build 17.0.1+12, mixed mode, sharing)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.7.5.Final

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

Apache Maven 3.8.4 (9b656c72d54e5bacbed989b64718c159fe39b537) Maven home: /home/peter/JavaApps/apache-maven-3 Java version: 17.0.1, vendor: Red Hat, Inc., runtime: /usr/lib/jvm/java-17-openjdk-17.0.1.0.12-2.rolling.fc33.x86_64 Default locale: en_US, platform encoding: UTF-8 OS name: "linux", version: "5.14.18-100.fc33.x86_64", arch: "amd64", family: "unix"

Additional information

Not that quarkus-rest-client does not have this problem.

[quarkus-bot]

/cc @michalszynkiewicz

[gsmet]

Thanks for the detailed analysis @plevart .

@michalszynkiewicz @geoand let's try to fix this before CR1 if we can.

[plevart]

Perhaps the following method should be called from the generated XXX$$QuarkusRestClientInterface class:
org.jboss.resteasy.reactive.client.impl.WebTargetImpl#queryParamNoTemplate instead of org.jboss.resteasy.reactive.client.impl.WebTargetImpl#queryParam . The former uses exactly the right method org.jboss.resteasy.reactive.common.jaxrs.UriBuilderImpl#clientQueryParam

[gsmet]

@plevart maybe you want to try to prepare a PR with a test? Even just the test would be helpful.

[geoand]

@michalszynkiewicz I vaguely remember this coming up before...

[michalszynkiewicz]

there was something similar, a request to support @Encoded on clients. But I don't remember this exactly