From 44282214cf38991c7a1b0f5131a36b94634a35da 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: Tue, 5 Nov 2019 10:05:47 +0100 Subject: [PATCH] fix(kernel): revert behavior change around `any` serialization The change of behavior in `any` serialization introduced in #825 risks breaking some emerging use-cases of the AWS CDK that are likely to be prevalent in the installed user base. As a consequence, surgically reverting this particular change in order to restore the behavior that users might be dependent on. This somehow uncovered an outstanding bug in the Python runtime that was not triggering with the "reverted" behavior (it broke unit tests). The runtime was fixed in order to address this bug. --- packages/jsii-kernel/lib/objects.ts | 17 --- packages/jsii-kernel/lib/serialization.ts | 101 ++++++------------ packages/jsii-kernel/test/kernel.test.ts | 22 ++-- .../src/jsii/_reference_map.py | 63 ++++++++--- 4 files changed, 87 insertions(+), 116 deletions(-) diff --git a/packages/jsii-kernel/lib/objects.ts b/packages/jsii-kernel/lib/objects.ts index 620ce94b44..249718e739 100644 --- a/packages/jsii-kernel/lib/objects.ts +++ b/packages/jsii-kernel/lib/objects.ts @@ -58,23 +58,6 @@ function tagObject(obj: object, objid: string, interfaces?: string[]) { managed[IFACES_SYMBOL] = interfaces; } -/** - * Ensure there's a hidden map with the given symbol name on the given object, and return it - */ -export function hiddenMap(obj: any, mapSymbol: symbol): {[key: string]: T} { - let map: any = obj[mapSymbol]; - if (!map) { - map = {}; - Object.defineProperty(obj, mapSymbol, { - value: map, - configurable: false, - enumerable: false, - writable: false - }); - } - return map; -} - /** * Set the JSII FQN for classes produced by a given constructor */ diff --git a/packages/jsii-kernel/lib/serialization.ts b/packages/jsii-kernel/lib/serialization.ts index 64f17adf30..5e5174b6c1 100644 --- a/packages/jsii-kernel/lib/serialization.ts +++ b/packages/jsii-kernel/lib/serialization.ts @@ -28,7 +28,7 @@ import * as spec from 'jsii-spec'; import { isObjRef, isWireDate, isWireEnum, isWireMap, ObjRef, TOKEN_DATE, TOKEN_ENUM, TOKEN_MAP, WireDate, WireEnum } from './api'; -import { hiddenMap, jsiiTypeFqn, objectReference, ObjectTable } from './objects'; +import { jsiiTypeFqn, objectReference, ObjectTable } from './objects'; import { api } from '.'; /** @@ -273,14 +273,6 @@ 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/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 - // feature or not. - const prevRef = objectReference(value); - if (prevRef) { return prevRef; } - /* This is what we'd like to do, but we can't because at least the Java client does not understand by-value serialized interface types, so we'll have to @@ -298,8 +290,7 @@ export const SERIALIZERS: {[k: string]: Serializer} = { host.debug('Returning value type by reference'); - const wireFqn = selectWireType(value, optionalValue.type as spec.NamedTypeReference, host.lookupType); - return host.objects.registerObject(value, wireFqn); + return host.objects.registerObject(value, 'Object', [(optionalValue.type as spec.NamedTypeReference).fqn]); }, deserialize(value, optionalValue, host) { if (typeof value === 'object' && Object.keys(value || {}).length === 0) { @@ -361,11 +352,13 @@ export const SERIALIZERS: {[k: string]: Serializer} = { throw new Error(`Expected object reference, got ${JSON.stringify(value)}`); } - const prevRef = objectReference(value); - if (prevRef) { return prevRef; } + const expectedType = host.lookupType((optionalValue.type as spec.NamedTypeReference).fqn); + const interfaces = spec.isInterfaceType(expectedType) + ? [expectedType.fqn] + : undefined; + const jsiiType = jsiiTypeFqn(value) || (spec.isClassType(expectedType) ? expectedType.fqn : 'Object'); - const wireFqn = selectWireType(value, optionalValue.type as spec.NamedTypeReference, host.lookupType); - return host.objects.registerObject(value, wireFqn); + return host.objects.registerObject(value, jsiiType, interfaces); }, deserialize(value, optionalValue, host) { if (nullAndOk(value, optionalValue)) { return undefined; } @@ -400,7 +393,7 @@ export const SERIALIZERS: {[k: string]: Serializer} = { // ---------------------------------------------------------------------- [SerializationClass.Any]: { - serialize(value, type, host) { + serialize(value, _type, host) { if (value == null) { return undefined; } if (isDate(value)) { return serializeDate(value); } @@ -412,6 +405,10 @@ export const SERIALIZERS: {[k: string]: Serializer} = { // Note: no case for "ENUM" here, without type declaration we can't tell the difference // between an enum member and a scalar. + if (typeof value === 'function') { + throw new Error('JSII Kernel is unable to serialize `function`. An instance with methods might have been returned by an `any` method?'); + } + if (typeof value !== 'object' || value == null) { throw new Error(`JSII kernel assumption violated, ${JSON.stringify(value)} is not an object`); } @@ -436,12 +433,24 @@ export const SERIALIZERS: {[k: string]: Serializer} = { // way, and the by-value serialized object will be quite useless. if (value instanceof Set || value instanceof Map) { throw new Error("Can't return objects of type Set or Map"); } - // Pass-by-reference, so we're sure we don't end up doing anything unexpected - const jsiiType = jsiiTypeFqn(value) || EMPTY_OBJECT_FQN; - const interfaces = type !== 'void' && spec.isNamedTypeReference(type.type) - ? [type.type.fqn] - : undefined; - return host.objects.registerObject(value, jsiiType, interfaces); + // Use a previous reference to maintain object identity. NOTE: this may cause us to return + // a different type than requested! This is just how it is right now. + // https://github.com/aws/jsii/issues/399 + const prevRef = objectReference(value); + if (prevRef) { return prevRef; } + + // If this is or should be a reference type, pass or make the reference + // (Like regular reftype serialization, but without the type derivation to an interface) + const jsiiType = jsiiTypeFqn(value); + if (jsiiType) { return host.objects.registerObject(value, jsiiType); } + + // At this point we have an object that is not of an exported type. Either an object + // literal, or an instance of a fully private class (cannot distinguish those cases). + + // We will serialize by-value, but recurse for serialization so that if + // the object contains reference objects, they will be serialized appropriately. + // (Basically, serialize anything else as a map of 'any'). + return mapValues(value, (v) => host.recurse(v, { type: spec.CANONICAL_ANY })); }, deserialize(value, _type, host) { @@ -561,10 +570,8 @@ export function serializationType(typeRef: OptionalValueOrVoid, lookup: TypeLook return [{ serializationClass: SerializationClass.Enum, typeRef }]; } - if (spec.isInterfaceType(type)) { - return type.datatype - ? [{ serializationClass: SerializationClass.Struct, typeRef }] - : [{ serializationClass: SerializationClass.Any, typeRef }]; + if (spec.isInterfaceType(type) && type.datatype) { + return [{ serializationClass: SerializationClass.Struct, typeRef }]; } return [{ serializationClass: SerializationClass.ReferenceType, typeRef }]; @@ -632,48 +639,6 @@ function propertiesOf(t: spec.Type, lookup: TypeLookup): {[name: string]: spec.P return ret; } -const WIRE_TYPE_MAP = Symbol('$__jsii_wire_type__$'); - -/** - * Select the wire type for the given object and requested type - * - * Should return the most specific type that is in the JSII assembly and - * assignable to the required type. - * - * We actually don't need to search much; because of prototypal constructor - * linking, object.constructor.__jsii__ will have the FQN of the most specific - * exported JSII class this object is an instance of. - * - * Either that's assignable to the requested type, in which case we return it, - * or it's not, in which case there's a hidden class that implements the interface - * and we just return the interface so the other side can instantiate an interface - * proxy for it. - * - * Cache the analysis on the object to avoid having to do too many searches through - * the type system for repeated accesses on the same object. - */ -function selectWireType(obj: any, expectedType: spec.NamedTypeReference, lookup: TypeLookup): string { - const map = hiddenMap(obj, WIRE_TYPE_MAP); - - if (!(expectedType.fqn in map)) { - const jsiiType = jsiiTypeFqn(obj); - if (jsiiType) { - const assignable = isAssignable(jsiiType, expectedType, lookup); - - // If we're not assignable and both types are class types, this cannot be satisfied. - if (!assignable && spec.isClassType(lookup(expectedType.fqn))) { - throw new Error(`Object of type ${jsiiType} is not convertible to ${expectedType.fqn}`); - } - - map[expectedType.fqn] = assignable ? jsiiType : expectedType.fqn; - } else { - map[expectedType.fqn] = expectedType.fqn; - } - } - - return map[expectedType.fqn]; -} - /** * Tests whether a given type (by it's FQN) can be assigned to a named type reference. * diff --git a/packages/jsii-kernel/test/kernel.test.ts b/packages/jsii-kernel/test/kernel.test.ts index 48486251ba..d156392e98 100644 --- a/packages/jsii-kernel/test/kernel.test.ts +++ b/packages/jsii-kernel/test/kernel.test.ts @@ -32,27 +32,19 @@ if (recordingOutput) { console.error(`JSII_RECORD=${recordingOutput}`); } -function defineTest(name: string, method: (sandbox: Kernel) => Promise | any) { +function defineTest(name: string, method: (sandbox: Kernel) => Promise | any, testFunc = test) { const recording = name.replace(/[^A-Za-z]/g, '_'); - test(name, async () => { + testFunc(name, async () => { const kernel = await createCalculatorSandbox(recording); await method(kernel); - await closeRecording(kernel); + return closeRecording(kernel); }); } -defineTest('sandbox allows loading arbitrary javascript into it', (sandbox) => { - const objid = sandbox.create({ fqn: '@scope/jsii-calc-lib.Number', args: [12] }); - expect(sandbox.get({ objref: objid, property: 'doubleValue' }).value).toBe(24); - expect(sandbox.invoke({ objref: objid, method: 'typeName', args: [] }).result).toBe('Number'); - - const lhs = sandbox.create({ fqn: '@scope/jsii-calc-lib.Number', args: [10] }); - const rhs = sandbox.create({ fqn: '@scope/jsii-calc-lib.Number', args: [20] }); - const add = sandbox.create({ fqn: 'jsii-calc.Add', args: [lhs, rhs] }); - - expect(sandbox.get({ objref: add, property: 'value' }).value).toBe(30); -}); +defineTest.skip = function (name: string, method: (sandbox: Kernel) => Promise | any) { + return defineTest(name, method, test.skip); +} defineTest('stats() return sandbox statistics', (sandbox) => { const stats = sandbox.stats({ }); @@ -307,7 +299,7 @@ defineTest('verify object literals are converted to real classes', (sandbox) => expect(obj2[api.TOKEN_REF]).toBeTruthy(); // verify that we received a ref as a result; const objid: string = obj2[api.TOKEN_REF]; - expect(objid.startsWith('jsii-calc.JSObjectLiteralToNativeClass')).toBeTruthy(); // verify the type of the returned object' + expect(objid.startsWith('jsii-calc.JSObjectLiteralToNativeClass@'), `${objid} does not have the intended prefix`).toBeTruthy(); // verify the type of the returned object' }); defineTest('get a property from an type that only has base class properties', (sandbox) => { diff --git a/packages/jsii-python-runtime/src/jsii/_reference_map.py b/packages/jsii-python-runtime/src/jsii/_reference_map.py index bf04bff5b5..f4de964a6f 100644 --- a/packages/jsii-python-runtime/src/jsii/_reference_map.py +++ b/packages/jsii-python-runtime/src/jsii/_reference_map.py @@ -87,23 +87,25 @@ def resolve(self, kernel, ref): return data_type(**python_props) elif class_fqn in _enums: inst = _enums[class_fqn] - elif class_fqn in _interfaces: - # Get our proxy class by finding our interface, then asking it to give us - # the proxy class. - iface = _interfaces[class_fqn] - klass = iface.__jsii_proxy_class__() - - # Create our instance, bypassing __init__ by directly calling __new__, and - # then assign our reference to __jsii_ref__ - inst = klass.__new__(klass) - inst.__jsii_ref__ = ref elif class_fqn == "Object" and ref.interfaces is not None: - ifaces = [_interfaces[fqn] for fqn in ref.interfaces] - classes = [iface.__jsii_proxy_class__() for iface in ifaces] - insts = [klass.__new__(klass) for klass in classes] - for inst in insts: - inst.__jsii_ref__ = ref - return InterfaceDynamicProxy(insts) + if any(fqn in _data_types for fqn in ref.interfaces): + # Ugly delayed import here because I can't solve the cyclic + # package dependency right now :(. + from ._runtime import python_jsii_mapping + + structs = [_data_types[fqn] for fqn in ref.interfaces] + remote_struct = _FakeReference(ref) + insts = [struct(**{ + python_name: kernel.get(remote_struct, jsii_name) for python_name, jsii_name in python_jsii_mapping(struct).items() + }) for struct in structs] + return StructDynamicProxy(insts) + else: + ifaces = [_interfaces[fqn] for fqn in ref.interfaces] + classes = [iface.__jsii_proxy_class__() for iface in ifaces] + insts = [klass.__new__(klass) for klass in classes] + for inst in insts: + inst.__jsii_ref__ = ref + return InterfaceDynamicProxy(insts) else: raise ValueError(f"Unknown type: {class_fqn}") @@ -125,6 +127,35 @@ def __getattr__(self, name): pass return None + +class StructDynamicProxy(object): + def __init__(self, delegates): + self._delegates = delegates + + def __getattr__(self, name): + for delegate in self._delegates: + try: + return getattr(delegate, name) + except NameError: + pass + return None + + def __eq__(self, rhs) -> bool: + if len(self._delegates) == 1: + return rhs == self._delegates[0] + return isinstance(rhs, self.__class__) and rhs._values == self._values + + def __ne__(self, rhs) -> bool: + return not (rhs == self) + + def __repr__(self) -> str: + if len(self._delegates) == 1: + return self._delegates[0].__repr__() + return '%s(%s)' % ( + ' & '.join([delegate.__class__.__jsii_type__ for delegate in self._delegates]), + ', '.join(k + '=' + repr(v) for k, v in self._values.items()) + ) + _refs = _ReferenceMap(_types)