From ce206d5e43f5260ce71bc792ac5dd576bd4ff2f5 Mon Sep 17 00:00:00 2001 From: Auri Munoz Date: Thu, 14 Dec 2023 19:30:28 +0100 Subject: [PATCH] Stork path param resolution fix: use raw path and avoid double encoding 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 --- .../client/reactive/stork/HelloClient.java | 13 +++++++ .../client/reactive/stork/HelloResource.java | 18 ++++++++++ .../reactive/stork/PassThroughResource.java | 13 +++++++ .../reactive/stork/StorkDevModeTest.java | 21 +++++++++++ .../reactive/stork/StorkIntegrationTest.java | 35 ++++++++++++------- .../StorkResponseTimeLoadBalancerTest.java | 6 ++-- .../stork/StorkWithPathIntegrationTest.java | 32 +++++++++++------ .../client/impl/StorkClientRequestFilter.java | 10 +++--- .../it/rest/client/reactive/stork/Client.java | 8 +++++ .../reactive/stork/ClientCallingResource.java | 10 ++++++ .../reactive/stork/FastWiremockServer.java | 3 ++ .../stork/RestClientReactiveStorkTest.java | 13 +++++++ .../reactive/stork/SlowWiremockServer.java | 3 ++ 13 files changed, 154 insertions(+), 31 deletions(-) diff --git a/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/HelloClient.java b/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/HelloClient.java index 664dea477ef7d..baa2e8d3771d5 100644 --- a/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/HelloClient.java +++ b/extensions/resteasy-reactive/rest-client-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/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/HelloResource.java b/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/HelloResource.java index e9966a8d8eac6..1a544e2ab878e 100644 --- a/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/HelloResource.java +++ b/extensions/resteasy-reactive/rest-client-reactive/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/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/PassThroughResource.java b/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/PassThroughResource.java index 129b7aece4cda..51f11c1b539ca 100644 --- a/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/PassThroughResource.java +++ b/extensions/resteasy-reactive/rest-client-reactive/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/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkDevModeTest.java b/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkDevModeTest.java index f30d13b937008..5a12b520c497b 100644 --- a/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkDevModeTest.java +++ b/extensions/resteasy-reactive/rest-client-reactive/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/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkIntegrationTest.java b/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkIntegrationTest.java index cb22c1393db59..639ae39cd8fac 100644 --- a/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkIntegrationTest.java +++ b/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkIntegrationTest.java @@ -15,8 +15,6 @@ 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; @@ -24,45 +22,58 @@ 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/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkResponseTimeLoadBalancerTest.java b/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkResponseTimeLoadBalancerTest.java index 507ca9eb31b1a..9dc52a8d0d271 100644 --- a/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkResponseTimeLoadBalancerTest.java +++ b/extensions/resteasy-reactive/rest-client-reactive/deployment/src/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/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkWithPathIntegrationTest.java b/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkWithPathIntegrationTest.java index 26ba43279cbae..26ac15b363f45 100644 --- a/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkWithPathIntegrationTest.java +++ b/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/stork/StorkWithPathIntegrationTest.java @@ -15,8 +15,6 @@ 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; @@ -24,45 +22,57 @@ 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/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/StorkClientRequestFilter.java b/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/StorkClientRequestFilter.java index 7083d96803940..60990009a9d88 100644 --- a/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/StorkClientRequestFilter.java +++ b/independent-projects/resteasy-reactive/client/runtime/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 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/integration-tests/rest-client-reactive-stork/src/main/java/io/quarkus/it/rest/client/reactive/stork/Client.java b/integration-tests/rest-client-reactive-stork/src/main/java/io/quarkus/it/rest/client/reactive/stork/Client.java index 3c3bcea3042bc..ebf44dc314680 100644 --- a/integration-tests/rest-client-reactive-stork/src/main/java/io/quarkus/it/rest/client/reactive/stork/Client.java +++ b/integration-tests/rest-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/integration-tests/rest-client-reactive-stork/src/main/java/io/quarkus/it/rest/client/reactive/stork/ClientCallingResource.java b/integration-tests/rest-client-reactive-stork/src/main/java/io/quarkus/it/rest/client/reactive/stork/ClientCallingResource.java index 2e3a307174dc8..782165a5c3895 100644 --- a/integration-tests/rest-client-reactive-stork/src/main/java/io/quarkus/it/rest/client/reactive/stork/ClientCallingResource.java +++ b/integration-tests/rest-client-reactive-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/integration-tests/rest-client-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/FastWiremockServer.java b/integration-tests/rest-client-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/FastWiremockServer.java index ba55cdc0f30f0..a6b277681a970 100644 --- a/integration-tests/rest-client-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/FastWiremockServer.java +++ b/integration-tests/rest-client-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 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"); } } diff --git a/integration-tests/rest-client-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/RestClientReactiveStorkTest.java b/integration-tests/rest-client-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/RestClientReactiveStorkTest.java index 5adb6924ee71b..884d5ffbbfc0f 100644 --- a/integration-tests/rest-client-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/RestClientReactiveStorkTest.java +++ b/integration-tests/rest-client-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/RestClientReactiveStorkTest.java @@ -54,4 +54,17 @@ void shouldUseFasterService() { // after hitting the slow endpoint, we should only use the fast one: assertThat(responses).containsOnly(FAST_RESPONSE, FAST_RESPONSE, FAST_RESPONSE); } + + @Test + void shouldUseV2Service() { + Set responses = new HashSet<>(); + + for (int i = 0; i < 2; i++) { + Response response = when().get("/client/quarkus"); + response.then().statusCode(200); + } + + responses.clear(); + + } } diff --git a/integration-tests/rest-client-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/SlowWiremockServer.java b/integration-tests/rest-client-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/SlowWiremockServer.java index 7dbc7f74b9b9d..3b1a051f345a6 100644 --- a/integration-tests/rest-client-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/SlowWiremockServer.java +++ b/integration-tests/rest-client-reactive-stork/src/test/java/io/quarkus/it/rest/reactive/stork/SlowWiremockServer.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; @@ -26,6 +27,8 @@ protected Map initWireMock(WireMockServer server) { server.stubFor(WireMock.get("/hello") .willReturn(aResponse().withFixedDelay(1000) .withBody(SLOW_RESPONSE).withStatus(200))); + server.stubFor(WireMock.get(urlPathTemplate("/hello/v2/{name}")) + .willReturn(aResponse().withFixedDelay(1000).withBody(SLOW_RESPONSE).withStatus(200))); return Map.of("slow-service", "localhost:8444"); } }