fix(JSON): JSONify Java maps as JSON dicts

[ettersi]

Currently, GenericValues that happen to be Java Maps are JSONified as generic objects, not JSON maps. This is inconsistent with how Java Maps are serialized elsewhere (e.g. in ParamChangeNtf), leading to much confusion. This PR changes the GenericValue serialization code so Java maps are JSONified as JSON maps, with imho is the correct behavior. Unfortunately, this change may break code that relied on the old behavior.

[mchitre]

Do we have code that uses Maps as generic values? Can this be made backward compatible?

[ettersi]

Do we have code that uses Maps as generic values?

SWIS makes use of this.

Can this be made backward compatible?

I guess we could introduce a ParameterRsp2 type for the new behavior. But that's just SemVer at the type rather than the package level.

[notthetup]

Do we have code that uses Maps as generic values?

I am guessing this happens if we have an Agent parameter whose value is of a type Map?

IMO we should formally define the types that are supported for Parameters in the "On the wire" protocol for fjåge. If we don't want to support Map/Object then we should specifically document that and also throw exceptions when that does happen.

[mchitre]

I have, in the past, avoided complex objects as parameters, and used maps sparingly if at all. I find this to be easiest to support across languages, especially C, etc. So my vote to limit parameter types.

[notthetup]

If the C gateway is the only issue, another option could be to support it everywhere except the C gateway and then log a warning when the C gateway encounters a Map as a Parameter value

[ettersi]

The concrete use case which triggered this feature request is SWIS, where we have a variable number of seabed drivers and for each driver we want to expose a name, file list, sensor data, etc. Currently, I represent this data as a multi-level dictionary of the form seabedState[driver]["fileList" | "sensorData" | ... ]. I guess I could somehow break down this data structure into simpler components, but I suspect the result would be pretty messy and hard to use. In cases like these, I don't see the harm in supporting map types. It seems very likely that any language that would ever have to interact with sdc will support dictionaries out of the box (in particular, both GSON and the JS JSON library do), and conversely, it seems very unlikely that we would ever have to interact with sdc from C, which AFAICT is the only relevant language lacking proper dictionary support.

[notthetup]

One way to deal with this without using Maps in fjåge is to use index-ed parameters and a secondary parameter for to map the driver name to an index.

sdc.drivers => ["foo", "bar", "baz"]
sdc[1].filelist => [....]  // file list for "foo" driver"
sdc[1].sensorData => [...] // sensor data
sdc[3].filelist => [....]  // file list for "baz" driver"

It ugly but it should work without any changes.

...

The other way would be to use custom messages that can then carry a Map as a field, instead of Parameters. I believe that should work on the wire right?

As for C, while it doesn't have native Map support, I am sure we can depend on some nice simple library that could give us that. But we'll need something that also potentially can deal with JSON parsing of those Maps. I don't think dropping support for the C gateway is an option. We could temporarily drop the support for Map parameters in the C Gateway, but we'll have to support it soon enough.

[mchitre]

The concrete use case which triggered this feature request is SWIS, where we have a variable number of seabed drivers and for each driver we want to expose a name, file list, sensor data, etc. Currently, I represent this data as a multi-level dictionary of the form seabedState[driver]["fileList" | "sensorData" | ... ].

This use case sounds like a classic case for indexed parameters. Each driver would be an index, and keys would be parameter names.

While I don't have a strong objection to supporting maps as parameter values, I think we should reuse existing mechanisms where possible, and only add complexity where necessary.

The other advantage of indexed parameters is that the display in shell is much nicer than big maps as parameters. That will get unwieldy quickly.

[ettersi]

I don't agree that fixing the map support is the more complicated solution to the problem at hand. From my point of view, the situation is as follows.

• I could spend days writing code for converting the data produced by SWIS into and out of the fjage indexed parameter API. Because this code is highly customised, it would likely have a ton of mishandled edge-cases, it would have poor error handling, it would be hard to update if we decide to change any part of the data structure, and it would be highly project-specific meaning once we move on to the next project we'd more or less start the whole dance from scratch.

• We could make a small change to fjage which would basically just fix a feature that already existed (already in current fjage you can expose Map parameters, they are just JSONified in a way that you need an additional .data to get to the map that you want). Doing so would relieve us from all the complications of serialising and deserialising complicated data structures in a reliable and maintainable way, and it would allow downstream code to easily apply all the standard map tooling (key lookups, for each loops, filters, etc...) to the result that they get.

---

The other way would be to use custom messages that can then carry a Map as a field, instead of Parameters. I believe that should work on the wire right?

Custom messages would in principle work, but then we would have to also customise all the tooling around the fjage parameter mechanism (e.g. the agentParams JS function and the ObservableParameter groovy class). Also, allowing maps in some placed but not in others sounds like quite the foot gun to me.

---

I don't think dropping support for the C gateway is an option. We could temporarily drop the support for Map parameters in the C Gateway, but we'll have to support it soon enough.

I'm not proposing to discontinue the C gateway, of course, but I think adding support for maps to all gateways except C would be a workable compromise. All we would have to do for this is make sure that the C gateway correctly skips any maps it may encounter, and this should be fairly straightforward (you just count the number of { and } and keep reading until they cancel out). It seems quite unlikely that anyone would ever want to handle complicated application data in C, but in other languages this need arises constantly and maps are a very useful and very widely supported tool for this.

[mchitre]

I am OK adding Map support to parameters if that suits your needs, as long as it does not break existing JSON used by all users all around the world. Breaking on-wire protocol such that gateways fail other users of fjåge is not to be taken lightly.

Can you please add some examples of the JSON that would have been created before this PR and after this PR? Also, I believe Map works in parameters at present, right? So it would be good to articulate what this PR fixes and why it is important to fix (and what its costs/implications are, e.g., backward compatibility).

[notthetup]

It seems quite unlikely that anyone would ever want to handle complicated application data in C, but in other languages this need arises constantly and maps are a very useful and very widely supported tool for this.

I would like to disagree on this statement. I believe many users of fjåge fall into this category. I will agree they're not very well served by the design decisions of the project, but alienating them further is likely going to cause much more harm.

adding support for maps to all gateways except C would be a workable compromise

I'm OK with this as long as it's well-documented.

Also, allowing maps in some places but not in others sounds like quite the foot gun to me.

This is a good point. We should think through the design of this carefully as well. Does the serialization of a field in a subclass of a Message and a value of a ParameterChangeNtf follow the same logic? They're both fields of a Message in the end. So the behaviour should be similar in with respect to serialization?

[mchitre]

As discussed offline:

• We need the type information encoded as clazz in the JSON correctly for proper deserialization in Java for arbitrary classes.
• We should retain this and update ParamChangeNtf to use GenericValue as well, so that it uses the same encoding.
• We may wish to support simple JSON map as a special case on the Java side, where not specifying a clazz defaults to using a HashMap.

[ettersi]

We should [...] update ParamChangeNtf to use GenericValue as well, so that it uses the same encoding.

Implemented in https://github.com/org-arl/unet/pull/1946.

• We may wish to support simple JSON map as a special case on the Java side, where not specifying a clazz defaults to using a HashMap.

Not urgent at the moment, can be implemented whenever the need arises.