fix(java): pass java POJOs as values instead of as references

Implement a serialization method for Java POJOs (produced by calling `build()` on the generated builders such that they are passed by-value to JavaScript instead of by-reference. Also, erase any nulls passed in objects to JS, so they are treated as unset values for the purpose of `key in obj`. This fixes aws/aws-cdk#965 and fixes #375.
aws · Mar 12, 2019 · 6917b26 · 6917b26
1 parent cc3ec87
commit 6917b26
Show file tree

Hide file tree

Showing 44 changed files with 746 additions and 11 deletions.
diff --git a/packages/jsii-calc/lib/compliance.ts b/packages/jsii-calc/lib/compliance.ts
@@ -1335,3 +1335,20 @@ export class Constructors {
         return new PrivateClass();
     }
 }
+
+// fixture to verify that null/undefined values in object hashes are treated
+// as "unset". see awslabs/aws-cdk#965.
+export interface EraseUndefinedHashValuesOptions {
+    option1?: string;
+    option2?: string;
+}
+
+export class EraseUndefinedHashValues {
+    /**
+     * Returns `true` if `key` is defined in `opts`. Used to check that undefined/null hash values
+     * are being erased when sending values from native code to JS.
+     */
+    public static doesKeyExist(opts: EraseUndefinedHashValuesOptions, key: string): boolean {
+        return key in opts;
+    }
+}
diff --git a/packages/jsii-calc/test/assembly.jsii b/packages/jsii-calc/test/assembly.jsii
@@ -1452,6 +1452,66 @@
       ],
       "name": "DoubleTrouble"
     },
+    "jsii-calc.EraseUndefinedHashValues": {
+      "assembly": "jsii-calc",
+      "fqn": "jsii-calc.EraseUndefinedHashValues",
+      "initializer": {
+        "initializer": true
+      },
+      "kind": "class",
+      "methods": [
+        {
+          "docs": {
+            "comment": "Returns `true` if `key` is defined in `opts`. Used to check that undefined/null hash values\nare being erased when sending values from native code to JS."
+          },
+          "name": "doesKeyExist",
+          "parameters": [
+            {
+              "name": "opts",
+              "type": {
+                "fqn": "jsii-calc.EraseUndefinedHashValuesOptions"
+              }
+            },
+            {
+              "name": "key",
+              "type": {
+                "primitive": "string"
+              }
+            }
+          ],
+          "returns": {
+            "primitive": "boolean"
+          },
+          "static": true
+        }
+      ],
+      "name": "EraseUndefinedHashValues"
+    },
+    "jsii-calc.EraseUndefinedHashValuesOptions": {
+      "assembly": "jsii-calc",
+      "datatype": true,
+      "fqn": "jsii-calc.EraseUndefinedHashValuesOptions",
+      "kind": "interface",
+      "name": "EraseUndefinedHashValuesOptions",
+      "properties": [
+        {
+          "abstract": true,
+          "name": "option1",
+          "type": {
+            "optional": true,
+            "primitive": "string"
+          }
+        },
+        {
+          "abstract": true,
+          "name": "option2",
+          "type": {
+            "optional": true,
+            "primitive": "string"
+          }
+        }
+      ]
+    },
     "jsii-calc.ExportedBaseClass": {
       "assembly": "jsii-calc",
       "fqn": "jsii-calc.ExportedBaseClass",
@@ -3949,5 +4009,5 @@
     }
   },
   "version": "0.7.15",
-  "fingerprint": "IWSOEhdZzuvrss5K2WBjZCawXayV13yCAKTj/kJ9+mo="
+  "fingerprint": "posWS/nMgx0E/o0GXKtEIhC/dyYKJg151HrfX77VydI="
 }
diff --git a/...-java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java b/...-java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java
@@ -13,6 +13,8 @@
 import software.amazon.jsii.tests.calculator.DerivedStruct;
 import software.amazon.jsii.tests.calculator.DoNotOverridePrivates;
 import software.amazon.jsii.tests.calculator.DoubleTrouble;
+import software.amazon.jsii.tests.calculator.EraseUndefinedHashValues;
+import software.amazon.jsii.tests.calculator.EraseUndefinedHashValuesOptions;
 import software.amazon.jsii.tests.calculator.GiveMeStructs;
 import software.amazon.jsii.tests.calculator.GreetingAugmenter;
 import software.amazon.jsii.tests.calculator.IFriendlier;
@@ -63,6 +65,7 @@
 import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
