From 5bf3b3ae00d1f8d18a55e37fc7b137ce3f4535a0 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Wed, 20 Jan 2021 11:32:15 +0100 Subject: [PATCH 01/21] refactor(node-runtime-worker-thread): Simplify promisified type --- packages/node-runtime-worker-thread/src/index.d.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/index.d.ts b/packages/node-runtime-worker-thread/src/index.d.ts index 712712a8e4..2e0a92deb2 100644 --- a/packages/node-runtime-worker-thread/src/index.d.ts +++ b/packages/node-runtime-worker-thread/src/index.d.ts @@ -46,11 +46,9 @@ declare type Params any> = T extends ( : never; declare type Promisified = { - [k in keyof T]: T[k] extends (...args: any) => any + [k in keyof T]: T[k] extends (...args: infer Args) => infer ReturnVal ? ( - ...args: Params - ) => ReturnType extends Promise - ? ReturnType - : Promise> + ...args: Args + ) => ReturnVal extends Promise ? ReturnVal : Promise : T[k]; }; From 1e321065ffbd222e3677ff16d005a3272f9e8bc8 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Wed, 20 Jan 2021 11:35:25 +0100 Subject: [PATCH 02/21] feat(node-runtime-worker-thread): Better serialization for errors thrown inside an rpc helper --- .../src/rpc.spec.ts | 106 +++++++++++------- .../node-runtime-worker-thread/src/rpc.ts | 24 +++- .../src/serializer.ts | 32 ++++++ 3 files changed, 121 insertions(+), 41 deletions(-) create mode 100644 packages/node-runtime-worker-thread/src/serializer.ts diff --git a/packages/node-runtime-worker-thread/src/rpc.spec.ts b/packages/node-runtime-worker-thread/src/rpc.spec.ts index 885736db38..757e208db2 100644 --- a/packages/node-runtime-worker-thread/src/rpc.spec.ts +++ b/packages/node-runtime-worker-thread/src/rpc.spec.ts @@ -28,58 +28,86 @@ describe('rpc', () => { expect(await caller.meow()).to.equal('Meow meow meow!'); }); -}); - -describe('createCaller', () => { - it('creates a caller with provided method names', () => { - const rpcProcess = createMockRpcMesageBus(); - const caller = createCaller(['meow', 'woof'], rpcProcess); - expect(caller).to.have.property('meow'); - expect(caller).to.have.property('woof'); - rpcProcess.removeAllListeners(); - }); - - it('attaches caller listener to provided process', (done) => { - const rpcProcess = createMockRpcMesageBus(); - const caller = createCaller(['meow'], rpcProcess); - - rpcProcess.on('message', (data) => { - expect(data).to.have.property('func'); - expect((data as any).func).to.equal('meow'); - done(); - }); - - caller.meow(); - }); -}); -describe('exposeAll', () => { - it('exposes passed methods on provided process', (done) => { + it('serializes and de-serializes errors when thrown', async() => { const rpcProcess = createMockRpcMesageBus(); + const caller = createCaller(['throws'], rpcProcess); exposeAll( { - meow() { - return 'Meow meow meow meow!'; + throws() { + throw new TypeError('Uh-oh, error!'); } }, rpcProcess ); - rpcProcess.on('message', (data: any) => { - // Due to how our mocks implemented we have to introduce an if here to - // skip our own message being received by the message bus - if (data.sender === 'postmsg-rpc/server') { - expect(data.id).to.be.equal('123abc'); - expect(data.res).to.be.equal('Meow meow meow meow!'); + let err: Error; + + try { + await caller.throws(); + } catch (e) { + err = e; + } + + expect(err).to.be.instanceof(Error); + expect(err).to.have.property('name', 'TypeError'); + expect(err).to.have.property('message', 'Uh-oh, error!'); + expect(err).to.have.property('stack').match(/TypeError: Uh-oh, error!\r?\n\s+at throws/); + }); + + describe('createCaller', () => { + it('creates a caller with provided method names', () => { + const rpcProcess = createMockRpcMesageBus(); + const caller = createCaller(['meow', 'woof'], rpcProcess); + expect(caller).to.have.property('meow'); + expect(caller).to.have.property('woof'); + rpcProcess.removeAllListeners(); + }); + + it('attaches caller listener to provided process', (done) => { + const rpcProcess = createMockRpcMesageBus(); + const caller = createCaller(['meow'], rpcProcess); + + rpcProcess.on('message', (data) => { + expect(data).to.have.property('func'); + expect((data as any).func).to.equal('meow'); done(); - } + }); + + caller.meow(); }); + }); + + describe('exposeAll', () => { + it('exposes passed methods on provided process', (done) => { + const rpcProcess = createMockRpcMesageBus(); + + exposeAll( + { + meow() { + return 'Meow meow meow meow!'; + } + }, + rpcProcess + ); + + rpcProcess.on('message', (data: any) => { + // Due to how our mocks implemented we have to introduce an if here to + // skip our own message being received by the message bus + if (data.sender === 'postmsg-rpc/server') { + expect(data.id).to.be.equal('123abc'); + expect(data.res).to.be.equal('Meow meow meow meow!'); + done(); + } + }); - rpcProcess.send({ - sender: 'postmsg-rpc/client', - func: 'meow', - id: '123abc' + rpcProcess.send({ + sender: 'postmsg-rpc/client', + func: 'meow', + id: '123abc' + }); }); }); }); + diff --git a/packages/node-runtime-worker-thread/src/rpc.ts b/packages/node-runtime-worker-thread/src/rpc.ts index c0aef96acd..0d2dae50e1 100644 --- a/packages/node-runtime-worker-thread/src/rpc.ts +++ b/packages/node-runtime-worker-thread/src/rpc.ts @@ -7,6 +7,7 @@ import { ServerMessageData, ClientMessageData } from 'postmsg-rpc'; +import { deserializeError, serializeError } from './serializer'; type RPCMessageBus = { on: Function; off: Function } & ( | { postMessage: Function; send?: never } @@ -96,9 +97,21 @@ function getRPCOptions(messageBus: RPCMessageBus): PostmsgRpcOptions { export type WithClose = { [k in keyof T]: T[k] & { close(): void } }; +const ERROR = '$$ERROR'; + export function exposeAll(obj: O, messageBus: RPCMessageBus): WithClose { Object.entries(obj).forEach(([key, val]) => { - const { close } = expose(key, val, getRPCOptions(messageBus)); + const { close } = expose( + key, + async(...args: unknown[]) => { + try { + return await val(...args); + } catch (e) { + return { [ERROR]: serializeError(e) }; + } + }, + getRPCOptions(messageBus) + ); (val as any).close = close; }); return obj as WithClose; @@ -114,7 +127,14 @@ export function createCaller( ): Caller { const obj = {}; methodNames.forEach((name) => { - (obj as any)[name] = caller(name as string, getRPCOptions(messageBus)); + const c = caller(name as string, getRPCOptions(messageBus)); + (obj as any)[name] = async(...args: unknown[]) => { + const result = await c(...args); + if (typeof result === 'object' && result !== null && ERROR in result) { + throw deserializeError((result as any)[ERROR]); + } + return result; + }; }); return obj as Caller; } diff --git a/packages/node-runtime-worker-thread/src/serializer.ts b/packages/node-runtime-worker-thread/src/serializer.ts new file mode 100644 index 0000000000..0f46b49140 --- /dev/null +++ b/packages/node-runtime-worker-thread/src/serializer.ts @@ -0,0 +1,32 @@ +import { ShellResult } from '@mongosh/shell-evaluator'; + +function isError(e: any): e is Error { + return e && e.name && e.message && e.stack; +} + +/** + * Extracts non-enumerable params from errors so they can be serialized and + * de-serialized when passed between threads. + * + * Even though v8 {de}serialize supports Error serialization, some data (e.g., + * error `name` if you have a custom error) still can be lost during this + * process, so serializing it produces a better output. + */ +export function serializeError({ + message, + name, + stack, + code, + errno, + path, + syscall +}: NodeJS.ErrnoException) { + return { message, name, stack, code, errno, path, syscall }; +} + +/** + * Creates an instance of Error from error params (Error-like objects) + */ +export function deserializeError(e: NodeJS.ErrnoException) { + return Object.assign(new Error(), e); +} From 262e8d8a636b411e659c88b27115445167bc5ea1 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Wed, 20 Jan 2021 13:13:42 +0100 Subject: [PATCH 03/21] feat(node-runtime-worker-thread): Serialize errors thrown or returned by worker runtime --- .../src/index.spec.ts | 44 +++++++++++- .../node-runtime-worker-thread/src/index.ts | 5 +- .../src/serializer.ts | 39 +++++++++++ .../src/worker-runtime.spec.ts | 67 +++++++++++++++++-- .../src/worker-runtime.ts | 5 +- 5 files changed, 151 insertions(+), 9 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/index.spec.ts b/packages/node-runtime-worker-thread/src/index.spec.ts index edae130024..bc9a17ef2d 100644 --- a/packages/node-runtime-worker-thread/src/index.spec.ts +++ b/packages/node-runtime-worker-thread/src/index.spec.ts @@ -12,10 +12,50 @@ describe('WorkerRuntime', () => { const runtime = new WorkerRuntime('mongodb://nodb/', {}, { nodb: true }); const result = await runtime.evaluate('1+1'); - expect(result.rawValue).to.equal(2); + expect(result.printable).to.equal(2); await runtime.terminate(); }); + + describe('errors', () => { + let runtime: WorkerRuntime; + + beforeEach(() => { + runtime = new WorkerRuntime('mongodb://nodb/', {}, { nodb: true }); + }); + + afterEach(async() => { + await runtime.terminate(); + }); + + it("should throw an error if it's thrown during evaluation", async() => { + let err: Error; + + try { + await runtime.evaluate('throw new TypeError("Oh no, types!")'); + } catch (e) { + err = e; + } + + expect(err).to.be.instanceof(Error); + expect(err).to.have.property('name', 'TypeError'); + expect(err).to.have.property('message', 'Oh no, types!'); + expect(err) + .to.have.property('stack') + .matches(/TypeError: Oh no, types!/); + }); + + it("should return an error if it's returned from evaluation", async() => { + const { printable } = await runtime.evaluate('new SyntaxError("Syntax!")'); + + expect(printable).to.be.instanceof(Error); + expect(printable).to.have.property('name', 'SyntaxError'); + expect(printable).to.have.property('message', 'Syntax!'); + expect(printable) + .to.have.property('stack') + .matches(/SyntaxError: Syntax!/); + }); + }); }); describe('getCompletions', () => { @@ -44,7 +84,7 @@ describe('WorkerRuntime', () => { const password = await runtime.evaluate('passwordPrompt()'); expect(evalListener.onPrompt).to.have.been.called; - expect(password.rawValue).to.equal('password123'); + expect(password.printable).to.equal('password123'); await runtime.terminate(); }); diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index a8d0a46f6a..115e958895 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -10,6 +10,7 @@ import { Caller, createCaller } from './rpc'; import { ChildProcessEvaluationListener } from './child-process-evaluation-listener'; import type { WorkerRuntime as WorkerThreadWorkerRuntime } from './worker-runtime'; import childProcessProxySrc from 'inline-entry-loader!./child-process-proxy'; +import { deserializeEvaluationResult } from './serializer'; type ChildProcessRuntime = Caller; class WorkerRuntime implements Runtime { @@ -63,7 +64,9 @@ class WorkerRuntime implements Runtime { async evaluate(code: string) { await this.initWorkerPromise; - return this.childProcessRuntime.evaluate(code); + return deserializeEvaluationResult( + await this.childProcessRuntime.evaluate(code) + ); } async getCompletions(code: string) { diff --git a/packages/node-runtime-worker-thread/src/serializer.ts b/packages/node-runtime-worker-thread/src/serializer.ts index 0f46b49140..1c6045fc86 100644 --- a/packages/node-runtime-worker-thread/src/serializer.ts +++ b/packages/node-runtime-worker-thread/src/serializer.ts @@ -30,3 +30,42 @@ export function serializeError({ export function deserializeError(e: NodeJS.ErrnoException) { return Object.assign(new Error(), e); } + +enum SerializedResultTypes { + SerializedError = 'SerializedError' +} + +export function serializeEvaluationResult(result: ShellResult): ShellResult { + result = { ...result }; + + if (result.type === null && isError(result.rawValue)) { + result.type = SerializedResultTypes.SerializedError; + result.printable = serializeError(result.rawValue); + } + + // `rawValue` can't be serialized "by design", we don't really care about it + // for the worker thread runtime so as an easy workaround we will just remove + // it completely from the result + delete result.rawValue; + + return result; +} + +export function deserializeEvaluationResult(result: ShellResult): ShellResult { + result = { ...result }; + + if (result.type === SerializedResultTypes.SerializedError) { + result.type = null; + result.printable = deserializeError(result.printable); + } + + Object.defineProperty(result, 'rawValue', { + get() { + throw new Error( + '`rawValue` is not available for evaluation result produced by WorkerRuntime' + ); + } + }); + + return result; +} diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts index 21ff42caf0..0b2213743b 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts @@ -6,9 +6,10 @@ import chai, { expect } from 'chai'; import chaiAsPromised from 'chai-as-promised'; import sinonChai from 'sinon-chai'; import sinon from 'sinon'; - import { startTestServer } from '../../../testing/integration-testing-hooks'; -import { createCaller, exposeAll } from './rpc'; +import { Caller, createCaller, exposeAll } from './rpc'; +import { deserializeEvaluationResult } from './serializer'; +import type { WorkerRuntime } from './worker-runtime'; chai.use(sinonChai); chai.use(chaiAsPromised); @@ -47,16 +48,72 @@ describe('worker', () => { const result = await evaluate('1 + 1'); expect(result).to.have.property('type', null); - expect(result).to.have.property('rawValue', 2); expect(result).to.have.property('printable', 2); }); + + describe('errors', () => { + let caller: Caller; + + beforeEach(() => { + const c = createCaller(['init', 'evaluate'], worker); + caller = { + ...c, + async evaluate(code: string) { + return deserializeEvaluationResult(await c.evaluate(code)); + } + }; + (caller.evaluate as any).close = c.evaluate.close; + }); + + afterEach(() => { + caller = null; + }); + + it("should throw an error if it's thrown during evaluation", async() => { + const { init, evaluate } = caller; + + await init('mongodb://nodb/', {}, { nodb: true }); + + let err: Error; + try { + await evaluate('throw new TypeError("Oh no, types!")'); + } catch (e) { + err = e; + } + + expect(err).to.be.instanceof(Error); + expect(err).to.have.property('name', 'TypeError'); + expect(err).to.have.property('message', 'Oh no, types!'); + expect(err) + .to.have.property('stack') + .matches(/TypeError: Oh no, types!/); + }); + + it("should return an error if it's returned from evaluation", async() => { + const { init, evaluate } = caller; + + await init('mongodb://nodb/', {}, { nodb: true }); + + const { printable } = await evaluate('new SyntaxError("Syntax!")'); + + expect(printable).to.be.instanceof(Error); + expect(printable).to.have.property('name', 'SyntaxError'); + expect(printable).to.have.property('message', 'Syntax!'); + expect(printable) + .to.have.property('stack') + .matches(/SyntaxError: Syntax!/); + }); + }); }); describe('getShellPrompt', () => { const testServer = startTestServer('shared'); it('should return prompt when connected to the server', async() => { - const { init, getShellPrompt } = createCaller(['init', 'getShellPrompt'], worker); + const { init, getShellPrompt } = createCaller( + ['init', 'getShellPrompt'], + worker + ); await init(await testServer.connectionString()); @@ -121,7 +178,7 @@ describe('worker', () => { const password = await evaluate('passwordPrompt()'); expect(evalListener.onPrompt).to.have.been.called; - expect(password.rawValue).to.equal('123'); + expect(password.printable).to.equal('123'); }); }); diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.ts b/packages/node-runtime-worker-thread/src/worker-runtime.ts index 696d4ca8ad..635c15b837 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.ts @@ -11,6 +11,7 @@ import { import { CliServiceProvider } from '@mongosh/service-provider-server'; import { EvaluationListener } from '@mongosh/shell-evaluator'; import { exposeAll, createCaller } from './rpc'; +import { serializeEvaluationResult } from './serializer'; if (!parentPort || isMainThread) { throw new Error('Worker runtime can be used only in a worker thread'); @@ -53,7 +54,9 @@ const workerRuntime: WorkerRuntime = { }, async evaluate(code) { - return ensureRuntime('evaluate').evaluate(code); + return serializeEvaluationResult( + await ensureRuntime('evaluate').evaluate(code) + ); }, async getCompletions(code) { From a6ae152d1a576d31b7b32e5f4af710d8c3caf377 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Thu, 21 Jan 2021 15:34:31 +0100 Subject: [PATCH 04/21] refactor(node-runtime-worker-thread): Propagate serialization errors back to the client --- .../src/rpc.spec.ts | 63 +++++++++++++++++-- .../node-runtime-worker-thread/src/rpc.ts | 37 +++++------ .../src/serializer.ts | 20 +++++- 3 files changed, 95 insertions(+), 25 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/rpc.spec.ts b/packages/node-runtime-worker-thread/src/rpc.spec.ts index 757e208db2..f7786ad98b 100644 --- a/packages/node-runtime-worker-thread/src/rpc.spec.ts +++ b/packages/node-runtime-worker-thread/src/rpc.spec.ts @@ -53,7 +53,63 @@ describe('rpc', () => { expect(err).to.be.instanceof(Error); expect(err).to.have.property('name', 'TypeError'); expect(err).to.have.property('message', 'Uh-oh, error!'); - expect(err).to.have.property('stack').match(/TypeError: Uh-oh, error!\r?\n\s+at throws/); + expect(err) + .to.have.property('stack') + .match(/TypeError: Uh-oh, error!\r?\n\s+at throws/); + }); + + it('throws on client if arguments are not serializable', async() => { + const rpcProcess = createMockRpcMesageBus(); + const caller = createCaller(['callMe'], rpcProcess); + + exposeAll( + { + callMe(fn: any) { + fn(1, 2); + } + }, + rpcProcess + ); + + let err: Error; + + try { + await caller.callMe((a: number, b: number) => a + b); + } catch (e) { + err = e; + } + + expect(err).to.be.instanceof(Error); + expect(err) + .to.have.property('message') + .match(/could not be cloned/); + }); + + it('throws on client if retured value from the server is not serializable', async() => { + const rpcProcess = createMockRpcMesageBus(); + const caller = createCaller(['returnsFunction'], rpcProcess); + + exposeAll( + { + returnsFunction() { + return () => {}; + } + }, + rpcProcess + ); + + let err: Error; + + try { + await caller.returnsFunction(); + } catch (e) { + err = e; + } + + expect(err).to.be.instanceof(Error); + expect(err) + .to.have.property('message') + .match(/could not be cloned/); }); describe('createCaller', () => { @@ -93,8 +149,8 @@ describe('rpc', () => { ); rpcProcess.on('message', (data: any) => { - // Due to how our mocks implemented we have to introduce an if here to - // skip our own message being received by the message bus + // Due to how our mocks implemented we have to introduce an if here to + // skip our own message being received by the message bus if (data.sender === 'postmsg-rpc/server') { expect(data.id).to.be.equal('123abc'); expect(data.res).to.be.equal('Meow meow meow meow!'); @@ -110,4 +166,3 @@ describe('rpc', () => { }); }); }); - diff --git a/packages/node-runtime-worker-thread/src/rpc.ts b/packages/node-runtime-worker-thread/src/rpc.ts index 0d2dae50e1..53e197a118 100644 --- a/packages/node-runtime-worker-thread/src/rpc.ts +++ b/packages/node-runtime-worker-thread/src/rpc.ts @@ -1,4 +1,3 @@ -import v8 from 'v8'; import { expose, caller, @@ -7,13 +6,20 @@ import { ServerMessageData, ClientMessageData } from 'postmsg-rpc'; -import { deserializeError, serializeError } from './serializer'; +import { + serialize, + deserialize, + deserializeError, + serializeError +} from './serializer'; type RPCMessageBus = { on: Function; off: Function } & ( | { postMessage: Function; send?: never } | { postMessage?: never; send?: Function } ); +const ERROR = '$$ERROR'; + function isMessageData(data: any): data is MessageData { return data && typeof data === 'object' && 'id' in data && 'sender' in data; } @@ -49,30 +55,27 @@ function send(messageBus: RPCMessageBus, data: any): void { } } -function serialize(data: unknown): string { - return `data:;base64,${v8.serialize(data).toString('base64')}`; -} - -function deserialize(str: string): T | string { - if (/^data:;base64,.+/.test(str)) { - return v8.deserialize( - Buffer.from(str.replace('data:;base64,', ''), 'base64') - ); - } - return str; -} - function getRPCOptions(messageBus: RPCMessageBus): PostmsgRpcOptions { return { addListener: messageBus.on.bind(messageBus), removeListener: messageBus.off.bind(messageBus), postMessage(data) { if (isClientMessageData(data) && Array.isArray(data.args)) { + // We don't guard against serialization errors on the client, if they + // happen, client is responsible for handling them data.args = serialize(removeTrailingUndefined(data.args)); } if (isServerMessageData(data)) { - data.res = serialize(data.res); + // If serialization error happened on the server, we use our special + // error return value to propagate the error back to the client, + // otherwise error can be lost on the server and client call will never + // resolve + try { + data.res = serialize(data.res); + } catch (err) { + data.res = { [ERROR]: serializeError(err) }; + } } return send(messageBus, data); @@ -97,8 +100,6 @@ function getRPCOptions(messageBus: RPCMessageBus): PostmsgRpcOptions { export type WithClose = { [k in keyof T]: T[k] & { close(): void } }; -const ERROR = '$$ERROR'; - export function exposeAll(obj: O, messageBus: RPCMessageBus): WithClose { Object.entries(obj).forEach(([key, val]) => { const { close } = expose( diff --git a/packages/node-runtime-worker-thread/src/serializer.ts b/packages/node-runtime-worker-thread/src/serializer.ts index 1c6045fc86..580525acbf 100644 --- a/packages/node-runtime-worker-thread/src/serializer.ts +++ b/packages/node-runtime-worker-thread/src/serializer.ts @@ -1,5 +1,19 @@ +import v8 from 'v8'; import { ShellResult } from '@mongosh/shell-evaluator'; +export function serialize(data: unknown): string { + return `data:;base64,${v8.serialize(data).toString('base64')}`; +} + +export function deserialize(str: string): T | string { + if (/^data:;base64,.+/.test(str)) { + return v8.deserialize( + Buffer.from(str.replace('data:;base64,', ''), 'base64') + ); + } + return str; +} + function isError(e: any): e is Error { return e && e.name && e.message && e.stack; } @@ -32,14 +46,14 @@ export function deserializeError(e: NodeJS.ErrnoException) { } enum SerializedResultTypes { - SerializedError = 'SerializedError' + SerializedErrorResult = 'SerializedErrorResult' } export function serializeEvaluationResult(result: ShellResult): ShellResult { result = { ...result }; if (result.type === null && isError(result.rawValue)) { - result.type = SerializedResultTypes.SerializedError; + result.type = SerializedResultTypes.SerializedErrorResult; result.printable = serializeError(result.rawValue); } @@ -54,7 +68,7 @@ export function serializeEvaluationResult(result: ShellResult): ShellResult { export function deserializeEvaluationResult(result: ShellResult): ShellResult { result = { ...result }; - if (result.type === SerializedResultTypes.SerializedError) { + if (result.type === SerializedResultTypes.SerializedErrorResult) { result.type = null; result.printable = deserializeError(result.printable); } From 355cd29304048a20f30d377d7f14c45637f832b8 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Fri, 22 Jan 2021 11:54:06 +0100 Subject: [PATCH 05/21] feat(node-runtime-worker-thread): Inspect complex, non-distinguishable shell result values before returning them --- .../src/serializer.ts | 59 ++++++---- .../src/worker-runtime.spec.ts | 105 ++++++++++++++---- 2 files changed, 122 insertions(+), 42 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/serializer.ts b/packages/node-runtime-worker-thread/src/serializer.ts index 580525acbf..d5aefc68c0 100644 --- a/packages/node-runtime-worker-thread/src/serializer.ts +++ b/packages/node-runtime-worker-thread/src/serializer.ts @@ -1,5 +1,6 @@ import v8 from 'v8'; import { ShellResult } from '@mongosh/shell-evaluator'; +import { inspect } from 'util'; export function serialize(data: unknown): string { return `data:;base64,${v8.serialize(data).toString('base64')}`; @@ -14,8 +15,18 @@ export function deserialize(str: string): T | string { return str; } -function isError(e: any): e is Error { - return e && e.name && e.message && e.stack; +function isPrimitive( + val: any +): val is boolean | string | number | undefined | null { + return (typeof val !== 'object' && typeof val !== 'function') || val === null; +} + +function isError(val: any): val is Error { + return val && val.name && val.message && val.stack; +} + +function getNames(obj: T): (keyof T)[] { + return Object.getOwnPropertyNames(obj) as (keyof T)[]; } /** @@ -26,35 +37,45 @@ function isError(e: any): e is Error { * error `name` if you have a custom error) still can be lost during this * process, so serializing it produces a better output. */ -export function serializeError({ - message, - name, - stack, - code, - errno, - path, - syscall -}: NodeJS.ErrnoException) { - return { message, name, stack, code, errno, path, syscall }; +export function serializeError(err: Error) { + // Name is the only constructor property we care about + const keys = getNames(err).concat('name'); + return keys.reduce((acc, key) => { + (acc as any)[key] = err[key]; + return acc; + }, {} as Error); } /** * Creates an instance of Error from error params (Error-like objects) */ -export function deserializeError(e: NodeJS.ErrnoException) { - return Object.assign(new Error(), e); +export function deserializeError(err: any): Error { + return Object.assign(new Error(), err); } -enum SerializedResultTypes { - SerializedErrorResult = 'SerializedErrorResult' +export enum SerializedResultTypes { + SerializedErrorResult = 'SerializedErrorResult', + InspectResult = 'InspectResult' } export function serializeEvaluationResult(result: ShellResult): ShellResult { result = { ...result }; - if (result.type === null && isError(result.rawValue)) { - result.type = SerializedResultTypes.SerializedErrorResult; - result.printable = serializeError(result.rawValue); + // Type `null` indicates anything that is not returned by shell-api, any + // usual javascript value returned from evaluation + if (result.type === null) { + // Errors get special treatment to achieve better serialization + if (isError(result.rawValue)) { + result.type = SerializedResultTypes.SerializedErrorResult; + result.printable = serializeError(result.rawValue); + } else if (!isPrimitive(result.rawValue)) { + // For everything else that is not a primitive value there is just too + // many possible combinations of data types to distinguish between them, + // if we don't know the type and it's not an error, let's inspect and + // return an inspection result + result.type = SerializedResultTypes.InspectResult; + result.printable = inspect(result.rawValue); + } } // `rawValue` can't be serialized "by design", we don't really care about it diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts index 0b2213743b..c9592252b1 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts @@ -41,34 +41,93 @@ describe('worker', () => { }); describe('evaluate', () => { - it('should evaluate simple code and return simple, serializable values', async() => { - const { init, evaluate } = createCaller(['init', 'evaluate'], worker); - await init('mongodb://nodb/', {}, { nodb: true }); - - const result = await evaluate('1 + 1'); - - expect(result).to.have.property('type', null); - expect(result).to.have.property('printable', 2); + let caller: Caller; + + beforeEach(() => { + const c = createCaller(['init', 'evaluate'], worker); + caller = { + ...c, + async evaluate(code: string) { + return deserializeEvaluationResult(await c.evaluate(code)); + } + }; + (caller.evaluate as any).close = c.evaluate.close; }); - describe('errors', () => { - let caller: Caller; - - beforeEach(() => { - const c = createCaller(['init', 'evaluate'], worker); - caller = { - ...c, - async evaluate(code: string) { - return deserializeEvaluationResult(await c.evaluate(code)); - } - }; - (caller.evaluate as any).close = c.evaluate.close; - }); + afterEach(() => { + caller = null; + }); - afterEach(() => { - caller = null; + describe('basic shell result values', () => { + const primitiveValues: [string, string, unknown][] = [ + ['null', 'null', null], + ['undefined', 'undefined', undefined], + ['boolean', '!false', true], + ['number', '1+1', 2], + ['string', '"hello"', 'hello'] + ]; + + const everythingElse: [string, string, string][] = [ + ['function', 'function abc() {}; abc', '[Function: abc]'], + [ + 'function with properties', + 'function def() {}; def.def = 1; def', + '[Function: def] { def: 1 }' + ], + ['anonymous function', '(() => {})', '[Function (anonymous)]'], + ['class constructor', 'class BCD {}; BCD', '[class BCD]'], + [ + 'class instalce', + 'class ABC { constructor() { this.abc = 1; } }; var abc = new ABC(); abc', + 'ABC { abc: 1 }' + ], + ['simple array', '[1, 2, 3]', '[ 1, 2, 3 ]'], + [ + 'simple array with empty items', + '[1, 2,, 4]', + '[ 1, 2, <1 empty item>, 4 ]' + ], + [ + 'non-serializable array', + '[1, 2, 3, () => {}]', + '[ 1, 2, 3, [Function (anonymous)] ]' + ], + [ + 'simple object', + '({str: "foo", num: 123})', + "{ str: 'foo', num: 123 }" + ], + [ + 'non-serializable object', + '({str: "foo", num: 123, bool: false, fn() {}})', + "{ str: 'foo', num: 123, bool: false, fn: [Function: fn] }" + ], + [ + 'object with bson', + '({min: MinKey(), max: MaxKey(), int: NumberInt("1")})', + '{ min: MinKey(), max: MaxKey(), int: Int32(1) }' + ], + [ + 'object with everything', + '({ cls: class A{}, fn() {}, bsonType: NumberInt("1"), str: "123"})', + "{ cls: [class A], fn: [Function: fn], bsonType: Int32(1), str: '123' }" + ] + ]; + + primitiveValues.concat(everythingElse).forEach((testCase) => { + const [testName, evalValue, printable] = testCase; + + it(testName, async() => { + const { init, evaluate } = createCaller(['init', 'evaluate'], worker); + await init('mongodb://nodb/', {}, { nodb: true }); + const result = await evaluate(evalValue); + expect(result).to.have.property('printable'); + expect(result.printable).to.deep.equal(printable); + }); }); + }); + describe('errors', () => { it("should throw an error if it's thrown during evaluation", async() => { const { init, evaluate } = caller; From b917c989c62cd29b970e862dc91aa1094123081b Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Fri, 22 Jan 2021 16:29:50 +0100 Subject: [PATCH 06/21] fix(node-runtime-worker-thread): Use caller defined in beforeEach --- packages/node-runtime-worker-thread/src/worker-runtime.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts index c9592252b1..71533792c4 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts @@ -118,7 +118,7 @@ describe('worker', () => { const [testName, evalValue, printable] = testCase; it(testName, async() => { - const { init, evaluate } = createCaller(['init', 'evaluate'], worker); + const { init, evaluate } = caller; await init('mongodb://nodb/', {}, { nodb: true }); const result = await evaluate(evalValue); expect(result).to.have.property('printable'); From d9b38f78650fb5e692258247a6cd944ec711495d Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Fri, 22 Jan 2021 19:27:28 +0100 Subject: [PATCH 07/21] feat(node-runtime-worker-thread): Serialize shell-api return types for better output --- .../node-runtime-worker-thread/package.json | 1 + .../src/serializer.ts | 41 ++++++- .../src/worker-runtime.spec.ts | 103 ++++++++++++++++++ 3 files changed, 144 insertions(+), 1 deletion(-) diff --git a/packages/node-runtime-worker-thread/package.json b/packages/node-runtime-worker-thread/package.json index b07bd881d7..4acde56eae 100644 --- a/packages/node-runtime-worker-thread/package.json +++ b/packages/node-runtime-worker-thread/package.json @@ -44,6 +44,7 @@ "@mongosh/service-provider-core": "0.0.0-dev.0", "@mongosh/service-provider-server": "0.0.0-dev.0", "@mongosh/shell-evaluator": "0.0.0-dev.0", + "bson": "^4.2.2", "postmsg-rpc": "^2.4.0" } } diff --git a/packages/node-runtime-worker-thread/src/serializer.ts b/packages/node-runtime-worker-thread/src/serializer.ts index d5aefc68c0..a41fc1bac9 100644 --- a/packages/node-runtime-worker-thread/src/serializer.ts +++ b/packages/node-runtime-worker-thread/src/serializer.ts @@ -1,6 +1,7 @@ import v8 from 'v8'; import { ShellResult } from '@mongosh/shell-evaluator'; import { inspect } from 'util'; +import { EJSON, Document } from 'bson'; export function serialize(data: unknown): string { return `data:;base64,${v8.serialize(data).toString('base64')}`; @@ -55,9 +56,16 @@ export function deserializeError(err: any): Error { export enum SerializedResultTypes { SerializedErrorResult = 'SerializedErrorResult', - InspectResult = 'InspectResult' + InspectResult = 'InspectResult', + SerializedShellApiResult = 'SerializedShellApiResult' } +type SerializedShellApiResult = { + origType: string; + serializedValue: Document; + nonEnumPropertyDescriptors: Record; +}; + export function serializeEvaluationResult(result: ShellResult): ShellResult { result = { ...result }; @@ -76,6 +84,23 @@ export function serializeEvaluationResult(result: ShellResult): ShellResult { result.type = SerializedResultTypes.InspectResult; result.printable = inspect(result.rawValue); } + } else if (!isPrimitive(result.printable)) { + // If type is present we are dealing with shell-api return value. In most + // cases printable values of shell-api are primitive or serializable as + // (E)JSON, but they also might have some non-enumerable stuff (looking at + // you, Cursor!) that we need to serialize additionally, otherwiser those + // params will be lost in the ether causing issues down the way + const origType = result.type; + result.type = SerializedResultTypes.SerializedShellApiResult; + result.printable = { + origType, + serializedValue: EJSON.serialize(result.printable), + nonEnumPropertyDescriptors: Object.fromEntries( + Object.entries( + Object.getOwnPropertyDescriptors(result.printable) + ).filter(([, descriptor]) => !descriptor.enumerable) + ) + }; } // `rawValue` can't be serialized "by design", we don't really care about it @@ -94,6 +119,20 @@ export function deserializeEvaluationResult(result: ShellResult): ShellResult { result.printable = deserializeError(result.printable); } + if (result.type === SerializedResultTypes.SerializedShellApiResult) { + const value: SerializedShellApiResult = result.printable; + const printable = EJSON.deserialize(value.serializedValue); + + // Primitives should not end up here ever, but we need to convince TS that + // its true + if (!isPrimitive(printable)) { + Object.defineProperties(printable, value.nonEnumPropertyDescriptors); + } + + result.type = value.origType; + result.printable = printable; + } + Object.defineProperty(result, 'rawValue', { get() { throw new Error( diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts index 71533792c4..666796ee39 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts @@ -10,10 +10,30 @@ import { startTestServer } from '../../../testing/integration-testing-hooks'; import { Caller, createCaller, exposeAll } from './rpc'; import { deserializeEvaluationResult } from './serializer'; import type { WorkerRuntime } from './worker-runtime'; +import { ObjectId } from 'bson'; +import { inspect } from 'util'; chai.use(sinonChai); chai.use(chaiAsPromised); +const chaiBson: Chai.ChaiPlugin = (chai) => { + chai.Assertion.addMethod('bson', function bson(expected) { + const obj = this._obj; + + new (chai.Assertion)(obj).to.be.instanceof(expected.constructor); + + this.assert( + inspect(obj) === inspect(expected), + 'expected #{this} to match #{exp} but got #{act}', + 'expected #{this} not to match #{exp}', + obj, + expected + ); + }); +}; + +chai.use(chaiBson); + // We need a compiled version so we can import it as a worker const workerThreadModule = fs.readFile( path.resolve(__dirname, '..', 'dist', 'worker-runtime.js'), @@ -127,6 +147,89 @@ describe('worker', () => { }); }); + describe('shell-api results', () => { + const testServer = startTestServer('shared'); + const db = `test-db-${Date.now().toString(16)}`; + + type CommandTestRecord = + | [string | string[], string] + | [string | string[], string, any]; + + const showCommand: CommandTestRecord[] = [ + ['show dbs', 'ShowDatabasesResult'], + ['show collections', 'ShowCollectionsResult'], + ['show profile', 'ShowProfileResult'], + ['show roles', 'ShowResult'] + ]; + + const useCommand: CommandTestRecord[] = [ + [`use ${db}`, null, `switched to db ${db}`] + ]; + + const cursors: CommandTestRecord[] = [ + [ + [ + `use ${db}`, + 'db.coll.insertOne({ _id: ObjectId("000000000000000000000000"), foo: 321 });', + 'db.coll.aggregate({ $match: { foo: 321 } })' + ], + 'AggregationCursor', + (result) => { + expect(result.printable).to.have.property('cursorHasMore', false); + expect(result.printable[0]) + .to.have.property('_id') + // TODO: chai assertion types + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + .bson(new ObjectId('000000000000000000000000')); + expect(result.printable[0]).to.have.property('foo', 321); + } + ] + ]; + + showCommand + .concat(useCommand) + .concat(cursors) + .forEach((testCase) => { + const [commands, resultType, printable] = testCase; + + let command: string; + let prepare: undefined | string[]; + + if (Array.isArray(commands)) { + command = commands.pop(); + prepare = commands; + } else { + command = commands; + } + + it(`"${command}" should return ${resultType}`, async() => { + const { init, evaluate } = caller; + await init(await testServer.connectionString(), {}, {}); + + if (prepare) { + for (const code of prepare) { + await evaluate(code); + } + } + + const result = await evaluate(command); + + expect(result).to.have.property('type', resultType); + + if (typeof printable === 'function') { + printable(result); + } else if (printable instanceof RegExp) { + expect(result).to.have.property('printable').match(printable); + } else if (printable) { + expect(result) + .to.have.property('printable') + .deep.equal(printable); + } + }); + }); + }); + describe('errors', () => { it("should throw an error if it's thrown during evaluation", async() => { const { init, evaluate } = caller; From 0c29a962ee584f3b2af63532d4c7d3f157820fe9 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Mon, 25 Jan 2021 15:11:31 +0100 Subject: [PATCH 08/21] test(node-runtime-worker-thread): Add more tests for different types of return values --- .../src/worker-runtime.spec.ts | 137 ++++++++++++++++-- 1 file changed, 123 insertions(+), 14 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts index 666796ee39..359879d903 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts @@ -6,12 +6,13 @@ import chai, { expect } from 'chai'; import chaiAsPromised from 'chai-as-promised'; import sinonChai from 'sinon-chai'; import sinon from 'sinon'; +import { ObjectId } from 'bson'; +import { inspect } from 'util'; +import { ShellResult } from '@mongosh/shell-evaluator'; import { startTestServer } from '../../../testing/integration-testing-hooks'; import { Caller, createCaller, exposeAll } from './rpc'; import { deserializeEvaluationResult } from './serializer'; import type { WorkerRuntime } from './worker-runtime'; -import { ObjectId } from 'bson'; -import { inspect } from 'util'; chai.use(sinonChai); chai.use(chaiAsPromised); @@ -20,7 +21,7 @@ const chaiBson: Chai.ChaiPlugin = (chai) => { chai.Assertion.addMethod('bson', function bson(expected) { const obj = this._obj; - new (chai.Assertion)(obj).to.be.instanceof(expected.constructor); + new chai.Assertion(obj).to.be.instanceof(expected.constructor); this.assert( inspect(obj) === inspect(expected), @@ -156,16 +157,43 @@ describe('worker', () => { | [string | string[], string, any]; const showCommand: CommandTestRecord[] = [ - ['show dbs', 'ShowDatabasesResult'], - ['show collections', 'ShowCollectionsResult'], - ['show profile', 'ShowProfileResult'], - ['show roles', 'ShowResult'] + [ + 'show dbs', + 'ShowDatabasesResult', + ({ printable }: ShellResult) => { + expect(printable.find(({ name }: any) => name === 'admin')).to.not + .be.undefined; + } + ], + ['show collections', 'ShowCollectionsResult', []], + ['show profile', 'ShowProfileResult', { count: 0 }], + [ + 'show roles', + 'ShowResult', + ({ printable }: ShellResult) => { + expect(printable.find(({ role }: any) => role === 'dbAdmin')).to.not + .be.undefined; + } + ] ]; const useCommand: CommandTestRecord[] = [ [`use ${db}`, null, `switched to db ${db}`] ]; + const helpCommand: CommandTestRecord[] = [ + [ + 'help', + 'Help', + ({ printable }: ShellResult) => { + expect(printable).to.have.property('help', 'Shell Help'); + expect(printable) + .to.have.property('docs') + .match(/https:\/\/docs.mongodb.com/); + } + ] + ]; + const cursors: CommandTestRecord[] = [ [ [ @@ -174,22 +202,103 @@ describe('worker', () => { 'db.coll.aggregate({ $match: { foo: 321 } })' ], 'AggregationCursor', - (result) => { - expect(result.printable).to.have.property('cursorHasMore', false); - expect(result.printable[0]) - .to.have.property('_id') + ({ printable }: ShellResult) => { + expect(printable).to.have.property('cursorHasMore', false); + expect(printable) + .to.have.nested.property('[0]._id') // TODO: chai assertion types // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore .bson(new ObjectId('000000000000000000000000')); - expect(result.printable[0]).to.have.property('foo', 321); + expect(printable).to.have.nested.property('[0].foo', 321); + } + ], + [ + [ + `use ${db}`, + 'db.coll.insertMany([1, 2, 3, 4, 5, 6, 7, 8, 9, 10].map(i => ({ i })))', + 'db.coll.find({ i: { $mod: [2, 0] } }, { _id: 0 })' + ], + 'Cursor', + [{ i: 2 }, { i: 4 }, { i: 6 }, { i: 8 }, { i: 10 }] + ], + [ + [ + `use ${db}`, + "db.coll.insertMany('a'.repeat(100).split('').map(a => ({ a })))", + 'db.coll.find({}, { _id: 0 })', + 'it' + ], + 'CursorIterationResult', + ({ printable }: ShellResult) => { + expect(printable).to.include.deep.members([{ a: 'a' }]); + } + ] + ]; + + const crudCommands: CommandTestRecord[] = [ + [ + [`use ${db}`, 'db.coll.insertOne({ a: "a" })'], + 'InsertOneResult', + ({ printable }: ShellResult) => { + expect(printable).to.have.property('acknowledged', true); + expect(printable) + .to.have.property('insertedId') + .instanceof(ObjectId); + } + ], + [ + [`use ${db}`, 'db.coll.insertMany([{ b: "b" }, { c: "c" }])'], + 'InsertManyResult', + ({ printable }: ShellResult) => { + expect(printable).to.have.property('acknowledged', true); + expect(printable) + .to.have.nested.property('insertedIds[0]') + .instanceof(ObjectId); + } + ], + [ + [ + `use ${db}`, + 'db.coll.insertOne({ a: "a" })', + 'db.coll.updateOne({ a: "a" }, { $set: { a: "b" } })' + ], + 'UpdateResult', + { + acknowledged: true, + insertedId: null, + matchedCount: 1, + modifiedCount: 1, + upsertedCount: 0 + } + ], + [ + [ + `use ${db}`, + 'db.coll.insertOne({ a: "a" })', + 'db.coll.deleteOne({ a: "a" })' + ], + 'DeleteResult', + { acknowledged: true, deletedCount: 1 } + ], + [ + [`use ${db}`, 'db.coll.bulkWrite([{ insertOne: { d: "d" } }])'], + 'BulkWriteResult', + ({ printable }: ShellResult) => { + expect(printable).to.have.property('acknowledged', true); + expect(printable).to.have.property('insertedCount', 1); + expect(printable) + .to.have.nested.property('insertedIds[0]') + .instanceof(ObjectId); } ] ]; showCommand .concat(useCommand) + .concat(helpCommand) .concat(cursors) + .concat(crudCommands) .forEach((testCase) => { const [commands, resultType, printable] = testCase; @@ -203,7 +312,7 @@ describe('worker', () => { command = commands; } - it(`"${command}" should return ${resultType}`, async() => { + it(`"${command}" should return ${resultType} result`, async() => { const { init, evaluate } = caller; await init(await testServer.connectionString(), {}, {}); @@ -221,7 +330,7 @@ describe('worker', () => { printable(result); } else if (printable instanceof RegExp) { expect(result).to.have.property('printable').match(printable); - } else if (printable) { + } else if (typeof printable !== 'undefined') { expect(result) .to.have.property('printable') .deep.equal(printable); From 3963910c4ab9ec0782aad80ddab4b5edc96a2d75 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Mon, 25 Jan 2021 16:32:29 +0100 Subject: [PATCH 09/21] fix(node-runtime-worker-thread): Fix bson missing from package-lock --- .../package-lock.json | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/packages/node-runtime-worker-thread/package-lock.json b/packages/node-runtime-worker-thread/package-lock.json index 60edcb7681..23e26ba1bc 100644 --- a/packages/node-runtime-worker-thread/package-lock.json +++ b/packages/node-runtime-worker-thread/package-lock.json @@ -496,8 +496,7 @@ "base64-js": { "version": "1.5.1", "resolved": "https://registry.npmjs.org/base64-js/-/base64-js-1.5.1.tgz", - "integrity": "sha512-AKpaYlHn8t4SVbOHCy+b5+KKgvR4vrsD8vbvrbiQJps7fKDTkjkDry6ji0rUJjC0kzbNePLwzxq8iypo41qeWA==", - "dev": true + "integrity": "sha512-AKpaYlHn8t4SVbOHCy+b5+KKgvR4vrsD8vbvrbiQJps7fKDTkjkDry6ji0rUJjC0kzbNePLwzxq8iypo41qeWA==" }, "big.js": { "version": "5.2.2", @@ -650,6 +649,25 @@ "pako": "~1.0.5" } }, + "bson": { + "version": "4.2.2", + "resolved": "https://registry.npmjs.org/bson/-/bson-4.2.2.tgz", + "integrity": "sha512-9fX257PVHAUpiRGmY3356RVWKQxLA73BgjA/x5MGuJkTEMeG7yzjuBrsiFB67EXRJnFVKrbJY9t/M+oElKYktQ==", + "requires": { + "buffer": "^5.6.0" + }, + "dependencies": { + "buffer": { + "version": "5.7.1", + "resolved": "https://registry.npmjs.org/buffer/-/buffer-5.7.1.tgz", + "integrity": "sha512-EHcyIPBQ4BSGlvjB16k5KgAJ27CIsHY/2JBmCRReo48y9rQ3MaUzWX3KVlBa4U7MyX02HdVj0K7C3WaB3ju7FQ==", + "requires": { + "base64-js": "^1.3.1", + "ieee754": "^1.1.13" + } + } + } + }, "buffer": { "version": "4.9.2", "resolved": "https://registry.npmjs.org/buffer/-/buffer-4.9.2.tgz", @@ -1932,8 +1950,7 @@ "ieee754": { "version": "1.2.1", "resolved": "https://registry.npmjs.org/ieee754/-/ieee754-1.2.1.tgz", - "integrity": "sha512-dcyqhDvX1C46lXZcVqCpK+FtMRQVdIMN6/Df5js2zouUsqG7I6sFxitIC+7KYK29KdXOLHdu9zL4sFnoVQnqaA==", - "dev": true + "integrity": "sha512-dcyqhDvX1C46lXZcVqCpK+FtMRQVdIMN6/Df5js2zouUsqG7I6sFxitIC+7KYK29KdXOLHdu9zL4sFnoVQnqaA==" }, "iferr": { "version": "0.1.5", From 294483422886631c71d8b996ec0ae56c01e6e50c Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Mon, 25 Jan 2021 16:59:38 +0100 Subject: [PATCH 10/21] test(node-runtime-worker-thread): Fix anonymous fn inspection check for older node versions --- .../src/worker-runtime.spec.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts index 359879d903..fa5001f1ca 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts @@ -88,14 +88,14 @@ describe('worker', () => { ['string', '"hello"', 'hello'] ]; - const everythingElse: [string, string, string][] = [ + const everythingElse: [string, string, string | RegExp][] = [ ['function', 'function abc() {}; abc', '[Function: abc]'], [ 'function with properties', 'function def() {}; def.def = 1; def', '[Function: def] { def: 1 }' ], - ['anonymous function', '(() => {})', '[Function (anonymous)]'], + ['anonymous function', '(() => {})', /\[Function( \(anonymous\))?\]/], ['class constructor', 'class BCD {}; BCD', '[class BCD]'], [ 'class instalce', @@ -111,7 +111,7 @@ describe('worker', () => { [ 'non-serializable array', '[1, 2, 3, () => {}]', - '[ 1, 2, 3, [Function (anonymous)] ]' + /\[ 1, 2, 3, \[Function( \(anonymous\))?\] \]/ ], [ 'simple object', @@ -143,7 +143,11 @@ describe('worker', () => { await init('mongodb://nodb/', {}, { nodb: true }); const result = await evaluate(evalValue); expect(result).to.have.property('printable'); - expect(result.printable).to.deep.equal(printable); + if (printable instanceof RegExp) { + expect(result.printable).to.match(printable); + } else { + expect(result.printable).to.deep.equal(printable); + } }); }); }); From 9acceeee17db2fbf1d3fcfca5589c4a75772056e Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Tue, 26 Jan 2021 17:45:12 +0100 Subject: [PATCH 11/21] refactor(browser-runtime-core, node-runtime-worker-thread): Runtimes should return only serializable parts of shell result on evaluation --- .../src/iframe-runtime/iframe-runtime.ts | 9 ++++---- packages/browser-runtime-core/src/index.ts | 2 +- .../src/open-context-runtime.ts | 21 +++++++++++------ packages/browser-runtime-core/src/runtime.ts | 21 +++++++++++++---- .../src/electron-runtime.spec.ts | 4 ++-- .../src/electron-runtime.ts | 9 ++++---- .../node-runtime-worker-thread/package.json | 1 - .../src/child-process-evaluation-listener.ts | 6 ++--- .../node-runtime-worker-thread/src/index.ts | 9 ++++---- .../src/serializer.ts | 23 +++++++++---------- .../src/worker-runtime.spec.ts | 18 +++++++-------- .../src/worker-runtime.ts | 5 ++-- 12 files changed, 72 insertions(+), 56 deletions(-) diff --git a/packages/browser-repl/src/iframe-runtime/iframe-runtime.ts b/packages/browser-repl/src/iframe-runtime/iframe-runtime.ts index 5c5549f2dd..1a8c502edf 100644 --- a/packages/browser-repl/src/iframe-runtime/iframe-runtime.ts +++ b/packages/browser-repl/src/iframe-runtime/iframe-runtime.ts @@ -4,12 +4,13 @@ import { import { Runtime, + RuntimeEvaluationListener, + RuntimeEvaluationResult, Completion, OpenContextRuntime } from '@mongosh/browser-runtime-core'; import { ServiceProvider } from '@mongosh/service-provider-core'; -import { ShellResult, EvaluationListener } from '@mongosh/shell-evaluator'; export class IframeRuntime implements Runtime { private openContextRuntime: OpenContextRuntime | null = null; @@ -17,13 +18,13 @@ export class IframeRuntime implements Runtime { private iframe: HTMLIFrameElement | null = null; private container: HTMLDivElement | null = null; private serviceProvider: ServiceProvider; - private evaluationListener: EvaluationListener | null = null; + private evaluationListener: RuntimeEvaluationListener | null = null; constructor(serviceProvider: ServiceProvider) { this.serviceProvider = serviceProvider; } - setEvaluationListener(listener: EvaluationListener): EvaluationListener | null { + setEvaluationListener(listener: RuntimeEvaluationListener): RuntimeEvaluationListener | null { const prev = this.evaluationListener; this.evaluationListener = listener; if (this.openContextRuntime) { @@ -32,7 +33,7 @@ export class IframeRuntime implements Runtime { return prev; } - async evaluate(code: string): Promise { + async evaluate(code: string): Promise { const runtime = await this.initialize(); return await runtime.evaluate(code); } diff --git a/packages/browser-runtime-core/src/index.ts b/packages/browser-runtime-core/src/index.ts index 0595be6734..2de9fd97dc 100644 --- a/packages/browser-runtime-core/src/index.ts +++ b/packages/browser-runtime-core/src/index.ts @@ -1,4 +1,4 @@ -export { Runtime } from './runtime'; +export { Runtime, RuntimeEvaluationListener, RuntimeEvaluationResult } from './runtime'; export { ContextValue, InterpreterEnvironment } from './interpreter'; export { OpenContextRuntime } from './open-context-runtime'; export { Autocompleter, Completion } from './autocompleter/autocompleter'; diff --git a/packages/browser-runtime-core/src/open-context-runtime.ts b/packages/browser-runtime-core/src/open-context-runtime.ts index 183f301756..c2229f9427 100644 --- a/packages/browser-runtime-core/src/open-context-runtime.ts +++ b/packages/browser-runtime-core/src/open-context-runtime.ts @@ -2,11 +2,15 @@ import { Completion } from './autocompleter/autocompleter'; import { ServiceProvider } from '@mongosh/service-provider-core'; import { ShellApiAutocompleter } from './autocompleter/shell-api-autocompleter'; import { Interpreter, InterpreterEnvironment } from './interpreter'; -import { Runtime } from './runtime'; +import { + Runtime, + RuntimeEvaluationResult, + RuntimeEvaluationListener +} from './runtime'; import { EventEmitter } from 'events'; -import { ShellInternalState, ShellResult } from '@mongosh/shell-api'; +import { ShellInternalState } from '@mongosh/shell-api'; -import { ShellEvaluator, EvaluationListener } from '@mongosh/shell-evaluator'; +import { ShellEvaluator } from '@mongosh/shell-evaluator'; import type { MongoshBus } from '@mongosh/types'; /** @@ -23,7 +27,7 @@ export class OpenContextRuntime implements Runtime { private autocompleter: ShellApiAutocompleter | null = null; private shellEvaluator: ShellEvaluator; private internalState: ShellInternalState; - private evaluationListener: EvaluationListener | null = null; + private evaluationListener: RuntimeEvaluationListener | null = null; private updatedConnectionInfo = false; constructor( @@ -48,17 +52,20 @@ export class OpenContextRuntime implements Runtime { return this.autocompleter.getCompletions(code); } - async evaluate(code: string): Promise { + async evaluate(code: string): Promise { const evalFn = this.interpreter.evaluate.bind(this.interpreter); - return await this.shellEvaluator.customEval( + // Omitting rawValue before returning the result hence the unused variable + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { rawValue, ...result } = await this.shellEvaluator.customEval( evalFn, code, this.interpreterEnvironment.getContextObject(), '' ); + return result; } - setEvaluationListener(listener: EvaluationListener): EvaluationListener | null { + setEvaluationListener(listener: RuntimeEvaluationListener): RuntimeEvaluationListener | null { const prev = this.evaluationListener; this.evaluationListener = listener; this.internalState.setEvaluationListener(listener); diff --git a/packages/browser-runtime-core/src/runtime.ts b/packages/browser-runtime-core/src/runtime.ts index 220eab3780..8683d1553e 100644 --- a/packages/browser-runtime-core/src/runtime.ts +++ b/packages/browser-runtime-core/src/runtime.ts @@ -3,23 +3,34 @@ import { ShellResult, EvaluationListener } from '@mongosh/shell-evaluator'; export type ContextValue = any; +export type RuntimeEvaluationResult = Pick< + ShellResult, + 'type' | 'printable' | 'source' +>; + +export interface RuntimeEvaluationListener extends EvaluationListener { + onPrint?: (value: RuntimeEvaluationResult[]) => Promise | void; +} + export interface Runtime { /** * Sets a listener for certain events, e.g. onPrint() when print() is called * in the shell. * - * @param {EvaluationListener} listener - The new listener. - * @return {EvaluationListener | null} The previous listener, if any. + * @param {RuntimeEvaluationListener} listener - The new listener. + * @return {RuntimeEvaluationListener | null} The previous listener, if any. */ - setEvaluationListener(listener: EvaluationListener): EvaluationListener | null; + setEvaluationListener( + listener: RuntimeEvaluationListener + ): RuntimeEvaluationListener | null; /** * Evaluates code * * @param {string} code - A string of code - * @return {Promise} the result of the evaluation + * @return {Promise} the result of the evaluation */ - evaluate(code: string): Promise; + evaluate(code: string): Promise; /** * Get shell api completions give a code prefix diff --git a/packages/browser-runtime-electron/src/electron-runtime.spec.ts b/packages/browser-runtime-electron/src/electron-runtime.spec.ts index 373ad54cbc..0c765ae910 100644 --- a/packages/browser-runtime-electron/src/electron-runtime.spec.ts +++ b/packages/browser-runtime-electron/src/electron-runtime.spec.ts @@ -8,12 +8,12 @@ import { CliServiceProvider } from '@mongosh/service-provider-server'; import { bson } from '@mongosh/service-provider-core'; import { ElectronRuntime } from './electron-runtime'; import { EventEmitter } from 'events'; -import { EvaluationListener } from '@mongosh/shell-evaluator'; +import { RuntimeEvaluationListener } from '@mongosh/browser-runtime-core'; describe('Electron runtime', function() { let serviceProvider: SinonStubbedInstance; let messageBus: SinonStubbedInstance; - let evaluationListener: SinonStubbedInstance; + let evaluationListener: SinonStubbedInstance; let electronRuntime: ElectronRuntime; beforeEach(async() => { diff --git a/packages/browser-runtime-electron/src/electron-runtime.ts b/packages/browser-runtime-electron/src/electron-runtime.ts index fa090f029a..0edeeb742a 100644 --- a/packages/browser-runtime-electron/src/electron-runtime.ts +++ b/packages/browser-runtime-electron/src/electron-runtime.ts @@ -6,11 +6,12 @@ import { import { Runtime, OpenContextRuntime, - Completion + Completion, + RuntimeEvaluationListener, + RuntimeEvaluationResult } from '@mongosh/browser-runtime-core'; import { ServiceProvider } from '@mongosh/service-provider-core'; -import { ShellResult, EvaluationListener } from '@mongosh/shell-evaluator'; import type { MongoshBus } from '@mongosh/types'; declare const __webpack_require__: any; @@ -42,11 +43,11 @@ export class ElectronRuntime implements Runtime { ); } - setEvaluationListener(listener: EvaluationListener): EvaluationListener | null { + setEvaluationListener(listener: RuntimeEvaluationListener): RuntimeEvaluationListener | null { return this.openContextRuntime.setEvaluationListener(listener); } - async evaluate(code: string): Promise { + async evaluate(code: string): Promise { return await this.openContextRuntime.evaluate(code); } diff --git a/packages/node-runtime-worker-thread/package.json b/packages/node-runtime-worker-thread/package.json index 4acde56eae..780da41e68 100644 --- a/packages/node-runtime-worker-thread/package.json +++ b/packages/node-runtime-worker-thread/package.json @@ -43,7 +43,6 @@ "@mongosh/browser-runtime-electron": "0.0.0-dev.0", "@mongosh/service-provider-core": "0.0.0-dev.0", "@mongosh/service-provider-server": "0.0.0-dev.0", - "@mongosh/shell-evaluator": "0.0.0-dev.0", "bson": "^4.2.2", "postmsg-rpc": "^2.4.0" } diff --git a/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts b/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts index 1ac19271cb..3c55f2ae26 100644 --- a/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts +++ b/packages/node-runtime-worker-thread/src/child-process-evaluation-listener.ts @@ -1,13 +1,13 @@ import { ChildProcess } from 'child_process'; -import { EvaluationListener } from '@mongosh/shell-evaluator'; import { exposeAll, WithClose } from './rpc'; import type { WorkerRuntime } from './index'; +import { RuntimeEvaluationListener } from '@mongosh/browser-runtime-core'; export class ChildProcessEvaluationListener { - exposedListener: WithClose; + exposedListener: WithClose; constructor(workerRuntime: WorkerRuntime, childProcess: ChildProcess) { - this.exposedListener = exposeAll( + this.exposedListener = exposeAll( { onPrompt(question, type) { return ( diff --git a/packages/node-runtime-worker-thread/src/index.ts b/packages/node-runtime-worker-thread/src/index.ts index 115e958895..0216f47174 100644 --- a/packages/node-runtime-worker-thread/src/index.ts +++ b/packages/node-runtime-worker-thread/src/index.ts @@ -3,8 +3,7 @@ import { ChildProcess, SpawnOptionsWithoutStdio } from 'child_process'; import { MongoClientOptions } from '@mongosh/service-provider-core'; -import { Runtime } from '@mongosh/browser-runtime-core'; -import { EvaluationListener } from '@mongosh/shell-evaluator'; +import { Runtime, RuntimeEvaluationListener, RuntimeEvaluationResult } from '@mongosh/browser-runtime-core'; import spawnChildFromSource, { kill } from './spawn-child-from-source'; import { Caller, createCaller } from './rpc'; import { ChildProcessEvaluationListener } from './child-process-evaluation-listener'; @@ -21,7 +20,7 @@ class WorkerRuntime implements Runtime { spawnOptions: SpawnOptionsWithoutStdio; }; - evaluationListener: EvaluationListener | null = null; + evaluationListener: RuntimeEvaluationListener | null = null; private childProcessEvaluationListener!: ChildProcessEvaluationListener; @@ -62,7 +61,7 @@ class WorkerRuntime implements Runtime { await this.childProcessRuntime.init(uri, driverOptions, cliOptions); } - async evaluate(code: string) { + async evaluate(code: string): Promise { await this.initWorkerPromise; return deserializeEvaluationResult( await this.childProcessRuntime.evaluate(code) @@ -79,7 +78,7 @@ class WorkerRuntime implements Runtime { return await this.childProcessRuntime.getShellPrompt(); } - setEvaluationListener(listener: EvaluationListener | null) { + setEvaluationListener(listener: RuntimeEvaluationListener | null) { const prev = this.evaluationListener; this.evaluationListener = listener; return prev; diff --git a/packages/node-runtime-worker-thread/src/serializer.ts b/packages/node-runtime-worker-thread/src/serializer.ts index a41fc1bac9..09cd920592 100644 --- a/packages/node-runtime-worker-thread/src/serializer.ts +++ b/packages/node-runtime-worker-thread/src/serializer.ts @@ -1,7 +1,7 @@ import v8 from 'v8'; -import { ShellResult } from '@mongosh/shell-evaluator'; import { inspect } from 'util'; import { EJSON, Document } from 'bson'; +import { RuntimeEvaluationResult } from '@mongosh/browser-runtime-core'; export function serialize(data: unknown): string { return `data:;base64,${v8.serialize(data).toString('base64')}`; @@ -66,23 +66,25 @@ type SerializedShellApiResult = { nonEnumPropertyDescriptors: Record; }; -export function serializeEvaluationResult(result: ShellResult): ShellResult { +export function serializeEvaluationResult( + result: RuntimeEvaluationResult +): RuntimeEvaluationResult { result = { ...result }; // Type `null` indicates anything that is not returned by shell-api, any // usual javascript value returned from evaluation if (result.type === null) { // Errors get special treatment to achieve better serialization - if (isError(result.rawValue)) { + if (isError(result.printable)) { result.type = SerializedResultTypes.SerializedErrorResult; - result.printable = serializeError(result.rawValue); - } else if (!isPrimitive(result.rawValue)) { + result.printable = serializeError(result.printable); + } else if (!isPrimitive(result.printable)) { // For everything else that is not a primitive value there is just too // many possible combinations of data types to distinguish between them, // if we don't know the type and it's not an error, let's inspect and // return an inspection result result.type = SerializedResultTypes.InspectResult; - result.printable = inspect(result.rawValue); + result.printable = inspect(result.printable); } } else if (!isPrimitive(result.printable)) { // If type is present we are dealing with shell-api return value. In most @@ -103,15 +105,12 @@ export function serializeEvaluationResult(result: ShellResult): ShellResult { }; } - // `rawValue` can't be serialized "by design", we don't really care about it - // for the worker thread runtime so as an easy workaround we will just remove - // it completely from the result - delete result.rawValue; - return result; } -export function deserializeEvaluationResult(result: ShellResult): ShellResult { +export function deserializeEvaluationResult( + result: RuntimeEvaluationResult +): RuntimeEvaluationResult { result = { ...result }; if (result.type === SerializedResultTypes.SerializedErrorResult) { diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts index fa5001f1ca..c5df645033 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts @@ -8,11 +8,11 @@ import sinonChai from 'sinon-chai'; import sinon from 'sinon'; import { ObjectId } from 'bson'; import { inspect } from 'util'; -import { ShellResult } from '@mongosh/shell-evaluator'; import { startTestServer } from '../../../testing/integration-testing-hooks'; import { Caller, createCaller, exposeAll } from './rpc'; import { deserializeEvaluationResult } from './serializer'; import type { WorkerRuntime } from './worker-runtime'; +import { RuntimeEvaluationResult } from '@mongosh/browser-runtime-core'; chai.use(sinonChai); chai.use(chaiAsPromised); @@ -164,7 +164,7 @@ describe('worker', () => { [ 'show dbs', 'ShowDatabasesResult', - ({ printable }: ShellResult) => { + ({ printable }: RuntimeEvaluationResult) => { expect(printable.find(({ name }: any) => name === 'admin')).to.not .be.undefined; } @@ -174,7 +174,7 @@ describe('worker', () => { [ 'show roles', 'ShowResult', - ({ printable }: ShellResult) => { + ({ printable }: RuntimeEvaluationResult) => { expect(printable.find(({ role }: any) => role === 'dbAdmin')).to.not .be.undefined; } @@ -189,7 +189,7 @@ describe('worker', () => { [ 'help', 'Help', - ({ printable }: ShellResult) => { + ({ printable }: RuntimeEvaluationResult) => { expect(printable).to.have.property('help', 'Shell Help'); expect(printable) .to.have.property('docs') @@ -206,7 +206,7 @@ describe('worker', () => { 'db.coll.aggregate({ $match: { foo: 321 } })' ], 'AggregationCursor', - ({ printable }: ShellResult) => { + ({ printable }: RuntimeEvaluationResult) => { expect(printable).to.have.property('cursorHasMore', false); expect(printable) .to.have.nested.property('[0]._id') @@ -234,7 +234,7 @@ describe('worker', () => { 'it' ], 'CursorIterationResult', - ({ printable }: ShellResult) => { + ({ printable }: RuntimeEvaluationResult) => { expect(printable).to.include.deep.members([{ a: 'a' }]); } ] @@ -244,7 +244,7 @@ describe('worker', () => { [ [`use ${db}`, 'db.coll.insertOne({ a: "a" })'], 'InsertOneResult', - ({ printable }: ShellResult) => { + ({ printable }: RuntimeEvaluationResult) => { expect(printable).to.have.property('acknowledged', true); expect(printable) .to.have.property('insertedId') @@ -254,7 +254,7 @@ describe('worker', () => { [ [`use ${db}`, 'db.coll.insertMany([{ b: "b" }, { c: "c" }])'], 'InsertManyResult', - ({ printable }: ShellResult) => { + ({ printable }: RuntimeEvaluationResult) => { expect(printable).to.have.property('acknowledged', true); expect(printable) .to.have.nested.property('insertedIds[0]') @@ -288,7 +288,7 @@ describe('worker', () => { [ [`use ${db}`, 'db.coll.bulkWrite([{ insertOne: { d: "d" } }])'], 'BulkWriteResult', - ({ printable }: ShellResult) => { + ({ printable }: RuntimeEvaluationResult) => { expect(printable).to.have.property('acknowledged', true); expect(printable).to.have.property('insertedCount', 1); expect(printable) diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.ts b/packages/node-runtime-worker-thread/src/worker-runtime.ts index 635c15b837..4f8e7a6ccd 100644 --- a/packages/node-runtime-worker-thread/src/worker-runtime.ts +++ b/packages/node-runtime-worker-thread/src/worker-runtime.ts @@ -2,14 +2,13 @@ /* ^^^ we test the dist directly, so isntanbul can't calculate the coverage correctly */ import { parentPort, isMainThread } from 'worker_threads'; -import { Runtime } from '@mongosh/browser-runtime-core'; +import { Runtime, RuntimeEvaluationListener } from '@mongosh/browser-runtime-core'; import { ElectronRuntime } from '@mongosh/browser-runtime-electron'; import { MongoClientOptions, ServiceProvider } from '@mongosh/service-provider-core'; import { CliServiceProvider } from '@mongosh/service-provider-server'; -import { EvaluationListener } from '@mongosh/shell-evaluator'; import { exposeAll, createCaller } from './rpc'; import { serializeEvaluationResult } from './serializer'; @@ -30,7 +29,7 @@ function ensureRuntime(methodName: string): Runtime { return runtime; } -const evaluationListener = createCaller( +const evaluationListener = createCaller( ['onPrint', 'onPrompt', 'toggleTelemetry', 'onClearCommand', 'onExit'], parentPort ); From e42b23b97aa564dc339413a026e06882347d5546 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Tue, 26 Jan 2021 18:00:43 +0100 Subject: [PATCH 12/21] refactor(node-runtime-worker-thread): Simplify {de}serialization implementation --- .../src/serializer.ts | 135 +++++++++--------- 1 file changed, 67 insertions(+), 68 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/serializer.ts b/packages/node-runtime-worker-thread/src/serializer.ts index 09cd920592..f73b237fae 100644 --- a/packages/node-runtime-worker-thread/src/serializer.ts +++ b/packages/node-runtime-worker-thread/src/serializer.ts @@ -1,6 +1,6 @@ import v8 from 'v8'; import { inspect } from 'util'; -import { EJSON, Document } from 'bson'; +import { EJSON } from 'bson'; import { RuntimeEvaluationResult } from '@mongosh/browser-runtime-core'; export function serialize(data: unknown): string { @@ -60,85 +60,84 @@ export enum SerializedResultTypes { SerializedShellApiResult = 'SerializedShellApiResult' } -type SerializedShellApiResult = { - origType: string; - serializedValue: Document; - nonEnumPropertyDescriptors: Record; -}; - -export function serializeEvaluationResult( - result: RuntimeEvaluationResult -): RuntimeEvaluationResult { - result = { ...result }; - - // Type `null` indicates anything that is not returned by shell-api, any - // usual javascript value returned from evaluation - if (result.type === null) { - // Errors get special treatment to achieve better serialization - if (isError(result.printable)) { - result.type = SerializedResultTypes.SerializedErrorResult; - result.printable = serializeError(result.printable); - } else if (!isPrimitive(result.printable)) { - // For everything else that is not a primitive value there is just too - // many possible combinations of data types to distinguish between them, - // if we don't know the type and it's not an error, let's inspect and - // return an inspection result - result.type = SerializedResultTypes.InspectResult; - result.printable = inspect(result.printable); - } - } else if (!isPrimitive(result.printable)) { - // If type is present we are dealing with shell-api return value. In most - // cases printable values of shell-api are primitive or serializable as - // (E)JSON, but they also might have some non-enumerable stuff (looking at - // you, Cursor!) that we need to serialize additionally, otherwiser those - // params will be lost in the ether causing issues down the way - const origType = result.type; - result.type = SerializedResultTypes.SerializedShellApiResult; - result.printable = { - origType, - serializedValue: EJSON.serialize(result.printable), - nonEnumPropertyDescriptors: Object.fromEntries( - Object.entries( - Object.getOwnPropertyDescriptors(result.printable) - ).filter(([, descriptor]) => !descriptor.enumerable) - ) +export function serializeEvaluationResult({ + type, + printable, + source +}: RuntimeEvaluationResult): RuntimeEvaluationResult { + // Primitive values don't require any special treatment for serialization + if (isPrimitive(printable)) { + return { type, printable, source }; + } + + // Errors are serialized as some error metadata can be lost without this + if (isError(printable)) { + return { + type: SerializedResultTypes.SerializedErrorResult, + printable: serializeError(printable), + source }; } - return result; -} + // `null` type indicates evaluation result that is anyuthing, but shell-api + // result. There are too many different combinations of what this can be, both + // easily serializable and completely non-serializable. Instead of handing + // those cases and becase we don't really care for preserving those as close + // to real value as possible, we will convert them to inspect string result + // before passing to the main thread + if (type === null) { + return { + type: SerializedResultTypes.InspectResult, + printable: inspect(printable), + source + }; + } -export function deserializeEvaluationResult( - result: RuntimeEvaluationResult -): RuntimeEvaluationResult { - result = { ...result }; + // For everything else that we consider a shell-api result we will do our best + // to preserve as much information as possible, including serializing the + // printable value to EJSON as its a common thing to be returned by shell-api + // and copying over all non-enumerable properties from the result + return { + type: SerializedResultTypes.SerializedShellApiResult, + printable: { + origType: type, + serializedValue: EJSON.serialize(printable), + nonEnumPropertyDescriptors: Object.fromEntries( + Object.entries(Object.getOwnPropertyDescriptors(printable)).filter( + ([, descriptor]) => !descriptor.enumerable + ) + ) + } + }; +} - if (result.type === SerializedResultTypes.SerializedErrorResult) { - result.type = null; - result.printable = deserializeError(result.printable); +export function deserializeEvaluationResult({ + type, + printable, + source +}: RuntimeEvaluationResult): RuntimeEvaluationResult { + if (type === SerializedResultTypes.SerializedErrorResult) { + return { type, printable: deserializeError(printable), source }; } - if (result.type === SerializedResultTypes.SerializedShellApiResult) { - const value: SerializedShellApiResult = result.printable; - const printable = EJSON.deserialize(value.serializedValue); + if (type === SerializedResultTypes.SerializedShellApiResult) { + const deserializedValue = EJSON.deserialize(printable.serializedValue); // Primitives should not end up here ever, but we need to convince TS that // its true - if (!isPrimitive(printable)) { - Object.defineProperties(printable, value.nonEnumPropertyDescriptors); + if (!isPrimitive(deserializedValue)) { + Object.defineProperties( + deserializedValue, + printable.nonEnumPropertyDescriptors + ); } - result.type = value.origType; - result.printable = printable; + return { + type: printable.origType, + printable: deserializedValue, + source + }; } - Object.defineProperty(result, 'rawValue', { - get() { - throw new Error( - '`rawValue` is not available for evaluation result produced by WorkerRuntime' - ); - } - }); - - return result; + return { type, printable, source }; } From d0f3aefe86440672090cc4515083d5ddbcb54ad8 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Tue, 26 Jan 2021 18:02:46 +0100 Subject: [PATCH 13/21] refactor(node-runtime-worker-thread): Move v8 {de}serialize to rpc helper as the sole user of those methods --- .../node-runtime-worker-thread/src/rpc.ts | 21 +++++++++++++------ .../src/serializer.ts | 14 ------------- 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/packages/node-runtime-worker-thread/src/rpc.ts b/packages/node-runtime-worker-thread/src/rpc.ts index 53e197a118..f2d2818961 100644 --- a/packages/node-runtime-worker-thread/src/rpc.ts +++ b/packages/node-runtime-worker-thread/src/rpc.ts @@ -1,3 +1,4 @@ +import v8 from 'v8'; import { expose, caller, @@ -6,12 +7,20 @@ import { ServerMessageData, ClientMessageData } from 'postmsg-rpc'; -import { - serialize, - deserialize, - deserializeError, - serializeError -} from './serializer'; +import { deserializeError, serializeError } from './serializer'; + +function serialize(data: unknown): string { + return `data:;base64,${v8.serialize(data).toString('base64')}`; +} + +function deserialize(str: string): T | string { + if (/^data:;base64,.+/.test(str)) { + return v8.deserialize( + Buffer.from(str.replace('data:;base64,', ''), 'base64') + ); + } + return str; +} type RPCMessageBus = { on: Function; off: Function } & ( | { postMessage: Function; send?: never } diff --git a/packages/node-runtime-worker-thread/src/serializer.ts b/packages/node-runtime-worker-thread/src/serializer.ts index f73b237fae..497650e8e1 100644 --- a/packages/node-runtime-worker-thread/src/serializer.ts +++ b/packages/node-runtime-worker-thread/src/serializer.ts @@ -1,21 +1,7 @@ -import v8 from 'v8'; import { inspect } from 'util'; import { EJSON } from 'bson'; import { RuntimeEvaluationResult } from '@mongosh/browser-runtime-core'; -export function serialize(data: unknown): string { - return `data:;base64,${v8.serialize(data).toString('base64')}`; -} - -export function deserialize(str: string): T | string { - if (/^data:;base64,.+/.test(str)) { - return v8.deserialize( - Buffer.from(str.replace('data:;base64,', ''), 'base64') - ); - } - return str; -} - function isPrimitive( val: any ): val is boolean | string | number | undefined | null { From 38be23ade49618839d354c30b4f358dedabada55 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Wed, 27 Jan 2021 08:51:53 +0100 Subject: [PATCH 14/21] refactor(shell-api, browser-repl, cli-repl, node-runtime-worker-thread): Do not rely on non-enumerable, non-serializable properties in Cursor printable --- .../types/cursor-iteration-result-output.tsx | 19 +++++---- .../components/types/cursor-output.spec.tsx | 9 ++-- .../src/components/types/cursor-output.tsx | 4 +- packages/cli-repl/src/format-output.spec.ts | 42 +++++++++++-------- packages/cli-repl/src/format-output.ts | 6 +-- .../src/serializer.ts | 21 +--------- .../src/worker-runtime.spec.ts | 11 +++-- packages/shell-api/src/result.ts | 15 +++++-- 8 files changed, 67 insertions(+), 60 deletions(-) diff --git a/packages/browser-repl/src/components/types/cursor-iteration-result-output.tsx b/packages/browser-repl/src/components/types/cursor-iteration-result-output.tsx index 3805ad0c71..ae48d1bb5d 100644 --- a/packages/browser-repl/src/components/types/cursor-iteration-result-output.tsx +++ b/packages/browser-repl/src/components/types/cursor-iteration-result-output.tsx @@ -8,7 +8,7 @@ export interface Document { } interface CursorIterationResultOutputProps { - value: Document[] & { cursorHasMore: boolean }; + value: { documents: Document[]; cursorHasMore: boolean }; } export class CursorIterationResultOutput extends Component { @@ -17,17 +17,22 @@ export class CursorIterationResultOutput extends Component{i18n.__('shell-api.classes.Cursor.iteration.no-cursor')}; + if (!this.props.value.documents.length) { + return ( +
{i18n.__('shell-api.classes.Cursor.iteration.no-cursor')}
+ ); } const more = this.props.value.cursorHasMore ? (
{i18n.__('shell-api.classes.Cursor.iteration.type-it-for-more')}
) : ''; - return (
- {this.props.value.map(this.renderDocument)} - {more} -
); + + return ( +
+ {this.props.value.documents.map(this.renderDocument)} + {more} +
+ ); } renderDocument = (document: Document, i: number): JSX.Element => { diff --git a/packages/browser-repl/src/components/types/cursor-output.spec.tsx b/packages/browser-repl/src/components/types/cursor-output.spec.tsx index e586a262e9..e71788c134 100644 --- a/packages/browser-repl/src/components/types/cursor-output.spec.tsx +++ b/packages/browser-repl/src/components/types/cursor-output.spec.tsx @@ -7,14 +7,15 @@ import { CursorIterationResultOutput } from './cursor-iteration-result-output'; describe('CursorOutput', () => { it('renders "no cursor" if value is empty', () => { - const wrapper = shallow(); + const docs = { documents: [], cursorHasMore: false }; + const wrapper = shallow(); expect(wrapper.find(CursorIterationResultOutput)).to.have.lengthOf(0); }); it('renders a CursorIterationResultOutput if value contains elements', () => { - const docs = [{ doc: 1 }, { doc: 2 }]; + const docs = { documents: [{ doc: 1 }, { doc: 2 }], cursorHasMore: false }; const wrapper = shallow(); expect(wrapper.find(CursorIterationResultOutput).prop('value')).to.deep.equal(docs); @@ -22,7 +23,7 @@ describe('CursorOutput', () => { context('when value has more elements available', () => { it('prompts to type "it"', () => { - const docs = Object.assign([{}], { cursorHasMore: true }); + const docs = { documents: [{}], cursorHasMore: true }; const wrapper = mount(); expect(wrapper.find(CursorIterationResultOutput).text()).to.contain('Type "it" for more'); @@ -31,7 +32,7 @@ describe('CursorOutput', () => { context('when value does not have more elements available', () => { it('does not prompt to type "it"', () => { - const docs = Object.assign([{}], { cursorHasMore: false }); + const docs = { documents: [{}], cursorHasMore: false }; const wrapper = mount(); expect(wrapper.find(CursorIterationResultOutput).text()).not.to.contain('Type "it" for more'); diff --git a/packages/browser-repl/src/components/types/cursor-output.tsx b/packages/browser-repl/src/components/types/cursor-output.tsx index 84e1b0a362..706c41a688 100644 --- a/packages/browser-repl/src/components/types/cursor-output.tsx +++ b/packages/browser-repl/src/components/types/cursor-output.tsx @@ -3,7 +3,7 @@ import PropTypes from 'prop-types'; import { CursorIterationResultOutput, Document } from './cursor-iteration-result-output'; interface CursorOutputProps { - value: Document[] & { cursorHasMore: boolean }; + value: { documents: Document[], cursorHasMore: boolean }; } export class CursorOutput extends Component { @@ -12,7 +12,7 @@ export class CursorOutput extends Component { }; render(): JSX.Element { - if (!this.props.value.length) { + if (!this.props.value.documents.length) { return
;
     }
 
diff --git a/packages/cli-repl/src/format-output.spec.ts b/packages/cli-repl/src/format-output.spec.ts
index aacf777dc8..d6e3589e17 100644
--- a/packages/cli-repl/src/format-output.spec.ts
+++ b/packages/cli-repl/src/format-output.spec.ts
@@ -30,10 +30,12 @@ for (const colors of [ false, true ]) {
     context('when the result is a Cursor', () => {
       context('when the Cursor is not empty', () => {
         it('returns the inspection', () => {
-          const output = stripAnsiColors(format({
-            value: Object.assign([{ doc: 1 }, { doc: 2 }], { cursorHasMore: true }),
-            type: 'Cursor'
-          }));
+          const output = stripAnsiColors(
+            format({
+              value: { documents: [{ doc: 1 }, { doc: 2 }], cursorHasMore: true },
+              type: 'Cursor'
+            })
+          );
 
           expect(output).to.include('doc: 1');
           expect(output).to.include('doc: 2');
@@ -42,10 +44,12 @@ for (const colors of [ false, true ]) {
 
       context('when the Cursor is empty', () => {
         it('returns an empty string', () => {
-          const output = stripAnsiColors(format({
-            value: Object.assign([], { cursorHasMore: false }),
-            type: 'Cursor'
-          }));
+          const output = stripAnsiColors(
+            format({
+              value: { documents: [], cursorHasMore: false },
+              type: 'Cursor'
+            })
+          );
 
           expect(output).to.equal('');
         });
@@ -55,10 +59,12 @@ for (const colors of [ false, true ]) {
     context('when the result is a CursorIterationResult', () => {
       context('when the CursorIterationResult is not empty', () => {
         it('returns the inspection', () => {
-          const output = stripAnsiColors(format({
-            value: Object.assign([{ doc: 1 }, { doc: 2 }], { cursorHasMore: true }),
-            type: 'CursorIterationResult'
-          }));
+          const output = stripAnsiColors(
+            format({
+              value: { documents: [{ doc: 1 }, { doc: 2 }], cursorHasMore: true },
+              type: 'CursorIterationResult'
+            })
+          );
 
           expect(output).to.include('doc: 1');
           expect(output).to.include('doc: 2');
@@ -68,10 +74,12 @@ for (const colors of [ false, true ]) {
 
       context('when the CursorIterationResult is not empty but exhausted', () => {
         it('returns the inspection', () => {
-          const output = stripAnsiColors(format({
-            value: Object.assign([{ doc: 1 }, { doc: 2 }], { cursorHasMore: false }),
-            type: 'CursorIterationResult'
-          }));
+          const output = stripAnsiColors(
+            format({
+              value: { documents: [{ doc: 1 }, { doc: 2 }], cursorHasMore: false },
+              type: 'CursorIterationResult'
+            })
+          );
 
           expect(output).to.include('doc: 1');
           expect(output).to.include('doc: 2');
@@ -82,7 +90,7 @@ for (const colors of [ false, true ]) {
       context('when the CursorIterationResult is empty', () => {
         it('returns "no cursor"', () => {
           const output = stripAnsiColors(format({
-            value: Object.assign([], { cursorHasMore: false }),
+            value: { documents: [], cursorHasMore: false },
             type: 'CursorIterationResult'
           }));
 
diff --git a/packages/cli-repl/src/format-output.ts b/packages/cli-repl/src/format-output.ts
index 453f5c9341..2d893a1aa2 100644
--- a/packages/cli-repl/src/format-output.ts
+++ b/packages/cli-repl/src/format-output.ts
@@ -168,7 +168,7 @@ function inspect(output: any, options: FormatOptions): any {
 }
 
 function formatCursor(value: any, options: FormatOptions): any {
-  if (!value.length) {
+  if (!value.documents.length) {
     return '';
   }
 
@@ -176,11 +176,11 @@ function formatCursor(value: any, options: FormatOptions): any {
 }
 
 function formatCursorIterationResult(value: any, options: FormatOptions): any {
-  if (!value.length) {
+  if (!value.documents.length) {
     return i18n.__('shell-api.classes.Cursor.iteration.no-cursor');
   }
 
-  let ret = inspect(value, options);
+  let ret = inspect(value.documents, options);
   if (value.cursorHasMore) {
     ret += '\n' + i18n.__('shell-api.classes.Cursor.iteration.type-it-for-more');
   }
diff --git a/packages/node-runtime-worker-thread/src/serializer.ts b/packages/node-runtime-worker-thread/src/serializer.ts
index 497650e8e1..89643250e4 100644
--- a/packages/node-runtime-worker-thread/src/serializer.ts
+++ b/packages/node-runtime-worker-thread/src/serializer.ts
@@ -82,17 +82,11 @@ export function serializeEvaluationResult({
   // For everything else that we consider a shell-api result we will do our best
   // to preserve as much information as possible, including serializing the
   // printable value to EJSON as its a common thing to be returned by shell-api
-  // and copying over all non-enumerable properties from the result
   return {
     type: SerializedResultTypes.SerializedShellApiResult,
     printable: {
       origType: type,
-      serializedValue: EJSON.serialize(printable),
-      nonEnumPropertyDescriptors: Object.fromEntries(
-        Object.entries(Object.getOwnPropertyDescriptors(printable)).filter(
-          ([, descriptor]) => !descriptor.enumerable
-        )
-      )
+      serializedValue: EJSON.serialize(printable)
     }
   };
 }
@@ -107,20 +101,9 @@ export function deserializeEvaluationResult({
   }
 
   if (type === SerializedResultTypes.SerializedShellApiResult) {
-    const deserializedValue = EJSON.deserialize(printable.serializedValue);
-
-    // Primitives should not end up here ever, but we need to convince TS that
-    // its true
-    if (!isPrimitive(deserializedValue)) {
-      Object.defineProperties(
-        deserializedValue,
-        printable.nonEnumPropertyDescriptors
-      );
-    }
-
     return {
       type: printable.origType,
-      printable: deserializedValue,
+      printable: EJSON.deserialize(printable.serializedValue),
       source
     };
   }
diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts
index c5df645033..b12e62edfa 100644
--- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts
+++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts
@@ -209,12 +209,12 @@ describe('worker', () => {
           ({ printable }: RuntimeEvaluationResult) => {
             expect(printable).to.have.property('cursorHasMore', false);
             expect(printable)
-              .to.have.nested.property('[0]._id')
+              .to.have.nested.property('documents[0]._id')
               // TODO: chai assertion types
               // eslint-disable-next-line @typescript-eslint/ban-ts-comment
               // @ts-ignore
               .bson(new ObjectId('000000000000000000000000'));
-            expect(printable).to.have.nested.property('[0].foo', 321);
+            expect(printable).to.have.nested.property('documents[0].foo', 321);
           }
         ],
         [
@@ -224,7 +224,10 @@ describe('worker', () => {
             'db.coll.find({ i: { $mod: [2, 0] } }, { _id: 0 })'
           ],
           'Cursor',
-          [{ i: 2 }, { i: 4 }, { i: 6 }, { i: 8 }, { i: 10 }]
+          {
+            documents: [{ i: 2 }, { i: 4 }, { i: 6 }, { i: 8 }, { i: 10 }],
+            cursorHasMore: false
+          }
         ],
         [
           [
@@ -235,7 +238,7 @@ describe('worker', () => {
           ],
           'CursorIterationResult',
           ({ printable }: RuntimeEvaluationResult) => {
-            expect(printable).to.include.deep.members([{ a: 'a' }]);
+            expect(printable.documents).to.include.deep.members([{ a: 'a' }]);
           }
         ]
       ];
diff --git a/packages/shell-api/src/result.ts b/packages/shell-api/src/result.ts
index e76fdfeaee..b9294bf4ba 100644
--- a/packages/shell-api/src/result.ts
+++ b/packages/shell-api/src/result.ts
@@ -1,7 +1,7 @@
 import { ShellApiClass, shellApiClassDefault } from './decorators';
 import { shellApiType, asPrintable } from './enums';
 import { addHiddenDataProperty } from './helpers';
-import { ObjectIdType } from '@mongosh/service-provider-core';
+import { Document, ObjectIdType } from '@mongosh/service-provider-core';
 
 @shellApiClassDefault
 export class CommandResult extends ShellApiClass {
@@ -108,6 +108,15 @@ export class DeleteResult extends ShellApiClass {
   }
 }
 
+export class PrintableCursorIterationResult {
+  documents: Document[];
+  cursorHasMore: boolean;
+  constructor(cursor: CursorIterationResult) {
+    this.documents = [...cursor];
+    this.cursorHasMore = cursor.hasMore;
+  }
+}
+
 // NOTE: because this is inherited, the decorator does not add attributes. So no help() function.
 @shellApiClassDefault
 export class CursorIterationResult extends Array {
@@ -121,8 +130,6 @@ export class CursorIterationResult extends Array {
   }
 
   [asPrintable]() {
-    const ret = [...this];
-    addHiddenDataProperty(ret, 'cursorHasMore', this.hasMore);
-    return ret;
+    return new PrintableCursorIterationResult(this);
   }
 }

From 042da3de4380199b1ddbd8f6e66bf78086280208 Mon Sep 17 00:00:00 2001
From: Sergey Petushkov 
Date: Wed, 27 Jan 2021 11:57:55 +0100
Subject: [PATCH 15/21] fix(shell-api): Fix failing tests

---
 packages/shell-api/src/aggregation-cursor.spec.ts | 7 +++++--
 packages/shell-api/src/collection.spec.ts         | 9 +++++++--
 packages/shell-api/src/cursor.spec.ts             | 7 +++++--
 packages/shell-api/src/integration.spec.ts        | 9 +++++++--
 packages/shell-api/src/result.spec.ts             | 9 +++++++--
 packages/shell-api/src/result.ts                  | 2 +-
 6 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/packages/shell-api/src/aggregation-cursor.spec.ts b/packages/shell-api/src/aggregation-cursor.spec.ts
index ba0a917e7d..ef3636112e 100644
--- a/packages/shell-api/src/aggregation-cursor.spec.ts
+++ b/packages/shell-api/src/aggregation-cursor.spec.ts
@@ -47,7 +47,10 @@ describe('AggregationCursor', () => {
     it('sets dynamic properties', async() => {
       expect((await toShellResult(cursor)).type).to.equal('AggregationCursor');
       expect((await toShellResult(cursor._it())).type).to.equal('CursorIterationResult');
-      expect((await toShellResult(cursor)).printable).to.deep.equal([]);
+      expect((await toShellResult(cursor)).printable).to.deep.equal({
+        documents: [],
+        cursorHasMore: false
+      });
       expect((await toShellResult(cursor.help)).type).to.equal('Help');
     });
 
@@ -219,7 +222,7 @@ describe('AggregationCursor', () => {
         shellApiCursor.batchSize(10);
         const result = (await toShellResult(shellApiCursor)).printable;
         expect(i).to.equal(10);
-        expect(result.length).to.equal(10);
+        expect(result).to.have.nested.property('documents.length', 10);
       });
     });
   });
diff --git a/packages/shell-api/src/collection.spec.ts b/packages/shell-api/src/collection.spec.ts
index 286c7f328a..9e8f048d70 100644
--- a/packages/shell-api/src/collection.spec.ts
+++ b/packages/shell-api/src/collection.spec.ts
@@ -1569,8 +1569,13 @@ describe('Collection', () => {
         const cursor = await collection.find();
         const result = await toShellResult(cursor);
         expect(result.type).to.equal('Cursor');
-        expect(result.printable.length).to.not.equal(0);
-        expect(result.printable[0]._id).to.equal('abc');
+        expect(result)
+          .to.have.nested.property('printable.documents.length')
+          .not.equal(0);
+        expect(result).to.have.nested.property(
+          'printable.documents[0]._id',
+          'abc'
+        );
         expect(result.source).to.deep.equal({
           namespace: {
             db: 'db1',
diff --git a/packages/shell-api/src/cursor.spec.ts b/packages/shell-api/src/cursor.spec.ts
index fe83e643ed..510853f867 100644
--- a/packages/shell-api/src/cursor.spec.ts
+++ b/packages/shell-api/src/cursor.spec.ts
@@ -52,7 +52,10 @@ describe('Cursor', () => {
     it('sets dynamic properties', async() => {
       expect((await toShellResult(cursor)).type).to.equal('Cursor');
       expect((await toShellResult(cursor._it())).type).to.equal('CursorIterationResult');
-      expect((await toShellResult(cursor)).printable).to.deep.equal([]);
+      expect((await toShellResult(cursor)).printable).to.deep.equal({
+        documents: [],
+        cursorHasMore: false
+      });
       expect((await toShellResult(cursor.help)).type).to.equal('Help');
     });
 
@@ -768,7 +771,7 @@ describe('Cursor', () => {
         shellApiCursor.batchSize(10);
         const result = (await toShellResult(shellApiCursor)).printable;
         expect(i).to.equal(10);
-        expect(result.length).to.equal(10);
+        expect(result).to.have.nested.property('documents.length', 10);
       });
     });
   });
diff --git a/packages/shell-api/src/integration.spec.ts b/packages/shell-api/src/integration.spec.ts
index e9762c5225..d2af3c364c 100644
--- a/packages/shell-api/src/integration.spec.ts
+++ b/packages/shell-api/src/integration.spec.ts
@@ -156,8 +156,13 @@ describe('Shell API (integration)', function() {
 
         describe('when calling toShellResult on the cursor', () => {
           it('returns the right documents', async() => {
-            expect((await toShellResult(cursor)).printable.constructor).to.equal(Array);
-            expect((await toShellResult(cursor)).printable).to.deep.equal([{ doc: 2 }]);
+            expect(await toShellResult(cursor)).to.have.nested.property(
+              'printable.documents.constructor',
+              Array
+            );
+            expect(await toShellResult(cursor))
+              .to.have.nested.property('printable.documents')
+              .deep.equal([{ doc: 2 }]);
           });
         });
       });
diff --git a/packages/shell-api/src/result.spec.ts b/packages/shell-api/src/result.spec.ts
index c4787087b0..379c02e0fc 100644
--- a/packages/shell-api/src/result.spec.ts
+++ b/packages/shell-api/src/result.spec.ts
@@ -125,8 +125,13 @@ describe('Results', () => {
       expect(r.length).to.equal(3);
     });
     it('toShellResult', async() => {
-      expect((await toShellResult(r)).type).to.equal('CursorIterationResult');
-      expect((await toShellResult(r)).printable).to.deep.equal(JSON.parse(JSON.stringify(r)));
+      expect(await toShellResult(r)).to.have.property(
+        'type',
+        'CursorIterationResult'
+      );
+      expect(await toShellResult(r))
+        .to.have.nested.property('printable.documents')
+        .deep.equal(JSON.parse(JSON.stringify(r)));
     });
   });
 });
diff --git a/packages/shell-api/src/result.ts b/packages/shell-api/src/result.ts
index b9294bf4ba..1332fdbd6d 100644
--- a/packages/shell-api/src/result.ts
+++ b/packages/shell-api/src/result.ts
@@ -108,7 +108,7 @@ export class DeleteResult extends ShellApiClass {
   }
 }
 
-export class PrintableCursorIterationResult {
+class PrintableCursorIterationResult {
   documents: Document[];
   cursorHasMore: boolean;
   constructor(cursor: CursorIterationResult) {

From 4846f1e46734d2d0e86154a18e5bb07461c7b986 Mon Sep 17 00:00:00 2001
From: Sergey Petushkov 
Date: Wed, 27 Jan 2021 12:03:52 +0100
Subject: [PATCH 16/21] refactor(node-runtime-worker-thread): Remove custom
 bson assertion

---
 .../src/worker-runtime.spec.ts                | 35 +++++--------------
 1 file changed, 8 insertions(+), 27 deletions(-)

diff --git a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts
index b12e62edfa..07534a46f3 100644
--- a/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts
+++ b/packages/node-runtime-worker-thread/src/worker-runtime.spec.ts
@@ -6,8 +6,7 @@ import chai, { expect } from 'chai';
 import chaiAsPromised from 'chai-as-promised';
 import sinonChai from 'sinon-chai';
 import sinon from 'sinon';
-import { ObjectId } from 'bson';
-import { inspect } from 'util';
+import { EJSON, ObjectId } from 'bson';
 import { startTestServer } from '../../../testing/integration-testing-hooks';
 import { Caller, createCaller, exposeAll } from './rpc';
 import { deserializeEvaluationResult } from './serializer';
@@ -17,24 +16,6 @@ import { RuntimeEvaluationResult } from '@mongosh/browser-runtime-core';
 chai.use(sinonChai);
 chai.use(chaiAsPromised);
 
-const chaiBson: Chai.ChaiPlugin = (chai) => {
-  chai.Assertion.addMethod('bson', function bson(expected) {
-    const obj = this._obj;
-
-    new chai.Assertion(obj).to.be.instanceof(expected.constructor);
-
-    this.assert(
-      inspect(obj) === inspect(expected),
-      'expected #{this} to match #{exp} but got #{act}',
-      'expected #{this} not to match #{exp}',
-      obj,
-      expected
-    );
-  });
-};
-
-chai.use(chaiBson);
-
 // We need a compiled version so we can import it as a worker
 const workerThreadModule = fs.readFile(
   path.resolve(__dirname, '..', 'dist', 'worker-runtime.js'),
@@ -208,13 +189,13 @@ describe('worker', () => {
           'AggregationCursor',
           ({ printable }: RuntimeEvaluationResult) => {
             expect(printable).to.have.property('cursorHasMore', false);
-            expect(printable)
-              .to.have.nested.property('documents[0]._id')
-              // TODO: chai assertion types
-              // eslint-disable-next-line @typescript-eslint/ban-ts-comment
-              // @ts-ignore
-              .bson(new ObjectId('000000000000000000000000'));
-            expect(printable).to.have.nested.property('documents[0].foo', 321);
+            const doc = printable.documents[0];
+            expect(EJSON.serialize(doc)).to.deep.equal(
+              EJSON.serialize({
+                _id: new ObjectId('000000000000000000000000'),
+                foo: 321
+              })
+            );
           }
         ],
         [

From be9ba41664a31bad894622b1c3ce747ce30c8cd6 Mon Sep 17 00:00:00 2001
From: Sergey Petushkov 
Date: Wed, 27 Jan 2021 13:48:30 +0100
Subject: [PATCH 17/21] fix(browser-repl): Fix more failing tests that depended
 on cursor printable

---
 .../types/cursor-iteration-result-output.spec.tsx        | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/packages/browser-repl/src/components/types/cursor-iteration-result-output.spec.tsx b/packages/browser-repl/src/components/types/cursor-iteration-result-output.spec.tsx
index 781720c169..718695c989 100644
--- a/packages/browser-repl/src/components/types/cursor-iteration-result-output.spec.tsx
+++ b/packages/browser-repl/src/components/types/cursor-iteration-result-output.spec.tsx
@@ -7,13 +7,18 @@ import { ObjectOutput } from './object-output';
 
 describe('CursorIterationResultOutput', () => {
   it('renders no ObjectOutput if value is empty', () => {
-    const wrapper = shallow();
+    const printable = { documents: [], cursorHasMore: false };
+    const wrapper = shallow();
 
     expect(wrapper.text()).to.contain('no cursor');
   });
 
   it('renders a ObjectOutput for each element in value', () => {
-    const wrapper = shallow();
+    const printable = {
+      documents: [{ doc: 1 }, { doc: 2 }],
+      cursorHasMore: false
+    };
+    const wrapper = shallow();
 
     expect(wrapper.find(ObjectOutput)).to.have.lengthOf(2);
   });

From 1ab1e9d46fa64656366451e7456b5d6987be7d83 Mon Sep 17 00:00:00 2001
From: Sergey Petushkov 
Date: Wed, 27 Jan 2021 17:38:32 +0100
Subject: [PATCH 18/21] refactor(node-runtime-worker-thread): Pick properties
 instead of omitting them

---
 packages/browser-runtime-core/src/open-context-runtime.ts | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/packages/browser-runtime-core/src/open-context-runtime.ts b/packages/browser-runtime-core/src/open-context-runtime.ts
index c2229f9427..cd9fc12a8e 100644
--- a/packages/browser-runtime-core/src/open-context-runtime.ts
+++ b/packages/browser-runtime-core/src/open-context-runtime.ts
@@ -54,15 +54,13 @@ export class OpenContextRuntime implements Runtime {
 
   async evaluate(code: string): Promise {
     const evalFn = this.interpreter.evaluate.bind(this.interpreter);
-    // Omitting rawValue before returning the result hence the unused variable
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars
-    const { rawValue, ...result } = await this.shellEvaluator.customEval(
+    const { type, printable, source } = await this.shellEvaluator.customEval(
       evalFn,
       code,
       this.interpreterEnvironment.getContextObject(),
       ''
     );
-    return result;
+    return { type, printable, source };
   }
 
   setEvaluationListener(listener: RuntimeEvaluationListener): RuntimeEvaluationListener | null {

From 6cd0e3bca409a6ce96d9ad585611863d56fef24d Mon Sep 17 00:00:00 2001
From: Sergey Petushkov 
Date: Wed, 27 Jan 2021 18:16:09 +0100
Subject: [PATCH 19/21] refactor(node-runtime-worker-thread): Explicitly pass
 typed payloads in rpc helper internally

---
 .../src/rpc.spec.ts                           |  7 ++--
 .../node-runtime-worker-thread/src/rpc.ts     | 32 +++++++++++++------
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/packages/node-runtime-worker-thread/src/rpc.spec.ts b/packages/node-runtime-worker-thread/src/rpc.spec.ts
index f7786ad98b..632c369a79 100644
--- a/packages/node-runtime-worker-thread/src/rpc.spec.ts
+++ b/packages/node-runtime-worker-thread/src/rpc.spec.ts
@@ -152,8 +152,11 @@ describe('rpc', () => {
         // Due to how our mocks implemented we have to introduce an if here to
         // skip our own message being received by the message bus
         if (data.sender === 'postmsg-rpc/server') {
-          expect(data.id).to.be.equal('123abc');
-          expect(data.res).to.be.equal('Meow meow meow meow!');
+          expect(data).to.have.property('id', '123abc');
+          expect(data).to.have.nested.property(
+            'res.payload',
+            'Meow meow meow meow!'
+          );
           done();
         }
       });
diff --git a/packages/node-runtime-worker-thread/src/rpc.ts b/packages/node-runtime-worker-thread/src/rpc.ts
index f2d2818961..574f3ea6bf 100644
--- a/packages/node-runtime-worker-thread/src/rpc.ts
+++ b/packages/node-runtime-worker-thread/src/rpc.ts
@@ -29,6 +29,22 @@ type RPCMessageBus = { on: Function; off: Function } & (
 
 const ERROR = '$$ERROR';
 
+const MESSAGE = '$$MESSAGE';
+
+type RPCMessage = {
+  type: typeof MESSAGE;
+  payload: string;
+};
+
+type RPCError = {
+  type: typeof ERROR;
+  payload: Error;
+};
+
+function isRPCError(data: any): data is RPCError {
+  return data && typeof data === 'object' && data.type === ERROR;
+}
+
 function isMessageData(data: any): data is MessageData {
   return data && typeof data === 'object' && 'id' in data && 'sender' in data;
 }
@@ -82,8 +98,8 @@ function getRPCOptions(messageBus: RPCMessageBus): PostmsgRpcOptions {
         // resolve
         try {
           data.res = serialize(data.res);
-        } catch (err) {
-          data.res = { [ERROR]: serializeError(err) };
+        } catch (e) {
+          data.res = serialize({ type: ERROR, payload: serializeError(e) });
         }
       }
 
@@ -115,9 +131,9 @@ export function exposeAll(obj: O, messageBus: RPCMessageBus): WithClose {
       key,
       async(...args: unknown[]) => {
         try {
-          return await val(...args);
+          return { type: MESSAGE, payload: await val(...args) };
         } catch (e) {
-          return { [ERROR]: serializeError(e) };
+          return { type: ERROR, payload: serializeError(e) };
         }
       },
       getRPCOptions(messageBus)
@@ -139,11 +155,9 @@ export function createCaller(
   methodNames.forEach((name) => {
     const c = caller(name as string, getRPCOptions(messageBus));
     (obj as any)[name] = async(...args: unknown[]) => {
-      const result = await c(...args);
-      if (typeof result === 'object' && result !== null && ERROR in result) {
-        throw deserializeError((result as any)[ERROR]);
-      }
-      return result;
+      const result = (await c(...args)) as RPCError | RPCMessage;
+      if (isRPCError(result)) throw deserializeError(result.payload);
+      return result.payload;
     };
   });
   return obj as Caller;

From 8a77045d88f204d6aad2d18d3ce21897bc1cfb81 Mon Sep 17 00:00:00 2001
From: Sergey Petushkov 
Date: Thu, 28 Jan 2021 10:36:55 +0100
Subject: [PATCH 20/21] chore(node-runtime-worker-thread): Remove confusing
 comments and replace them with (hopefully) less confusing ones

---
 packages/node-runtime-worker-thread/src/rpc.ts | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/packages/node-runtime-worker-thread/src/rpc.ts b/packages/node-runtime-worker-thread/src/rpc.ts
index 574f3ea6bf..83657270ff 100644
--- a/packages/node-runtime-worker-thread/src/rpc.ts
+++ b/packages/node-runtime-worker-thread/src/rpc.ts
@@ -86,16 +86,14 @@ function getRPCOptions(messageBus: RPCMessageBus): PostmsgRpcOptions {
     removeListener: messageBus.off.bind(messageBus),
     postMessage(data) {
       if (isClientMessageData(data) && Array.isArray(data.args)) {
-        // We don't guard against serialization errors on the client, if they
-        // happen, client is responsible for handling them
         data.args = serialize(removeTrailingUndefined(data.args));
       }
 
       if (isServerMessageData(data)) {
-        // If serialization error happened on the server, we use our special
-        // error return value to propagate the error back to the client,
-        // otherwise error can be lost on the server and client call will never
-        // resolve
+        // If serialization of the response failed for some reason (e.g., the
+        // value is not serializable) we want to propagate the error back to the
+        // client that issued the remote call instead of throwing on the server
+        // that was executing the method.
         try {
           data.res = serialize(data.res);
         } catch (e) {
@@ -133,6 +131,10 @@ export function exposeAll(obj: O, messageBus: RPCMessageBus): WithClose {
         try {
           return { type: MESSAGE, payload: await val(...args) };
         } catch (e) {
+          // If server (whatever is executing the exposed method) throws during
+          // the execution, we want to propagate error to the client (whatever
+          // issued the call) and re-throw there. We will do this with a special
+          // return type.
           return { type: ERROR, payload: serializeError(e) };
         }
       },

From 7066f06626cfdead14dffa05519c86cd5ee89aa5 Mon Sep 17 00:00:00 2001
From: Sergey Petushkov 
Date: Thu, 28 Jan 2021 17:33:42 +0100
Subject: [PATCH 21/21] refactor(node-runtime-worker-thread): Use enums instead
 of hardcoded strings

---
 .../node-runtime-worker-thread/src/rpc.ts     | 24 ++++++++++++-------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/packages/node-runtime-worker-thread/src/rpc.ts b/packages/node-runtime-worker-thread/src/rpc.ts
index 83657270ff..6efd899b19 100644
--- a/packages/node-runtime-worker-thread/src/rpc.ts
+++ b/packages/node-runtime-worker-thread/src/rpc.ts
@@ -27,22 +27,25 @@ type RPCMessageBus = { on: Function; off: Function } & (
   | { postMessage?: never; send?: Function }
 );
 
-const ERROR = '$$ERROR';
-
-const MESSAGE = '$$MESSAGE';
+enum RPCMessageTypes {
+  Message,
+  Error
+}
 
 type RPCMessage = {
-  type: typeof MESSAGE;
+  type: RPCMessageTypes.Message;
   payload: string;
 };
 
 type RPCError = {
-  type: typeof ERROR;
+  type: RPCMessageTypes.Error;
   payload: Error;
 };
 
 function isRPCError(data: any): data is RPCError {
-  return data && typeof data === 'object' && data.type === ERROR;
+  return (
+    data && typeof data === 'object' && data.type === RPCMessageTypes.Error
+  );
 }
 
 function isMessageData(data: any): data is MessageData {
@@ -97,7 +100,10 @@ function getRPCOptions(messageBus: RPCMessageBus): PostmsgRpcOptions {
         try {
           data.res = serialize(data.res);
         } catch (e) {
-          data.res = serialize({ type: ERROR, payload: serializeError(e) });
+          data.res = serialize({
+            type: RPCMessageTypes.Error,
+            payload: serializeError(e)
+          });
         }
       }
 
@@ -129,13 +135,13 @@ export function exposeAll(obj: O, messageBus: RPCMessageBus): WithClose {
       key,
       async(...args: unknown[]) => {
         try {
-          return { type: MESSAGE, payload: await val(...args) };
+          return { type: RPCMessageTypes.Message, payload: await val(...args) };
         } catch (e) {
           // If server (whatever is executing the exposed method) throws during
           // the execution, we want to propagate error to the client (whatever
           // issued the call) and re-throw there. We will do this with a special
           // return type.
-          return { type: ERROR, payload: serializeError(e) };
+          return { type: RPCMessageTypes.Error, payload: serializeError(e) };
         }
       },
       getRPCOptions(messageBus)