Resteasy Reactive: ContextResolver<ObjectMapper> not used

[westarne]

Describe the bug

When creating a RestClient with a ContextResolver<ObjectMapper> registered, this ContextResolver is never used and thus the wrong ObjectMapper (via CDI) is used. Other implementation approaches would be fine as well, but nothing seems to get this behaviour working. Multiple different ObjectMappers in the application do not seem to be supported.

Expected behavior

The ObjectMapper returned by a class that implements ContextResolver<ObjectMapper> used via RestClientBuilder#register(Object) is used in the given RestClient and only there. Multiple RestClients can use multiple different ObjectMappers.

Actual behavior

The ObjectMapper of the registered ContextResolver is not used at all. Instead an application scoped ObjectMapper bean is used in all RestClients.

How to Reproduce?

Run the following test class. The test should pass if matching the expected behaviour.

@QuarkusTest
class MyClientTest {

    MyClient clientAllowsUnknown;
    MyClient clientDisallowsUnknown;

    WireMockServer wireMockServer = getWireMockServer();

    @BeforeEach
    void setUp() throws MalformedURLException {
        wireMockServer.resetAll();

        clientAllowsUnknown = RestClientBuilder.newBuilder()
            .baseUrl(new URL(wireMockServer.baseUrl()))
            .register(ClientObjectMapperUnknown.class)
            .build(MyClient.class);

        clientDisallowsUnknown = RestClientBuilder.newBuilder()
            .baseUrl(new URL(wireMockServer.baseUrl()))
            .register(ClientObjectMapperNoUnknown.class)
            .build(MyClient.class);
    }

    @Test
    void something_withAdditionalIgnoredProperties() {
        var json = "{ \"value\": \"someValue\", \"secondValue\": \"toBeIgnored\" }";
        wireMockServer.stubFor(
            WireMock.get(WireMock.urlMatching("/something"))
                .willReturn(okJson(json)));

        var result = clientAllowsUnknown.something().await().indefinitely();

        // FAIL_ON_UNKNOWN_PROPERTIES disabled
        assertThatCode(() -> new ClientObjectMapperUnknown().getContext(ObjectMapper.class).readValue(json, Something.class))
            .doesNotThrowAnyException();
        assertThat(result).isEqualTo(Something.builder().withValue("someValue").build());

        // FAIL_ON_UNKNOWN_PROPERTIES enabled
        assertThatThrownBy(() -> new ClientObjectMapperNoUnknown().getContext(ObjectMapper.class).readValue(json, Something.class))
            .isInstanceOf(JsonProcessingException.class);
        assertThatThrownBy(() -> clientDisallowsUnknown.something().await().indefinitely())
            .isInstanceOf(JsonProcessingException.class);
    }

    @Path("/something")
    @Consumes(MediaType.APPLICATION_JSON)
    @Produces(MediaType.APPLICATION_JSON)
    public interface MyClient {
        @GET
        Uni<Something> something();
    }

    @Value
    @Builder(toBuilder = true, setterPrefix = "with")
    @Jacksonized
    public static class Something {
        String value;
    }

    public static class ClientObjectMapperUnknown implements ContextResolver<ObjectMapper> {
        @Override
        public ObjectMapper getContext(Class<?> type) {
            return new ObjectMapper()
                .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
                .disable(SerializationFeature.FAIL_ON_EMPTY_BEANS);
        }
    }

    public static class ClientObjectMapperNoUnknown implements ContextResolver<ObjectMapper> {
        @Override
        public ObjectMapper getContext(Class<?> type) {
            return new ObjectMapper()
                .enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
                .enable(SerializationFeature.FAIL_ON_EMPTY_BEANS);
        }
    }

    public static WireMockServer getWireMockServer() {
        var wireMockServer = new WireMockServer(options().port(getAvailableTcpPort(20000, 22000)));
        wireMockServer.start();
        return wireMockServer;
    }

    public static int getAvailableTcpPort(int min, int max) {
        var ports = IntStream.range(min, max).boxed().collect(Collectors.toList());
        Collections.shuffle(ports); // shuffle to get a random order and reduce the probability a port is already in use

        for (var port : ports) {
            try (ServerSocket serverSocket = new ServerSocket(port)) {
                return serverSocket.getLocalPort();
            } catch (IOException e) {
                // try next
            }
        }

        throw new IllegalStateException(MessageFormat.format("Could not find a free TCP port in range {0}:{1}.", min, max));
    }
}

With

        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-rest-client-reactive-jackson</artifactId>
        </dependency>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-resteasy-reactive</artifactId>
        </dependency>

        <dependency>
            <groupId>org.projectlombok</groupId>
            <artifactId>lombok</artifactId>
            <version>1.18.22</version>
        </dependency>

Output of uname -a or ver

