From 6ca76ecc00c556da34e44ded59adddada39f50ce Mon Sep 17 00:00:00 2001 From: Georgios Andrianakis Date: Thu, 28 Nov 2024 18:07:36 +0200 Subject: [PATCH] Introduce property to disable default mapper per REST Client The default mapper has a very confusing behavior where it throws an exception HTTP code >= 400 even if the method return type is Response. This PR means to provide a way so users can disable this mapper per client instead of globally. Closes: #44813 --- docs/src/main/asciidoc/rest-client.adoc | 28 ++++++ .../restclient/config/RestClientsConfig.java | 11 ++- .../GlobalExceptionMapperDisabledTest.java | 68 +++++++++++++++ .../PerClientExceptionMapperDisabledTest.java | 87 +++++++++++++++++++ .../reactive/QuarkusRestClientBuilder.java | 6 ++ .../runtime/QuarkusRestClientBuilderImpl.java | 6 ++ .../runtime/RestClientBuilderImpl.java | 34 ++++++-- .../runtime/RestClientCDIDelegateBuilder.java | 2 + 8 files changed, 233 insertions(+), 9 deletions(-) create mode 100644 extensions/resteasy-reactive/rest-client/deployment/src/test/java/io/quarkus/rest/client/reactive/error/GlobalExceptionMapperDisabledTest.java create mode 100644 extensions/resteasy-reactive/rest-client/deployment/src/test/java/io/quarkus/rest/client/reactive/error/PerClientExceptionMapperDisabledTest.java diff --git a/docs/src/main/asciidoc/rest-client.adoc b/docs/src/main/asciidoc/rest-client.adoc index 5cb6e2b44395c..71900e4abfd9d 100644 --- a/docs/src/main/asciidoc/rest-client.adoc +++ b/docs/src/main/asciidoc/rest-client.adoc @@ -1437,6 +1437,34 @@ public interface EchoClient { } ---- +=== Disabling the default mapper + +As mandated by the REST Client specification, a default exception mapper is included, that throws an exception when HTTP status code is higher than 400. +While this behavior is fine when the client returns a regular object, it is however very unintuitive when the client needs to return a `jakarta.ws.rs.core.Response` +(with the intention of allowing the caller to decide how to handle the HTTP status code). + +For this reason, the REST Client includes a property named `disable-default-mapper` which can be used to disable the default mapper when using a REST client in a declarative manner. + +For example, with a client like so: + +[source, java] +---- + @Path("foo") + @RegisterRestClient(configKey = "bar") + public interface Client { + @GET + Response get(); + } +---- + +The default exception mapper can be disabled by setting `quarkus.rest-client.bar.disable-default-mapper=true` to disable the exception mapper for the REST Client configured with the key `bar`. + +[NOTE] +==== +When using the programmatic approach for creating a REST Client, `QuarkusRestClientBuilder` provides a method named `disableDefaultMapper` +that provides the same feature. +==== + [[multipart]] == Multipart Form support diff --git a/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/RestClientsConfig.java b/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/RestClientsConfig.java index d5bfb8d86a5ab..1c3f23709530d 100644 --- a/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/RestClientsConfig.java +++ b/extensions/resteasy-classic/rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/RestClientsConfig.java @@ -594,8 +594,7 @@ default Optional uriReload() { Optional http2(); /** - * The max HTTP ch - * unk size (8096 bytes by default). + * The max HTTP chunk size (8096 bytes by default). *

