Stork path param resolution fix: use raw path and avoid double encodi…

…ng when creating new URI Related to #37713 refactor: clean up a few commented lines fix: use raw path and avoid double encoding, adapt tests accordingly (cherry picked from commit ce206d5)
quarkusio · Jan 9, 2024 · 842b3fe · 842b3fe
1 parent 9472824
commit 842b3fe
Show file tree

Hide file tree

Showing 13 changed files with 154 additions and 31 deletions.
diff --git a/...-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/HelloClient.java b/...-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/HelloClient.java
@@ -1,7 +1,11 @@
 package io.quarkus.rest.client.reactive.stork;
 
+import jakarta.ws.rs.Consumes;
 import jakarta.ws.rs.GET;
+import jakarta.ws.rs.POST;
 import jakarta.ws.rs.Path;
+import jakarta.ws.rs.PathParam;
+import jakarta.ws.rs.core.MediaType;
 
 import org.eclipse.microprofile.rest.client.inject.RegisterRestClient;
 
@@ -10,4 +14,13 @@
 public interface HelloClient {
     @GET
     String hello();
+
+    @POST
+    @Consumes(MediaType.TEXT_PLAIN)
+    @Path("/")
+    String echo(String name);
+
+    @GET
+    @Path("/{name}")
+    public String helloWithPathParam(@PathParam("name") String name);
 }
diff --git a/...eactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/HelloResource.java b/...eactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/HelloResource.java
@@ -1,7 +1,13 @@
 package io.quarkus.rest.client.reactive.stork;
 
 import jakarta.ws.rs.GET;
+import jakarta.ws.rs.POST;
 import jakarta.ws.rs.Path;
