Skip to content
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.

Sign up for GitHub

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(kernel): correct deserialization of structs in union contexts #919

Merged
merged 1 commit into from
Oct 30, 2019
Merged

fix(kernel): correct deserialization of structs in union contexts #919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions packages/jsii-calc/lib/compliance.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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