Skip to content

Commit

Permalink
fix(kernel): revert behavior change around any serialization
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@RomainMuller
RomainMuller committed Nov 5, 2019
1 parent 9952f86 commit 4428221
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 116 deletions.
17 changes: 0 additions & 17 deletions packages/jsii-kernel/lib/objects.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>(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
*/
Expand Down
101 changes: 33 additions & 68 deletions packages/jsii-kernel/lib/serialization.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 '.';

/**
Expand Down Expand Up @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -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; }
Expand Down Expand Up @@ -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); }
Expand All @@ -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`);
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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 }];
Expand Down Expand Up @@ -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<string>(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.
*
Expand Down
22 changes: 7 additions & 15 deletions packages/jsii-kernel/test/kernel.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,27 +32,19 @@ if (recordingOutput) {
console.error(`JSII_RECORD=${recordingOutput}`);
}

function defineTest(name: string, method: (sandbox: Kernel) => Promise<any> | any) {
function defineTest(name: string, method: (sandbox: Kernel) => Promise<any> | 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> | any) {
return defineTest(name, method, test.skip);
}

defineTest('stats() return sandbox statistics', (sandbox) => {
const stats = sandbox.stats({ });
Expand Down Expand Up @@ -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) => {
Expand Down
63 changes: 47 additions & 16 deletions packages/jsii-python-runtime/src/jsii/_reference_map.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}")

Expand All @@ -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)


Expand Down

0 comments on commit 4428221

Please sign in to comment.