Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distinct ObjectMappers for RestEasy Reactive / RestEasy Reactive Client #23979

Closed
chonton opened this issue Feb 25, 2022 · 23 comments · Fixed by #35025
Closed

Distinct ObjectMappers for RestEasy Reactive / RestEasy Reactive Client #23979

chonton opened this issue Feb 25, 2022 · 23 comments · Fixed by #35025
Labels
Milestone

Comments

@chonton
Copy link
Contributor

chonton commented Feb 25, 2022

Description

Quarkus allows a program to use specify a custom Jackson ObjectMapper by using CDI producer method or customize the ObjectMapper with ObjectMapperCustomizer instances.

I need to have different behaviors for RestEasy Reactive Client use and RestEasy Reactive use. Specifically, I want the service implementation to be set with .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, true) and the client to have .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)

Implementation ideas

Perhaps annotate the CDI producer method and the ObjectMapperCustomizer(s) with with a new qualifier:

package io.quarkus.jackson.runtime;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import javax.inject.Qualifier;
import javax.ws.rs.RuntimeType;

@Qualifier
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.TYPE})
public @interface ObjectMapperUse {
  RuntimeType value();
}

How much backwards compatibility is required? Is there a fallback to unqualified producer if qualified producer is unavailable?

@chonton chonton added the kind/enhancement New feature or request label Feb 25, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 25, 2022

/cc @FroMage, @geoand, @stuartwdouglas

@manofthepeace
Copy link
Contributor

Hi I have a need in my application that would need an even more broader solution that the one proposed in the PR. I was playing it it myself but could not make it work. I would be nice if in fact any Jax-rs resource could have it's own ObjectMapper, on demand.

In my, again, special case. I have 2 rests clients, and both are from different hosts. One returns time in a long format (millis), the other in a ISO string.

Jackson does not support out of the box long millis --> zoneddatetime so we wrote out custom deserializer for it, we need to register it as a module, so it can be a bean, so we can inject in it. But since we register our own, we break the one from the JavaTimeModule, which works for the other api.

I was looking at his answer on stackoverflow https://stackoverflow.com/a/63542422 which for me would solve all the case, but unfortunately I cannot make it work.

Having something like this, as a registered provider, would I think solve all the cases I can think of. In my case, the getContext() is never called, so it does not work.. I think having that working, would fix the case from @chonton and mine as well. A jax-rs server that needs a special ObjectMapper could have it this way, multiple could have multiple if really needed, and the same for jax-rs client, could use the default, of the one in the provider context if any. An application can have any number of rest client, so having @ClientObjectMapper makes little sense to me unfortunately, since any hosts can have their own format/needs. I know ObjectMapper is heavy, but in my case I would need 1 for quarkus, 2 for rest clients.

public class ClientObjectMapper1 implements ContextResolver<ObjectMapper> {

    @Named("clientMapper1")
    @Inject
    ObjectMapper mapper;

    @Override
    public ObjectMapper getContext(Class<?> type) {
        return mapper;
    }
}

@hamburml
Copy link
Contributor

