Skip to content

Commit

Permalink
fix(kernel): correct deserialization of structs in union contexts
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@RomainMuller
RomainMuller committed Oct 30, 2019
1 parent c7d2fc1 commit c79be71
Show file tree
Hide file tree
Showing 82 changed files with 2,026 additions and 256 deletions.
41 changes: 41 additions & 0 deletions packages/jsii-calc/lib/compliance.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1976,3 +1976,44 @@ class PrivateType extends Implementation implements IAnonymouslyImplementMe {
return 'to implement';
}
}

/**
* We can serialize and deserialize structs without silently ignoring optional fields.
*/
export interface StructA {
readonly requiredString: string;
readonly optionalString?: string;
readonly optionalNumber?: number;
}
/**
* This intentionally overlaps with StructA (where only requiredString is provided) to test htat
* the kernel properly disambiguates those.
*/
export interface StructB {
readonly requiredString: string;
readonly optionalBoolean?: boolean;
readonly optionalStructA?: StructA;
}
export class StructUnionConsumer {
public static isStructA(struct: StructA | StructB): struct is StructA {
const keys = new Set(Object.keys(struct));
switch (keys.size) {
case 1: return keys.has('requiredString');
case 2: return keys.has('requiredString') && (keys.has('optionalNumber') || keys.has('optionalString'));
case 3: return keys.has('requiredString') && keys.has('optionalNumber') && keys.has('optionalString');
default: return false;
}
}

public static isStructB(struct: StructA | StructB): struct is StructB {
const keys = new Set(Object.keys(struct));
switch (keys.size) {
case 1: return keys.has('requiredString');
case 2: return keys.has('requiredString') && (keys.has('optionalBoolean') || keys.has('optionalStructA'));
case 3: return keys.has('requiredString') && keys.has('optionalBoolean') && keys.has('optionalStructA');
default: return false;
}
}

private constructor() { }
}
211 changes: 210 additions & 1 deletion packages/jsii-calc/test/assembly.jsii
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8559,6 +8559,134 @@
}
]
},
"jsii-calc.StructA": {
"assembly": "jsii-calc",
"datatype": true,
"docs": {
"stability": "experimental",
"summary": "We can serialize and deserialize structs without silently ignoring optional fields."
},
"fqn": "jsii-calc.StructA",
"kind": "interface",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1983
},
"name": "StructA",
"properties": [
{
"abstract": true,
"docs": {
"stability": "experimental"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1984
},
"name": "requiredString",
"type": {
"primitive": "string"
}
},
{
"abstract": true,
"docs": {
"stability": "experimental"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1986
},
"name": "optionalNumber",
"optional": true,
"type": {
"primitive": "number"
}
},
{
"abstract": true,
"docs": {
"stability": "experimental"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1985
},
"name": "optionalString",
"optional": true,
"type": {
"primitive": "string"
}
}
]
},
"jsii-calc.StructB": {
"assembly": "jsii-calc",
"datatype": true,
"docs": {
"stability": "experimental",
"summary": "This intentionally overlaps with StructA (where only requiredString is provided) to test htat the kernel properly disambiguates those."
},
"fqn": "jsii-calc.StructB",
"kind": "interface",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1992
},
"name": "StructB",
"properties": [
{
"abstract": true,
"docs": {
"stability": "experimental"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1993
},
"name": "requiredString",
"type": {
"primitive": "string"
}
},
{
"abstract": true,
"docs": {
"stability": "experimental"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1994
},
"name": "optionalBoolean",
"optional": true,
"type": {
"primitive": "boolean"
}
},
{
"abstract": true,
"docs": {
"stability": "experimental"
},
"immutable": true,
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1995
},
"name": "optionalStructA",
"optional": true,
"type": {
"fqn": "jsii-calc.StructA"
}
}
]
},
"jsii-calc.StructPassing": {
"assembly": "jsii-calc",
"docs": {
Expand Down Expand Up @@ -8638,6 +8766,87 @@
],
"name": "StructPassing"
},
"jsii-calc.StructUnionConsumer": {
"assembly": "jsii-calc",
"docs": {
"stability": "experimental"
},
"fqn": "jsii-calc.StructUnionConsumer",
"kind": "class",
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1997
},
"methods": [
{
"docs": {
"stability": "experimental"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 1998
},
"name": "isStructA",
"parameters": [
{
"name": "struct",
"type": {
"union": {
"types": [
{
"fqn": "jsii-calc.StructA"
},
{
"fqn": "jsii-calc.StructB"
}
]
}
}
}
],
"returns": {
"type": {
"primitive": "boolean"
}
},
"static": true
},
{
"docs": {
"stability": "experimental"
},
"locationInModule": {
"filename": "lib/compliance.ts",
"line": 2008
},
"name": "isStructB",
"parameters": [
{
"name": "struct",
"type": {
"union": {
"types": [
{
"fqn": "jsii-calc.StructA"
},
{
"fqn": "jsii-calc.StructB"
}
]
}
}
}
],
"returns": {
"type": {
"primitive": "boolean"
}
},
"static": true
}
],
"name": "StructUnionConsumer"
},
"jsii-calc.StructWithJavaReservedWords": {
"assembly": "jsii-calc",
"datatype": true,
Expand Down Expand Up @@ -10119,5 +10328,5 @@
}
},
"version": "0.19.0",
"fingerprint": "MZT68aeJ7MEeDJORDPKkIyqOqOLC+Yb3LcDU7XrxgCQ="
"fingerprint": "x2+sbdlYAHP8MHude74WmIipYqqFoEMnnpxBnz5DvZ0="
}
19 changes: 19 additions & 0 deletions ...ges/jsii-dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ComplianceTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1015,6 +1015,25 @@ public void CanLeverageIndirectInterfacePolymorphism()
Assert.Equal(1337d, provider.ProvideAsInterface().Value);
Assert.Equal("to implement", provider.ProvideAsInterface().Verb());
}

