From fbaee9ac02a98a9d3e0c312979861fad473b4a8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Tue, 2 Sep 2025 16:05:10 +0200 Subject: [PATCH] refactor(core): Consolidate task result validation in Code node (#18928) --- .../__tests__/js-task-runner.test.ts | 96 +++------- .../__tests__/result-validation.test.ts | 119 ------------ .../js-task-runner/errors/validation-error.ts | 44 ----- .../src/js-task-runner/js-task-runner.ts | 19 +- .../src/js-task-runner/result-validation.ts | 148 --------------- packages/@n8n/task-runner/src/runner-types.ts | 3 +- ...-task-runner-execution.integration.test.ts | 16 +- .../nodes/Code/JsTaskRunnerSandbox.ts | 29 ++- packages/nodes-base/nodes/Code/Sandbox.ts | 165 ++--------------- .../nodes/Code/reserved-key-found-error.ts} | 2 +- .../nodes/Code/result-validation.ts | 174 ++++++++++++++++++ .../Code/test/JsTaskRunnerSandbox.test.ts | 7 + 12 files changed, 269 insertions(+), 553 deletions(-) delete mode 100644 packages/@n8n/task-runner/src/js-task-runner/__tests__/result-validation.test.ts delete mode 100644 packages/@n8n/task-runner/src/js-task-runner/errors/validation-error.ts delete mode 100644 packages/@n8n/task-runner/src/js-task-runner/result-validation.ts rename packages/{@n8n/task-runner/src/js-task-runner/errors/reserved-key-not-found.error.ts => nodes-base/nodes/Code/reserved-key-found-error.ts} (89%) create mode 100644 packages/nodes-base/nodes/Code/result-validation.ts 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 970d08dab8..a7c44fcb71 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 @@ -13,7 +13,6 @@ import type { JsRunnerConfig } from '@/config/js-runner-config'; import { MainConfig } from '@/config/main-config'; import { ExecutionError } from '@/js-task-runner/errors/execution-error'; import { UnsupportedFunctionError } from '@/js-task-runner/errors/unsupported-function.error'; -import { ValidationError } from '@/js-task-runner/errors/validation-error'; import type { JSExecSettings } from '@/js-task-runner/js-task-runner'; import { JsTaskRunner } from '@/js-task-runner/js-task-runner'; import { @@ -30,7 +29,6 @@ import { withPairedItem, wrapIntoJson, } from './test-data'; -import { ReservedKeyFoundError } from '../errors/reserved-key-not-found.error'; jest.mock('ws'); @@ -237,7 +235,7 @@ describe('JsTaskRunner', () => { inputItems, }); - expect(outcome.result).toEqual([wrapIntoJson(needsWrapping ? { val: expected } : expected)]); + expect(outcome.result).toEqual(needsWrapping ? { val: expected } : expected); }; const testExpressionForEachItem = async ( @@ -250,22 +248,28 @@ describe('JsTaskRunner', () => { inputItems, }); - expect(outcome.result).toEqual([ - withPairedItem(0, wrapIntoJson(needsWrapping ? { val: expected } : expected)), - ]); + // if expected has json property, use it, else use entire result + const expectedJson = + expected !== null && typeof expected === 'object' && 'json' in expected + ? expected.json + : needsWrapping + ? { val: expected } + : expected; + + expect(outcome.result).toEqual([{ json: expectedJson, pairedItem: { item: 0 } }]); }; const testGroups = { // https://docs.n8n.io/code/builtin/current-node-input/ 'current node input': [ - ['$input.first()', inputItems[0]], - ['$input.last()', inputItems[inputItems.length - 1]], + ['$input.first()', { json: inputItems[0] }], + ['$input.last()', { json: inputItems[inputItems.length - 1] }], ['$input.params', { manualTriggerParam: 'empty' }], ], // https://docs.n8n.io/code/builtin/output-other-nodes/ 'output of other nodes': [ - ['$("Trigger").first()', inputItems[0]], - ['$("Trigger").last()', inputItems[inputItems.length - 1]], + ['$("Trigger").first()', { json: inputItems[0] }], + ['$("Trigger").last()', { json: inputItems[inputItems.length - 1] }], ['$("Trigger").params', { manualTriggerParam: 'empty' }], ], // https://docs.n8n.io/code/builtin/date-time/ @@ -360,7 +364,7 @@ describe('JsTaskRunner', () => { }), }); - expect(outcome.result).toEqual([wrapIntoJson({ val: 'value' })]); + expect(outcome.result).toEqual({ val: 'value' }); }); it('should be possible to access env if it has been blocked', async () => { @@ -411,7 +415,7 @@ describe('JsTaskRunner', () => { }), }); - expect(outcome.result).toEqual([wrapIntoJson({ val: undefined })]); + expect(outcome.result).toEqual({ val: undefined }); }); }); @@ -431,7 +435,7 @@ describe('JsTaskRunner', () => { }); const helsinkiTimeNow = DateTime.now().setZone('Europe/Helsinki').toSeconds(); - expect(outcome.result[0].json.val).toBeCloseTo(helsinkiTimeNow, 1); + expect(outcome.result).toEqual({ val: expect.closeTo(helsinkiTimeNow, 1) }); }); it('should use the default timezone', async () => { @@ -448,7 +452,7 @@ describe('JsTaskRunner', () => { }); const helsinkiTimeNow = DateTime.now().setZone('Europe/Helsinki').toSeconds(); - expect(outcome.result[0].json.val).toBeCloseTo(helsinkiTimeNow, 1); + expect(outcome.result).toEqual({ val: expect.closeTo(helsinkiTimeNow, 1) }); }); }); @@ -466,7 +470,7 @@ describe('JsTaskRunner', () => { }), }); - expect(outcome.result).toEqual([wrapIntoJson({ val: { key: 'value' } })]); + expect(outcome.result).toEqual({ val: { key: 'value' } }); }); it('should have the global workflow static data available in runOnceForEachItem', async () => { @@ -559,7 +563,7 @@ describe('JsTaskRunner', () => { taskData: createTaskDataWithNodeStaticData({ key: 'value' }), }); - expect(outcome.result).toEqual([wrapIntoJson({ val: { key: 'value' } })]); + expect(outcome.result).toEqual({ val: { key: 'value' } }); }); it('should have the node workflow static data available in runOnceForEachItem', async () => { @@ -745,7 +749,7 @@ describe('JsTaskRunner', () => { }), }); - expect(outcomeAll.result).toEqual([wrapIntoJson({ val: 'test-buffer' })]); + expect(outcomeAll.result).toEqual({ val: 'test-buffer' }); const outcomePer = await execTaskWithParams({ task: newTaskParamsWithSettings({ @@ -789,38 +793,6 @@ describe('JsTaskRunner', () => { }); }); - describe('invalid output', () => { - test.each([['undefined'], ['42'], ['"a string"']])( - 'should throw a ValidationError if the code output is %s', - async (output) => { - await expect( - executeForAllItems({ - code: `return ${output}`, - inputItems: [{ a: 1 }], - }), - ).rejects.toThrow(ValidationError); - }, - ); - - it('should throw a ValidationError if some items are wrapped in json and some are not', async () => { - await expect( - executeForAllItems({ - code: 'return [{b: 1}, {json: {b: 2}}]', - inputItems: [{ a: 1 }], - }), - ).rejects.toThrow(ValidationError); - }); - - it('should throw a ReservedKeyFoundError if there are unknown keys alongside reserved keys', async () => { - await expect( - executeForAllItems({ - code: 'return [{json: {b: 1}, objectId: "123"}]', - inputItems: [{ a: 1 }], - }), - ).rejects.toThrow(ReservedKeyFoundError); - }); - }); - it('should return static items', async () => { const outcome = await executeForAllItems({ code: 'return [{json: {b: 1}}]', @@ -852,19 +824,19 @@ describe('JsTaskRunner', () => { }); expect(outcome).toEqual({ - result: [wrapIntoJson({ b: 1 })], + result: [{ b: 1 }], customData: undefined, }); }); - it('should wrap single item into an array and json', async () => { + it('should not wrap single item into an array and json', async () => { const outcome = await executeForAllItems({ code: 'return {b: 1}', inputItems: [{ a: 1 }], }); expect(outcome).toEqual({ - result: [wrapIntoJson({ b: 1 })], + result: { b: 1 }, customData: undefined, }); }); @@ -914,20 +886,6 @@ describe('JsTaskRunner', () => { }); }); - describe('invalid output', () => { - test.each([['undefined'], ['42'], ['"a string"'], ['[]'], ['[1,2,3]']])( - 'should throw a ValidationError if the code output is %s', - async (output) => { - await expect( - executeForEachItem({ - code: `return ${output}`, - inputItems: [{ a: 1 }], - }), - ).rejects.toThrow(ValidationError); - }, - ); - }); - describe('chunked execution', () => { it('should use correct index for each item', async () => { const outcome = await executeForEachItem({ @@ -1112,7 +1070,7 @@ describe('JsTaskRunner', () => { runner, }); - expect(outcome.result).toEqual([wrapIntoJson({ val: expected })]); + expect(outcome.result).toEqual({ val: expected }); }, ); @@ -1174,7 +1132,7 @@ describe('JsTaskRunner', () => { runner, }); - expect(outcome.result).toEqual([wrapIntoJson({ val: expected })]); + expect(outcome.result).toEqual({ val: expected }); }, ); @@ -1526,7 +1484,7 @@ describe('JsTaskRunner', () => { inputItems: [], }); - expect(outcome.result).toEqual([wrapIntoJson({ val: 2 })]); + expect(outcome.result).toEqual({ val: 2 }); }); }); }); diff --git a/packages/@n8n/task-runner/src/js-task-runner/__tests__/result-validation.test.ts b/packages/@n8n/task-runner/src/js-task-runner/__tests__/result-validation.test.ts deleted file mode 100644 index 65587280c3..0000000000 --- a/packages/@n8n/task-runner/src/js-task-runner/__tests__/result-validation.test.ts +++ /dev/null @@ -1,119 +0,0 @@ -import { ValidationError } from '@/js-task-runner/errors/validation-error'; -import { - NonArrayOfObjectsError, - validateRunForAllItemsOutput, - validateRunForEachItemOutput, -} from '@/js-task-runner/result-validation'; - -describe('result validation', () => { - describe('validateRunForAllItemsOutput', () => { - it('should throw an error if the output is not an object', () => { - expect(() => { - validateRunForAllItemsOutput(undefined); - }).toThrowError(ValidationError); - }); - - it('should throw an error if the output is an array and at least one item has a non-n8n key', () => { - expect(() => { - validateRunForAllItemsOutput([{ json: {} }, { json: {}, unknownKey: {} }]); - }).toThrowError(ValidationError); - }); - - it('should not throw an error if the output is an array and all items are json wrapped', () => { - expect(() => { - validateRunForAllItemsOutput([{ json: {} }, { json: {} }, { json: {} }]); - }).not.toThrow(); - }); - - it('should throw a NonArrayOfObjectsError if the output is an array of arrays (empty)', () => { - expect(() => { - // @ts-expect-error Intentionally invalid - validateRunForAllItemsOutput([[]]); - }).toThrowError(NonArrayOfObjectsError); - }); - - test.each([ - ['binary', {}], - ['pairedItem', {}], - ['error', {}], - ['index', {}], // temporarily allowed until refactored out - ])( - 'should not throw an error if the output item has %s key in addition to json', - (key, value) => { - expect(() => { - validateRunForAllItemsOutput([{ json: {} }, { json: {}, [key]: value }]); - }).not.toThrow(); - }, - ); - - it('should not throw an error if the output is an array and all items are not json wrapped', () => { - expect(() => { - validateRunForAllItemsOutput([ - { - id: 1, - name: 'test3', - }, - { - id: 2, - name: 'test4', - }, - { - id: 3, - name: 'test5', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ] as any); - }).not.toThrow(); - }); - - it('should throw if json is not an object', () => { - expect(() => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - validateRunForAllItemsOutput([{ json: 1 } as any]); - }).toThrowError(ValidationError); - }); - }); - - describe('validateRunForEachItemOutput', () => { - const index = 0; - - it('should throw an error if the output is not an object', () => { - expect(() => { - validateRunForEachItemOutput(undefined, index); - }).toThrowError(ValidationError); - }); - - it('should throw an error if the output is an array', () => { - expect(() => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - validateRunForEachItemOutput([] as any, index); - }).toThrowError(ValidationError); - }); - - it('should throw if json is not an object', () => { - expect(() => { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - validateRunForEachItemOutput({ json: 1 } as any, index); - }).toThrowError(ValidationError); - }); - - it('should throw an error if the output is an array and at least one item has a non-n8n key', () => { - expect(() => { - validateRunForEachItemOutput({ json: {}, unknownKey: {} }, index); - }).toThrowError(ValidationError); - }); - - test.each([ - ['binary', {}], - ['pairedItem', {}], - ['error', {}], - ])( - 'should not throw an error if the output item has %s key in addition to json', - (key, value) => { - expect(() => { - validateRunForEachItemOutput({ json: {}, [key]: value }, index); - }).not.toThrow(); - }, - ); - }); -}); diff --git a/packages/@n8n/task-runner/src/js-task-runner/errors/validation-error.ts b/packages/@n8n/task-runner/src/js-task-runner/errors/validation-error.ts deleted file mode 100644 index bf66136ccf..0000000000 --- a/packages/@n8n/task-runner/src/js-task-runner/errors/validation-error.ts +++ /dev/null @@ -1,44 +0,0 @@ -import { SerializableError } from './serializable-error'; - -export class ValidationError extends SerializableError { - description = ''; - - itemIndex: number | undefined = undefined; - - context: { itemIndex: number } | undefined = undefined; - - lineNumber: number | undefined = undefined; - - constructor({ - message, - description, - itemIndex, - lineNumber, - }: { - message: string; - description: string; - itemIndex?: number; - lineNumber?: number; - }) { - super(message); - - this.lineNumber = lineNumber; - this.itemIndex = itemIndex; - - if (this.lineNumber !== undefined && this.itemIndex !== undefined) { - this.message = `${message} [line ${lineNumber}, for item ${itemIndex}]`; - } else if (this.lineNumber !== undefined) { - this.message = `${message} [line ${lineNumber}]`; - } else if (this.itemIndex !== undefined) { - this.message = `${message} [item ${itemIndex}]`; - } else { - this.message = message; - } - - this.description = description; - - if (this.itemIndex !== undefined) { - this.context = { itemIndex: this.itemIndex }; - } - } -} 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 017a20eb40..a15ac5727b 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 @@ -48,7 +48,6 @@ import { makeSerializable } from './errors/serializable-error'; import { TimeoutError } from './errors/timeout-error'; import type { RequireResolver } from './require-resolver'; import { createRequireResolver } from './require-resolver'; -import { validateRunForAllItemsOutput, validateRunForEachItemOutput } from './result-validation'; import { DataRequestResponseReconstruct } from '../data-request/data-request-response-reconstruct'; export interface RpcCallObject { @@ -240,7 +239,7 @@ export class JsTaskRunner extends TaskRunner { data: JsTaskData, workflow: Workflow, signal: AbortSignal, - ): Promise { + ): Promise { const dataProxy = this.createDataProxy(data, workflow, data.itemIndex); const inputItems = data.connectionInputData; @@ -278,7 +277,7 @@ export class JsTaskRunner extends TaskRunner { return []; } - return validateRunForAllItemsOutput(result); + return result; } catch (e) { // Errors thrown by the VM are not instances of Error, so map them to an ExecutionError const error = this.toExecutionErrorIfNeeded(e); @@ -318,7 +317,7 @@ export class JsTaskRunner extends TaskRunner { Object.assign(context, dataProxy, { item: inputItems[index] }); try { - let result = await new Promise((resolve, reject) => { + const result = await new Promise((resolve, reject) => { const abortHandler = () => { reject(new TimeoutError(this.taskTimeout)); }; @@ -348,17 +347,23 @@ export class JsTaskRunner extends TaskRunner { continue; } - result = validateRunForEachItemOutput(result, index); if (result) { + let jsonData; + if (isObject(result) && 'json' in result) { + jsonData = result.json; + } else { + jsonData = result; + } + returnData.push( result.binary ? { - json: result.json, + json: jsonData, pairedItem: { item: index }, binary: result.binary, } : { - json: result.json, + json: jsonData, pairedItem: { item: index }, }, ); diff --git a/packages/@n8n/task-runner/src/js-task-runner/result-validation.ts b/packages/@n8n/task-runner/src/js-task-runner/result-validation.ts deleted file mode 100644 index dd5378a916..0000000000 --- a/packages/@n8n/task-runner/src/js-task-runner/result-validation.ts +++ /dev/null @@ -1,148 +0,0 @@ -import { normalizeItems } from 'n8n-core'; -import type { INodeExecutionData } from 'n8n-workflow'; - -import { ReservedKeyFoundError } from './errors/reserved-key-not-found.error'; -import { ValidationError } from './errors/validation-error'; -import { isObject } from './obj-utils'; - -export const REQUIRED_N8N_ITEM_KEYS = new Set([ - 'json', - 'binary', - 'pairedItem', - 'error', - - /** - * The `index` key was added accidentally to Function, FunctionItem, Gong, - * Execute Workflow, and ToolWorkflowV2, so we need to allow it temporarily. - * Once we stop using it in all nodes, we can stop allowing the `index` key. - */ - 'index', -]); - -function validateTopLevelKeys(item: INodeExecutionData, itemIndex: number) { - let foundReservedKey: string | null = null; - const unknownKeys: string[] = []; - - for (const key in item) { - if (!Object.prototype.hasOwnProperty.call(item, key)) continue; - - if (REQUIRED_N8N_ITEM_KEYS.has(key)) { - foundReservedKey ??= key; - } else { - unknownKeys.push(key); - } - } - - if (unknownKeys.length > 0) { - if (foundReservedKey) throw new ReservedKeyFoundError(foundReservedKey, itemIndex); - - throw new ValidationError({ - message: `Unknown top-level item key: ${unknownKeys[0]}`, - description: 'Access the properties of an item under `.json`, e.g. `item.json`', - itemIndex, - }); - } -} - -function validateItem({ json, binary }: INodeExecutionData, itemIndex: number) { - 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, - }); - } - - 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, - }); - } -} - -export class NonArrayOfObjectsError extends ValidationError { - constructor() { - super({ - message: "Code doesn't return items properly", - description: 'Please return an array of objects, one for each item you would like to output.', - }); - } -} - -/** - * Validates the output of a code node in 'Run for All Items' mode. - */ -export function validateRunForAllItemsOutput( - executionResult: INodeExecutionData | INodeExecutionData[] | undefined, -) { - if (Array.isArray(executionResult)) { - for (const item of executionResult) { - if (!isObject(item)) throw new NonArrayOfObjectsError(); - } - - /** - * If at least one top-level key is an n8n item key (`json`, `binary`, etc.), - * then require all item keys to be an n8n item key. - * - * If no top-level key is an n8n key, then skip this check, allowing non-n8n - * item keys to be wrapped in `json` when normalizing items below. - */ - const mustHaveTopLevelN8nKey = executionResult.some((item) => - Object.keys(item).find((key) => REQUIRED_N8N_ITEM_KEYS.has(key)), - ); - - if (mustHaveTopLevelN8nKey) { - for (let index = 0; index < executionResult.length; index++) { - const item = executionResult[index]; - validateTopLevelKeys(item, index); - } - } - } else if (!isObject(executionResult)) { - throw new NonArrayOfObjectsError(); - } - - const returnData = normalizeItems(executionResult); - returnData.forEach(validateItem); - return returnData; -} - -/** - * Validates the output of a code node in 'Run for Each Item' mode for single item - */ -export function validateRunForEachItemOutput( - executionResult: INodeExecutionData | undefined, - itemIndex: number, -) { - if (typeof executionResult !== 'object') { - throw new ValidationError({ - message: "Code doesn't return an object", - description: `Please return an object representing the output item. ('${executionResult}' was returned instead.)`, - 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, - }); - } - - const [returnData] = normalizeItems([executionResult]); - - validateItem(returnData, 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 - validateTopLevelKeys(returnData, itemIndex); - - return returnData; -} diff --git a/packages/@n8n/task-runner/src/runner-types.ts b/packages/@n8n/task-runner/src/runner-types.ts index 7b13e8fe70..a3e3d7f1cd 100644 --- a/packages/@n8n/task-runner/src/runner-types.ts +++ b/packages/@n8n/task-runner/src/runner-types.ts @@ -59,7 +59,8 @@ export interface DataRequestResponse { } export interface TaskResultData { - result: INodeExecutionData[]; + /** Raw user output, i.e. not yet validated or normalized. */ + result: unknown; customData?: Record; staticData?: IDataObject; } diff --git a/packages/cli/test/integration/task-runners/js-task-runner-execution.integration.test.ts b/packages/cli/test/integration/task-runners/js-task-runner-execution.integration.test.ts index 582c29e844..20b04f52c2 100644 --- a/packages/cli/test/integration/task-runners/js-task-runner-execution.integration.test.ts +++ b/packages/cli/test/integration/task-runners/js-task-runner-execution.integration.test.ts @@ -185,12 +185,12 @@ describe('JS TaskRunner execution on internal mode', () => { it('should execute a simple JS task', async () => { // Act - const result = await runTaskWithCode('return [{ hello: "world" }]'); + const result = await runTaskWithCode('return { hello: "world" }'); // Assert expect(result).toEqual({ ok: true, - result: [{ json: { hello: 'world' } }], + result: { hello: 'world' }, }); }); }); @@ -210,17 +210,17 @@ describe('JS TaskRunner execution on internal mode', () => { // Act const result = await runTaskWithCode(` const crypto = require("crypto"); - return [{ + return { digest: crypto .createHmac("sha256", Buffer.from("MySecretKey")) .update("MESSAGE") .digest("base64") - }] + } `); expect(result).toEqual({ ok: true, - result: [{ json: { digest: 'T09DMv7upNDKMD3Ht36FkwzrmWSgWpPiUNlcIX9/yaI=' } }], + result: { digest: 'T09DMv7upNDKMD3Ht36FkwzrmWSgWpPiUNlcIX9/yaI=' }, }); }); @@ -228,7 +228,7 @@ describe('JS TaskRunner execution on internal mode', () => { // Act const result = await runTaskWithCode(` const fs = require("fs"); - return [{ file: fs.readFileSync("test.txt") }] + return { file: fs.readFileSync("test.txt") } `); expect(result).toEqual({ @@ -243,12 +243,12 @@ describe('JS TaskRunner execution on internal mode', () => { // Act const result = await runTaskWithCode(` const moment = require("moment"); - return [{ time: moment("1995-12-25").format("YYYY-MM-DD") }] + return { time: moment("1995-12-25").format("YYYY-MM-DD") } `); expect(result).toEqual({ ok: true, - result: [{ json: { time: '1995-12-25' } }], + result: { time: '1995-12-25' }, }); }); diff --git a/packages/nodes-base/nodes/Code/JsTaskRunnerSandbox.ts b/packages/nodes-base/nodes/Code/JsTaskRunnerSandbox.ts index 4f0419844f..ca6acb582a 100644 --- a/packages/nodes-base/nodes/Code/JsTaskRunnerSandbox.ts +++ b/packages/nodes-base/nodes/Code/JsTaskRunnerSandbox.ts @@ -6,8 +6,14 @@ import { } from 'n8n-workflow'; import { validateNoDisallowedMethodsInRunForEach } from './JsCodeValidator'; +import type { TextKeys } from './result-validation'; +import { validateRunCodeAllItems, validateRunCodeEachItem } from './result-validation'; import { throwExecutionError } from './throw-execution-error'; +const JS_TEXT_KEYS: TextKeys = { + object: { singular: 'object', plural: 'objects' }, +}; + /** * JS Code execution sandbox that executes the JS code using task runner. */ @@ -34,9 +40,15 @@ export class JsTaskRunnerSandbox { itemIndex, ); - return executionResult.ok - ? executionResult.result - : throwExecutionError('error' in executionResult ? executionResult.error : {}); + if (!executionResult.ok) { + throwExecutionError('error' in executionResult ? executionResult.error : {}); + } + + return validateRunCodeAllItems( + executionResult.result, + JS_TEXT_KEYS, + this.executeFunctions.helpers.normalizeItems.bind(this.executeFunctions.helpers), + ); } async runCodeForEachItem(numInputItems: number): Promise { @@ -66,6 +78,17 @@ export class JsTaskRunnerSandbox { return throwExecutionError('error' in executionResult ? executionResult.error : {}); } + for (let i = 0; i < executionResult.result.length; i++) { + const actualItemIndex = chunk.startIdx + i; + const validatedItem = validateRunCodeEachItem( + executionResult.result[i], + actualItemIndex, + JS_TEXT_KEYS, + this.executeFunctions.helpers.normalizeItems.bind(this.executeFunctions.helpers), + ); + executionResult.result[i] = validatedItem; + } + executionResults = executionResults.concat(executionResult.result); } diff --git a/packages/nodes-base/nodes/Code/Sandbox.ts b/packages/nodes-base/nodes/Code/Sandbox.ts index 0cfa4fb534..5a6f481c0f 100644 --- a/packages/nodes-base/nodes/Code/Sandbox.ts +++ b/packages/nodes-base/nodes/Code/Sandbox.ts @@ -6,8 +6,7 @@ import type { IWorkflowDataProxyData, } from 'n8n-workflow'; -import { isObject } from './utils'; -import { ValidationError } from './ValidationError'; +import { validateRunCodeAllItems, validateRunCodeEachItem } from './result-validation'; interface SandboxTextKeys { object: { @@ -22,20 +21,6 @@ export interface SandboxContext extends IWorkflowDataProxyData { helpers: IExecuteFunctions['helpers']; } -export const REQUIRED_N8N_ITEM_KEYS = new Set([ - 'json', - 'binary', - 'pairedItem', - 'error', - - /** - * The `index` key was added accidentally to Function, FunctionItem, Gong, - * Execute Workflow, and ToolWorkflowV2, so we need to allow it temporarily. - * Once we stop using it in all nodes, we can stop allowing the `index` key. - */ - 'index', -]); - export function getSandboxContext( this: IExecuteFunctions | ISupplyDataFunctions, index: number, @@ -75,147 +60,21 @@ export abstract class Sandbox extends EventEmitter { executionResult: INodeExecutionData | undefined, itemIndex: number, ): INodeExecutionData { - if (typeof executionResult !== 'object') { - throw new ValidationError({ - message: `Code doesn't return ${this.getTextKey('object', { includeArticle: true })}`, - description: `Please return ${this.getTextKey('object', { - includeArticle: true, - })} representing the output item. ('${executionResult}' was returned instead.)`, - 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 ${this.getTextKey('object')}`, - description: `${firstSentence} If you need to output multiple items, please use the 'Run Once for All Items' mode instead.`, - itemIndex, - }); - } - - const [returnData] = this.helpers.normalizeItems([executionResult]); - - this.validateItem(returnData, 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 - this.validateTopLevelKeys(returnData, itemIndex); - - return returnData; + return validateRunCodeEachItem( + executionResult, + itemIndex, + this.textKeys, + this.helpers.normalizeItems.bind(this.helpers), + ); } validateRunCodeAllItems( executionResult: INodeExecutionData | INodeExecutionData[] | undefined, ): INodeExecutionData[] { - if (typeof executionResult !== 'object') { - throw new ValidationError({ - message: "Code doesn't return items properly", - description: `Please return an array of ${this.getTextKey('object', { - plural: true, - })}, one for each item you would like to output.`, - }); - } - - if (Array.isArray(executionResult)) { - /** - * If at least one top-level key is an n8n item key (`json`, `binary`, etc.), - * then require all item keys to be an n8n item key. - * - * If no top-level key is an n8n key, then skip this check, allowing non-n8n - * item keys to be wrapped in `json` when normalizing items below. - */ - const mustHaveTopLevelN8nKey = executionResult.some((item) => - Object.keys(item).find((key) => REQUIRED_N8N_ITEM_KEYS.has(key)), - ); - - if (mustHaveTopLevelN8nKey) { - for (let index = 0; index < executionResult.length; index++) { - const item = executionResult[index]; - this.validateTopLevelKeys(item, index); - } - } - } - - const returnData = this.helpers.normalizeItems(executionResult); - returnData.forEach((item, index) => this.validateItem(item, index)); - return returnData; - } - - private getTextKey( - key: keyof SandboxTextKeys, - options?: { includeArticle?: boolean; plural?: boolean }, - ) { - const response = this.textKeys[key][options?.plural ? 'plural' : 'singular']; - if (!options?.includeArticle) { - return response; - } - if (['a', 'e', 'i', 'o', 'u'].some((value) => response.startsWith(value))) { - return `an ${response}`; - } - return `a ${response}`; - } - - private validateItem({ json, binary }: INodeExecutionData, itemIndex: number) { - if (json === undefined || !isObject(json)) { - throw new ValidationError({ - message: `A 'json' property isn't ${this.getTextKey('object', { includeArticle: true })}`, - description: `In the returned data, every key named 'json' must point to ${this.getTextKey( - 'object', - { includeArticle: true }, - )}.`, - itemIndex, - }); - } - - if (binary !== undefined && !isObject(binary)) { - throw new ValidationError({ - message: `A 'binary' property isn't ${this.getTextKey('object', { includeArticle: true })}`, - description: `In the returned data, every key named 'binary’ must point to ${this.getTextKey( - 'object', - { includeArticle: true }, - )}.`, - itemIndex, - }); - } - } - - private validateTopLevelKeys(item: INodeExecutionData, itemIndex: number) { - let foundReservedKey: string | null = null; - const unknownKeys: string[] = []; - - for (const key in item) { - if (!Object.prototype.hasOwnProperty.call(item, key)) continue; - - if (REQUIRED_N8N_ITEM_KEYS.has(key)) { - foundReservedKey ??= key; - } else { - unknownKeys.push(key); - } - } - - if (unknownKeys.length > 0) { - if (foundReservedKey) throw new ReservedKeyFoundError(foundReservedKey, itemIndex); - - throw new ValidationError({ - message: `Unknown top-level item key: ${unknownKeys[0]}`, - description: 'Access the properties of an item under `.json`, e.g. `item.json`', - itemIndex, - }); - } - } -} - -class ReservedKeyFoundError extends ValidationError { - constructor(reservedKey: string, itemIndex: number) { - super({ - message: 'Invalid output format', - description: `An output item contains the reserved key ${reservedKey}. To get around this, please wrap each item in an object, under a key called json. Example`, - itemIndex, - }); + return validateRunCodeAllItems( + executionResult, + this.textKeys, + this.helpers.normalizeItems.bind(this.helpers), + ); } } diff --git a/packages/@n8n/task-runner/src/js-task-runner/errors/reserved-key-not-found.error.ts b/packages/nodes-base/nodes/Code/reserved-key-found-error.ts similarity index 89% rename from packages/@n8n/task-runner/src/js-task-runner/errors/reserved-key-not-found.error.ts rename to packages/nodes-base/nodes/Code/reserved-key-found-error.ts index 2e32466b85..50e8092d95 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/errors/reserved-key-not-found.error.ts +++ b/packages/nodes-base/nodes/Code/reserved-key-found-error.ts @@ -1,4 +1,4 @@ -import { ValidationError } from './validation-error'; +import { ValidationError } from './ValidationError'; export class ReservedKeyFoundError extends ValidationError { constructor(reservedKey: string, itemIndex: number) { diff --git a/packages/nodes-base/nodes/Code/result-validation.ts b/packages/nodes-base/nodes/Code/result-validation.ts new file mode 100644 index 0000000000..f131fea580 --- /dev/null +++ b/packages/nodes-base/nodes/Code/result-validation.ts @@ -0,0 +1,174 @@ +import type { INodeExecutionData } from 'n8n-workflow'; + +import { ReservedKeyFoundError } from './reserved-key-found-error'; +import { isObject } from './utils'; +import { ValidationError } from './ValidationError'; + +export interface TextKeys { + object: { + singular: string; + plural: string; + }; +} + +export const REQUIRED_N8N_ITEM_KEYS = new Set(['json', 'binary', 'pairedItem', 'error', 'index']); + +export function getTextKey( + textKeys: TextKeys, + key: keyof TextKeys, + options?: { includeArticle?: boolean; plural?: boolean }, +) { + const response = textKeys[key][options?.plural ? 'plural' : 'singular']; + if (!options?.includeArticle) { + return response; + } + if (['a', 'e', 'i', 'o', 'u'].some((value) => response.startsWith(value))) { + return `an ${response}`; + } + return `a ${response}`; +} + +export function validateItem( + { json, binary }: INodeExecutionData, + itemIndex: number, + textKeys: TextKeys, +) { + if (json === undefined || !isObject(json)) { + throw new ValidationError({ + message: `A 'json' property isn't ${getTextKey(textKeys, 'object', { includeArticle: true })}`, + description: `In the returned data, every key named 'json' must point to ${getTextKey( + textKeys, + 'object', + { includeArticle: true }, + )}.`, + itemIndex, + }); + } + + if (binary !== undefined && !isObject(binary)) { + throw new ValidationError({ + message: `A 'binary' property isn't ${getTextKey(textKeys, 'object', { includeArticle: true })}`, + description: `In the returned data, every key named 'binary' must point to ${getTextKey( + textKeys, + 'object', + { includeArticle: true }, + )}.`, + itemIndex, + }); + } +} + +export function validateTopLevelKeys(item: INodeExecutionData, itemIndex: number) { + let foundReservedKey: string | null = null; + const unknownKeys: string[] = []; + + for (const key in item) { + if (!Object.prototype.hasOwnProperty.call(item, key)) continue; + + if (REQUIRED_N8N_ITEM_KEYS.has(key)) { + foundReservedKey ??= key; + } else { + unknownKeys.push(key); + } + } + + if (unknownKeys.length > 0) { + if (foundReservedKey) throw new ReservedKeyFoundError(foundReservedKey, itemIndex); + + throw new ValidationError({ + message: `Unknown top-level item key: ${unknownKeys[0]}`, + description: 'Access the properties of an item under `.json`, e.g. `item.json`', + itemIndex, + }); + } +} + +export function validateRunCodeEachItem( + executionResult: INodeExecutionData | undefined, + itemIndex: number, + textKeys: TextKeys, + normalizeItems: (items: INodeExecutionData[]) => INodeExecutionData[], +): INodeExecutionData { + if (typeof executionResult !== 'object') { + throw new ValidationError({ + message: `Code doesn't return ${getTextKey(textKeys, 'object', { includeArticle: true })}`, + description: `Please return ${getTextKey(textKeys, 'object', { + includeArticle: true, + })} representing the output item. ('${executionResult}' was returned instead.)`, + 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 ${getTextKey(textKeys, 'object')}`, + description: `${firstSentence} If you need to output multiple items, please use the 'Run Once for All Items' mode instead.`, + itemIndex, + }); + } + + const [returnData] = normalizeItems([executionResult]); + + validateItem(returnData, itemIndex, textKeys); + + // 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 + validateTopLevelKeys(returnData, itemIndex); + + return returnData; +} + +export function validateRunCodeAllItems( + executionResult: INodeExecutionData | INodeExecutionData[] | undefined, + textKeys: TextKeys, + normalizeItems: (items: INodeExecutionData | INodeExecutionData[]) => INodeExecutionData[], +): INodeExecutionData[] { + if (typeof executionResult !== 'object') { + throw new ValidationError({ + message: "Code doesn't return items properly", + description: `Please return an array of ${getTextKey(textKeys, 'object', { + plural: true, + })}, one for each item you would like to output.`, + }); + } + + if (Array.isArray(executionResult)) { + /** + * If at least one top-level key is an n8n item key (`json`, `binary`, etc.), + * then require all item keys to be an n8n item key. + * + * If no top-level key is an n8n key, then skip this check, allowing non-n8n + * item keys to be wrapped in `json` when normalizing items below. + */ + for (const item of executionResult) { + if (!isObject(item)) { + throw new ValidationError({ + message: "Code doesn't return items properly", + description: `Please return an array of ${getTextKey(textKeys, 'object', { + plural: true, + })}, one for each item you would like to output.`, + }); + } + } + + const mustHaveTopLevelN8nKey = executionResult.some((item) => + Object.keys(item).find((key) => REQUIRED_N8N_ITEM_KEYS.has(key)), + ); + + if (mustHaveTopLevelN8nKey) { + for (let index = 0; index < executionResult.length; index++) { + const item = executionResult[index]; + validateTopLevelKeys(item, index); + } + } + } + + const returnData = normalizeItems(executionResult); + returnData.forEach((item, index) => validateItem(item, index, textKeys)); + return returnData; +} diff --git a/packages/nodes-base/nodes/Code/test/JsTaskRunnerSandbox.test.ts b/packages/nodes-base/nodes/Code/test/JsTaskRunnerSandbox.test.ts index 00eb17544a..741fbfb464 100644 --- a/packages/nodes-base/nodes/Code/test/JsTaskRunnerSandbox.test.ts +++ b/packages/nodes-base/nodes/Code/test/JsTaskRunnerSandbox.test.ts @@ -11,6 +11,13 @@ describe('JsTaskRunnerSandbox', () => { const nodeMode = 'runOnceForEachItem'; const workflowMode = 'manual'; const executeFunctions = mock(); + executeFunctions.helpers = { + ...executeFunctions.helpers, + normalizeItems: jest + .fn() + .mockImplementation((items) => (Array.isArray(items) ? items : [items])), + }; + const sandbox = new JsTaskRunnerSandbox(jsCode, nodeMode, workflowMode, executeFunctions, 2); let i = 1; executeFunctions.startJob.mockResolvedValue(createResultOk([{ json: { item: i++ } }]));