fix(kernel): validate presence of required struct properties (#591)

Detect when required struct properties are not supplied, so that
we can produce useful error messages to untyped jsii languages (in
particular, Python).

Also check for an array in place of a variadic argument, and throw
a hopefully helpful message to hint at a common misuse.

Fix the test to be actually variadic, which it wasn't :o.

Author: rix0rrr

packages/jsii-calc/lib/compliance.ts

@@ -1780,7 +1780,7 @@ export class StructPassing {
         };
     }
 
-    public static howManyVarArgsDidIPass(_positional: number, inputs: TopLevelStruct[]): number {
+    public static howManyVarArgsDidIPass(_positional: number, ...inputs: TopLevelStruct[]): number {
         return inputs.length;
     }
 }

packages/jsii-calc/test/assembly.jsii

@@ -7837,21 +7837,18 @@
             {
               "name": "inputs",
               "type": {
-                "collection": {
-                  "elementtype": {
-                    "fqn": "jsii-calc.TopLevelStruct"
-                  },
-                  "kind": "array"
-                }
-              }
+                "fqn": "jsii-calc.TopLevelStruct"
+              },
+              "variadic": true
             }
           ],
           "returns": {
             "type": {
               "primitive": "number"
             }
           },
-          "static": true
+          "static": true,
+          "variadic": true
         },
         {
           "docs": {
@@ -9055,5 +9052,5 @@
     }
   },
   "version": "0.14.0",
-  "fingerprint": "FCuQcrhxNzRu5psy8XKe+fkbvJhTtFAEc2eiULxQP8w="
+  "fingerprint": "3KJr12zCrNOW5f2XkO6HxM761PQoUBbT9VLR5+foBOo="
 }

packages/jsii-kernel/lib/serialization.ts

@@ -170,7 +170,7 @@ export const SERIALIZERS: {[k: string]: Serializer} = {
       return value;
     },
     deserialize(value, optionalValue) {
-      // /!\ Top-level "null" will turn to underfined, but any null nested in the value is valid JSON, so it'll stay!
+      // /!\ Top-level "null" will turn to undefined, but any null nested in the value is valid JSON, so it'll stay!
       if (nullAndOk(value, optionalValue)) { return undefined; }
       return value;
     },
@@ -258,7 +258,7 @@ export const SERIALIZERS: {[k: string]: Serializer} = {
         throw new Error(`Expected object, got ${JSON.stringify(value)}`);
       }
 
-      // This looks odd, but if an object was originally passed in as a by-ref
+      // This looks odd, but if an object was originally passed in/out as a by-ref
       // class, and it happens to conform to a datatype interface we say we're
       // returning, return the actual object instead of the serialized value.
       // NOTE: Not entirely sure yet whether this is a bug masquerading as a
@@ -296,16 +296,29 @@ export const SERIALIZERS: {[k: string]: Serializer} = {
         throw new Error(`Expected object reference, got ${JSON.stringify(value)}`);
       }
 
-      // Similarly to other end, we might be getting a reference type where we're
-      // expecting a value type. Accept this for now.
+      const namedType = host.lookupType((optionalValue.type as spec.NamedTypeReference).fqn);
+      const props = propertiesOf(namedType, host.lookupType);
+
+      if (Array.isArray(value)) {
+        throw new Error(`Got an array where a ${namedType.fqn} was expected. Did you mean to pass a variable number of arguments?`);
+      }
+
+      // Similarly to serialization, we might be getting a reference type where we're
+      // expecting a value type. Accept this for now (but also validate that object
+      // for presence of the right properties).
       if (isObjRef(value)) {
         host.debug('Expected value type but got reference type, accepting for now (awslabs/jsii#400)');
-        return host.objects.findObject(value).instance;
+
+        // Return same INSTANCE (shouldn't matter but we don't know for sure that it doesn't)
+        return validateRequiredProps(
+          host.objects.findObject(value).instance,
+          namedType.fqn,
+          props);
       }
 
-      const namedType = host.lookupType((optionalValue.type as spec.NamedTypeReference).fqn);
-      const props = propertiesOf(namedType, host.lookupType);
+      value = validateRequiredProps(value, namedType.fqn, props);
 
