jakartaee / jsonb-api

Jakarta JSON Binding
https://eclipse-ee4j.github.io/jsonb-api/
Other
78 stars 39 forks source link

Specification should clarify the actual serialized format of Map<K,V> #287

Open mkarg opened 2 years ago

mkarg commented 2 years ago

Given JSON-B is serializing a Map<K, V>, currently Yasson produces different serialized formats depending on K:

In fact, it is rather unintuitive and not told anywhere in the spec that case (B) the marshaller introduces the string literals key and value, or that anything else but String keys will turn the map effectively into an array. Users of JSON-B cannot know this, but must have certainty and control over the actual output of any implementation, so a clarification is needed in the specification.

(1) Independent of the question whether this actually is the intended behavior, I request that the specification is amended by a clear wording how the serialized form MUST look like for which cases of K.

(2) In case the literals key and value are intended behavior, I request that the specification declares a way how the user of JSON-B can create output (A) for non-String K without writing a custom serializer and which only solves the Map case but does not chime in for non-maps.

rmannibucau commented 2 years ago

+1 to stick to JSON philosophy and design and not invent a representation which is not natural. It means for me that either we forbid it or we simple use the toString() flavor of the key and stay aligned on "key: json" representation which is the only one JSON knows about. For any other cases we have mecanisms to control the serialization so question is implicit or not (IMHO, the implicit case is not uncommon but can be hard to deserialize so +-0). An alternative is to mention the behavior is out of the spec and not portable (which means: don't use it in your applications until you are bound to a provider), wouldn't be shocking too I think. But the map -> array representation is clearly not a good one because it is not natural it is key/value (name/value is as common AFAIK) and it kind of breaks the shape (object -> array) so is a very particular impl choice.

mkarg commented 2 years ago

My personal opinion is to go with the toString() flavor of the key always and mention that explicitly in the spec. It feels just natural.

mkarg commented 2 years ago

@m0mus WDYT?

mkarg commented 2 years ago

@bravehorsie WDYT?

oliviercailloux commented 2 years ago

If I provide an adapter between K and String I’d like that a Map<K, V> becomes treated as if it was a Map<String, V>. Can this be added to the spec as well?

Perhaps related to #104.

rmannibucau commented 2 years ago

@oliviercailloux don't think we need it since you can write an adapter/(de)serializer for the map or enclosing object reusing JSON-B serialization capabilities so making the map too specific is mainly about making the API verbose but you don't get any feature for end users IMHO.

hontvari commented 1 year ago

This behavior was quite a surprise, the specification tells that maps must be supported, but does not tell how to support them. If the keys are not strings then the reference implementation gives the first impression that is does not support maps, but instead it directly serializes the map implementation.

rmannibucau commented 1 year ago

@hontvari only Map<String, ?> is referenced in the spec, rest does not seem cover but we can surely make explicit JSON-B does not handle not string keys until there is a map adapter/(de)serializer.

hontvari commented 1 year ago

@rmannibucau As far as I see, the spec says in 3.11 that:

Implementations MUST support the binding of the following collection interfaces, classes and their implementations: • java.util.Collection • java.util.Map

It does not mention that only Map<String, ...> is supported. It does mention Map<String, ...> for serialization when the target class is Object.

An idea: if an adapter exists for the key type which converts the key (into a string or into something other) then the implementation should use that adapter to serialize Map keys.

rmannibucau commented 1 year ago

Well, only string would be usable by (json) design and adapters are more designed for values so it would need a way to distinguish between Foo being a key and being a value (assimilated to used alone too).

I clearly think it is easier to stay in one of these cases (last already covers the issue):

  1. Just enforce string like keys (charsequence?)
  2. Register a map<x,y> (de)serializer or adapter

Adding more magic conversion never works smoothly and, on an API standpoint, any global conf must have its local config and I think it is more misleading there than helping (what I'd like to avoid: Map<@JsonbAdapter(...) String, X> or similar).

mkarg commented 1 year ago

It would be nice if someone from the Yasson team would chime in, so we can hear their opinion. @m0mus @Verdent