fix(kernel + java): skip overrides of private methods/properties (#254)

jsii-kernel will ignore override requests for methods/properties that
resolve on the object but are not defined in the public API of the
type.

java runtime will not request overrides for methods/properties that
use "private" accessibility.

Fixes #244

Author: Elad Ben-Israel

Diff for: package-lock.json

@@ -871,7 +871,6 @@ class ConcreteClass extends AbstractClass {
     }
 }
 
-
 export class AbstractClassReturner {
     public giveMeAbstract(): AbstractClass {
         return new ConcreteClass();
@@ -895,3 +894,23 @@ export interface MutableObjectLiteral {
 export class ClassWithMutableObjectLiteralProperty {
     public mutableObject: MutableObjectLiteral = { value: 'default' };
 }
+
+export class DoNotOverridePrivates {
+    private privateMethod(): string {
+        return 'privateMethod';
+    }
+
+    private privateProperty = 'privateProperty';
+
+    public privateMethodValue() {
+        return this.privateMethod();
+    }
+
+    public privatePropertyValue() {
+        return this.privateProperty;
+    }
+
+    public changePrivatePropertyValue(newValue: string) {
+        this.privateProperty = newValue;
+    }
+}

Diff for: packages/jsii-calc/lib/compliance.ts

@@ -1198,6 +1198,40 @@
         }
       ]
     },
+    "jsii-calc.DoNotOverridePrivates": {
+      "assembly": "jsii-calc",
+      "fqn": "jsii-calc.DoNotOverridePrivates",
+      "initializer": {
+        "initializer": true
+      },
+      "kind": "class",
+      "methods": [
+        {
+          "name": "changePrivatePropertyValue",
+          "parameters": [
+            {
+              "name": "newValue",
+              "type": {
+                "primitive": "string"
+              }
+            }
+          ]
+        },
+        {
+          "name": "privateMethodValue",
+          "returns": {
+            "primitive": "string"
+          }
+        },
+        {
+          "name": "privatePropertyValue",
+          "returns": {
+            "primitive": "string"
+          }
+        }
+      ],
+      "name": "DoNotOverridePrivates"
+    },
     "jsii-calc.DoubleTrouble": {
       "assembly": "jsii-calc",
       "fqn": "jsii-calc.DoubleTrouble",
@@ -3263,5 +3297,5 @@
     }
   },
   "version": "0.7.6",
-  "fingerprint": "BFT/z3s6IMAAkCjnq3nn+dZUxSqp7tx/E54uljg/XvQ="
+  "fingerprint": "IrPnQp841TiCOiG/Z2z18s0K8pxTwuMglW1UJ2t1zsM="
 }

Diff for: packages/jsii-calc/test/assembly.jsii

@@ -11,6 +11,7 @@
 import software.amazon.jsii.tests.calculator.Calculator;
 import software.amazon.jsii.tests.calculator.CalculatorProps;
 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.GiveMeStructs;
 import software.amazon.jsii.tests.calculator.IFriendlier;
@@ -837,6 +838,86 @@ public void returnAbstract() {
         assertEquals("hello-abstract-property", obj.getReturnAbstractFromProperty().getAbstractProperty());
     }
 