+import jakarta.ws.rs.PathParam;
+import jakarta.ws.rs.Produces;
+import jakarta.ws.rs.core.Context;
+import jakarta.ws.rs.core.MediaType;
+import jakarta.ws.rs.core.Request;
 
 @Path("/hello")
 public class HelloResource {
@@ -12,4 +18,16 @@ public class HelloResource {
     public String hello() {
         return HELLO_WORLD;
     }
+
+    @GET
+    @Path("/{name}")
+    @Produces(MediaType.TEXT_PLAIN)
+    public String invoke(@PathParam("name") String name) {
+        return "Hello, " + name;
+    }
+
+    @POST
+    public String echo(String name, @Context Request request) {
+        return "hello, " + name;
+    }
 }
diff --git a/...e/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/PassThroughResource.java b/...e/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/PassThroughResource.java
@@ -4,6 +4,7 @@
 
 import jakarta.ws.rs.GET;
 import jakarta.ws.rs.Path;
+import jakarta.ws.rs.PathParam;
 
 import org.eclipse.microprofile.rest.client.RestClientBuilder;
 import org.eclipse.microprofile.rest.client.inject.RestClient;
@@ -22,6 +23,18 @@ public String invokeClient() {
         return client.hello();
     }
 
+    @Path("/v2/{name}")
+    @GET
+    public String invokeClientWithPathParamContainingSlash(@PathParam("name") String name) {
+        return client.helloWithPathParam(name + "/" + name);
+    }
+
+    @Path("/{name}")
+    @GET
+    public String invokeClientWithPathParam(@PathParam("name") String name) {
+        return client.helloWithPathParam(name);
+    }
+
     @Path("/cdi")
     @GET
     public String invokeCdiClient() {

diff --git a/...tive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkDevModeTest.java b/...tive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkDevModeTest.java
@@ -67,4 +67,25 @@ void shouldModifyStorkSettings() {
                 .body(equalTo(WIREMOCK_RESPONSE));
         // @formatter:on
     }
+
+    @Test
+    void shouldSayHelloNameWithSlash() {
+        when()
+                .get("/helper/v2/stork")
+                .then()
+                .statusCode(200)
+                // The response contains an encoded `/`
+                .body(equalTo("Hello, stork/stork"));
+
+    }
+
+    @Test
+    void shouldSayHelloNameWithBlank() {
+        when()
+                .get("/helper/smallrye stork")
+                .then()
+                .statusCode(200)
+                // The response contains an encoded blank espace
+                .body(equalTo("Hello, smallrye stork"));
+    }
 }
diff --git a/.../deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkIntegrationTest.java b/.../deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkIntegrationTest.java
@@ -15,54 +15,65 @@
 import org.junit.jupiter.api.Timeout;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
-import io.quarkus.rest.client.reactive.HelloClient2;
-import io.quarkus.rest.client.reactive.HelloResource;
 import io.quarkus.test.QuarkusUnitTest;
 import io.smallrye.stork.api.NoSuchServiceDefinitionException;
 
 public class StorkIntegrationTest {
     @RegisterExtension
     static final QuarkusUnitTest TEST = new QuarkusUnitTest()
             .withApplicationRoot((jar) -> jar
-                    .addClasses(HelloClient2.class, HelloResource.class))
+                    .addClasses(HelloClient.class, HelloResource.class))
             .withConfigurationResource("stork-application.properties");
 
     @RestClient
-    HelloClient2 client;
+    HelloClient client;
 
     @Test
     void shouldDetermineUrlViaStork() {
         String greeting = RestClientBuilder.newBuilder().baseUri(URI.create("stork://hello-service/hello"))
-                .build(HelloClient2.class)
+                .build(HelloClient.class)
                 .echo("black and white bird");
         assertThat(greeting).isEqualTo("hello, black and white bird");
+
+        greeting = RestClientBuilder.newBuilder().baseUri(URI.create("stork://hello-service/hello"))
+                .build(HelloClient.class)
+                .helloWithPathParam("black and white bird");
+        assertThat(greeting).isEqualTo("Hello, black and white bird");
     }
 
     @Test
     void shouldDetermineUrlViaStorkWhenUsingTarget() throws URISyntaxException {
-        String greeting = ClientBuilder.newClient().target("stork://hello-service/hello").request().get(String.class);
-        assertThat(greeting).isEqualTo("Hello");
+        String greeting = ClientBuilder.newClient().target("stork://hello-service/hello").request()
+                .get(String.class);
+        assertThat(greeting).isEqualTo("Hello, World!");
 
         greeting = ClientBuilder.newClient().target(new URI("stork://hello-service/hello")).request().get(String.class);
-        assertThat(greeting).isEqualTo("Hello");
+        assertThat(greeting).isEqualTo("Hello, World!");
 
         greeting = ClientBuilder.newClient().target(UriBuilder.fromUri("stork://hello-service/hello")).request()
                 .get(String.class);
-        assertThat(greeting).isEqualTo("Hello");
+        assertThat(greeting).isEqualTo("Hello, World!");
+
+        greeting = ClientBuilder.newClient().target("stork://hello-service/hello").path("big bird").request()
+                .get(String.class);
+        assertThat(greeting).isEqualTo("Hello, big bird");
     }
 
     @Test
     void shouldDetermineUrlViaStorkCDI() {
         String greeting = client.echo("big bird");
         assertThat(greeting).isEqualTo("hello, big bird");
+
+        greeting = client.helloWithPathParam("big bird");
+        assertThat(greeting).isEqualTo("Hello, big bird");
     }
 
     @Test
     @Timeout(20)
     void shouldFailOnUnknownService() {
-        HelloClient2 client2 = RestClientBuilder.newBuilder()
+        HelloClient client = RestClientBuilder.newBuilder()
                 .baseUri(URI.create("stork://nonexistent-service"))
-                .build(HelloClient2.class);
-        assertThatThrownBy(() -> client2.echo("foo")).isInstanceOf(NoSuchServiceDefinitionException.class);
+                .build(HelloClient.class);
+        assertThatThrownBy(() -> client.echo("foo")).isInstanceOf(NoSuchServiceDefinitionException.class);
     }
 }
diff --git a/...rc/test/java/io/quarkus/rest/client/reactive/stork/StorkResponseTimeLoadBalancerTest.java b/...rc/test/java/io/quarkus/rest/client/reactive/stork/StorkResponseTimeLoadBalancerTest.java
@@ -16,8 +16,6 @@
 import com.github.tomakehurst.wiremock.WireMockServer;
 import com.github.tomakehurst.wiremock.client.WireMock;
 