+      // Return a dict COPY, we have by-value semantics anyway.
       return mapValues(value, (v, key) => {
         if (!props[key]) { return undefined; } // Don't map if unknown property
         return host.recurse(v, props[key]);
@@ -648,3 +661,16 @@ function isAssignable(actualTypeFqn: string, requiredType: spec.NamedTypeReferen
   }
   return false;
 }
+
+function validateRequiredProps(actualProps: {[key: string]: any}, typeName: string, specProps: {[key: string]: spec.Property}) {
+  // Check for required properties
+  const missingRequiredProps = Object.keys(specProps)
+      .filter(name => !specProps[name].optional)
+      .filter(name => !(name in actualProps));
+
+  if (missingRequiredProps.length > 0) {
+    throw new Error(`Missing required properties for ${typeName}: ${missingRequiredProps}`);
+  }
+
+  return actualProps;
+}

packages/jsii-kernel/test/test.kernel.ts

@@ -1118,6 +1118,36 @@ defineTest('can set and retrieve union properties', async (test, sandbox) => {
     test.equal(get(sandbox, unionArray[1])('value'), 33);
 });
 
+defineTest('require presence of required properties -- top level', async (test, sandbox) => {
+    test.throws(() => {
+        sandbox.sinvoke({ fqn: 'jsii-calc.StructPassing', method: 'roundTrip', args: [
+            123,
+            { incomplete: true }
+        ]});
+    }, /Missing required properties for jsii-calc.TopLevelStruct: required,secondLevel/);
+});
+
+defineTest('require presence of required properties -- deeper level', async (test, sandbox) => {
+    test.throws(() => {
+        sandbox.sinvoke({ fqn: 'jsii-calc.StructPassing', method: 'roundTrip', args: [
+            123,
+            {
+                required: 'abc',
+                secondLevel: { alsoIncomplete: true, }
+            }
+        ]});
+    }, /Missing required properties for jsii-calc.SecondLevelStruct: deeperRequiredProp/);
+});
+
+defineTest('notice when an array is passed instead of varargs', async (test, sandbox) => {
+    test.throws(() => {
+        sandbox.sinvoke({ fqn: 'jsii-calc.StructPassing', method: 'howManyVarArgsDidIPass', args: [
+            123,
+            [ { required: 'abc', secondLevel: 6 } ]
+        ]});
+    }, /Got an array where a jsii-calc.TopLevelStruct was expected/);
+});
+
 defineTest('Object ID does not get re-allocated when the constructor passes "this" out', async (test, sandbox) => {
     sandbox.callbackHandler = makeSyncCallbackHandler((callback) => {
         test.equal(callback.invoke && callback.invoke.method, 'consumePartiallyInitializedThis');

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

@@ -7837,21 +7837,18 @@
             {
               "name": "inputs",
               "type": {
-                "collection": {
-                  "elementtype": {
-                    "fqn": "jsii-calc.TopLevelStruct"
-                  },
-                  "kind": "array"
-                }
-              }
+                "fqn": "jsii-calc.TopLevelStruct"
+              },
+              "variadic": true
             }
           ],
           "returns": {
             "type": {
               "primitive": "number"
             }
           },
-          "static": true
+          "static": true,
+          "variadic": true
         },
         {
           "docs": {
@@ -9055,5 +9052,5 @@
     }
   },
   "version": "0.14.0",
-  "fingerprint": "FCuQcrhxNzRu5psy8XKe+fkbvJhTtFAEc2eiULxQP8w="
+  "fingerprint": "3KJr12zCrNOW5f2XkO6HxM761PQoUBbT9VLR5+foBOo="
 }

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

@@ -19,8 +19,8 @@ protected StructPassing(DeputyProps props): base(props)
         }
 
         /// <remarks>stability: Experimental</remarks>