+    @Test
+    public void doNotOverridePrivates_method_public() {
+        DoNotOverridePrivates obj = new DoNotOverridePrivates() {
+            public String privateMethod() {
+                return "privateMethod-Override";
+            }
+        };
+
+        assertEquals("privateMethod", obj.privateMethodValue());
+    }
+
+    @Test
+    public void doNotOverridePrivates_method_private() {
+        DoNotOverridePrivates obj = new DoNotOverridePrivates() {
+            private String privateMethod() {
+                return "privateMethod-Override";
+            }
+        };
+
+        assertEquals("privateMethod", obj.privateMethodValue());
+    }
+
+    @Test
+    public void doNotOverridePrivates_property_by_name_private() {
+        DoNotOverridePrivates obj = new DoNotOverridePrivates() {
+            private String privateProperty() {
+                return "privateProperty-Override";
+            }
+        };
+
+        assertEquals("privateProperty", obj.privatePropertyValue());
+    }
+
+    @Test
+    public void doNotOverridePrivates_property_by_name_public() {
+        DoNotOverridePrivates obj = new DoNotOverridePrivates() {
+            public String privateProperty() {
+                return "privateProperty-Override";
+            }
+        };
+
+        assertEquals("privateProperty", obj.privatePropertyValue());
+    }
+
+    @Test
+    public void doNotOverridePrivates_property_getter_public() {
+        DoNotOverridePrivates obj = new DoNotOverridePrivates() {
+            public String getPrivateProperty() {
+                return "privateProperty-Override";
+            }
+            public void setPrivateProperty(String value) {
+                throw new RuntimeException("Boom");
+            }
+        };
+
+        assertEquals("privateProperty", obj.privatePropertyValue());
+
+        // verify the setter override is not invoked.
+        obj.changePrivatePropertyValue("MyNewValue");
+        assertEquals("MyNewValue", obj.privatePropertyValue());
+    }
+
+    @Test
+    public void doNotOverridePrivates_property_getter_private() {
+        DoNotOverridePrivates obj = new DoNotOverridePrivates() {
+            private String getPrivateProperty() {
+                return "privateProperty-Override";
+            }
+            public void setPrivateProperty(String value) {
+                throw new RuntimeException("Boom");
+            }
+        };
+
+        assertEquals("privateProperty", obj.privatePropertyValue());
+
+        // verify the setter override is not invoked.
+        obj.changePrivatePropertyValue("MyNewValue");
+        assertEquals("MyNewValue", obj.privatePropertyValue());
+    }
+
     static class MulTen extends Multiply {
         public MulTen(final int value) {
             super(new Number(value), new Number(10));

Diff for: packages/jsii-java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java

@@ -468,6 +468,10 @@ private static Collection<JsiiOverride> discoverOverrides(final Class<?> classTo
 
             // add all the methods in the current class
             for (Method method : klass.getDeclaredMethods()) {
+                if (Modifier.isPrivate(method.getModifiers())) {
+                    continue;
+                }
+
                 String methodName = method.getName();
 
                 // check if this is a property ("getXXX" or "setXXX", oh java!)

Diff for: packages/jsii-java-runtime/project/src/main/java/software/amazon/jsii/JsiiEngine.java

@@ -474,12 +474,7 @@ export class Kernel {
         const objref = this._createObjref(obj, fqn);
 
         // overrides: for each one of the override method names, installs a
-        // method on the newly created object which represents the remote
-        // override. Overrides are always async. When an override is called, it
-        // returns a promise which adds a callback to the pending callbacks
-        // list. This list is then retrieved by the client (using
-        // pendingCallbacks() and promises are fulfilled using
-        // completeCallback(), which in turn, fulfills the internal promise.
+        // method on the newly created object which represents the remote "reverse proxy".
 
         if (overrides) {
             this._debug('overrides', overrides);
@@ -495,19 +490,34 @@ export class Kernel {
 
                     methods.add(override.method);
 
-                    // check that the method being overridden actually exists on the
-                    // class and is an async method.
+                    // check that the method being overridden actually exists
                     let methodInfo;
                     if (fqn !== EMPTY_OBJECT_FQN) {
-                        methodInfo = this._tryTypeInfoForMethod(fqn, override.method); // throws if method cannot be found
+                        // error if we can find a property with this name
+                        if (this._tryTypeInfoForProperty(fqn, override.method)) {
+                            throw new Error(`Trying to override property '${override.method}' as a method`);
+                        }
+
+                        methodInfo = this._tryTypeInfoForMethod(fqn, override.method);
                     }
 
                     this._applyMethodOverride(obj, objref, override, methodInfo);
                 } else if (override.property) {
                     if (override.method) { throw new Error(overrideTypeErrorMessage); }
                     if (properties.has(override.property)) { throw Error(`Duplicate override for property '${override.property}'`); }
                     properties.add(override.property);
-                    this._applyPropertyOverride(obj, objref, override);
+
+                    let propInfo: spec.Property | undefined;
+                    if (fqn !== EMPTY_OBJECT_FQN) {
+                        // error if we can find a method with this name
+                        if (this._tryTypeInfoForMethod(fqn, override.property)) {
+                            throw new Error(`Trying to override method '${override.property}' as a property`);
+                        }
+
+                        propInfo = this._tryTypeInfoForProperty(fqn, override.property);
+                    }
+
+                    this._applyPropertyOverride(obj, objref, override, propInfo);
                 } else {
                     throw new Error(overrideTypeErrorMessage);
                 }
@@ -521,10 +531,16 @@ export class Kernel {
         return `$jsii$super$${name}$`;
     }
 
-    private _applyPropertyOverride(obj: any, objref: api.ObjRef, override: api.Override) {
+    private _applyPropertyOverride(obj: any, objref: api.ObjRef, override: api.Override, propInfo?: spec.Property) {
         const self = this;
         const propertyName = override.property!;
 
+        // if this is a private property (i.e. doesn't have `propInfo` the object has a key)
+        if (!propInfo && propertyName in obj) {
+            this._debug(`Skipping override of private property ${propertyName}`);
+            return;
+        }
+
         this._debug('apply override', propertyName);
 
         // save the old property under $jsii$super$<prop>$ so that property overrides
@@ -570,6 +586,13 @@ export class Kernel {
         const self = this;
         const methodName = override.method!;
 
+        // If this is a private method (doesn't have methodInfo, key resolves on the object), we
+        // are going to skip the override.
+        if (!methodInfo && obj[methodName]) {
+            this._debug(`Skipping override of private method ${methodName}`);
+            return;
+        }
+
         // note that we are applying the override even if the method doesn't exist
         // on the type spec in order to allow native code to override methods from
         // interfaces.
@@ -802,11 +825,10 @@ export class Kernel {
         return undefined;
     }
 
-    private _typeInfoForProperty(fqn: string, property: string): spec.Property {
+    private _tryTypeInfoForProperty(fqn: string, property: string): spec.Property | undefined {
         if (!fqn) {
             throw new Error('missing "fqn"');
         }
-
         const typeInfo = this._typeInfoForFqn(fqn);
 
         let properties;
@@ -832,10 +854,21 @@ export class Kernel {
 
         // recurse to parent type (if exists)
         for (const baseFqn of bases) {
-            return this._typeInfoForProperty(baseFqn, property);
+            const ret = this._tryTypeInfoForProperty(baseFqn, property);
+            if (ret) {
+                return ret;
+            }
         }
 
-        throw new Error(`Type ${typeInfo.fqn} doesn't have a property '${property}'`);
+        return undefined;
+    }
+
+    private _typeInfoForProperty(fqn: string, property: string): spec.Property {
+        const typeInfo = this._tryTypeInfoForProperty(fqn, property);
+        if (!typeInfo) {
+            throw new Error(`Type ${fqn} doesn't have a property '${property}'`);
+        }
+        return typeInfo;
     }
 
     private _toSandbox(v: any): any {

Diff for: packages/jsii-kernel/lib/kernel.ts

@@ -18,6 +18,8 @@ const calcVersion = require('jsii-calc/package.json').version.replace(/\+.+$/, '
 // tslint:disable:no-console
 // tslint:disable:max-line-length
 
+process.setMaxListeners(9999); // since every kernel instance adds an `on('exit')` handler.
+
 process.on('unhandledRejection', e => {
     console.error(e.stack);
     process.exit(1);
@@ -890,6 +892,52 @@ defineTest('object literals are returned by reference', async (test, sandbox) =>
     sandbox.del({ objref: property });
 });
 
+defineTest('overrides: method instead of property with the same name', async (test, sandbox) => {
+    test.throws(() => {
+        sandbox.create({ fqn: 'jsii-calc.SyncVirtualMethods', overrides: [
+            { method: 'theProperty' }
+        ]});
+    }, /Trying to override property/);
+});
+
+defineTest('overrides: property instead of method with the same name', async (test, sandbox) => {
+    test.throws(() => {
+        sandbox.create({ fqn: 'jsii-calc.SyncVirtualMethods', overrides: [
+            { property: 'virtualMethod' }
+        ]});
+    }, /Trying to override method/);
+});
+
+defineTest('overrides: skip overrides of private methods', async (test, sandbox) => {
+    const objref = sandbox.create({ fqn: 'jsii-calc.DoNotOverridePrivates', overrides: [
+        { method: 'privateMethod' }
+    ]});
+
+    sandbox.callbackHandler = makeSyncCallbackHandler(_ => {
+        test.ok(false, 'override callback should not be called');
+        return 'privateMethodBoom!';
+    });
+
+    const result = sandbox.invoke({ objref, method: 'privateMethodValue' });
+    test.deepEqual(result.result, 'privateMethod');
+});
+
+defineTest('overrides: skip overrides of private properties', async (test, sandbox) => {
+    const objref = sandbox.create({ fqn: 'jsii-calc.DoNotOverridePrivates', overrides: [
+        { property: 'privateProperty' }
+    ]});
+
+    sandbox.callbackHandler = makeSyncCallbackHandler(_ => {
+        test.ok(false, 'override callback should not be called');
+        return 'privatePropertyBoom!';
+    });
+
+    const result = sandbox.invoke({ objref, method: 'privatePropertyValue' });
+    test.deepEqual(result.result, 'privateProperty');
+});
+
+// =================================================================================================
+
 const testNames: { [name: string]: boolean } = { };
 
 async function createCalculatorSandbox(name: string) {