Skip to content

Commit e88e5e2

Browse files
RomainMullermergify[bot]
RomainMuller
authored and
mergify[bot]
committed
fix(java): handle null-able collections correctly (#986)
When a `null` value was returned fro an optional collection (List or Map), it would be passed to the corresponding unmodifiable wrapper (`java.util.Collections.unmodifiable~`), however these method are not `null`-safe. This makes the wrapping conditional to the collection not being `null`. This addresses the cause of aws/aws-cdk#4316
1 parent a8096b7 commit e88e5e2

16 files changed

Lines changed: 343 additions & 32 deletions

File tree

‎packages/jsii-calc/lib/compliance.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2267,7 +2267,7 @@ export class Demonstrate982 {
22672267
public constructor() { }
22682268
}
22692269

2270-
/**
2270+
/*
22712271
* This tries to confuse Jackson by having overloaded property setters.
22722272
*
22732273
* @see https://github.com/aws/aws-cdk/issues/4080
@@ -2288,3 +2288,16 @@ export class ConfusingToJackson {
22882288
export interface ConfusingToJacksonStruct {
22892289
readonly unionProperty?: Array<IFriendly | AbstractClass> | IFriendly;
22902290
}
2291+
2292+
/**
2293+
* Verifies that null/undefined can be returned for optional collections.
2294+
*
2295+
* This source of collections is disappointing - it'll always give you nothing :(
2296+
*/
2297+
export class DisappointingCollectionSource {
2298+
/** Some List of strings, maybe? (Nah, just a billion dollars mistake!) */
2299+
public static readonly maybeList?: string[] = undefined;
2300+
/** Some Map of strings to numbers, maybe? (Nah, just a billion dollars mistake!) */
2301+
public static readonly maybeMap?: { [key: string]: number } = undefined;
2302+
private constructor() { }
2303+
}

‎packages/jsii-calc/test/assembly.jsii‎

Lines changed: 67 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2222,9 +2222,7 @@
22222222
"jsii-calc.ConfusingToJackson": {
22232223
"assembly": "jsii-calc",
22242224
"docs": {
2225-
"see": "https://github.com/aws/aws-cdk/issues/4080",
2226-
"stability": "experimental",
2227-
"summary": "This tries to confuse Jackson by having overloaded property setters."
2225+
"stability": "experimental"
22282226
},
22292227
"fqn": "jsii-calc.ConfusingToJackson",
22302228
"kind": "class",
@@ -3506,6 +3504,71 @@
35063504
}
35073505
]
35083506
},
3507+
"jsii-calc.DisappointingCollectionSource": {
3508+
"assembly": "jsii-calc",
3509+
"docs": {
3510+
"remarks": "This source of collections is disappointing - it'll always give you nothing :(",
3511+
"stability": "experimental",
3512+
"summary": "Verifies that null/undefined can be returned for optional collections."
3513+
},
3514+
"fqn": "jsii-calc.DisappointingCollectionSource",
3515+
"kind": "class",
3516+
"locationInModule": {
3517+
"filename": "lib/compliance.ts",
3518+
"line": 2297
3519+
},
3520+
"name": "DisappointingCollectionSource",
3521+
"properties": [
3522+
{
3523+
"const": true,
3524+
"docs": {
3525+
"remarks": "(Nah, just a billion dollars mistake!)",
3526+
"stability": "experimental",
3527+
"summary": "Some List of strings, maybe?"
3528+
},
3529+
"immutable": true,
3530+
"locationInModule": {
3531+
"filename": "lib/compliance.ts",
3532+
"line": 2299
3533+
},
3534+
"name": "maybeList",
3535+
"optional": true,
3536+
"static": true,
3537+
"type": {
3538+
"collection": {
3539+
"elementtype": {
3540+
"primitive": "string"
3541+
},
3542+
"kind": "array"
3543+
}
3544+
}
3545+
},
3546+
{
3547+
"const": true,
3548+
"docs": {
3549+
"remarks": "(Nah, just a billion dollars mistake!)",
3550+
"stability": "experimental",
3551+
"summary": "Some Map of strings to numbers, maybe?"
3552+
},
3553+
"immutable": true,
3554+
"locationInModule": {
3555+
"filename": "lib/compliance.ts",
3556+
"line": 2301
3557+
},
3558+
"name": "maybeMap",
3559+
"optional": true,
3560+
"static": true,
3561+
"type": {
3562+
"collection": {
3563+
"elementtype": {
3564+
"primitive": "number"
3565+
},
3566+
"kind": "map"
3567+
}
3568+
}
3569+
}
3570+
]
3571+
},
35093572
"jsii-calc.DoNotOverridePrivates": {
35103573
"assembly": "jsii-calc",
35113574
"docs": {
@@ -11271,5 +11334,5 @@
1127111334
}
1127211335
},
1127311336
"version": "0.20.5",
11274-
"fingerprint": "vKeZwjjLC1e84B6FiOQCH8JDNoINrldYfdEWYKqCFSQ="
11337+
"fingerprint": "SpqWbp4sQfTLiw/Oogb9KSxfoPs7Uf0h6C3oQonjvYg="
1127511338
}

