fix(kernel): calling super.property unexpectedly returns undefined (#…

…1932)

When overriding a property, the `@jsii/kernel` creates a shadow property
for the previous implementation. In order to preserve the existing
implementation (whether the property is dynamic or not), the property
descriptor is retrieved and re-declared with the new name.

This process did not crawl up the prototype chain, and only looked at
the object itself. In many cases, this will never return anything (since
`Object.getOwnPropertyDescriptor` will not return inherited descriptors)
and in this case the `undefined` value was always used.

This would break whenever attempting to override a property that was
inherited from a parent class, and appears to fail with certain `node`
runtimes where the dynamic properties are set on the object's prototype
and not on the object itself.

This change adds the necessary logic to traverse the prototype chain all
the way up to `Object` (not included), to locate the property descriptor
and upon failing to identify one, uses the current value of the property
instead of `undefined`.

packages/@jsii/kernel/lib/kernel.ts

@@ -612,8 +612,8 @@ export class Kernel {
 
     // save the old property under $jsii$super$<prop>$ so that property overrides
     // can still access it via `super.<prop>`.
-    const prev = Object.getOwnPropertyDescriptor(obj, propertyName) ?? {
-      value: undefined,
+    const prev = getPropertyDescriptor(obj, propertyName) ?? {
+      value: obj[propertyName],
       writable: true,
       enumerable: true,
       configurable: true,
@@ -656,6 +656,22 @@ export class Kernel {
         });
       },
     });
+
+    function getPropertyDescriptor(
+      obj: any,
+      propertyName: string,
+    ): PropertyDescriptor | undefined {
+      const direct = Object.getOwnPropertyDescriptor(obj, propertyName);
+      if (direct != null) {
+        return direct;
+      }
+      const proto = Object.getPrototypeOf(obj);
+      if (proto == null && proto !== Object.prototype) {
+        // We reached Object or the prototype chain root, all hope is lost!
+        return undefined;
+      }
+      return getPropertyDescriptor(proto, propertyName);
+    }
   }
 
   private _applyMethodOverride(

packages/@jsii/kernel/package.json

@@ -50,6 +50,7 @@
     "jsii-build-tools": "^0.0.0",
     "jsii-calc": "^0.0.0",
     "prettier": "^2.0.5",
+    "ts-jest": "^26.2.0",
     "typescript": "~3.9.7"
   },
   "jest": {
@@ -73,7 +74,10 @@
     ],
     "testEnvironment": "node",
     "testMatch": [
-      "**/?(*.)+(spec|test).js"
-    ]
+      "**/?(*.)+(spec|test).ts"
+    ],
+    "transform": {
+      "\\.tsx?$": "ts-jest"
+    }
   }
 }

packages/@jsii/kernel/test/kernel.test.ts

@@ -2066,6 +2066,69 @@ defineTest('ANY serializer: ref', (sandbox) => {
   });
 });
 
+defineTest('Override transitive property', (sandbox) => {
+  //////////
+  // GIVEN
+  //////////
+  const originalString = 'r00t';
+  const initialOverriddenPropValue = 'Overridden Value';
+  let propValue = initialOverriddenPropValue;
+  sandbox.callbackHandler = makeSyncCallbackHandler((callback) => {
+    const getOrSet = callback.get ?? callback.set;
+    // We don't expect to receive any other callback
+    expect(getOrSet).toBeDefined();
+    expect(getOrSet?.property).toBe('dynamicProperty');
+
+    if (callback.get) {
+      return propValue;
+    }
+    propValue = callback.set?.value;
+    return void 0;
+  });
+
+  //////////
+  // WHEN
+  //////////
+  const objref = sandbox.create({
+    fqn: 'jsii-calc.DynamicPropertyBearerChild',
+    args: [originalString],
+    overrides: [{ property: 'dynamicProperty' }],
+  });
+
+  //////////
+  // THEN
+  //////////
+
+  // Reading the "super" property
+  expect(sandbox.get({ objref, property: 'dynamicProperty' }).value).toBe(
+    originalString,
+  );
+  expect(sandbox.get({ objref, property: 'valueStore' }).value).toBe(
+    originalString,
+  );
+
+  // Setting the dynamicProperty value through the override
+  expect(
+    sandbox.invoke({ objref, method: 'overrideValue', args: ['N3W'] }).result,
+  ).toBe(initialOverriddenPropValue);
+  // Checking the side effect happened on the override:
+  expect(propValue).toBe('N3W');
+  // Check the "super" property didn't change:
+  expect(sandbox.get({ objref, property: 'dynamicProperty' }).value).toBe(
+    originalString,
+  );
+  expect(sandbox.get({ objref, property: 'valueStore' }).value).toBe(
+    originalString,
+  );
+
+  // Set the "super" property now
+  sandbox.set({ objref, property: 'dynamicProperty', value: '' });
+  // Check the side effect made it to the storage
+  expect(sandbox.get({ objref, property: 'valueStore' }).value).toBe('');
+  // Check the overridden property didn't change...
+  expect(propValue).toBe('N3W');
+});
+
 // =================================================================================================
 
 const testNames: { [name: string]: boolean } = {};

packages/@jsii/runtime/package.json

@@ -49,6 +49,7 @@
     "prettier": "^2.0.5",
     "source-map": "^0.7.3",
     "source-map-loader": "^1.0.2",
+    "ts-jest": "^26.2.0",
     "typescript": "~3.9.7",
     "wasm-loader": "^1.3.0",
     "webpack": "^4.44.1",
