From 33991e92d0aabd13a44ba103de43a6ec2b90ca46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Mon, 25 Sep 2023 12:30:28 +0200 Subject: [PATCH] fix(core): Fix missing execution ID in webhook-based workflow producing binary data (#7244) Story: https://linear.app/n8n/issue/PAY-839 This is a longstanding bug, fixed now so that the S3 backend for binary data can use execution IDs as part of the filename. To reproduce: 1. Set up a workflow with a POST Webhook node that accepts binary data. 2. Activate the workflow and call it sending a binary file, e.g. `curl -X POST -F "file=@/path/to/binary/file/test.jpg" http://localhost:5678/webhook/uuid` 3. Check `~/.n8n/binaryData`. The binary data and metadata files will be missing the execution ID, e.g. `11869055-83c4-4493-876a-9092c4708b9b` instead of `39011869055-83c4-4493-876a-9092c4708b9b`. --- .../cli/src/WorkflowExecuteAdditionalData.ts | 5 ++ .../restoreBinaryDataId.ts | 43 +++++++++ .../cli/test/unit/execution.lifecycle.test.ts | 87 +++++++++++++++++++ .../core/src/BinaryData/BinaryData.service.ts | 8 ++ .../core/src/BinaryData/FileSystem.manager.ts | 11 +++ packages/core/src/BinaryData/types.ts | 2 + 6 files changed, 156 insertions(+) create mode 100644 packages/cli/src/executionLifecycleHooks/restoreBinaryDataId.ts create mode 100644 packages/cli/test/unit/execution.lifecycle.test.ts diff --git a/packages/cli/src/WorkflowExecuteAdditionalData.ts b/packages/cli/src/WorkflowExecuteAdditionalData.ts index 08bfd71031c8e..5c94de7f12438 100644 --- a/packages/cli/src/WorkflowExecuteAdditionalData.ts +++ b/packages/cli/src/WorkflowExecuteAdditionalData.ts @@ -65,6 +65,7 @@ import { prepareExecutionDataForDbUpdate, updateExistingExecution, } from './executionLifecycleHooks/shared/sharedHookFunctions'; +import { restoreBinaryDataId } from './executionLifecycleHooks/restoreBinaryDataId'; const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType'); @@ -446,6 +447,10 @@ function hookFunctionsSave(parentProcessMode?: string): IWorkflowExecuteHooks { workflowId: this.workflowData.id, }); + if (this.mode === 'webhook' && config.getEnv('binaryDataManager.mode') === 'filesystem') { + await restoreBinaryDataId(fullRunData, this.executionId); + } + const isManualMode = [this.mode, parentProcessMode].includes('manual'); try { diff --git a/packages/cli/src/executionLifecycleHooks/restoreBinaryDataId.ts b/packages/cli/src/executionLifecycleHooks/restoreBinaryDataId.ts new file mode 100644 index 0000000000000..95682494d5c09 --- /dev/null +++ b/packages/cli/src/executionLifecycleHooks/restoreBinaryDataId.ts @@ -0,0 +1,43 @@ +import Container from 'typedi'; +import { BinaryDataService } from 'n8n-core'; +import type { IRun } from 'n8n-workflow'; + +export function isMissingExecutionId(binaryDataId: string) { + const UUID_CHAR_LENGTH = 36; + + return [UUID_CHAR_LENGTH + 'filesystem:'.length, UUID_CHAR_LENGTH + 's3:'.length].some( + (incorrectLength) => binaryDataId.length === incorrectLength, + ); +} + +/** + * Whenever the execution ID is not available to the binary data service at the + * time of writing a binary data file, its name is missing the execution ID. + * + * This function restores the ID in the file name and run data reference. + * + * ```txt + * filesystem:11869055-83c4-4493-876a-9092c4708b9b -> + * filesystem:39011869055-83c4-4493-876a-9092c4708b9b + * ``` + */ +export async function restoreBinaryDataId(run: IRun, executionId: string) { + const { runData } = run.data.resultData; + + const promises = Object.keys(runData).map(async (nodeName) => { + const binaryDataId = runData[nodeName]?.[0]?.data?.main?.[0]?.[0].binary?.data.id; + + if (!binaryDataId || !isMissingExecutionId(binaryDataId)) return; + + const [mode, incorrectFileId] = binaryDataId.split(':'); + const correctFileId = `${executionId}${incorrectFileId}`; + const correctBinaryDataId = `${mode}:${correctFileId}`; + + await Container.get(BinaryDataService).rename(incorrectFileId, correctFileId); + + // @ts-expect-error Validated at the top + run.data.resultData.runData[nodeName][0].data.main[0][0].binary.data.id = correctBinaryDataId; + }); + + await Promise.all(promises); +} diff --git a/packages/cli/test/unit/execution.lifecycle.test.ts b/packages/cli/test/unit/execution.lifecycle.test.ts new file mode 100644 index 0000000000000..093bb3d286bd5 --- /dev/null +++ b/packages/cli/test/unit/execution.lifecycle.test.ts @@ -0,0 +1,87 @@ +import { restoreBinaryDataId } from '@/executionLifecycleHooks/restoreBinaryDataId'; +import { BinaryDataService } from 'n8n-core'; +import { mockInstance } from '../integration/shared/utils/mocking'; +import type { IRun } from 'n8n-workflow'; + +function toIRun(item: object) { + return { + data: { + resultData: { + runData: { + myNode: [ + { + data: { + main: [[item]], + }, + }, + ], + }, + }, + }, + } as unknown as IRun; +} + +function getDataId(run: IRun, kind: 'binary' | 'json') { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + return run.data.resultData.runData.myNode[0].data.main[0][0][kind].data.id; +} + +describe('restoreBinaryDataId()', () => { + const binaryDataService = mockInstance(BinaryDataService); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should restore if binary data ID is missing execution ID', async () => { + const executionId = '999'; + const incorrectFileId = 'a5c3f1ed-9d59-4155-bc68-9a370b3c51f6'; + const run = toIRun({ + binary: { + data: { id: `filesystem:${incorrectFileId}` }, + }, + }); + + await restoreBinaryDataId(run, executionId); + + const correctFileId = `${executionId}${incorrectFileId}`; + const correctBinaryDataId = `filesystem:${correctFileId}`; + + expect(binaryDataService.rename).toHaveBeenCalledWith(incorrectFileId, correctFileId); + expect(getDataId(run, 'binary')).toBe(correctBinaryDataId); + }); + + it('should do nothing if binary data ID is not missing execution ID', async () => { + const executionId = '999'; + const fileId = `${executionId}a5c3f1ed-9d59-4155-bc68-9a370b3c51f6`; + const binaryDataId = `filesystem:${fileId}`; + const run = toIRun({ + binary: { + data: { + id: binaryDataId, + }, + }, + }); + + await restoreBinaryDataId(run, executionId); + + expect(binaryDataService.rename).not.toHaveBeenCalled(); + expect(getDataId(run, 'binary')).toBe(binaryDataId); + }); + + it('should do nothing if no binary data ID', async () => { + const executionId = '999'; + const dataId = '123'; + const run = toIRun({ + json: { + data: { id: dataId }, + }, + }); + + await restoreBinaryDataId(run, executionId); + + expect(binaryDataService.rename).not.toHaveBeenCalled(); + expect(getDataId(run, 'json')).toBe(dataId); + }); +}); diff --git a/packages/core/src/BinaryData/BinaryData.service.ts b/packages/core/src/BinaryData/BinaryData.service.ts index 8b7d77b68b3ce..3c0d9dfee3718 100644 --- a/packages/core/src/BinaryData/BinaryData.service.ts +++ b/packages/core/src/BinaryData/BinaryData.service.ts @@ -154,6 +154,14 @@ export class BinaryDataService { return inputData as INodeExecutionData[][]; } + async rename(oldFileId: string, newFileId: string) { + const manager = this.getManager(this.mode); + + if (!manager) return; + + await manager.rename(oldFileId, newFileId); + } + // ---------------------------------- // private methods // ---------------------------------- diff --git a/packages/core/src/BinaryData/FileSystem.manager.ts b/packages/core/src/BinaryData/FileSystem.manager.ts index 7667fd9f80d34..33c2d7d39d6fb 100644 --- a/packages/core/src/BinaryData/FileSystem.manager.ts +++ b/packages/core/src/BinaryData/FileSystem.manager.ts @@ -3,6 +3,7 @@ import fs from 'fs/promises'; import path from 'path'; import { v4 as uuid } from 'uuid'; import { jsonParse } from 'n8n-workflow'; +import { rename } from 'node:fs/promises'; import { FileNotFoundError } from '../errors'; import { ensureDirExists } from './utils'; @@ -124,6 +125,16 @@ export class FileSystemManager implements BinaryData.Manager { return newFileId; } + async rename(oldFileId: string, newFileId: string) { + const oldPath = this.getPath(oldFileId); + const newPath = this.getPath(newFileId); + + await Promise.all([ + rename(oldPath, newPath), + rename(`${oldPath}.metadata`, `${newPath}.metadata`), + ]); + } + // ---------------------------------- // private methods // ---------------------------------- diff --git a/packages/core/src/BinaryData/types.ts b/packages/core/src/BinaryData/types.ts index 368e3717d40cc..f0edbdfb71eee 100644 --- a/packages/core/src/BinaryData/types.ts +++ b/packages/core/src/BinaryData/types.ts @@ -47,5 +47,7 @@ export namespace BinaryData { // @TODO: Refactor to also receive `workflowId` to support full path-like identifier: // `workflows/{workflowId}/executions/{executionId}/binary_data/{fileId}` deleteManyByExecutionIds(executionIds: string[]): Promise; + + rename(oldFileId: string, newFileId: string): Promise; } }