From 0860fbe97108edc21bc01dec3b6ef13e60e728d4 Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Tue, 31 Dec 2024 09:19:36 +0200 Subject: [PATCH] fix(core): Don't fail task runner task if logging fails (#12401) --- .../__tests__/js-task-runner.test.ts | 41 +++++++- .../src/js-task-runner/js-task-runner.ts | 98 +++++++++++-------- packages/@n8n/task-runner/src/task-runner.ts | 16 +-- 3 files changed, 105 insertions(+), 50 deletions(-) diff --git a/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts b/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts index e93ff10dc4..00715621b2 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts @@ -139,7 +139,8 @@ describe('JsTaskRunner', () => { }); expect(defaultTaskRunner.makeRpcCall).toHaveBeenCalledWith(task.taskId, 'logNodeOutput', [ - 'Hello world!', + "'Hello'", + "'world!'", ]); }, ); @@ -173,6 +174,44 @@ describe('JsTaskRunner', () => { }), ).resolves.toBeDefined(); }); + + it('should not throw when trying to log the context object', async () => { + const task = newTaskWithSettings({ + code: ` + console.log(this); + return {json: {}} + `, + nodeMode: 'runOnceForAllItems', + }); + + await expect( + execTaskWithParams({ + task, + taskData: newDataRequestResponse([wrapIntoJson({})]), + }), + ).resolves.toBeDefined(); + }); + + it('should log the context object as [[ExecutionContext]]', async () => { + const rpcCallSpy = jest.spyOn(defaultTaskRunner, 'makeRpcCall').mockResolvedValue(undefined); + + const task = newTaskWithSettings({ + code: ` + console.log(this); + return {json: {}} + `, + nodeMode: 'runOnceForAllItems', + }); + + await execTaskWithParams({ + task, + taskData: newDataRequestResponse([wrapIntoJson({})]), + }); + + expect(rpcCallSpy).toHaveBeenCalledWith(task.taskId, 'logNodeOutput', [ + '[[ExecutionContext]]', + ]); + }); }); describe('built-in methods and variables available in the context', () => { diff --git a/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts b/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts index 802b78a640..db93f3c338 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts @@ -15,8 +15,10 @@ import type { EnvProviderState, IExecuteData, INodeTypeDescription, + IWorkflowDataProxyData, } from 'n8n-workflow'; import * as a from 'node:assert'; +import { inspect } from 'node:util'; import { runInNewContext, type Context } from 'node:vm'; import type { MainConfig } from '@/config/main-config'; @@ -79,6 +81,8 @@ type CustomConsole = { log: (...args: unknown[]) => void; }; +const noOp = () => {}; + export class JsTaskRunner extends TaskRunner { private readonly requireResolver: RequireResolver; @@ -129,29 +133,12 @@ export class JsTaskRunner extends TaskRunner { nodeTypes: this.nodeTypes, }); - const noOp = () => {}; - const customConsole = { - // all except `log` are dummy methods that disregard without throwing, following existing Code node behavior - ...Object.keys(console).reduce void>>((acc, name) => { - acc[name] = noOp; - return acc; - }, {}), - // Send log output back to the main process. It will take care of forwarding - // it to the UI or printing to console. - log: (...args: unknown[]) => { - const logOutput = args - .map((arg) => (typeof arg === 'object' && arg !== null ? JSON.stringify(arg) : arg)) - .join(' '); - void this.makeRpcCall(task.taskId, 'logNodeOutput', [logOutput]); - }, - }; - workflow.staticData = ObservableObject.create(workflow.staticData); const result = settings.nodeMode === 'runOnceForAllItems' - ? await this.runForAllItems(task.taskId, settings, data, workflow, customConsole, signal) - : await this.runForEachItem(task.taskId, settings, data, workflow, customConsole, signal); + ? await this.runForAllItems(task.taskId, settings, data, workflow, signal) + : await this.runForEachItem(task.taskId, settings, data, workflow, signal); return { result, @@ -200,22 +187,14 @@ export class JsTaskRunner extends TaskRunner { settings: JSExecSettings, data: JsTaskData, workflow: Workflow, - customConsole: CustomConsole, signal: AbortSignal, ): Promise { const dataProxy = this.createDataProxy(data, workflow, data.itemIndex); const inputItems = data.connectionInputData; - const context: Context = { - require: this.requireResolver, - module: {}, - console: customConsole, + const context = this.buildContext(taskId, workflow, data.node, dataProxy, { items: inputItems, - $getWorkflowStaticData: (type: 'global' | 'node') => workflow.getStaticData(type, data.node), - ...this.getNativeVariables(), - ...dataProxy, - ...this.buildRpcCallObject(taskId), - }; + }); try { const result = await new Promise((resolve, reject) => { @@ -264,7 +243,6 @@ export class JsTaskRunner extends TaskRunner { settings: JSExecSettings, data: JsTaskData, workflow: Workflow, - customConsole: CustomConsole, signal: AbortSignal, ): Promise { const inputItems = data.connectionInputData; @@ -279,17 +257,7 @@ export class JsTaskRunner extends TaskRunner { for (let index = chunkStartIdx; index < chunkEndIdx; index++) { const item = inputItems[index]; const dataProxy = this.createDataProxy(data, workflow, index); - const context: Context = { - require: this.requireResolver, - module: {}, - console: customConsole, - item, - $getWorkflowStaticData: (type: 'global' | 'node') => - workflow.getStaticData(type, data.node), - ...this.getNativeVariables(), - ...dataProxy, - ...this.buildRpcCallObject(taskId), - }; + const context = this.buildContext(taskId, workflow, data.node, dataProxy, { item }); try { let result = await new Promise((resolve, reject) => { @@ -467,4 +435,52 @@ export class JsTaskRunner extends TaskRunner { return rpcObject; } + + private buildCustomConsole(taskId: string): CustomConsole { + return { + // all except `log` are dummy methods that disregard without throwing, following existing Code node behavior + ...Object.keys(console).reduce void>>((acc, name) => { + acc[name] = noOp; + return acc; + }, {}), + + // Send log output back to the main process. It will take care of forwarding + // it to the UI or printing to console. + log: (...args: unknown[]) => { + const formattedLogArgs = args.map((arg) => inspect(arg)); + void this.makeRpcCall(taskId, 'logNodeOutput', formattedLogArgs); + }, + }; + } + + /** + * Builds the 'global' context object that is passed to the script + * + * @param taskId The ID of the task. Needed for RPC calls + * @param workflow The workflow that is being executed. Needed for static data + * @param node The node that is being executed. Needed for static data + * @param dataProxy The data proxy object that provides access to built-ins + * @param additionalProperties Additional properties to add to the context + */ + private buildContext( + taskId: string, + workflow: Workflow, + node: INode, + dataProxy: IWorkflowDataProxyData, + additionalProperties: Record = {}, + ): Context { + const context: Context = { + [inspect.custom]: () => '[[ExecutionContext]]', + require: this.requireResolver, + module: {}, + console: this.buildCustomConsole(taskId), + $getWorkflowStaticData: (type: 'global' | 'node') => workflow.getStaticData(type, node), + ...this.getNativeVariables(), + ...dataProxy, + ...this.buildRpcCallObject(taskId), + ...additionalProperties, + }; + + return context; + } } diff --git a/packages/@n8n/task-runner/src/task-runner.ts b/packages/@n8n/task-runner/src/task-runner.ts index 4254aad99c..d9e288eaab 100644 --- a/packages/@n8n/task-runner/src/task-runner.ts +++ b/packages/@n8n/task-runner/src/task-runner.ts @@ -452,15 +452,15 @@ export abstract class TaskRunner extends EventEmitter { }); }); - this.send({ - type: 'runner:rpc', - callId, - taskId, - name, - params, - }); - try { + this.send({ + type: 'runner:rpc', + callId, + taskId, + name, + params, + }); + const returnValue = await dataPromise; return isSerializedBuffer(returnValue) ? toBuffer(returnValue) : returnValue;