@@ -76,7 +77,10 @@
     ],
     "testEnvironment": "node",
     "testMatch": [
-      "**/?(*.)+(spec|test).js"
-    ]
+      "**/?(*.)+(spec|test).ts"
+    ],
+    "transform": {
+      "\\.tsx?$": "ts-jest"
+    }
   }
 }

packages/@jsii/runtime/test/playback.test.ts

@@ -42,7 +42,7 @@ function createRecords(): string {
       ...process.execArgv,
       require.resolve('jest/bin/jest'),
       '--coverage=false',
-      'test/kernel.test.js',
+      'test/kernel.test.ts',
     ],
     {
       env: { ...process.env, JSII_RECORD: records, JSII_NOSTACK: '1' },

packages/jsii-calc/lib/compliance.ts

@@ -2786,3 +2786,36 @@ export abstract class BurriedAnonymousObject {
 }
 
 import { StaticConsumer } from '@scope/jsii-calc-base-of-base';
+
+/**
+ * Ensures we can override a dynamic property that was inherited.
+ */
+export class DynamicPropertyBearer {
+  public constructor(public valueStore: string) {}
+
+  public get dynamicProperty(): string {
+    return this.valueStore;
+  }
+
+  public set dynamicProperty(value: string) {
+    this.valueStore = value;
+  }
+}
+export class DynamicPropertyBearerChild extends DynamicPropertyBearer {
+  public constructor(public readonly originalValue: string) {
+    super(originalValue);
+  }
+
+  /**
+   * Sets `this.dynamicProperty` to the new value, and returns the old value.
+   *
+   * @param newValue the new value to be set.
+   *
+   * @returns the old value that was set.
+   */
+  public overrideValue(newValue: string): string {
+    const oldValue = this.dynamicProperty;
+    this.dynamicProperty = newValue;
+    return oldValue;
+  }
+}

packages/jsii-calc/test/assembly.jsii

@@ -4283,6 +4283,142 @@
       ],
       "name": "DoubleTrouble"
     },
+    "jsii-calc.DynamicPropertyBearer": {
+      "assembly": "jsii-calc",
+      "docs": {
+        "stability": "experimental",
+        "summary": "Ensures we can override a dynamic property that was inherited."
+      },
+      "fqn": "jsii-calc.DynamicPropertyBearer",
+      "initializer": {
+        "docs": {
+          "stability": "experimental"
+        },
+        "locationInModule": {
+          "filename": "lib/compliance.ts",
+          "line": 2794
+        },
+        "parameters": [
+          {
+            "name": "valueStore",
+            "type": {
+              "primitive": "string"
+            }
+          }
+        ]
+      },
+      "kind": "class",
+      "locationInModule": {
+        "filename": "lib/compliance.ts",
+        "line": 2793
+      },
+      "name": "DynamicPropertyBearer",
+      "properties": [
+        {
+          "docs": {
+            "stability": "experimental"
+          },
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2796
+          },
+          "name": "dynamicProperty",
+          "type": {
+            "primitive": "string"
+          }
+        },
+        {
+          "docs": {
+            "stability": "experimental"
+          },
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2794
+          },
+          "name": "valueStore",
+          "type": {
+            "primitive": "string"
+          }
+        }
+      ]
+    },
+    "jsii-calc.DynamicPropertyBearerChild": {
+      "assembly": "jsii-calc",
+      "base": "jsii-calc.DynamicPropertyBearer",
+      "docs": {
+        "stability": "experimental"
+      },
+      "fqn": "jsii-calc.DynamicPropertyBearerChild",
+      "initializer": {
+        "docs": {
+          "stability": "experimental"
+        },
+        "locationInModule": {
+          "filename": "lib/compliance.ts",
+          "line": 2805
+        },
+        "parameters": [
+          {
+            "name": "originalValue",
+            "type": {
+              "primitive": "string"
+            }
+          }
+        ]
+      },
+      "kind": "class",
+      "locationInModule": {
+        "filename": "lib/compliance.ts",
+        "line": 2804
+      },
+      "methods": [
+        {
+          "docs": {
+            "returns": "the old value that was set.",
+            "stability": "experimental",
+            "summary": "Sets `this.dynamicProperty` to the new value, and returns the old value."
+          },
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2816
+          },
+          "name": "overrideValue",
+          "parameters": [
+            {
+              "docs": {
+                "summary": "the new value to be set."
+              },
+              "name": "newValue",
+              "type": {
+                "primitive": "string"
+              }
+            }
+          ],
+          "returns": {
+            "type": {
+              "primitive": "string"
+            }
+          }
+        }
+      ],
+      "name": "DynamicPropertyBearerChild",
+      "properties": [
+        {
+          "docs": {
+            "stability": "experimental"
+          },
+          "immutable": true,
+          "locationInModule": {
+            "filename": "lib/compliance.ts",
+            "line": 2805
+          },
+          "name": "originalValue",
+          "type": {
+            "primitive": "string"
+          }
+        }
+      ]
+    },
     "jsii-calc.EnumDispenser": {
       "assembly": "jsii-calc",
       "docs": {
@@ -13826,5 +13962,5 @@
     }
   },
   "version": "0.0.0",
-  "fingerprint": "2+VsCif006Pa9j/t5xrvI5psaT2mh2x58UwU31ByN5E="
+  "fingerprint": "cyoh4941c6DZUpcqFIkyprTV/kYc5HyNK4JT1ibPKIE="
 }