Darwin M042112251A.local 20.6.0 Darwin Kernel Version 20.6.0: Wed Nov 10 22:23:07 PST 2021; root:xnu-7195.141.14~1/RELEASE_X86_64 x86_64

Output of java -version

openjdk version "11.0.14.1" 2022-02-08 OpenJDK Runtime Environment Temurin-11.0.14.1+1 (build 11.0.14.1+1) OpenJDK 64-Bit Server VM Temurin-11.0.14.1+1 (build 11.0.14.1+1, mixed mode)

GraalVM version (if different from Java)

not used

Quarkus version or git rev

2.8.0.Final, 2.9.2.Final, 2.10.0.CR1

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

Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)

Additional information

No response

[quarkus-bot[bot]]

/cc @FroMage, @geoand, @stuartwdouglas

[Sgitario]

@westarne have you tried to provide your custom ObjectMapper instance by following the instructions in https://quarkus.io/guides/rest-json#json ? Something like this should work:

import com.fasterxml.jackson.databind.ObjectMapper;
import io.quarkus.jackson.ObjectMapperCustomizer;

import javax.enterprise.inject.Instance;
import javax.inject.Singleton;

public class CustomObjectMapper {

    // Replaces the CDI producer for ObjectMapper built into Quarkus
    @Singleton
    ObjectMapper objectMapper(Instance<ObjectMapperCustomizer> customizers) {
        ObjectMapper mapper = myObjectMapper(); // Custom `ObjectMapper`

        // Apply all ObjectMapperCustomizer beans (incl. Quarkus)
        for (ObjectMapperCustomizer customizer : customizers) {
            customizer.customize(mapper);
        }

        return mapper;
    }
}

[westarne]

@westarne have you tried to provide your custom ObjectMapper instance by following the instructions in https://quarkus.io/guides/rest-json#json ? Something like this should work:

import com.fasterxml.jackson.databind.ObjectMapper;
import io.quarkus.jackson.ObjectMapperCustomizer;

import javax.enterprise.inject.Instance;
import javax.inject.Singleton;

public class CustomObjectMapper {

    // Replaces the CDI producer for ObjectMapper built into Quarkus
    @Singleton
    ObjectMapper objectMapper(Instance<ObjectMapperCustomizer> customizers) {
        ObjectMapper mapper = myObjectMapper(); // Custom `ObjectMapper`

        // Apply all ObjectMapperCustomizer beans (incl. Quarkus)
        for (ObjectMapperCustomizer customizer : customizers) {
            customizer.customize(mapper);
        }

        return mapper;
    }
}

Yes, that's working for one object mapper, but not for multiple object mappers with different configurations for each client

[westarne]

Yes, that was still the same behavior for me as far as I remember.

But I wanted to use the same client with different object mappers (for different backends). So I did this by extending the base client and added the @RegisterProvider there.

So something like @RegisterProvier(SpecificObjectMapperProvider.class) public interface SpecificRestClient extends RestClient

[hamburml]

I have a similar issue. I tried to migrate fom resteasy to resteasy-reactive and from rest-client to rest-client-reactive. Every Client has it's own ObjectMapper which is registered via @RegisterProvider(....class). In rest-client this works, in rest-client-reactive it's not working. If a breakpoint is set in the getContext() method you see that it's called only when rest-client is used.

[geoand]

Does someone have a ready made sample I can use to see the problem?

[hamburml]

@geoand You can use this https://github.com/hamburml/context-resolver

When quarkus-resteasy-jackson and quarkus-rest-client-jackson is used the GreetingRestClientMapper works (getContext is called). When using quarkus-resteasy-reactive-jackson, quarkus-resteasy-reactive and quarkus-rest-client-reactive-jackson instead of the older counterparts getContext is not called anymore.

[geoand]

To be honest, I very much dislike the use of ContextResolver to solve such kind of issues... I would much prefer us do something like https://github.com/quarkusio/quarkus/pull/23995

[westarne]

To be honest, I very much dislike the use of ContextResolver to solve such kind of issues... I would much prefer us do something like #23995

As far as I understand that issue, it will still not allow distinct ObjectMappers for different clients, right? And that is exactly what we need. Some object mapper features are just not compatible and cannot be used for multiple different clients. So a separation of server and client is also required (at least to have the possibility), but for systems with a lot of different clients it just does not work with one object mapper for all of these. So what we currently do is have several services for several clients just to be able to combine the info then in another service, which is really a big overhead for such a use case in my opinion.

Obviously it shouldn't be enforced to have several OMs, just the possibility to set a specific OM instance (maybe even in the RestClientBuilder) would be important for us.

[geoand]

Yeah, I understand that and it's completely reasonable. The PR mentioned above does not exactly do that, but it could lay the groundwork for making things easier when it comes to addressing this use case.

I personally think ContextResolver is a very dated API that most people don't know about anyway so we should strive to come up with something more modern and usable.

[geoand]

