Make JSON dictionary output of Map[K, V] usable for other types of K #381
Conversation
| @@ -103,7 +103,7 @@ trait Api | |||
| def writeBinaryTo[T: Writer](t: T, out: java.io.OutputStream): Unit = { | |||
| streamBinary[T](t).writeBytesTo(out) | |||
| } | |||
| def writeBinaryToByteArray[T: Writer](t: T, out: java.io.OutputStream): Unit = { | |||
| def writeBinaryToByteArray[T: Writer](t: T) = { | |||
There was a problem hiding this comment.
Marker trait is brilliant here. I wish I had considered that for rallyhealth/weePickle#28.
Picking a behavior was difficult since visitArray/visitObject are valid keys for msgpack, but not JSON. Seems like in a perfect world, the fix should be on the Visitor side rather than the Writer side. I ended up using the keyWriter 100% of the time (i.e. no array tuples). Weepickle's JsonGeneratorVisitor.keyVisitor coerces primitives to strings, but throws on visitArray/visitObject. I justified it with the argument that the latter are a corner case, should get caught during tests, and are fixable in user space.
OTOH, your approach favors safe JSON over idiomatic msgpack. Very reasonable compromise. Thanks for the tag on this.
|
@htmldoug your comment made me think of how we could support idiomatic MessagePack dictionaries, since MessagePack does not have a string-keys-only restriction. I pushed another update that implements this: in effect, we avoid making the "encode as dictionary or string" decision in That lets the JSON-related backends use the flag to swap between a json-dictionary encoding (now called One subtlety is that |
|
Great solution! Unrelated, but one thing to be mindful of with the marker trait is that the |
|
@htmldoug good catch with the Perhaps later we can move the I also broadened our test suite to test the |
Yep, that's the main cause of #314. |
Currently, only
Map[String, V]is hardcoded to serialize as a JSON dictionary{"...": ..., "...": ...}, with other non-StringTs serializing as a JSON list-of-lists[["...", ...], ["...", ...]]. The list-of-lists representation, while usable, has downsides:{"foo": "bar", "baz": "qux"} [["foo", "bar"], ["baz", "qux"]] { "foo": "bar", "baz": "qux" } [ [ "foo", "bar" ], [ "baz", "qux" ] ]Ideally, we should use the dictionary representation whenever possible. This PR makes it possible to use it for non-
Stringtypes: primitives likeInt,Long,Boolean, string-y types likeUUIDorDuration, and even user-defined types whoseReader/Writerinherits fromMapKeyor are wrapped in astringKeyRW. For now, we leave the case where theReader/Writergenerates non-primitive JSON types like Arrays or Dictionaries as unsupported.The change is accomplished by some tweaks to the
MapWriter0function to remove the hardcoded special-casing ofStringWriter, and tweaks to theBaseElemRendererprotocol to allow it to accept usage of non-string keys. We instead allow anything to be written when a key is required, surrounded by quotes, with the built-in quoting of strings disabled to avoid double-quoting. Again, this does not work properly for JSON Arrays or Dictionaries.This is a forward-breaking change in the uPickle JSON serialization format: newer versions of uPickle will be able to read the old format, but older versions of uPickle will not be able to read the new format. Nevertheless, I think the problem this fixes is sufficiently important to justify the breakage. We can bump the major version number if necessary.
Also did some tightening up of the unit test suite, to make sure that the
rwhelper checks that the input value is serialized to the first of the JSON strings given. We previously did not validate this at all, only that the JSON strings all deserialized to the input value, and that the input value round tripped, but not that the string that the input value round tripped through was in the list of strings given.