From f9b3aeac44520d0eb65dea5934ac6572bc1e3fbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Wed, 19 Apr 2023 13:09:46 +0200 Subject: [PATCH] refactor(Code Node): Constently handle various kinds of data returned by user code (#6002) --- packages/nodes-base/nodes/Code/Code.node.ts | 28 +-- packages/nodes-base/nodes/Code/Sandbox.ts | 160 ++++++------------ .../nodes/Code/test/Code.node.test.ts | 131 ++++++++++++++ packages/nodes-base/nodes/Code/utils.ts | 5 +- packages/workflow/src/Interfaces.ts | 25 ++- packages/workflow/src/WorkflowDataProxy.ts | 148 ++++++++-------- 6 files changed, 293 insertions(+), 204 deletions(-) diff --git a/packages/nodes-base/nodes/Code/Code.node.ts b/packages/nodes-base/nodes/Code/Code.node.ts index 4d6b647c822b0..ac64931d25fc7 100644 --- a/packages/nodes-base/nodes/Code/Code.node.ts +++ b/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++) { - let item = items[index]; + const items = this.getInputData(); + 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 }), }); } } diff --git a/packages/nodes-base/nodes/Code/Sandbox.ts b/packages/nodes-base/nodes/Code/Sandbox.ts index 9cb72c1bbb37c..9fd9e9fd522df 100644 --- a/packages/nodes-base/nodes/Code/Sandbox.ts +++ b/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 { - private jsCode = ''; +interface SandboxContext extends IWorkflowDataProxyData { + $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, + context: SandboxContext, + private jsCode: string, workflowMode: WorkflowExecuteMode, - private nodeMode: CodeNodeMode, private helpers: IExecuteFunctions['helpers'], ) { - super(Sandbox.getSandboxOptions(context, workflowMode)); - } - - static getSandboxOptions( - context: ReturnType, - workflowMode: WorkflowExecuteMode, - ): NodeVMOptions { - const { NODE_FUNCTION_ALLOW_BUILTIN: builtIn, NODE_FUNCTION_ALLOW_EXTERNAL: external } = - process.env; - - return { + super({ 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) { - this.jsCode = jsCode; - this.itemIndex = itemIndex; - - return this.nodeMode === 'runOnceForAllItems' ? this.runCodeAllItems() : this.runCodeEachItem(); + }); } - private async runCodeAllItems() { + async runCodeAllItems(): Promise { 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) => { - 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, - }); + this.validateTopLevelKeys(item); } } - } 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 { + this.itemIndex = itemIndex; const script = `module.exports = async function() {${this.jsCode}\n}()`; const match = this.jsCode.match(/\$input\.(?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,7 +143,32 @@ export class Sandbox extends NodeVM { }); } - if (executionResult.json !== undefined && !isObject(executionResult.json)) { + if (Array.isArray(executionResult)) { + const firstSentence = + executionResult.length > 0 + ? `An array of ${typeof executionResult[0]}s was returned.` + : 'An empty array was returned.'; + + throw new ValidationError({ + message: "Code doesn't return a single object", + description: `${firstSentence} If you need to output multiple items, please use the 'Run Once for All Items' mode instead`, + itemIndex: this.itemIndex, + }); + } + + 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", @@ -199,62 +176,35 @@ export class Sandbox extends NodeVM { }); } - if (executionResult.binary !== undefined && !isObject(executionResult.binary)) { + 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, }); } + } - // 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) => { + 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, }); }); - - if (Array.isArray(executionResult)) { - const firstSentence = - executionResult.length > 0 - ? `An array of ${typeof executionResult[0]}s was returned.` - : 'An empty array was returned.'; - - throw new ValidationError({ - message: "Code doesn't return a single object", - description: `${firstSentence} If you need to output multiple items, please use the 'Run Once for All Items' mode instead`, - itemIndex: this.itemIndex, - }); - } - - return executionResult.json ? executionResult : { json: executionResult }; } } -export function getSandboxContext(this: IExecuteFunctions, index?: number) { - const sandboxContext: Record & { - $item: (i: number) => IWorkflowDataProxyData; - $input: any; - } = { +export function getSandboxContext(this: IExecuteFunctions, index?: number): SandboxContext { + return { // from NodeExecuteFunctions $getNodeParameter: this.getNodeParameter, $getWorkflowStaticData: this.getWorkflowStaticData, helpers: this.helpers, // to bring in all $-prefixed vars and methods from WorkflowDataProxy - $item: this.getWorkflowDataProxy, - $input: null, + ...this.getWorkflowDataProxy(index ?? 0), }; - - // $node, $items(), $parameter, $json, $env, etc. - Object.assign(sandboxContext, sandboxContext.$item(index ?? 0)); - - return sandboxContext; } diff --git a/packages/nodes-base/nodes/Code/test/Code.node.test.ts b/packages/nodes-base/nodes/Code/test/Code.node.test.ts index e232d6513da8b..f21b3c42e6e51 100644 --- a/packages/nodes-base/nodes/Code/test/Code.node.test.ts +++ b/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({ + helpers: { normalizeItems }, + prepareOutputData: NodeHelpers.prepareOutputData, + }); + const workflowDataProxy = mock({ $input: mock() }); + thisArg.getWorkflowDataProxy.mockReturnValue(workflowDataProxy); + + describe('runOnceForAllItems', () => { + beforeEach(() => { + thisArg.getNodeParameter.calledWith('mode', 0).mockReturnValueOnce('runOnceForAllItems'); + }); + + describe('valid return data', () => { + const tests: Record = { + '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 = { + '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]"); + } + }), + ); + }); + }); +}); diff --git a/packages/nodes-base/nodes/Code/utils.ts b/packages/nodes-base/nodes/Code/utils.ts index 3899a8be769ce..891768f6ffd89 100644 --- a/packages/nodes-base/nodes/Code/utils.ts +++ b/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; diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index f4f89d30edc7f..e18299e79f7cc 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/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; - $item: any; - $items: any; - $json: any; + $evaluateExpression: (expression: string, itemIndex?: number) => NodeParameterValueType; + $item: (itemIndex: number, runIndex?: number) => IWorkflowDataProxyData; + $items: (nodeName?: string, outputIndex?: number, runIndex?: number) => INodeExecutionData[]; + $json: INodeExecutionData['json']; $node: any; - $parameter: any; - $position: any; + $parameter: INodeParameters; + $position: number; $workflow: any; $: any; - $input: any; + $input: ProxyInput; $thisItem: any; $thisRunIndex: number; $thisItemIndex: number; diff --git a/packages/workflow/src/WorkflowDataProxy.ts b/packages/workflow/src/WorkflowDataProxy.ts index 48567826ae58d..c6466ae41e8a1 100644 --- a/packages/workflow/src/WorkflowDataProxy.ts +++ b/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( - {}, - { - 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 []; - }; - } + $input: new Proxy({} as ProxyInput, { + 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 (['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, - }); + 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 sourceData: ISourceData = that.executeData.source.main[0] as ISourceData; + 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'); + } - if (property === 'context') { - return that.nodeContextGetter(sourceData.previousNode); + const result = that.connectionInputData; + if (result.length && result[result.length - 1]) { + return result[result.length - 1]; } - if (property === 'params') { - return that.workflow.getNode(sourceData.previousNode)?.parameters; + 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, + }); } - return Reflect.get(target, property, receiver); - }, + 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