@@ -982,6 +985,20 @@ public void objRefsAreLabelledUsingWithTheMostCorrectType() {
         assertTrue(ifaceRef instanceof IPublicInterface);
     }
 
+    /**
+     * Verifies that data values that are not set are recognized as unset keys
+     * in JavaScript-land. See https://github.com/awslabs/jsii/issues/375
+     */
+    @Test
+    public void eraseUnsetDataValues() {
+        EraseUndefinedHashValuesOptions opts = EraseUndefinedHashValuesOptions.builder()
+                .withOption1("option1")
+                .build();
+
+        assertTrue(EraseUndefinedHashValues.doesKeyExist(opts, "option1"));
+        assertFalse(EraseUndefinedHashValues.doesKeyExist(opts, "option2"));
+    }
+
     static class MulTen extends Multiply {
         public MulTen(final int value) {
             super(new Number(value), new Number(10));

diff --git a/...-java-runtime-test/project/src/test/java/software/amazon/jsii/testing/JsiiClientTest.java b/...-java-runtime-test/project/src/test/java/software/amazon/jsii/testing/JsiiClientTest.java
@@ -5,11 +5,14 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.databind.node.ArrayNode;
 import com.fasterxml.jackson.databind.node.JsonNodeFactory;
+import com.fasterxml.jackson.databind.node.ObjectNode;
 import software.amazon.jsii.JsiiClient;
 import software.amazon.jsii.JsiiException;
+import software.amazon.jsii.JsiiObjectMapper;
 import software.amazon.jsii.JsiiObjectRef;
 import software.amazon.jsii.JsiiPromise;
 import software.amazon.jsii.JsiiRuntime;
+import software.amazon.jsii.JsiiSerializable;
 import software.amazon.jsii.api.Callback;
 import software.amazon.jsii.api.JsiiOverride;
 import org.junit.Before;
@@ -217,6 +220,25 @@ public void staticMethods() {
         assertEquals("hello ,Foo!", result.textValue());
     }
 
+    /**
+     * If a JsiiSerializable object has a method named "$jsii$toJson", it will be
+     * used to serialize the object instead of the normal
+     */
+    @Test
+    public void serializeViaJsiiToJsonIfExists() {
+        JsiiObjectMapper om = JsiiObjectMapper.instance;
+        JsonNode result = om.valueToTree(new JsiiSerializable() {
+            public JsonNode $jsii$toJson() {
+                ObjectNode node = JSON.objectNode();
+                node.set("foo", OM.valueToTree("bar"));
+                node.set("hey", OM.valueToTree(42));
+                return node;
+            }
+        });
+
+        assertEquals("{\"foo\":\"bar\",\"hey\":42}", result.toString());
+    }
+
     private ArrayNode toSandboxArray(final Object... values) {
         return OM.valueToTree(values);
     }

diff --git a/packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiClient.java b/packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiClient.java
@@ -35,7 +35,7 @@ public final class JsiiClient {
     /**
      * Jsii custom object mapper.
      */
-    private static final JsiiObjectMapper JSII_OM = new JsiiObjectMapper();
+    private static final JsiiObjectMapper JSII_OM = JsiiObjectMapper.instance;
 
     /**
      * TCP port to connect to (always "localhost").

diff --git a/packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiEngine.java b/packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiEngine.java
@@ -48,7 +48,7 @@ public final class JsiiEngine implements JsiiCallbackHandler {
     /**
      * JSON object mapper.
      */
-    private static final JsiiObjectMapper OM = new JsiiObjectMapper();
+    private static final JsiiObjectMapper OM = JsiiObjectMapper.instance;
 
     /**
      * The set of modules we already loaded into the VM.

diff --git a/packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiObject.java b/packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiObject.java
@@ -1,6 +1,7 @@
 package software.amazon.jsii;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.core.TreeNode;
 import com.fasterxml.jackson.databind.JsonNode;
 
 import javax.annotation.Nullable;
@@ -13,7 +14,7 @@ public class JsiiObject implements JsiiSerializable {
     /**
      * JSON object mapper.
      */
-    private static final JsiiObjectMapper OM = new JsiiObjectMapper();
+    private static final JsiiObjectMapper OM = JsiiObjectMapper.instance;
 
     /**
      * The jsii engine used by this object.

diff --git a/packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiObjectMapper.java b/packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiObjectMapper.java
@@ -23,6 +23,10 @@
  * Implements serialization/deserialization of jsii data.
  */
 public final class JsiiObjectMapper {
+    /**
+     * Singleton instance of the object mapper.
+     */
+    public static JsiiObjectMapper instance = new JsiiObjectMapper();
 
     /**
      * JSON token that represents an object reference.
@@ -52,7 +56,7 @@ public final class JsiiObjectMapper {
     /**
      * Creates an object mapper.
      */
-    public JsiiObjectMapper() {
+    private JsiiObjectMapper() {
         this.serializer = new ObjectMapper();
         this.serializer.setSerializationInclusion(JsonInclude.Include.NON_NULL);
 
@@ -175,9 +179,8 @@ private static class JsiiSerializer extends JsonSerializer<JsiiSerializable> {
         public void serialize(final JsiiSerializable o,
                               final JsonGenerator jsonGenerator,
                               final SerializerProvider serializerProvider) throws IOException {
-            JsiiObjectRef objRef = JsiiEngine.getInstance().nativeToObjRef(o);
-            jsonGenerator.writeTree(objRef.toJson());
+
+            jsonGenerator.writeTree(o.$jsii$toJson());
         }
     }
-
 }
diff --git a/packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiSerializable.java b/packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiSerializable.java
@@ -1,5 +1,7 @@
 package software.amazon.jsii;
 
+import com.fasterxml.jackson.core.TreeNode;
+
 /**
  * Marks a class as serializable from native to javascript.
  * {@link JsiiObject} implements this as well as all generated jsii interfaces.
@@ -8,4 +10,12 @@
  * See {@link JsiiObjectMapper} for details.
  */
 public interface JsiiSerializable {
+    /**
+     * Serializes this object to JSON. The default behavior is to return an object reference.
+     * However, builders implement this method by emitting an actual JSON object of the key/values.
+     */
+    default TreeNode $jsii$toJson() {
+        JsiiObjectRef objRef = JsiiEngine.getInstance().nativeToObjRef(this);
+        return objRef.toJson();
+    }
 }
diff --git a/packages/jsii-kernel/lib/kernel.ts b/packages/jsii-kernel/lib/kernel.ts
@@ -930,7 +930,19 @@ export class Kernel {
         if (typeof v === 'object') {
             const out: any = { };
             for (const k of Object.keys(v)) {
-                out[k] = this._toSandbox(v[k]);
+                const value = this._toSandbox(v[k]);
+
+                // javascript has a fun behavior where
+                //     { ...{ x: 'hello' }, ...{ x: undefined } }
+                // will result in:
+                //     { x: undefined }
+                // so omit any keys that have an `undefined` values.
+                // see awslabs/aws-cdk#965 and compliance test "mergeObjects"
+                if (value === undefined) {
+                    continue;
+                }
+
+                out[k] = value;
             }
             return out;
         }
@@ -947,6 +959,10 @@ export class Kernel {
             return undefined;
         }
 
+        if (v === null) {
+            return undefined;
+        }
+
         // existing object
         const objid = v[OBJID_PROP];
         if (objid) {
@@ -1019,7 +1035,11 @@ export class Kernel {
             this._debug('map', v);
             const out: any = { };
             for (const k of Object.keys(v)) {
-                out[k] = this._fromSandbox(v[k]);
+                const value = this._fromSandbox(v[k]);
+                if (value === undefined) {
+                    continue;
+                }
+                out[k] = value;
             }
             return out;
         }

diff --git a/packages/jsii-kernel/test/test.kernel.ts b/packages/jsii-kernel/test/test.kernel.ts
@@ -973,6 +973,29 @@ defineTest('ObjRefs are labeled with the "most correct" type', async (test, sand
             `${ifaceRef[api.TOKEN_REF]} starts with jsii-calc.IPublicInterface`);
 });
 
+defineTest('toSandbox: "null" in hash values send to JS should be treated as non-existing key', async (test, sandbox) => {
+    const input = { option1: null, option2: 'hello' };
+    const option1Exists = sandbox.sinvoke({ fqn: 'jsii-calc.EraseUndefinedHashValues', method: 'doesKeyExist', args: [ input, 'option1' ] });
+    test.equal(option1Exists.result, false);
+
+    const option2Exists = sandbox.sinvoke({ fqn: 'jsii-calc.EraseUndefinedHashValues', method: 'doesKeyExist', args: [ input, 'option2' ] });
+    test.equal(option2Exists.result, true);
+});
+
+defineTest('toSandbox: "undefined" in hash values sent to JS should be treated as non-existing key', async (test, sandbox) => {
+    const input = { option1: undefined, option2: 'hello' };
+    const option1Exists = sandbox.sinvoke({ fqn: 'jsii-calc.EraseUndefinedHashValues', method: 'doesKeyExist', args: [ input, 'option1' ] });
+    test.equal(option1Exists.result, false);
+    const option2Exists = sandbox.sinvoke({ fqn: 'jsii-calc.EraseUndefinedHashValues', method: 'doesKeyExist', args: [ input, 'option2' ] });
+    test.equal(option2Exists.result, true);
+});
+
+defineTest('fromSandbox: "null" in hash values returned from JS should be treated as non-existing keys', async (test, sandbox) => {
+    const output = sandbox.sinvoke({ fqn: 'jsii-calc.EraseUndefinedHashValues', method: 'returnHashWithUndefinedOption1' });
+    const option1 = sandbox.get({ objref: output.result, property: 'option1' });
+    test.equal(option1, {});
+});
+
 // =================================================================================================
 
 const testNames: { [name: string]: boolean } = { };

diff --git a/packages/jsii-pacmak/lib/targets/java.ts b/packages/jsii-pacmak/lib/targets/java.ts
@@ -800,6 +800,22 @@ class JavaGenerator extends Generator {
                 }
             }
         }
+
+        // emit $jsii$toJson which will be called to serialize this object when sent to JS
+        this.code.line();
+        this.code.openBlock(`public com.fasterxml.jackson.databind.JsonNode $jsii$toJson()`);
+        this.code.line(`software.amazon.jsii.JsiiObjectMapper om = software.amazon.jsii.JsiiObjectMapper.instance;`);
+        // tslint:disable-next-line:max-line-length
+        this.code.line(`com.fasterxml.jackson.databind.node.ObjectNode obj = com.fasterxml.jackson.databind.node.JsonNodeFactory.instance.objectNode();`);
+
+        for (const prop of props) {
+            this.code.line(`obj.set(\"${prop.spec.name}\", om.valueToTree(this.get${prop.propName}()));`);
+        }
+
+        this.code.line(`return obj;`);
+
+        this.code.closeBlock();
+
         this.code.unindent();
         this.code.line(`};`); /* return new Foo() */
 

diff --git a/...ii-calc-base/java/src/main/java/software/amazon/jsii/tests/calculator/base/BaseProps.java b/...ii-calc-base/java/src/main/java/software/amazon/jsii/tests/calculator/base/BaseProps.java
@@ -68,6 +68,14 @@ public void setFoo(final software.amazon.jsii.tests.calculator.baseofbase.Very v
                     this.$foo = java.util.Objects.requireNonNull(value, "foo is required");
                 }
 
+                public com.fasterxml.jackson.databind.JsonNode $jsii$toJson() {
+                    software.amazon.jsii.JsiiObjectMapper om = software.amazon.jsii.JsiiObjectMapper.instance;
+                    com.fasterxml.jackson.databind.node.ObjectNode obj = com.fasterxml.jackson.databind.node.JsonNodeFactory.instance.objectNode();
+                    obj.set("bar", om.valueToTree(this.getBar()));
+                    obj.set("foo", om.valueToTree(this.getFoo()));
+                    return obj;
+                }
+
             };
         }
     }

diff --git a/...-calc-lib/java/src/main/java/software/amazon/jsii/tests/calculator/lib/MyFirstStruct.java b/...-calc-lib/java/src/main/java/software/amazon/jsii/tests/calculator/lib/MyFirstStruct.java
@@ -110,6 +110,15 @@ public void setFirstOptional(@javax.annotation.Nullable final java.util.List<jav
                     this.$firstOptional = value;
                 }
 
+                public com.fasterxml.jackson.databind.JsonNode $jsii$toJson() {
+                    software.amazon.jsii.JsiiObjectMapper om = software.amazon.jsii.JsiiObjectMapper.instance;
+                    com.fasterxml.jackson.databind.node.ObjectNode obj = com.fasterxml.jackson.databind.node.JsonNodeFactory.instance.objectNode();
+                    obj.set("anumber", om.valueToTree(this.getAnumber()));
+                    obj.set("astring", om.valueToTree(this.getAstring()));
+                    obj.set("firstOptional", om.valueToTree(this.getFirstOptional()));
+                    return obj;
+                }
+
             };
         }
     }