fix(kernel): correct deserialization of structs in union contexts #919
Conversation
When deserializing structures in a Union context, the Kernel used to try
options in an arbitrary order (actually - the declaration order which
often is alphanumerically sorted). In addition, it would ignore any
additional property encountered silently.
In cases where several of the union's possibilities had overlapping
required properties, the first attempted option would succeed, even if a
subsequent option would have consumed more properties. A pathologic case
of this is when the first candidate is a type with only optional
properties; as such a type would *always* successfully deserialize
anything, possibly ignoring all properties.
In order to address this, the `structs` can now be passed into the
Kernel by wrapping the object in a decorator box:
```js
{
"$jsii.struct": {
"fqn": "fully.qualified.struct.TypeName",
"data": {
/* the actual data included in the struct instance */
}
}
}
```
This enables "native" languages to correctly communicate the intended
type to the Kernel, so it can make an appropriate deserialization
decision.
The encoding of structs from the Kernel to "native" languages has not
changed: those are still passed by-reference in order to maximize the
compatibility; and to avoid undeterministic behavior in case where union
of structs are returned.
Fixes #822
Fixes aws/aws-cdk#3917
RomainMuller
requested review from
bmaizels,
garnaat and
a team
as code owners
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
It does not seem that this PR includes a solution for Python. Should be just as affected by this issue, no? |
|
The most trivial solution to me seems just taking the by-value object from the wire, validating it, but not having the validation change the value that is used. Do we transform/deep-deserialize in some way that makes this impossible/impractical? A potentially different way of going about this is having the It would be harder though. |
|
@rix0rrr - there's a change & new compliance test in the Python runtime... Are you saying this is insufficient? With respects to trying out serialization orders -- candidates can overlap in such a way that it is not possible to know which one is the "best one to try first" -- the compliance test I added very specifically represents one such situation (although maybe not as pathological as it could be). In order to go this route, we'd have to go for the deserialization that ignores the fewest properties from the input (ideally, one should ignore none); but this requires more complexity on the kernel (and probably would have slightly worse performance characteristics). |
When deserializing structures in a Union context, the Kernel used to try
options in an arbitrary order (actually - the declaration order which
often is alphanumerically sorted). In addition, it would ignore any
additional property encountered silently.
In cases where several of the union's possibilities had overlapping
required properties, the first attempted option would succeed, even if a
subsequent option would have consumed more properties. A pathologic case
of this is when the first candidate is a type with only optional
properties; as such a type would always successfully deserialize
anything, possibly ignoring all properties.
In order to address this, the
structscan now be passed into theKernel by wrapping the object in a decorator box:
This enables "native" languages to correctly communicate the intended
type to the Kernel, so it can make an appropriate deserialization
decision.
The encoding of structs from the Kernel to "native" languages has not
changed: those are still passed by-reference in order to maximize the
compatibility; and to avoid undeterministic behavior in case where union
of structs are returned.
Fixes #822
Fixes aws/aws-cdk#3917
Fixes aws/aws-cdk#2013
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.