@manofthepeace The stackoverflow answer does work with the rest-client dependency but sadly not with rest-client-reactive :(

@westarne
Copy link

westarne commented Jan 17, 2023

Seems like this is related to a problem we encountered. We initially created an issue about ObjectMappers not being specific for the clients #26152 that was seemingly resolved with #27203.

But we now noticed that it is not resolved or not working when using quarkus-resteasy-reactive-jackson in addition to the quarkus-rest-client-reactive-jackson.

If you setup a base project in which this test succeeds (using quarkus-rest-client-reactive-jackson) and then add quarkus-resteasy-reactive-jackson as a dependency, the test fails since the whole ObjectMapper resolving from the context is not called anymore and I currently think that this is a bug or unwanted.

Internally the ServerJacksonMessageBodyReader is used, once you add the dependency, and thus the change to the ClientJacksonMessageBodyReader from the PR is ignored. @Sgitario do you maybe have an idea why this is the case, or how that could be prevented?

Otherwise I think this could be the solution to your and our problem.

@geoand
Copy link
Contributor

geoand commented Jan 17, 2023

We definitely need to address this on our end

Sgitario added a commit to Sgitario/quarkus that referenced this issue Jan 18, 2023
After quarkusio#27203, we can customize the object mappers to be used by REST Client Reactive.
However, because of quarkusio#16368, the implementation was never picked up when the resteasy reactive jackson extension is in place. 
I tried to remove the ResteasyReactiveJacksonProviderDefinedBuildItem build item and surprisingly everything kept working fine (I verified the test that was added as part of  quarkusio#16368.

Fix quarkusio#23979
@Sgitario
Copy link
Contributor

Proposed fix: #30436

Sgitario added a commit to Sgitario/quarkus that referenced this issue Jan 18, 2023
After quarkusio#27203, we can customize the object mappers to be used by REST Client Reactive.
However, because of quarkusio#16368, the implementation was never picked up when the resteasy reactive jackson extension is in place. 
I tried to remove the ResteasyReactiveJacksonProviderDefinedBuildItem build item and surprisingly everything kept working fine (I verified the test that was added as part of  quarkusio#16368.

Fix quarkusio#23979
Sgitario added a commit to Sgitario/quarkus that referenced this issue Jan 18, 2023
After quarkusio#27203, we can customize the object mappers to be used by REST Client Reactive.
However, because of quarkusio#16368, the implementation was never picked up when the resteasy reactive jackson extension is in place. 
I tried to remove the ResteasyReactiveJacksonProviderDefinedBuildItem build item and surprisingly everything kept working fine (I verified the test that was added as part of  quarkusio#16368.

Fix quarkusio#23979
Sgitario added a commit to Sgitario/quarkus that referenced this issue Jan 19, 2023
After quarkusio#27203, we can customize the object mappers to be used by REST Client Reactive.
However, because of quarkusio#16368, the implementation was never picked up when the resteasy reactive jackson extension is in place. 
I tried to remove the ResteasyReactiveJacksonProviderDefinedBuildItem build item and surprisingly everything kept working fine (I verified the test that was added as part of  quarkusio#16368.

Fix quarkusio#23979
@anandbhaskaran
Copy link

Does the proposed fix got merged with any of the latest Quarkus versions? Is there any alternative solution for this in the meantime?

@geoand
Copy link
Contributor

geoand commented Mar 3, 2023

Not yet.

We'll see what we can do for Quarkus 3.1

@Sgitario
Copy link
Contributor

Sgitario commented Mar 9, 2023

I don't follow what is missing here. After #31422 is fixed, we should be able to configure different object mappers by distinct clients and the server. I've written a test case to ensure this is working: #31711

@geoand
Copy link
Contributor

geoand commented Mar 9, 2023

I still don't like the fact that we need a ContextResolver to do this. Ideally we would have a CDI qualifier that would allow users to specify the ObjectMapper that could apply to all or some of the REST Client objects.

It's something I'll look into when I have more time (post Quarkus 3.0 most likely), so let's leave this open

@chonton
Copy link
Contributor Author

chonton commented Jul 22, 2023

@geoand Quarkus 3.2.1 has been released... Do you have time to look at this?

@geoand
Copy link
Contributor

geoand commented Jul 23, 2023

I do want to revisit this for sure!

But it will probably be in September or so

@geoand
Copy link
Contributor

geoand commented Jul 24, 2023

We actually sort of have a way to do this currently that was introduced in #27203.

@Sgitario as ContextResolver is an API that is non-intuitive to use, I wonder if should introduce one or all of the following:

  • A config property (both per client and global) that will allow users to specify some kind of resolver for ObjectMapper
  • An annotation for the same
  • A method in QuarkusRestClientBuilder

WDYT?

@Sgitario
Copy link
Contributor

We actually sort of have a way to do this currently that was introduced in #27203.

@Sgitario as ContextResolver is an API that is non-intuitive to use, I wonder if should introduce one or all of the following:

  • A config property (both per client and global) that will allow users to specify some kind of resolver for ObjectMapper
  • An annotation for the same
  • A method in QuarkusRestClientBuilder

WDYT?

ObjectMapper only makes sense when using the Jackson dependency, so I'm reluctant to add a method for this in QuarkusRestClientBuilder or via config properties which are meant for general-purpose usage only.

Even though we could probably add a config property only for the REST Client Jackson property, I think adding an annotation for this is the way to go.

@geoand
Copy link
Contributor

geoand commented Jul 24, 2023

ObjectMapper only makes sense when using the Jackson dependency, so I'm reluctant to add a method for this in QuarkusRestClientBuilder or via config properties which are meant for general-purpose usage only.

Really good point.

Even though we could probably add a config property only for the REST Client Jackson property, I think adding an annotation for this is the way to go.

Yeah, my only issue is that one would need to add the annotation to every REST Client, which is why I proposed the globalk config property.

@Sgitario
Copy link
Contributor

ObjectMapper only makes sense when using the Jackson dependency, so I'm reluctant to add a method for this in QuarkusRestClientBuilder or via config properties which are meant for general-purpose usage only.

Really good point.

Even though we could probably add a config property only for the REST Client Jackson property, I think adding an annotation for this is the way to go.

Yeah, my only issue is that one would need to add the annotation to every REST Client, which is why I proposed the global config property.

I will work on this (annotation + global property) in a couple of days.

@geoand
Copy link
Contributor

geoand commented Jul 24, 2023

🙏🏼

@geoand
Copy link
Contributor

geoand commented Jul 24, 2023

An alternative to the annotation would be to have a default or static method on the interface which could produce the ObjectMapper (given the global Quarkus one).

@Sgitario
Copy link
Contributor

I started looking into this and let's try first to put all the options here to better decide how to proceed.

At the moment, we can only provide ObjectMappers via ContextResolvers.

This is an example of usage for a single client:

    @Path("/server")
    @Produces(MediaType.APPLICATION_JSON)
    @RegisterProvider(MyCustomObjectMapper.class)
    public interface MyClient {
        @GET
        Request get();
    }

   public static class MyCustomObjectMapper implements ContextResolver<ObjectMapper> {

        @Override
        public ObjectMapper getContext(Class<?> type) {
            return new ObjectMapper();
        }
    }

Or how to use it for all the clients and/or servers (we can use the @ConstrainedTo annotation to mark the usage for client or server only):

   @Provider
   public static class MyCustomObjectMapper implements ContextResolver<ObjectMapper> {

        @Override
        public ObjectMapper getContext(Class<?> type) {
            return new ObjectMapper();
        }
    }

Let's now enumerate the options we have:

  • OPTION A: add a new annotation for ObjectMappers only:
    @Path("/server")
    @Produces(MediaType.APPLICATION_JSON)
    @ObjectMapper(MyCustomObjectMapper.class)
    public interface MyClient {
        @GET
        Request get();
    }

   public class MyCustomObjectMapper implements ObjectMapperSupplier {

        @Override
        public ObjectMapper get() {
            return new ObjectMapper();
        }
    }

Note that the interface ObjectMapperSupplier is simply a public interface ObjectMapperSupplier extends Supplier<ObjectMapper>.

The new annotation @ObjectMapper could also be used for the server.
Internally, it would wrap the provided object mapper instance into the context resolver, so we could reuse most of the existing code and also be compatible with existing APIs like Providers.

To apply an ObjectMapper to multiple clients, we could allow using:

   @ObjectMapper
   @ConstrainedTo(RuntimeType.CLIENT)
   public class MyCustomObjectMapper implements ObjectMapperSupplier {

        @Override
        public ObjectMapper get() {
            return new ObjectMapper();
        }
    }
  • OPTION B: have a default or static method on the interface
    @Path("/server")
    @Produces(MediaType.APPLICATION_JSON)
    public interface MyClient {
        @GET
        Request get();

        default ObjectMapper customObjectMapper() {
           return new ObjectMapper();
        }
    }

Internally, it would work as option A (annotations) and the advantage is that users would not need to learn about new annotations.

The disadvantage is that does not resolve the fact that we can't apply the custom object mapper for multiple clients.

NOTE: I don't see any options to provide the custom object mapper via properties because, at the moment, there are no properties only related to jackson, but for the extensions (jaxb, jackson, jsonb).

@geoand wdyt? I even see options A and B are compatible with each other, though I see option A is more user-friendly. Also, If you see other options, even including properties, let me know.

@geoand
Copy link
Contributor

geoand commented Jul 26, 2023

First of all, I think that none of the options should affect the server, it's far more likely in real applications that the client will need customizations (as it communicates with systems outside the developer's control).

As for which option is better, I think we need to think about the case where an application has multiple rest clients and potentially needs to have a different mapper for each one. In that case, which option makes things easier? Probably B.
Also, I would like us to be able to give the user the original ObjectMapper, so they can call copy (if they so choose) thus inheriting all the settings of the default one.

@Sgitario
Copy link
Contributor

Sgitario commented Jul 26, 2023

As for which option is better, I think we need to think about the case where an application has multiple rest clients and potentially needs to have a different mapper for each one. In that case, which option makes things easier? Probably B.

Also, there are already existing use cases that use the same mechanism (for ex: to provide custom exception mappers).

Also, I would like us to be able to give the user the original ObjectMapper, so they can call copy (if they so choose) thus inheriting all the settings of the default one.

This can be done in both options.

First of all, I think that none of the options should affect the server, it's far more likely in real applications that the client will need customizations (as it communicates with systems outside the developer's control).

Yet, users that want all the clients to use a custom object mapper would need to provide it using:

   @Provider
   @ConstrainedTo(RuntimeType.CLIENT)
   public static class MyCustomObjectMapper implements ContextResolver<ObjectMapper> {

        @Override
        public ObjectMapper getContext(Class<?> type) {
            return new ObjectMapper();
        }
    }

Actually, I would need to verify we can do this and confirm the @ConstrainedTo annotation works, but it would simply be a matter of documenting this use case.

In conclusion, let's go for option B + documenting the case users want to apply the same object mapper for all the clients. We can always implement option A in the future if and only if needed/requested. Are you ok with this?

@geoand
Copy link
Contributor

geoand commented Jul 26, 2023

Yet, users that want all the clients to use a custom object mapper would need to provide it using:

I really don't want users to have to use @ConstrainedTo(RuntimeType.CLIENT) for this. I really want the feature limited to the client only :) Nevermind, I misread this

@geoand
Copy link
Contributor

geoand commented Jul 26, 2023

In conclusion, let's go for option B + documenting the case users want to apply the same object mapper for all the clients. We can always implement option A in the future if and only if needed/requested. Are you ok with this?

+1

Sgitario added a commit to Sgitario/quarkus that referenced this issue Jul 26, 2023
The REST Client Reactive supports adding contextual beans to the Client using the annotation `@ClientProvider` that will be available as part of the `jakarta.ws.rs.ext.Providers` interface in filters/handlers/message readers and writers. 

A simple example is to provide a custom ObjectMapper to the REST Client Reactive Jackson extension by doing:

```java
@path("/extensions")
@RegisterRestClient
public interface ExtensionsService {

    @get
    Set<Extension> getById(@QueryParam("id") String id);

    @ClientProvider <1>
    static ObjectMapper objectMapper(ObjectMapper defaultObjectMapper) { <2>
        return defaultObjectMapper.copy()
                .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
                .disable(DeserializationFeature.UNWRAP_ROOT_VALUE);
    }
}
```

<1> The method must be annotated with `@ClientProvider`.
<2> It's must be a static method. Also, the parameter `defaultObjectMapper` will be resolved via CDI. If not found, it will throw an exception at runtime.

Now, we can resolve the object we provided using the `jakarta.ws.rs.ext.Providers` interface, for example:

```java
@Provider
public class TestClientRequestFilter implements ResteasyReactiveClientRequestFilter {

    @OverRide
    public void filter(ResteasyReactiveClientRequestFilter requestContext) {
        Providers providers = requestContext.getProviders();
        ObjectMapper objectMapper = providers.getContextResolver(ObjectMapper.class, MediaType.APPLICATION_JSON).getContext(null);
        // ...
    }
}
```

NOTE: When providing a custom ObjectMapper, the REST Client Reactive Jackson extension will automatically check the client context to use it. 

Fix quarkusio#23979
Sgitario added a commit to Sgitario/quarkus that referenced this issue Jul 26, 2023
The REST Client Reactive supports adding a custom ObjectMapper to be used only the Client using the annotation `@ClientObjectMapper`. 

A simple example is to provide a custom ObjectMapper to the REST Client Reactive Jackson extension by doing:

```java
@path("/extensions")
@RegisterRestClient
public interface ExtensionsService {

    @get
    Set<Extension> getById(@QueryParam("id") String id);

    @ClientObjectMapper <1>
    static ObjectMapper objectMapper(ObjectMapper defaultObjectMapper) { <2>
        return defaultObjectMapper.copy()
                .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
                .disable(DeserializationFeature.UNWRAP_ROOT_VALUE);
    }
}
```

<1> The method must be annotated with `@ClientObjectMapper`.
<2> It's must be a static method. Also, the parameter `defaultObjectMapper` will be resolved via CDI. If not found, it will throw an exception at runtime.

Fix quarkusio#23979
Sgitario added a commit to Sgitario/quarkus that referenced this issue Jul 26, 2023
The REST Client Reactive supports adding a custom ObjectMapper to be used only the Client using the annotation `@ClientObjectMapper`. 

A simple example is to provide a custom ObjectMapper to the REST Client Reactive Jackson extension by doing:

```java
@path("/extensions")
@RegisterRestClient
public interface ExtensionsService {

    @get
    Set<Extension> getById(@QueryParam("id") String id);

    @ClientObjectMapper <1>
    static ObjectMapper objectMapper(ObjectMapper defaultObjectMapper) { <2>
        return defaultObjectMapper.copy()
                .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
                .disable(DeserializationFeature.UNWRAP_ROOT_VALUE);
    }
}
```

<1> The method must be annotated with `@ClientObjectMapper`.
<2> It's must be a static method. Also, the parameter `defaultObjectMapper` will be resolved via CDI. If not found, it will throw an exception at runtime.

Fix quarkusio#23979
Sgitario added a commit to Sgitario/quarkus that referenced this issue Jul 26, 2023
The REST Client Reactive supports adding a custom ObjectMapper to be used only the Client using the annotation `@ClientObjectMapper`. 

A simple example is to provide a custom ObjectMapper to the REST Client Reactive Jackson extension by doing:

```java
@path("/extensions")
@RegisterRestClient
public interface ExtensionsService {

    @get
    Set<Extension> getById(@QueryParam("id") String id);

    @ClientObjectMapper <1>
    static ObjectMapper objectMapper(ObjectMapper defaultObjectMapper) { <2>
        return defaultObjectMapper.copy()
                .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
                .disable(DeserializationFeature.UNWRAP_ROOT_VALUE);
    }
}
```

<1> The method must be annotated with `@ClientObjectMapper`.
<2> It's must be a static method. Also, the parameter `defaultObjectMapper` will be resolved via CDI. If not found, it will throw an exception at runtime.

Fix quarkusio#23979
Sgitario added a commit to Sgitario/quarkus that referenced this issue Jul 26, 2023
The REST Client Reactive supports adding a custom ObjectMapper to be used only the Client using the annotation `@ClientObjectMapper`. 

A simple example is to provide a custom ObjectMapper to the REST Client Reactive Jackson extension by doing:

```java
@path("/extensions")
@RegisterRestClient
public interface ExtensionsService {

    @get
    Set<Extension> getById(@QueryParam("id") String id);

    @ClientObjectMapper <1>
    static ObjectMapper objectMapper(ObjectMapper defaultObjectMapper) { <2>
        return defaultObjectMapper.copy()
                .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
                .disable(DeserializationFeature.UNWRAP_ROOT_VALUE);
    }
}
```

<1> The method must be annotated with `@ClientObjectMapper`.
<2> It's must be a static method. Also, the parameter `defaultObjectMapper` will be resolved via CDI. If not found, it will throw an exception at runtime.

Fix quarkusio#23979
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment