refactor(Code Node): Constently handle various kinds of data returned by user code (#6002)

packages/nodes-base/nodes/Code/Code.node.ts

@@ -66,8 +66,6 @@ export class Code implements INodeType {
 	};
 
 	async execute(this: IExecuteFunctions) {
-		let items = this.getInputData();
-
 		const nodeMode = this.getNodeParameter('mode', 0) as CodeNodeMode;
 		const workflowMode = this.getMode();
 
@@ -80,24 +78,25 @@ export class Code implements INodeType {
 
 			const context = getSandboxContext.call(this);
 			context.items = context.$input.all();
-			const sandbox = new Sandbox(context, workflowMode, nodeMode, this.helpers);
+			const sandbox = new Sandbox(context, jsCodeAllItems, workflowMode, this.helpers);
 
 			if (workflowMode === 'manual') {
 				sandbox.on('console.log', this.sendMessageToUI);
 			}
 
+			let result: INodeExecutionData[];
 			try {
-				items = await sandbox.runCode(jsCodeAllItems);
+				result = await sandbox.runCodeAllItems();
 			} catch (error) {
 				if (!this.continueOnFail()) return Promise.reject(error);
-				items = [{ json: { error: error.message } }];
+				result = [{ json: { error: error.message } }];
 			}
 
-			for (const item of items) {
+			for (const item of result) {
 				standardizeOutput(item.json);
 			}
 
-			return this.prepareOutputData(items);
+			return this.prepareOutputData(result);
 		}
 
 		// ----------------------------------
@@ -106,31 +105,32 @@ export class Code implements INodeType {
 
 		const returnData: INodeExecutionData[] = [];
 
-		for (let index = 0; index < items.length; index++) {
+		const items = this.getInputData();
-			let item = items[index];
 
+		for (let index = 0; index < items.length; index++) {
 			const jsCodeEachItem = this.getNodeParameter('jsCode', index) as string;
 
 			const context = getSandboxContext.call(this, index);
 			context.item = context.$input.item;
-			const sandbox = new Sandbox(context, workflowMode, nodeMode, this.helpers);
+			const sandbox = new Sandbox(context, jsCodeEachItem, workflowMode, this.helpers);
 
 			if (workflowMode === 'manual') {
 				sandbox.on('console.log', this.sendMessageToUI);
 			}
 
+			let result: INodeExecutionData | undefined;
 			try {
-				item = await sandbox.runCode(jsCodeEachItem, index);
+				result = await sandbox.runCodeEachItem(index);
 			} catch (error) {
 				if (!this.continueOnFail()) return Promise.reject(error);
 				returnData.push({ json: { error: error.message } });
 			}
 
-			if (item) {
+			if (result) {
 				returnData.push({
-					json: standardizeOutput(item.json),
+					json: standardizeOutput(result.json),
 					pairedItem: { item: index },
-					...(item.binary && { binary: item.binary }),
+					...(result.binary && { binary: result.binary }),
 				});
 			}
 		}

packages/nodes-base/nodes/Code/Sandbox.ts

@@ -1,8 +1,6 @@
-import type { NodeVMOptions } from 'vm2';
 import { NodeVM } from 'vm2';
 import { ValidationError } from './ValidationError';
 import { ExecutionError } from './ExecutionError';
-import type { CodeNodeMode } from './utils';
 import { isObject, REQUIRED_N8N_ITEM_KEYS } from './utils';
 
 import type {
@@ -13,48 +11,38 @@ import type {
 	WorkflowExecuteMode,
 } from 'n8n-workflow';
 
-export class Sandbox extends NodeVM {
+interface SandboxContext extends IWorkflowDataProxyData {
-	private jsCode = '';
+	$getNodeParameter: IExecuteFunctions['getNodeParameter'];
+	$getWorkflowStaticData: IExecuteFunctions['getWorkflowStaticData'];
+	helpers: IExecuteFunctions['helpers'];
+}
 
+const { NODE_FUNCTION_ALLOW_BUILTIN: builtIn, NODE_FUNCTION_ALLOW_EXTERNAL: external } =
+	process.env;
+
+export class Sandbox extends NodeVM {
 	private itemIndex: number | undefined = undefined;
 
 	constructor(
-		context: ReturnType<typeof getSandboxContext>,
+		context: SandboxContext,
+		private jsCode: string,
 		workflowMode: WorkflowExecuteMode,
-		private nodeMode: CodeNodeMode,
 		private helpers: IExecuteFunctions['helpers'],
 	) {
-		super(Sandbox.getSandboxOptions(context, workflowMode));
+		super({
-	}
-
-	static getSandboxOptions(
-		context: ReturnType<typeof getSandboxContext>,
-		workflowMode: WorkflowExecuteMode,
-	): NodeVMOptions {
-		const { NODE_FUNCTION_ALLOW_BUILTIN: builtIn, NODE_FUNCTION_ALLOW_EXTERNAL: external } =
-			process.env;
-
-		return {
 			console: workflowMode === 'manual' ? 'redirect' : 'inherit',
 			sandbox: context,
 			require: {
 				builtin: builtIn ? builtIn.split(',') : [],
 				external: external ? { modules: external.split(','), transitive: false } : false,
 			},
-		};
+		});
 	}
 
-	async runCode(jsCode: string, itemIndex?: number) {
+	async runCodeAllItems(): Promise<INodeExecutionData[]> {
-		this.jsCode = jsCode;
-		this.itemIndex = itemIndex;
-
-		return this.nodeMode === 'runOnceForAllItems' ? this.runCodeAllItems() : this.runCodeEachItem();
-	}
-
-	private async runCodeAllItems() {
 		const script = `module.exports = async function() {${this.jsCode}\n}()`;
 
-		let executionResult;
+		let executionResult: INodeExecutionData | INodeExecutionData[];
 
 		try {
 			executionResult = await this.run(script, __dirname);
@@ -93,55 +81,19 @@ export class Sandbox extends NodeVM {
 			);
 
 			for (const item of executionResult) {
-				if (item.json !== undefined && !isObject(item.json)) {
-					throw new ValidationError({
-						message: "A 'json' property isn't an object",
-						description: "In the returned data, every key named 'json' must point to an object",
-						itemIndex: this.itemIndex,
-					});
-				}
-
 				if (mustHaveTopLevelN8nKey) {
-					Object.keys(item as IDataObject).forEach((key) => {
+					this.validateTopLevelKeys(item);
-						if (REQUIRED_N8N_ITEM_KEYS.has(key)) return;
-						throw new ValidationError({
-							message: `Unknown top-level item key: ${key}`,
-							description: 'Access the properties of an item under `.json`, e.g. `item.json`',
-							itemIndex: this.itemIndex,
-						});
-					});
 				}
-
-				if (item.binary !== undefined && !isObject(item.binary)) {
-					throw new ValidationError({
-						message: "A 'binary' property isn't an object",
-						description: "In the returned data, every key named 'binary’ must point to an object.",
-						itemIndex: this.itemIndex,
-					});
-				}
-			}
-		} else {
-			if (executionResult.json !== undefined && !isObject(executionResult.json)) {
-				throw new ValidationError({
-					message: "A 'json' property isn't an object",
-					description: "In the returned data, every key named 'json' must point to an object",
-					itemIndex: this.itemIndex,
-				});
-			}
-
-			if (executionResult.binary !== undefined && !isObject(executionResult.binary)) {
-				throw new ValidationError({
-					message: "A 'binary' property isn't an object",
-					description: "In the returned data, every key named 'binary’ must point to an object.",
-					itemIndex: this.itemIndex,
-				});
 			}
 		}
 
-		return this.helpers.normalizeItems(executionResult as INodeExecutionData[]);
+		const returnData = this.helpers.normalizeItems(executionResult);
+		returnData.forEach((item) => this.validateResult(item));
+		return returnData;
 	}
 
-	private async runCodeEachItem() {
+	async runCodeEachItem(itemIndex: number): Promise<INodeExecutionData | undefined> {
+		this.itemIndex = itemIndex;
 		const script = `module.exports = async function() {${this.jsCode}\n}()`;
 
 		const match = this.jsCode.match(/\$input\.(?<disallowedMethod>first|last|all|itemMatching)/);
@@ -166,7 +118,7 @@ export class Sandbox extends NodeVM {
 			}
 		}
 
-		let executionResult;
+		let executionResult: INodeExecutionData;
 
 		try {
 			executionResult = await this.run(script, __dirname);
@@ -191,36 +143,6 @@ export class Sandbox extends NodeVM {
 			});
 		}
 
-		if (executionResult.json !== undefined && !isObject(executionResult.json)) {
-			throw new ValidationError({
-				message: "A 'json' property isn't an object",
-				description: "In the returned data, every key named 'json' must point to an object",
-				itemIndex: this.itemIndex,
-			});
-		}
-
-		if (executionResult.binary !== undefined && !isObject(executionResult.binary)) {
-			throw new ValidationError({
-				message: "A 'binary' property isn't an object",
-				description: "In the returned data, every key named 'binary’ must point to an object.",
-				itemIndex: this.itemIndex,
-			});
-		}
-
-		// If at least one top-level key is a supported item key (`json`, `binary`, etc.),
-		// and another top-level key is unrecognized, then the user mis-added a property
-		// directly on the item, when they intended to add it on the `json` property
-
-		Object.keys(executionResult as IDataObject).forEach((key) => {
-			if (REQUIRED_N8N_ITEM_KEYS.has(key)) return;
-
-			throw new ValidationError({
-				message: `Unknown top-level item key: ${key}`,
-				description: 'Access the properties of an item under `.json`, e.g. `item.json`',
-				itemIndex: this.itemIndex,
-			});
-		});
-
 		if (Array.isArray(executionResult)) {
 			const firstSentence =
 				executionResult.length > 0
@@ -234,27 +156,55 @@ export class Sandbox extends NodeVM {
 			});
 		}
 
-		return executionResult.json ? executionResult : { json: executionResult };
+		const [returnData] = this.helpers.normalizeItems([executionResult]);
+		this.validateResult(returnData);
+
+		// If at least one top-level key is a supported item key (`json`, `binary`, etc.),
+		// and another top-level key is unrecognized, then the user mis-added a property
+		// directly on the item, when they intended to add it on the `json` property
+		this.validateTopLevelKeys(returnData);
+
+		return returnData;
+	}
+
+	private validateResult({ json, binary }: INodeExecutionData) {
+		if (json === undefined || !isObject(json)) {
+			throw new ValidationError({
+				message: "A 'json' property isn't an object",
+				description: "In the returned data, every key named 'json' must point to an object",
+				itemIndex: this.itemIndex,
+			});
+		}
+
+		if (binary !== undefined && !isObject(binary)) {
+			throw new ValidationError({
+				message: "A 'binary' property isn't an object",
+				description: "In the returned data, every key named 'binary’ must point to an object.",
+				itemIndex: this.itemIndex,
+			});
+		}
+	}
+
+	private validateTopLevelKeys(item: INodeExecutionData) {
+		Object.keys(item).forEach((key) => {
+			if (REQUIRED_N8N_ITEM_KEYS.has(key)) return;
+			throw new ValidationError({
+				message: `Unknown top-level item key: ${key}`,
+				description: 'Access the properties of an item under `.json`, e.g. `item.json`',
+				itemIndex: this.itemIndex,
+			});
+		});
 	}
 }
 
-export function getSandboxContext(this: IExecuteFunctions, index?: number) {
+export function getSandboxContext(this: IExecuteFunctions, index?: number): SandboxContext {
-	const sandboxContext: Record<string, unknown> & {
+	return {
-		$item: (i: number) => IWorkflowDataProxyData;
-		$input: any;
-	} = {
 		// from NodeExecuteFunctions
 		$getNodeParameter: this.getNodeParameter,
 		$getWorkflowStaticData: this.getWorkflowStaticData,
 		helpers: this.helpers,
 
 		// to bring in all $-prefixed vars and methods from WorkflowDataProxy
-		$item: this.getWorkflowDataProxy,
+		...this.getWorkflowDataProxy(index ?? 0),
-		$input: null,
 	};
-
-	// $node, $items(), $parameter, $json, $env, etc.
-	Object.assign(sandboxContext, sandboxContext.$item(index ?? 0));
-
-	return sandboxContext;
 }

packages/nodes-base/nodes/Code/test/Code.node.test.ts

@@ -1,5 +1,136 @@
+import { anyNumber, mock } from 'jest-mock-extended';
+import type { IExecuteFunctions, IWorkflowDataProxyData } from 'n8n-workflow';
+import { NodeHelpers } from 'n8n-workflow';
+import { normalizeItems } from 'n8n-core';
 import { testWorkflows, getWorkflowFilenames } from '../../../test/nodes/Helpers';
+import { Code } from '../Code.node';
+import { Sandbox } from '../Sandbox';
+import { ValidationError } from '../ValidationError';
 
 const workflows = getWorkflowFilenames(__dirname);
 
 describe('Test Code Node', () => testWorkflows(workflows));
+
+describe('Code Node unit test', () => {
+	const node = new Code();
+	const thisArg = mock<IExecuteFunctions>({
+		helpers: { normalizeItems },
+		prepareOutputData: NodeHelpers.prepareOutputData,
+	});
+	const workflowDataProxy = mock<IWorkflowDataProxyData>({ $input: mock() });
+	thisArg.getWorkflowDataProxy.mockReturnValue(workflowDataProxy);
+
+	describe('runOnceForAllItems', () => {
+		beforeEach(() => {
+			thisArg.getNodeParameter.calledWith('mode', 0).mockReturnValueOnce('runOnceForAllItems');
+		});
+
+		describe('valid return data', () => {
+			const tests: Record<string, [object | null, object]> = {
+				'should handle null': [null, []],
+				'should handle pre-normalized result': [
+					[{ json: { count: 42 } }],
+					[{ json: { count: 42 } }],
+				],
+				'should handle when returned data is not an array': [
+					{ json: { count: 42 } },
+					[{ json: { count: 42 } }],
+				],
+				'should handle when returned data is an array with items missing `json` key': [
+					[{ count: 42 }],
+					[{ json: { count: 42 } }],
+				],
+				'should handle when returned data missing `json` key': [
+					{ count: 42 },
+					[{ json: { count: 42 } }],
+				],
+			};
+
+			Object.entries(tests).forEach(([title, [input, expected]]) =>
+				test(title, async () => {
+					jest.spyOn(Sandbox.prototype, 'run').mockResolvedValueOnce(input);
+
+					const output = await node.execute.call(thisArg);
+					expect(output).toEqual([expected]);
+				}),
+			);
+		});
+
+		describe('invalid return data', () => {
+			const tests = {
+				undefined,
+				null: null,
+				date: new Date(),
+				string: 'string',
+				boolean: true,
+				array: [],
+			};
+
+			Object.entries(tests).forEach(([title, returnData]) =>
+				test(`return error if \`.json\` is ${title}`, async () => {
+					jest.spyOn(Sandbox.prototype, 'run').mockResolvedValueOnce([{ json: returnData }]);
+
+					try {
+						await node.execute.call(thisArg);
+						throw new Error("Validation error wasn't thrown");
+					} catch (error) {
+						expect(error).toBeInstanceOf(ValidationError);
+						expect(error.message).toEqual("A 'json' property isn't an object");
+					}
+				}),
+			);
+		});
+	});
+
+	describe('runOnceForEachItem', () => {
+		beforeEach(() => {
+			thisArg.getNodeParameter.calledWith('mode', 0).mockReturnValueOnce('runOnceForEachItem');
+			thisArg.getNodeParameter.calledWith('jsCode', anyNumber()).mockReturnValueOnce('');
+			thisArg.getInputData.mockReturnValueOnce([{ json: {} }]);
+		});
+
+		describe('valid return data', () => {
+			const tests: Record<string, [object | null, { json: any } | null]> = {
+				'should handle pre-normalized result': [{ json: { count: 42 } }, { json: { count: 42 } }],
+				'should handle when returned data missing `json` key': [
+					{ count: 42 },
+					{ json: { count: 42 } },
+				],
+			};
+
+			Object.entries(tests).forEach(([title, [input, expected]]) =>
+				test(title, async () => {
+					jest.spyOn(Sandbox.prototype, 'run').mockResolvedValueOnce(input);
+
+					const output = await node.execute.call(thisArg);
+					expect(output).toEqual([[{ json: expected?.json, pairedItem: { item: 0 } }]]);
+				}),
+			);
+		});
+
+		describe('invalid return data', () => {
+			const tests = {
+				undefined,
+				null: null,
+				date: new Date(),
+				string: 'string',
+				boolean: true,
+				array: [],
+			};
+
+			Object.entries(tests).forEach(([title, returnData]) =>
+				test(`return error if \`.json\` is ${title}`, async () => {
+					jest.spyOn(Sandbox.prototype, 'run').mockResolvedValueOnce({ json: returnData });
+
+					try {
+						await node.execute.call(thisArg);
+						throw new Error("Validation error wasn't thrown");
+					} catch (error) {
+						expect(error).toBeInstanceOf(ValidationError);
+						expect(error.message).toEqual("A 'json' property isn't an object [item 0]");
+					}
+				}),
+			);
+		});
+	});
+});

packages/nodes-base/nodes/Code/utils.ts

@@ -1,7 +1,9 @@
 import type { IDataObject } from 'n8n-workflow';
 
 export function isObject(maybe: unknown): maybe is { [key: string]: unknown } {
-	return typeof maybe === 'object' && maybe !== null && !Array.isArray(maybe);
+	return (
+		typeof maybe === 'object' && maybe !== null && !Array.isArray(maybe) && !(maybe instanceof Date)
+	);
 }
 
 function isTraversable(maybe: unknown): maybe is IDataObject {
@@ -13,7 +15,6 @@ function isTraversable(maybe: unknown): maybe is IDataObject {
  */
 export function standardizeOutput(output: IDataObject) {
 	function standardizeOutputRecursive(obj: IDataObject, knownObjects = new WeakSet()): IDataObject {
-		if (obj === undefined || obj === null) return obj;
 		for (const [key, value] of Object.entries(obj)) {
 			if (!isTraversable(value)) continue;
 

packages/workflow/src/Interfaces.ts

@@ -1462,21 +1462,30 @@ export interface IWebhookDescription {
 	restartWebhook?: boolean;
 }
 
+export interface ProxyInput {
+	all: () => INodeExecutionData[];
+	context: any;
+	first: () => INodeExecutionData | undefined;
+	item: INodeExecutionData | undefined;
+	last: () => INodeExecutionData | undefined;
+	params?: INodeParameters;
+}
+
 export interface IWorkflowDataProxyData {
 	[key: string]: any;
-	$binary: any;
+	$binary: INodeExecutionData['binary'];
 	$data: any;
 	$env: any;
-	$evaluateExpression: any;
+	$evaluateExpression: (expression: string, itemIndex?: number) => NodeParameterValueType;
-	$item: any;
+	$item: (itemIndex: number, runIndex?: number) => IWorkflowDataProxyData;
-	$items: any;
+	$items: (nodeName?: string, outputIndex?: number, runIndex?: number) => INodeExecutionData[];
-	$json: any;
+	$json: INodeExecutionData['json'];
 	$node: any;
-	$parameter: any;
+	$parameter: INodeParameters;
-	$position: any;
+	$position: number;
 	$workflow: any;
 	$: any;
-	$input: any;
+	$input: ProxyInput;
 	$thisItem: any;
 	$thisRunIndex: number;
 	$thisItemIndex: number;

packages/workflow/src/WorkflowDataProxy.ts

@@ -24,6 +24,7 @@ import type {
 	INodeParameterResourceLocator,
 	NodeParameterValueType,
 	WorkflowExecuteMode,
+	ProxyInput,
 } from './Interfaces';
 import * as NodeHelpers from './NodeHelpers';
 import { ExpressionError } from './ExpressionError';
@@ -1067,90 +1068,87 @@ export class WorkflowDataProxy {
 				);
 			},
 
-			$input: new Proxy(
+			$input: new Proxy({} as ProxyInput, {
-				{},
+				ownKeys(target) {
-				{
+					return ['all', 'context', 'first', 'item', 'last', 'params'];
-					ownKeys(target) {
-						return ['all', 'context', 'first', 'item', 'last', 'params'];
-					},
-					getOwnPropertyDescriptor(k) {
-						return {
-							enumerable: true,
-							configurable: true,
-						};
-					},
-					get(target, property, receiver) {
-						if (property === 'isProxy') return true;
-
-						if (property === 'item') {
-							return that.connectionInputData[that.itemIndex];
-						}
-						if (property === 'first') {
-							return (...args: unknown[]) => {
-								if (args.length) {
-									throw createExpressionError('$input.first() should have no arguments');
-								}
-
-								const result = that.connectionInputData;
-								if (result[0]) {
-									return result[0];
-								}
-								return undefined;
-							};
-						}
-						if (property === 'last') {
-							return (...args: unknown[]) => {
-								if (args.length) {
-									throw createExpressionError('$input.last() should have no arguments');
-								}
-
-								const result = that.connectionInputData;
-								if (result.length && result[result.length - 1]) {
-									return result[result.length - 1];
-								}
-								return undefined;
-							};
-						}
-						if (property === 'all') {
-							return () => {
-								const result = that.connectionInputData;
-								if (result.length) {
-									return result;
-								}
-								return [];
-							};
-						}
-
-						if (['context', 'params'].includes(property as string)) {
-							// For the following properties we need the source data so fail in case it is missing
-							// for some reason (even though that should actually never happen)
-							if (!that.executeData?.source) {
-								throw createExpressionError('Can’t get data for expression', {
-									messageTemplate: 'Can’t get data for expression under ‘%%PARAMETER%%’ field',
-									functionOverrides: {
-										message: 'Can’t get data',
-									},
-									description:
-										'Apologies, this is an internal error. See details for more information',
-									causeDetailed: 'Missing sourceData (probably an internal error)',
-									runIndex: that.runIndex,
-								});
-							}
-
-							const sourceData: ISourceData = that.executeData.source.main[0] as ISourceData;
-
-							if (property === 'context') {
-								return that.nodeContextGetter(sourceData.previousNode);
-							}
-							if (property === 'params') {
-								return that.workflow.getNode(sourceData.previousNode)?.parameters;
-							}
-						}
-
-						return Reflect.get(target, property, receiver);
-					},
 				},
-			),
+				getOwnPropertyDescriptor(k) {
+					return {
+						enumerable: true,
+						configurable: true,
+					};
+				},
+				get(target, property, receiver) {
+					if (property === 'isProxy') return true;
+
+					if (property === 'item') {
+						return that.connectionInputData[that.itemIndex];
+					}
+					if (property === 'first') {
+						return (...args: unknown[]) => {
+							if (args.length) {
+								throw createExpressionError('$input.first() should have no arguments');
+							}
+
+							const result = that.connectionInputData;
+							if (result[0]) {
+								return result[0];
+							}
+							return undefined;
+						};
+					}
+					if (property === 'last') {
+						return (...args: unknown[]) => {
+							if (args.length) {
+								throw createExpressionError('$input.last() should have no arguments');
+							}
+
+							const result = that.connectionInputData;
+							if (result.length && result[result.length - 1]) {
+								return result[result.length - 1];
+							}
+							return undefined;
+						};
+					}
+					if (property === 'all') {
+						return () => {
+							const result = that.connectionInputData;
+							if (result.length) {
+								return result;
+							}
+							return [];
+						};
+					}
+
+					if (['context', 'params'].includes(property as string)) {
+						// For the following properties we need the source data so fail in case it is missing
+						// for some reason (even though that should actually never happen)
+						if (!that.executeData?.source) {
+							throw createExpressionError('Can’t get data for expression', {
+								messageTemplate: 'Can’t get data for expression under ‘%%PARAMETER%%’ field',
+								functionOverrides: {
+									message: 'Can’t get data',
+								},
+								description:
+									'Apologies, this is an internal error. See details for more information',
+								causeDetailed: 'Missing sourceData (probably an internal error)',
+								runIndex: that.runIndex,
+							});
+						}
+
+						const sourceData: ISourceData = that.executeData.source.main[0] as ISourceData;
+
+						if (property === 'context') {
+							return that.nodeContextGetter(sourceData.previousNode);
+						}
+						if (property === 'params') {
+							return that.workflow.getNode(sourceData.previousNode)?.parameters;
+						}
+					}
+
+					return Reflect.get(target, property, receiver);
+				},
+			}),
 
 			$binary: {}, // Placeholder
 			$data: {}, // Placeholder