Allows customizing the ObjectMapper in REST Client Reactive Jackson

[Sgitario]

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:

@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.

Closes #23979

[Sgitario]

@geoand let me know what you think about this solution:

• it follows the same pattern as what was done for @ClientExceptionMapper or @ClientRedirectHandler
• instead of providing a specific solution for Object Mappers, I thought it was a better idea to find a general solution for any kind of service by adding @ClientProvider which is fully integrated by REST Client Jackson and the Providers interface (I've also updated the documentation to guide users how to provide a custom object mapper per client)

[geoand]

To be honest, I don't see why would call the annotation @ClientProvider and give the illusion that it's generic.
For Jakarta REST Provider classes, users can already use @RegisterProvider, so I think the name is very misleading.

[Sgitario]

Provider

@ClientProvider is equal to @Provider but for the client only and for methods. Any alternative name that came up to you?

[geoand]

What I am saying is that I prefer we have a specific annotation, just like we do (as you pointed out) for @ClientExceptionMapper and others.

Also keep in mind, that the name Provider means pretty much nothing to most people (let's not forget that most people never have to use them in Jakarta REST)

[Sgitario]

What I am saying is that I prefer we have a specific annotation, just like we do (as you pointed out) for @ClientExceptionMapper and others.

Also keep in mind, that the name Provider means pretty much nothing to most people (let's not forget that most people never have to use them in Jakarta REST)

Do you mean to reduce the scope of the feature to only support object mappers? And so, provide a specific annotation @ClientObjectMapper.

If so, I encounter the current solution as much more powerful. The existing annotation allows adding beans to the client context in a generic way. Though, we could discuss the name (perhaps @RegisterContextResolver or sth similar).

I also like having something @ClientObjectMapper, but without reducing the scope of these changes. This is having this annotation as a synonym of the @ClientProvider annotation (or how we agree to name it).

[geoand]

Do you mean to reduce the scope of the feature to only support object mappers? And so, provide a specific annotation @ClientObjectMapper

Yes

If so, I encounter the current solution as much more powerful. The existing annotation allows adding beans to the client context in a generic way. Though, we could discuss the name (perhaps @RegisterContextResolver or sth similar

The problem is that these non-Quarkus APIs are very arcance to use (hence the original issue in the first place).
As these are not APIs we would have come up with if we were starting from scratch, I don't really want to have more way to expose these.

Having said that, if you want to keep the "infrastructure" you created to support it and simply not "expose" it, that's fine with me (as long as the design decision is mentioned properly in the code comments)

[Sgitario]

Do you mean to reduce the scope of the feature to only support object mappers? And so, provide a specific annotation @ClientObjectMapper

Yes

If so, I encounter the current solution as much more powerful. The existing annotation allows adding beans to the client context in a generic way. Though, we could discuss the name (perhaps @RegisterContextResolver or sth similar

The problem is that these non-Quarkus APIs are very arcance to use (hence the original issue in the first place). As these are not APIs we would have come up with if we were starting from scratch, I don't really want to have more way to expose these.

What non-Quarkus APIs do you mean? The Providers interface and ContextResolver are coming from the JAX-RS specification, so the more we are integrated with it, the better (because users already know it).

Moreover, we're documenting this use case, so from a user's perspective, it does exist and it's up to the users, to use it or not.

[geoand]

What non-Quarkus APIs do you mean? The Providers interface and ContextResolver are coming from the JAX-RS specification

These are what I am talking about

so the more we are integrated with it, the better (because users already know it)

We already have integration with them. What I am saying that is they are terrible APIs and we should not be trying to make things easier for users by exposing them more.

[Sgitario]

What non-Quarkus APIs do you mean? The Providers interface and ContextResolver are coming from the JAX-RS specification

These are what I am talking about

so the more we are integrated with it, the better (because users already know it)

We already have integration with them. What I am saying that is they are terrible APIs and we should not be trying to make things easier for users by exposing them more.

This is a biased argument ;)

But yes, I agree with you that having to do something like this providers.getContextResolver(ObjectMapper.class, MediaType.APPLICATION_JSON).getContext(null); is not great.

I've already updated the solution and will update the pull request once all the checks are green in my local.

[geoand]

This is a biased argument ;)

It is :). But I also get to see 10s of issues, chats, SO questions and reproducers everyday so it's not a totally uninformed opinion :)

[Sgitario]

PR updated and PR title/description updated @geoand

[geoand]

We should mention here that users should NEVER change the defaultObjectMapper but instead always use copy if they pan to inherit the default settings.
The same goes for the Javadoc of @ClientObjectMapper

[Sgitario]

Added

[Sgitario]

PR updated with your comments.

[geoand]

Suggested change

	<3> In this example, we're creating a copy of the default object mapper. You should *NEVER* modify the default object mapper, but creating a copy instead.
	<3> In this example, we're creating a copy of the default object mapper. You should *NEVER* modify the default object mapper, but create a copy instead.

[Sgitario]

Changed.

[geoand]

It probably makes sense to also change something here.

[Sgitario]

Yes, it makes sense. Added.

[Sgitario]

PR updated again with your latest comments.

[geoand]

Thanks!

[github-actions]

🙈 The PR is closed and the preview is expired.

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.