From 1c65e20b549925e49b41daeabc155325d6265eb6 Mon Sep 17 00:00:00 2001 From: Georgios Andrianakis Date: Mon, 25 Sep 2023 12:28:21 +0300 Subject: [PATCH] Properly cache ContextResolver usage for ObjectMapper in client code Fixes: #36067 --- ...entObjectMapperForClientAndServerTest.java | 34 ++++++++---- .../ClientJacksonMessageBodyReader.java | 38 +++---------- .../ClientJacksonMessageBodyWriter.java | 18 +++---- .../runtime/serialisers/JacksonUtil.java | 53 +++++++++++++++++++ .../runtime/serialisers/ResolverMapKey.java | 53 +++++++++++++++++++ 5 files changed, 143 insertions(+), 53 deletions(-) create mode 100644 extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/JacksonUtil.java create mode 100644 extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ResolverMapKey.java diff --git a/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/src/test/java/io/quarkus/rest/client/reactive/jackson/test/DifferentObjectMapperForClientAndServerTest.java b/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/src/test/java/io/quarkus/rest/client/reactive/jackson/test/DifferentObjectMapperForClientAndServerTest.java index dc444fdd5d9ff..90f3de332246c 100644 --- a/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/src/test/java/io/quarkus/rest/client/reactive/jackson/test/DifferentObjectMapperForClientAndServerTest.java +++ b/extensions/resteasy-reactive/rest-client-reactive-jackson/deployment/src/test/java/io/quarkus/rest/client/reactive/jackson/test/DifferentObjectMapperForClientAndServerTest.java @@ -3,13 +3,11 @@ import static io.restassured.RestAssured.given; import static org.hamcrest.CoreMatchers.equalTo; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertTrue; import java.net.URI; import java.util.Objects; -import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; import jakarta.inject.Singleton; import jakarta.ws.rs.GET; @@ -70,10 +68,17 @@ void serverShouldWrapRootElement() { */ @Test void shouldClientUseCustomObjectMapperUnwrappingRootElement() { - assertFalse(ClientObjectMapperUnwrappingRootElement.USED.get()); + AtomicLong count = ClientObjectMapperUnwrappingRootElement.COUNT; + assertEquals(0, count.get()); Request request = clientUnwrappingRootElement.get(); assertEquals("good", request.value); - assertTrue(ClientObjectMapperUnwrappingRootElement.USED.get()); + assertEquals(1, count.get()); + + assertEquals("good", clientUnwrappingRootElement.get().value); + assertEquals("good", clientUnwrappingRootElement.get().value); + assertEquals("good", clientUnwrappingRootElement.get().value); + // count should not change as the resolution of the ObjectMapper should be cached + assertEquals(1, count.get()); } /** @@ -82,10 +87,17 @@ void shouldClientUseCustomObjectMapperUnwrappingRootElement() { */ @Test void shouldClientUseCustomObjectMapperNotUnwrappingRootElement() { - assertFalse(MyClientNotUnwrappingRootElement.CUSTOM_OBJECT_MAPPER_USED.get()); + AtomicLong count = MyClientNotUnwrappingRootElement.CUSTOM_OBJECT_MAPPER_COUNT; + assertEquals(0, count.get()); Request request = clientNotUnwrappingRootElement.get(); assertNull(request.value); - assertTrue(MyClientNotUnwrappingRootElement.CUSTOM_OBJECT_MAPPER_USED.get()); + assertEquals(1, count.get()); + + assertNull(clientNotUnwrappingRootElement.get().value); + assertNull(clientNotUnwrappingRootElement.get().value); + assertNull(clientNotUnwrappingRootElement.get().value); + // count should not change as the resolution of the ObjectMapper should be cached + assertEquals(1, count.get()); } @Path("/server") @@ -108,14 +120,14 @@ public interface MyClientUnwrappingRootElement { @Path("/server") @Produces(MediaType.APPLICATION_JSON) public interface MyClientNotUnwrappingRootElement { - AtomicBoolean CUSTOM_OBJECT_MAPPER_USED = new AtomicBoolean(false); + AtomicLong CUSTOM_OBJECT_MAPPER_COUNT = new AtomicLong(); @GET Request get(); @ClientObjectMapper static ObjectMapper objectMapper(ObjectMapper defaultObjectMapper) { - CUSTOM_OBJECT_MAPPER_USED.set(true); + CUSTOM_OBJECT_MAPPER_COUNT.incrementAndGet(); return defaultObjectMapper.copy() .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES) .disable(DeserializationFeature.UNWRAP_ROOT_VALUE); @@ -158,11 +170,11 @@ public int hashCode() { } public static class ClientObjectMapperUnwrappingRootElement implements ContextResolver { - static final AtomicBoolean USED = new AtomicBoolean(false); + static final AtomicLong COUNT = new AtomicLong(); @Override public ObjectMapper getContext(Class type) { - USED.set(true); + COUNT.incrementAndGet(); return new ObjectMapper().enable(DeserializationFeature.UNWRAP_ROOT_VALUE); } } diff --git a/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ClientJacksonMessageBodyReader.java b/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ClientJacksonMessageBodyReader.java index ad7e481b8405b..63c4fb8cec20c 100644 --- a/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ClientJacksonMessageBodyReader.java +++ b/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ClientJacksonMessageBodyReader.java @@ -1,5 +1,7 @@ package io.quarkus.rest.client.reactive.jackson.runtime.serialisers; +import static io.quarkus.rest.client.reactive.jackson.runtime.serialisers.JacksonUtil.getObjectMapperFromContext; + import java.io.IOException; import java.io.InputStream; import java.lang.annotation.Annotation; @@ -13,8 +15,6 @@ import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.MultivaluedMap; import jakarta.ws.rs.core.Response; -import jakarta.ws.rs.ext.ContextResolver; -import jakarta.ws.rs.ext.Providers; import org.jboss.logging.Logger; import org.jboss.resteasy.reactive.ClientWebApplicationException; @@ -33,7 +33,8 @@ public class ClientJacksonMessageBodyReader extends JacksonBasicMessageBodyReade private static final Logger log = Logger.getLogger(ClientJacksonMessageBodyReader.class); - private final ConcurrentMap contextResolverMap = new ConcurrentHashMap<>(); + private final ConcurrentMap contextResolverMap = new ConcurrentHashMap<>(); + private final ConcurrentMap objectReaderMap = new ConcurrentHashMap<>(); private RestClientRequestContext context; @Inject @@ -66,43 +67,16 @@ public void handle(RestClientRequestContext requestContext) { } private ObjectReader getEffectiveReader(Class type, MediaType responseMediaType) { - ObjectMapper effectiveMapper = getObjectMapperFromContext(type, responseMediaType); + ObjectMapper effectiveMapper = getObjectMapperFromContext(type, responseMediaType, context, contextResolverMap); if (effectiveMapper == null) { return getEffectiveReader(); } - return contextResolverMap.computeIfAbsent(effectiveMapper, new Function<>() { + return objectReaderMap.computeIfAbsent(effectiveMapper, new Function<>() { @Override public ObjectReader apply(ObjectMapper objectMapper) { return objectMapper.reader(); } }); } - - private ObjectMapper getObjectMapperFromContext(Class type, MediaType responseMediaType) { - Providers providers = getProviders(); - if (providers == null) { - return null; - } - - ContextResolver contextResolver = providers.getContextResolver(ObjectMapper.class, - responseMediaType); - if (contextResolver == null) { - // TODO: not sure if this is correct, but Jackson does this as well... - contextResolver = providers.getContextResolver(ObjectMapper.class, null); - } - if (contextResolver != null) { - return contextResolver.getContext(type); - } - - return null; - } - - private Providers getProviders() { - if (context != null && context.getClientRequestContext() != null) { - return context.getClientRequestContext().getProviders(); - } - - return null; - } } diff --git a/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ClientJacksonMessageBodyWriter.java b/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ClientJacksonMessageBodyWriter.java index 98a52d39391fe..9c71047af4a28 100644 --- a/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ClientJacksonMessageBodyWriter.java +++ b/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ClientJacksonMessageBodyWriter.java @@ -1,5 +1,6 @@ package io.quarkus.rest.client.reactive.jackson.runtime.serialisers; +import static io.quarkus.rest.client.reactive.jackson.runtime.serialisers.JacksonUtil.getObjectMapperFromContext; import static org.jboss.resteasy.reactive.server.jackson.JacksonMessageBodyWriterUtil.createDefaultWriter; import static org.jboss.resteasy.reactive.server.jackson.JacksonMessageBodyWriterUtil.doLegacyWrite; @@ -27,7 +28,8 @@ public class ClientJacksonMessageBodyWriter implements MessageBodyWriter protected final ObjectMapper originalMapper; protected final ObjectWriter defaultWriter; - private final ConcurrentMap contextResolverMap = new ConcurrentHashMap<>(); + private final ConcurrentMap contextResolverMap = new ConcurrentHashMap<>(); + private final ConcurrentMap objectWriterMap = new ConcurrentHashMap<>(); private RestClientRequestContext context; @Inject @@ -44,7 +46,7 @@ public boolean isWriteable(Class type, Type genericType, Annotation[] annotation @Override public void writeTo(Object o, Class type, Type genericType, Annotation[] annotations, MediaType mediaType, MultivaluedMap httpHeaders, OutputStream entityStream) throws IOException, WebApplicationException { - doLegacyWrite(o, annotations, httpHeaders, entityStream, getEffectiveWriter()); + doLegacyWrite(o, annotations, httpHeaders, entityStream, getEffectiveWriter(type, mediaType)); } @Override @@ -52,22 +54,18 @@ public void handle(RestClientRequestContext requestContext) throws Exception { this.context = requestContext; } - protected ObjectWriter getEffectiveWriter() { - if (context == null) { - // no context injected when writer is not running within a rest client context - return defaultWriter; - } - - ObjectMapper objectMapper = context.getConfiguration().getFromContext(ObjectMapper.class); + protected ObjectWriter getEffectiveWriter(Class type, MediaType responseMediaType) { + ObjectMapper objectMapper = getObjectMapperFromContext(type, responseMediaType, context, contextResolverMap); if (objectMapper == null) { return defaultWriter; } - return contextResolverMap.computeIfAbsent(objectMapper, new Function<>() { + return objectWriterMap.computeIfAbsent(objectMapper, new Function<>() { @Override public ObjectWriter apply(ObjectMapper objectMapper) { return createDefaultWriter(objectMapper); } }); } + } diff --git a/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/JacksonUtil.java b/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/JacksonUtil.java new file mode 100644 index 0000000000000..bb8c217f4ac35 --- /dev/null +++ b/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/JacksonUtil.java @@ -0,0 +1,53 @@ +package io.quarkus.rest.client.reactive.jackson.runtime.serialisers; + +import java.util.concurrent.ConcurrentMap; +import java.util.function.Function; + +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.ext.ContextResolver; +import jakarta.ws.rs.ext.Providers; + +import org.jboss.resteasy.reactive.client.impl.RestClientRequestContext; + +import com.fasterxml.jackson.databind.ObjectMapper; + +final class JacksonUtil { + + private JacksonUtil() { + } + + static ObjectMapper getObjectMapperFromContext(Class type, MediaType responseMediaType, RestClientRequestContext context, + ConcurrentMap contextResolverMap) { + Providers providers = getProviders(context); + if (providers == null) { + return null; + } + + ContextResolver contextResolver = providers.getContextResolver(ObjectMapper.class, + responseMediaType); + if (contextResolver == null) { + // TODO: not sure if this is correct, but Jackson does this as well... + contextResolver = providers.getContextResolver(ObjectMapper.class, null); + } + if (contextResolver != null) { + var cr = contextResolver; + var key = new ResolverMapKey(type, context.getConfiguration(), context.getInvokedMethod().getDeclaringClass()); + return contextResolverMap.computeIfAbsent(key, new Function<>() { + @Override + public ObjectMapper apply(ResolverMapKey resolverMapKey) { + return cr.getContext(resolverMapKey.getType()); + } + }); + } + + return null; + } + + private static Providers getProviders(RestClientRequestContext context) { + if (context != null && context.getClientRequestContext() != null) { + return context.getClientRequestContext().getProviders(); + } + + return null; + } +} diff --git a/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ResolverMapKey.java b/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ResolverMapKey.java new file mode 100644 index 0000000000000..02b8148f06b40 --- /dev/null +++ b/extensions/resteasy-reactive/rest-client-reactive-jackson/runtime/src/main/java/io/quarkus/rest/client/reactive/jackson/runtime/serialisers/ResolverMapKey.java @@ -0,0 +1,53 @@ +package io.quarkus.rest.client.reactive.jackson.runtime.serialisers; + +import java.util.Objects; + +import jakarta.ws.rs.core.Configuration; + +/** + * Each REST Client can potentially have different providers, so we need to make sure that + * caching for one client does not affect caching of another + */ +public final class ResolverMapKey { + + private final Class type; + private final Configuration configuration; + + private final Class restClientClass; + + public ResolverMapKey(Class type, Configuration configuration, Class restClientClass) { + this.type = type; + this.configuration = configuration; + this.restClientClass = restClientClass; + } + + public Class getType() { + return type; + } + + public Configuration getConfiguration() { + return configuration; + } + + public Class getRestClientClass() { + return restClientClass; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof ResolverMapKey)) { + return false; + } + ResolverMapKey that = (ResolverMapKey) o; + return Objects.equals(type, that.type) && Objects.equals(configuration, that.configuration) + && Objects.equals(restClientClass, that.restClientClass); + } + + @Override + public int hashCode() { + return Objects.hash(type, configuration, restClientClass); + } +}