[Fact(DisplayName = Prefix + nameof(CorrectlyDeserializesStructUnions))]
public void CorrectlyDeserializesStructUnions()
{
var a0 = new StructA { RequiredString = "Present!", OptionalString = "Bazinga!" };
var a1 = new StructA { RequiredString = "Present!", OptionalNumber = 1337 };
var b0 = new StructB { RequiredString = "Present!", OptionalBoolean = true };
var b1 = new StructB { RequiredString = "Present!", OptionalStructA = a1 };

Assert.True(StructUnionConsumer.IsStructA(a0));
Assert.True(StructUnionConsumer.IsStructA(a1));
Assert.False(StructUnionConsumer.IsStructA(b0));
Assert.False(StructUnionConsumer.IsStructA(b1));

Assert.False(StructUnionConsumer.IsStructB(a0));
Assert.False(StructUnionConsumer.IsStructB(a1));
Assert.True(StructUnionConsumer.IsStructB(b0));
Assert.True(StructUnionConsumer.IsStructB(b1));
}

class DataRendererSubclass : DataRenderer
{
Expand Down
6 changes: 6 additions & 0 deletions packages/jsii-dotnet-runtime/src/Amazon.JSII.Runtime/Deputy/JsiiByValueAttribute.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,11 @@ namespace Amazon.JSII.Runtime.Deputy
[AttributeUsage(AttributeTargets.Class)]
public sealed class JsiiByValueAttribute : Attribute
{
public JsiiByValueAttribute(string fqn)
{
Fqn = fqn ?? throw new ArgumentNullException(nameof(fqn));
}

public string Fqn { get; }
}
}
14 changes: 11 additions & 3 deletions ...ii-dotnet-runtime/src/Amazon.JSII.Runtime/Services/Converters/FrameworkToJsiiConverter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,10 @@ protected override bool TryConvertClass(Type type, IReferenceMap referenceMap, o
return true;
}

if (Attribute.GetCustomAttribute(value.GetType(), typeof(JsiiByValueAttribute)) != null)
var byValueAttribute = value.GetType().GetCustomAttribute<JsiiByValueAttribute>();
if (byValueAttribute != null)
{
var resultObject = new JObject();
var data = new JObject();
foreach (var prop in value.GetType().GetProperties())
{
var jsiiProperty = (JsiiPropertyAttribute) prop.GetCustomAttribute(typeof(JsiiPropertyAttribute), true);
Expand All @@ -68,9 +69,16 @@ protected override bool TryConvertClass(Type type, IReferenceMap referenceMap, o
continue;
}

resultObject.Add(new JProperty(jsiiProperty.Name, convertedPropValue));
data.Add(new JProperty(jsiiProperty.Name, convertedPropValue));
}

var structInfo = new JObject();
structInfo.Add(new JProperty("fqn", byValueAttribute.Fqn));
structInfo.Add(new JProperty("data", data));

var resultObject = new JObject();
resultObject.Add(new JProperty("$jsii.struct", structInfo));

result = resultObject;
return true;
}
Expand Down
30 changes: 30 additions & 0 deletions ...-java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1361,6 +1361,36 @@ public void canLeverageIndirectInterfacePolymorphism() {
assertEquals("to implement", provider.provideAsInterface().verb());
}

@Test
public void correctlyDeserializesStructUnions() {
final StructA a0 = StructA.builder()
.requiredString("Present!")
.optionalString("Bazinga!")
.build();
final StructA a1 = StructA.builder()
.requiredString("Present!")
.optionalNumber(1337)
.build();
final StructB b0 = StructB.builder()
.requiredString("Present!")
.optionalBoolean(true)
.build();
final StructB b1 = StructB.builder()
.requiredString("Present!")
.optionalStructA(a1)
.build();

assertTrue(StructUnionConsumer.isStructA(a0));
assertTrue(StructUnionConsumer.isStructA(a1));
assertFalse(StructUnionConsumer.isStructA(b0));
assertFalse(StructUnionConsumer.isStructA(b1));

assertFalse(StructUnionConsumer.isStructB(a0));
assertFalse(StructUnionConsumer.isStructB(a1));
assertTrue(StructUnionConsumer.isStructB(b0));
assertTrue(StructUnionConsumer.isStructB(b1));
}

static class PartiallyInitializedThisConsumerImpl extends PartiallyInitializedThisConsumer {
@Override
public String consumePartiallyInitializedThis(final ConstructorPassesThisOut obj,
Expand Down
Loading

0 comments on commit c79be71

Please sign in to comment.