-import io.quarkus.rest.client.reactive.HelloClient2;
-import io.quarkus.rest.client.reactive.HelloResource;
 import io.quarkus.test.QuarkusUnitTest;
 
 public class StorkResponseTimeLoadBalancerTest {
@@ -28,7 +26,7 @@ public class StorkResponseTimeLoadBalancerTest {
     @RegisterExtension
     static final QuarkusUnitTest TEST = new QuarkusUnitTest()
             .withApplicationRoot((jar) -> jar
-                    .addClasses(HelloClient2.class, HelloResource.class))
+                    .addClasses(HelloClient.class, HelloResource.class))
             .withConfigurationResource("stork-stat-lb.properties");
 
     @BeforeAll
@@ -46,7 +44,7 @@ public static void shutDown() {
     }
 
     @RestClient
-    HelloClient2 client;
+    HelloClient client;
 
     @Test
     void shouldUseFasterService() {

diff --git a/...ent/src/test/java/io/quarkus/rest/client/reactive/stork/StorkWithPathIntegrationTest.java b/...ent/src/test/java/io/quarkus/rest/client/reactive/stork/StorkWithPathIntegrationTest.java
@@ -15,54 +15,64 @@
 import org.junit.jupiter.api.Timeout;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
-import io.quarkus.rest.client.reactive.HelloClient2;
-import io.quarkus.rest.client.reactive.HelloResource;
 import io.quarkus.test.QuarkusUnitTest;
 import io.smallrye.stork.api.NoSuchServiceDefinitionException;
 
 public class StorkWithPathIntegrationTest {
     @RegisterExtension
     static final QuarkusUnitTest TEST = new QuarkusUnitTest()
             .withApplicationRoot((jar) -> jar
-                    .addClasses(HelloClient2.class, HelloResource.class))
+                    .addClasses(HelloClient.class, HelloResource.class))
             .withConfigurationResource("stork-application-with-path.properties");
 
     @RestClient
-    HelloClient2 client;
+    HelloClient client;
 
     @Test
     void shouldDetermineUrlViaStork() {
         String greeting = RestClientBuilder.newBuilder().baseUri(URI.create("stork://hello-service"))
-                .build(HelloClient2.class)
+                .build(HelloClient.class)
                 .echo("black and white bird");
         assertThat(greeting).isEqualTo("hello, black and white bird");
+
+        greeting = RestClientBuilder.newBuilder().baseUri(URI.create("stork://hello-service"))
+                .build(HelloClient.class)
+                .helloWithPathParam("black and white bird");
+        assertThat(greeting).isEqualTo("Hello, black and white bird");
     }
 
     @Test
     void shouldDetermineUrlViaStorkWhenUsingTarget() throws URISyntaxException {
         String greeting = ClientBuilder.newClient().target("stork://hello-service").request().get(String.class);
-        assertThat(greeting).isEqualTo("Hello");
+        assertThat(greeting).isEqualTo("Hello, World!");
 
         greeting = ClientBuilder.newClient().target(new URI("stork://hello-service")).request().get(String.class);
-        assertThat(greeting).isEqualTo("Hello");
+        assertThat(greeting).isEqualTo("Hello, World!");
 
         greeting = ClientBuilder.newClient().target(UriBuilder.fromUri("stork://hello-service/")).request()
                 .get(String.class);
-        assertThat(greeting).isEqualTo("Hello");
+        assertThat(greeting).isEqualTo("Hello, World!");
+
+        greeting = ClientBuilder.newClient().target("stork://hello-service/").path("big bird").request()
+                .get(String.class);
+        assertThat(greeting).isEqualTo("Hello, big bird");
     }
 
     @Test
     void shouldDetermineUrlViaStorkCDI() {
         String greeting = client.echo("big bird");
         assertThat(greeting).isEqualTo("hello, big bird");
+
+        greeting = client.helloWithPathParam("big bird");
+        assertThat(greeting).isEqualTo("Hello, big bird");
     }
 
     @Test
     @Timeout(20)
     void shouldFailOnUnknownService() {
-        HelloClient2 client2 = RestClientBuilder.newBuilder()
+        HelloClient client = RestClientBuilder.newBuilder()
                 .baseUri(URI.create("stork://nonexistent-service"))
-                .build(HelloClient2.class);
-        assertThatThrownBy(() -> client2.echo("foo")).isInstanceOf(NoSuchServiceDefinitionException.class);
+                .build(HelloClient.class);
+        assertThatThrownBy(() -> client.echo("foo")).isInstanceOf(NoSuchServiceDefinitionException.class);
     }
 }
diff --git a/...ntime/src/main/java/org/jboss/resteasy/reactive/client/impl/StorkClientRequestFilter.java b/...ntime/src/main/java/org/jboss/resteasy/reactive/client/impl/StorkClientRequestFilter.java
@@ -7,6 +7,7 @@
 import jakarta.annotation.Priority;
 import jakarta.ws.rs.Priorities;
 import jakarta.ws.rs.core.GenericType;
+import jakarta.ws.rs.core.UriBuilder;
 import jakarta.ws.rs.ext.Provider;
 
 import org.jboss.logging.Logger;
@@ -62,7 +63,7 @@ public void filter(ResteasyReactiveClientRequestContext requestContext) {
                             }
                             // Service instance can also contain an optional path.
                             Optional<String> path = instance.getPath();
-                            String actualPath = uri.getPath();
+                            String actualPath = uri.getRawPath();
                             if (path.isPresent()) {
                                 var p = path.get();
                                 if (!p.startsWith("/")) {
@@ -79,11 +80,12 @@ public void filter(ResteasyReactiveClientRequestContext requestContext) {
                                     }
                                 }
                             }
-
+                            //To avoid the path double encoding we create uri with path=null and set the path after
                             URI newUri = new URI(scheme,
                                     uri.getUserInfo(), host, port,
-                                    actualPath, uri.getQuery(), uri.getFragment());
-                            requestContext.setUri(newUri);
+                                    null, uri.getQuery(), uri.getFragment());
+                            URI build = UriBuilder.fromUri(newUri).path(actualPath).build();
+                            requestContext.setUri(build);
                             if (measureTime && instance.gatherStatistics()) {
                                 requestContext.setCallStatsCollector(instance);
                             }