-        [JsiiMethod(name: "howManyVarArgsDidIPass", returnsJson: "{\"type\":{\"primitive\":\"number\"}}", parametersJson: "[{\"name\":\"_positional\",\"type\":{\"primitive\":\"number\"}},{\"name\":\"inputs\",\"type\":{\"collection\":{\"kind\":\"array\",\"elementtype\":{\"fqn\":\"jsii-calc.TopLevelStruct\"}}}}]")]
-        public static double HowManyVarArgsDidIPass(double _positional, ITopLevelStruct[] inputs)
+        [JsiiMethod(name: "howManyVarArgsDidIPass", returnsJson: "{\"type\":{\"primitive\":\"number\"}}", parametersJson: "[{\"name\":\"_positional\",\"type\":{\"primitive\":\"number\"}},{\"name\":\"inputs\",\"variadic\":true,\"type\":{\"fqn\":\"jsii-calc.TopLevelStruct\"}}]")]
+        public static double HowManyVarArgsDidIPass(double _positional, ITopLevelStruct inputs)
         {
             return InvokeStaticMethod<double>(typeof(StructPassing), new object[]{_positional, inputs});
         }

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

@@ -19,8 +19,8 @@ public StructPassing() {
      * EXPERIMENTAL
      */
     @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental)
-    public static java.lang.Number howManyVarArgsDidIPass(final java.lang.Number _positional, final java.util.List<software.amazon.jsii.tests.calculator.TopLevelStruct> inputs) {
-        return software.amazon.jsii.JsiiObject.jsiiStaticCall(software.amazon.jsii.tests.calculator.StructPassing.class, "howManyVarArgsDidIPass", java.lang.Number.class, new Object[] { java.util.Objects.requireNonNull(_positional, "_positional is required"), java.util.Objects.requireNonNull(inputs, "inputs is required") });
+    public static java.lang.Number howManyVarArgsDidIPass(final java.lang.Number _positional, final software.amazon.jsii.tests.calculator.TopLevelStruct... inputs) {
+        return software.amazon.jsii.JsiiObject.jsiiStaticCall(software.amazon.jsii.tests.calculator.StructPassing.class, "howManyVarArgsDidIPass", java.lang.Number.class, java.util.stream.Stream.concat(java.util.Arrays.<Object>stream(new Object[] { java.util.Objects.requireNonNull(_positional, "_positional is required") }), java.util.Arrays.<Object>stream(inputs)).toArray(Object[]::new));
     }
 
     /**

packages/jsii-pacmak/test/expected.jsii-calc/python/src/jsii_calc/__init__.py

@@ -5341,15 +5341,15 @@ def __init__(self) -> None:
 
     @jsii.member(jsii_name="howManyVarArgsDidIPass")
     @classmethod
-    def how_many_var_args_did_i_pass(cls, _positional: jsii.Number, inputs: typing.List["TopLevelStruct"]) -> jsii.Number:
+    def how_many_var_args_did_i_pass(cls, _positional: jsii.Number, *inputs: "TopLevelStruct") -> jsii.Number:
         """
         :param _positional: -
         :param inputs: -
 
         stability
         :stability: experimental
         """
-        return jsii.sinvoke(cls, "howManyVarArgsDidIPass", [_positional, inputs])
+        return jsii.sinvoke(cls, "howManyVarArgsDidIPass", [_positional, *inputs])
 
     @jsii.member(jsii_name="roundTrip")
     @classmethod

packages/jsii-pacmak/test/expected.jsii-calc/sphinx/jsii-calc.rst

@@ -6436,12 +6436,12 @@ StructPassing
 
 
 
-   .. py:staticmethod:: howManyVarArgsDidIPass(_positional, inputs) -> number
+   .. py:staticmethod:: howManyVarArgsDidIPass(_positional, *inputs) -> number
 
       :param _positional: 
       :type _positional: number
-      :param inputs: 
-      :type inputs: :py:class:`~jsii-calc.TopLevelStruct`\ []
+      :param \*inputs: 
+      :type \*inputs: :py:class:`~jsii-calc.TopLevelStruct`\ 
       :rtype: number
 
 

packages/jsii-python-runtime/tests/test_compliance.py

@@ -936,10 +936,10 @@ def test_passNestedScalar():
     assert output.second_level == 5
 
 def test_passStructsInVariadic():
-    output = StructPassing.how_many_var_args_did_i_pass(123, [
+    output = StructPassing.how_many_var_args_did_i_pass(123,
         TopLevelStruct(required='hello', second_level=1),
         TopLevelStruct(required='bye', second_level=SecondLevelStruct(deeper_required_prop='ciao'))
-    ])
+    )
     assert output == 2
 
 def test_structEquality():