Feature/issue 185 WIP

[jabrena]

Starting point to fix the issue:
#185

Hi @pivovarit,

Can you review a potential solution for this scenario?
https://github.com/jabrena/vavr-jackson/blob/feature/issue-185/src/test/java/io/vavr/jackson/issues/issue185/VavrJsonMergeMapDeserializer.java

If you like the approach, I could stress the concept adding several tests to review more datatypes and internal complexity.

Juan Antonio

[pivovarit]

We don't need two separate map deserializers, right? Just one that's JsonMerge-friendly - would be good to make sure that the new deserializer implementation passes all the existing tests as well - then we could simply swap the implementation with the new one

[jabrena]

Hi @pivovarit,

as we discussed, the PR offer an example in case of an user require to interact with @ jsonMerge but implement a complete solution with support for that annotation is pretty expensive.

We should merge this PR and add an example for that scenario.

[pivovarit]

But why can't we integrate it with the existing Map deserializer?

[jabrena]

Current VAVR Module support all Data Structures from the library for Serialize/Deserialize:

    //Example
    ObjectMapper mapper = new ObjectMapper()
      .configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true)
      .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
      .setDefaultPropertyInclusion(Include.NON_EMPTY)
      .registerModule(new VavrModule(new Settings().deserializeNullAsEmptyCollection(true)));

But add JsonMerge support will create an extra effort to update the Module and not only to support that Types, if the library, accept JsonMerge support, it should be necessary to accept it for the rest of the types.

I consider that it is better to maintain the Module in current state and maybe in the future evolve current support using some specific subclasses like ContainerDeserializerBase<T>

• https://www.javadoc.io/doc/com.fasterxml.jackson.core/jackson-databind/latest/com/fasterxml/jackson/databind/deser/std/ContainerDeserializerBase.html
• https://www.javadoc.io/doc/com.fasterxml.jackson.core/jackson-databind/latest/com/fasterxml/jackson/databind/deser/std/MapDeserializer.html

For users who require this behaviour, the library add an example as a Workaround in the tests related to this issue.

As a summary:

• It is possible to Merge adding a custom serializer but generilize the support, not only will have to be reviewed for that Types, it is necessary to be reviewed for all VAVR Data structures and in the another hand, how many users use that annotation? This is the reason that the example in the test it could be valuable for this tiny audience using that annotation but for the general audience with current Module implementation is enough IMHO.

Juan Antonio

[pivovarit]

The new serializer needs to be compatible with the existing one - otherwise, people won't find it helpful.

So, there are two options here:

• we patch the existing one with JsonMerge support and refactor it later
• the new serializer matches the behaviour of the original one +/- JsonMerge support (which means that it should be tested using existing tests)

I believe that it's much better for users to just roll forward with the first option until we're sure that the new one doesn't introduce breaking changes by accident. There's also no point in keeping a non-backward compatible serializer just as an example because if users try to use it and it doesn't work just the existing one, they will get mad.

First option is already implemented here: #238

[jabrena]

Oki, you won :p

In that case, maybe it doesn´t have any sense to maintain opened this PR, but I will take a look the changes and I will review current test in order to see if something is possible to be reinforced.

Agree?

[pivovarit]

Sounds good!