‎packages/jsii-dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ComplianceTests.cs‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1037,7 +1037,7 @@ public void VariadicCallbacksAreHandledCorrectly()
10371037
public void ReturnSubclassThatImplementsInterface976()
10381038
{
10391039
var obj = SomeTypeJsii976.ReturnReturn();
1040-
Assert.Equal(obj.Foo, 333);
1040+
Assert.Equal(333, obj.Foo);
10411041
}
10421042

10431043
private sealed class OverrideVariadicMethod : VariadicMethod
@@ -1277,5 +1277,17 @@ public void CanObtainStructReferenceWithOverloadedSetters()
12771277
{
12781278
Assert.NotNull(ConfusingToJackson.MakeStructInstance());
12791279
}
1280+
1281+
[Fact(DisplayName = Prefix + nameof(NullIsAValidOptionalList))]
1282+
public void NullIsAValidOptionalList()
1283+
{
1284+
Assert.Null(DisappointingCollectionSource.MaybeList);
1285+
}
1286+
1287+
[Fact(DisplayName = Prefix + nameof(NullIsAValidOptionalMap))]
1288+
public void NullIsAValidOptionalMap()
1289+
{
1290+
Assert.Null(DisappointingCollectionSource.MaybeMap);
1291+
}
12801292
}
12811293
}

‎packages/jsii-java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1403,6 +1403,16 @@ public void testStructsCanBeDowncastedToParentType() {
14031403
assertNotNull(Demonstrate982.takeThisToo());
14041404
}
14051405

