Object Mapper default configuration

[ahoeing]

Hi folks and especially @SDA-SE/sda-commons-maintainer ,

as stated in Jackson Bundle Readme, the object mapper is currently mapped for maximum fault tolerance. But is this the right configuration for deserialization of REST JSON?

Currently, there is an discussion to change the default values for the object mapper in the service as follows:

   environment.getObjectMapper().disable(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_AS_NULL);
    environment
        .getObjectMapper()
        .disable(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_USING_DEFAULT_VALUE);
    environment.getObjectMapper().enable(DeserializationFeature.FAIL_ON_INVALID_SUBTYPE);

    environment.jersey().register(InvalidTypeIdExceptionMapper.class);
    environment.jersey().register(InvalidFormatExceptionMapper.class);

Reason:

• If we define an enum, in the API, why should we not validate that enum during deserialization
• For inheritance in Java, we use Subtypes in JSON. When the consumer uses a not known subtype, then no exception is thrown. Instead of that, there is a high risk on NullPointer Exceptions

The two additional object mappers maps the error to the default SDA error structure.

If you think we should change this default behavior, I would be happy to create the PR.

Thanks for your opinions.

André

[JoergSiebahn]

Imho not failing on deserialization of unknown enum values is a key behavior for backward compatibility. Clients will not fail directly but may fail on the null value. There are many cases where an unknown enum can be silently ignored and the service is still functional without upgrading to a new API. This gives the server the ability to introduce new enum values (with care of course). If you fail on API level by default, you have no chance to ignore unknown enums if they are not important for your case.

[christopher-cudennec]

My impression also was that the configuration is a manifestation of the "tolerant reader" pattern.

Nevertheless you can get backwards compatibility by marking enums as deprecated AFAIK. This would be in sync with changing other parts of the API. With the current configuration it's probably not possible to show a 422 result in case the client sends a wrong value for an enum, right?

[JoergSiebahn]

On server side, you may add @NotNull to the enum for validation. But on the client side you may have an issue when the server introduces new values. And yes, on the server side you should mark enum values you want to drop as deprecated and handle them in a backward compatible way. You may also log their use as warning to be able to inform the consumers and verify when they can be deleted.

[pathec]

Unfortunately, the value still is Null in the service with the default SDA mapper configuration and the @NotNull annotation. Maybe a problem with the order of validation:

1. first check for not null --> all OK because a value is present
2. deserialize the enum --> unknown --> Null

So for a @NotNull annotated enum attribute with an unknown enum the result is Null.

@JoergSiebahn Or did you mean it differently?

[JoergSiebahn]

I was referring to the server side. With @Valid on the method parameter of a REST service and @NotNull on the property, the server will reject a POST body with unknown enum value using 422 for semantical validation error. With the proposed change, it will reject (due to a JsonProcessingException) with 400 for syntax error (as defined in our REST Guide). From my perspective a wrong value is a semantic error, that is achieved with the current implementation.
I added some examples for the server side here.

For a consumer you are right. When a server provides an enum value that is unknown in the consumer model, null will be passed to the caller of the client. Received objects are not validated by default in Jersey Clients. In my opinion it's on the consumer how to handle unknown fields/values. Many consumers can deal without full information and are able to use a fallback. But that is always a business decision for a specific service. Changing the default in the ObjectMapper would make it impossible by default to handle such data because the client will fail on JSON processing. However, if a client should not deal with changed APIs, the client can apply a validator on the received response or do individual validation or fallback implementation on the object. You may want to take a look on this example.