* This property is not applicable to the RESTEasy Client. */ @@ -615,6 +614,14 @@ default Optional uriReload() { * This stacktrace will be used if the invocation throws an exception */ Optional captureStacktrace(); + + /** + * If set to {@code true}, then this REST Client will not the default exception mapper which + * always throws an exception if HTTP response code >= 400. + * This property is not applicable to the RESTEasy Client. + */ + @WithDefault("${microprofile.rest.client.disable.default.mapper:false}") + Boolean disableDefaultMapper(); } class RestClientKeysProvider implements Supplier> { diff --git a/extensions/resteasy-reactive/rest-client/deployment/src/test/java/io/quarkus/rest/client/reactive/error/GlobalExceptionMapperDisabledTest.java b/extensions/resteasy-reactive/rest-client/deployment/src/test/java/io/quarkus/rest/client/reactive/error/GlobalExceptionMapperDisabledTest.java new file mode 100644 index 0000000000000..78aeb73848cc5 --- /dev/null +++ b/extensions/resteasy-reactive/rest-client/deployment/src/test/java/io/quarkus/rest/client/reactive/error/GlobalExceptionMapperDisabledTest.java @@ -0,0 +1,68 @@ +package io.quarkus.rest.client.reactive.error; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.net.URI; + +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.core.Response; + +import org.eclipse.microprofile.rest.client.inject.RegisterRestClient; +import org.eclipse.microprofile.rest.client.inject.RestClient; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.quarkus.rest.client.reactive.QuarkusRestClientBuilder; +import io.quarkus.test.QuarkusUnitTest; +import io.quarkus.test.common.http.TestHTTPResource; + +public class GlobalExceptionMapperDisabledTest { + + @RegisterExtension + static final QuarkusUnitTest TEST = new QuarkusUnitTest() + .withApplicationRoot((jar) -> jar + .addClasses(Client.class, Resource.class)) + .overrideRuntimeConfigKey("quarkus.rest-client.test.url", "${test.url}") + .overrideRuntimeConfigKey("microprofile.rest.client.disable.default.mapper", "true"); + + @RestClient + Client client; + + @TestHTTPResource + URI baseUri; + + @Test + void testDeclarativeClient() { + Response response = client.get(); + assertThat(response.getStatus()).isEqualTo(404); + } + + @Test + void testProgrammaticClient() { + OtherClient otherClient = QuarkusRestClientBuilder.newBuilder().baseUri(baseUri).build(OtherClient.class); + Response response = otherClient.get(); + assertThat(response.getStatus()).isEqualTo(404); + } + + @Path("/error") + public static class Resource { + @GET + public Response returnError() { + return Response.status(404).build(); + } + } + + @Path("/error") + @RegisterRestClient(configKey = "test") + public interface Client { + @GET + Response get(); + } + + @Path("/error") + public interface OtherClient { + @GET + Response get(); + } +} diff --git a/extensions/resteasy-reactive/rest-client/deployment/src/test/java/io/quarkus/rest/client/reactive/error/PerClientExceptionMapperDisabledTest.java b/extensions/resteasy-reactive/rest-client/deployment/src/test/java/io/quarkus/rest/client/reactive/error/PerClientExceptionMapperDisabledTest.java new file mode 100644 index 0000000000000..8a62ffeb6cc35 --- /dev/null +++ b/extensions/resteasy-reactive/rest-client/deployment/src/test/java/io/quarkus/rest/client/reactive/error/PerClientExceptionMapperDisabledTest.java @@ -0,0 +1,87 @@ +package io.quarkus.rest.client.reactive.error; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.net.URI; + +import jakarta.ws.rs.GET; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.WebApplicationException; +import jakarta.ws.rs.core.Response; + +import org.eclipse.microprofile.rest.client.inject.RegisterRestClient; +import org.eclipse.microprofile.rest.client.inject.RestClient; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +import io.quarkus.rest.client.reactive.QuarkusRestClientBuilder; +import io.quarkus.test.QuarkusUnitTest; +import io.quarkus.test.common.http.TestHTTPResource; + +public class PerClientExceptionMapperDisabledTest { + + @RegisterExtension + static final QuarkusUnitTest TEST = new QuarkusUnitTest() + .withApplicationRoot((jar) -> jar + .addClasses(Client.class, Resource.class)) + .overrideRuntimeConfigKey("quarkus.rest-client.test.url", "${test.url}") + .overrideRuntimeConfigKey("quarkus.rest-client.test.disable-default-mapper", "true") + .overrideRuntimeConfigKey("quarkus.rest-client.test2.url", "${test.url}"); + + @RestClient + Client client; + + @RestClient + Client2 client2; + + @TestHTTPResource + URI baseUri; + + @Test + void testDeclarativeClient() { + Response response = client.get(); + assertThat(response.getStatus()).isEqualTo(404); + + assertThatThrownBy(() -> client2.get()).isInstanceOf(WebApplicationException.class); + } + + @Test + void testProgrammaticClient() { + OtherClient otherClient = QuarkusRestClientBuilder.newBuilder().baseUri(baseUri).disableDefaultMapper(true) + .build(OtherClient.class); + Response response = otherClient.get(); + assertThat(response.getStatus()).isEqualTo(404); + + OtherClient otherClient2 = QuarkusRestClientBuilder.newBuilder().baseUri(baseUri).build(OtherClient.class); + assertThatThrownBy(otherClient2::get).isInstanceOf(WebApplicationException.class); + } + + @Path("/error") + public static class Resource { + @GET + public Response returnError() { + return Response.status(404).build(); + } + } + + @Path("/error") + @RegisterRestClient(configKey = "test") + public interface Client { + @GET + Response get(); + } + + @Path("/error") + @RegisterRestClient(configKey = "test2") + public interface Client2 { + @GET + Response get(); + } + + @Path("/error") + public interface OtherClient { + @GET + Response get(); + } +} diff --git a/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/QuarkusRestClientBuilder.java b/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/QuarkusRestClientBuilder.java index 1b475b298deaa..6ed77f572ee65 100644 --- a/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/QuarkusRestClientBuilder.java +++ b/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/QuarkusRestClientBuilder.java @@ -298,6 +298,12 @@ static QuarkusRestClientBuilder newBuilder() { */ QuarkusRestClientBuilder userAgent(String userAgent); + /** + * If set to {@code true}, then this REST Client will not the default exception mapper which + * always throws an exception if HTTP response code >= 400 + */ + QuarkusRestClientBuilder disableDefaultMapper(Boolean disable); + /** * Based on the configured QuarkusRestClientBuilder, creates a new instance of the given REST interface to invoke API calls * against. diff --git a/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/QuarkusRestClientBuilderImpl.java b/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/QuarkusRestClientBuilderImpl.java index 884f878844e56..f05460ea975d9 100644 --- a/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/QuarkusRestClientBuilderImpl.java +++ b/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/QuarkusRestClientBuilderImpl.java @@ -262,6 +262,12 @@ public QuarkusRestClientBuilder userAgent(String userAgent) { return this; } + @Override + public QuarkusRestClientBuilder disableDefaultMapper(Boolean disable) { + proxy.disableDefaultMapper(disable); + return this; + } + @Override public T build(Class clazz) throws IllegalStateException, RestClientDefinitionException { return proxy.build(clazz); diff --git a/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientBuilderImpl.java b/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientBuilderImpl.java index 3bf91513c9abc..a498404808278 100644 --- a/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientBuilderImpl.java +++ b/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientBuilderImpl.java @@ -88,6 +88,7 @@ public class RestClientBuilderImpl implements RestClientBuilder { private Boolean trustAll; private String userAgent; + private Boolean disableDefaultMapper; @Override public RestClientBuilderImpl baseUrl(URL url) { @@ -260,6 +261,11 @@ public RestClientBuilderImpl userAgent(String userAgent) { return this; } + public RestClientBuilderImpl disableDefaultMapper(Boolean disableDefaultMapper) { + this.disableDefaultMapper = disableDefaultMapper; + return this; + } + @Override public RestClientBuilderImpl executorService(ExecutorService executor) { throw new IllegalArgumentException("Specifying executor service is not supported. " + @@ -429,13 +435,6 @@ public T build(Class aClass) throws IllegalStateException, RestClientDefi register(mapper.getKey(), mapper.getValue()); } - Object defaultMapperDisabled = getConfiguration().getProperty(DEFAULT_MAPPER_DISABLED); - Boolean globallyDisabledMapper = ConfigProvider.getConfig() - .getOptionalValue(DEFAULT_MAPPER_DISABLED, Boolean.class).orElse(false); - if (!globallyDisabledMapper && !(defaultMapperDisabled instanceof Boolean && (Boolean) defaultMapperDisabled)) { - exceptionMappers.add(new DefaultMicroprofileRestClientExceptionMapper()); - } - exceptionMappers.sort(Comparator.comparingInt(ResponseExceptionMapper::getPriority)); redirectHandlers.sort(Comparator.comparingInt(RedirectHandler::getPriority)); clientBuilder.register(new MicroProfileRestClientResponseFilter(exceptionMappers)); @@ -483,6 +482,26 @@ public T build(Class aClass) throws IllegalStateException, RestClientDefi clientBuilder.setUserAgent(restClients.userAgent().get()); } + Boolean effectiveDisableDefaultMapper = disableDefaultMapper; + if (effectiveDisableDefaultMapper == null) { + var configOpt = ConfigProvider.getConfig().getOptionalValue(DEFAULT_MAPPER_DISABLED, Boolean.class); + if (configOpt.isEmpty()) { + // need to support the legacy way where the user does .property("microprofile.rest.client.disable.default.mapper", true) + var defaultMapperDisabledFromProperty = getConfiguration().getProperty(DEFAULT_MAPPER_DISABLED); + if (defaultMapperDisabledFromProperty instanceof Boolean b) { + effectiveDisableDefaultMapper = b; + } else { + effectiveDisableDefaultMapper = false; + } + } else { + effectiveDisableDefaultMapper = configOpt.get(); + } + } + + if (!effectiveDisableDefaultMapper) { + exceptionMappers.add(new DefaultMicroprofileRestClientExceptionMapper()); + } + Integer maxChunkSize = (Integer) getConfiguration().getProperty(QuarkusRestClientProperties.MAX_CHUNK_SIZE); if (maxChunkSize != null) { clientBuilder.maxChunkSize(maxChunkSize); @@ -573,4 +592,5 @@ private MultiQueryParamMode toMultiQueryParamMode(QueryParamStyle queryParamStyl } return null; } + } diff --git a/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientCDIDelegateBuilder.java b/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientCDIDelegateBuilder.java index 721e612255f53..b8f7cad7658d8 100644 --- a/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientCDIDelegateBuilder.java +++ b/extensions/resteasy-reactive/rest-client/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientCDIDelegateBuilder.java @@ -142,6 +142,8 @@ private void configureCustomProperties(QuarkusRestClientBuilder builder) { Boolean captureStacktrace = oneOf(restClientConfig.captureStacktrace()).orElse(configRoot.captureStacktrace()); builder.property(QuarkusRestClientProperties.CAPTURE_STACKTRACE, captureStacktrace); + + builder.disableDefaultMapper(restClientConfig.disableDefaultMapper()); } private static Function intChunkSize() {