-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(JSON): JSONify Java maps as JSON dicts #309
Conversation
Do we have code that uses Maps as generic values? Can this be made backward compatible? |
SWIS makes use of this.
I guess we could introduce a |
I am guessing this happens if we have an Agent parameter whose value is of a type IMO we should formally define the types that are supported for |
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. |
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 |
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 |
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.
It ugly but it should work without any changes. ... The other way would be to use custom messages that can then carry a 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. |
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. |
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.
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
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 |
I am OK adding Can you please add some examples of the JSON that would have been created before this PR and after this PR? Also, I believe |
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.
I'm OK with this as long as it's well-documented.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 useGenericValue
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 aHashMap
.
Implemented in https://github.com/org-arl/unet/pull/1946.
Not urgent at the moment, can be implemented whenever the need arises. |
Currently,
GenericValue
s that happen to be JavaMap
s are JSONified as generic objects, not JSON maps. This is inconsistent with how JavaMap
s are serialized elsewhere (e.g. inParamChangeNtf
), leading to much confusion. This PR changes theGenericValue
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.