Issue #283: Deserializing Map with enum keys results in runtime strin…

[rmartinc]

Issue: #283

The main change is that now the MapDeserializer tries to parse the String key into an object using the parameter type. Comments:

• A new config property yasson.mapToObjectSerializer.nullKey was added to specify a custom key string for the null key. By default it is "null" as before but it can be modified to other value (just in case). This is not exactly part of the issue but I think null keys are also not managed OK in this map serializer/deserializer. We can remove this part if you prefer.
• In order to have the Unmarshaller in the addResult a new variant for the method has been added to the AbstractContainerDeserializer. The default implementation calls to the variant without the context. This way the MapDeserializer can use the passed context to parse the key.
• Tests added to MapToObjectSerializerTest for several map types and null keys.

[Verdent]

I am still not sure how to address this new property. I know what you are trying to achieve here, but what if you have large amount of maps with different keys? Such as Boolean, String, Integer, Date, UUID etc. The problem here is that this config option would be used to all of them and lets say it is set to "1". This wouldn't make much sense I think :-) Am I getting this right?

[rmartinc]

All the maps (no matter the key type) can have a null in the key. That key should be represented by a string value (now it's "null" at serializing but it's never read in the deserialization part, the string is just read like that, "null" string not the null value, so the null value is lost right now). The idea was having a property you can change the key value for null (and yes, globally). By default it is "null" as now. It can be something like "yasson.null" which is more complicated to be a valid String key (a real key which is that string after serialization). I thought that a global conf was enough but if you see a better idea just let me know.

But, as I commented in the description, this is an extra to allow the null key to be configurable (use a different string in case it collides with a real key). We can just remove this part and use always "null" or other fixed value.

I'll re-do the PR with all the changes when we agree what to do with this point. :-)

Thanks a lot for looking this!

[Verdent]

Oh I miss looked here. it is serializer stuff and I was actually talking about deserialization. Sorry. But anyway we should be able to get the same Map from the json as we were using to create it. So even though we will use the substitute token to replace null value key, it is a bit unclear to me how it would behave in terms of deserialization (to obtain the same Map as we had before) :-) I will try to think about it a bit, but so far I do not exactly see any clear solution.

[rmartinc]

Yes, exactly. The property is used in both: the serializer, when null is detected the string used as key is obtained from that property; and the deserializer, when the key read is exactly the property value it is considered the null key. The property is just added to let the user define a new value if it collides with a real key (as in json the key is a string, hypothetically it can always collide with a real key). And the same, I used default "null" value because it was used before, but we can choose a very weird default string to minimize the problem ("org.eclipse.yasson.null-key" for example).

[rmartinc]

@Verdent Maybe we are missing the obvious solution. The problem is that the MapToObjectSerializer ({"key1":"value1",...}) cannot represent null keys. Just because JSON doesn't allow null keys ({null:"value1"}is not valid), only strings, but java allows them. So this comment in the serializer is wrong.

What if we don't use this impl if there is a null key in the map and use the MapToEntriesArraySerializer one? This avoids all the property stuff, and, in the end, null keys are not managed OK right now, so we are not breaking anything that was not broken before.

[Verdent]

@rmartinc I kind of like the idea of doing this the way you proposed now. The only problem is, that it is backwards incompatible change. What we should probably do is to allow this enhancement via implementation specific config property. Default behavior for this null handling would be as is right now, but if anyone knows what he is doing, I think that having a way to change the null handling is fine.

[rmartinc]

@Verdent OK, perfect, I'll add a new property as I did previously with my first PR but using a boolean prop to revert the old behavior. I'll update the PR probably on Monday. :-)

[rmartinc]

Done!

A new property yasson.map-object-serializer-for-null-keys was defined (default value is false). I also added a new test in SerializersTest.java called testSerializeMapWithNullsBackwardsCompatible which tests that the same output is got when serializing a Map with a null key and the prop is set to true (previous behavior).

[Verdent]

Thank you for the change, but the problem is, that the "new" behavior has to be enabled via property and not used by default. :-)

[rmartinc]

Argh! I understood the opposite, OK, it's modified now. Changed the property name to yasson.force-map-array-serializer-for-null-keys and the tests now expect the new output only when that prop is set.

Just a note, right now a null key would be transformed to String "null" if the MapToObjectSreializer is chosen, which mainly means that the serialization/deserialization for that map is broken. That's probably why I understood that I had to set the new behavior as the default one. But as nulls as map keys are rare (and a bad practice) I don't think this will be an important problem. The new property can be set to make it work.

[rmartinc]

@Verdent I was brave and I decided to go with my proposal for the null key. Now the MapToEntriesArraySerializer is used if there is a null key in the map. Take a look, I saved the previous branch with another name if we need it.

[rmartinc]

@Verdent News about this? Do you see everything OK now or are you waiting some more action from my side? Thanks!

[rmartinc]

@Verdent Sorry for asking again, but is this OK? If that's the case I can start preparing the PR for the 1.x branch...

[Verdent]

@rmartinc Sorry for late response. I am just having too much work on other project. Yes, I think the change is OK. :-) I am just thinking if we should not mark it as experimental for a while (to be able to change it, if we find out that we have missed something)

[rmartinc]

@Verdent OK, no problem, whatever you prefer. I don't know what marking as experimental means, but if you need something on the PR just let me know. Thanks!

[Verdent]

LGTM. Thank you for patience. Now I am finally starting to have free Yasson time :-)

[rmartinc]

@Verdent Do I prepare the same for 1.x branch too?