Json customization via MapperFactory doesn't work

[yurybubnov]

Describe the bug It is impossible to customize JSON serialization/deserialization using https://github.com/pingidentity/scim2/blob/master/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/MapperFactory.java and https://github.com/pingidentity/scim2/blob/master/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/JsonUtils.java#L1049

To Reproduce Calling https://github.com/pingidentity/scim2/blob/master/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/JsonUtils.java#L1049 will not have any useful effect since https://github.com/pingidentity/scim2/blob/master/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/MapperFactory.java#L139 method is static and linked at compile time. Thus https://github.com/pingidentity/scim2/blob/master/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/JsonUtils.java#L1052 will always call createObjectMapper of MapperFactory and passed customMapperFactory will be effectively ignored.

Expected behavior SDK_OBJECT_MAPPER should be assigned new value from customMapperFactory

Additional context Right now it is incompatible with Okta's version of SCIM messages. Namely, fields roles and entitlements. This fix allows customized json payload so the library is compatible

Add any other context about the problem here. For example:

• Java version:13
• SCIM 2 SDK version: [e.g. 2.3.4, master branch]

Solution All fields in MapperFactory and method createObjectMapper should not be static. Just removing static solves the problem

[edysli]

Please fix this, it makes this SDK harder to use with Spring Boot since both instantiate ObjectMappers. Which one is actually used? If it were configurable, I'd be able to inject the Spring Boot-created ObjectMapper into this SDK. Moreover, given that the loaded Jackson modules are hardcoded in https://github.com/pingidentity/scim2/blob/e4cdd2d552144c00f34f8fcfaf22b6ae1326b227/scim2-sdk-common/src/main/java/com/unboundid/scim2/common/utils/MapperFactory.java#L139, Jackson's JavaDateTime module to support Java8 DateTime types cannot be used, preventing correct serialisation of java.time.Instant for example.

[kqarryzada]

@yurybubnov, I've spent some time looking into this issue. I think it does make sense for those fields to be non-static. However, in practice, I don't see many situations where you'd have more than one MapperFactory configuration, so I was curious about the problem you described.

I tried to reproduce this with a new basic Java application, but I was able to configure the SCIM SDK's object mapper without any problems. I've written the following (Java 17) test application:

// The 'schemas' parameter should be an array, so this JSON is technically
// invalid.
String jsonString = """
        {
          "schemas": "urn:ietf:params:scim:schemas:core:2.0:User",
          "userName": "kenny"
        }
        """;

// Convert the JSON to a UserResource with the default settings. This is
// expected to fail.
try {
  JsonUtils.getObjectReader().forType(UserResource.class).readValue(jsonString);
  throw new RuntimeException();
}
catch (JsonProcessingException e) {
  // Expected.
}

// Customize the SCIM SDK to allow strings for array parameters. This should
// allow the 'schemas' string above to be interpreted as a single-valued array.
MapperFactory factory = new MapperFactory();
factory.setDeserializationCustomFeatures(Map.of(ACCEPT_SINGLE_VALUE_AS_ARRAY, true));
JsonUtils.setCustomMapperFactory(factory);

// Attempt the same JSON conversion again. If this conversion fails, an
// JsonProcessingException will be thrown.
UserResource user = JsonUtils.getObjectReader().forType(UserResource.class).readValue(jsonString);
assert "kenny".equals(user.getUserName());
System.out.println(user.getUserName());

The output of the above code snippet is kenny, as expected. This code makes two attempts to convert the string into a UserResource, and only succeeds in the second case, indicating that it is possible to update the object mapper configuration.

Could you give an example on what types of responses you're receiving from Okta's SCIM provisioner, as well as some code that reproduces the problem? From a glance at their documentation, I didn't immediately see anything about the roles attribute that would be incompatible.

[kqarryzada]

...it makes this SDK harder to use with Spring Boot since both instantiate ObjectMappers. Which one is actually used?

@edysli, When you use the SCIM SDK in a Spring Boot project, the operations within the SCIM SDK's library will use the SCIM SDK's internal ObjectMapper instance. Spring, however, is configured to use its own object mapper. So by default, Spring will use its own object mapper to convert HTTP requests and responses that are captured in the Controller layer, which is likely not what you want for a project that accepts and returns SCIM requests/responses.

I wasn't able to reproduce an issue with updating the SCIM SDK's configuration (see my previous comment). Regardless, if you want to have a consistent configuration between the two ObjectMapper types (which is a good idea), you can do the following:

• Grab an MapperFactory from the SCIM SDK with new MapperFactory()
• Update it to use the desired configuration
• Update the SCIM SDK's configuration with JsonUtils.setCustomMapperFactory()
• Fetch a new ObjectMapper from the SCIM SDK with JsonUtils.createObjectMapper(), which will have the new configuration
• Register the new ObjectMapper as a Spring Bean so that your Spring app uses the same settings

There's some more info on this in our newly-published FAQ that might help.

[yurybubnov]

@kqarryzada this only works because call factory.setDeserializationCustomFeatures also updates static field and call to setCustomMapperFactory uses same ObjectMapper. Now, consider you need to use your own ObjectMapper. For example, call .enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES) For this you need to extend MapperFactory and override createObjectMapper() but this won't make any difference since createObjectMapper is static

[kqarryzada]

Thanks for the quick reply, @yurybubnov. That MapperFeature should be configurable on the latest version of the SDK with:

MapperFactory factory = new MapperFactory().setMapperCustomFeatures(
    Map.of(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES, true)
);
JsonUtils.setCustomMapperFactory(factory);

The SCIM SDK already enables this property, but I did test that it was possible to set the value to false.

You're correct that the current implementation doesn't allow you to provide your own pre-configured ObjectMapper. But even if this was allowed, I think you would want to fetch an ObjectMapper from the superclass and then modify the pre-built one to suit your needs. Fields like ACCEPT_CASE_INSENSITIVE_PROPERTIES and the timestamp formatting that the SCIM SDK provides are unlikely to require changes, so providing a new ObjectMapper from scratch can easily neglect these important settings. Are there other customizations that you've been unable to make?

I'm not opposed to making these fields non-static and possible to override, and I'm inclined to do this anyway. But I'd like to understand the problem better.

[yurybubnov]

During my implementation of SCIM, I had to provide a custom implementation of Roles and Entitlement. Unfortunately, RFC doesn't specify this part, and, for example, Okta and Azure have different understandings of the Roles object. Also, different IDPs send parameters' names and values in different case format. So I have following customization of the ObjectMapper

       module.addSerializer(Entitlement.class, new EntitlementSerializer());
        module.addDeserializer(Entitlement.class, new EntitlementDeserializer());
        module.addSerializer(Role.class, new RoleSerializer());
        module.addDeserializer(Role.class, new RoleDeserializer());
        mapper.registerModule(module);
        mapper.enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES);
        mapper.enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_VALUES);
        mapper.enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS);

[kqarryzada]

Thanks for the background. I think it makes sense to allow custom serializers/deserializers, so I plan to work on supporting this soon.