1406+
@Test
1407+
public void testNullIsAValidOptionalList() {
1408+
assertNull(DisappointingCollectionSource.MAYBE_LIST);
1409+
}
1410+
1411+
@Test
1412+
public void testNullIsAValidOptionalMap() {
1413+
assertNull(DisappointingCollectionSource.MAYBE_MAP);
1414+
}
1415+
14061416
static class PartiallyInitializedThisConsumerImpl extends PartiallyInitializedThisConsumer {
14071417
@Override
14081418
public String consumePartiallyInitializedThis(final ConstructorPassesThisOut obj,

‎packages/jsii-pacmak/lib/targets/java.ts‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -859,7 +859,8 @@ class JavaGenerator extends Generator {
859859
for (const prop of consts) {
860860
const constName = this.renderConstName(prop);
861861
const propClass = this.toJavaType(prop.type, true);
862-
this.code.line(`${constName} = software.amazon.jsii.JsiiObject.jsiiStaticGet(${javaClass}.class, "${prop.name}", ${propClass}.class);`);
862+
const statement = `software.amazon.jsii.JsiiObject.jsiiStaticGet(${javaClass}.class, "${prop.name}", ${propClass}.class)`;
863+
this.code.line(`${constName} = ${this.wrapCollection(statement, prop.type, prop.optional)};`);
863864
}
864865

865866
this.code.closeBlock();
@@ -905,7 +906,7 @@ class JavaGenerator extends Generator {
905906

906907
statement += `"${prop.name}", ${propClass}.class)`;
907908

908-
this.code.line(`return ${this.wrapCollection(statement, prop.type)};`);
909+
this.code.line(`return ${this.wrapCollection(statement, prop.type, prop.optional)};`);
909910
this.code.closeBlock();
910911
}
911912

@@ -1644,7 +1645,7 @@ class JavaGenerator extends Generator {
16441645
statement += `${this.renderMethodCallArguments(method)})`;
16451646

16461647
if (method.returns) {
1647-
statement = this.wrapCollection(statement, method.returns.type);
1648+
statement = this.wrapCollection(statement, method.returns.type, method.returns.optional);
16481649
}
16491650

16501651
if (method.returns) {
@@ -1658,19 +1659,26 @@ class JavaGenerator extends Generator {
16581659
* Wraps a collection into an unmodifiable collection else returns the existing statement.
16591660
* @param statement The statement to wrap if necessary.
16601661
* @param type The type of the object to wrap.
1662+
* @param optional Whether the value is optional (can be null/undefined) or not.
16611663
* @returns The modified or original statement.
16621664
*/
1663-
private wrapCollection(statement: string, type: spec.TypeReference): string {
1665+
private wrapCollection(statement: string, type: spec.TypeReference, optional?: boolean): string {
16641666
if (spec.isCollectionTypeReference(type)) {
1665-
const ref = type;
1666-
switch (ref.collection.kind) {
1667+
let wrapper: string;
1668+
switch (type.collection.kind) {
16671669
case spec.CollectionKind.Array:
1668-
return `java.util.Collections.unmodifiableList(${statement})`;
1670+
wrapper = 'unmodifiableList';
1671+
break;
16691672
case spec.CollectionKind.Map:
1670-
return `java.util.Collections.unmodifiableMap(${statement})`;
1673+
wrapper = 'unmodifiableMap';
1674+
break;
16711675
default:
1672-
throw new Error(`Unsupported collection kind: ${ref.collection.kind}`);
1676+
throw new Error(`Unsupported collection kind: ${type.collection.kind}`);
16731677
}
1678+
// In the case of "optional", the value needs ot be explicitly cast to allow for cases where the raw type was returned.
1679+
return optional
1680+
? `java.util.Optional.ofNullable((${this.toJavaType(type)})(${statement})).map(java.util.Collections::${wrapper}).orElse(null)`
1681+
: `java.util.Collections.${wrapper}(${statement})`;
16741682
}
16751683

16761684
return statement;

‎packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii‎

Lines changed: 67 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2222,9 +2222,7 @@
22222222
"jsii-calc.ConfusingToJackson": {
22232223
"assembly": "jsii-calc",
22242224
"docs": {
2225-
"see": "https://github.com/aws/aws-cdk/issues/4080",
2226-
"stability": "experimental",
2227-
"summary": "This tries to confuse Jackson by having overloaded property setters."
2225+
"stability": "experimental"
22282226
},
22292227
"fqn": "jsii-calc.ConfusingToJackson",
22302228
"kind": "class",
@@ -3506,6 +3504,71 @@
35063504
}
35073505
]
35083506
},
3507+
"jsii-calc.DisappointingCollectionSource": {
3508+
"assembly": "jsii-calc",
3509+
"docs": {
3510+
"remarks": "This source of collections is disappointing - it'll always give you nothing :(",
3511+
"stability": "experimental",
3512+
"summary": "Verifies that null/undefined can be returned for optional collections."
3513+
},
3514+
"fqn": "jsii-calc.DisappointingCollectionSource",
3515+
"kind": "class",
3516+
"locationInModule": {
3517+
"filename": "lib/compliance.ts",
3518+
"line": 2297
3519+
},
3520+
"name": "DisappointingCollectionSource",
3521+
"properties": [
3522+
{
3523+
"const": true,
3524+
"docs": {
3525+
"remarks": "(Nah, just a billion dollars mistake!)",
3526+
"stability": "experimental",
3527+
"summary": "Some List of strings, maybe?"
3528+
},
3529+
"immutable": true,
3530+
"locationInModule": {
3531+
"filename": "lib/compliance.ts",
3532+
"line": 2299
3533+
},
3534+
"name": "maybeList",
3535+
"optional": true,
3536+
"static": true,
3537+
"type": {
3538+
"collection": {
3539+
"elementtype": {
3540+
"primitive": "string"
3541+
},
3542+
"kind": "array"
3543+
}
3544+
}
3545+
},
3546+
{
3547+
"const": true,
3548+
"docs": {
3549+
"remarks": "(Nah, just a billion dollars mistake!)",
3550+
"stability": "experimental",
3551+
"summary": "Some Map of strings to numbers, maybe?"
3552+
},
3553+
"immutable": true,
3554+
"locationInModule": {
3555+
"filename": "lib/compliance.ts",
3556+
"line": 2301
3557+
},
3558+
"name": "maybeMap",
3559+
"optional": true,
3560+
"static": true,
3561+
"type": {
3562+
"collection": {
3563+
"elementtype": {
3564+
"primitive": "number"
3565+
},
3566+
"kind": "map"
3567+
}
3568+
}
3569+
}
3570+
]
3571+
},
35093572
"jsii-calc.DoNotOverridePrivates": {
35103573
"assembly": "jsii-calc",
35113574
"docs": {
@@ -11271,5 +11334,5 @@
1127111334
}
1127211335
},
1127311336
"version": "0.20.5",
11274-
"fingerprint": "vKeZwjjLC1e84B6FiOQCH8JDNoINrldYfdEWYKqCFSQ="
11337+
"fingerprint": "SpqWbp4sQfTLiw/Oogb9KSxfoPs7Uf0h6C3oQonjvYg="
1127511338
}

‎packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/Amazon/JSII/Tests/CalculatorNamespace/ConfusingToJackson.cs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,8 @@
22

33
namespace Amazon.JSII.Tests.CalculatorNamespace
44
{
5-
/// <summary>This tries to confuse Jackson by having overloaded property setters.</summary>
65
/// <remarks>
76
/// stability: Experimental
8-
/// see:
9-
/// https://github.com/aws/aws-cdk/issues/4080
107
/// </remarks>
118
[JsiiClass(nativeType: typeof(Amazon.JSII.Tests.CalculatorNamespace.ConfusingToJackson), fullyQualifiedName: "jsii-calc.ConfusingToJackson")]
129
public class ConfusingToJackson : DeputyBase

‎packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/Amazon/JSII/Tests/CalculatorNamespace/DisappointingCollectionSource.cs‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
using Amazon.JSII.Runtime.Deputy;
2+
3+
namespace Amazon.JSII.Tests.CalculatorNamespace
4+
{
5+
/// <summary>Verifies that null/undefined can be returned for optional collections.</summary>
6+
/// <remarks>
7+
/// This source of collections is disappointing - it'll always give you nothing :(
8+
/// stability: Experimental
9+
/// </remarks>
10+
[JsiiClass(nativeType: typeof(Amazon.JSII.Tests.CalculatorNamespace.DisappointingCollectionSource), fullyQualifiedName: "jsii-calc.DisappointingCollectionSource")]
11+
public class DisappointingCollectionSource : DeputyBase
12+
{
13+
protected DisappointingCollectionSource(ByRefValue reference): base(reference)
14+
{
15+
}
16+
17+
protected DisappointingCollectionSource(DeputyProps props): base(props)
18+
{
19+
}
20+
21+
/// <summary>Some List of strings, maybe?</summary>
22+
/// <remarks>
23+
/// (Nah, just a billion dollars mistake!)
24+
/// stability: Experimental
25+
/// </remarks>
26+
[JsiiProperty(name: "maybeList", typeJson: "{\"collection\":{\"elementtype\":{\"primitive\":\"string\"},\"kind\":\"array\"}}", isOptional: true)]
27+
public static string[] MaybeList
28+
{
29+
get;
30+
}
31+
= GetStaticProperty<string[]>(typeof(Amazon.JSII.Tests.CalculatorNamespace.DisappointingCollectionSource));
32+
33+
/// <summary>Some Map of strings to numbers, maybe?</summary>
34+
/// <remarks>
35+
/// (Nah, just a billion dollars mistake!)
36+
/// stability: Experimental
37+
/// </remarks>
38+
[JsiiProperty(name: "maybeMap", typeJson: "{\"collection\":{\"elementtype\":{\"primitive\":\"number\"},\"kind\":\"map\"}}", isOptional: true)]
39+
public static System.Collections.Generic.IDictionary<string, double> MaybeMap
40+
{
41+
get;
42+
}
43+
= GetStaticProperty<System.Collections.Generic.IDictionary<string, double>>(typeof(Amazon.JSII.Tests.CalculatorNamespace.DisappointingCollectionSource));
44+
}
45+
}

‎packages/jsii-pacmak/test/expected.jsii-calc/java/src/main/java/software/amazon/jsii/tests/calculator/$Module.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ protected Class<?> resolveClass(final String fqn) throws ClassNotFoundException
6060
case "jsii-calc.DiamondInheritanceFirstMidLevelStruct": return software.amazon.jsii.tests.calculator.DiamondInheritanceFirstMidLevelStruct.class;
6161
case "jsii-calc.DiamondInheritanceSecondMidLevelStruct": return software.amazon.jsii.tests.calculator.DiamondInheritanceSecondMidLevelStruct.class;
6262
case "jsii-calc.DiamondInheritanceTopLevelStruct": return software.amazon.jsii.tests.calculator.DiamondInheritanceTopLevelStruct.class;
63+
case "jsii-calc.DisappointingCollectionSource": return software.amazon.jsii.tests.calculator.DisappointingCollectionSource.class;
6364
case "jsii-calc.DoNotOverridePrivates": return software.amazon.jsii.tests.calculator.DoNotOverridePrivates.class;
6465
case "jsii-calc.DoNotRecognizeAnyAsOptional": return software.amazon.jsii.tests.calculator.DoNotRecognizeAnyAsOptional.class;
6566
case "jsii-calc.DocumentedClass": return software.amazon.jsii.tests.calculator.DocumentedClass.class;

‎packages/jsii-pacmak/test/expected.jsii-calc/java/src/main/java/software/amazon/jsii/tests/calculator/ConfusingToJackson.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,7 @@
11
package software.amazon.jsii.tests.calculator;
22

33
/**
4-
* This tries to confuse Jackson by having overloaded property setters.
5-
*
64
* EXPERIMENTAL
7-
*
8-
* @see https://github.com/aws/aws-cdk/issues/4080
95
*/
106
@javax.annotation.Generated(value = "jsii-pacmak")
117
@software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental)

0 commit comments

Comments
 (0)
⚡