new JSON formats for some DAML-LF data #2463
Conversation
- restores totality checking for interface ADT - makes alternate input formats obvious at a glance - lets us restructure in the future to cache type branch lookups
…-lf-value-json-formats
…parately - not dependent on JavaScript safe integer range, because that is not the purpose of this option
There was a problem hiding this comment.
What are the use cases for enabling encodeDecimalAsString and encodeInt64AsString? Storing data in a database? Serializing numeric data as JsNumber is not safe unless you have total control over who is going to deserialize the JSON object (now or in the future).
FYI, we used to treat the Navigator GraphQL API (which the JSON encoding is part of) as a stable API, but we gave up on this a while back when we decided not to offer the Navigator code base as a framework for writing UIs. Doing breaking changes in the Navigator JSON encoding is fine as far as Navigator is concerned.
| V.ValueOptional(Some(jsValueToApiValue(v, prim.typArgs.head, defs))) | ||
| case Some(_) => deserializationError(s"Can't read ${value.prettyPrint} as Optional") | ||
| case None => deserializationError(s"Can't read ${value.prettyPrint} as Optional") | ||
| (prim.typ, value).match2 { |
There was a problem hiding this comment.
Using match2 is to preserve exhaustiveness checking? Or is there any other advantage?
There was a problem hiding this comment.
Yes; additionally, we might decide to resolve the first match layer in advance of having the value in the future, notwithstanding DAML recursive cases.
| val id = Model.DamlLfIdentifier( | ||
| typeCon.name.identifier.packageId, | ||
| typeCon.name.identifier.qualifiedName) | ||
| val id = typeCon.name.identifier | ||
| // val dt = typeCon.instantiate(defs(id).getOrElse(deserializationError(s"Type $id not found"))) | ||
| val dt = Model.damlLfInstantiate( |
There was a problem hiding this comment.
Unrelated to this commit: AFAIR Model.damlLfInstantiate is a copy of typeCon.instantiate, introduced when the daml.lf version had bug and Navigator could not upgrade to the latest daml.lf library version.
New issue for fixing this: #2506
@rautenrieth-da That's a good one. We do plan to replace extractor's JSON encoder with this one. The real reason it appears in this PR is "that's what's written in the spec".
I could equally say that a deserializer could discard every character in a number-string after the seventh. This is not without precedent 🙂 so I wouldn't say this is a reason it's "unsafe". Instead, I would say that our choice of default for the HTTP JSON API (and Navigator) "avoids common mistakes", whereas alternate cases e.g. extractor will simply make alternate choices. Even in the cases you're referring to, it's not necessarily a concern that some data loss might occur, because not all use cases require the JSON encoding of contracts to be injective. Consider an application that charts some decimal values from a set of contracts, for example. We might want to provide such applications a way to [explicitly] request number encoding in the future. |
@S11001001 I don't think we can make it injective, there always will be a case when @rautenrieth-da see javascript |
|
@leo-da I should have said "JSON encoding of contracts of the same type". |
|
In any case if it is up to me, I would keep the option to represent |
…-lf-value-json-formats
The
ApiCodecCompresseduses some formats by default we don't want to use anymore, like{"Some": {}}forSome (). We'll make the encoder use new formats, keeping the branches for the previous formats whenever no inconsistency arises, but adding new branches.NoneFixes #2361. Documented in #2519.
Pull Request Checklist
NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines runtotrigger the build.