From 6d42fad31a8929858bee71c95a2035e319cf0c55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 3 Nov 2023 11:41:15 +0100 Subject: [PATCH] refactor(core): Consolidate binary file not found errors (no-changelog) (#7585) Logging was originally to see if there was a binary data file failing to be written for [this user](https://linear.app/n8n/issue/PAY-844/filesystem-binary-data-mode-causing-alerts-in-cloud) but the cause was not a file failed to be written but a missing `fileId` in a binary data item in an execution. The error should no longer be thrown as of 1.12. See story for more info. This PR is for cleanup and to consolidate any file not found errors in the context of binary data, to track if this happens again. --- .../core/src/BinaryData/BinaryData.service.ts | 10 ++----- .../core/src/BinaryData/FileSystem.manager.ts | 20 ++++++++----- packages/core/src/BinaryData/utils.ts | 9 ++++++ .../core/src/decorators/LogCatch.decorator.ts | 29 ------------------- packages/core/src/errors.ts | 8 +++++ packages/core/test/FileSystem.manager.test.ts | 8 +++++ 6 files changed, 39 insertions(+), 45 deletions(-) delete mode 100644 packages/core/src/decorators/LogCatch.decorator.ts diff --git a/packages/core/src/BinaryData/BinaryData.service.ts b/packages/core/src/BinaryData/BinaryData.service.ts index 4aec461ff3..b3377f8181 100644 --- a/packages/core/src/BinaryData/BinaryData.service.ts +++ b/packages/core/src/BinaryData/BinaryData.service.ts @@ -1,14 +1,13 @@ import { readFile, stat } from 'node:fs/promises'; import prettyBytes from 'pretty-bytes'; import Container, { Service } from 'typedi'; -import { BINARY_ENCODING, LoggerProxy as Logger, IBinaryData } from 'n8n-workflow'; +import { BINARY_ENCODING } from 'n8n-workflow'; import { UnknownManagerError, InvalidModeError } from './errors'; import { areConfigModes, toBuffer } from './utils'; -import { LogCatch } from '../decorators/LogCatch.decorator'; import type { Readable } from 'stream'; import type { BinaryData } from './types'; -import type { INodeExecutionData } from 'n8n-workflow'; +import type { INodeExecutionData, IBinaryData } from 'n8n-workflow'; @Service() export class BinaryDataService { @@ -40,7 +39,6 @@ export class BinaryDataService { } } - @LogCatch((error) => Logger.error('Failed to copy binary data file', { error })) async copyBinaryFile( workflowId: string, executionId: string, @@ -76,7 +74,6 @@ export class BinaryDataService { return binaryData; } - @LogCatch((error) => Logger.error('Failed to write binary data file', { error })) async store( workflowId: string, executionId: string, @@ -152,9 +149,6 @@ export class BinaryDataService { if (manager.deleteMany) await manager.deleteMany(ids); } - @LogCatch((error) => - Logger.error('Failed to copy all binary data files for execution', { error }), - ) async duplicateBinaryData( workflowId: string, executionId: string, diff --git a/packages/core/src/BinaryData/FileSystem.manager.ts b/packages/core/src/BinaryData/FileSystem.manager.ts index f210597266..23b26dee09 100644 --- a/packages/core/src/BinaryData/FileSystem.manager.ts +++ b/packages/core/src/BinaryData/FileSystem.manager.ts @@ -3,8 +3,8 @@ import fs from 'node:fs/promises'; import path from 'node:path'; import { v4 as uuid } from 'uuid'; import { jsonParse } from 'n8n-workflow'; -import { assertDir } from './utils'; -import { FileNotFoundError } from '../errors'; +import { assertDir, doesNotExist } from './utils'; +import { BinaryFileNotFoundError, InvalidPathError } from '../errors'; import type { Readable } from 'stream'; import type { BinaryData } from './types'; @@ -46,17 +46,21 @@ export class FileSystemManager implements BinaryData.Manager { async getAsStream(fileId: string, chunkSize?: number) { const filePath = this.resolvePath(fileId); + if (await doesNotExist(filePath)) { + throw new BinaryFileNotFoundError(filePath); + } + return createReadStream(filePath, { highWaterMark: chunkSize }); } async getAsBuffer(fileId: string) { const filePath = this.resolvePath(fileId); - try { - return await fs.readFile(filePath); - } catch { - throw new Error(`Error finding file: ${filePath}`); + if (await doesNotExist(filePath)) { + throw new BinaryFileNotFoundError(filePath); } + + return fs.readFile(filePath); } async getMetadata(fileId: string): Promise { @@ -167,7 +171,7 @@ export class FileSystemManager implements BinaryData.Manager { const returnPath = path.join(this.storagePath, ...args); if (path.relative(this.storagePath, returnPath).startsWith('..')) { - throw new FileNotFoundError('Invalid path detected'); + throw new InvalidPathError(returnPath); } return returnPath; @@ -186,7 +190,7 @@ export class FileSystemManager implements BinaryData.Manager { const stats = await fs.stat(filePath); return stats.size; } catch (error) { - throw new Error('Failed to find binary data file in filesystem', { cause: error }); + throw new BinaryFileNotFoundError(filePath); } } } diff --git a/packages/core/src/BinaryData/utils.ts b/packages/core/src/BinaryData/utils.ts index 14a9b45788..cea0c40af5 100644 --- a/packages/core/src/BinaryData/utils.ts +++ b/packages/core/src/BinaryData/utils.ts @@ -23,6 +23,15 @@ export async function assertDir(dir: string) { } } +export async function doesNotExist(dir: string) { + try { + await fs.access(dir); + return false; + } catch { + return true; + } +} + export async function toBuffer(body: Buffer | Readable) { return new Promise((resolve) => { if (Buffer.isBuffer(body)) resolve(body); diff --git a/packages/core/src/decorators/LogCatch.decorator.ts b/packages/core/src/decorators/LogCatch.decorator.ts deleted file mode 100644 index a5999c50ad..0000000000 --- a/packages/core/src/decorators/LogCatch.decorator.ts +++ /dev/null @@ -1,29 +0,0 @@ -/* eslint-disable @typescript-eslint/no-unsafe-call */ -/* eslint-disable @typescript-eslint/no-unsafe-member-access */ -/* eslint-disable @typescript-eslint/no-unsafe-assignment */ - -export const LogCatch = (logFn: (error: unknown) => void) => { - return (target: unknown, propertyKey: string, descriptor: PropertyDescriptor) => { - const originalMethod = descriptor.value; - - descriptor.value = function (...args: unknown[]) { - try { - const result: unknown = originalMethod.apply(this, args); - - if (result && result instanceof Promise) { - return result.catch((error: unknown) => { - logFn(error); - throw error; - }); - } - - return result; - } catch (error) { - logFn(error); - throw error; - } - }; - - return descriptor; - }; -}; diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts index c425675c89..a8fea81380 100644 --- a/packages/core/src/errors.ts +++ b/packages/core/src/errors.ts @@ -3,3 +3,11 @@ export class FileNotFoundError extends Error { super(`File not found: ${filePath}`); } } + +export class BinaryFileNotFoundError extends FileNotFoundError {} + +export class InvalidPathError extends Error { + constructor(readonly filePath: string) { + super(`Invalid path detected: ${filePath}`); + } +} diff --git a/packages/core/test/FileSystem.manager.test.ts b/packages/core/test/FileSystem.manager.test.ts index 039307a57d..7087242726 100644 --- a/packages/core/test/FileSystem.manager.test.ts +++ b/packages/core/test/FileSystem.manager.test.ts @@ -53,6 +53,7 @@ describe('getPath()', () => { describe('getAsBuffer()', () => { it('should return a buffer', async () => { fsp.readFile = jest.fn().mockResolvedValue(mockBuffer); + fsp.access = jest.fn().mockImplementation(async () => {}); const result = await fsManager.getAsBuffer(fileId); @@ -64,6 +65,7 @@ describe('getAsBuffer()', () => { describe('getAsStream()', () => { it('should return a stream', async () => { fs.createReadStream = jest.fn().mockReturnValue(mockStream); + fsp.access = jest.fn().mockImplementation(async () => {}); const stream = await fsManager.getAsStream(fileId); @@ -123,6 +125,7 @@ describe('copyByFilePath()', () => { const targetPath = toFullFilePath(otherFileId); fsp.cp = jest.fn().mockResolvedValue(undefined); + fsp.writeFile = jest.fn().mockResolvedValue(undefined); const result = await fsManager.copyByFilePath( workflowId, @@ -132,6 +135,11 @@ describe('copyByFilePath()', () => { ); expect(fsp.cp).toHaveBeenCalledWith(sourceFilePath, targetPath); + expect(fsp.writeFile).toHaveBeenCalledWith( + `${toFullFilePath(otherFileId)}.metadata`, + JSON.stringify({ ...metadata, fileSize: mockBuffer.length }), + { encoding: 'utf-8' }, + ); expect(result.fileSize).toBe(mockBuffer.length); }); });