We could for example use CDI qualifiers on the class, or even utilize a static (or default) method on the interface that would yield an object mapper.

My point is that there are a lot of ways of making this better than relying on the antiquated and arcane ContextResolver API

[gsmet]

A CDI qualifier or an annotation could make sense. In any case, it's a very valid use case given you will get all sorts of JSON formats coming from various apps your clients are targeting.

Now I don't know how hard supporting ContextResolver would be. If not, maybe we should implement it anyway to simplify migrations but only document the more modern approach we want people to have. My concern is that not using the appropriate ObjectMapper might break your app in a very subtle way that you only find out later (and could potentially lead to data loss if you are persisting what comes from the client). Now, if it's hard and complex, it's probably not worth it.

[geoand]

My concern is that not using the appropriate ObjectMapper might break your app in a very subtle way that you only find out later (and could potentially lead to data loss if you are persisting what comes from the client)

Completely agreed, this should be addressed. My point is that I see no reason to resolve it using ContextResolver.

[hamburml]

Thanks for your answers. Looks like we will stop our migration to reactive dependencies till this is configurable. I am not so sure but I think I read somewhere that "resteasy" and "resteasy-reactive" (e.g. "rest-client" and "rest-client-reactive") is compatible to each other which is not the case. Maybe if I find the docs again I send them to you.

[geoand]

They are compatible, but obviously this is a case where things fall short. It will be addressed however (for Quarkus 2.12 or 2.13)

[gsmet]

My point is that I see no reason to resolve it using ContextResolver.

You're missing my point then :) - it might not be enough to justify the work but better make sure you have it. My concern is that people migrating from the REST Client to the Reactive one won't notice until too late that their ContextResolver won't actually be taken into account. It's not as if the failure would always be big enough to notice it. For instance, you could just lose a field that you used to persist or something similar. And realize a few days later that the data is missing. That's my biggest concern about us not supporting the "old" way.

[geoand]

I looked into this a little more and making the ContextResolver thing work for the client is not going to pretty at all... Due to how the APIs are constructed, there is really no integration point at which a MessageBodyReader or MessageBodyWriter can access the JAX-RS Configuration (which is where the ContextResolver is stored for each client). We also can't really rely on the @Context annotation because that would result in obtaining the ContextResolver classes configured for the server (stuff like this is a constant source of bugs in RESTEasy Classic).

So the only way I see to get this functionality is to provide our own ClientMessageBodyReader and ClientMessageBodyWriter interfaces that have access to the necessary information in the reader / writer methods. That would suffice for us to then change the our Jackson related readers / writers to lookup the matching ContextResolver classes.

This is an invasive change so I won't be doing it without the agreement of @michalszynkiewicz and @Sgitario

[hamburml]

I also must say that without our api gateway which validates the request body against a scheme I probably would have missed that ContextResolver doesn't work. For me it is 100% important that it works like before (or there is an even better way how it's done). Thanks for your work @geoand

[geoand]

Understood, thanks.

[geoand]

https://github.com/quarkusio/quarkus/pull/26988 takes care of the server part.

For the client part, this comment still applies.

[hamburml]

@michalszynkiewicz and @Sgitario Could you please give a statement to this issue? Thanks!

[geoand]

They are both on PTO IIRC

[Sgitario]

I looked into this issue before my PTO and shared the same thoughts as @geoand . The most straightforward approach is to provide a custom message reader / writer that checks for custom context resolvers objects. However, this is not a general solution which is ok (it would only work for the jackson extension), and we also need to take into account the performance pitfall of checking for the custom context resolvers.

[geoand]

and we also need to take into account the performance pitfall of checking for the custom context resolvers.

We can alleviate this in the same way I did it for the server

[Sgitario]

and we also need to take into account the performance pitfall of checking for the custom context resolvers.

We can alleviate this in the same way I did it for the server

Sure. I will do the changes on the client side.

[geoand]

Thanks!

[timonz-de]

Maybe I don't understand the comments correctly, but to me it reads like ContextResolver<ObjectMapper> should work in resteasy-reactive like in resteasy classic. I am currently switching our dependencies to resteasy-reactive and have the same problem as described in this issue. The provider is not called (Quarkus 2.16.3.Final).

[hamburml]

@timonz-de You are right, it does not work. I just tried it. Maybe you should stick to the non-reactive dependencies. @gsmet @Sgitario You could take a look please? I used https://github.com/hamburml/context-resolver, used the reactive dependencies and updates to new quarkus version and tried it. GreetingRestClientMapper::getContext is never called

[Sgitario]

The CDI support was partially fixed by https://github.com/quarkusio/quarkus/commit/b60b23def77cbd4b31896259239ea46784abee25 but, I've just spotted another issue that will be fixed in https://github.com/quarkusio/quarkus/pull/31422.

[hamburml]

Sorry, just saw your edit. I answered to the mail :) Thx for fixing so fast.