ECF / JaxRSProviders

Remote Services distribution provider based upon JaxRS. Includes imples based upon Jersey and CXF.
Apache License 2.0
13 stars 18 forks source link

Custom ObjectMapper not used because of Jackson priority value #25

Closed modular-mind closed 4 years ago

modular-mind commented 4 years ago

I noticed in my sample code that the custom ObjectMapper provided by the ObjectMapperContextResolver was not being used. The setting on that ObjectMapper is DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES = false, and this setting was not being applied.

I'm not sure what the underlying issue is, but I've found that the registration of the JaxRSClientJacksonJaxbJsonProvider (in JaxRSClientJacksonFeature) is unsuccessful when the priority is 1 or higher. Setting the priority to 0 seems to fix the issue.

I can look into this more on my own, but I wanted to see if you have any immediate thoughts. For now, I've added this VM arg to my product configuration:

-Dorg.eclipse.ecf.provider.jaxrs.client.JaxRSClientContainer.jacksonDefaultPriority=0

scottslewis commented 4 years ago

Hi,

At first glance the behavior you are observing is a little confusing. Ultimately the priority of jackson feature and the JaxRSClientJacksonJaxbJsonProvider is done here:

https://github.com/ECF/JaxRSProviders/blob/master/bundles/org.eclipse.ecf.provider.jaxrs.client/src/org/eclipse/ecf/provider/jaxrs/client/JaxRSClientContainer.java#L154

Although looking at it now, not that line 153 has this:

cb.register(new ObjectMapperContextResolver(), ContextResolver.class);

Perhaps since this is missing the jacksonPriority (which has to be set with register(Object,Map<Class,Integer>) version of ClientBuilder.register), it's using the default priority always and this is messing up your registration.

I scratched out this code as a replacement for line 153:

Map<Class<?>,Integer> typeMap = new HashMap<Class<?>,Integer>();
typeMap.put(ContextResolver.class, jacksonPriority);
cb.register(new ObjectMapperContextResolver(), typeMap);

Would you be able to test this in your environment to see if it fixes this issue?

It might be something else of course, but this seems like it could explain what you are experiencing.

Or maybe there is something weird about the handling of the register priority value, or maybe it's being used incorrectly.

This version of things is using the 2.1.1 version of javax.ws.rs-api-2.1.1.

scottslewis commented 4 years ago

Also: I was just looking at these docs for Configurable.register methods

https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/Configurable.html

and they do have this:

Note that in case the priority is not applicable to a particular provider contract implemented by the class of the registered component, the supplied priority value will be ignored for that contract.

I don't know whether this is relevant or not, but it was surprising to me on this reading.

modular-mind commented 4 years ago

Thanks for looking into it. I tried the replacement code but it didn't resolve the issue.

I guess what I'm wondering is whether the priority needs to be set. Was there a particular goal you're trying to achieve by using a priority for all registrations in the context? When I take out the priority for the JaxRSClientJacksonJaxbJsonProvider everything works fine.

scottslewis commented 4 years ago

My goal was simply to allow for prioritization...as supported by Jax-RS spec register methods. It may be that since the feature is registered with a priority that the feature priority overrides anything registered within that feature context...like the JaxRSClientJacksonJaxbJsonProvider.

In any case, I think I'll simplify by deprecating old constructor/3 for JaxRSClientJacksonFeature and replace it with

public JaxRSClientJacksonFeature(RemoteServiceClientRegistration reg, ClassLoader cl) {
    this.reg = reg;
    this.cl = cl;
}
scottslewis commented 4 years ago

Applied fix for this issue by removing Jackson priority from JaxRSClientJacksonFeature constructor (with new constructor and deprecated JaxRSClientJacksonFeature/init/3.