From 36c6395d2afd7dbf04468e3a290bd62951bef22e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Thu, 8 Aug 2019 16:10:38 -0700 Subject: [PATCH] fix(dotnet): stop mutating Dictionary when iterating on it When bringding callback results into dotnet types, we used to modify the Dictionary item within the loop, which is illegal in C#. Any mutation on the collection will cause the iterator to abort. Insead, create a new Dictionary and add the resolved values to that. Fixes #690 --- packages/jsii-calc/lib/compliance.ts | 4 ++ packages/jsii-calc/test/assembly.jsii | 50 +++++++++++++++---- .../ComplianceTests.cs | 6 +++ .../Amazon.JSII.Runtime/CallbackExtensions.cs | 9 +++- .../.jsii | 50 +++++++++++++++---- .../Tests/CalculatorNamespace/DataRenderer.cs | 9 ++++ .../jsii/tests/calculator/DataRenderer.java | 8 +++ .../python/src/jsii_calc/__init__.py | 10 ++++ .../expected.jsii-calc/sphinx/jsii-calc.rst | 7 +++ .../test/jsii-tree.test.all.expected.txt | 5 ++ .../test/jsii-tree.test.members.expected.txt | 1 + 11 files changed, 135 insertions(+), 24 deletions(-) diff --git a/packages/jsii-calc/lib/compliance.ts b/packages/jsii-calc/lib/compliance.ts index daa3675682..68334924bf 100644 --- a/packages/jsii-calc/lib/compliance.ts +++ b/packages/jsii-calc/lib/compliance.ts @@ -1737,6 +1737,10 @@ export class DataRenderer { return this.renderMap(data); } + public renderArbitrary(data: { [key: string]: any }): string { + return this.renderMap(data); + } + public renderMap(map: { [key: string]: any }): string { return JSON.stringify(map, null, 2); } diff --git a/packages/jsii-calc/test/assembly.jsii b/packages/jsii-calc/test/assembly.jsii index a97affaf2e..da2532f86d 100644 --- a/packages/jsii-calc/test/assembly.jsii +++ b/packages/jsii-calc/test/assembly.jsii @@ -2132,6 +2132,34 @@ "filename": "lib/compliance.ts", "line": 1740 }, + "name": "renderArbitrary", + "parameters": [ + { + "name": "data", + "type": { + "collection": { + "elementtype": { + "primitive": "any" + }, + "kind": "map" + } + } + } + ], + "returns": { + "type": { + "primitive": "string" + } + } + }, + { + "docs": { + "stability": "experimental" + }, + "locationInModule": { + "filename": "lib/compliance.ts", + "line": 1744 + }, "name": "renderMap", "parameters": [ { @@ -7160,7 +7188,7 @@ "kind": "interface", "locationInModule": { "filename": "lib/compliance.ts", - "line": 1762 + "line": 1766 }, "name": "SecondLevelStruct", "properties": [ @@ -7173,7 +7201,7 @@ "immutable": true, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1766 + "line": 1770 }, "name": "deeperRequiredProp", "type": { @@ -7189,7 +7217,7 @@ "immutable": true, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1771 + "line": 1775 }, "name": "deeperOptionalProp", "optional": true, @@ -7816,7 +7844,7 @@ "kind": "class", "locationInModule": { "filename": "lib/compliance.ts", - "line": 1779 + "line": 1783 }, "methods": [ { @@ -7825,7 +7853,7 @@ }, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1788 + "line": 1792 }, "name": "howManyVarArgsDidIPass", "parameters": [ @@ -7857,7 +7885,7 @@ }, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1780 + "line": 1784 }, "name": "roundTrip", "parameters": [ @@ -8242,7 +8270,7 @@ "kind": "interface", "locationInModule": { "filename": "lib/compliance.ts", - "line": 1745 + "line": 1749 }, "name": "TopLevelStruct", "properties": [ @@ -8255,7 +8283,7 @@ "immutable": true, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1749 + "line": 1753 }, "name": "required", "type": { @@ -8271,7 +8299,7 @@ "immutable": true, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1759 + "line": 1763 }, "name": "secondLevel", "type": { @@ -8296,7 +8324,7 @@ "immutable": true, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1754 + "line": 1758 }, "name": "optional", "optional": true, @@ -9053,5 +9081,5 @@ } }, "version": "0.14.3", - "fingerprint": "HflAzjllHZLqkcqeSKW9kn6gvwv+uDmBpezvDUDp9RY=" + "fingerprint": "Ld7vqOD5LvQ5a/I1LdRkj08XkdS+7syV580DELBDa+Y=" } diff --git a/packages/jsii-dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ComplianceTests.cs b/packages/jsii-dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ComplianceTests.cs index 62ad6a627c..be14ff0df7 100644 --- a/packages/jsii-dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ComplianceTests.cs +++ b/packages/jsii-dotnet-runtime-test/test/Amazon.JSII.Runtime.IntegrationTests/ComplianceTests.cs @@ -920,6 +920,12 @@ public void CallbacksCorrectlyDeserializeArguments() { var obj = new DataRendererSubclass(); Assert.Equal("{\n \"anumber\": 42,\n \"astring\": \"bazinga!\"\n}", obj.Render(null)); + + Assert.Equal("{\n \"Key\": {},\n \"Baz\": \"Zinga\"\n}", obj.RenderArbitrary(new Dictionary() + { + { "Key", obj }, + { "Baz", "Zinga" } + })); } class DataRendererSubclass : DataRenderer diff --git a/packages/jsii-dotnet-runtime/src/Amazon.JSII.Runtime/CallbackExtensions.cs b/packages/jsii-dotnet-runtime/src/Amazon.JSII.Runtime/CallbackExtensions.cs index 5456b8d9bd..359f147323 100644 --- a/packages/jsii-dotnet-runtime/src/Amazon.JSII.Runtime/CallbackExtensions.cs +++ b/packages/jsii-dotnet-runtime/src/Amazon.JSII.Runtime/CallbackExtensions.cs @@ -144,15 +144,20 @@ private static object FromKernel(object obj, IReferenceMap referenceMap) * result in an ArgumentError for not being able to convert JObject to IDictionary. */ var dict = ((JObject)obj).ToObject>(); + var mapped = new Dictionary(dict.Count); foreach (var key in dict.Keys) { var value = dict[key]; if (value != null && value.GetType() == typeof(JObject)) { - dict[key] = FromKernel(value, referenceMap); + mapped[key] = FromKernel(value, referenceMap); + } + else + { + mapped[key] = value; } } - return dict; + return mapped; } return obj; } diff --git a/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii b/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii index a97affaf2e..da2532f86d 100644 --- a/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii +++ b/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/.jsii @@ -2132,6 +2132,34 @@ "filename": "lib/compliance.ts", "line": 1740 }, + "name": "renderArbitrary", + "parameters": [ + { + "name": "data", + "type": { + "collection": { + "elementtype": { + "primitive": "any" + }, + "kind": "map" + } + } + } + ], + "returns": { + "type": { + "primitive": "string" + } + } + }, + { + "docs": { + "stability": "experimental" + }, + "locationInModule": { + "filename": "lib/compliance.ts", + "line": 1744 + }, "name": "renderMap", "parameters": [ { @@ -7160,7 +7188,7 @@ "kind": "interface", "locationInModule": { "filename": "lib/compliance.ts", - "line": 1762 + "line": 1766 }, "name": "SecondLevelStruct", "properties": [ @@ -7173,7 +7201,7 @@ "immutable": true, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1766 + "line": 1770 }, "name": "deeperRequiredProp", "type": { @@ -7189,7 +7217,7 @@ "immutable": true, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1771 + "line": 1775 }, "name": "deeperOptionalProp", "optional": true, @@ -7816,7 +7844,7 @@ "kind": "class", "locationInModule": { "filename": "lib/compliance.ts", - "line": 1779 + "line": 1783 }, "methods": [ { @@ -7825,7 +7853,7 @@ }, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1788 + "line": 1792 }, "name": "howManyVarArgsDidIPass", "parameters": [ @@ -7857,7 +7885,7 @@ }, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1780 + "line": 1784 }, "name": "roundTrip", "parameters": [ @@ -8242,7 +8270,7 @@ "kind": "interface", "locationInModule": { "filename": "lib/compliance.ts", - "line": 1745 + "line": 1749 }, "name": "TopLevelStruct", "properties": [ @@ -8255,7 +8283,7 @@ "immutable": true, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1749 + "line": 1753 }, "name": "required", "type": { @@ -8271,7 +8299,7 @@ "immutable": true, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1759 + "line": 1763 }, "name": "secondLevel", "type": { @@ -8296,7 +8324,7 @@ "immutable": true, "locationInModule": { "filename": "lib/compliance.ts", - "line": 1754 + "line": 1758 }, "name": "optional", "optional": true, @@ -9053,5 +9081,5 @@ } }, "version": "0.14.3", - "fingerprint": "HflAzjllHZLqkcqeSKW9kn6gvwv+uDmBpezvDUDp9RY=" + "fingerprint": "Ld7vqOD5LvQ5a/I1LdRkj08XkdS+7syV580DELBDa+Y=" } diff --git a/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/Amazon/JSII/Tests/CalculatorNamespace/DataRenderer.cs b/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/Amazon/JSII/Tests/CalculatorNamespace/DataRenderer.cs index 9f5471593a..f0d0cfc113 100644 --- a/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/Amazon/JSII/Tests/CalculatorNamespace/DataRenderer.cs +++ b/packages/jsii-pacmak/test/expected.jsii-calc/dotnet/Amazon.JSII.Tests.CalculatorPackageId/Amazon/JSII/Tests/CalculatorNamespace/DataRenderer.cs @@ -33,6 +33,15 @@ public virtual string Render(Amazon.JSII.Tests.CalculatorNamespace.LibNamespace. return InvokeInstanceMethod(new object[]{data}); } + /// + /// stability: Experimental + /// + [JsiiMethod(name: "renderArbitrary", returnsJson: "{\"type\":{\"primitive\":\"string\"}}", parametersJson: "[{\"name\":\"data\",\"type\":{\"collection\":{\"elementtype\":{\"primitive\":\"any\"},\"kind\":\"map\"}}}]")] + public virtual string RenderArbitrary(System.Collections.Generic.IDictionary data) + { + return InvokeInstanceMethod(new object[]{data}); + } + /// /// stability: Experimental /// diff --git a/packages/jsii-pacmak/test/expected.jsii-calc/java/src/main/java/software/amazon/jsii/tests/calculator/DataRenderer.java b/packages/jsii-pacmak/test/expected.jsii-calc/java/src/main/java/software/amazon/jsii/tests/calculator/DataRenderer.java index fa32f1b056..e3001d2c4e 100644 --- a/packages/jsii-pacmak/test/expected.jsii-calc/java/src/main/java/software/amazon/jsii/tests/calculator/DataRenderer.java +++ b/packages/jsii-pacmak/test/expected.jsii-calc/java/src/main/java/software/amazon/jsii/tests/calculator/DataRenderer.java @@ -37,6 +37,14 @@ public java.lang.String render() { return this.jsiiCall("render", java.lang.String.class); } + /** + * EXPERIMENTAL + */ + @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental) + public java.lang.String renderArbitrary(final java.util.Map data) { + return this.jsiiCall("renderArbitrary", java.lang.String.class, new Object[] { java.util.Objects.requireNonNull(data, "data is required") }); + } + /** * EXPERIMENTAL */ diff --git a/packages/jsii-pacmak/test/expected.jsii-calc/python/src/jsii_calc/__init__.py b/packages/jsii-pacmak/test/expected.jsii-calc/python/src/jsii_calc/__init__.py index 7b9ca638c8..33f04f884e 100644 --- a/packages/jsii-pacmak/test/expected.jsii-calc/python/src/jsii_calc/__init__.py +++ b/packages/jsii-pacmak/test/expected.jsii-calc/python/src/jsii_calc/__init__.py @@ -850,6 +850,16 @@ def render(self, *, anumber: jsii.Number, astring: str, first_optional: typing.O return jsii.invoke(self, "render", [data]) + @jsii.member(jsii_name="renderArbitrary") + def render_arbitrary(self, data: typing.Mapping[str,typing.Any]) -> str: + """ + :param data: - + + stability + :stability: experimental + """ + return jsii.invoke(self, "renderArbitrary", [data]) + @jsii.member(jsii_name="renderMap") def render_map(self, map: typing.Mapping[str,typing.Any]) -> str: """ diff --git a/packages/jsii-pacmak/test/expected.jsii-calc/sphinx/jsii-calc.rst b/packages/jsii-pacmak/test/expected.jsii-calc/sphinx/jsii-calc.rst index 14feaaad5c..e3e96793a9 100644 --- a/packages/jsii-pacmak/test/expected.jsii-calc/sphinx/jsii-calc.rst +++ b/packages/jsii-pacmak/test/expected.jsii-calc/sphinx/jsii-calc.rst @@ -1479,6 +1479,13 @@ DataRenderer :rtype: string + .. py:method:: renderArbitrary(data) -> string + + :param data: + :type data: string => any + :rtype: string + + .. py:method:: renderMap(map) -> string :param map: diff --git a/packages/jsii-reflect/test/jsii-tree.test.all.expected.txt b/packages/jsii-reflect/test/jsii-tree.test.all.expected.txt index 9ebcbd2683..47046fdcec 100644 --- a/packages/jsii-reflect/test/jsii-tree.test.all.expected.txt +++ b/packages/jsii-reflect/test/jsii-tree.test.all.expected.txt @@ -320,6 +320,11 @@ assemblies │ │ │ │ └─┬ data │ │ │ │ └── type: Optional<@scope/jsii-calc-lib.MyFirstStruct> │ │ │ └── returns: string + │ │ ├─┬ renderArbitrary(data) method (experimental) + │ │ │ ├─┬ parameters + │ │ │ │ └─┬ data + │ │ │ │ └── type: Map any> + │ │ │ └── returns: string │ │ └─┬ renderMap(map) method (experimental) │ │ ├─┬ parameters │ │ │ └─┬ map diff --git a/packages/jsii-reflect/test/jsii-tree.test.members.expected.txt b/packages/jsii-reflect/test/jsii-tree.test.members.expected.txt index b31f9b0669..c64c56bbcb 100644 --- a/packages/jsii-reflect/test/jsii-tree.test.members.expected.txt +++ b/packages/jsii-reflect/test/jsii-tree.test.members.expected.txt @@ -136,6 +136,7 @@ assemblies │ │ └─┬ members │ │ ├── () initializer │ │ ├── render(data) method + │ │ ├── renderArbitrary(data) method │ │ └── renderMap(map) method │ ├─┬ class DefaultedConstructorArgument │ │ └─┬ members