diff --git a/...-client-reactive-stork/src/main/java/io/quarkus/it/rest/client/reactive/stork/Client.java b/...-client-reactive-stork/src/main/java/io/quarkus/it/rest/client/reactive/stork/Client.java
@@ -2,6 +2,9 @@
 
 import jakarta.ws.rs.Consumes;
 import jakarta.ws.rs.GET;
+import jakarta.ws.rs.Path;
+import jakarta.ws.rs.PathParam;
+import jakarta.ws.rs.Produces;
 import jakarta.ws.rs.core.MediaType;
 
 import org.eclipse.microprofile.rest.client.inject.RegisterRestClient;
@@ -11,4 +14,9 @@ public interface Client {
     @GET
     @Consumes(MediaType.TEXT_PLAIN)
     String echo(String name);
+
+    @GET
+    @Path("/v2/{name}")
+    @Produces(MediaType.TEXT_PLAIN)
+    String invoke(@PathParam("name") String name);
 }
diff --git a/...e-stork/src/main/java/io/quarkus/it/rest/client/reactive/stork/ClientCallingResource.java b/...e-stork/src/main/java/io/quarkus/it/rest/client/reactive/stork/ClientCallingResource.java
@@ -3,6 +3,9 @@
 import jakarta.enterprise.context.ApplicationScoped;
 import jakarta.ws.rs.GET;
 import jakarta.ws.rs.Path;
+import jakarta.ws.rs.PathParam;
+import jakarta.ws.rs.Produces;
+import jakarta.ws.rs.core.MediaType;
 
 import org.eclipse.microprofile.rest.client.inject.RestClient;
 
@@ -17,4 +20,11 @@ public class ClientCallingResource {
     public String passThrough() {
         return client.echo("World!");
     }
+
+    @GET
+    @Path("/{name}")
+    @Produces(MediaType.TEXT_PLAIN)
+    public String invoke(@PathParam("name") String name) {
+        return client.invoke(name + "/" + name);
+    }
 }
diff --git a/...nt-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/FastWiremockServer.java b/...nt-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/FastWiremockServer.java
@@ -1,6 +1,7 @@
 package io.quarkus.it.rest.reactive.stork;
 
 import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlPathTemplate;
 
 import java.util.Map;
 
@@ -25,6 +26,8 @@ int httpsPort() {
     protected Map<String, String> initWireMock(WireMockServer server) {
         server.stubFor(WireMock.get("/hello")
                 .willReturn(aResponse().withBody(FAST_RESPONSE).withStatus(200)));
+        server.stubFor(WireMock.get(urlPathTemplate("/hello/v2/{name}"))
+                .willReturn(aResponse().withBody(FAST_RESPONSE).withStatus(200)));
         return Map.of("fast-service", "localhost:8443");
     }
 }